PlzNavigate: make sure we don't execute XFO checks on downloads |
||||||
Issue descriptionhttps://codereview.chromium.org/2847443002 landed which enables proper pausing of the load in the IO stack when the navigation is a download in PlzNavigate. In particular, this means that downloads will now get cancelled by NavigationThrottles. We should check whether some of the NavigationThrottles should skip checks when the navigation is a download. Examples are AncestorNavigationThrottle and MixedContentThrottle. @mkwst: is that right that they should not execute?
,
May 5 2017
This caused the initial XFO patch to be reverted since it blocked some downloads, so this is blocking IMO.
,
May 5 2017
@mkwst: Maybe http://crbug.com/710748 is related? I don't know what is the correct behavior. I am interested in your answer to @clamy's question in #1. The bugfix would probably be as simple as adding an if ("navigation_handle_impl->is_download_") { "ignore" } in the AncestorThrottle:WillProcessResponse()
,
May 9 2017
,
May 10 2017
I agree with clamy@; we ought to ignore XFO for downloads. That's the behavior Chrome's had for a long time, and matches Firefox, Safari, and Edge.
,
May 10 2017
Okay, thanks mkwst@. I will work on this.
,
May 10 2017
Thanks Arthur for volunteering, but I had some time and put a CL together to save you some cycles to work on other bugs. https://codereview.chromium.org/2874933002
,
May 11 2017
Just wondering, should AncestorThrottle similarly ignore CSP frame-src for downloads? Looks like the renderer-side frame-src check in MaybeCheckCSP only applies if policy == kNavigationPolicyCurrentTab, so seems like it'd get skipped for kNavigationPolicyDownload.
,
May 11 2017
,
May 11 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1aa98d7cf548e42ef1a75828caa6d6831445172b commit 1aa98d7cf548e42ef1a75828caa6d6831445172b Author: nasko <nasko@chromium.org> Date: Thu May 11 02:00:45 2017 Don't enforce X-Frame-Options for downloads. This CL excludes downloads from X-Frame-Options checks with PlzNavigate. BUG= 717971 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2874933002 Cr-Commit-Position: refs/heads/master@{#470772} [modify] https://crrev.com/1aa98d7cf548e42ef1a75828caa6d6831445172b/content/browser/download/download_browsertest.cc [modify] https://crrev.com/1aa98d7cf548e42ef1a75828caa6d6831445172b/content/browser/frame_host/ancestor_throttle.cc [add] https://crrev.com/1aa98d7cf548e42ef1a75828caa6d6831445172b/content/test/data/download/download-with-xfo-deny.html [add] https://crrev.com/1aa98d7cf548e42ef1a75828caa6d6831445172b/content/test/data/download/download-with-xfo-deny.html.mock-http-headers
,
May 11 2017
Comment 9: For frame-ancestors, which is the equivalent of XFO in CSP, we enforce it in the renderer process, so it should behave identically with or without PlzNavigate. For frame-src and form-action, I believe they are different in intent. As a site operator, if I say frame-src: 'foo.com', I'm telling the browser to allow only going to foo.com. If we make downloads exempt from this, it allows the attacker to download files from arbitrary origins to the user's machine even when the site CSP say never go outside of my own domain. As such, I'm closing this bug a s fixed, but if you feel my reasoning is incorrect, feel free to reopen with more details. |
||||||
►
Sign in to add a comment |
||||||
Comment 1 by nasko@chromium.org
, May 4 2017