Add CORS headers to URLRequestRedirectJob.

In Chromium, requests can be redirected before they hit the network by (re)starting
the request with a URLRequestRedirectJob. This is used by HSTS, the extension
webRequest API and protocol handlers.

These redirects are trusted and must be followed. However when such redirects are
triggered  for a cross-origin resource, e.g. <img src=".." crossorigin="anonymous">,
Blink blocks the redirect because the Access-Control-Allow-{Origin,Credentials}
response headers are missing.
This CL adds these headers to fix the problem.

Adding these CORS headers to the redirect response is safe, because CORS is still
enforced at the redirect target. For example, if HSTS is active for google.com and
an evil page embeds <img src="http://google.com/" crossorigin="use-credentials">,
then the image is not displayed because google.com does not reply with
"Access-Control-Allow-Origin: null".

BUG=387198
TEST=ExtensionWebRequestApiTest.WebRequestBlocking, HTTPSRequestTest.HSTSCrossOriginAddHeaders

Committed: https://crrev.com/4e0be1f338c3bb9bb3c347cdb4c5e3b7944c5c67
Cr-Commit-Position: refs/heads/master@{#294494}

[robwu, 2014-06-22 23:13:23]

Hi mmenke, could you review this?
I have added tests for the webRequest API, but not HSTS, because the underlying
causes for the bug are identical. Let me know if you also want a browser test
for HSTS.

[mmenke, 2014-06-22 23:24:51]

On 2014/06/22 23:13:23, robwu wrote:
> Hi mmenke, could you review this?
> I have added tests for the webRequest API, but not HSTS, because the
underlying
> causes for the bug are identical. Let me know if you also want a browser test
> for HSTS.

I'm insufficiently familiar with the cross-origin security model to review this
CL.  rsleevi, maybe?  This should probably also undergo a review by the security
team.

[robwu, 2014-06-22 23:42:34]

Adam, could you review the sanity of my patch (CORS)?

Ryan, could you review the network part if you feel qualified?

Mmenke, who shall I add for a Security review?

[mmenke, 2014-06-23 00:35:23]

On 2014/06/22 23:42:34, robwu wrote:
> Adam, could you review the sanity of my patch (CORS)?
> 
> Ryan, could you review the network part if you feel qualified?
> 
> Mmenke, who shall I add for a Security review?

I'm not as positive now that a security review is needed as I was earlier.  I
defer to Ryan and Adam over whether it's needed.

[abarth-chromium, 2014-06-23 17:01:09]

I don't have enough context to evaluate whether this CL make sense.   Sorry.

https://codereview.chromium.org/348253002/diff/1/net/url_request/url_request_...
File net/url_request/url_request_redirect_job.cc (right):

https://codereview.chromium.org/348253002/diff/1/net/url_request/url_request_...
net/url_request/url_request_redirect_job.cc:68: status_line += " " +
redirect_reason_;;
This is a very slow way to construct a string.

https://codereview.chromium.org/348253002/diff/1/net/url_request/url_request_...
net/url_request/url_request_redirect_job.cc:78:
response_headers->AddHeader("Access-Control-Allow-Origin: " + http_origin);
Does this line represent an HTTP header injection vulnerability?  How do you
know that http_origin won't split the header?

[robwu, 2014-06-23 17:28:19]

On 2014/06/23 17:01:09, abarth wrote:
> I don't have enough context to evaluate whether this CL make sense.   Sorry.

In Chromium, requests can be redirected before they hit the network by
(re)starting the request with a URLRequestRedirectJob. This is used by HSTS, the
extension webRequest API and protocol handlers.

These redirects are trusted and must be followed. However when this redirect is
performed for a cross-origin resource, e.g. <img src=".."
crossorigin="anonymous">, then Blink blocks the redirect because the response is
invalid. This is solved by setting the status line with a non-zero status code
(e.g. HTTP status code 307).

After setting the status line, the redirect is still blocked because the
response has no CORS headers. This is fixed by setting the
Access-Control-Allow-{Origin,Credentials} response headers.

I don't see any (security) issues with adding these CORS headers to the redirect
response, because CORS is still enforced for the redirection target. For
example, if HSTS is active for google.com, and an evil page embeds <img
src="http://google.com/" crossorigin="use-credentials">, then the image is not
displayed because google.com does not reply with "Access-Control-Allow-Origin:
null".

https://codereview.chromium.org/348253002/diff/1/net/url_request/url_request_...
File net/url_request/url_request_redirect_job.cc (right):

https://codereview.chromium.org/348253002/diff/1/net/url_request/url_request_...
net/url_request/url_request_redirect_job.cc:68: status_line += " " +
redirect_reason_;;
On 2014/06/23 17:01:09, abarth wrote:
> This is a very slow way to construct a string.

What about:

std::string status_line(base::StringPrintf("HTTP/1.1 %d %s", http_status_code_,
redirect_reason_.c_str()));

https://codereview.chromium.org/348253002/diff/1/net/url_request/url_request_...
net/url_request/url_request_redirect_job.cc:78:
response_headers->AddHeader("Access-Control-Allow-Origin: " + http_origin);
On 2014/06/23 17:01:09, abarth wrote:
> Does this line represent an HTTP header injection vulnerability?  How do you
> know that http_origin won't split the header?

The Origin request header is set by the browser (Blink). This header cannot be
forged by regular web pages (not even through XMLHttpRequest).

[abarth-chromium, 2014-06-23 18:07:56]

On 2014/06/23 at 17:28:19, rob wrote:
> On 2014/06/23 17:01:09, abarth wrote:
> > I don't have enough context to evaluate whether this CL make sense.   Sorry.
> 
> In Chromium, requests can be redirected before they hit the network by
(re)starting the request with a URLRequestRedirectJob. This is used by HSTS, the
extension webRequest API and protocol handlers.
> 
> These redirects are trusted and must be followed. However when this redirect
is performed for a cross-origin resource, e.g. <img src=".."
crossorigin="anonymous">, then Blink blocks the redirect because the response is
invalid. This is solved by setting the status line with a non-zero status code
(e.g. HTTP status code 307).
> 
> After setting the status line, the redirect is still blocked because the
response has no CORS headers. This is fixed by setting the
Access-Control-Allow-{Origin,Credentials} response headers.
> 
> I don't see any (security) issues with adding these CORS headers to the
redirect response, because CORS is still enforced for the redirection target.
For example, if HSTS is active for google.com, and an evil page embeds <img
src="http://google.com/" crossorigin="use-credentials">, then the image is not
displayed because google.com does not reply with "Access-Control-Allow-Origin:
null".

That makes sense to me.

>
https://codereview.chromium.org/348253002/diff/1/net/url_request/url_request_...
> File net/url_request/url_request_redirect_job.cc (right):
> 
>
https://codereview.chromium.org/348253002/diff/1/net/url_request/url_request_...
> net/url_request/url_request_redirect_job.cc:68: status_line += " " +
redirect_reason_;;
> On 2014/06/23 17:01:09, abarth wrote:
> > This is a very slow way to construct a string.
> 
> What about:
> 
> std::string status_line(base::StringPrintf("HTTP/1.1 %d %s",
http_status_code_, redirect_reason_.c_str()));

I'm not sure what the appropriate idiom is in Chromium, but that looks better
than what you had before (which realloced the string many times).

>
https://codereview.chromium.org/348253002/diff/1/net/url_request/url_request_...
> net/url_request/url_request_redirect_job.cc:78:
response_headers->AddHeader("Access-Control-Allow-Origin: " + http_origin);
> On 2014/06/23 17:01:09, abarth wrote:
> > Does this line represent an HTTP header injection vulnerability?  How do you
> > know that http_origin won't split the header?
> 
> The Origin request header is set by the browser (Blink). This header cannot be
forged by regular web pages (not even through XMLHttpRequest).

I guess we could be in trouble if the renderer is compromised...  Maybe that's
not worth worrying about here?  What about other clients, like NaCl plugins?

[Ryan Sleevi, 2014-06-23 21:46:47]

I'm adding palmer here to comment on the HSTS-affecting behaviour.

Caveat: I am not too familiar with CORS (that's Adam, no doubt), although I
agree, your explanation of the problem sounds reasonable and sane.

There's been a lot of discussion about how to handle HSTS in terms of mixed
content (blocking and signalling), which conceptually relates to how we treat
the origin of the data. Whether to block content that is mixed-but-rewritten
(e.g.: treating the origin-as-written as the canonical origin) or allow it
(e.g.: using the 'effective' transport as the determinant).

As it stands now, the security policies Chrome applies to HSTS content are the
security policies "as written" (e.g. in the source), rather than the security
policies from the network.

I can certainly buy the argument that extension-initiated rewrites should be
transparent to the page author, and the current behaviour makes it apparent, so
we should fix that for extensions (webRequest). I'm not fully sure on the HSTS
though whether or not we truly want to make that transparent.

I've added Chris and cc'd Mike to see if there are concerns. As it relates to
inter-UA behaviour, how do Safari and Firefox handle the HSTS here?

[palmer, 2014-06-25 21:44:12]

I don't think I am the right person to review this. (I know nearly nothing about
CORS.) Perhaps tsepez is?

[robwu, 2014-07-07 21:54:19]

Review status?

@abarth
In your previous comment, you voiced the concern that the Origin header could
have been spoofed.
However this value originates from the HttpRequestHeaders class, so it should be
safe to use it. Currently the code path from Blink to Chomium uses
HttpRequestHeaders::AddHeaderFromString, which is already properly sanitized.
There are a few other ways to modify the RequestHeaders instance without
validation, but that is a bug that should be fixed (http://crbug.com/390458).

I believe that you're the web security/CORS expert at the Chrome team, so if you
review the CORS part of this CL, then it could be easier for the network stack
guys to review the rest.

[abarth-chromium, 2014-07-07 22:14:10]

On 2014/07/07 at 21:54:19, rob wrote:
> @abarth
> In your previous comment, you voiced the concern that the Origin header could
have been spoofed.
> However this value originates from the HttpRequestHeaders class, so it should
be safe to use it. Currently the code path from Blink to Chomium uses
HttpRequestHeaders::AddHeaderFromString, which is already properly sanitized.
There are a few other ways to modify the RequestHeaders instance without
validation, but that is a bug that should be fixed (http://crbug.com/390458).

Ok.

> I believe that you're the web security/CORS expert at the Chrome team, so if
you review the CORS part of this CL, then it could be easier for the network
stack guys to review the rest.

What you're doing is a plausible use of CORS, but as I wrote above, I don't
understand enough of the context to evaluate whether it's sensible.  I'm not
going to spend the time to learn this entire system to be able to review this
CL.  Perhaps someone else would be a more appropriate reviewer?

[palmer, 2014-07-07 22:16:46]

Seems reasonable to me, but I think tsepez and abarth are much better people to
sign off on it than me.

[Ryan Sleevi, 2014-07-07 22:22:30]

abarth: A sanity check would be making sure that the headers are the correct
value and imply the sane behaviour.

That is, if we assume that these synthetic redirects are only instantiated by
trusted code, and only in response to situations where the *actual* request is
meant to be indistinguishable, by the recipient, from the *original* request, do
these headers suffice?

I can take on the task of validating that it's only trusted code. When I looked
at this (and paged in palmer@), this was my analysis after a few hours, but I
wanted to make sure that the value of the headers, assuming this supposition,
are correct.

We'll likely want to page in one of the webRequest OWNERs to make sure there's
no use case for an extension to synthetically inject CORS headers (although I
believe they'd be able/allowed to), since this *implementation* would end up
overriding them, and that may not be desirable.

But that's a separate discussion from what I was hoping Adam/Tom could answer
best.

https://codereview.chromium.org/348253002/diff/20001/net/url_request/url_requ...
File net/url_request/url_request_redirect_job.h (right):

https://codereview.chromium.org/348253002/diff/20001/net/url_request/url_requ...
net/url_request/url_request_redirect_job.h:21: // on the result of another job.
Because this (potentially) introduces security implications to
URLRequestRedirectJob (e.g., that it shouldn't be used for arbitrary/hostile
redirects, but only trusted redirects, AIUI), it seems well to add a note here
to that effect.

[abarth-chromium, 2014-07-07 22:29:40]

On 2014/07/07 at 22:22:30, rsleevi wrote:
> abarth: A sanity check would be making sure that the headers are the correct
value and imply the sane behaviour.

The behavior they imply is that everyone on the Internet is allowed to access
the response.

> That is, if we assume that these synthetic redirects are only instantiated by
trusted code, and only in response to situations where the *actual* request is
meant to be indistinguishable, by the recipient, from the *original* request, do
these headers suffice?

It's not going to be indistinguishable.  For example, the fetch API:

http://fetch.spec.whatwg.org/#fetch-api

will see the difference.  (Note: We don't yet implement the fetch API.)

Also, I would expect XMLHttpRequest#responseURL
http://xhr.spec.whatwg.org/#the-responseurl-attribute to show the URL after the
redirect.

[Ryan Sleevi, 2014-07-07 22:42:26]

On 2014/07/07 22:29:40, abarth wrote:
> On 2014/07/07 at 22:22:30, rsleevi wrote:
> > abarth: A sanity check would be making sure that the headers are the correct
> value and imply the sane behaviour.
> 
> The behavior they imply is that everyone on the Internet is allowed to access
> the response.

Well, thanks for confirming this is sufficiently scary and full of dragons ;)

That said, the comment (which I can chase down) on URLRequestRedirectJob implies
this isn't the case, if the destination URL/redirect target doesn't also serve
up CORS headers.

That is, it would seem this is just a synthetic intermediate step, that allows
'everyone' access to see a redirect happened, but not the contents of the
redirected-to page.

> 
> > That is, if we assume that these synthetic redirects are only instantiated
by
> trusted code, and only in response to situations where the *actual* request is
> meant to be indistinguishable, by the recipient, from the *original* request,
do
> these headers suffice?
> 
> It's not going to be indistinguishable.  For example, the fetch API:
> 
> http://fetch.spec.whatwg.org/#fetch-api
> 
> will see the difference.  (Note: We don't yet implement the fetch API.)
> 
> Also, I would expect XMLHttpRequest#responseURL
> http://xhr.spec.whatwg.org/#the-responseurl-attribute to show the URL after
the
> redirect.

Ok, that shoots that plan in the water. I suppose we need to treat these like
they were 'actual' 307s.

[robwu, 2014-07-08 10:49:27]

On 2014/07/07 22:22:30, Ryan Sleevi wrote:
> We'll likely want to page in one of the webRequest OWNERs to make sure there's
> no use case for an extension to synthetically inject CORS headers (although I
> believe they'd be able/allowed to), since this *implementation* would end up
> overriding them, and that may not be desirable.
I've implemented part of redirecting in the webRequest API, so I can pitch in
here.
The webRequest API can be used to redirect a request at two stages:
1. Before the request starts.
2. When the headers are received.

At the first stage, there is no way for an extension developer to specify
additional response headers, so this CL will not limit the flexibility of the
webRequest API (actually, it fixes the bug that redirects are cancelled in
cross-origin contexts).
At the second stage, the redirect is not handled by URLRequestRedirectJob, but
by a code path similar to HTTP-redirects, so this CL does not affect the
behavior of redirects at the HeadersReceived stage.

On 2014/07/07 22:29:40, abarth wrote:
> On 2014/07/07 at 22:22:30, rsleevi wrote:
> > abarth: A sanity check would be making sure that the headers are the correct
> value and imply the sane behaviour.
> 
> The behavior they imply is that everyone on the Internet is allowed to access
> the response.

Only if the response of the redirection target includes the appropriate CORS
headers.

> 
> > That is, if we assume that these synthetic redirects are only instantiated
by
> trusted code, and only in response to situations where the *actual* request is
> meant to be indistinguishable, by the recipient, from the *original* request,
do
> these headers suffice?
> 
> It's not going to be indistinguishable.  For example, the fetch API:
> 
> http://fetch.spec.whatwg.org/#fetch-api
> 
> will see the difference.  (Note: We don't yet implement the fetch API.)
> 
> Also, I would expect XMLHttpRequest#responseURL
> http://xhr.spec.whatwg.org/#the-responseurl-attribute to show the URL after
the
> redirect.

I assume that the (final) redirect URL is only visible to APIs (XHR) when the
request completes successfully. This is only the case if the response from the
redirection includes CORS headers. Let's evaluate the current uses of
URLRequestRedirectJob:

- HSTS - The redirect URL is not a secret. The HSTS spec strongly recommends web
servers to send a 301 redirect when a HSTS URL is accessed over http, so API
users should not be able to get any information from a HSTS redirect URL. (RFC
6797, section 7.2) Actually, with this CL, even *fewer* information is leaked.
Currently, cross-origin 301 redirects will succeed, but cross-origin HSTS will
fail, allowing attackers to detect whether the user has accessed a website
before.

- Extension webRequest API - at the BeforeRequest stage, extension developers
don't know anything about a request, except for the URL and request body. That
is not sufficient to make any security decisions, so extension authors who
redirect a request expect that a redirect is transparently followed. If an
extension developer has explicitly enabled the redirect for request with
type=xmlhttprequest, then they expect that the redirect is followed, provided
that it is a same-origin redirect, or if the redirection target supports CORS,
or if the extension also adds CORS response headers to the redirection target.

- Protocol handlers - XHR cannot perform requests to custom schemes.

Given that the redirect URL is not supposed to be a secret in the current use
cases, I think that it is safe to add the CORS headers to the redirect. If it is
important to distinguish between public and secret URLs in
URLRequestRedirectJob, then an extra flag can be added to its constructor.

On 2014/07/07 22:42:26, Ryan Sleevi wrote:
> Ok, that shoots that plan in the water. I suppose we need to treat these like
> they were 'actual' 307s.

What do you mean by "actual 307s"? How would it solve the problem that synthetic
redirects are being blocked due to the lack of CORS response headers?

[Ryan Sleevi, 2014-07-08 18:40:09]

On 2014/07/08 10:49:27, robwu wrote:
> Given that the redirect URL is not supposed to be a secret in the current use
> cases, I think that it is safe to add the CORS headers to the redirect. If it
is
> important to distinguish between public and secret URLs in
> URLRequestRedirectJob, then an extra flag can be added to its constructor.

My comment was not about "secret" URLs, apologies for confusing the matter. The
comment was about making sure 'hostile' code can't cause the creation of a
URLRequestRedirectJob, and thus result in CORS headers being injected on a
request that would otherwise lack them.

However, the evaluated CORS headers for script purposes are going to be on the
final job, not the intermediate redirect, so this seems reasonable.

> What do you mean by "actual 307s"? How would it solve the problem that
synthetic
> redirects are being blocked due to the lack of CORS response headers?

Just that we should ensure that, from the point of view of the network stack, a
URLRequestRedirectJob is treated indistinguishably from a 'well formed' 307. In
this case, 'well-formed' may include 'proper [synthetic] CORS headers'.

That is, we should NOT try to hide that a redirect has happened - we should
instead try to make it look like the *network* did the redirect.

[robwu, 2014-08-26 12:33:09]

crbug.com/390458 has been fixed, so the Origin header is now super reliable.
Could you resume the review of this CL?

[Ryan Sleevi, 2014-09-02 23:55:25]

rsleevi@chromium.org changed reviewers:
+ mkwst@chromium.org

[Ryan Sleevi, 2014-09-02 23:55:26]

Ok, one last reviewer shuffle, but having read through this more (and having
recently dealt with language-lawyering CORS with respect to TLS client certs), I
think I'm good with this change.

I'm adding Mike, for lack of a better reviewer on the Blink side intimately
familiar with CORS, to make sure I'm not crazy.

[Mike West, 2014-09-03 10:35:28]

This looks reasonable on its face. As noted in the comments above, it shouldn't
expose additional data to JavaScript unless the target of the redirect includes
the relevant CORS headers; the CORS headers on the redirect response itself
shouldn't have any particular impact, as they aren't carried over to the target
response.

It would be valuable to add a test for the HSTS case to verify that that
cross-origin redirect works as expected. Would you be willing to add such a
test, robwu?

https://codereview.chromium.org/348253002/diff/40001/net/url_request/url_requ...
File net/url_request/url_request_redirect_job.cc (right):

https://codereview.chromium.org/348253002/diff/40001/net/url_request/url_requ...
net/url_request/url_request_redirect_job.cc:81: void
URLRequestRedirectJob::StartAsync() {
Please note somewhere in this class's header that, if CORS-enabled, the redirect
will be visible to script.

https://codereview.chromium.org/348253002/diff/40001/net/url_request/url_requ...
net/url_request/url_request_redirect_job.cc:108: http_origin.c_str());
It's probably worth DCHECKing that http_origin doesn't contain newlines.

[robwu, 2014-09-03 17:45:32]

On 2014/09/03 10:35:28, Mike West wrote:
> It would be valuable to add a test for the HSTS case to verify that that
> cross-origin redirect works as expected. Would you be willing to add such a
> test, robwu?

The most important part, namely the interaction between URLRequestRedirectJob is
already covered by the additional webRequest API test. Nevertheless, an extra
HSTS won't hurt.
I'm going to write a test that does something along the lines of:
1. Load https://host/ site (which replies with a STS header).
2. Load http site that embeds the an image at http://host/
3. Verify that the image is loaded.

What would be the best place for this test? I'm thinking of
src/content/browser/loader/resource_dispatcher_host_browsertest.cc.

https://codereview.chromium.org/348253002/diff/40001/net/url_request/url_requ...
File net/url_request/url_request_redirect_job.cc (right):

https://codereview.chromium.org/348253002/diff/40001/net/url_request/url_requ...
net/url_request/url_request_redirect_job.cc:81: void
URLRequestRedirectJob::StartAsync() {
On 2014/09/03 10:35:28, Mike West wrote:
> Please note somewhere in this class's header that, if CORS-enabled, the
redirect
> will be visible to script.

Done.

https://codereview.chromium.org/348253002/diff/40001/net/url_request/url_requ...
net/url_request/url_request_redirect_job.cc:108: http_origin.c_str());
On 2014/09/03 10:35:27, Mike West wrote:
> It's probably worth DCHECKing that http_origin doesn't contain newlines.

https://codereview.chromium.org/491123004 added DCHECKs to make sure that the
HttpRequestHeaders object doesn't contain any invalid characters (NUL, CR, LF).
Because of that check, adding another DCHECK here seems unnecessary.

[Ryan Sleevi, 2014-09-03 19:25:42]

On 2014/09/03 17:45:32, robwu wrote:
> On 2014/09/03 10:35:28, Mike West wrote:
> > It would be valuable to add a test for the HSTS case to verify that that
> > cross-origin redirect works as expected. Would you be willing to add such a
> > test, robwu?
> 
> The most important part, namely the interaction between URLRequestRedirectJob
is
> already covered by the additional webRequest API test. Nevertheless, an extra
> HSTS won't hurt.
> I'm going to write a test that does something along the lines of:
> 1. Load https://host/ site (which replies with a STS header).
> 2. Load http site that embeds the an image at http://host/
> 3. Verify that the image is loaded.
> 
> What would be the best place for this test? I'm thinking of
> src/content/browser/loader/resource_dispatcher_host_browsertest.cc.

Do you really have to do something that high level?

Wouldn't a simple URLRequest test in the existing URL requests, and using our
test server, accomodate that? It'd make it far easier to stub all the
infrastructure out, and the goal being simply to ensure that your synthetically
injected headers appear in the resultant job, right?

Focus there first, and then let me know if I'm missing something in your test.

Note that I'm not opposed to adding a content-level test to ensure the
interaction with CSP is WAI, but I think the header injection test should be at
the //net layer

[robwu, 2014-09-04 22:43:59]

On 2014/09/03 19:25:42, Ryan Sleevi wrote:
> On 2014/09/03 17:45:32, robwu wrote:
> > On 2014/09/03 10:35:28, Mike West wrote:
> > > It would be valuable to add a test for the HSTS case to verify that that
> > > cross-origin redirect works as expected. Would you be willing to add such
a
> > > test, robwu?
> > 
> > The most important part, namely the interaction between
URLRequestRedirectJob
> is
> > already covered by the additional webRequest API test. Nevertheless, an
extra
> > HSTS won't hurt.

edit: "between URLRequestRedirectJob and Blink"

> > I'm going to write a test that does something along the lines of:
> > 1. Load https://host/ site (which replies with a STS header).
> > 2. Load http site that embeds the an image at http://host/
> > 3. Verify that the image is loaded.
> > 
> > What would be the best place for this test? I'm thinking of
> > src/content/browser/loader/resource_dispatcher_host_browsertest.cc.
> 
> Do you really have to do something that high level?
> 
> Wouldn't a simple URLRequest test in the existing URL requests, and using our
> test server, accomodate that? It'd make it far easier to stub all the
> infrastructure out, and the goal being simply to ensure that your
synthetically
> injected headers appear in the resultant job, right?

That will probably do. As said before, testing modification of headers via
URLRequestRedirectJob is already covered by another test, and Blink has its own
tests for CORS, so if we verify that HSTS redirects get CORS headers as well,
then we've covered pretty much every case.

> 
> Focus there first, and then let me know if I'm missing something in your test.
> 
> Note that I'm not opposed to adding a content-level test to ensure the
> interaction with CSP is WAI, but I think the header injection test should be
at
> the //net layer

[robwu, 2014-09-09 22:00:35]

Patchset #5 (id:80001) has been deleted

[robwu, 2014-09-09 23:23:53]

Patchset #5 (id:100001) has been deleted

[robwu, 2014-09-09 23:38:16]

Patchset #5 (id:120001) has been deleted

[robwu, 2014-09-10 09:09:46]

Patchset #5 (id:140001) has been deleted

[robwu, 2014-09-10 09:24:37]

Patchset #5 (id:160001) has been deleted

[robwu, 2014-09-10 10:32:34]

Added an extra test in //net

@rsleevi, could you review the patch?

[Mike West, 2014-09-10 10:39:52]

The code change and test LGTM, but Ryan or someone more familiar with //net
should have a look.

[Ryan Sleevi, 2014-09-10 21:47:47]

Mostly there, but a few nits.

This LG to me, but I'm going to ask mmenke@ to just make sure I've understood
the test code right and that this is the One True Blessed Way for test URL
request contexts.

https://codereview.chromium.org/348253002/diff/180001/net/url_request/url_req...
File net/url_request/url_request_redirect_job.cc (right):

https://codereview.chromium.org/348253002/diff/180001/net/url_request/url_req...
net/url_request/url_request_redirect_job.cc:103: // in the header string.
These last two lines are a bit of a layering violation (//net doesn't know about
//content or Blink).

Rather than buried here, this should be surfaced to the .h as an API requirement
in the contract (that Blink happens to meet)

e.g.
// If the request has an Origin header, the value must not be attacker
// controlled (e.g. it should be synthesized by the code making the
// request, not the user or network)

This is just a rough example, feel free to wordsmith the exact details, but
express it not in terms of what Blink does today, but what all //net-stack
embedders need to do to use this safely.

https://codereview.chromium.org/348253002/diff/180001/net/url_request/url_req...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/348253002/diff/180001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:6814: // Force https for
www.somewhere.com.
Use a truly reserved domain name

test.example
foo.example
example.net

etc

https://codereview.chromium.org/348253002/diff/180001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:6819: include_subdomains);
TransportSecurityState has a MaxAge of 30 days

Just set the value at something simple, like a day, or an hour.

https://codereview.chromium.org/348253002/diff/180001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:6832:
d.set_allow_certificate_errors(true);
You don't want to do this, as this triggers the error-path side-effect.

Instead, use a MockCertVerifier and set_cert_verifier

[mmenke, 2014-09-10 22:08:06]

I'll get back to you tomorrow.

On 2014/09/10 21:47:47, Ryan Sleevi wrote:
> Mostly there, but a few nits.
> 
> This LG to me, but I'm going to ask mmenke@ to just make sure I've understood
> the test code right and that this is the One True Blessed Way for test URL
> request contexts.
> 
>
https://codereview.chromium.org/348253002/diff/180001/net/url_request/url_req...
> File net/url_request/url_request_redirect_job.cc (right):
> 
>
https://codereview.chromium.org/348253002/diff/180001/net/url_request/url_req...
> net/url_request/url_request_redirect_job.cc:103: // in the header string.
> These last two lines are a bit of a layering violation (//net doesn't know
about
> //content or Blink).
> 
> Rather than buried here, this should be surfaced to the .h as an API
requirement
> in the contract (that Blink happens to meet)
> 
> e.g.
> // If the request has an Origin header, the value must not be attacker
> // controlled (e.g. it should be synthesized by the code making the
> // request, not the user or network)
> 
> This is just a rough example, feel free to wordsmith the exact details, but
> express it not in terms of what Blink does today, but what all //net-stack
> embedders need to do to use this safely.
> 
>
https://codereview.chromium.org/348253002/diff/180001/net/url_request/url_req...
> File net/url_request/url_request_unittest.cc (right):
> 
>
https://codereview.chromium.org/348253002/diff/180001/net/url_request/url_req...
> net/url_request/url_request_unittest.cc:6814: // Force https for
> http://www.somewhere.com.
> Use a truly reserved domain name
> 
> test.example
> foo.example
> http://example.net
> 
> etc
> 
>
https://codereview.chromium.org/348253002/diff/180001/net/url_request/url_req...
> net/url_request/url_request_unittest.cc:6819: include_subdomains);
> TransportSecurityState has a MaxAge of 30 days
> 
> Just set the value at something simple, like a day, or an hour.
> 
>
https://codereview.chromium.org/348253002/diff/180001/net/url_request/url_req...
> net/url_request/url_request_unittest.cc:6832:
> d.set_allow_certificate_errors(true);
> You don't want to do this, as this triggers the error-path side-effect.
> 
> Instead, use a MockCertVerifier and set_cert_verifier

[robwu, 2014-09-11 11:54:29]

https://codereview.chromium.org/348253002/diff/180001/net/url_request/url_req...
File net/url_request/url_request_redirect_job.cc (right):

https://codereview.chromium.org/348253002/diff/180001/net/url_request/url_req...
net/url_request/url_request_redirect_job.cc:103: // in the header string.
On 2014/09/10 21:47:47, Ryan Sleevi wrote:
> These last two lines are a bit of a layering violation (//net doesn't know
about
> //content or Blink).

These two lines are obsolete since the header values from HttpRequestHeaders can
be trusted after I fixed https://crbug.com/390458. I have removed these lines.

https://codereview.chromium.org/348253002/diff/180001/net/url_request/url_req...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/348253002/diff/180001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:6814: // Force https for
www.somewhere.com.
On 2014/09/10 21:47:47, Ryan Sleevi wrote:
> Use a truly reserved domain name
> 
> test.example
> foo.example
> http://example.net
> 
> etc

Done.

https://codereview.chromium.org/348253002/diff/180001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:6819: include_subdomains);
On 2014/09/10 21:47:47, Ryan Sleevi wrote:
> TransportSecurityState has a MaxAge of 30 days
> 
> Just set the value at something simple, like a day, or an hour.

Done.

https://codereview.chromium.org/348253002/diff/180001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:6832:
d.set_allow_certificate_errors(true);
On 2014/09/10 21:47:47, Ryan Sleevi wrote:
> You don't want to do this, as this triggers the error-path side-effect.
> 
> Instead, use a MockCertVerifier and set_cert_verifier

Done. Note that most of the code before this line was copied from the
HSTSPreservesPosts test, let me know if the original test should also be edited
following the previous comments as a part of this CL.

[robwu, 2014-09-11 12:19:40]

Patchset #6 (id:200001) has been deleted

[mmenke, 2014-09-11 14:38:09]

url_request_unittests LGTM, just some minor suggestions.

https://codereview.chromium.org/348253002/diff/220001/net/url_request/url_req...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/348253002/diff/220001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:6834:
GURL(base::StringPrintf("http://example.net:%d/echo",
"echo" is a path the test server recognizes.  Since we aren't checking the body
(Or even trying to read it), suggest using a non-sense path, to make it a little
clearer it doesn't matter ("blah"?  Recommend against "foo", as it seems
slightly likely to end up being added to the test server)

https://codereview.chromium.org/348253002/diff/220001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:6835:
test_server.host_port_pair().port())),
You should store this URL in a constant, rather than assembling it twice.

https://codereview.chromium.org/348253002/diff/220001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:6844: 
Maybe "EXPECT_EQ(1, d.received_redirect_count());" as a sanity check?  We have
other tests for the redirects, anyways, but an extra check can't hurt.

[robwu, 2014-09-11 14:55:40]

https://codereview.chromium.org/348253002/diff/220001/net/url_request/url_req...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/348253002/diff/220001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:6834:
GURL(base::StringPrintf("http://example.net:%d/echo",
On 2014/09/11 14:38:08, mmenke wrote:
> "echo" is a path the test server recognizes.  Since we aren't checking the
body
> (Or even trying to read it), suggest using a non-sense path, to make it a
little
> clearer it doesn't matter ("blah"?  Recommend against "foo", as it seems
> slightly likely to end up being added to the test server)

Done.

https://codereview.chromium.org/348253002/diff/220001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:6835:
test_server.host_port_pair().port())),
On 2014/09/11 14:38:09, mmenke wrote:
> You should store this URL in a constant, rather than assembling it twice.

Done (generated the https-URL from a pre-defined http-URL).

https://codereview.chromium.org/348253002/diff/220001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:6844: 
On 2014/09/11 14:38:09, mmenke wrote:
> Maybe "EXPECT_EQ(1, d.received_redirect_count());" as a sanity check?  We have
> other tests for the redirects, anyways, but an extra check can't hurt.

Done.

[robwu, 2014-09-11 19:24:10]

The CQ bit was checked by rob@robwu.nl

[commit-bot: I haz the power, 2014-09-11 19:33:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/348253002/240001

[commit-bot: I haz the power, 2014-09-11 20:30:30]

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

[commit-bot: I haz the power, 2014-09-11 20:30:32]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[robwu, 2014-09-11 21:10:46]

The CQ bit was checked by rob@robwu.nl

[commit-bot: I haz the power, 2014-09-11 21:30:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/348253002/260001

[commit-bot: I haz the power, 2014-09-11 23:42:31]

Committed patchset #8 (id:260001) as b950723a0a25c481d2c7914bd520704f5df8343b

[commit-bot: I haz the power, 2014-09-12 00:22:12]

Patchset 8 (id:??) landed as
https://crrev.com/4e0be1f338c3bb9bb3c347cdb4c5e3b7944c5c67
Cr-Commit-Position: refs/heads/master@{#294494}