Origin header should be preserved in http requests made via OpenURL code path.

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
Committed: https://crrev.com/82fbd88a31485e5bca1c0938724ed71026ad3f8d
Cr-Commit-Position: refs/heads/master@{#411044}

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

Description was changed from

==========
Origin header should be preserved in http requests made via OpenURL code path.

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

to

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

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

[Łukasz Anforowicz, 2016-08-09 17:15:30]

Description was changed from

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

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

to

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

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

[Łukasz Anforowicz, 2016-08-09 17:54:23]

Description was changed from

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

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

to

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

[Łukasz Anforowicz, 2016-08-09 17:55:50]

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

[Łukasz Anforowicz, 2016-08-09 17:55:51]

jww@, could you please take a look?

The bug + CL description hopefully gives enough context to understand what I am
trying to do in this CL :-)

https://codereview.chromium.org/2225383003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/navigation/form-targets-cross-site-frame-get-expected.txt
(right):

https://codereview.chromium.org/2225383003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/navigation/form-targets-cross-site-frame-get-expected.txt:17:
HTTP_UPGRADE_INSECURE_REQUESTS = 1
Interestingly, there is no HTTP_ORIGIN header here.  This is surprising, because
AFAICT the CORS spec doesn't call for a different behavior between POST and GET
(and this testcase is the same as form-targets-cross-site-frame-post.html,
except that this testcase uses GET method).  See also
https://crbug.com/635400#c13.

https://codereview.chromium.org/2225383003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/navigation/form-targets-cross-site-frame-post-expected.txt
(right):

https://codereview.chromium.org/2225383003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/navigation/form-targets-cross-site-frame-post-expected.txt:17:
HTTP_ORIGIN = http://127.0.0.1:8000
1. When this test with --site-per-process flag, we get HTTP_ORIGIN = null - this
is obviously wrong.

