Invalid location URL redirect response handling for Fetch API. |
|
Issue descriptionAccording to the spec(*), when the server respond with 30x redirect response with an invalid URL 'location' header, fetch() API with manual redirect mode request should be resolved with an opaque redirect response. (*) This commit on the spec changed the expected behavior. https://github.com/whatwg/fetch/commit/3e501f29eceff41eb81c60fb9937e33e23cf5492 But Chrome handles the invalid location URL in the browser process as an error, and cancels the request. https://chromium.googlesource.com/chromium/src/+/41d7e018/content/browser/child_process_security_policy_impl.cc#616 ResourceLoader::OnReceivedRedirect() ->ChildProcessSecurityPolicyImpl::GetInstance()->CanRequestURL() -> if (!url.is_valid()) return false; So it is difficult to distinguish the invalid location error in the renderer process. There is the same problem while handling the redirect response of Service Worker Navigation Preload. https://codereview.chromium.org/2790433003/diff/180001/content/browser/service_worker/service_worker_browsertest.cc#newcode2142
,
Apr 13 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/cb6838f5badc8c2df03f387f4aa726629214179a commit cb6838f5badc8c2df03f387f4aa726629214179a Author: horo <horo@chromium.org> Date: Thu Apr 13 07:27:00 2017 Make no-location redirect response to be "opaque redirect" when redirect mode is manual. According to the spec, even if location header is not set, we should treat the redirect response as "opaqueredirect" if the redirect mode of the fetch request is "manual". This behavior was changed by this commit on the spec. https://github.com/whatwg/fetch/commit/3e501f29eceff41eb81c60fb9937e33e23cf5492 BUG=707185 Review-Url: https://codereview.chromium.org/2785123002 Cr-Commit-Position: refs/heads/master@{#464329} [modify] https://crrev.com/cb6838f5badc8c2df03f387f4aa726629214179a/content/browser/cache_storage/cache_storage_cache.cc [modify] https://crrev.com/cb6838f5badc8c2df03f387f4aa726629214179a/third_party/WebKit/LayoutTests/external/wpt/fetch/api/redirect/redirect-location.js [modify] https://crrev.com/cb6838f5badc8c2df03f387f4aa726629214179a/third_party/WebKit/LayoutTests/http/tests/fetch/resources/thorough-util.js [modify] https://crrev.com/cb6838f5badc8c2df03f387f4aa726629214179a/third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/fetch.js [modify] https://crrev.com/cb6838f5badc8c2df03f387f4aa726629214179a/third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/thorough/redirect.js [modify] https://crrev.com/cb6838f5badc8c2df03f387f4aa726629214179a/third_party/WebKit/LayoutTests/http/tests/serviceworker/navigation-redirect.html [modify] https://crrev.com/cb6838f5badc8c2df03f387f4aa726629214179a/third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/navigation-redirect-scope1.php [modify] https://crrev.com/cb6838f5badc8c2df03f387f4aa726629214179a/third_party/WebKit/Source/modules/fetch/FetchManager.cpp
,
Apr 20 2017
Worth noting that we treat redirect response codes without a location header as non-redirects, since Google is using 308 responses as "RESUME INCOMPLETE", which per spec, they're redirects. Changing this behavior would break google APIs (I tried to get this dependency on changed 3+ years ago, but had no luck). So in other words, we can't really support the spec, as defined, in Chrome.
,
Jun 6 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d894710be3b56a15871376c5979615053dddcd70 commit d894710be3b56a15871376c5979615053dddcd70 Author: davidben <davidben@chromium.org> Date: Tue Jun 06 19:28:44 2017 Perform redirect checks before OnReceivedRedirect in //net. Right now, if URLRequest sees a redirect, it first signals OnReceivedRedirect, gives the caller a chance to reject the redirect and, if okay, it carries on to URLRequest::Redirect. URLRequest::Redirect then performs additional checks and potentially rejects the redirect. This ordering has two quirks: First, new_url.is_valid() is checked very late, but most consumers' checks presumably are going to check the URLs. This means that we reject invalid redirect URLs based not on //net code, but whatever consumer logic happens to notice. In //content, this is ChildProcessSecurityPolicyImpl, which interprets the renderer as trying to fetch somewhere questionable and just cancels the request. This results in an aborted request and no visible error, which is confusing, particularly for navigations. Second, URLRequest::Delegate gets called in an odd order. If we don't think request->url() should be allowed to redirect to new_url, whatever error we surface should be associated with request->url(), not new_url. That is, new_url did not misbehave necessarily. request->url() did for daring to utter such an obviously invalid URL. In particular, this is relevant for browser navigation logic, which needs to associate the error page with the right URL. If the error page is associate with new_url, reloading will skip the check, which is problematic. We do actually handle this properly. request->url() is not updated until later, but since Chrome is a complex multi-threaded multi-process IPC monster, the URLRequest is not immediate accessible. Instead, code likes to update the working URL when it receives OnReceivedRedirect. //net's current behavior is incompatible with this. In a normal redirect situation, we might signal: OnReceivedRedirect(new_url) // url() => old_url OnResponseStarted(net_error) // url() => new_url. But in this situation, we'll signal: OnReceivedRedirect(new_url) // url() => old_url OnResponseStarted(net_error) // url() => old_url! This is weird. This CL fixes this so it looks like: OnResponseStarted(net_error) // url() => old_url This means redirects to unparseable URLs simply fail with ERR_INVALID_REDIRECT (previously //net used ERR_INVALID_URL, but since the error page will commit at the old URL, ERR_INVALID_REDIRECT seems clearer). It also means that code which receives OnReceivedRedirect can eagerly update the URL, provided, of course, it orders it correctly with its own redirect checks. But the accept/reject signal may be propagated on-path, whereas a two-phase check (first consumer, then //net) requires potentially two round-trips. Unfortunately, this change does as a result require checking in newly failing test expectations for the redirect to data URL variant of issue #707185. Before this CL, whether we produced an opaque-redirect filtered response for an invalid redirect depended on whether the check happened in //net or outside. This now makes them treated similarly. Later, we can see about fixing both cases. (One thought is to teach //net about manual redirect mode, but this is subtle because //net persists info in the cache based on what it believes is and isn't a redirect. Another is to check the response object in OnResponseStarted and, if it's redirect-shaped, produce a filtered response wrapping an error response. This probably will involve adding a boolean to ResourceRequestCompletionStatus.) BUG= 462272 , 723796 ,707185 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2917133002 Cr-Commit-Position: refs/heads/master@{#477371} [modify] https://crrev.com/d894710be3b56a15871376c5979615053dddcd70/chrome/browser/net/errorpage_browsertest.cc [modify] https://crrev.com/d894710be3b56a15871376c5979615053dddcd70/components/error_page/common/localized_error.cc [modify] https://crrev.com/d894710be3b56a15871376c5979615053dddcd70/content/browser/browser_side_navigation_browsertest.cc [modify] https://crrev.com/d894710be3b56a15871376c5979615053dddcd70/content/browser/frame_host/data_url_navigation_browsertest.cc [modify] https://crrev.com/d894710be3b56a15871376c5979615053dddcd70/content/browser/frame_host/navigation_controller_impl_browsertest.cc [modify] https://crrev.com/d894710be3b56a15871376c5979615053dddcd70/content/browser/frame_host/navigation_handle_impl_browsertest.cc [modify] https://crrev.com/d894710be3b56a15871376c5979615053dddcd70/content/browser/service_worker/service_worker_browsertest.cc [modify] https://crrev.com/d894710be3b56a15871376c5979615053dddcd70/net/base/net_error_list.h [modify] https://crrev.com/d894710be3b56a15871376c5979615053dddcd70/net/url_request/url_request.cc [modify] https://crrev.com/d894710be3b56a15871376c5979615053dddcd70/net/url_request/url_request.h [modify] https://crrev.com/d894710be3b56a15871376c5979615053dddcd70/net/url_request/url_request_job.cc [modify] https://crrev.com/d894710be3b56a15871376c5979615053dddcd70/net/url_request/url_request_job.h [modify] https://crrev.com/d894710be3b56a15871376c5979615053dddcd70/net/url_request/url_request_unittest.cc [add] https://crrev.com/d894710be3b56a15871376c5979615053dddcd70/third_party/WebKit/LayoutTests/external/wpt/fetch/api/redirect/redirect-location-expected.txt [add] https://crrev.com/d894710be3b56a15871376c5979615053dddcd70/third_party/WebKit/LayoutTests/external/wpt/fetch/api/redirect/redirect-location-worker-expected.txt [modify] https://crrev.com/d894710be3b56a15871376c5979615053dddcd70/tools/metrics/histograms/enums.xml
,
Feb 14 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/da2cad242070f973962254885cbda03a5145cd6a commit da2cad242070f973962254885cbda03a5145cd6a Author: Matt Falkenhagen <falken@chromium.org> Date: Wed Feb 14 04:38:54 2018 Fetch API: Don't DCHECK on a 302 response with a Location header with empty value. Blink was failing a DCHECK when it expected a redirect to be taken because the request mode was 'follow' and the response had a redirect status code and Location header. However, Chrome's //net considers a Location header with an empty value to be the same as no Location header at all. See the comments on bug 810288 about net::HttpResponseHeaders::IsRedirect(). WPT tests are added. The spec is unclear (https://github.com/whatwg/url/issues/373) but seems to either say to fail with network error or enter a loop of self-redirects that will eventually hit the redirect limit and fail with network error. Chrome currently fails this test because it treats the Location as as non-existent, i.e., not a redirect response. Bug: 810288 ,707185 Change-Id: Iaca93976aaa697380b1542ab45d63c318f5050b0 Reviewed-on: https://chromium-review.googlesource.com/910753 Reviewed-by: Yutaka Hirano <yhirano@chromium.org> Reviewed-by: Tsuyoshi Horo <horo@chromium.org> Commit-Queue: Matt Falkenhagen <falken@chromium.org> Cr-Commit-Position: refs/heads/master@{#536644} [add] https://crrev.com/da2cad242070f973962254885cbda03a5145cd6a/third_party/WebKit/LayoutTests/external/wpt/fetch/api/redirect/redirect-empty-location-expected.txt [add] https://crrev.com/da2cad242070f973962254885cbda03a5145cd6a/third_party/WebKit/LayoutTests/external/wpt/fetch/api/redirect/redirect-empty-location-worker-expected.txt [add] https://crrev.com/da2cad242070f973962254885cbda03a5145cd6a/third_party/WebKit/LayoutTests/external/wpt/fetch/api/redirect/redirect-empty-location-worker.html [add] https://crrev.com/da2cad242070f973962254885cbda03a5145cd6a/third_party/WebKit/LayoutTests/external/wpt/fetch/api/redirect/redirect-empty-location.html [add] https://crrev.com/da2cad242070f973962254885cbda03a5145cd6a/third_party/WebKit/LayoutTests/external/wpt/fetch/api/redirect/redirect-empty-location.js [add] https://crrev.com/da2cad242070f973962254885cbda03a5145cd6a/third_party/WebKit/LayoutTests/external/wpt/fetch/api/resources/redirect-empty-location.py [modify] https://crrev.com/da2cad242070f973962254885cbda03a5145cd6a/third_party/WebKit/Source/core/fetch/FetchManager.cpp |
|
►
Sign in to add a comment |
|
Comment 1 by horo@chromium.org
, Apr 12 2017