Adding Omnibox component, which is a more likely suspect here.

Broadening labels.  I can't repro it on dev...But I get a different issue instead.  The request is a post, but origin is set to null, which causes it to response with "forbidden".  I suspect these are different manifestations of the same underlying problem.

Since the network stack doesn't set the method or the origin, only modifies them on redirects, and there's no redirect in the logs here, removing the network label.

Adding search and navigation labels.

The original bug sounds like a duplicate of  issue 344348 , which was fixed by lukasza@ in M53.

lukasza@: Can you look at mmenke's comment 2 to see if there's something else wrong in this case?  (mmenke@, which version were you testing on?)

Sorry, should have mentioned that.  I'm on Windows Beta - 53.0.2785.46.

mmenke@, could you please clarify what you mean in:

> The request is a post, but origin is set to null, which causes it to response with "forbidden".

Q1: Can you help me understand what you mean by "origin is set to null"?  I looked at //net/url_request/url_request.h and the only origin-related field there is |initiator_|.  Is this what you are referring to?  There is also CORS-related "Origin" HTTP header - is this what you are referring to?

Q2: Can you help me understand what you mean by "causes it to response with "forbidden""?  "it" = URLRequest (or the external http server?)?


At any rate, RenderFrameImpl::OpenURL / FrameHostMsg_OpenURL_Params doesn't cover 1) initiator and 2) extra http headers - this would explain why they are getting dropped when POST request goes through this path.  Furthermore there are more things from WebURLRequest that are not covered by FrameHostMsg_OpenURL_Params.

Q3: Should we worry about other things that are present in WebURLRequest but missing from FrameHostMsg_OpenURL_Params? (e.g. attachedCredential or LoFiState?)  And if yes, then should we keep adding the missing things into FrameHostMsg_OpenURL_Params vs reuse ResourceRequest struct?

The CORS origin header is null (Meaning it's set, and the value is the string "null"), which causes Duck Duck Go to respond with an HTTP status code of 413.  413 is the HTTP status code for "forbidden".

I'm assuming Q3 is aimed at cries rather than me - I'm not too familiar with navigation.

*creis, rather.  Sorry about that.  :)

Also, Canary has the same issue as beta.  Upping the priority - this seems a significant issue that we really should get a fix in for M54, or, better, M53, if we can.

Thanks for the explanation - I understand the immediate issue and can repro via http/tests/navigation/form-targets-cross-site-frame-post.html layout test after adding dumping of http headers to http/tests/navigation/resources/form-target.pl.

Let me first focus on fixing the immediate issue and we can discuss later (e.g. during the code review?) whether there is a broader issue (not passing other request properties via OpenURL code path).

So far, I've verified that RenderFrameProxy::navigate sees a correct |request.requestorOrigin()|, but since this part of the web request is not covered by the OpenURL code path, it gets dropped on the floor... :-/

jww@, could you please help me figure out the desired behavior related to CORS?
(I am also adding mkwst@ to CC, but he is OOO for a few more days AFAIK)

Can you help me understand behavior of submitting a form with a cross-site target (in default case / without OOPIFs / without --site-per-process / when not targeting a default search engine):

Q1: Why no Origin http header is sent with GET requests, but Origin http header is sent with POST requests?
What is the desired behavior here? (sent Origin http header both for GET and POST?  for neither GET nor POST?  note that these are form submissions / navigation requests - these are not XHRs;  also note that the CORS spec doesn't seem to distinguish between these 2 http methods)

Q2: Why the POST request succeeds even though the http response doesn't include Access-Control-Allow-Origin http header?
If current behavior is okay, then what is the point of sending Origin http header in this case - isn't it redundant with Referer header?
Stepping back, what is the desired behavior here?


I have a CL @ https://codereview.chromium.org/2225383003 for improving layout tests coverage, so the tests can detect the difference between default and --site-per-process (OOPIF) case.  I've left some comments in https://codereview.chromium.org/2225383003/#ps1 that help illustrate and highlight issues discussed below.

Using the improved tests, I can repro the problem with POST:

    "Origin" HTTP header is incorrectly set to "null" when
    1) submiting a form with a cross-site target and
    2) --site-per-process is used, and
    3) form method = "POST".