2. Http response doesn't include Access-Control-Allow-Origin header, so it is a
bit surprising that the request succeeds (see also https://crbug.com/635400#c13)

https://codereview.chromium.org/2225383003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/navigation/form-targets-cross-site-frame-post-expected.txt:18:
HTTP_REFERER =
http://127.0.0.1:8000/navigation/form-targets-cross-site-frame-post.html
It is interesting that Origin header above provides a subset of information
already provided by the Referer header.

https://codereview.chromium.org/2225383003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/navigation/form-targets-cross-site-frame-post.html
(right):

https://codereview.chromium.org/2225383003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/navigation/form-targets-cross-site-frame-post.html:29:
action="http://localhost:8000/navigation/resources/form-target.pl"
Non-https scheme means it is easier to manually repro the layout test scenario
in a normal browser.  OOPIFs are present even with non-https scheme.

[jww, 2016-08-09 22:38:23]

Generally lgtm, with a few nits and comments.

https://codereview.chromium.org/2225383003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/navigation/form-targets-cross-site-frame-get-expected.txt
(right):

https://codereview.chromium.org/2225383003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/navigation/form-targets-cross-site-frame-get-expected.txt:17:
HTTP_UPGRADE_INSECURE_REQUESTS = 1
On 2016/08/09 17:55:51, Łukasz (OOO until Aug the 8th) wrote:
> Interestingly, there is no HTTP_ORIGIN header here.  This is surprising,
because
> AFAICT the CORS spec doesn't call for a different behavior between POST and
GET
> (and this testcase is the same as form-targets-cross-site-frame-post.html,
> except that this testcase uses GET method).  See also
> https://crbug.com/635400#c13.

The Origin header is defined by the Web Origin Concept spec, and it is pretty
specific that the Origin header is optional for all HTTP requests:
https://tools.ietf.org/html/rfc6454#page-13 This, I believe, is because the
origin idea of the Origin header was that it was for CSRF, which means it's only
technically a concern on POST request, and the CORS bits are a later addition.
I'll look into what the actually CORS requirements are separately.

https://codereview.chromium.org/2225383003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/navigation/form-targets-cross-site-frame-post-expected.txt
(right):

https://codereview.chromium.org/2225383003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/navigation/form-targets-cross-site-frame-post-expected.txt:17:
HTTP_ORIGIN = http://127.0.0.1:8000
On 2016/08/09 17:55:51, Łukasz (OOO until Aug the 8th) wrote:
> 1. When this test with --site-per-process flag, we get HTTP_ORIGIN = null -
this
> is obviously wrong.
I'm not sure what's going on, but it is not "obviously wrong" to me. The Origin
header spec is very clear that any "privacy sensitive" contexts should set the
header to 'null' and it leaves "privacy sensitive" contexts up to the user agent
to define. I agree, though, that it feels weird.
> 
> 2. Http response doesn't include Access-Control-Allow-Origin header, so it is
a
> bit surprising that the request succeeds (see also
https://crbug.com/635400#c13)

https://codereview.chromium.org/2225383003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/navigation/form-targets-cross-site-frame-post-expected.txt:18:
HTTP_REFERER =
http://127.0.0.1:8000/navigation/form-targets-cross-site-frame-post.html
On 2016/08/09 17:55:51, Łukasz (OOO until Aug the 8th) wrote:
> It is interesting that Origin header above provides a subset of information
> already provided by the Referer header.

Why is that surprising? One of the goals of the Origin header is to allow you to
strip the privacy-invasive Referer header (which, for example, some firewalls
will do) while still keeping the Origin information.

https://codereview.chromium.org/2225383003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/navigation/form-targets-cross-site-frame-post.html
(right):

https://codereview.chromium.org/2225383003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/navigation/form-targets-cross-site-frame-post.html:29:
action="http://localhost:8000/navigation/resources/form-target.pl"
On 2016/08/09 17:55:51, Łukasz (OOO until Aug the 8th) wrote:
> Non-https scheme means it is easier to manually repro the layout test scenario
> in a normal browser.  OOPIFs are present even with non-https scheme.

Ack, and I suppose this is fine, although I don't love the idea of dropping
tests that specifically include HTTPS, and I also don't consider using
LayoutTests in "normal browsers" a worthy goal :-)

https://codereview.chromium.org/2225383003/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/navigation/form-targets-cross-site-frame-get.html
(right):

https://codereview.chromium.org/2225383003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/navigation/form-targets-cross-site-frame-get.html:29:
action="http://localhost:8000/navigation/resources/form-target.pl"
Please make this port 8080 (or whatever the alternate port is for testing).
Especially in manual testing, it's easy to accidentally use 'localhost' instead
of 127.0.0.1, and then you'll accidentally have same-origin. Having a different
port makes it super explicit.

https://codereview.chromium.org/2225383003/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/navigation/resources/form-target.pl
(right):

https://codereview.chromium.org/2225383003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/navigation/resources/form-target.pl:27:
print <<HEADER2;
There are a lot of tests that use form-target.pl, and you'll need to change the
test expectations for all of them if you do this, I think. For exmaple,
"form-action-src-allowed.html" uses this target. Instead, maybe add a parameter
to form-target.pl that gets the header printing. Maybe only print if the request
is to "form-target.pl?print-headers=true"?

[jww, 2016-08-09 22:44:46]

On 2016/08/09 22:38:23, jww wrote:
> Generally lgtm, with a few nits and comments.
> 
>
https://codereview.chromium.org/2225383003/diff/1/third_party/WebKit/LayoutTe...
> File
>
third_party/WebKit/LayoutTests/http/tests/navigation/form-targets-cross-site-frame-get-expected.txt
> (right):
> 
>
https://codereview.chromium.org/2225383003/diff/1/third_party/WebKit/LayoutTe...
>
third_party/WebKit/LayoutTests/http/tests/navigation/form-targets-cross-site-frame-get-expected.txt:17:
> HTTP_UPGRADE_INSECURE_REQUESTS = 1
> On 2016/08/09 17:55:51, Łukasz (OOO until Aug the 8th) wrote:
> > Interestingly, there is no HTTP_ORIGIN header here.  This is surprising,
> because
> > AFAICT the CORS spec doesn't call for a different behavior between POST and
> GET
> > (and this testcase is the same as form-targets-cross-site-frame-post.html,
> > except that this testcase uses GET method).  See also
> > https://crbug.com/635400#c13.
> 
> The Origin header is defined by the Web Origin Concept spec, and it is pretty
> specific that the Origin header is optional for all HTTP requests:
> https://tools.ietf.org/html/rfc6454#page-13 This, I believe, is because the
> origin idea of the Origin header was that it was for CSRF, which means it's
only
> technically a concern on POST request, and the CORS bits are a later addition.
> I'll look into what the actually CORS requirements are separately.
> 
>
https://codereview.chromium.org/2225383003/diff/1/third_party/WebKit/LayoutTe...
> File
>
third_party/WebKit/LayoutTests/http/tests/navigation/form-targets-cross-site-frame-post-expected.txt
> (right):
> 
>
https://codereview.chromium.org/2225383003/diff/1/third_party/WebKit/LayoutTe...
>
third_party/WebKit/LayoutTests/http/tests/navigation/form-targets-cross-site-frame-post-expected.txt:17:
> HTTP_ORIGIN = http://127.0.0.1:8000
> On 2016/08/09 17:55:51, Łukasz (OOO until Aug the 8th) wrote:
> > 1. When this test with --site-per-process flag, we get HTTP_ORIGIN = null -
> this
> > is obviously wrong.
> I'm not sure what's going on, but it is not "obviously wrong" to me. The
Origin
> header spec is very clear that any "privacy sensitive" contexts should set the
> header to 'null' and it leaves "privacy sensitive" contexts up to the user
agent
> to define. I agree, though, that it feels weird.
> > 
> > 2. Http response doesn't include Access-Control-Allow-Origin header, so it
is
> a
> > bit surprising that the request succeeds (see also
> https://crbug.com/635400#c13)
> 
>
https://codereview.chromium.org/2225383003/diff/1/third_party/WebKit/LayoutTe...
>
third_party/WebKit/LayoutTests/http/tests/navigation/form-targets-cross-site-frame-post-expected.txt:18:
> HTTP_REFERER =
> http://127.0.0.1:8000/navigation/form-targets-cross-site-frame-post.html
> On 2016/08/09 17:55:51, Łukasz (OOO until Aug the 8th) wrote:
> > It is interesting that Origin header above provides a subset of information
> > already provided by the Referer header.
> 
> Why is that surprising? One of the goals of the Origin header is to allow you
to
> strip the privacy-invasive Referer header (which, for example, some firewalls
> will do) while still keeping the Origin information.
> 
>
https://codereview.chromium.org/2225383003/diff/1/third_party/WebKit/LayoutTe...
> File
>
third_party/WebKit/LayoutTests/http/tests/navigation/form-targets-cross-site-frame-post.html
> (right):
> 
>
https://codereview.chromium.org/2225383003/diff/1/third_party/WebKit/LayoutTe...
>
third_party/WebKit/LayoutTests/http/tests/navigation/form-targets-cross-site-frame-post.html:29:
> action="http://localhost:8000/navigation/resources/form-target.pl"
> On 2016/08/09 17:55:51, Łukasz (OOO until Aug the 8th) wrote:
> > Non-https scheme means it is easier to manually repro the layout test
scenario
> > in a normal browser.  OOPIFs are present even with non-https scheme.
> 
> Ack, and I suppose this is fine, although I don't love the idea of dropping
> tests that specifically include HTTPS, and I also don't consider using
> LayoutTests in "normal browsers" a worthy goal :-)
> 
>
https://codereview.chromium.org/2225383003/diff/20001/third_party/WebKit/Layo...
> File
>
third_party/WebKit/LayoutTests/http/tests/navigation/form-targets-cross-site-frame-get.html
> (right):
> 
>
https://codereview.chromium.org/2225383003/diff/20001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/http/tests/navigation/form-targets-cross-site-frame-get.html:29:
> action="http://localhost:8000/navigation/resources/form-target.pl"
> Please make this port 8080 (or whatever the alternate port is for testing).
> Especially in manual testing, it's easy to accidentally use 'localhost'
instead
> of 127.0.0.1, and then you'll accidentally have same-origin. Having a
different
> port makes it super explicit.
> 
>
https://codereview.chromium.org/2225383003/diff/20001/third_party/WebKit/Layo...
> File
> third_party/WebKit/LayoutTests/http/tests/navigation/resources/form-target.pl
> (right):
> 
>
https://codereview.chromium.org/2225383003/diff/20001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/http/tests/navigation/resources/form-target.pl:27:
> print <<HEADER2;
> There are a lot of tests that use form-target.pl, and you'll need to change
the
> test expectations for all of them if you do this, I think. For exmaple,
> "form-action-src-allowed.html" uses this target. Instead, maybe add a
parameter
> to form-target.pl that gets the header printing. Maybe only print if the
request
> is to "form-target.pl?print-headers=true"?

To clarify some of my comments, I certainly agree that the behavior isn't
correct, esp. with regards to how it works with OOPIF. I'm currently looking at
https://fetch.spec.whatwg.org/, which I believe is what we should be following
at this point for Origin header behavior.

[Łukasz Anforowicz, 2016-08-10 00:03:20]

jww@, can you take another look please?  (disclaimer: the trybots are still
running, but hopefully they'll turn green + I am curious about your reply to my
last question below)

https://codereview.chromium.org/2225383003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/navigation/form-targets-cross-site-frame-get-expected.txt
(right):

https://codereview.chromium.org/2225383003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/navigation/form-targets-cross-site-frame-get-expected.txt:17:
HTTP_UPGRADE_INSECURE_REQUESTS = 1
On 2016/08/09 22:38:22, jww wrote:
> On 2016/08/09 17:55:51, Łukasz (OOO until Aug the 8th) wrote:
> > Interestingly, there is no HTTP_ORIGIN header here.  This is surprising,
> because
> > AFAICT the CORS spec doesn't call for a different behavior between POST and
> GET
> > (and this testcase is the same as form-targets-cross-site-frame-post.html,
> > except that this testcase uses GET method).  See also
> > https://crbug.com/635400#c13.
> 
> The Origin header is defined by the Web Origin Concept spec, and it is pretty
> specific that the Origin header is optional for all HTTP requests:
> https://tools.ietf.org/html/rfc6454#page-13 This, I believe, is because the
> origin idea of the Origin header was that it was for CSRF, which means it's
only
> technically a concern on POST request, and the CORS bits are a later addition.
> I'll look into what the actually CORS requirements are separately.

Thanks for the explanation.  I feel much better about the GET-vs-POST difference
knowing now that 1) Origin header is not necessarily/always tied with CORS
protocol and 2) that (as pointed out by mmenke@ in https://crbug.com/635400#c18)
the Fetch spec asks for Origin header in POST but not in GET requests
(https://fetch.spec.whatwg.org/#origin-header).

https://codereview.chromium.org/2225383003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/navigation/form-targets-cross-site-frame-post-expected.txt
(right):

https://codereview.chromium.org/2225383003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/navigation/form-targets-cross-site-frame-post-expected.txt:17:
HTTP_ORIGIN = http://127.0.0.1:8000
On 2016/08/09 22:38:22, jww wrote:
> On 2016/08/09 17:55:51, Łukasz (OOO until Aug the 8th) wrote:
> > 1. When this test with --site-per-process flag, we get HTTP_ORIGIN = null -
> this
> > is obviously wrong.
> I'm not sure what's going on, but it is not "obviously wrong" to me. The
Origin
> header spec is very clear that any "privacy sensitive" contexts should set the
> header to 'null' and it leaves "privacy sensitive" contexts up to the user
agent
> to define. I agree, though, that it feels weird.

The "obviously wrong" part is that behavior shouldn't be different between
OOPIFs (i.e. --site-per-process mode) and regular/default mode.  If Origin=null
was desirable, then we should be also using it without --site-per-process. 
AFAICT using the real (i.e. non-null) Origin is okay in this scenario.

> > 2. Http response doesn't include Access-Control-Allow-Origin header, so it
is
> a
> > bit surprising that the request succeeds (see also
> https://crbug.com/635400#c13)
> 

I guess I learned today that Origin header might be used outside of CORS
protocol and in this case lack of Access-Control-Allow-Origin http response
header shouldn't trigger a failure.  Is this correct?

https://codereview.chromium.org/2225383003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/navigation/form-targets-cross-site-frame-post-expected.txt:18:
HTTP_REFERER =
http://127.0.0.1:8000/navigation/form-targets-cross-site-frame-post.html
On 2016/08/09 22:38:22, jww wrote:
> On 2016/08/09 17:55:51, Łukasz (OOO until Aug the 8th) wrote:
> > It is interesting that Origin header above provides a subset of information
> > already provided by the Referer header.
> 
> Why is that surprising? One of the goals of the Origin header is to allow you
to
> strip the privacy-invasive Referer header (which, for example, some firewalls
> will do) while still keeping the Origin information.

Thanks for the explanation.  TIL :-)

https://codereview.chromium.org/2225383003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/navigation/form-targets-cross-site-frame-post.html
(right):

https://codereview.chromium.org/2225383003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/navigation/form-targets-cross-site-frame-post.html:29:
action="http://localhost:8000/navigation/resources/form-target.pl"
On 2016/08/09 22:38:23, jww wrote:
> On 2016/08/09 17:55:51, Łukasz (OOO until Aug the 8th) wrote:
> > Non-https scheme means it is easier to manually repro the layout test
scenario
> > in a normal browser.  OOPIFs are present even with non-https scheme.
> 
> Ack, and I suppose this is fine, although I don't love the idea of dropping
> tests that specifically include HTTPS, and I also don't consider using
> LayoutTests in "normal browsers" a worthy goal :-)

Okay.  FWIW, I authored the initial test (and can attest that the choice of
https-vs-http was accidental / non-essential to the test :-).

https://codereview.chromium.org/2225383003/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/navigation/form-targets-cross-site-frame-get.html
(right):

https://codereview.chromium.org/2225383003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/navigation/form-targets-cross-site-frame-get.html:29:
action="http://localhost:8000/navigation/resources/form-target.pl"
On 2016/08/09 22:38:23, jww wrote:
> Please make this port 8080 (or whatever the alternate port is for testing).
> Especially in manual testing, it's easy to accidentally use 'localhost'
instead
> of 127.0.0.1, and then you'll accidentally have same-origin. Having a
different
> port makes it super explicit.

Good point.  This (desire to avoid accidentally ending up with same-origin) was
actually one of the reasons why I initially went forward with "https" rather
than "http".  I've changed the port to 8080 and rerun the tests locally to
update the expectation files.

Done.

https://codereview.chromium.org/2225383003/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/navigation/resources/form-target.pl
(right):

https://codereview.chromium.org/2225383003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/navigation/resources/form-target.pl:27:
print <<HEADER2;
On 2016/08/09 22:38:23, jww wrote:
> There are a lot of tests that use form-target.pl, and you'll need to change
the
> test expectations for all of them if you do this, I think. For exmaple,
> "form-action-src-allowed.html" uses this target. Instead, maybe add a
parameter
> to form-target.pl that gets the header printing. Maybe only print if the
request
> is to "form-target.pl?print-headers=true"?

Ooops.  I thought that I fixed expectations of all the affected tests, but
obviously I've missed tests outside of http/tests/navigation.

I thought about making the headers dump optional, but decided against it because
of worries about the interaction with forms using get method.  I also like the
extra coverage provided by the dumping of the headers from other testcases (e.g.
previously we could start sending Origin with GET requests and the existing
tests wouldn't have caught this).

Would it be okay if I just updated test expectations for the 3 affected tests
under http/tests/security/contentSecurityPolicy/1.1?  This is what I am doing in
patchset 4.  (if this is not okay not, then I think I also could 1) modify
changes to form-target.pl so we only dump headers if a query parameter is
present and 2) modify changes to form-targets-cross-site-frame-post.html so it
is the only layout test that asks for dump of http request headers).

