New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 5 users

Issue metadata

Status: Fixed
Owner:
Closed: May 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 1
Type: Bug

Blocking:
issue 520765



Sign in to add a comment

WebView: disallow geolocation on insecure origins for apps targeting N and higher

Project Member Reported by timvolod...@chromium.org, Apr 14 2016

Issue description

In Chrome geolocation has been disabled on insecure origins
previously in crrev.com/1485973002.

We should do the same for WebView. To allow for a smooth transition and remain reasonably backwards compatible with older apps we aim to enable this behavior for apps targeting N and up for now.

 
Cc: miguelg@chromium.org torne@chromium.org
Components: Mobile>WebView
Labels: OS-Android
Owner: timvolod...@chromium.org
Status: Assigned (was: Untriaged)
Blocking: 520765
Labels: Merge-Request-51
Labels: -Pri-3 ReleaseBlock-Beta Pri-1
Cc: kerz@chromium.org

Comment 7 by tin...@google.com, Apr 15 2016

Labels: -Merge-Request-51 Merge-Approved-51 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M51 (branch: 2704)
Project Member

Comment 8 by bugdroid1@chromium.org, Apr 15 2016

Labels: -merge-approved-51 merge-merged-2704
The following revision refers to this bug:
  https://chrome-internal.googlesource.com/clank/internal/apps/+/8760c554224e26c10982b3b160a08c31a430c9e9

commit 8760c554224e26c10982b3b160a08c31a430c9e9
Author: Tim Volodine <timvolodine@chromium.org>
Date: Thu Apr 14 14:18:51 2016

Project Member

Comment 9 by bugdroid1@chromium.org, Apr 15 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d0e727309bff971ecd6e4809a2c73cb5520d6bc6

commit d0e727309bff971ecd6e4809a2c73cb5520d6bc6
Author: timvolodine <timvolodine@chromium.org>
Date: Fri Apr 15 15:49:14 2016

[WebView] Disallow geolocation on insecure origins for apps targeting N and higher.

Merge to M51.

In Chrome geolocation has been disabled on insecure origins
previously in crrev.com/1485973002.

For WebView we are enabling same behavior for apps that are
targeting N and higher versions of Android. This is to allow
for a smooth transition and remain reasonably backwards
compatible with older apps.

BUG= 603574 
TBR=torne@chromium.org
NOTRY=true
NOPRESUBMIT=true

Review URL: https://codereview.chromium.org/1882783002

Cr-Commit-Position: refs/heads/master@{#387299}
(cherry picked from commit 4f4086735aeb59b937e8216c923f0ea7c58a3602)

Review URL: https://codereview.chromium.org/1894563002

Cr-Commit-Position: refs/branch-heads/2704@{#75}
Cr-Branched-From: 6e53600def8f60d8c632fadc70d7c1939ccea347-refs/heads/master@{#386251}

[modify] https://crrev.com/d0e727309bff971ecd6e4809a2c73cb5520d6bc6/android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java
[modify] https://crrev.com/d0e727309bff971ecd6e4809a2c73cb5520d6bc6/android_webview/java/src/org/chromium/android_webview/AwSettings.java
[modify] https://crrev.com/d0e727309bff971ecd6e4809a2c73cb5520d6bc6/android_webview/javatests/src/org/chromium/android_webview/test/AwSettingsTest.java
[modify] https://crrev.com/d0e727309bff971ecd6e4809a2c73cb5520d6bc6/android_webview/javatests/src/org/chromium/android_webview/test/AwTestBase.java
[modify] https://crrev.com/d0e727309bff971ecd6e4809a2c73cb5520d6bc6/android_webview/javatests/src/org/chromium/android_webview/test/GeolocationTest.java
[modify] https://crrev.com/d0e727309bff971ecd6e4809a2c73cb5520d6bc6/android_webview/native/aw_settings.cc
[modify] https://crrev.com/d0e727309bff971ecd6e4809a2c73cb5520d6bc6/android_webview/test/shell/src/org/chromium/android_webview/shell/AwShellActivity.java

Cc: aluo@chromium.org
Hey Andrew, kerz@ wanted to double check this one with test.
Project Member

Comment 11 by bugdroid1@chromium.org, May 4 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f2ec5e07edc351ea4a8dda1fca6f78eb649b2530

commit f2ec5e07edc351ea4a8dda1fca6f78eb649b2530
Author: timvolodine <timvolodine@chromium.org>
Date: Wed May 04 12:50:13 2016

[WebViewShell] retarget the shell to N for testing.

Increase targetSdkVersion to 24 to be able to execute
specific manual tests on N, e.g. geolocation api on
insecure origins.

BUG= 603574 

Review-Url: https://codereview.chromium.org/1942273003
Cr-Commit-Position: refs/heads/master@{#391485}

[modify] https://crrev.com/f2ec5e07edc351ea4a8dda1fca6f78eb649b2530/android_webview/tools/system_webview_shell/apk/AndroidManifest.xml

testing this can be done with a recent build of system_webview_shell_apk (with the above patch) and the following link:
https://timvolodine.github.io/geolocation-test/geolocation-test.html

with and without the s in https.

looks like we don't automatically build the shell in pantheon, so attaching the apk just in case.
SystemWebViewShell.apk
96.7 KB Download

Comment 14 Deleted

Comment 15 Deleted

Comment 16 by aluo@chromium.org, May 4 2016

Verified insecure origin version of the test site is blocked while the secure origin is allowed on the test app.  For apps targeting pre-N, verified that insecure origin version of the all is not blocked.

Comment 17 by aluo@chromium.org, May 4 2016

There is an issue where in gps only mode, chrome and webview shell app is not able to get the location (no errors, just the callback does not fire).  This also happens on chrome 46, so not related to the insecure origins fix.
todays build May 6 attached
SystemWebViewShell.apk
96.6 KB Download
#17 should not be related AFAIK. is there any work reamining here?

Comment 20 by torne@chromium.org, May 19 2016

Don't think so?
Status: Fixed (was: Assigned)
nope

Comment 22 Deleted

timvolodine: I still see some bits which I think might be related to this:
https://cs.chromium.org/search/?q=getAllowGeolocationOnInsecureOrigins+package:%5Echromium$&type=cs

Can those references now be removed?
Don't think they can be removed because we still need need this setting, i.e. we still allow it for apps targeting M and below.

(https://cs.chromium.org/chromium/src/android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java?l=175)

Comment 25 by torne@chromium.org, Jun 21 2017

Yeah, currently we don't have a specific plan to entirely disallow this for older apps. If we wanted to remove this code entirely then we'd have to first figure out the impact: webview is just starting to collect UMA data so we can look there to see how often this is used, and we would also need to check whether older versions of CTS are broken by this.
Ah, thanks for explaining, that makes sense.

Sign in to add a comment