New issue
Advanced search Search tips

Issue 898589 link

Starred by 5 users

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Bug



Sign in to add a comment

WebView apps' usage of unknown origins broken in CORS contexts

Project Member Reported by torne@chromium.org, Oct 24

Issue description

Various WebView apps use made-up origins in their URLs (e.g. foo://whatever, anything they want), and provide the content for these URLs by overriding WebView's shouldInterceptRequest callback to intercept the request before the network stack rejects it for having an unknown origin.

The recent CL https://chromium-review.googlesource.com/c/chromium/src/+/1158089 changed handling of unknown schemes in CORS requests (such as web fonts), probably unintentionally, and now WebView apps are broken if they try to load a web font from one of these unknown origins: shouldInterceptRequest is never called, because the CORS code appears to be rejecting the request before it gets that far.

The old behaviour seemed to simply permit these cross-origin requests if the app intercepted the load and provided valid content (there is no way for the app to explicitly handle CORS in the interception case), and so apps depend on this :|

Is it possible to restore the old behaviour in WebView temporarily while we try to work out what to do longer-term? The old behaviour is weird and probably insecure, so I'm not sure it's something we actually want to support forever, but the change is breaking apps with the M70 stable rollout and we may need to respin.
 
Internal bug b/118342051
Cc: dskiba@chromium.org
Owner: yhirano@chromium.org
Status: Assigned (was: Untriaged)
Assigning to yhirano to comment as author of the CL in question.
torne@, can you verify https://chromium-review.googlesource.com/c/chromium/src/+/1298828 and land it if it fixes the problem?
Testing this locally now.
Works for my trivial test app; I posted a test APK on the internal bug for Books to verify.
Project Member

Comment 8 by bugdroid1@chromium.org, Oct 25

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

commit 14314b070e1e92dcae7fe4ccfa4531c6a128060e
Author: Yutaka Hirano <yhirano@chromium.org>
Date: Thu Oct 25 16:23:47 2018

Remove CORS scheme check temporarily

The scheme check added at
https://crrev.com/43b8facd3a2fbfb8f75b455f07d64acc2c13fb5c caused some
issues on WebView. This change deletes the check temporarily.

Bug: 898589
Change-Id: I6126b3b25f8700fc0547fa2c70b62bd183cc5e23
Reviewed-on: https://chromium-review.googlesource.com/c/1298828
Reviewed-by: Takashi Toyoshima <toyoshim@chromium.org>
Commit-Queue: Richard Coles <torne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#602736}
[modify] https://crrev.com/14314b070e1e92dcae7fe4ccfa4531c6a128060e/third_party/blink/renderer/platform/loader/fetch/resource_fetcher.cc

Labels: Merge-Request-70 Merge-Request-71
Labels: Merge-Approved-71 Merge-Approved-70
Approved for merge to 70 & 71.
Project Member

Comment 11 by sheriffbot@chromium.org, Oct 25

Labels: -Merge-Request-71 Hotlist-Merge-Review Merge-Review-71
This bug requires manual review: M71 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: benmason@(Android), kariahda@(iOS), kbleicher@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 12 by bugdroid1@chromium.org, Oct 25

Labels: -merge-approved-71 merge-merged-3578
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cdc63f518463bd16116e562cb7bb23d6520cfe29

commit cdc63f518463bd16116e562cb7bb23d6520cfe29
Author: Torne (Richard Coles) <torne@google.com>
Date: Thu Oct 25 16:31:52 2018

Remove CORS scheme check temporarily

The scheme check added at
https://crrev.com/43b8facd3a2fbfb8f75b455f07d64acc2c13fb5c caused some
issues on WebView. This change deletes the check temporarily.

TBR=yhirano@chromium.org

(cherry picked from commit 14314b070e1e92dcae7fe4ccfa4531c6a128060e)

Bug: 898589
Change-Id: I6126b3b25f8700fc0547fa2c70b62bd183cc5e23
Reviewed-on: https://chromium-review.googlesource.com/c/1298828
Reviewed-by: Takashi Toyoshima <toyoshim@chromium.org>
Commit-Queue: Richard Coles <torne@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#602736}
Reviewed-on: https://chromium-review.googlesource.com/c/1299436
Reviewed-by: Richard Coles <torne@chromium.org>
Cr-Commit-Position: refs/branch-heads/3578@{#320}
Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034}
[modify] https://crrev.com/cdc63f518463bd16116e562cb7bb23d6520cfe29/third_party/blink/renderer/platform/loader/fetch/resource_fetcher.cc

Labels: Merge-Merged-71-3578
The following revision refers to this bug: 
https://chromium.googlesource.com/chromium/src.git/+/cdc63f518463bd16116e562cb7bb23d6520cfe29

Commit: cdc63f518463bd16116e562cb7bb23d6520cfe29
Author: torne@google.com
Commiter: torne@chromium.org
Date: 2018-10-25 16:31:52 +0000 UTC

Remove CORS scheme check temporarily

The scheme check added at
https://crrev.com/43b8facd3a2fbfb8f75b455f07d64acc2c13fb5c caused some
issues on WebView. This change deletes the check temporarily.

TBR=yhirano@chromium.org

(cherry picked from commit 14314b070e1e92dcae7fe4ccfa4531c6a128060e)

Bug: 898589
Change-Id: I6126b3b25f8700fc0547fa2c70b62bd183cc5e23
Reviewed-on: https://chromium-review.googlesource.com/c/1298828
Reviewed-by: Takashi Toyoshima <toyoshim@chromium.org>
Commit-Queue: Richard Coles <torne@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#602736}
Reviewed-on: https://chromium-review.googlesource.com/c/1299436
Reviewed-by: Richard Coles <torne@chromium.org>
Cr-Commit-Position: refs/branch-heads/3578@{#320}
Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034}
Project Member

Comment 14 by bugdroid1@chromium.org, Oct 25

Labels: -merge-approved-70 merge-merged-3538
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d7e12a8117080fa0b8feb550b35fffccf340e27b

commit d7e12a8117080fa0b8feb550b35fffccf340e27b
Author: Torne (Richard Coles) <torne@google.com>
Date: Thu Oct 25 16:40:57 2018

Remove CORS scheme check temporarily

The scheme check added at
https://crrev.com/43b8facd3a2fbfb8f75b455f07d64acc2c13fb5c caused some
issues on WebView. This change deletes the check temporarily.

TBR=yhirano@chromium.org

(cherry picked from commit 14314b070e1e92dcae7fe4ccfa4531c6a128060e)

Bug: 898589
Change-Id: I6126b3b25f8700fc0547fa2c70b62bd183cc5e23
Reviewed-on: https://chromium-review.googlesource.com/c/1298828
Reviewed-by: Takashi Toyoshima <toyoshim@chromium.org>
Commit-Queue: Richard Coles <torne@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#602736}
Reviewed-on: https://chromium-review.googlesource.com/c/1299439
Reviewed-by: Richard Coles <torne@chromium.org>
Cr-Commit-Position: refs/branch-heads/3538@{#1045}
Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811}
[modify] https://crrev.com/d7e12a8117080fa0b8feb550b35fffccf340e27b/third_party/blink/renderer/platform/loader/fetch/resource_fetcher.cc

Labels: Merge-Merged-70-3538
The following revision refers to this bug: 
https://chromium.googlesource.com/chromium/src.git/+/d7e12a8117080fa0b8feb550b35fffccf340e27b

Commit: d7e12a8117080fa0b8feb550b35fffccf340e27b
Author: torne@google.com
Commiter: torne@chromium.org
Date: 2018-10-25 16:40:57 +0000 UTC

Remove CORS scheme check temporarily

The scheme check added at
https://crrev.com/43b8facd3a2fbfb8f75b455f07d64acc2c13fb5c caused some
issues on WebView. This change deletes the check temporarily.

TBR=yhirano@chromium.org

(cherry picked from commit 14314b070e1e92dcae7fe4ccfa4531c6a128060e)

Bug: 898589
Change-Id: I6126b3b25f8700fc0547fa2c70b62bd183cc5e23
Reviewed-on: https://chromium-review.googlesource.com/c/1298828
Reviewed-by: Takashi Toyoshima <toyoshim@chromium.org>
Commit-Queue: Richard Coles <torne@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#602736}
Reviewed-on: https://chromium-review.googlesource.com/c/1299439
Reviewed-by: Richard Coles <torne@chromium.org>
Cr-Commit-Position: refs/branch-heads/3538@{#1045}
Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811}
We should figure out the long term solution to this but this should be fixed in the short term for 70+ now. See also issue 896059 for a related issue with unknown origins where we need to come up with a good resolution for webview.
Thank you!
I have triggered a new build of 70, to be qualified and released on Monday.
Labels: -Merge-Request-70 Merge-Approved-70
Verified on Acer Predator (LMY47I) , Nexus 6P/OPM1.180608.001 ,LG v20 / NRD90M having 70.0.3538.80 on books list provided by Play books team , issue mentioned in #1 in b/118342051 is no longer reproducible.Thanks for the fix 
Issue verified on latest M71:71.0.3578.27 and M72 72.0.3595.0 as per#1

Tested devices:Nexus 5X / N2G48L

Thanks!
Owner: torne@chromium.org
I'll take this to work out what our long term plan is.
Project Member

Comment 23 by sheriffbot@chromium.org, Oct 29

This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: -Merge-Approved-70
Verified on Acer Predator (LMY47I) , Nexus 6P/OPM1.180608.001 ,having 71.0.3578.27 and 72.0.3595.0 on books list provided by Play books team , issue mentioned  is no longer reproducible.
Labels: -Hotlist-Merge-Review -Merge-Review-71
Removing labels as this is merged already.
 Issue 900528  has been merged into this issue.
This is still broken in Chrome 71.0.3578.45 according to the comments in  bug 900528 . Chrome 70 is fine though.
Labels: -Pri-1 -ReleaseBlock-Stable -Target-70 Target-72 M-72 Pri-2
 issue 900528  looks like it's actually a different but related regression introduced in M71, I've reopened there and will investigate further.

The M70 regression originally tracked here does appear to be fixed so I'm downgrading this bug to keep working on a longer term solution; going to aim to have a plan for M72.
Project Member

Comment 30 by bugdroid1@chromium.org, Nov 19

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

commit 73b9579dda0c94c358e412b26cfb1d38d8dbd82c
Author: Changwan Ryu <changwan@google.com>
Date: Mon Nov 19 23:14:52 2018

Add tests for XHR across custom-schemed URIs

When you use custom-schemed URLs across XHR, you will see an error
saying that you cannot send an XHR from 'null' origin. This CL adds
tests to compare the behavior between HTTP-schemed URL and
custom-schemed URL such that the actual fix can pick it up.

Bug: 898589,  900528 , 896059
Change-Id: I43b4748a6595298d4ac6fdb216c8db1738719adf
Reviewed-on: https://chromium-review.googlesource.com/c/1340999
Reviewed-by: Richard Coles <torne@chromium.org>
Commit-Queue: Changwan Ryu <changwan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#609490}
[modify] https://crrev.com/73b9579dda0c94c358e412b26cfb1d38d8dbd82c/android_webview/javatests/src/org/chromium/android_webview/test/LoadDataWithBaseUrlTest.java

Verified/ on webview: 71.0.3578.64 on Samsing Tab S2/NRD90M
Project Member

Comment 32 by bugdroid1@chromium.org, Nov 21

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

commit 743e405a230b9b70f5ec5f9ccfeec45f35e6fd17
Author: Nate Fischer <ntfschr@chromium.org>
Date: Wed Nov 21 00:32:27 2018

AW: add test to network service filter

No change to production logic.

This adds a newly added test to the network service test filter:
LoadDataWithBaseUrlTest#testXhrForHttpSchemeUrl. This was added during
http://crrev/c/1340999.

R=jam@chromium.org

Bug: 898589,  900528 , 896059
Test: CQ
Change-Id: I6e83bc3da5389c27761cf2c80620c8990bb6a850
Cq-Include-Trybots: master.tryserver.chromium.android:android_mojo
Reviewed-on: https://chromium-review.googlesource.com/c/1345246
Reviewed-by: Clark DuVall <cduvall@chromium.org>
Commit-Queue: Nate Fischer <ntfschr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#609866}
[modify] https://crrev.com/743e405a230b9b70f5ec5f9ccfeec45f35e6fd17/testing/buildbot/filters/mojo.fyi.network_webview_instrumentation_test_apk.filter

Project Member

Comment 33 by bugdroid1@chromium.org, Nov 30

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

commit dd548ee4bc979f76190bb74d7818a5d29a9fcd8f
Author: Changwan Ryu <changwan@google.com>
Date: Fri Nov 30 22:31:03 2018

//url: Allow Android WebView to use origins with non-standard schemes

This adds an escape hatch so that Android WebView can restore the old
behavior before https://crrev.com/c/1208811/.

Bug: 896059
Change-Id: I90cc51fe5c6ddaa95281a51a5b89795e8a3958fe
Reviewed-on: https://chromium-review.googlesource.com/c/1338660
Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
Reviewed-by: Changwan Ryu <changwan@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#611436}
(cherry picked from commit c2b752b1d03355b2e9bbde897731b616e9f70883)

Add tests for XHR across custom-schemed URIs

When you use custom-schemed URLs across XHR, you will see an error
saying that you cannot send an XHR from 'null' origin. This CL adds
tests to compare the behavior between HTTP-schemed URL and
custom-schemed URL such that the actual fix can pick it up.

Bug: 898589,  900528 , 896059
Change-Id: I43b4748a6595298d4ac6fdb216c8db1738719adf
Reviewed-on: https://chromium-review.googlesource.com/c/1340999
Reviewed-by: Richard Coles <torne@chromium.org>
Commit-Queue: Changwan Ryu <changwan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#609490}
(cherry picked from commit 73b9579dda0c94c358e412b26cfb1d38d8dbd82c)

Add a test for window.origin

Check window.origin value for HTTPS-schemed base Uri vs
custom-schemed base Uri.

(cherry picked from commit 54ef219a7191071dd74382ab6a9668ebd76be81c)

Bug: 896059
Change-Id: Ic310c584e0826d14683734f8e6190875dddcc28f
Reviewed-on: https://chromium-review.googlesource.com/c/1340821
Reviewed-by: Bo <boliu@chromium.org>
Commit-Queue: Changwan Ryu <changwan@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#608887}
Reviewed-on: https://chromium-review.googlesource.com/c/1357559
Reviewed-by: Changwan Ryu <changwan@chromium.org>
Cr-Commit-Position: refs/branch-heads/3578@{#863}
Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034}
[modify] https://crrev.com/dd548ee4bc979f76190bb74d7818a5d29a9fcd8f/android_webview/common/aw_content_client.cc
[modify] https://crrev.com/dd548ee4bc979f76190bb74d7818a5d29a9fcd8f/android_webview/javatests/src/org/chromium/android_webview/test/LoadDataWithBaseUrlTest.java
[modify] https://crrev.com/dd548ee4bc979f76190bb74d7818a5d29a9fcd8f/content/common/url_schemes.cc
[modify] https://crrev.com/dd548ee4bc979f76190bb74d7818a5d29a9fcd8f/content/public/common/content_client.h
[modify] https://crrev.com/dd548ee4bc979f76190bb74d7818a5d29a9fcd8f/third_party/blink/renderer/platform/weborigin/security_origin.cc
[modify] https://crrev.com/dd548ee4bc979f76190bb74d7818a5d29a9fcd8f/third_party/blink/renderer/platform/weborigin/security_origin_test.cc
[modify] https://crrev.com/dd548ee4bc979f76190bb74d7818a5d29a9fcd8f/url/origin.cc
[modify] https://crrev.com/dd548ee4bc979f76190bb74d7818a5d29a9fcd8f/url/origin_unittest.cc
[modify] https://crrev.com/dd548ee4bc979f76190bb74d7818a5d29a9fcd8f/url/scheme_host_port.cc
[modify] https://crrev.com/dd548ee4bc979f76190bb74d7818a5d29a9fcd8f/url/url_util.cc
[modify] https://crrev.com/dd548ee4bc979f76190bb74d7818a5d29a9fcd8f/url/url_util.h

Sign in to add a comment