WDYT?

[jww, 2016-08-10 00:44:53]

Still lgtm. Thanks!

https://codereview.chromium.org/2225383003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/navigation/form-targets-cross-site-frame-get-expected.txt
(right):

https://codereview.chromium.org/2225383003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/navigation/form-targets-cross-site-frame-get-expected.txt:17:
HTTP_UPGRADE_INSECURE_REQUESTS = 1
On 2016/08/10 00:03:19, Łukasz Anforowicz wrote:
> On 2016/08/09 22:38:22, jww wrote:
> > On 2016/08/09 17:55:51, Łukasz (OOO until Aug the 8th) wrote:
> > > Interestingly, there is no HTTP_ORIGIN header here.  This is surprising,
> > because
> > > AFAICT the CORS spec doesn't call for a different behavior between POST
and
> > GET
> > > (and this testcase is the same as form-targets-cross-site-frame-post.html,
> > > except that this testcase uses GET method).  See also
> > > https://crbug.com/635400#c13.
> > 
> > The Origin header is defined by the Web Origin Concept spec, and it is
pretty
> > specific that the Origin header is optional for all HTTP requests:
> > https://tools.ietf.org/html/rfc6454#page-13 This, I believe, is because the
> > origin idea of the Origin header was that it was for CSRF, which means it's
> only
> > technically a concern on POST request, and the CORS bits are a later
addition.
> > I'll look into what the actually CORS requirements are separately.
> 
> Thanks for the explanation.  I feel much better about the GET-vs-POST
difference
> knowing now that 1) Origin header is not necessarily/always tied with CORS
> protocol and 2) that (as pointed out by mmenke@ in
https://crbug.com/635400#c18)
> the Fetch spec asks for Origin header in POST but not in GET requests
> (https://fetch.spec.whatwg.org/#origin-header).

As mentioned on the bug, the Fetch spec is the definitive guide to the Origin
header these days, but I wanted to point out that old RFC to give context for
why it originally was created.

https://codereview.chromium.org/2225383003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/navigation/form-targets-cross-site-frame-post-expected.txt
(right):

https://codereview.chromium.org/2225383003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/navigation/form-targets-cross-site-frame-post-expected.txt:17:
HTTP_ORIGIN = http://127.0.0.1:8000
On 2016/08/10 00:03:19, Łukasz Anforowicz wrote:
> On 2016/08/09 22:38:22, jww wrote:
> > On 2016/08/09 17:55:51, Łukasz (OOO until Aug the 8th) wrote:
> > > 1. When this test with --site-per-process flag, we get HTTP_ORIGIN = null
-
> > this
> > > is obviously wrong.
> > I'm not sure what's going on, but it is not "obviously wrong" to me. The
> Origin
> > header spec is very clear that any "privacy sensitive" contexts should set
the
> > header to 'null' and it leaves "privacy sensitive" contexts up to the user
> agent
> > to define. I agree, though, that it feels weird.
> 
> The "obviously wrong" part is that behavior shouldn't be different between
> OOPIFs (i.e. --site-per-process mode) and regular/default mode.  If
Origin=null
> was desirable, then we should be also using it without --site-per-process. 
> AFAICT using the real (i.e. non-null) Origin is okay in this scenario.
Ah, yes, agreed.
> 
> > > 2. Http response doesn't include Access-Control-Allow-Origin header, so it
> is
> > a
> > > bit surprising that the request succeeds (see also
> > https://crbug.com/635400#c13)
> > 
> 
> I guess I learned today that Origin header might be used outside of CORS
> protocol and in this case lack of Access-Control-Allow-Origin http response
> header shouldn't trigger a failure.  Is this correct?
Yup!

https://codereview.chromium.org/2225383003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/navigation/form-targets-cross-site-frame-post.html
(right):

https://codereview.chromium.org/2225383003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/navigation/form-targets-cross-site-frame-post.html:29:
action="http://localhost:8000/navigation/resources/form-target.pl"
On 2016/08/10 00:03:19, Łukasz Anforowicz wrote:
> On 2016/08/09 22:38:23, jww wrote:
> > On 2016/08/09 17:55:51, Łukasz (OOO until Aug the 8th) wrote:
> > > Non-https scheme means it is easier to manually repro the layout test
> scenario
> > > in a normal browser.  OOPIFs are present even with non-https scheme.
> > 
> > Ack, and I suppose this is fine, although I don't love the idea of dropping
> > tests that specifically include HTTPS, and I also don't consider using
> > LayoutTests in "normal browsers" a worthy goal :-)
> 
> Okay.  FWIW, I authored the initial test (and can attest that the choice of
> https-vs-http was accidental / non-essential to the test :-).

Ah, that does make me feel better :-)

https://codereview.chromium.org/2225383003/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/navigation/resources/form-target.pl
(right):

https://codereview.chromium.org/2225383003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/navigation/resources/form-target.pl:27:
print <<HEADER2;
On 2016/08/10 00:03:19, Łukasz Anforowicz wrote:
> On 2016/08/09 22:38:23, jww wrote:
> > There are a lot of tests that use form-target.pl, and you'll need to change
> the
> > test expectations for all of them if you do this, I think. For exmaple,
> > "form-action-src-allowed.html" uses this target. Instead, maybe add a
> parameter
> > to form-target.pl that gets the header printing. Maybe only print if the
> request
> > is to "form-target.pl?print-headers=true"?
> 
> Ooops.  I thought that I fixed expectations of all the affected tests, but
> obviously I've missed tests outside of http/tests/navigation.
> 
> I thought about making the headers dump optional, but decided against it
because
> of worries about the interaction with forms using get method.  I also like the
> extra coverage provided by the dumping of the headers from other testcases
(e.g.
> previously we could start sending Origin with GET requests and the existing
> tests wouldn't have caught this).
> 
> Would it be okay if I just updated test expectations for the 3 affected tests
> under http/tests/security/contentSecurityPolicy/1.1?  This is what I am doing
in
> patchset 4.  (if this is not okay not, then I think I also could 1) modify
> changes to form-target.pl so we only dump headers if a query parameter is
> present and 2) modify changes to form-targets-cross-site-frame-post.html so it
> is the only layout test that asks for dump of http request headers).
> 
> WDYT?

SGTM. If it's just the CSP tests using this, that's esp. fine, since it probably
behooves them to verify the headers as well. I'm a big fan of providing options
to test resources like this, but I appreciate your concerns about forms using
the GET method.

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

The CQ bit was checked by lukasza@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-10 02:57:56]

Dry run: 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-10 04:32:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-10 04:32:29]

Dry run: This issue passed the CQ dry run.

[Łukasz Anforowicz, 2016-08-10 15:20:54]

The CQ bit was checked by lukasza@chromium.org

[commit-bot: I haz the power, 2016-08-10 15:21:15]

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-10 15:25:09]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-08-10 15:27:08]

Description was changed from

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

to

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

Committed: https://crrev.com/82fbd88a31485e5bca1c0938724ed71026ad3f8d
Cr-Commit-Position: refs/heads/master@{#411044}
==========

[commit-bot: I haz the power, 2016-08-10 15:27:09]

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