Navigation via "OpenURL" code path doesn't preserve Content-Type of http request |
||||||||||
Issue description
Repro:
1. Prepare a page with a form using a non-default encoding.
1.a. Either use enctype attribute:
<form
method="POST"
enctype="multipart/form-data" ...
1.b. Or have a form that requires non-default encoding
(e.g. because the form needs to transfer files)
2. Force the form to be submitted via "OpenURL" code path
2.a. Either have the form target a remote frame
2.b. Or submit the frame in a way that forces ShouldFork
to return true (i.e. submit the frame from within an
extension pop-up as in issue 646261 ).
,
Sep 20 2016
+clamy@ In the fix proposal above, I am not moving anywhere the already existing |extra_headers| fields in StartNavigationParams and BeginNavigationParams. OTOH, I remember that in r395915, we moved HTTP POST body from StartNavigationParams to CommonNavigationParams. So - QUESTION: should we also consider moving |extra_headers| from StartNavigationParams and from BeginNavigationParams into CommonNavigationParams? Will that help with anything?
,
Sep 20 2016
+mmenke@ and davidben@ (because of an old r229930) When I tried to preserve all http request headers in OpenURL navigation path, then I saw that it breaks [3] some prerender browser tests, because 1) chrome::PrerenderManager [1] will avoid using prerendered page if the navigation uses_post OR contains *any* extra_headers and 2) blink::FrameLoader [2] will add "Upgrade-Insecure-Requests: 1" to all (I think) outgoing navigation requests. I am not sure what to do about this - I am brainstorming some silly ideas below, but I would really appreciate your feedback. I would vote for option #4 myself. - Treat "Upgrade-Insecure-Requests" in a special way: - Option #1: strip "Upgrade-Insecure-Requests" in content::GetWebURLRequestHeaders (since AFAICT it will get re-added anyway after being reissued by the new renderer) - Option #2: parse (!?) extra_headers in PrerenderManager::MaybeUsePrerenderedPage and ignore "Upgrade-Insecure-Requests" - Option #3: Move where "Upgrade-Insecure-Requests" gets added - if it gets added *after* a call to content::GetWebURLRequestHeaders, then we should be okay. - Option #4: Compare "prerenderer" and "new" extra_headers in PrerenderManager::MaybeUsePrerenderedPage. nasko@ pointed out to me that we can get the "prerendered" NavigationEntry via prerender_data->contents() ->prerender_contents()->GetController()->GetLastCommittedEntry(). To make this work we would also need to expose NavigationEntryImpl::extra_headers_ field via NavigationEntry. Hmmm... actually I tried this out a moment ago and old_headers were empty / didn't include "Upgrade-Insecure-Requests: 1"... :-( So maybe this needs to be combined with option #3... [1] https://cs.chromium.org/chromium/src/chrome/browser/prerender/prerender_manager.cc?type=cs&sq=package:chromium&rcl=1474384719&l=312 [2] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/FrameLoader.cpp?q=upgrade-insecure-requests&sq=package:chromium&dr=C&l=1630 [3] https://crrev.com/2355023002/#ps20001 failed tryjobs - for example: https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/300954/steps/browser_tests%20%28with%20patch%29%20on%20Ubuntu-12.04
,
Sep 20 2016
+mkwst@ Would there be any harm from: A) *always* sending "Upgrade-Insecure-Requests" header B) injecting the header in ResourceDispatcherHostImpl::BeginRequest, rather than in FrameLoader::upgradeInsecureRequest? (A) seems compatible with the spec at [1]. In particular, I don't understand why FrameLoader::upgradeInsecureRequest doesn't inject the header when |(resourceRequest.frameType() != WebURLRequest::FrameTypeNone)|; there is a comment next to this condition talking about "outgoing navigational requests" [2], but the link to "Fetch" spec is stale. (B) would postpone adding the header, so that it doesn't show up to OpenURLParams. [1] https://w3c.github.io/webappsec-upgrade-insecure-requests/#feature-detect
,
Sep 20 2016
,
Sep 21 2016
Regarding the NavParams change, I've been meaning to get OpenURL & BeginNavigation to use the same set of params. It seems to me that the information they need to transmit to the browser is very similar, so they should use the same structs. We talked with nasko@ about merging the two IPCs but decided that just having them share the same params should be good.
,
Sep 21 2016
What prerender really cares about is that the headers of the prerendered main frame request, and the headers of the navigation we use for the prerender are the same. The prerender IPC doesn't come with any extra headers, I believe, so we just check that the navigation didn't have any, either.
,
Sep 21 2016
RE: prerender really cares about is that the headers of the prerendered main frame request, and the headers of the navigation we use for the prerender are the same The problem is that "the request headers" are not well-defined / change over time. The request headers: 1) are empty in NavigationEntryImpl::extra_headers_ associated with PrerenderContents 2) include "Upgrade-Insecure-Requests" and "User-Agent" when ResourceDispatcherHostImpl::BeginRequest processes both requests (PrerenderContents + real WebContents) (looking at headers string from ResourceRequest::headers field) 3) include only "Upgrade-Insecure-Requests" when RenderFrameImpl::OpenURL is called when navigating the real WebContents (looking at headers string extracted via GetWebURLRequestHeaders). This makes it difficult to compare the headers of the prerendered main frame request, and the headers of the navigation we use for the prerender and decide if they are "the same". They *are* the same when looking inside ResourceDispatcherHostImpl::BeginRequest call, but are different when comparing #1 with #3 (and this difference means that after plumbing (https://crrev.com/2355023002) extra headers from RenderFrameImpl::OpenURL into content::OpenURLParams the PrerenderManager starts to incorrectly thinks that it cannot use the PrerenderContents). It seems to me that the root problem is that extra headers can be added by Blink on top of what NavigationEntry asks for (and therefore NavigationEntryImpl::extra_headers_ is out-of-sync wrt the actual headers seen by ResourceDispatcherHostImpl::BeginRequest and/or the headers seen by RenderFrameImpl::OpenURL). So - which of the following options from #c3 sound least bad / most reasonable?: Option #1: Maybe I should introduce a clone of GetWebURLRequestHeaders with extra filtering to remove headers that might be added by Blink and are not normally cared about in the browser (i.e. headers that one would not normally see in content::OpenURLParams, chrome::NavigateParams, content::NavigationController::LoadURLParams and content::NavigationEntryImpl::extra_headers_). This new function could be called something like GetWebURLRequestHeadersExceptOnesThatWillBeReaddedByBlink :->. In the initial implementation, I could filter out "Upgrade-Insecure-Requests" and "User-Agent". OTOH, this strikes me as fragile - there are no guarantees that the filtering removes *all* headers that browser (i.e. content::OpenURLParams) doesn't know/care about. Option #3: Move where "Upgrade-Insecure-Requests" gets added - if it gets added *after* a call to content::GetWebURLRequestHeaders made by content::RenderFrameImpl::OpenURL and/or content::RenderFrameProxy::navigate, then we should be okay. Alternatively, maybe "Upgrade-Insecure-Requests" should be automatically added (to NavigationEntryImpl::extra_headers_) for all navigation requests - this could be done inside content::NavigationController::CreateNavigationEntry() or content::NavigationControllerImpl::LoadURLWithParams(). WDYT? Also - are there other approaches / options to consider?
,
Sep 21 2016
Unfortunately, I'm just not familiar enough with navigation to comment intelligently here.
,
Sep 22 2016
,
Sep 26 2016
,
Oct 4 2016
Status update: we are iterating on the fix at https://crrev.com/2355023002 and right now we are leaning toward ignoring some http request headers in prerender_manager.cc rather than in content/child/web_url_request_util.cc. One risk that we've identified is that prior to the fix above, OpenURL IPC didn't include extra_headers field at all - this means that before the fix, PrerenderManager::MaybeUsePrerenderedPage in prerender_manager.cc would always see empty chrome::NavigateParams::extra_headers in case of renderer-initiated navigations (i.e. ones that go through the OpenURL IPC). So - after the fix the PrerenderManager::MaybeUsePrerenderedPage can refuse to use the prerendered contents more often than before the fix. This might not be that bad after all - in some (but not all) of these cases the server might be legitimately returning different contents because of the difference in the request headers. To help understand this risk, I've tried enumerating different kinds of HTTP request headers that are set in Blink via WebURLRequest::addHTTPHeaderField / setHTTPHeaderField, ResourceRequest::addHTTPHeaderField / setHTTPHeaderField. I think the risk is okay and shouldn't block the CL at https://crrev.com/2355023002. WDYT? Please shout if you think I got something incorrectly. Headers set via blink (via blink::WebURLRequest and/or blink::ResourceRequest): - Upgrade-Insecure-Requests: 1 - Okay to ignore in prerender, because AFAICT this header ia added to *all* "navigational requests" - FrameLoader::upgradeInsecureRequest will always add the header, unles the resource doesn't target a frame (i.e. resourceRequest.frameType() != WebURLRequest::FrameTypeNone). - Content-Type - Content-Transfer-Encoding - These are applicable only if the HTTP request has a body, and PrerenderManager::MaybeUsePrerenderedPage already returns false if params->uses_post - DPR - Width - Viewport-Width - DPR = Device Pixel Ratio - I don't see any callers of ClientHintsPreferences::setShouldSendDPR (and the the default [and only] constructor initializes m_shouldSendDPR to false). Similarily for the other 2 headers. - CSP: active - Never sent - see CSPDirectiveList::shouldSendCSPHeader - Accept-Encoding - If-Match - Range - Set from media/blink/resource_multibuffer_data_provider.cc Hopefully this is not a navigation request. - Accept: application/p-pnacl, */* - Okay to ignore - not a navigation request. - User-Agent - X-Requested-With - I think both headers can only be set from pepper (i.e. CreateWebURLRequest in content/renderer/pepper/url_request_info_util.cc seems to be the only caller of RequestExtraData::set_requested_with and RequestExtraData::set_custom_user_agent that can provide fresh values [rather than copy them from another request]) - Okay to ignore, because apparently this is added later than the OpenURL IPC (i.e. this is added in RenderFrameImpl::willSendRequest) - OTOH, if these headers are important then prerender might be making a wrong decision today (because it never sees those headers). - X-DevTools-Emulate-Network-Conditions-Client-Id - Can ignore IMO - this is done via DevTools, so shouldn't affect regular navigations and prerenders.
,
Oct 6 2016
mkwst@, I might try to move where "Upgrade-Insecure-Requests: 1" header is added (from the bowels of Blink into browser-side - possibly into ResourceDispatcherHost(Impl) as suggested by mmenke@ in https://crrev.com/2355023002/#msg53). Since AFAIK you've added this http(s) request header, I wanted to give you a heads up here. Please shout if you think moving where the header is added is a bad idea. Also +carlosk@ and +arthursonzogni@ - they had to move where "Upgrade-Insecure-Requests: 1" header is added to make it work better with PlzNavigate: r417949 and r404567.
,
Oct 6 2016
If need be, I can live with the check in the prerender code, it just seems lower overhead (In terms of IPCs, and redundant copies of headers) and simpler to have it in the RDH. Also means it covers other requests (It may not be added for <a download> requests, for instance).
,
Oct 7 2016
In regards to PlzNavigate, as long as the URL sent back to the browser in DidStartProvisionalLoad is already upgraded, it should be fine. As you are considering moving the header setting to an earlier moment I don't expect issues to arise.
,
Oct 11 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f097ee58fe6bb4c329999f03253bc4849cc50302 commit f097ee58fe6bb4c329999f03253bc4849cc50302 Author: lukasza <lukasza@chromium.org> Date: Tue Oct 11 23:17:52 2016 Preserving extra http request headers in OpenURL navigation path. This CL makes sure that extra http request headers (e.g. in case of HTTP POST, the Content-Type: multipart/form-data; boundary=... header) are preserved when navigation uses the "OpenURL" code path. BUG= 648648 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2355023002 Cr-Commit-Position: refs/heads/master@{#424588} [modify] https://crrev.com/f097ee58fe6bb4c329999f03253bc4849cc50302/chrome/browser/prerender/prerender_manager.cc [modify] https://crrev.com/f097ee58fe6bb4c329999f03253bc4849cc50302/content/browser/frame_host/navigator.h [modify] https://crrev.com/f097ee58fe6bb4c329999f03253bc4849cc50302/content/browser/frame_host/navigator_impl.cc [modify] https://crrev.com/f097ee58fe6bb4c329999f03253bc4849cc50302/content/browser/frame_host/navigator_impl.h [modify] https://crrev.com/f097ee58fe6bb4c329999f03253bc4849cc50302/content/browser/frame_host/render_frame_host_impl.cc [modify] https://crrev.com/f097ee58fe6bb4c329999f03253bc4849cc50302/content/browser/frame_host/render_frame_host_manager.cc [modify] https://crrev.com/f097ee58fe6bb4c329999f03253bc4849cc50302/content/browser/frame_host/render_frame_proxy_host.cc [modify] https://crrev.com/f097ee58fe6bb4c329999f03253bc4849cc50302/content/browser/security_exploit_browsertest.cc [modify] https://crrev.com/f097ee58fe6bb4c329999f03253bc4849cc50302/content/common/frame_messages.h [modify] https://crrev.com/f097ee58fe6bb4c329999f03253bc4849cc50302/content/renderer/render_frame_impl.cc [modify] https://crrev.com/f097ee58fe6bb4c329999f03253bc4849cc50302/content/renderer/render_frame_impl.h [modify] https://crrev.com/f097ee58fe6bb4c329999f03253bc4849cc50302/content/renderer/render_frame_proxy.cc [add] https://crrev.com/f097ee58fe6bb4c329999f03253bc4849cc50302/third_party/WebKit/LayoutTests/http/tests/navigation/form-with-enctype-targets-cross-site-frame-expected.txt [add] https://crrev.com/f097ee58fe6bb4c329999f03253bc4849cc50302/third_party/WebKit/LayoutTests/http/tests/navigation/form-with-enctype-targets-cross-site-frame.html
,
Oct 14 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/375969abb781814249ca4666b90a916f52b15fa2 commit 375969abb781814249ca4666b90a916f52b15fa2 Author: lukasza <lukasza@chromium.org> Date: Fri Oct 14 21:36:48 2016 Revert of Preserving Content-Type header from http request in OpenURL path. (patchset #13 id:240001 of https://codereview.chromium.org/2355023002/ ) Reason for revert: This CL caused a regression - https://crbug.com/655568 . Original issue's description: > Preserving extra http request headers in OpenURL navigation path. > > This CL makes sure that extra http request headers (e.g. in case of HTTP > POST, the Content-Type: multipart/form-data; boundary=... header) are > preserved when navigation uses the "OpenURL" code path. > > BUG= 648648 > CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation > > Committed: https://crrev.com/f097ee58fe6bb4c329999f03253bc4849cc50302 > Cr-Commit-Position: refs/heads/master@{#424588} TBR=creis@chromium.org,nasko@chromium.org,mmenke@chromium.org # Not skipping CQ checks because original CL landed more than 1 days ago. BUG= 648648 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2419093002 Cr-Commit-Position: refs/heads/master@{#425473} [modify] https://crrev.com/375969abb781814249ca4666b90a916f52b15fa2/chrome/browser/prerender/prerender_manager.cc [modify] https://crrev.com/375969abb781814249ca4666b90a916f52b15fa2/content/browser/frame_host/navigator.h [modify] https://crrev.com/375969abb781814249ca4666b90a916f52b15fa2/content/browser/frame_host/navigator_impl.cc [modify] https://crrev.com/375969abb781814249ca4666b90a916f52b15fa2/content/browser/frame_host/navigator_impl.h [modify] https://crrev.com/375969abb781814249ca4666b90a916f52b15fa2/content/browser/frame_host/render_frame_host_impl.cc [modify] https://crrev.com/375969abb781814249ca4666b90a916f52b15fa2/content/browser/frame_host/render_frame_host_manager.cc [modify] https://crrev.com/375969abb781814249ca4666b90a916f52b15fa2/content/browser/frame_host/render_frame_proxy_host.cc [modify] https://crrev.com/375969abb781814249ca4666b90a916f52b15fa2/content/browser/security_exploit_browsertest.cc [modify] https://crrev.com/375969abb781814249ca4666b90a916f52b15fa2/content/common/frame_messages.h [modify] https://crrev.com/375969abb781814249ca4666b90a916f52b15fa2/content/renderer/render_frame_impl.cc [modify] https://crrev.com/375969abb781814249ca4666b90a916f52b15fa2/content/renderer/render_frame_impl.h [modify] https://crrev.com/375969abb781814249ca4666b90a916f52b15fa2/content/renderer/render_frame_proxy.cc [delete] https://crrev.com/c42ef588592151c472460e402bc10f0c699f1b2f/third_party/WebKit/LayoutTests/http/tests/navigation/form-with-enctype-targets-cross-site-frame-expected.txt [delete] https://crrev.com/c42ef588592151c472460e402bc10f0c699f1b2f/third_party/WebKit/LayoutTests/http/tests/navigation/form-with-enctype-targets-cross-site-frame.html
,
Oct 14 2016
,
Oct 18 2016
|LoadURLParams::extra_headers| are no longer included with subresource requests after r425972 - this fixes the part of issue 656179 that was blocking the current bug. This also enables relanding the fix from #c16 (r424588) that got reverted because of issue 655568 . I'll give r425972 a little bit of bake time and then reland r424588.
,
Oct 19 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4a07d3a40109a3badac5463e780f7402d167dc30 commit 4a07d3a40109a3badac5463e780f7402d167dc30 Author: lukasza <lukasza@chromium.org> Date: Wed Oct 19 21:03:22 2016 Preserving extra http request headers in OpenURL navigation path [relanding]. This CL makes sure that extra http request headers (e.g. in case of HTTP POST, the Content-Type: multipart/form-data; boundary=... header) are preserved when navigation uses the "OpenURL" code path. BUG= 648648 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation TBR=creis@chromium.org,nasko@chromium.org,mmenke@chromium.org Review-Url: https://chromiumcodereview.appspot.com/2423233002 Cr-Commit-Position: refs/heads/master@{#426281} [modify] https://crrev.com/4a07d3a40109a3badac5463e780f7402d167dc30/chrome/browser/prerender/prerender_manager.cc [modify] https://crrev.com/4a07d3a40109a3badac5463e780f7402d167dc30/content/browser/frame_host/navigator.h [modify] https://crrev.com/4a07d3a40109a3badac5463e780f7402d167dc30/content/browser/frame_host/navigator_impl.cc [modify] https://crrev.com/4a07d3a40109a3badac5463e780f7402d167dc30/content/browser/frame_host/navigator_impl.h [modify] https://crrev.com/4a07d3a40109a3badac5463e780f7402d167dc30/content/browser/frame_host/render_frame_host_impl.cc [modify] https://crrev.com/4a07d3a40109a3badac5463e780f7402d167dc30/content/browser/frame_host/render_frame_host_manager.cc [modify] https://crrev.com/4a07d3a40109a3badac5463e780f7402d167dc30/content/browser/frame_host/render_frame_proxy_host.cc [modify] https://crrev.com/4a07d3a40109a3badac5463e780f7402d167dc30/content/browser/security_exploit_browsertest.cc [modify] https://crrev.com/4a07d3a40109a3badac5463e780f7402d167dc30/content/common/frame_messages.h [modify] https://crrev.com/4a07d3a40109a3badac5463e780f7402d167dc30/content/renderer/render_frame_impl.cc [modify] https://crrev.com/4a07d3a40109a3badac5463e780f7402d167dc30/content/renderer/render_frame_impl.h [modify] https://crrev.com/4a07d3a40109a3badac5463e780f7402d167dc30/content/renderer/render_frame_proxy.cc [add] https://crrev.com/4a07d3a40109a3badac5463e780f7402d167dc30/third_party/WebKit/LayoutTests/http/tests/navigation/form-with-enctype-targets-cross-site-frame-expected.txt [add] https://crrev.com/4a07d3a40109a3badac5463e780f7402d167dc30/third_party/WebKit/LayoutTests/http/tests/navigation/form-with-enctype-targets-cross-site-frame.html
,
Oct 28 2016
|
||||||||||
►
Sign in to add a comment |
||||||||||
Comment 1 by lukasza@chromium.org
, Sep 20 2016