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
link

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

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

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.
 

Comment 1 by timvolod...@chromium.org, Apr 14 2016

Cc: miguelg@chromium.org torne@chromium.org
Components: Mobile>WebView
Labels: OS-Android
Owner: timvolod...@chromium.org
Status: Assigned (was: Untriaged)

Comment 2 by timvolod...@chromium.org, Apr 14 2016

Blocking: 520765

Comment 4 by timvolod...@chromium.org, Apr 14 2016

Labels: Merge-Request-51

Comment 5 by miguelg@chromium.org, Apr 15 2016

Labels: -Pri-3 ReleaseBlock-Beta Pri-1

Comment 6 by miguelg@chromium.org, Apr 15 2016

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)

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

Project Member
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

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

Project Member
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

Comment 10 by sbirch@chromium.org, Apr 22 2016

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

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

Project Member
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

Comment 12 by timvolod...@chromium.org, May 4 2016

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.

Comment 13 by timvolod...@chromium.org, May 4 2016

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.

Comment 18 by timvolod...@chromium.org, May 6 2016

todays build May 6 attached
SystemWebViewShell.apk
96.6 KB Download

Comment 19 by sgu...@chromium.org, May 19 2016

#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?

Comment 21 by timvolod...@chromium.org, May 19 2016

Status: Fixed (was: Assigned)
nope

Comment 22 Deleted

Comment 23 by raymes@chromium.org, Jun 21 2017

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?

Comment 24 by timvolod...@chromium.org, Jun 21 2017

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.

Comment 26 by raymes@chromium.org, Jun 26 2017

Ah, thanks for explaining, that makes sense.

Sign in to add a comment