This behavior doesn't repro when OOPIFs are not present (which is consistent with the observation from #c2 that the regular repro happens only if the form target is a default search engine - this condition forces OOPIFs) - without --site-per-process the "Origin" HTTP header is present with a reasonable value of "http://127.0.0.1:8000".


I am surprised that I cannot repro the problem when form method = "GET" - in this case, even without --site-per-process flag, the "Origin" HTTP header is not sent with the request.  I've double checked that form method is the only difference between the two tests I am using:

    .../LayoutTests/http/tests/navigation$ diff form-targets-cross-site-frame-get.html \
                                                form-targets-cross-site-frame-post.html 
    28c28
    <   method="GET"
    ---
    >   method="POST"


Looking at the CORS spec (https://www.w3.org/TR/cors) I think that GET and POST method should behave in the same way (wrt CORS and Origin http header):

1. According to section "7.1 Cross-Origin Request", we need to follow "simple cross-origin request algorithm", because the following conditions are met:

    1.a. The request method is a simple method and the force preflight flag is unset.

         lukasza> A method is said to be a simple method if it is a case-sensitive match
         lukasza> for one of the following: GET, HEAD, POST

    1.b. Each of the author request headers is a simple header or author request headers is empty.

         lukasza> I am not sure what "author request headers" means,
         lukasza> but AFAIK it is empty in case of a form submit?

2. Since there is no redirect in the 2 layout tests, my just need to perform a "resource sharing check" (according to section "7.1.4 Simple Cross-Origin Request").

3. "7.2 Resource Sharing Check" section asks to do the following: "If the response includes zero or more than one Access-Control-Allow-Origin header values, return fail and terminate this algorithm."

         lukasza> I've verified that the http server used by layout tests doesn't include
         lukasza> Access-Control-Allow-Origin http header in the http response (verification
         lukasza> done via devtools when manually testing in local build of chrome after
         lukasza> clicking "Submit" on the form from form-targets-cross-site-frame-post.html).

         lukasza> POST /navigation/resources/form-target.pl HTTP/1.1
         lukasza> Host: localhost:8000
         lukasza> Connection: keep-alive
         lukasza> Content-Length: 21
         lukasza> Cache-Control: max-age=0
         lukasza> Origin: http://127.0.0.1:8000
         lukasza> Upgrade-Insecure-Requests: 1
         lukasza> User-Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/54.0.2821.0 Safari/537.36
         lukasza> Content-Type: application/x-www-form-urlencoded
         lukasza> Accept: text/html,application/xhtml+xml,application/xml;q=0.9,image/webp,*/*;q=0.8
         lukasza> Referer: http://127.0.0.1:8000/navigation/form-targets-cross-site-frame-post.html
         lukasza> Accept-Encoding: gzip, deflate, br
         lukasza> Accept-Language: en-US,en;q=0.8

         lukasza> HTTP/1.1 200 OK
         lukasza> Date: Tue, 09 Aug 2016 16:36:15 GMT
         lukasza> Server: Apache/2.4.10 (Ubuntu)
         lukasza> Keep-Alive: timeout=15, max=100
         lukasza> Connection: Keep-Alive
         lukasza> Transfer-Encoding: chunked
         lukasza> Content-Type: text/html

+clamy@, as PlzNavigate also cares about replicating full request information to the browser

1) the current bug shows that FrameHostMsg_OpenURL_Params doesn't preserve blink::WebURLRequest::requestorOrigin but there are possibly more properties of blink::WebURLRequest that should be audited and potentially preserved by covering them via FrameHostMsg_OpenURL_Params - for example:

- firstPartyForCookies
- requestorOrigin [this bug] (and maybe addHTTPOriginIfNeeded?  is this separate?)
- bool allowStoredCredentials + attachedCredential
- WebCachePolicy
- extra http headers set via setHTTPHeaderField
- WebReferrerPolicy [otoh, maybe this is already covered by FrameHostMsg_OpenURL_Params::referrer?]
- int appCacheHostID + bool shouldResetAppCache
- bool downloadToFile
- bool skipServiceWorker
- LoFiState
- Priority
- uiStartTime

2) there is also a question of whether FrameHostMsg_OpenURL_Params should be expanded further (e.g. to cover requestorOrigin) VS whether we should reuse PlzNavigate structures which already cover some (but AFAICT not all) of the things above:

- CommonNavigationParams already covers:
  - allow_download
  - ui_timestamp
  - lofi_state
- BeginNavigationParams already covers:
  - headers (not sure if this covers Origin)
  - skip_service_worker

FWIW, I've checked that duckduckgo.com accepts HTTP POST requests with

1) no Origin header
   (no "Access-Control-Allow-Origin" header in http response in this case)

2) Origin header set to https://duckduckgo.com
   (in this case "Access-Control-Allow-Origin: https://duckduckgo.com" is in the http response)

and replies to HTTP POST requests with HTTP/1.1 403 Forbidden to

3) Origin header set to "null" (the problem pointed out in #c2 / the problem this bug is tracking right now).


The observations above:

1) give extra confirmation that sending of "Origin: null" is the root cause of the trouble here.

2) point out that duckduckgo.com will work regardless of the answer to the 2 questions asked in #c13 above (i.e. it will continue to work if we stop sending Origin with POST requests and/or if we start enforcing presence of Access-Control-Allow-Origin header in the responses).

FWIW, I've checked what Firefox (47.0) does when submitting a form from the navigation/form-targets-cross-site-frame-post.html layout test.  It turned out that unlike Chromium, Firefox did not include Origin header in the http request:

Host: localhost:8000
User-Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:47.0) Gecko/20100101 Firefox/47.0
Accept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8
Accept-Language: en-US,en;q=0.5
Accept-Encoding: gzip, deflate
Referer: http://127.0.0.1:8000/navigation/form-targets-cross-site-frame-post.html
Connection: keep-alive

I think sending an Origin for all POSTs is a new-ish part of the fetch spec.  "The `Origin` header is a version of the `Referer` [sic] header that does not reveal a path. It is used for all HTTP fetches whose CORS flag is set as well as those where request's method is `POST`. Due to compatibility constraints it is not included in all fetches."

We think this probably isn't blocking for --isolate-extensions.

+estark@ who is our resident Referer header expert, in case she touched anything that might have affected this.

RE: #c18

Thanks mmenke@ for the explanation.  The comment from #c18 makes me more confident that OOPIF-less behavior is correct (i.e. that it is okay to 1) include Origin header with POST but not GET navigation requests and 2) that such requests are not part of CORS protocol and therefore responses do not necessarily need  header).

Unless anybody objects, I think questions I raised in #c13 (about desired behavior wrt GET-vs-POST and Access-Control-Allow-Origin http response header) can be ignored - we already have an answer (thanks!).



RE: #c20 by jww@ and #c9 by mmenke@

jww@ / mmenke@: This is not a recent regression.  In fact, quite recently POST requests would be flattened into GET requests in case of OpenURL code path (so - in case of a default search engine).  Gory details are available in  issue 344348 .  Recent changes (i.e. r400442) made it slightly less broken (we don't drop the POST body or change the HTTP method anymore) but left some things still broken (i.e. the Origin header, possibly other things) in the OpenURL code path.

Therefore:

- I think the priority of the bug can be lowered

- I don't think things estark@ or somebody else touched might have affected this (i.e. AFAIK OpenURL code path never covered requestorOrigin).



I talked with creis@ about history of FrameHostMsg_OpenURL_Params and we think that rather than patching up FrameHostMsg_OpenURL_Params (e.g. by adding requestorOrigin) we should instead try to use a single struct for passing navigation params to the browser.  Therefore I will try to make FrameHostMsg_OpenURL_Params = content::CommonNavigationParams + content::BrowserNavigationParams + //chrome-layer info (e.g. window disposition).

- This will require exposing content::CommonNavigationParams + content::BrowserNavigationParams via //content's public API.  I think this is doable.

- This will mean that in the long-term we only have to add missing things (e.g. things listed in #c14) to content::CommonNavigationParams or content::BrowserNavigationParams (and FrameHostMsg_OpenURL_Params will automatically get them via these structs).

- I need to figure out if requestorOrigin can be covered by the already existing content::BeginNavigationParams::headers.

- This is probably more work than just plumbing requestorOrigin through - I am not sure if this will be done before the M54 branch point.

Yes, mmenke@'s analysis sounds correct to me. It is generally not sent as part of simple fetches, although notably that means that I think it means it would be sent on a Get with a custom header, for example.

I'm a bit confused at this point about the repro of the bug. It sounds to me like it only occurs when --site-per-process is enabled, correct? If is, then I definitely agree that the priority should be lowered. If not, then I'm suspicious that it has been sitting around for such a long time without us noticing, but I suppose it's possible.

I think it happens whenever there's a POST for a browser-handled navigation (set-per-process does it, but so can navigating on a page marked as your default search engine, which was the original repro)

Ah, got it, thanks!

Also, if you're curious, here's the code, with comment, that doesn't add the Origin header for GET requests for privacy reasons:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/network/ResourceRequest.cpp?sq=package:chromium&rcl=1470626560&l=274

If 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/82fbd88a31485e5bca1c0938724ed71026ad3f8d

commit 82fbd88a31485e5bca1c0938724ed71026ad3f8d
Author: lukasza <lukasza@chromium.org>
Date: Wed Aug 10 15:24:47 2016

Layout tests for presence of Origin header in cross-site form target requests.

This CL primarily adds dumping of HTTP response headers to
navigation/resources/form-target.pl and adds appropriate
expectations to layout tests that depend on this URI.

This CL also makes a few extra minor tweaks:

- Using http instead of https scheme in
  navigation/form-targets-cross-site-frame-get.html
  and navigation/form-targets-cross-site-frame-post.html
  (this makes it easier to use these URIs in manual repro
  attempts made with a regular browser)

- Adding a test expectation to FlagExpectations/site-per-process
  to account for  bug 635400 

- Changing the initial URI of the cross-site frame that is the
  target of the form in
  navigation/form-targets-cross-site-frame-get.html
  and navigation/form-targets-cross-site-frame-post.html
  I think the test is easier to understand when the initial URI
  and form's target URI are different.

BUG= 635400 

Review-Url: https://codereview.chromium.org/2225383003
Cr-Commit-Position: refs/heads/master@{#411044}

[modify] https://crrev.com/82fbd88a31485e5bca1c0938724ed71026ad3f8d/third_party/WebKit/LayoutTests/FlagExpectations/site-per-process
[modify] https://crrev.com/82fbd88a31485e5bca1c0938724ed71026ad3f8d/third_party/WebKit/LayoutTests/http/tests/navigation/form-targets-cross-site-frame-get-expected.txt
[modify] https://crrev.com/82fbd88a31485e5bca1c0938724ed71026ad3f8d/third_party/WebKit/LayoutTests/http/tests/navigation/form-targets-cross-site-frame-get.html
[modify] https://crrev.com/82fbd88a31485e5bca1c0938724ed71026ad3f8d/third_party/WebKit/LayoutTests/http/tests/navigation/form-targets-cross-site-frame-post-expected.txt
[modify] https://crrev.com/82fbd88a31485e5bca1c0938724ed71026ad3f8d/third_party/WebKit/LayoutTests/http/tests/navigation/form-targets-cross-site-frame-post.html
[modify] https://crrev.com/82fbd88a31485e5bca1c0938724ed71026ad3f8d/third_party/WebKit/LayoutTests/http/tests/navigation/post-basic-expected.txt
[modify] https://crrev.com/82fbd88a31485e5bca1c0938724ed71026ad3f8d/third_party/WebKit/LayoutTests/http/tests/navigation/post-frames-expected.txt
[modify] https://crrev.com/82fbd88a31485e5bca1c0938724ed71026ad3f8d/third_party/WebKit/LayoutTests/http/tests/navigation/post-frames-goback1-expected.txt
[modify] https://crrev.com/82fbd88a31485e5bca1c0938724ed71026ad3f8d/third_party/WebKit/LayoutTests/http/tests/navigation/post-goback1-expected.txt
[modify] https://crrev.com/82fbd88a31485e5bca1c0938724ed71026ad3f8d/third_party/WebKit/LayoutTests/http/tests/navigation/resources/form-target.pl
[modify] https://crrev.com/82fbd88a31485e5bca1c0938724ed71026ad3f8d/third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/1.1/form-action-src-allowed-expected.txt
[modify] https://crrev.com/82fbd88a31485e5bca1c0938724ed71026ad3f8d/third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/1.1/form-action-src-default-ignored-expected.txt
[modify] https://crrev.com/82fbd88a31485e5bca1c0938724ed71026ad3f8d/third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/1.1/form-action-src-get-allowed-expected.txt

It turns out that for history items, we use the origin derived from referrer.  Interestingly, we only do this if formData is present:

    FrameLoader::resourceRequestFromHistoryItem:
    ...
        if (formData) {
            request.setHTTPMethod(HTTPNames::POST);
            request.setHTTPBody(formData);
            request.setHTTPContentType(item->formContentType());
            RefPtr<SecurityOrigin> securityOrigin = SecurityOrigin::createFromString(item->referrer().referrer);
            request.addHTTPOriginIfNeeded(securityOrigin);
        }

Similarily, we also derive the "Origin" from "Referer" in FrameLoader::setReferrerForFrameRequest.  In this case, this is done unconditionally (i.e. without looking whether form data is present).

The OpenURL code path already preserves the referrer all the way into RenderFrameImpl::NavigateInternal, so one possible fix would be to just call WebURLRequest::addHTTPOriginIfNeeded from CreateURLRequestForNavigation in content/renderer/render_frame_impl.cc.  I've verified that this makes the layout tests from r411044 pass.  For now I've uploaded this fix to https://codereview.chromium.org/2230103003/

Hm, given that the referrer can be stripped by the Referer Policy (for example, in HistoryItem, this happens when m_referrer is set: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/HistoryItem.cpp?sq=package:chromium&rcl=1470626560&l=92), we should probably explicitly carry an Origin as well. Seems unfortunate to carry both, but probably necessary.

jww@ - thanks for pointing out the Referrer Policy.  I've tried adding (see https://crrev.com/2230103003/#ps20001) a test with "no-referrer" policy and found out that with and without --site-per-process, the form-related POST request was made with 1) no Referer header and 2) Origin header set to "null".  This behavior seems to be compatible both with 1) the referrer policy and 2) with CSRF protection goals (which I assume are the reason for attaching Origin header to [presumably state-changing] POST requests).


I have also (https://crrev.com/2230103003/#ps40001) tried seeing what the behavior is in presence of redirects (307s to preserve everything as a POST).  I did this by 1) tweaking content/test/data/form_that_posts_cross_site.html so that it makes 2 redirect hops instead of 1 and 2) adding a CrossSiteTransferTest similar to CrossSiteTransferTest.PostWithFileData and SessionHistoryTest.GoBackToCrossSitePostWithRedirect but which looks at http request headers dumped by /echoall.  The sequence of redirects is (confirmed by ad-hoc logging from EmbeddedTestServer::HandleRequest):

    0. a.com:37040/form_that_posts_cross_site.html
    1. a.com:37040/cross-site-307/i.com/cross-site-307/x.com/echoall
    2. i.com:37040/cross-site-307/x.com/echoall
    3. x.com:37040/echoall

I see that (with and without IsolateAllSitesForTesting commented out in CrossSiteTransferTest::SetUpCommandLine) I would get the following results:

    Origin: null
    Referer: http://a.com:50800/form_that_posts_cross_site.html

BTW - in case anybody is interested in comparing overlap between various structs holding navigation parameters, I've shared a quick&dirty comparison table at https://docs.google.com/document/d/1MxboPa5wpwzBh6Ut-6GjTBSkkn1ZzCW_7YD1ex6vcLk

Unfortunately, I don't see an easy way to consolidate the structs (and I am now less motivated to do so, after discovering that the Origin trouble can be solved with a simple fix being reviewed in https://crrev.com/2230103003).

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/693329deb699ad58094fa7ef4d75087aa2411c2b

commit 693329deb699ad58094fa7ef4d75087aa2411c2b
Author: lukasza <lukasza@chromium.org>
Date: Sat Aug 13 01:10:32 2016

Propagate Origin header via CreateURLRequestForNavigation.

The CL makes sure that when CreateURLRequestForNavigation populates
Referrer of WebURLRequest, it also adds Origin header if needed, by
calling WebURLRequest::addHTTPOriginIfNeeded with Origin derrived from
Referrer.  Because ResourceRequest::addHTTPOriginIfNeeded method
inspects the HTTP method, the call to WebURLRequest::setHTTPMethod had
to be moved slightly earlier.

The modified CreateURLRequestForNavigation helper function is called from
1) RenderFrameImpl::NavigateInternal and
2) RenderFrameImpl::OnFailedNavigation (PlzNavigate-only, returned
   WebURLRequest is only used in a call to GetNavigationErrorStrings)

The CL also adds 2 extra tests to ensure that not only the CL fixes
form-targets-cross-site-frame-post.html scenario, but that Origin
and Referer headers are also behaving expectedly if 1) referrer
policy is present or 2) redirects are present.

BUG= 635400 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2230103003
Cr-Commit-Position: refs/heads/master@{#411841}

[modify] https://crrev.com/693329deb699ad58094fa7ef4d75087aa2411c2b/content/browser/frame_host/navigation_controller_impl_browsertest.cc
[modify] https://crrev.com/693329deb699ad58094fa7ef4d75087aa2411c2b/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/693329deb699ad58094fa7ef4d75087aa2411c2b/content/test/data/form_that_posts_cross_site.html
[modify] https://crrev.com/693329deb699ad58094fa7ef4d75087aa2411c2b/third_party/WebKit/LayoutTests/FlagExpectations/site-per-process
[add] https://crrev.com/693329deb699ad58094fa7ef4d75087aa2411c2b/third_party/WebKit/LayoutTests/http/tests/navigation/form-targets-cross-site-frame-no-referrer-expected.txt
[add] https://crrev.com/693329deb699ad58094fa7ef4d75087aa2411c2b/third_party/WebKit/LayoutTests/http/tests/navigation/form-targets-cross-site-frame-no-referrer.html

I've tried the repro steps in 54.0.2829.0 (Official Build) canary (64-bit) [which should include r411841 from above] and got the http request / response headers copied below.  AFAICT this means that this bug is fixed - the Origin header looks correct below.  OTOH, for some reason the response from duckduckgo.com didn't include any results / I ended up on the query page... (but this probably should be treated as a separate bug?).

Request headers:

POST /html/ HTTP/1.1
Host: duckduckgo.com
Connection: keep-alive
Content-Length: 18
Cache-Control: max-age=0
Origin: https://duckduckgo.com
Upgrade-Insecure-Requests: 1
User-Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/54.0.2829.0 Safari/537.36
Accept: text/html,application/xhtml+xml,application/xml;q=0.9,image/webp,*/*;q=0.8
Referer: https://duckduckgo.com/
Accept-Encoding: gzip, deflate, br
Accept-Language: en-US,en;q=0.8

Request payload:

q=adam+mickiewicz&b=&kl=us-en

Response headers:

HTTP/1.1 200 OK
Server: nginx
Date: Mon, 15 Aug 2016 18:28:11 GMT
Content-Type: text/html; charset=UTF-8
Transfer-Encoding: chunked
Connection: keep-alive
Access-Control-Allow-Origin: https://duckduckgo.com
Access-Control-Expose-Headers: 
Expires: Mon, 15 Aug 2016 18:28:12 GMT
Cache-Control: max-age=1
Strict-Transport-Security: max-age=31536000
X-DuckDuckGo-Locale: en_US
Content-Encoding: gzip