Issue metadata
Sign in to add a comment
|
URLLoader accepts parameters it doesn't respect |
||||||||||||||||||||||
Issue descriptionnetwork::ResourceRequest has a number of mojom::Fetch* fields. None of these are respected by URLLoader/URLLoaderFactory. Instead, they're only respected by CORSURLLoader / CORSURLLoaderFactory. This could lead to consumers setting flags when using a URLLoaderFactory and expecting them to be obeyed, while the URLLoaderFactory just ignores them. Ideally, we'd use separate classes, but that may be a bit too much effort. A lower effort solution would be to have URLLoaderFactory DCHECK on values it doesn't respect. This issue came up when I saw FetchRedirectMode::kError being applied to a request that was using URLLoaderFactory, so this is likely going to be a real class of problems.
,
Jul 11
CORSURLLoader uses network::URLLoader, so even when CORSURLLoader handles them correctly network::URLLoader sees such values.
,
Jul 11
This is true, but the current patternt has already led to problems - we can either use a private CORSURLLoader->URLLoader path to avoid the DCHECKs, or clear the fields on the parameters we pass to the URLLoader.
,
Jul 11
When out-of-renderer CORS is enabled by default, I think we should always create CORSURLLoaderFactory in NetworkContext::CreateURLLoaderFactory so that CORS related properties should always be handled. Maybe we should change the default value of ResourceRequest::fetch_request_mode to kNoCORS so as to CORSURLLoader doesn't do anything for requests made by customers who are not interested in CORS.
,
Jul 11
Is the plan to enable CORS for browser-side consumers? I was not aware of that. Either way, as long as we offer an interface where CORS is not supported, we should prevent consumers from passing CORS-only arguments to it. Note that "FetchRedirectMode::kError" has no clear implication that it's a CORS-only feature.
,
Jul 11
The fetch redirect mode must be "follow" when the fetch mode is "no-cors" in the fetch spec, so it should be reasonable to have the same restriction in CORSURLLoader, and network::URLLoader too until CORSURLLoader is fully enabled, I think (with some comments in network::ResourceRequest).
,
Jul 11
,
Jul 13
I found that CSP violates the rule. I filed a spec bug: https://github.com/w3c/webappsec-csp/issues/320
,
Jul 13
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6b31355b4ae63f627de7a8a003e26b76b1bd9899 commit 6b31355b4ae63f627de7a8a003e26b76b1bd9899 Author: Yutaka Hirano <yhirano@chromium.org> Date: Fri Jul 13 05:04:36 2018 ResourceRequest::fetch_request_mode should be kNoCORS by default We are planning to enable CORSURLLoader in services/network. As many browser-initiated requests are not interested in CORS, this CL changes the default value for ResourceRequest::fetch_request_mode from kSameOrigin to kNOCORS which bypasses all CORS logic. This CL also changes the initial value for fetch_credentials_mode so that it matches with the default load flags. This CL also adds a precondition for the redirect mode: as long as the request mode is kNoCORS, the redirect mode must be kFollow, which matches with the fetch spec. I will enforce the precondition in a subsequent CL. Bug: 862184 Cq-Include-Trybots: luci.chromium.try:linux_mojo Change-Id: Icbdf9c113f7eb310fa40495c54bbaa09d6313dce Reviewed-on: https://chromium-review.googlesource.com/1132832 Reviewed-by: Matt Menke <mmenke@chromium.org> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Commit-Queue: Yutaka Hirano <yhirano@chromium.org> Cr-Commit-Position: refs/heads/master@{#574841} [modify] https://crrev.com/6b31355b4ae63f627de7a8a003e26b76b1bd9899/services/network/cors/cors_url_loader_unittest.cc [modify] https://crrev.com/6b31355b4ae63f627de7a8a003e26b76b1bd9899/services/network/cors/preflight_controller_unittest.cc [modify] https://crrev.com/6b31355b4ae63f627de7a8a003e26b76b1bd9899/services/network/public/cpp/resource_request.h
,
Jul 19
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c9dcd5e903db6fd1a4943b9e5e2eb3afdaca6ad7 commit c9dcd5e903db6fd1a4943b9e5e2eb3afdaca6ad7 Author: Yutaka Hirano <yhirano@chromium.org> Date: Thu Jul 19 05:22:08 2018 Hook up CORSURLLoader to Network Service This CL consists of several parts: - Move mojo::Binding from network::URLLoaderFactory to network::CORSURLLoaderFactory: Now CORSURLLoaderFactory is the implementation of network::URLLoaderFactory returned from NetworkContext::CreateURLLoaderFactory, so this CL moves the binding logic to CORSURLLoader. - Loosen the redirect mode check in CORSURLLoader: Some requests with kNavigate mode and kManual redirect mode calls FollowRedirect. It seems a reasonable usage, so CORSURLLoader now allows that. - Add virtual/outofblink-cors-ns: From some infrastructure issues we cannot test out-of-blink CORS with Network Service, with virtual/outofblink-cors on linux-mojo. This CL explicitly adds one more virtual test suite instead. Bug: 736308, 862184 Cq-Include-Trybots: luci.chromium.try:linux_mojo Change-Id: If5db2725b2c5af83a2f1c1481c612bcb4ea75567 Reviewed-on: https://chromium-review.googlesource.com/1134926 Commit-Queue: Yutaka Hirano <yhirano@chromium.org> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Reviewed-by: Matt Menke <mmenke@chromium.org> Cr-Commit-Position: refs/heads/master@{#576381} [modify] https://crrev.com/c9dcd5e903db6fd1a4943b9e5e2eb3afdaca6ad7/content/browser/loader/navigation_url_loader_impl_unittest.cc [modify] https://crrev.com/c9dcd5e903db6fd1a4943b9e5e2eb3afdaca6ad7/content/browser/loader/resource_message_filter.cc [modify] https://crrev.com/c9dcd5e903db6fd1a4943b9e5e2eb3afdaca6ad7/services/network/cors/cors_url_loader.cc [modify] https://crrev.com/c9dcd5e903db6fd1a4943b9e5e2eb3afdaca6ad7/services/network/cors/cors_url_loader.h [modify] https://crrev.com/c9dcd5e903db6fd1a4943b9e5e2eb3afdaca6ad7/services/network/cors/cors_url_loader_factory.cc [modify] https://crrev.com/c9dcd5e903db6fd1a4943b9e5e2eb3afdaca6ad7/services/network/cors/cors_url_loader_factory.h [modify] https://crrev.com/c9dcd5e903db6fd1a4943b9e5e2eb3afdaca6ad7/services/network/cors/cors_url_loader_unittest.cc [modify] https://crrev.com/c9dcd5e903db6fd1a4943b9e5e2eb3afdaca6ad7/services/network/network_context.cc [modify] https://crrev.com/c9dcd5e903db6fd1a4943b9e5e2eb3afdaca6ad7/services/network/network_context.h [modify] https://crrev.com/c9dcd5e903db6fd1a4943b9e5e2eb3afdaca6ad7/services/network/url_loader.h [modify] https://crrev.com/c9dcd5e903db6fd1a4943b9e5e2eb3afdaca6ad7/services/network/url_loader_factory.cc [modify] https://crrev.com/c9dcd5e903db6fd1a4943b9e5e2eb3afdaca6ad7/services/network/url_loader_factory.h [modify] https://crrev.com/c9dcd5e903db6fd1a4943b9e5e2eb3afdaca6ad7/services/network/url_loader_unittest.cc [modify] https://crrev.com/c9dcd5e903db6fd1a4943b9e5e2eb3afdaca6ad7/third_party/WebKit/LayoutTests/TestExpectations [modify] https://crrev.com/c9dcd5e903db6fd1a4943b9e5e2eb3afdaca6ad7/third_party/WebKit/LayoutTests/VirtualTestSuites [add] https://crrev.com/c9dcd5e903db6fd1a4943b9e5e2eb3afdaca6ad7/third_party/WebKit/LayoutTests/virtual/outofblink-cors-ns/external/wpt/fetch/README.txt [add] https://crrev.com/c9dcd5e903db6fd1a4943b9e5e2eb3afdaca6ad7/third_party/WebKit/LayoutTests/virtual/outofblink-cors-ns/external/wpt/http/README.txt [add] https://crrev.com/c9dcd5e903db6fd1a4943b9e5e2eb3afdaca6ad7/third_party/WebKit/LayoutTests/virtual/outofblink-cors-ns/external/wpt/referrer-policy/README.txt [add] https://crrev.com/c9dcd5e903db6fd1a4943b9e5e2eb3afdaca6ad7/third_party/WebKit/LayoutTests/virtual/outofblink-cors-ns/external/wpt/service-workers/README.txt [add] https://crrev.com/c9dcd5e903db6fd1a4943b9e5e2eb3afdaca6ad7/third_party/WebKit/LayoutTests/virtual/outofblink-cors-ns/external/wpt/service-workers/service-worker/clients-matchall-client-types.https-expected.txt [add] https://crrev.com/c9dcd5e903db6fd1a4943b9e5e2eb3afdaca6ad7/third_party/WebKit/LayoutTests/virtual/outofblink-cors-ns/http/tests/fetch/README.txt [add] https://crrev.com/c9dcd5e903db6fd1a4943b9e5e2eb3afdaca6ad7/third_party/WebKit/LayoutTests/virtual/outofblink-cors-ns/http/tests/xmlhttprequest/README.txt [add] https://crrev.com/c9dcd5e903db6fd1a4943b9e5e2eb3afdaca6ad7/third_party/WebKit/LayoutTests/virtual/outofblink-cors-ns/http/tests/xmlhttprequest/open-in-body-onload-sync-to-invalid-redirect-response-handling-expected.txt [add] https://crrev.com/c9dcd5e903db6fd1a4943b9e5e2eb3afdaca6ad7/third_party/WebKit/LayoutTests/virtual/outofblink-cors-ns/http/tests/xmlhttprequest/origin-header-cross-origin-get-sync-expected.txt [add] https://crrev.com/c9dcd5e903db6fd1a4943b9e5e2eb3afdaca6ad7/third_party/WebKit/LayoutTests/virtual/outofblink-cors-ns/http/tests/xmlhttprequest/origin-header-cross-origin-post-sync-expected.txt [add] https://crrev.com/c9dcd5e903db6fd1a4943b9e5e2eb3afdaca6ad7/third_party/WebKit/LayoutTests/virtual/outofblink-cors-ns/http/tests/xmlhttprequest/origin-header-same-origin-post-sync-expected.txt [add] https://crrev.com/c9dcd5e903db6fd1a4943b9e5e2eb3afdaca6ad7/third_party/WebKit/LayoutTests/virtual/outofblink-cors-ns/http/tests/xmlhttprequest/redirect-cross-origin-sync-double-expected.txt
,
Jul 19
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/27f088978ef6cb2b39dc72bd3ef3e558b4502045 commit 27f088978ef6cb2b39dc72bd3ef3e558b4502045 Author: Yutaka Hirano <yhirano@chromium.org> Date: Thu Jul 19 10:01:41 2018 Reject insane requests in CORSURLLoaderFactory With this CL, CORSURLLoaderFactory rejects ill-configuared requests. - CORS needs a proper origin (including an opaque unique origin) attached to a request. Hence CORSURLLoaderFactory rejects a request which has a CORS-enabled mode and null request_initiator. Also, a request with null request_initiator won't set the CORS flag with this CL. - The relationship between fetch credentials mode and load_flags is a bit unclear. If a request's credentials mode is "omit" but one of LOAD_DO_NOT_SAVE_COOKIES, LOAD_DO_NOT_SEND_COOKIES and LOAD_DO_NOT_SEND_AUTH_DATA is not set on load_flags, that is likely a mis-configuration, so fail the request. Bug: 736308, 862184 Cq-Include-Trybots: luci.chromium.try:linux_mojo Change-Id: I51fb491b865de330b22b028a0eddbc30043e6b69 Reviewed-on: https://chromium-review.googlesource.com/1136342 Commit-Queue: Yutaka Hirano <yhirano@chromium.org> Reviewed-by: Matt Menke <mmenke@chromium.org> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Cr-Commit-Position: refs/heads/master@{#576430} [modify] https://crrev.com/27f088978ef6cb2b39dc72bd3ef3e558b4502045/services/network/cors/cors_url_loader.cc [modify] https://crrev.com/27f088978ef6cb2b39dc72bd3ef3e558b4502045/services/network/cors/cors_url_loader_factory.cc [modify] https://crrev.com/27f088978ef6cb2b39dc72bd3ef3e558b4502045/services/network/cors/cors_url_loader_factory.h [modify] https://crrev.com/27f088978ef6cb2b39dc72bd3ef3e558b4502045/services/network/cors/cors_url_loader_unittest.cc [modify] https://crrev.com/27f088978ef6cb2b39dc72bd3ef3e558b4502045/services/network/public/cpp/resource_request.h
,
Jul 19
The following tests are failing on WebKit Linux Trusty MSAN. I will disable them for linux. * virtual/outofblink-cors-ns/http/tests/fetch/chromium/call-extra-crash-tee.html * virtual/outofblink-cors-ns/http/tests/fetch/chromium/release-handle-crash.html https://ci.chromium.org/buildbot/chromium.webkit/WebKit%20Linux%20Trusty%20MSAN/8969
,
Jul 19
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/bbc82f70c0ba7ea2ddc0b34cdcdd90b75fdfa5b8 commit bbc82f70c0ba7ea2ddc0b34cdcdd90b75fdfa5b8 Author: Christian Dullweber <dullweber@chromium.org> Date: Thu Jul 19 12:38:40 2018 Disable fetch webkit tests on Linux Disable failing tests on Linux: virtual/outofblink-cors-ns/http/tests/fetch/chromium/call-extra-crash-tee.html virtual/outofblink-cors-ns/http/tests/fetch/chromium/release-handle-crash.html https://ci.chromium.org/buildbot/chromium.webkit/WebKit%20Linux%20Trusty%20MSAN/8969 Tbr: yhirano@chromium.org Bug: 862184 Change-Id: Ib65bb4bb3b3b2d4cd58f8ffb10e2f49b5772e632 Reviewed-on: https://chromium-review.googlesource.com/1143273 Reviewed-by: Yutaka Hirano <yhirano@chromium.org> Commit-Queue: Christian Dullweber <dullweber@chromium.org> Cr-Commit-Position: refs/heads/master@{#576456} [modify] https://crrev.com/bbc82f70c0ba7ea2ddc0b34cdcdd90b75fdfa5b8/third_party/WebKit/LayoutTests/TestExpectations
,
Jul 19
Also timing out intermittently on two Linux builders: virtual/outofblink-cors-ns/http/tests/fetch/serviceworker-proxied/thorough/cors-base-https-other-https.html https://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=virtual%2Foutofblink-cors-ns%2Fhttp%2Ftests%2Ffetch%2Fserviceworker-proxied%2Fthorough%2Fcors-base-https-other-https.html&testType=site_per_process_webkit_layout_tests (Markign as flaky in TestExpectations)
,
Jul 19
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/22179d3943015494b82b0e462f34e5007a0b7857 commit 22179d3943015494b82b0e462f34e5007a0b7857 Author: Ian Clelland <iclelland@chromium.org> Date: Thu Jul 19 20:58:18 2018 Mark one more OOB-CORS test as flaky. Bug: 862184 Tbr: yhirano@chromium.org Change-Id: Ibd202a89747e0e0567f1b11155bf67594deeb55b Reviewed-on: https://chromium-review.googlesource.com/1144050 Reviewed-by: Ian Clelland <iclelland@chromium.org> Commit-Queue: Ian Clelland <iclelland@chromium.org> Cr-Commit-Position: refs/heads/master@{#576621} [modify] https://crrev.com/22179d3943015494b82b0e462f34e5007a0b7857/third_party/WebKit/LayoutTests/TestExpectations
,
Jul 20
> #14 That is not specific to outofblink-cors[-ns]. See https://test-results.appspot.com/dashboards/flakiness_dashboard.html#testType=site_per_process_webkit_layout_tests&tests=http%2Ftests%2Ffetch%2Fserviceworker-proxied%2Fthorough%2Fcors-base-https-other-https.html
,
Jul 20
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/627748f5bf4ffd7fd4bda776f1c21b8fbe3af4e3 commit 627748f5bf4ffd7fd4bda776f1c21b8fbe3af4e3 Author: Yutaka Hirano <yhirano@chromium.org> Date: Fri Jul 20 01:26:52 2018 [OOR-CORS] Reorganize layout test expectations for site-per-process virtual/outofblink-cors and virtual/outofblink-cors-ns should inherit existing test expectations. There is a dedicated bug for virtual/outofblink-cors, https://crbug.com/854630 , but that doesn't make sense as none of them are OOR-CORS specific, so I changed the bug number. TBR=dcheng@chromium.org No-Try: true Bug: 834185 , 854630 , 859988, 862184 Change-Id: I4886c8ed063e95306d79234649dbc54a8bb53100 Reviewed-on: https://chromium-review.googlesource.com/1144582 Reviewed-by: Yutaka Hirano <yhirano@chromium.org> Commit-Queue: Yutaka Hirano <yhirano@chromium.org> Cr-Commit-Position: refs/heads/master@{#576749} [modify] https://crrev.com/627748f5bf4ffd7fd4bda776f1c21b8fbe3af4e3/third_party/WebKit/LayoutTests/FlagExpectations/site-per-process [modify] https://crrev.com/627748f5bf4ffd7fd4bda776f1c21b8fbe3af4e3/third_party/WebKit/LayoutTests/TestExpectations
,
Jul 20
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e5e1145d96b45d730a223207f8c52ecaacb5aef1 commit e5e1145d96b45d730a223207f8c52ecaacb5aef1 Author: Yutaka Hirano <yhirano@chromium.org> Date: Fri Jul 20 04:28:27 2018 [OOR-CORS] Copy base tests' slowness expectation to outofblink-cors-ns http/tests/fetch/chromium/call-extra-crash-tee.html and http/tests/fetch/chromium/release-handle-crash.html are marked as slow. Let's mark virtual/outofblink-cors-ns/ ones as slow as well instead of marking them as TIMEOUT. Bug: 862184, 853977 Change-Id: Ia26c116e5c43912591ce61137abe50473c9e8881 Reviewed-on: https://chromium-review.googlesource.com/1144592 Reviewed-by: Adam Rice <ricea@chromium.org> Commit-Queue: Yutaka Hirano <yhirano@chromium.org> Cr-Commit-Position: refs/heads/master@{#576779} [modify] https://crrev.com/e5e1145d96b45d730a223207f8c52ecaacb5aef1/third_party/WebKit/LayoutTests/SlowTests [modify] https://crrev.com/e5e1145d96b45d730a223207f8c52ecaacb5aef1/third_party/WebKit/LayoutTests/TestExpectations |
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 Deleted