Network service: Cross origin xhr to a file url fails for an extension injected script. |
|||||||||||||||||||||
Issue descriptionRepro steps: (Copied from https://bugs.chromium.org/p/chromium/issues/detail?id=894812#c4), REPRODUCTION CASE 1. Install the attached extension. Ensure that "Allow access to file URLs" is enabled. 2. Once installed, the extension will open a new tab pointing to a file url. 3. It will then use chrome.tabs.executeScript to inject a script into the page that will make an XHR request to retrieve a file url every 4 seconds. See the console on Devtools. The xhr will fail when network service is enabled. It shouldn't. Version: 72.0.3582.0 (Canary) Assigning to jam@ for triage.
,
Nov 12
,
Nov 12
,
Nov 13
,
Nov 13
Looks like this may have been due to http://crrev.com/c/1270301, I'll investigate a bit more and put up a fix.
,
Nov 13
It looks like this may be a bit complicated, assigning to yhirano@ since he made the original change and has more context on CORS. Extensions add to the allow list in the renderer here[1], but we don't use that information in FileURLLoaderFactory when we fail the request for CORS here[2]. 1. https://cs.chromium.org/chromium/src/extensions/renderer/dispatcher.cc?l=1223&rcl=bf137c2ae9c25c57ac2d87ee60e74f9b487aa05e 2. https://cs.chromium.org/chromium/src/content/browser/file_url_loader_factory.cc?l=738&rcl=ca2f043a37f284107b2d8f82d925aebc39d98237
,
Nov 13
,
Nov 14
+mek@ toyoshim@, you know much more about the extension origin whitelist than me. Do you have any good idea?
,
Nov 14
I'm proposing a workaround: https://chromium-review.googlesource.com/c/chromium/src/+/1335078 dxie@, jam@, How urgent is this? Do you want me to merge the fix to M71?
,
Nov 14
Since I do not have time today, yhirano@ made a patch to disable browser side CORS checks. Even with this, we still have blink side checks as we have had in the default mode. I'd have a fix to enable this again, but it will rely on my current extension support patch that is under review.
,
Nov 19
,
Nov 19
,
Nov 19
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/fbb488d8c1fe6317bbae4f87713b3719085f1bd3 commit fbb488d8c1fe6317bbae4f87713b3719085f1bd3 Author: Yutaka Hirano <yhirano@chromium.org> Date: Mon Nov 19 09:39:55 2018 Disable CORS check in FileURLLoader It was introduced for out-of-blink CORS, but the check doesn't work with extensions. Until it's fixes let's rely on the renderer side check and disable the check. Bug: 895999 Change-Id: I59aaafcb9185e1937372cf8967ef45d3728dff64 Reviewed-on: https://chromium-review.googlesource.com/c/1335078 Commit-Queue: Yutaka Hirano <yhirano@chromium.org> Reviewed-by: John Abd-El-Malek <jam@chromium.org> Reviewed-by: Takashi Toyoshima <toyoshim@chromium.org> Reviewed-by: Marijn Kruisselbrink <mek@chromium.org> Cr-Commit-Position: refs/heads/master@{#609213} [modify] https://crrev.com/fbb488d8c1fe6317bbae4f87713b3719085f1bd3/content/browser/file_url_loader_factory.cc
,
Nov 19
,
Nov 20
Verified this issue on Mac 10.13.6 with chrome #72.0.3616.0 with provided extension in comment #0 and observed that xhr is successful when network service flag is enabled. Hence adding TE Verified labels. Note: The provided extension is not working in Debian & Windows.
,
Dec 4
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/deddf2bb8f2e519cf9f12d5f77139b3f14fb8236 commit deddf2bb8f2e519cf9f12d5f77139b3f14fb8236 Author: Takashi Toyoshima <toyoshim@chromium.org> Date: Tue Dec 04 14:51:28 2018 OOR-CORS: Origin access list support for FileURLLoader FileURLLoader does not know CORS access list for now, but it is needed to decide if the request is permitted by the access list beyond the CORS policy. Now, BrowserContext manages per-profile CORS access list. FileURLLoader also needs to access the list for this CORS exemption. Bug: 906536 , 895999 Change-Id: I3123de282d6eab5299388b902c19c96f434f4568 Reviewed-on: https://chromium-review.googlesource.com/c/1351321 Commit-Queue: Takashi Toyoshima <toyoshim@chromium.org> Reviewed-by: Yutaka Hirano <yhirano@chromium.org> Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org> Reviewed-by: Devlin <rdevlin.cronin@chromium.org> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Cr-Commit-Position: refs/heads/master@{#613547} [modify] https://crrev.com/deddf2bb8f2e519cf9f12d5f77139b3f14fb8236/content/browser/download/download_manager_impl.cc [modify] https://crrev.com/deddf2bb8f2e519cf9f12d5f77139b3f14fb8236/content/browser/download/file_download_url_loader_factory_getter.cc [modify] https://crrev.com/deddf2bb8f2e519cf9f12d5f77139b3f14fb8236/content/browser/download/file_download_url_loader_factory_getter.h [modify] https://crrev.com/deddf2bb8f2e519cf9f12d5f77139b3f14fb8236/content/browser/file_url_loader_factory.cc [modify] https://crrev.com/deddf2bb8f2e519cf9f12d5f77139b3f14fb8236/content/browser/file_url_loader_factory.h [modify] https://crrev.com/deddf2bb8f2e519cf9f12d5f77139b3f14fb8236/content/browser/frame_host/render_frame_host_impl.cc [modify] https://crrev.com/deddf2bb8f2e519cf9f12d5f77139b3f14fb8236/content/browser/loader/navigation_url_loader_impl.cc [modify] https://crrev.com/deddf2bb8f2e519cf9f12d5f77139b3f14fb8236/content/browser/worker_host/worker_script_fetch_initiator.cc [modify] https://crrev.com/deddf2bb8f2e519cf9f12d5f77139b3f14fb8236/content/public/browser/file_url_loader.h [modify] https://crrev.com/deddf2bb8f2e519cf9f12d5f77139b3f14fb8236/extensions/browser/updater/extension_downloader.cc
,
Dec 6
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b96c8462ac1f9070122df7597b665de78f31a544 commit b96c8462ac1f9070122df7597b665de78f31a544 Author: Takashi Toyoshima <toyoshim@chromium.org> Date: Thu Dec 06 08:23:01 2018 OOR-CORS: Origin access list support for FileURLLoader FileURLLoader does not know CORS access list for now, but it is needed to decide if the request is permitted by the access list beyond the CORS policy. Now, BrowserContext manages per-profile CORS access list. FileURLLoader also needs to access the list for this CORS exemption. Bug: 906536 , 895999 Change-Id: I3123de282d6eab5299388b902c19c96f434f4568 Reviewed-on: https://chromium-review.googlesource.com/c/1351321 Commit-Queue: Takashi Toyoshima <toyoshim@chromium.org> Reviewed-by: Yutaka Hirano <yhirano@chromium.org> Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org> Reviewed-by: Devlin <rdevlin.cronin@chromium.org> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Cr-Commit-Position: refs/heads/master@{#613547}(cherry picked from commit deddf2bb8f2e519cf9f12d5f77139b3f14fb8236) TBR=yhirano@chromium.org Change-Id: I3123de282d6eab5299388b902c19c96f434f4568 Reviewed-on: https://chromium-review.googlesource.com/c/1364973 Reviewed-by: Takashi Toyoshima <toyoshim@chromium.org> Cr-Commit-Position: refs/branch-heads/3626@{#97} Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437} [modify] https://crrev.com/b96c8462ac1f9070122df7597b665de78f31a544/content/browser/download/download_manager_impl.cc [modify] https://crrev.com/b96c8462ac1f9070122df7597b665de78f31a544/content/browser/download/file_download_url_loader_factory_getter.cc [modify] https://crrev.com/b96c8462ac1f9070122df7597b665de78f31a544/content/browser/download/file_download_url_loader_factory_getter.h [modify] https://crrev.com/b96c8462ac1f9070122df7597b665de78f31a544/content/browser/file_url_loader_factory.cc [modify] https://crrev.com/b96c8462ac1f9070122df7597b665de78f31a544/content/browser/file_url_loader_factory.h [modify] https://crrev.com/b96c8462ac1f9070122df7597b665de78f31a544/content/browser/frame_host/render_frame_host_impl.cc [modify] https://crrev.com/b96c8462ac1f9070122df7597b665de78f31a544/content/browser/loader/navigation_url_loader_impl.cc [modify] https://crrev.com/b96c8462ac1f9070122df7597b665de78f31a544/content/browser/worker_host/worker_script_fetch_initiator.cc [modify] https://crrev.com/b96c8462ac1f9070122df7597b665de78f31a544/content/public/browser/file_url_loader.h [modify] https://crrev.com/b96c8462ac1f9070122df7597b665de78f31a544/extensions/browser/updater/extension_downloader.cc
,
Dec 11
Tried testing this issue on Mac OS 10.14.0 on the reported version 72.0.3582.0 and the chrome M-72 72.0.3626.14 by following the below steps. 1. Launched Chrome 2. Attached given extension and enabled Network service flag form > chrome://flags 3. Enabled >> Allow access to file URLs 3. Opened Dev tools > Console and Network tab >> xhr As we have observed that the error-fail on xhr every 4 seconds >>same behaviour on both chrome 72.0.3582.0 and chrome 72.0.3626.14. Attached is the screen cast for reference. Takashi Toyoshima@ Request you to check and confirm if anything is missed from our end in verifying the issue and help us to verify the fix on the M-72 build. Thanks..!
,
Dec 11
yhirano@, i still see this issue on today's Dev RC#72.0.3626.14 for Mac OS X 10.13.6. Can you please validate from your end? Thank you!
,
Dec 11
Hi, can you verify whether - the network servicification (#network-service) - out of blink CORS (#out-of-blink-cors) are enabled (I'm asking for testing in all the four cases)? Both are in finch experiments so please explicitly enable/disable the feature from chrome://flags.
,
Dec 12
For C#20, Below are the observations on current M72 Dev#72.0.3626.14 for Mac OS X. 1. If i enable both the flags: FAIL 2. If i disable both the flags: PASS 3. #network-service - Enabled AND #out-of-blink-cors - Disabled: FAIL 4. #network-service - Disabled AND #out-of-blink-cors - Enabled: PASS ************************************************************************* For current Mac Canary# 73.0.3637.0 all the above 4 scenarios are PASS. ************************************************************************* For chrome# 72.0.3616.0 (Doesn't have fix in c#16) - Except both the flags enabled scenario, rest of all are PASSING. Thank you!
,
Dec 12
Thank you for the investigation. I'm reopening this bug.
,
Dec 12
Let me check details. It's unlikely to happen, but my patch may not work as I expect when it was merged to the 72 branch.
,
Dec 12
b96c8462ac1f9070122df7597b665de78f31a544 was missed in the 72 branch, and c#16 needed that change. Sorry, I thought that was submitted before the branch-cut, but it was wrong. I will request a merge of b96c8462ac1f9070122df7597b665de78f31a544 to the 72 branch.
,
Dec 12
,
Dec 12
Note that without this fix merged, there is a race on accessing the allowlist, and this test case results in DCHECK failure on debug build.
,
Dec 12
Is this only affecting network service or all use cases? How safe is this merge overall and could this introduce any new regressions?
,
Dec 13
> #24 Pasted hash was wrong. 8893ec99d0180595cf950a2f7bad2869d9c394b7 was the missed CL, and b96c8462ac1f9070122df7597b665de78f31a544 depended on that. Only the latter one was merged mistakenly. > #27 That CL touches NetworkService enabled code path only. So, I'm sure it only affects NetworkService. Change itself is relatively simple one, and is running on Canary/Dev under 50:50 NetworkService trial from Nov 30 (for two weeks). My impression is it's safe to be merged, and also having the existing first merge without this depending CL is not safe. It's failing on DCHECK, and it's a combination that is not considered and not tested at all.
,
Dec 13
Your change meets the bar and is auto-approved for M72. Please go ahead and merge the CL to branch 3626 manually. Please contact milestone owner if you have questions. Owners: govind@(Android), kariahda@(iOS), djmm@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Dec 13
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/abeca37d816b48b549eb840a8231f704aac82c03 commit abeca37d816b48b549eb840a8231f704aac82c03 Author: Takashi Toyoshima <toyoshim@chromium.org> Date: Thu Dec 13 06:55:58 2018 OOR-CORS: Manage per-profile access list even for NetworkService This patch makes BrowserContext manages per-profile CORS access lists even if NetworkService is enabled, and use it to setup initial access lists for the non-primary NetworkContext. It will also work for restoring per-profile CORS settings on network service restarts. This patch makes following tests work even with --enable-features=OutOfBlinkCors,NetworkService. - CrossOriginReadBlockingExtensionTest.ProgrammaticContentScriptVsAppCache - CrossOriginReadBlockingExtensionTest.WebViewContentScript - ExtensionWebRequestApiTest.ExtensionRequests - PlatformAppBrowserTest.Isolation Bug: 908324 , 891891, 895999 Change-Id: Ib0cfc2f5633f25187366a4d7d63168d60ea51f71 Reviewed-on: https://chromium-review.googlesource.com/c/1351208 Reviewed-by: Matt Menke <mmenke@chromium.org> Reviewed-by: Yutaka Hirano <yhirano@chromium.org> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Commit-Queue: Takashi Toyoshima <toyoshim@chromium.org> Cr-Commit-Position: refs/heads/master@{#612567}(cherry picked from commit 8893ec99d0180595cf950a2f7bad2869d9c394b7) TBR=yhirano@chromium.org Change-Id: Ib0cfc2f5633f25187366a4d7d63168d60ea51f71 Reviewed-on: https://chromium-review.googlesource.com/c/1373370 Reviewed-by: Takashi Toyoshima <toyoshim@chromium.org> Cr-Commit-Position: refs/branch-heads/3626@{#319} Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437} [modify] https://crrev.com/abeca37d816b48b549eb840a8231f704aac82c03/chrome/browser/net/profile_network_context_service.cc [modify] https://crrev.com/abeca37d816b48b549eb840a8231f704aac82c03/content/browser/browser_context.cc [modify] https://crrev.com/abeca37d816b48b549eb840a8231f704aac82c03/content/browser/loader/resource_message_filter.cc [modify] https://crrev.com/abeca37d816b48b549eb840a8231f704aac82c03/content/browser/loader/shared_cors_origin_access_list_impl.cc [modify] https://crrev.com/abeca37d816b48b549eb840a8231f704aac82c03/content/public/browser/shared_cors_origin_access_list.h [modify] https://crrev.com/abeca37d816b48b549eb840a8231f704aac82c03/services/network/network_context.cc [modify] https://crrev.com/abeca37d816b48b549eb840a8231f704aac82c03/services/network/network_context.h [modify] https://crrev.com/abeca37d816b48b549eb840a8231f704aac82c03/services/network/public/mojom/network_context.mojom
,
Dec 13
The fix was merged to the beta branch. The next build will pick up the fix. manoranjanr@: please let me know if still something is wrong in the next release.
,
Dec 19
Verified this issue on Mac 10.13.6 with chrome #72.0.3626.28 with provided extension in comment #0 and observed that xhr is successful when network service flag is enabled. Hence adding TE Verified labels. Attaching the screen-cast for reference. Note: The provided extension is not working in Debian & Windows.
,
Dec 19
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/abeca37d816b48b549eb840a8231f704aac82c03 Commit: abeca37d816b48b549eb840a8231f704aac82c03 Author: toyoshim@chromium.org Commiter: toyoshim@chromium.org Date: 2018-12-13 06:55:58 +0000 UTC OOR-CORS: Manage per-profile access list even for NetworkService This patch makes BrowserContext manages per-profile CORS access lists even if NetworkService is enabled, and use it to setup initial access lists for the non-primary NetworkContext. It will also work for restoring per-profile CORS settings on network service restarts. This patch makes following tests work even with --enable-features=OutOfBlinkCors,NetworkService. - CrossOriginReadBlockingExtensionTest.ProgrammaticContentScriptVsAppCache - CrossOriginReadBlockingExtensionTest.WebViewContentScript - ExtensionWebRequestApiTest.ExtensionRequests - PlatformAppBrowserTest.Isolation Bug: 908324 , 891891, 895999 Change-Id: Ib0cfc2f5633f25187366a4d7d63168d60ea51f71 Reviewed-on: https://chromium-review.googlesource.com/c/1351208 Reviewed-by: Matt Menke <mmenke@chromium.org> Reviewed-by: Yutaka Hirano <yhirano@chromium.org> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Commit-Queue: Takashi Toyoshima <toyoshim@chromium.org> Cr-Commit-Position: refs/heads/master@{#612567}(cherry picked from commit 8893ec99d0180595cf950a2f7bad2869d9c394b7) TBR=yhirano@chromium.org Change-Id: Ib0cfc2f5633f25187366a4d7d63168d60ea51f71 Reviewed-on: https://chromium-review.googlesource.com/c/1373370 Reviewed-by: Takashi Toyoshima <toyoshim@chromium.org> Cr-Commit-Position: refs/branch-heads/3626@{#319} Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437}
,
Dec 19
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b96c8462ac1f9070122df7597b665de78f31a544 Commit: b96c8462ac1f9070122df7597b665de78f31a544 Author: toyoshim@chromium.org Commiter: toyoshim@chromium.org Date: 2018-12-06 08:23:01 +0000 UTC OOR-CORS: Origin access list support for FileURLLoader FileURLLoader does not know CORS access list for now, but it is needed to decide if the request is permitted by the access list beyond the CORS policy. Now, BrowserContext manages per-profile CORS access list. FileURLLoader also needs to access the list for this CORS exemption. Bug: 906536 , 895999 Change-Id: I3123de282d6eab5299388b902c19c96f434f4568 Reviewed-on: https://chromium-review.googlesource.com/c/1351321 Commit-Queue: Takashi Toyoshima <toyoshim@chromium.org> Reviewed-by: Yutaka Hirano <yhirano@chromium.org> Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org> Reviewed-by: Devlin <rdevlin.cronin@chromium.org> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Cr-Commit-Position: refs/heads/master@{#613547}(cherry picked from commit deddf2bb8f2e519cf9f12d5f77139b3f14fb8236) TBR=yhirano@chromium.org Change-Id: I3123de282d6eab5299388b902c19c96f434f4568 Reviewed-on: https://chromium-review.googlesource.com/c/1364973 Reviewed-by: Takashi Toyoshima <toyoshim@chromium.org> Cr-Commit-Position: refs/branch-heads/3626@{#97} Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437} |
|||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||
Comment 1 by jam@chromium.org
, Nov 12Owner: cduvall@chromium.org