Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
212543005:
Do not copy reference fragments for overridden redirects. (Closed)
DescriptionDo not copy reference fragments for overridden redirects.
If a network delegates sets a redirect URL (in BeforeRequest or HeadersReceived), then it should get full
control over the target URL.
In particular, data:-URIs should remain unchanged because Chrome does not interpret # as a delimiter for reference fragments.
BUG=354653
TEST=net_unittests:
URLRequestTestHTTP.RedirectWithReferenceFragment:URLRequestTestHTTP.RedirectJobWithReferenceFragment (tests new behavior)
URLRequestTestHTTP.RedirectWithReferenceFragmentAndUnrelatedUnsafeUrl:URLRequestTestHTTP.Redirect302PreserveReferenceFragment (regression test)
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=261412
Patch Set 1 #
Total comments: 2
Patch Set 2 : Don't depend on allowed_unsafe_redirect_url_ #Patch Set 3 : Add URLRequestJob::IsRedirectFragmentModificationAllowed #
Total comments: 12
Patch Set 4 : Address mmenke's comments #
Total comments: 6
Patch Set 5 : edit comments, rename method to CopyFragmentOnRedirect #Patch Set 6 : call CopyFragmentOnRedirect (rename) #Patch Set 7 : Removed redundant OVERRIDE #Patch Set 8 : rebase #
Messages
Total messages: 42 (0 generated)
https://codereview.chromium.org/212543005/diff/1/net/url_request/url_request_... File net/url_request/url_request_http_job.cc (right): https://codereview.chromium.org/212543005/diff/1/net/url_request/url_request_... net/url_request/url_request_http_job.cc:1049: !allowed_unsafe_redirect_url_.is_valid()) { This partially breaks the reason for having allowed_unsafe_redirect_url_ separate from the headers. It should be compared to location. I also think this is a completely non-obvious behavior, not at all implied by "allowed_unsafe_redirect_url_". I don't currently have a suggestion to fix that, but I really don't want confusion over the API to lead to obscure future bugs, 5 years down the road. https://codereview.chromium.org/212543005/diff/1/net/url_request/url_request_... File net/url_request/url_request_job.cc (left): https://codereview.chromium.org/212543005/diff/1/net/url_request/url_request_... net/url_request/url_request_job.cc:334: const GURL& url = request_->url(); This breaks URLRequestRedirectJobs, I believe. Or any other job type that results in a redirect (Not sure there currently are any, but this CL shouldn't depend on that).
I've reverted the change, and implemented the fix as an additional SchemeIsHTTPOrHTTPS() check. Are you okay with this? At the moment, only HTTP-redirects to http, https and ftp schemes are allowed by default. With this change, the fragment will not be re-attached to FTP URLs any more. I think that this is a safe behavioral change, but if you feel that 100% backwards-compatibility is desired, then adding another check for FTP URLs is trivial.
On 2014/03/27 18:42:25, robwu wrote: > I've reverted the change, and implemented the fix as an additional > SchemeIsHTTPOrHTTPS() check. > > Are you okay with this? > At the moment, only HTTP-redirects to http, https and ftp schemes are allowed by > default. > With this change, the fragment will not be re-attached to FTP URLs any more. > I think that this is a safe behavioral change, but if you feel that 100% > backwards-compatibility is desired, then adding another check for FTP URLs is > trivial. Can't base-64 encoded URLs have a reference fragment? chrome-extension:// and chrome:// ones can, too. Not sure anything can redirect to the second, but I'm assuming extensions can redirect to the first.
On 2014/03/27 18:49:01, mmenke wrote: > On 2014/03/27 18:42:25, robwu wrote: > > I've reverted the change, and implemented the fix as an additional > > SchemeIsHTTPOrHTTPS() check. > > > > Are you okay with this? > > At the moment, only HTTP-redirects to http, https and ftp schemes are allowed > by > > default. > > With this change, the fragment will not be re-attached to FTP URLs any more. > > I think that this is a safe behavioral change, but if you feel that 100% > > backwards-compatibility is desired, then adding another check for FTP URLs is > > trivial. > > Can't base-64 encoded URLs have a reference fragment? chrome-extension:// and > chrome:// ones can, too. Not sure anything can redirect to the second, but I'm > assuming extensions can redirect to the first. *base-64 data urls
On 2014/03/27 18:49:20, mmenke wrote: > On 2014/03/27 18:49:01, mmenke wrote: > > On 2014/03/27 18:42:25, robwu wrote: > > > I've reverted the change, and implemented the fix as an additional > > > SchemeIsHTTPOrHTTPS() check. > > > > > > Are you okay with this? > > > At the moment, only HTTP-redirects to http, https and ftp schemes are > allowed > > by > > > default. > > > With this change, the fragment will not be re-attached to FTP URLs any more. > > > I think that this is a safe behavioral change, but if you feel that 100% > > > backwards-compatibility is desired, then adding another check for FTP URLs > is > > > trivial. > > > > Can't base-64 encoded URLs have a reference fragment? chrome-extension:// and > > chrome:// ones can, too. Not sure anything can redirect to the second, but > I'm > > assuming extensions can redirect to the first. > > *base-64 data urls Oh, right...chrome-extensions:// is why you want this... And they can only currently modify the URL before the request starts, so they'd have to copy the hash themselves at that point. Hrm...
On 2014/03/27 18:49:01, mmenke wrote: > On 2014/03/27 18:42:25, robwu wrote: > > I've reverted the change, and implemented the fix as an additional > > SchemeIsHTTPOrHTTPS() check. > > > > Are you okay with this? > > At the moment, only HTTP-redirects to http, https and ftp schemes are allowed > by > > default. > > With this change, the fragment will not be re-attached to FTP URLs any more. > > I think that this is a safe behavioral change, but if you feel that 100% > > backwards-compatibility is desired, then adding another check for FTP URLs is > > trivial. > > Can't base-64 data URLs have a reference fragment? chrome-extension:// and > chrome:// ones can, too. Not sure anything can redirect to the second, but I'm > assuming extensions can redirect to the first. Extensions are notified of the full URL, including reference fragment. If the extension author really wants to maintain the fragment, they can append the reference fragment to the redirection target. At the moment it is impossible for an extension to reliably redirect to an URL without any reference fragment. That's what I'm trying to fix (see associated bug report for more details).
On 2014/03/27 18:51:29, mmenke wrote: > On 2014/03/27 18:49:20, mmenke wrote: > > On 2014/03/27 18:49:01, mmenke wrote: > > > On 2014/03/27 18:42:25, robwu wrote: > > > > I've reverted the change, and implemented the fix as an additional > > > > SchemeIsHTTPOrHTTPS() check. > > > > > > > > Are you okay with this? > > > > At the moment, only HTTP-redirects to http, https and ftp schemes are > > allowed > > > by > > > > default. > > > > With this change, the fragment will not be re-attached to FTP URLs any > more. > > > > I think that this is a safe behavioral change, but if you feel that 100% > > > > backwards-compatibility is desired, then adding another check for FTP URLs > > is > > > > trivial. > > > > > > Can't base-64 encoded URLs have a reference fragment? chrome-extension:// > and > > > chrome:// ones can, too. Not sure anything can redirect to the second, but > > I'm > > > assuming extensions can redirect to the first. > > > > *base-64 data urls > > Oh, right...chrome-extensions:// is why you want this... And they can only > currently modify the URL before the request starts, so they'd have to copy the > hash themselves at that point. Hrm... Data URLs in base64 can still have references, though (Or at least I assume they can)
I was not happy with just excluding http(s) schemes, so I changed the implementation again. Now I've added a new method URLRequestJob::IsRedirectFragmentModificationAllowed, which can be overridden by subclasses to signal that the the fragment on the redirect URL can be changed. For URLRequestHttpJob, I'm comparing it with |allowed_unsafe_redirect_url_| (with a few lines of comments to avoid any confusion). In URLRequestRedirectJob, I'm just returning false, because I assume that the ones who instantiate the class want full control over the URL. Besides the unit tests, there are currently three uses of URLRequestRedirectJob: - Enforcing HTST. - Protocol handlers (JavaScript's navigator.registerProtocolHandler()). - Redirecting after receiving a redirect URL from the network delegate. The first two take the original URL, apply a string transformation and use the result. In other words, they already preserve the fragment, so the change has no observable difference. The behavior for the last case changes, but that's exactly the bug I want to fix (issue 354653).
On 2014/03/28 11:56:36, robwu wrote: > I was not happy with just excluding http(s) schemes, so I changed the > implementation again. > Now I've added a new method > URLRequestJob::IsRedirectFragmentModificationAllowed, which can > be overridden by subclasses to signal that the the fragment on the redirect URL > can be changed. > > For URLRequestHttpJob, I'm comparing it with |allowed_unsafe_redirect_url_| > (with a few lines of > comments to avoid any confusion). > > In URLRequestRedirectJob, I'm just returning false, because I assume that the > ones who instantiate the class want full control over the URL. Besides the unit > tests, there are currently three uses of URLRequestRedirectJob: > - Enforcing HTST. > - Protocol handlers (JavaScript's navigator.registerProtocolHandler()). > - Redirecting after receiving a redirect URL from the network delegate. > > The first two take the original URL, apply a string transformation and use the > result. In other words, they already preserve the fragment, so the change has no > observable difference. > The behavior for the last case changes, but that's exactly the bug I want to fix > (issue 354653). Sorry (yet again) for slowness. I want to double-check that this doesn't break either of the two RedirectJob cases, and hope to get back to you later today.
I think this is reasonable. More complexity than I'd like, but I don't see a better alternative. Could you also update the issue description to include the changed behavior when overriding the URL on start? Thanks! https://codereview.chromium.org/212543005/diff/40001/net/url_request/url_requ... File net/url_request/url_request_http_job.cc (right): https://codereview.chromium.org/212543005/diff/40001/net/url_request/url_requ... net/url_request/url_request_http_job.cc:1045: allowed_unsafe_redirect_url_ != location; The NetworkDelegate description should include this behavior. https://codereview.chromium.org/212543005/diff/40001/net/url_request/url_requ... net/url_request/url_request_http_job.cc:1045: allowed_unsafe_redirect_url_ != location; Hrm...I can't think of a reasonable name that implies this behavior as well as the unsafe behavior... allowed_unsafe_redirect_url_with_fragment_ works, but I think it's going a bit overboard. Open to suggestions. If you don't have one, keeping this name is fine. https://codereview.chromium.org/212543005/diff/40001/net/url_request/url_requ... net/url_request/url_request_http_job.cc:1057: return allowed_unsafe_redirect_url_ == location; Think this should be: if (allowed_unsafe_redirect_url_.is_valid() && allowed_unsafe_redirect_url_ == location) { return true; } (i.e. if two external objects are fighting over the redirect, and the winning one is not the one that set allowed_unsafe_redirect_url_, use normal logic here) https://codereview.chromium.org/212543005/diff/40001/net/url_request/url_requ... File net/url_request/url_request_job.h (right): https://codereview.chromium.org/212543005/diff/40001/net/url_request/url_requ... net/url_request/url_request_job.h:166: const GURL& location) OVERRIDE; nit: const https://codereview.chromium.org/212543005/diff/40001/net/url_request/url_requ... File net/url_request/url_request_redirect_job.cc (right): https://codereview.chromium.org/212543005/diff/40001/net/url_request/url_requ... net/url_request/url_request_redirect_job.cc:50: // including the reference fragment part of the URL. This behavior should be documented in NetworkDelegate::OnBeforeURLRequest https://codereview.chromium.org/212543005/diff/40001/net/url_request/url_requ... File net/url_request/url_request_unittest.cc (right): https://codereview.chromium.org/212543005/diff/40001/net/url_request/url_requ... net/url_request/url_request_unittest.cc:5411: } Suggest we keep the old "RedirectWithReferenceFragment" test from before (Possibly with a new name), as well as having these two tests.
Hi mmenke, I've edited the description and addressed your other comments. PTAL. https://codereview.chromium.org/212543005/diff/40001/net/url_request/url_requ... File net/url_request/url_request_http_job.cc (right): https://codereview.chromium.org/212543005/diff/40001/net/url_request/url_requ... net/url_request/url_request_http_job.cc:1045: allowed_unsafe_redirect_url_ != location; On 2014/03/31 20:45:49, mmenke wrote: > The NetworkDelegate description should include this behavior. Done. https://codereview.chromium.org/212543005/diff/40001/net/url_request/url_requ... net/url_request/url_request_http_job.cc:1045: allowed_unsafe_redirect_url_ != location; On 2014/03/31 20:45:49, mmenke wrote: > Hrm...I can't think of a reasonable name that implies this behavior as well as > the unsafe behavior... allowed_unsafe_redirect_url_with_fragment_ works, but I > think it's going a bit overboard. Open to suggestions. If you don't have one, > keeping this name is fine. |redirect_url_suggested_by_network_delegate_| as member name and |suggested_redirect_url| as parameter in delegates. WDYT? https://codereview.chromium.org/212543005/diff/40001/net/url_request/url_requ... net/url_request/url_request_http_job.cc:1057: return allowed_unsafe_redirect_url_ == location; On 2014/03/31 20:45:49, mmenke wrote: > Think this should be: > > if (allowed_unsafe_redirect_url_.is_valid() && > allowed_unsafe_redirect_url_ == location) { > return true; > } > > (i.e. if two external objects are fighting over the redirect, and the winning > one is not the one that set allowed_unsafe_redirect_url_, use normal logic here) Done. https://codereview.chromium.org/212543005/diff/40001/net/url_request/url_requ... File net/url_request/url_request_job.h (right): https://codereview.chromium.org/212543005/diff/40001/net/url_request/url_requ... net/url_request/url_request_job.h:166: const GURL& location) OVERRIDE; On 2014/03/31 20:45:49, mmenke wrote: > nit: const Done. https://codereview.chromium.org/212543005/diff/40001/net/url_request/url_requ... File net/url_request/url_request_redirect_job.cc (right): https://codereview.chromium.org/212543005/diff/40001/net/url_request/url_requ... net/url_request/url_request_redirect_job.cc:50: // including the reference fragment part of the URL. On 2014/03/31 20:45:49, mmenke wrote: > This behavior should be documented in NetworkDelegate::OnBeforeURLRequest Done. https://codereview.chromium.org/212543005/diff/40001/net/url_request/url_requ... File net/url_request/url_request_unittest.cc (right): https://codereview.chromium.org/212543005/diff/40001/net/url_request/url_requ... net/url_request/url_request_unittest.cc:5411: } On 2014/03/31 20:45:49, mmenke wrote: > Suggest we keep the old "RedirectWithReferenceFragment" test from before > (Possibly with a new name), as well as having these two tests. Done.
LGTM! (And good job on the unit tests - nice and complete) https://codereview.chromium.org/212543005/diff/60001/net/url_request/url_requ... File net/url_request/url_request_http_job.h (right): https://codereview.chromium.org/212543005/diff/60001/net/url_request/url_requ... net/url_request/url_request_http_job.h:262: // Reference fragment are not attached to the redirect URL if this URL is nit: fragment->fragments (Or "The reference fragment is not...") https://codereview.chromium.org/212543005/diff/60001/net/url_request/url_requ... File net/url_request/url_request_job.h (right): https://codereview.chromium.org/212543005/diff/60001/net/url_request/url_requ... net/url_request/url_request_job.h:162: // reference fragment. nit: Also mention that it's only copied if location does not have it's own fragment. https://codereview.chromium.org/212543005/diff/60001/net/url_request/url_requ... net/url_request/url_request_job.h:166: const GURL& location) const OVERRIDE; nit: Think "CopyFragmentOnRedirect" may be a little clearer.
https://codereview.chromium.org/212543005/diff/60001/net/url_request/url_requ... File net/url_request/url_request_http_job.h (right): https://codereview.chromium.org/212543005/diff/60001/net/url_request/url_requ... net/url_request/url_request_http_job.h:262: // Reference fragment are not attached to the redirect URL if this URL is On 2014/04/01 20:26:51, mmenke wrote: > nit: fragment->fragments (Or "The reference fragment is not...") Done. https://codereview.chromium.org/212543005/diff/60001/net/url_request/url_requ... File net/url_request/url_request_job.h (right): https://codereview.chromium.org/212543005/diff/60001/net/url_request/url_requ... net/url_request/url_request_job.h:162: // reference fragment. On 2014/04/01 20:26:51, mmenke wrote: > nit: Also mention that it's only copied if location does not have it's own > fragment. Done. https://codereview.chromium.org/212543005/diff/60001/net/url_request/url_requ... net/url_request/url_request_job.h:166: const GURL& location) const OVERRIDE; On 2014/04/01 20:26:51, mmenke wrote: > nit: Think "CopyFragmentOnRedirect" may be a little clearer. Done. Now the method signatures fit on one line as well :)
The CQ bit was checked by rob@robwu.nl
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rob@robwu.nl/212543005/80001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rob@robwu.nl/212543005/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on win_chromium_compile_dbg
The CQ bit was checked by rob@robwu.nl
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rob@robwu.nl/212543005/100001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_rel for step(s) base_unittests, browser_tests, interactive_ui_tests, net_unittests, unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
The CQ bit was checked by rob@robwu.nl
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rob@robwu.nl/212543005/120001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on android_aosp for step(s) compile http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_ao...
The CQ bit was checked by rob@robwu.nl
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rob@robwu.nl/212543005/120001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on android_aosp for step(s) compile http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_ao...
The CQ bit was checked by rob@robwu.nl
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rob@robwu.nl/212543005/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_chromeos_rel
The CQ bit was checked by rob@robwu.nl
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rob@robwu.nl/212543005/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_chromeos_rel
The CQ bit was checked by rob@robwu.nl
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rob@robwu.nl/212543005/140001
Message was sent while issue was closed.
Change committed as 261412 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
