Propagate Origin header via CreateURLRequestForNavigation.

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

Committed: https://crrev.com/693329deb699ad58094fa7ef4d75087aa2411c2b
Cr-Commit-Position: refs/heads/master@{#411841}

[Łukasz Anforowicz, 2016-08-11 21:50:01]

Description was changed from

==========
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)

BUG=635400
==========

to

==========
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)

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

[Łukasz Anforowicz, 2016-08-11 23:38:00]

Description was changed from

==========
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)

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

to

==========
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
==========

[Łukasz Anforowicz, 2016-08-12 15:59:08]

lukasza@chromium.org changed reviewers:
+ jww@chromium.org

[Łukasz Anforowicz, 2016-08-12 15:59:09]

jww@, can you take a look please?  The tests are passing (including tests for
referrer policy and for redirects), so IMO this fix is looking good so far. 
WDYT?

[jww, 2016-08-12 21:10:54]

This lgtm with one nit. Thanks for the work on all of this!

https://codereview.chromium.org/2230103003/diff/60001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2230103003/diff/60001/content/renderer/render...
content/renderer/render_frame_impl.cc:549:
WebSecurityOrigin(url::Origin(common_params.referrer.url))
This seems like an unnecessarily complex set of conversions. Why not just pass
in common_params.referrer.url.spec() or something?

[Łukasz Anforowicz, 2016-08-12 22:09:43]

Thanks.  avi@, could you please do an OWNERS review for //content?

https://codereview.chromium.org/2230103003/diff/60001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2230103003/diff/60001/content/renderer/render...
content/renderer/render_frame_impl.cc:549:
WebSecurityOrigin(url::Origin(common_params.referrer.url))
On 2016/08/12 21:10:54, jww wrote:
> This seems like an unnecessarily complex set of conversions. Why not just pass
> in common_params.referrer.url.spec() or something?

Done (although it still needs an explicit std::string -> WebString conversion).

https://codereview.chromium.org/2230103003/diff/80001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2230103003/diff/80001/content/renderer/render...
content/renderer/render_frame_impl.cc:549:
WebString::fromUTF8(common_params.referrer.url.GetOrigin().spec()));
In theory I could also get rid of |.GetOrigin()| call in the new code (i.e.
things continue to work without it), but it just feels wrong to pass a full url
to a method named WebURLRequest::addHTTP***Origin***IfNeeded.  For similar
reason, I am not just passing |web_referrer| (which is a full url).

[Łukasz Anforowicz, 2016-08-12 22:10:21]

lukasza@chromium.org changed reviewers:
+ avi@chromium.org

[Łukasz Anforowicz, 2016-08-12 22:10:22]

Actually adding avi@ this time around... :-)

[Avi (use Gerrit), 2016-08-12 22:54:33]

lgtm

https://codereview.chromium.org/2230103003/diff/80001/content/test/data/form_...
File content/test/data/form_that_posts_cross_site.html (right):

https://codereview.chromium.org/2230103003/diff/80001/content/test/data/form_...
content/test/data/form_that_posts_cross_site.html:5: <form id="text-form"
method="POST" action="/cross-site-307/i.com/cross-site-307/x.com/echoall">
This change doesn't interfere with the existing tests that use this file?

[Łukasz Anforowicz, 2016-08-12 23:00:30]

Thanks!

https://codereview.chromium.org/2230103003/diff/80001/content/test/data/form_...
File content/test/data/form_that_posts_cross_site.html (right):

https://codereview.chromium.org/2230103003/diff/80001/content/test/data/form_...
content/test/data/form_that_posts_cross_site.html:5: <form id="text-form"
method="POST" action="/cross-site-307/i.com/cross-site-307/x.com/echoall">
On 2016/08/12 22:54:33, Avi wrote:
> This change doesn't interfere with the existing tests that use this file?

Correct - it doesn't interfere with the existing tests (the change just adds an
extra redirect/hop - existing tests will just wait a tiny bit longer for the
navigation to complete).  To clarify why I am doing this - I am adding the extra
hop, so there is clearly a difference between 1) origin of referrer, 2) origin
of last redirect, 3) final destination.  Without the extra hop 1 == 2
(pseudo-code :-P).

[Łukasz Anforowicz, 2016-08-12 23:00:45]

The CQ bit was checked by lukasza@chromium.org

[Łukasz Anforowicz, 2016-08-12 23:00:45]

The patchset sent to the CQ was uploaded after l-g-t-m from jww@chromium.org
Link to the patchset: https://codereview.chromium.org/2230103003/#ps80001
(title: "Simplify code for converting GURL into WebString representing the
origin.")

[commit-bot: I haz the power, 2016-08-12 23:01:14]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-13 01:10:59]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-08-13 01:13:06]

Description was changed from

==========
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
==========

to

==========
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

Committed: https://crrev.com/693329deb699ad58094fa7ef4d75087aa2411c2b
Cr-Commit-Position: refs/heads/master@{#411841}
==========

[commit-bot: I haz the power, 2016-08-13 01:13:07]

Patchset 5 (id:??) landed as
https://crrev.com/693329deb699ad58094fa7ef4d75087aa2411c2b
Cr-Commit-Position: refs/heads/master@{#411841}