Support redirectUrl at onHeadersReceived in WebRequest / DWR API

Support redirectUrl at onHeadersReceived in WebRequest / DWR API

Add support for extension-initiated redirects via the (declarative) Web Request API.
The existing URL validation and conflict resolution logic of redirectUrl at onBeforeRequest is re-used for the implementation of redirectUrl at onHeadersReceived.

To make sure that redirects to data:// and chrome-extension:// URLs are not blocked, a new parameter has been added to the network delegate (allowed_unsafe_redirect_url).

BUG=280464, 115940
TEST=browser_tests: ExtensionWebRequestApiTest.WebRequestBlocking:ExtensionWebRequestApiTest.WebRequestDeclarative1
     unit_tests: ExtensionWebRequestTest.*:WebRequestActionWithThreadsTest.*
     net_unittests: URLRequestTestHTTP.NetworkDelegateRedirectRequestOnHeadersReceived*: URLRequestTestHTTP.UnsafeRedirect*: URLRequestTestHTTP.*ReferenceFragment*

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=259546

[robwu, 2014-02-04 21:36:22]

(sorry if you receive this mail twice, the previous one was submitted with the
wrong email address. This one matches the email in AUTHORS)

[robwu, 2014-02-11 21:46:47]

battre:
Please review chrome/* ((D)WR API and docs)
You can use the new tests to verify that the added feature works as intended.


eroman:
Please review net/url_request/*. The only functional changes were made in
url_request_http_job.cc, the other files were changed to match the new method
signature.


darin:
I have added an additional parameter to a function, so many files are affected
by (mostly) a trivial one-line change. Please review these (android_webview/*,
content/*, net/base/*, net/proxy/*).

[battre, 2014-02-12 11:54:47]

chrome/* LGTM

https://codereview.chromium.org/154473002/diff/90001/net/url_request/url_requ...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/154473002/diff/90001/net/url_request/url_requ...
net/url_request/url_request_http_job.cc:1042: int* http_status_code) {
nit: indentation.

https://codereview.chromium.org/154473002/diff/90001/net/url_request/url_requ...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/154473002/diff/90001/net/url_request/url_requ...
net/url_request/url_request_unittest.cc:482: int
BlockingNetworkDelegate::OnHeadersReceived(
I think this should have similar logic for redirects as
BlockingNetworkDelegate::OnBeforeURLRequest.

Then there should be a test similar to TEST_F(URLRequestTestHTTP,
NetworkDelegateRedirectRequest). Also you probably need to update
TestNetworkDelegate::OnBeforeRedirect.

[robwu, 2014-02-12 17:35:20]

On 2014/02/12 11:54:47, battre wrote:
>
https://codereview.chromium.org/154473002/diff/90001/net/url_request/url_requ...
> net/url_request/url_request_http_job.cc:1042: int* http_status_code) {
> nit: indentation.

Fixed.

>
https://codereview.chromium.org/154473002/diff/90001/net/url_request/url_requ...
> net/url_request/url_request_unittest.cc:482: int
> BlockingNetworkDelegate::OnHeadersReceived(
> I think this should have similar logic for redirects as
> BlockingNetworkDelegate::OnBeforeURLRequest.
> 
> Then there should be a test similar to TEST_F(URLRequestTestHTTP,
> NetworkDelegateRedirectRequest).

Added two net tests:
URLRequestTestHTTP.NetworkDelegateRedirectRequestOnHeadersReceived* 

> Also you probably need to update TestNetworkDelegate::OnBeforeRedirect.

I haven't edited this one. Why should this one be updated?

[darin (slow to review), 2014-02-12 18:29:30]

LGTM for the parts you asked me to review.

[battre, 2014-02-13 15:30:32]

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

https://codereview.chromium.org/154473002/diff/150001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:3055: EXPECT_EQ(1,
network_delegate.destroyed_requests());
This looks suspicious. Shouldn't there be 
a) only one created request?
b) the number of created requests match the number of destroyed requests?

[robwu, 2014-02-13 15:45:26]

On 2014/02/13 15:30:32, battre wrote:
>
https://codereview.chromium.org/154473002/diff/150001/net/url_request/url_req...
> File net/url_request/url_request_unittest.cc (right):
> 
>
https://codereview.chromium.org/154473002/diff/150001/net/url_request/url_req...
> net/url_request/url_request_unittest.cc:3055: EXPECT_EQ(1,
> network_delegate.destroyed_requests());
> This looks suspicious. Shouldn't there be 
> a) only one created request?
"created_requests()" records the number of OnBeforeRequest invocations.

2 is the correct number of onBeforeRequest calls: One for the original request
plus one for the redirected request.

> b) the number of created requests match the number of destroyed requests?

A redirect re-uses the existing URLRequest instance, so the number of destroyed
requests should really be one.

The new redirect feature is identical to a HTTP redirect, except that the range
of allowed URLs is much bigger, e.g. data:text/plain, chrome-extension:, blob:,
etc. You can see a similar number of created_requests() and destroyed_requests()
at the test for HTTP redirects (NetworkDelegateInfoRedirect).

[robwu, 2014-02-19 23:27:59]

eroman: Could you also review net/url_request/url_request_http_job.cc? It's the
last thing that needs to be reviewed before the patch is ready.

darin: if you feel qualified, please review
net/url_request/url_request_http_job.cc (the changes are quite straightforward).

[darin (slow to review), 2014-02-20 22:30:20]

I defer to Eric to review the URLRequestHttpJob changes. He's been in that
code more recently than me.
-Darin


On Wed, Feb 19, 2014 at 3:28 PM, <rob@robwu.nl> wrote:

> eroman: Could you also review net/url_request/url_request_http_job.cc?
> It's the
> last thing that needs to be reviewed before the patch is ready.
>
> darin: if you feel qualified, please review
> net/url_request/url_request_http_job.cc (the changes are quite
> straightforward).
>
> https://codereview.chromium.org/154473002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[robwu, 2014-02-28 11:27:42]

Ping Eric, please review net/url_request/url_request_http_job.cc .

[eroman, 2014-02-28 19:50:48]

I'm not sure that I understand why a new parameter is needed. The extensions
already have the ability to modify the response headers, wouldn't it be
sufficient to change the "Location" header then and not have to introduce the
override URL concept into network stack?

mmenke probably knows this code better than I, adding him for comment.

[mmenke, 2014-02-28 20:17:37]

On 2014/02/28 19:50:48, eroman wrote:
> I'm not sure that I understand why a new parameter is needed. The extensions
> already have the ability to modify the response headers, wouldn't it be
> sufficient to change the "Location" header then and not have to introduce the
> override URL concept into network stack?

+1 to this.  Seems like the status code could be changed, and a location header
added.  If that currently doesn't work for some reason, I'd rather see that
fixed, than do this.

> mmenke probably knows this code better than I, adding him for comment.

[robwu, 2014-02-28 20:49:31]

On 2014/02/28 20:17:37, mmenke wrote:
> On 2014/02/28 19:50:48, eroman wrote:
> > I'm not sure that I understand why a new parameter is needed. The extensions
> > already have the ability to modify the response headers, wouldn't it be
> > sufficient to change the "Location" header then and not have to introduce
the
> > override URL concept into network stack?
> 
> +1 to this.  Seems like the status code could be changed, and a location
header
> added.  If that currently doesn't work for some reason, I'd rather see that
> fixed, than do this.
> 
> > mmenke probably knows this code better than I, adding him for comment.

I initially requested a redirect by replacing the Location header and replacing
the status line. This did not yield the desired results, because only http/https
redirects were allowed (and chrome-extension:, data:, blob:, filesystem:, etc.
were not).
To distinguish between a safe redirect and a HTTP-redirect, I added a new
parameter.

The alternative is to add a new method (e.g. "SetSafeRedirect(GURL* location)")
to HttpResponseHeaders and a separate field to distinguish between safe
(=extension-initiated) and unsafe (=http) redirects. I will experiment with this
option and update the CL. Let me know if you see any obvious issues with the
alternative solution.

[robwu, 2014-03-02 14:52:08]

Updated CL, the redirect url is communicated to the network stack via the
ResponseHeaders instead of a new parameter. This is done by replacing the
Location header and the status line, and setting a flag to mark the redirect as
safe.

Eric:
Please review net/*.

battre:
Now, onBeforeRedirect also receives details about the source of the redirect, as
if the redirect was initiated by the server. The only difference is that the
Location header does not reflect the server's response, and the status line is
also fake, "HTTP/1.1 307 Temporary Redirect". Does this behavior look good to
you? (the alternative is to wipe the response info, as if the redirect was
triggered during onBeforeRequest.)

[robwu, 2014-03-11 09:22:10]

Eric, please review net/*.

[robwu, 2014-03-12 15:21:04]

+mmenke as reviewer for net/* since Eric admitted that you are more familiar
with the affected code.

[mmenke, 2014-03-12 15:30:40]

On 2014/03/12 15:21:04, robwu wrote:
> +mmenke as reviewer for net/* since Eric admitted that you are more familiar
> with the affected code.

Not a big fan of the extra code to mark the redirects safe - I had forgotten
that would be needed.  I want to spend some time digging through the code, to
make sure there isn't a better solution.

[mmenke, 2014-03-14 16:19:25]

So sorry for the slow response.  I've been deluged with reviews this week.

I don't think either of these approaches works in conjunctions with the cache -
we'd end up caching just the headers, and making it as a successful request
(Which is what we normally do for redirects - we ignore their bodies, if any,
but we do cache just their headers).  Could hack in a fix for this specifically,
but...  Seems messy, and I'd rather come up with a cleaner solution.  Putting
all this logic in URLHttpRequestJob also seems really weird.

Why exactly is this needed for?

[mmenke, 2014-03-14 16:20:49]

On 2014/03/14 16:19:25, mmenke wrote:
> So sorry for the slow response.  I've been deluged with reviews this week.
> 
> I don't think either of these approaches works in conjunctions with the cache
-
> we'd end up caching just the headers, and making it as a successful request
> (Which is what we normally do for redirects - we ignore their bodies, if any,
> but we do cache just their headers).  Could hack in a fix for this
specifically,
> but...  Seems messy, and I'd rather come up with a cleaner solution.  Putting
> all this logic in URLHttpRequestJob also seems really weird.
> 
> Why exactly is this needed for?

What exactly is this needed for, rather.

[robwu, 2014-03-14 17:36:21]

On 2014/03/14 16:20:49, mmenke wrote:
> On 2014/03/14 16:19:25, mmenke wrote:
> > So sorry for the slow response.  I've been deluged with reviews this week.
> > 
> > I don't think either of these approaches works in conjunctions with the
cache

The most recent approach (patch set 7) forces the response to not be cached, so
the redirect will never be cached.

> > we'd end up caching just the headers, and making it as a successful request
> > (Which is what we normally do for redirects - we ignore their bodies, if
any,
> > but we do cache just their headers).  Could hack in a fix for this
> specifically,
> > but...  Seems messy, and I'd rather come up with a cleaner solution. 
Putting
> > all this logic in URLHttpRequestJob also seems really weird.

What do you think of replacing |override_response_headers_| with
|override_response_|, of the following type:

struct OverrideResponse {
  scoped_refptr<HttpResponseHeaders> headers;
  GURL override_redirect;
};

This is more explicit than adding a hacky flag to HttpResponseHeaders, and it
makes it easier to add more features in the future (http://cbrug.com/104058
comes to mind).

> > 
> > Why exactly is this needed for?
> 
> What exactly is this needed for, rather.

I interpret this question as "Why do you want to implement a redirect
override?". Correct me if I'm wrong.
This CL allows extensions to redirect requests based on MIME type. By design,
extensions are allowed to redirect to schemes other than http/https.
For instance, the existing webRequest extension API offers a way to redirect to
transparent images, implemented via data://-URLs (convenient for ad blockers
which block requests solely based on URLs).
Content handlers (e.g. the PDF Viewer) need the ability to directly redirect to
a chrome-extension://-URL, which is not yet possible (there are some hacks to
get the desired result, but these are error-prone and inefficient).

[mmenke, 2014-03-14 18:33:09]

On 2014/03/14 17:36:21, robwu wrote:
> On 2014/03/14 16:20:49, mmenke wrote:
> > On 2014/03/14 16:19:25, mmenke wrote:
> > > So sorry for the slow response.  I've been deluged with reviews this week.
> > > 
> > > I don't think either of these approaches works in conjunctions with the
> cache
> 
> The most recent approach (patch set 7) forces the response to not be cached,
so
> the redirect will never be cached.
> 
> > > we'd end up caching just the headers, and making it as a successful
request
> > > (Which is what we normally do for redirects - we ignore their bodies, if
> any,
> > > but we do cache just their headers).  Could hack in a fix for this
> > specifically,
> > > but...  Seems messy, and I'd rather come up with a cleaner solution. 
> Putting
> > > all this logic in URLHttpRequestJob also seems really weird.
> 
> What do you think of replacing |override_response_headers_| with
> |override_response_|, of the following type:
> 
> struct OverrideResponse {
>   scoped_refptr<HttpResponseHeaders> headers;
>   GURL override_redirect;
> };
> 
> This is more explicit than adding a hacky flag to HttpResponseHeaders, and it
> makes it easier to add more features in the future (http://cbrug.com/104058
> comes to mind).

This sounds like a better approach to me, though I'd love it if we could get
this code out of UrlRequestHttpJob completely.

One other option I was thinking about is to give the URLRequest a function to
force a redirect, which cancels the current job, if there is one.  The advantage
there is that we could use the same method to redirect before the job was
started as well (Instead of the current approach).  The downside is that it's a
little ugly (We'd be deleting the Job while it's calling the delegate, which we
can do, but it's not lovely), and we'd be affecting externally visible events -
there'd extra events for the URLRequestRedirectJob we'd have to replace the
original Job with.  I think your approach is probably better, but I want to
trace through it, to make sure I'm happy with the solution, rather than having
you implement yet another solution and then changing my mind.

I'll try to get back to you later today.

> > > Why exactly is this needed for?
> > 
> > What exactly is this needed for, rather.
> 
> I interpret this question as "Why do you want to implement a redirect
> override?". Correct me if I'm wrong.
> This CL allows extensions to redirect requests based on MIME type. By design,
> extensions are allowed to redirect to schemes other than http/https.
> For instance, the existing webRequest extension API offers a way to redirect
to
> transparent images, implemented via data://-URLs (convenient for ad blockers
> which block requests solely based on URLs).
> Content handlers (e.g. the PDF Viewer) need the ability to directly redirect
to
> a chrome-extension://-URL, which is not yet possible (there are some hacks to
> get the desired result, but these are error-prone and inefficient).

Yea, I meant "Why isn't redirecting before requests are sent enough?"  I just
wanted to make sure that we weren't adding yet more hooks into the network stack
for no good reason.  This is the third review I've done in the last two weeks
seeking to add stuff to the public API, which is getting rather bloated.

That does sounds potentially pretty useful, and worth supporting (though I
suspect ad blockers generally want to block before requests reach the network).

[mmenke, 2014-03-17 19:10:27]

On 2014/03/14 18:33:09, mmenke wrote:
> On 2014/03/14 17:36:21, robwu wrote:
> > On 2014/03/14 16:20:49, mmenke wrote:
> > > On 2014/03/14 16:19:25, mmenke wrote:
> > > > So sorry for the slow response.  I've been deluged with reviews this
week.
> > > > 
> > > > I don't think either of these approaches works in conjunctions with the
> > cache
> > 
> > The most recent approach (patch set 7) forces the response to not be cached,
> so
> > the redirect will never be cached.
> > 
> > > > we'd end up caching just the headers, and making it as a successful
> request
> > > > (Which is what we normally do for redirects - we ignore their bodies, if
> > any,
> > > > but we do cache just their headers).  Could hack in a fix for this
> > > specifically,
> > > > but...  Seems messy, and I'd rather come up with a cleaner solution. 
> > Putting
> > > > all this logic in URLHttpRequestJob also seems really weird.
> > 
> > What do you think of replacing |override_response_headers_| with
> > |override_response_|, of the following type:
> > 
> > struct OverrideResponse {
> >   scoped_refptr<HttpResponseHeaders> headers;
> >   GURL override_redirect;
> > };
> > 
> > This is more explicit than adding a hacky flag to HttpResponseHeaders, and
it
> > makes it easier to add more features in the future (http://cbrug.com/104058
> > comes to mind).
> 
> This sounds like a better approach to me, though I'd love it if we could get
> this code out of UrlRequestHttpJob completely.
> 
> One other option I was thinking about is to give the URLRequest a function to
> force a redirect, which cancels the current job, if there is one.  The
advantage
> there is that we could use the same method to redirect before the job was
> started as well (Instead of the current approach).  The downside is that it's
a
> little ugly (We'd be deleting the Job while it's calling the delegate, which
we
> can do, but it's not lovely), and we'd be affecting externally visible events
-
> there'd extra events for the URLRequestRedirectJob we'd have to replace the
> original Job with.  I think your approach is probably better, but I want to
> trace through it, to make sure I'm happy with the solution, rather than having
> you implement yet another solution and then changing my mind.
> 
> I'll try to get back to you later today.

Sorry (yet again) for slowness.

One problem with this approach is that if someone tries to override a redirect
by setting its headers, and someone else does it by the new method, then we'll
ignore one, in a way other delegates may not be aware of.  I really don't like
an API that can lead to that kind of confusion.  Switching to a bool that
indicates we should ignore normal redirect safety logic has the same issue, of
course, with even more worrying potential consequences.

One option, which also isn't great, is to do two redirects (x->x->dest).  The
first redirect would be safe, the second would use OnBeforeURLRequest to force a
redirect, and then the third would be the real request.  That wouldn't require
any net/ changes.

Still open to other suggestions.  In general, I'd much prefer a solution where
the UrlRequestHttpJob has no idea what's going on (Not so concerned about the
UrlRequest itself).

[robwu, 2014-03-17 21:25:23]

On 2014/03/17 19:10:27, mmenke wrote:
> On 2014/03/14 18:33:09, mmenke wrote:
> > On 2014/03/14 17:36:21, robwu wrote:
> > > What do you think of replacing |override_response_headers_| with
> > > |override_response_|, of the following type:
> > > 
> > > struct OverrideResponse {
> > >   scoped_refptr<HttpResponseHeaders> headers;
> > >   GURL override_redirect;
> > > };
> 
> One problem with this approach is that if someone tries to override a redirect
> by setting its headers, and someone else does it by the new method, then we'll
> ignore one, in a way other delegates may not be aware of.  I really don't like
> an API that can lead to that kind of confusion.

The same argument also applies to overridden response headers, which has been
around for a while without any issues.
The proposed API is probably not that confusing; Besides unit tests, there is
only one piece of code that writes a "Location:" header (it just strips other
headers from an existing redirect)
(https://code.google.com/p/chromium/codesearch#search/&q=%22%5C%22location:%22...).

> 
> One option, which also isn't great, is to do two redirects (x->x->dest).  The
> first redirect would be safe, the second would use OnBeforeURLRequest to force
a
> redirect, and then the third would be the real request.  That wouldn't require
> any net/ changes.
> 
> Still open to other suggestions.  In general, I'd much prefer a solution where
> the UrlRequestHttpJob has no idea what's going on (Not so concerned about the
> UrlRequest itself).

An alternative method to achieve the desired result is to add
data/blob/chrome-extension to the list of safe schemes (currently only http,
https and ftp are marked safe for redirection).
A consequence is that servers would also be able to issue redirects to these
URLs, so the security impact of this should be considered:

Redirects to chrome-extension: are definitely safe, because only resources that
are whitelisted by the extension author are accessible
(https://developer.chrome.com/extensions/manifest/web_accessible_resources).

blob:-URLs are unpredictable and short-lived, so I don't see any harm in
allowing them.

The safety of data:-URLs is not as easily determined, because they can contain
any arbitrary data. All data:-URLs have an unique origin in Chromium, so
allowing them will not decrease the security of HTML pages in Chromium. Firefox
and Opera (old) both also allow redirects to data-URLs, which hasn't caused any
security issues, except for phishing attempts (using data URLs because they are
self-contained, see the overhyped research paper linked at
https://bugzilla.mozilla.org/show_bug.cgi?id=786275).

Allowing data/chrome-extension: would cover most use cases for extension
developers. If an extension wants to use other schemes, then they could redirect
to data:somethingUniqueToTheirExtension,...url..., and use the onBeforeRedirect
to redirect to their uncommon scheme.

WDYT?

[mmenke, 2014-03-18 17:00:06]

On 2014/03/17 21:25:23, robwu wrote:
> On 2014/03/17 19:10:27, mmenke wrote:
> > On 2014/03/14 18:33:09, mmenke wrote:
> > > On 2014/03/14 17:36:21, robwu wrote:
> > > > What do you think of replacing |override_response_headers_| with
> > > > |override_response_|, of the following type:
> > > > 
> > > > struct OverrideResponse {
> > > >   scoped_refptr<HttpResponseHeaders> headers;
> > > >   GURL override_redirect;
> > > > };
> > 
> > One problem with this approach is that if someone tries to override a
redirect
> > by setting its headers, and someone else does it by the new method, then
we'll
> > ignore one, in a way other delegates may not be aware of.  I really don't
like
> > an API that can lead to that kind of confusion.
> 
> The same argument also applies to overridden response headers, which has been
> around for a while without any issues.
> The proposed API is probably not that confusing; Besides unit tests, there is
> only one piece of code that writes a "Location:" header (it just strips other
> headers from an existing redirect)
>
(https://code.google.com/p/chromium/codesearch#search/&q=%2522%255C%2522locati...).

Actually, this is a NetworkDelegate function rather than a URLRequest::Delegate
one, so there's less concern here than I had thought.  If it were a
URLRequest::Delegate function, because of all the stacked ResourceHandlers and
ResourceThrottles, I'd be strongly opposed to this (Since what takes precedence
would now determined both by where things are in the handler/throttler stack
*and* the API they use).  As a NetworkDelegate function, however, it may well be
workable.

> > One option, which also isn't great, is to do two redirects (x->x->dest). 
The
> > first redirect would be safe, the second would use OnBeforeURLRequest to
force
> a
> > redirect, and then the third would be the real request.  That wouldn't
require
> > any net/ changes.
> > 
> > Still open to other suggestions.  In general, I'd much prefer a solution
where
> > the UrlRequestHttpJob has no idea what's going on (Not so concerned about
the
> > UrlRequest itself).
> 
> An alternative method to achieve the desired result is to add
> data/blob/chrome-extension to the list of safe schemes (currently only http,
> https and ftp are marked safe for redirection).
> A consequence is that servers would also be able to issue redirects to these
> URLs, so the security impact of this should be considered:
> 
> Redirects to chrome-extension: are definitely safe, because only resources
that
> are whitelisted by the extension author are accessible
> (https://developer.chrome.com/extensions/manifest/web_accessible_resources).
> 
> blob:-URLs are unpredictable and short-lived, so I don't see any harm in
> allowing them.
> 
> The safety of data:-URLs is not as easily determined, because they can contain
> any arbitrary data. All data:-URLs have an unique origin in Chromium, so
> allowing them will not decrease the security of HTML pages in Chromium.
Firefox
> and Opera (old) both also allow redirects to data-URLs, which hasn't caused
any
> security issues, except for phishing attempts (using data URLs because they
are
> self-contained, see the overhyped research paper linked at
> https://bugzilla.mozilla.org/show_bug.cgi?id=786275).
> 
> Allowing data/chrome-extension: would cover most use cases for extension
> developers. If an extension wants to use other schemes, then they could
redirect
> to data:somethingUniqueToTheirExtension,...url..., and use the
onBeforeRedirect
> to redirect to their uncommon scheme.
> 
> WDYT?

This would have to go through a careful security review.  I'd be more concerned
about blob urls and extension URLs than data URLs.  From a purely architectural
standpoint, I'm fine with it.

[mmenke, 2014-03-18 17:03:37]

On 2014/03/18 17:00:06, mmenke wrote:
> On 2014/03/17 21:25:23, robwu wrote:
> > On 2014/03/17 19:10:27, mmenke wrote:
> > > On 2014/03/14 18:33:09, mmenke wrote:
> > > > On 2014/03/14 17:36:21, robwu wrote:
> > > > > What do you think of replacing |override_response_headers_| with
> > > > > |override_response_|, of the following type:
> > > > > 
> > > > > struct OverrideResponse {
> > > > >   scoped_refptr<HttpResponseHeaders> headers;
> > > > >   GURL override_redirect;
> > > > > };
> > > 
> > > One problem with this approach is that if someone tries to override a
> redirect
> > > by setting its headers, and someone else does it by the new method, then
> we'll
> > > ignore one, in a way other delegates may not be aware of.  I really don't
> like
> > > an API that can lead to that kind of confusion.
> > 
> > The same argument also applies to overridden response headers, which has
been
> > around for a while without any issues.
> > The proposed API is probably not that confusing; Besides unit tests, there
is
> > only one piece of code that writes a "Location:" header (it just strips
other
> > headers from an existing redirect)
> >
>
(https://code.google.com/p/chromium/codesearch#search/&q=%252522%25255C%252522...).
> 
> Actually, this is a NetworkDelegate function rather than a
URLRequest::Delegate
> one, so there's less concern here than I had thought.  If it were a
> URLRequest::Delegate function, because of all the stacked ResourceHandlers and
> ResourceThrottles, I'd be strongly opposed to this (Since what takes
precedence
> would now determined both by where things are in the handler/throttler stack
> *and* the API they use).  As a NetworkDelegate function, however, it may well
be
> workable.
> 
> > > One option, which also isn't great, is to do two redirects (x->x->dest). 
> The
> > > first redirect would be safe, the second would use OnBeforeURLRequest to
> force
> > a
> > > redirect, and then the third would be the real request.  That wouldn't
> require
> > > any net/ changes.
> > > 
> > > Still open to other suggestions.  In general, I'd much prefer a solution
> where
> > > the UrlRequestHttpJob has no idea what's going on (Not so concerned about
> the
> > > UrlRequest itself).
> > 
> > An alternative method to achieve the desired result is to add
> > data/blob/chrome-extension to the list of safe schemes (currently only http,
> > https and ftp are marked safe for redirection).
> > A consequence is that servers would also be able to issue redirects to these
> > URLs, so the security impact of this should be considered:
> > 
> > Redirects to chrome-extension: are definitely safe, because only resources
> that
> > are whitelisted by the extension author are accessible
> > (https://developer.chrome.com/extensions/manifest/web_accessible_resources).
> > 
> > blob:-URLs are unpredictable and short-lived, so I don't see any harm in
> > allowing them.
> > 
> > The safety of data:-URLs is not as easily determined, because they can
contain
> > any arbitrary data. All data:-URLs have an unique origin in Chromium, so
> > allowing them will not decrease the security of HTML pages in Chromium.
> Firefox
> > and Opera (old) both also allow redirects to data-URLs, which hasn't caused
> any
> > security issues, except for phishing attempts (using data URLs because they
> are
> > self-contained, see the overhyped research paper linked at
> > https://bugzilla.mozilla.org/show_bug.cgi?id=786275).
> > 
> > Allowing data/chrome-extension: would cover most use cases for extension
> > developers. If an extension wants to use other schemes, then they could
> redirect
> > to data:somethingUniqueToTheirExtension,...url..., and use the
> onBeforeRedirect
> > to redirect to their uncommon scheme.
> > 
> > WDYT?
> 
> This would have to go through a careful security review.  I'd be more
concerned
> about blob urls and extension URLs than data URLs.  From a purely
architectural
> standpoint, I'm fine with it.

One other quick comment:  Adding to Chrome's potential attack surface area is
probably not the way to go - while I'm not a huge fan of the alternatives
discussed, this could have far ranging implications, and unless we want to go
down that path for other reasons, probably isn't the best way to go.

[robwu, 2014-03-18 18:00:14]

On 2014/03/18 17:03:37, mmenke wrote:
> On 2014/03/18 17:00:06, mmenke wrote:
> > On 2014/03/17 21:25:23, robwu wrote:
> > > On 2014/03/17 19:10:27, mmenke wrote:
> > > > On 2014/03/14 18:33:09, mmenke wrote:
> > > > > On 2014/03/14 17:36:21, robwu wrote:
> > > > > > What do you think of replacing |override_response_headers_| with
> > > > > > |override_response_|, of the following type:
> > > > > > 
> > > > > > struct OverrideResponse {
> > > > > >   scoped_refptr<HttpResponseHeaders> headers;
> > > > > >   GURL override_redirect;
> > > > > > };
> > > > 
> > > > One problem with this approach is that if someone tries to override a
> > redirect
> > > > by setting its headers, and someone else does it by the new method, then
> > we'll
> > > > ignore one, in a way other delegates may not be aware of.  I really
don't
> > like
> > > > an API that can lead to that kind of confusion.
> > > 
> > > The same argument also applies to overridden response headers, which has
> been
> > > around for a while without any issues.
> > > The proposed API is probably not that confusing; Besides unit tests, there
> is
> > > only one piece of code that writes a "Location:" header (it just strips
> other
> > > headers from an existing redirect)
> > >
> >
>
(https://code.google.com/p/chromium/codesearch#search/&q=%25252522%2525255C%25...).
> > 
> > Actually, this is a NetworkDelegate function rather than a
> URLRequest::Delegate
> > one, so there's less concern here than I had thought.  If it were a
> > URLRequest::Delegate function, because of all the stacked ResourceHandlers
and
> > ResourceThrottles, I'd be strongly opposed to this (Since what takes
> precedence
> > would now determined both by where things are in the handler/throttler stack
> > *and* the API they use).  As a NetworkDelegate function, however, it may
well
> be
> > workable.

So, you're okay with adding a new field |redirect_url| along with
|override_response_headers|, so the network delegate can change the desired
redirection target?
If so, then I will start to code so you can review the result.

> > 
> > > > One option, which also isn't great, is to do two redirects (x->x->dest).

> > The
> > > > first redirect would be safe, the second would use OnBeforeURLRequest to
> > force
> > > a
> > > > redirect, and then the third would be the real request.  That wouldn't
> > require
> > > > any net/ changes.
> > > > 
> > > > Still open to other suggestions.  In general, I'd much prefer a solution
> > where
> > > > the UrlRequestHttpJob has no idea what's going on (Not so concerned
about
> > the
> > > > UrlRequest itself).
> > > 
> > > An alternative method to achieve the desired result is to add
> > > data/blob/chrome-extension to the list of safe schemes (currently only
http,
> > > https and ftp are marked safe for redirection).
> > > A consequence is that servers would also be able to issue redirects to
these
> > > URLs, so the security impact of this should be considered:
> > > 
> > > Redirects to chrome-extension: are definitely safe, because only resources
> > that
> > > are whitelisted by the extension author are accessible
> > >
(https://developer.chrome.com/extensions/manifest/web_accessible_resources).
> > > 
> > > blob:-URLs are unpredictable and short-lived, so I don't see any harm in
> > > allowing them.
> > > 
> > > The safety of data:-URLs is not as easily determined, because they can
> contain
> > > any arbitrary data. All data:-URLs have an unique origin in Chromium, so
> > > allowing them will not decrease the security of HTML pages in Chromium.
> > Firefox
> > > and Opera (old) both also allow redirects to data-URLs, which hasn't
caused
> > any
> > > security issues, except for phishing attempts (using data URLs because
they
> > are
> > > self-contained, see the overhyped research paper linked at
> > > https://bugzilla.mozilla.org/show_bug.cgi?id=786275).
> > > 
> > > Allowing data/chrome-extension: would cover most use cases for extension
> > > developers. If an extension wants to use other schemes, then they could
> > redirect
> > > to data:somethingUniqueToTheirExtension,...url..., and use the
> > onBeforeRedirect
> > > to redirect to their uncommon scheme.
> > > 
> > > WDYT?
> > 
> > This would have to go through a careful security review.  I'd be more
> concerned
> > about blob urls and extension URLs than data URLs.  From a purely
> architectural
> > standpoint, I'm fine with it.
> 
> One other quick comment:  Adding to Chrome's potential attack surface area is
> probably not the way to go - while I'm not a huge fan of the alternatives
> discussed, this could have far ranging implications, and unless we want to go
> down that path for other reasons, probably isn't the best way to go.

Agreed. Although the risks of these schemes are low, the approach should only be
taken if the other methods are really awful.

[mmenke, 2014-03-19 15:02:57]

So here's what I think we should do...  Almost just what you suggest, except the
delegate also updates the headers.  So we have a modified
override_response_headers_ to include a redirect, and also have a GURL
allow_unsafe_redirect_url_, which is a specific URL that we return true on in
IsRedirectSafe (If it's valid).

Otherwise, if we have both modified response headers, and a redirect URL, things
get a little strange, as we'd have to mix data from the two, or have an order of
priority.

So WDYT?  Again, I'm sorry for slowness here - just wanted to take some time to
read through the code so I could come up with something I'm pretty happy with.

I'd also like a pair of unittests where we set override URLs - one where it
matches the redirect header we set, and one where it does not.  This shouldn't
be too hard.  See url_request_unittest, and look for the ERR_UNSAFE_REDIRECT
tests.  Can just copy one of those, but use a custom network delegate.

[robwu, 2014-03-19 22:59:26]

On 2014/03/19 15:02:57, mmenke wrote:
> So here's what I think we should do...  Almost just what you suggest, except
the
> delegate also updates the headers.  So we have a modified
> override_response_headers_ to include a redirect, and also have a GURL
> allow_unsafe_redirect_url_, which is a specific URL that we return true on in
> IsRedirectSafe (If it's valid).
> 
> Otherwise, if we have both modified response headers, and a redirect URL,
things
> get a little strange, as we'd have to mix data from the two, or have an order
of
> priority.
> 
> So WDYT?  Again, I'm sorry for slowness here - just wanted to take some time
to
> read through the code so I could come up with something I'm pretty happy with.

boolean -> GURL is a good idea, done.

> 
> I'd also like a pair of unittests where we set override URLs - one where it
> matches the redirect header we set, and one where it does not.  This shouldn't
> be too hard.  See url_request_unittest, and look for the ERR_UNSAFE_REDIRECT
> tests.  Can just copy one of those, but use a custom network delegate.

Added three tests:
- Redirect to same (unsafe but whitelisted) URL.
- Redirect to different unsafe URL.
- Redirect to safe URL (is this test too defensive? Should it be removed?).

[mmenke, 2014-03-20 15:22:24]

Looks pretty good.  Good job on the tests!

https://codereview.chromium.org/154473002/diff/560001/net/url_request/url_req...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/154473002/diff/560001/net/url_request/url_req...
net/url_request/url_request_http_job.cc:294: transaction_->StopCaching();
I don't think we actually need this.  My cache comment was because I got this
confused with Done(), which would make us finalize the partial cache entry, so
we could reuse it.

If we just delete the URLRequestJob, the HttpCache::Transaction will treat it
like a cancelled request, which we handle fine.

https://codereview.chromium.org/154473002/diff/560001/net/url_request/url_req...
net/url_request/url_request_http_job.cc:1045: if
(GetResponseHeaders()->IsSafeRedirect(location)) {
I don't think this should be part of the headers, but rather a value in the
URLRequestHttpJob.  We'll need to pass another GURL parameter to the delegate,
which is a bit of a pain, but I consider this a property of the job, rather than
a property of the overridden headers.  Know it's a bit of a pain, but being a
safe redirect isn't treated as a fundamental property of the headers.

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

https://codereview.chromium.org/154473002/diff/560001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:497: redirect_url_on_headers_received_
!= request->url()) {
Suggest resetting redirect_url_on_headers_received_ after following the redirect
instead.

https://codereview.chromium.org/154473002/diff/560001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:2715: // Unsafe URL that has been marked
as safe in OverrideRedirectNetworkDelegate.
This stuff (class and globals) should be in an anonymous namespace.

https://codereview.chromium.org/154473002/diff/560001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:2716: const char
kUnsafeUrlMarkedAsSafe[] = "data:text/html,something";
nit:  Blank line before class definition.

optional:  May be better to make this a public static member of
OverrideRedirectNetworkDelegate.  It's often not done in tests, but I think it
makes clearer that it's the TestNetworkDelegate that marks it is safe, so easier
to tell how things work from looking at the test body.

https://codereview.chromium.org/154473002/diff/560001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:2718: // |redirect_url2| (if set).
Fix description.

https://codereview.chromium.org/154473002/diff/560001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:2719: class
OverrideRedirectNetworkDelegate : public TestNetworkDelegate {
Suggest a clearer name.  Maybe "RedirectOnHeadersReceivedNetworkDelegate".  A
bit of a mouthful, but it makes the tests clearer without having to read this
class's description.

https://codereview.chromium.org/154473002/diff/560001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:2831: EXPECT_EQ(safe_url, r.url());
Suggest testing the response body, too, for completeness.

https://codereview.chromium.org/154473002/diff/560001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:2833: }
Hmm...Can we have a double invalid redirect?  Redirect from http://valid to
data://invalid to data://invalid?  I don't think we can since currently only
http URLs allow redirection.  If I'm mistaken, though, we should have a test for
that case, with the first one allowed and the second not, for the same URL.

[mmenke, 2014-03-20 15:22:26]

Looks pretty good.  Good job on the tests!

[mmenke, 2014-03-20 16:13:32]

One more comment.

https://codereview.chromium.org/154473002/diff/560001/net/http/http_response_...
File net/http/http_response_headers.cc (right):

https://codereview.chromium.org/154473002/diff/560001/net/http/http_response_...
net/http/http_response_headers.cc:921: replacements.ClearRef();
Hrm...  Is this needed?

[robwu, 2014-03-20 16:21:10]

Some replies (I will fix the nits without commenting on those).

https://codereview.chromium.org/154473002/diff/560001/net/http/http_response_...
File net/http/http_response_headers.cc (right):

https://codereview.chromium.org/154473002/diff/560001/net/http/http_response_...
net/http/http_response_headers.cc:921: replacements.ClearRef();
On 2014/03/20 16:13:32, mmenke wrote:
> Hrm...  Is this needed?

Yes, URLRequestJob::NotifyHeadersComplete() copies the ref from the original URL
to the redirect URL if the redirect target has no # part.

https://codereview.chromium.org/154473002/diff/560001/net/url_request/url_req...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/154473002/diff/560001/net/url_request/url_req...
net/url_request/url_request_http_job.cc:294: transaction_->StopCaching();
On 2014/03/20 15:22:25, mmenke wrote:
> I don't think we actually need this.  My cache comment was because I got this
> confused with Done(), which would make us finalize the partial cache entry, so
> we could reuse it.
> 
> If we just delete the URLRequestJob, the HttpCache::Transaction will treat it
> like a cancelled request, which we handle fine.

DoneReading() just calls DoneWritingToEntry(true); when the cache transaction is
in writing mode. If we end a request via an overridden redirect, then the cache
transaction ends "successfully". Consequently, the original response headers are
written to the cache, but not the response body.

When I tested my initial CL (before 7) as follows, I ended up with a bad state
(see step 6):
1. Load extension that sets up a redirect for http://example.com/
2. Visit http://example.com/ (note: example.com replies with caching directives)
3. Be redirected to data:text/plain,xxx via the extension.
4. Disable extension.
5. Visit http://localhost/.
6. Observe that the page is load from cache and completely blank. The response
without body also shows up in chrome://cache.

https://codereview.chromium.org/154473002/diff/560001/net/url_request/url_req...
net/url_request/url_request_http_job.cc:1045: if
(GetResponseHeaders()->IsSafeRedirect(location)) {
On 2014/03/20 15:22:25, mmenke wrote:
> I don't think this should be part of the headers, but rather a value in the
> URLRequestHttpJob.  We'll need to pass another GURL parameter to the delegate,
> which is a bit of a pain, but I consider this a property of the job, rather
than
> a property of the overridden headers.  Know it's a bit of a pain, but being a
> safe redirect isn't treated as a fundamental property of the headers.

So, basically the situation from patch set 1?
Then I prefer to use a struct over two parameters, because it is easier to
extend (e.g. with a toggle for status code, http 302 vs 307).

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

https://codereview.chromium.org/154473002/diff/560001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:2833: }
On 2014/03/20 15:22:25, mmenke wrote:
> Hmm...Can we have a double invalid redirect?  Redirect from http://valid to
> data://invalid to data://invalid?  I don't think we can since currently only
> http URLs allow redirection.  If I'm mistaken, though, we should have a test
for
> that case, with the first one allowed and the second not, for the same URL.

Only HTTP responses can be redirected at onHeadersReceived, so this test case is
not needed.

[mmenke, 2014-03-20 16:40:27]

https://codereview.chromium.org/154473002/diff/560001/net/http/http_response_...
File net/http/http_response_headers.cc (right):

https://codereview.chromium.org/154473002/diff/560001/net/http/http_response_...
net/http/http_response_headers.cc:921: replacements.ClearRef();
On 2014/03/20 16:21:11, robwu wrote:
> On 2014/03/20 16:13:32, mmenke wrote:
> > Hrm...  Is this needed?
> 
> Yes, URLRequestJob::NotifyHeadersComplete() copies the ref from the original
URL
> to the redirect URL if the redirect target has no # part.

Thanks for the pointer!  Then we should clear the ref on
allowed_unsafe_redirect_url_ as well (Since the forced redirect may have a ref,
and we can't rely on the caller to know it should be removed).

Should have some more tests for this logic.

Suggested cases:
* First url has ref, redirect and allowed_unsafe_redirect_url_ doesn't have a
ref.
* First url has no ref, redirect and allowed_unsafe_redirect_url_ have a ref
* First url has a ref, redirect and allowed_unsafe_redirect_url_ have a
different ref

https://codereview.chromium.org/154473002/diff/560001/net/url_request/url_req...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/154473002/diff/560001/net/url_request/url_req...
net/url_request/url_request_http_job.cc:294: transaction_->StopCaching();
On 2014/03/20 16:21:11, robwu wrote:
> On 2014/03/20 15:22:25, mmenke wrote:
> > I don't think we actually need this.  My cache comment was because I got
this
> > confused with Done(), which would make us finalize the partial cache entry,
so
> > we could reuse it.
> > 
> > If we just delete the URLRequestJob, the HttpCache::Transaction will treat
it
> > like a cancelled request, which we handle fine.
> 
> DoneReading() just calls DoneWritingToEntry(true); when the cache transaction
is
> in writing mode. If we end a request via an overridden redirect, then the
cache
> transaction ends "successfully". Consequently, the original response headers
are
> written to the cache, but not the response body.
> 
> When I tested my initial CL (before 7) as follows, I ended up with a bad state
> (see step 6):
> 1. Load extension that sets up a redirect for http://example.com/
> 2. Visit http://example.com/ (note: http://example.com replies with caching
directives)
> 3. Be redirected to data:text/plain,xxx via the extension.
> 4. Disable extension.
> 5. Visit http://localhost/.
> 6. Observe that the page is load from cache and completely blank. The response
> without body also shows up in chrome://cache.

We should have a test for that, too, then.

Does this mean (without the patch) if anything overrides the headers of a
non-redirect to be a "safe" redirect, we end up with a bad cache entry?

https://codereview.chromium.org/154473002/diff/560001/net/url_request/url_req...
net/url_request/url_request_http_job.cc:1045: if
(GetResponseHeaders()->IsSafeRedirect(location)) {
On 2014/03/20 16:21:11, robwu wrote:
> On 2014/03/20 15:22:25, mmenke wrote:
> > I don't think this should be part of the headers, but rather a value in the
> > URLRequestHttpJob.  We'll need to pass another GURL parameter to the
delegate,
> > which is a bit of a pain, but I consider this a property of the job, rather
> than
> > a property of the overridden headers.  Know it's a bit of a pain, but being
a
> > safe redirect isn't treated as a fundamental property of the headers.
> 
> So, basically the situation from patch set 1?
> Then I prefer to use a struct over two parameters, because it is easier to
> extend (e.g. with a toggle for status code, http 302 vs 307).

We allow overriding the entire headers, so I don't think we'd need the 302 vs
307 (Overriding the whole headers for that may be overkill, but I prefer that to
adding more cruft to the interface for things the embedder of the stack can
already do, and again, if you override both, that gives rise to ambiguities).

I'm weakly opposed to this largely on style grounds - it's not the style the
network stack currently uses, and this interface does not change frequently
enough that I think future changes are a big concern.

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

https://codereview.chromium.org/154473002/diff/560001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:2833: }
On 2014/03/20 16:21:11, robwu wrote:
> On 2014/03/20 15:22:25, mmenke wrote:
> > Hmm...Can we have a double invalid redirect?  Redirect from http://valid to
> > data://invalid to data://invalid?  I don't think we can since currently only
> > http URLs allow redirection.  If I'm mistaken, though, we should have a test
> for
> > that case, with the first one allowed and the second not, for the same URL.
> 
> Only HTTP responses can be redirected at onHeadersReceived, so this test case
is
> not needed.

And we always allow redirects to HTTP/HTTPS, of course.  Allrighty.

[mmenke, 2014-03-20 17:22:37]

https://codereview.chromium.org/154473002/diff/560001/net/url_request/url_req...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/154473002/diff/560001/net/url_request/url_req...
net/url_request/url_request_http_job.cc:294: transaction_->StopCaching();
On 2014/03/20 16:40:28, mmenke wrote:
> On 2014/03/20 16:21:11, robwu wrote:
> > On 2014/03/20 15:22:25, mmenke wrote:
> > > I don't think we actually need this.  My cache comment was because I got
> this
> > > confused with Done(), which would make us finalize the partial cache
entry,
> so
> > > we could reuse it.
> > > 
> > > If we just delete the URLRequestJob, the HttpCache::Transaction will treat
> it
> > > like a cancelled request, which we handle fine.
> > 
> > DoneReading() just calls DoneWritingToEntry(true); when the cache
transaction
> is
> > in writing mode. If we end a request via an overridden redirect, then the
> cache
> > transaction ends "successfully". Consequently, the original response headers
> are
> > written to the cache, but not the response body.
> > 
> > When I tested my initial CL (before 7) as follows, I ended up with a bad
state
> > (see step 6):
> > 1. Load extension that sets up a redirect for http://example.com/
> > 2. Visit http://example.com/ (note: http://example.com replies with caching
> directives)
> > 3. Be redirected to data:text/plain,xxx via the extension.
> > 4. Disable extension.
> > 5. Visit http://localhost/.
> > 6. Observe that the page is load from cache and completely blank. The
response
> > without body also shows up in chrome://cache.
> 
> We should have a test for that, too, then.
> 
> Does this mean (without the patch) if anything overrides the headers of a
> non-redirect to be a "safe" redirect, we end up with a bad cache entry?

Turns out this is a bug added back in September by
https://chromiumcodereview.appspot.com/23710059 (Which I reviewed...Hrm...). 
Don't worry about doing anything with the cache, I'll fix that issue today or
tomorrow, which should make this problem go away.

[robwu, 2014-03-20 17:28:14]

https://codereview.chromium.org/154473002/diff/560001/net/http/http_response_...
File net/http/http_response_headers.cc (right):

https://codereview.chromium.org/154473002/diff/560001/net/http/http_response_...
net/http/http_response_headers.cc:921: replacements.ClearRef();
On 2014/03/20 16:40:28, mmenke wrote:
> On 2014/03/20 16:21:11, robwu wrote:
> > On 2014/03/20 16:13:32, mmenke wrote:
> > > Hrm...  Is this needed?
> > 
> > Yes, URLRequestJob::NotifyHeadersComplete() copies the ref from the original
> URL
> > to the redirect URL if the redirect target has no # part.
> 
> Thanks for the pointer!  Then we should clear the ref on
> allowed_unsafe_redirect_url_ as well (Since the forced redirect may have a
ref,
> and we can't rely on the caller to know it should be removed).

The ref is already cleared on both URLs.

> 
> Should have some more tests for this logic.
> 
> Suggested cases:
> * First url has ref, redirect and allowed_unsafe_redirect_url_ doesn't have a
> ref.
> * First url has no ref, redirect and allowed_unsafe_redirect_url_ have a ref
> * First url has a ref, redirect and allowed_unsafe_redirect_url_ have a
> different ref

Will add.

https://codereview.chromium.org/154473002/diff/560001/net/url_request/url_req...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/154473002/diff/560001/net/url_request/url_req...
net/url_request/url_request_http_job.cc:294: transaction_->StopCaching();
On 2014/03/20 17:22:38, mmenke wrote:
> On 2014/03/20 16:40:28, mmenke wrote:
> > On 2014/03/20 16:21:11, robwu wrote:
> > > On 2014/03/20 15:22:25, mmenke wrote:
> > > > I don't think we actually need this.  My cache comment was because I got
> > this
> > > > confused with Done(), which would make us finalize the partial cache
> entry,
> > so
> > > > we could reuse it.
> > > > 
> > > > If we just delete the URLRequestJob, the HttpCache::Transaction will
treat
> > it
> > > > like a cancelled request, which we handle fine.
> > > 
> > > DoneReading() just calls DoneWritingToEntry(true); when the cache
> transaction
> > is
> > > in writing mode. If we end a request via an overridden redirect, then the
> > cache
> > > transaction ends "successfully". Consequently, the original response
headers
> > are
> > > written to the cache, but not the response body.
> > > 
> > > When I tested my initial CL (before 7) as follows, I ended up with a bad
> state
> > > (see step 6):
> > > 1. Load extension that sets up a redirect for http://example.com/
> > > 2. Visit http://example.com/ (note: http://example.com replies with
caching
> > directives)
> > > 3. Be redirected to data:text/plain,xxx via the extension.
> > > 4. Disable extension.
> > > 5. Visit http://localhost/.
> > > 6. Observe that the page is load from cache and completely blank. The
> response
> > > without body also shows up in chrome://cache.
> > 
> > We should have a test for that, too, then.
> > 
> > Does this mean (without the patch) if anything overrides the headers of a
> > non-redirect to be a "safe" redirect, we end up with a bad cache entry?
> 
> Turns out this is a bug added back in September by
> https://chromiumcodereview.appspot.com/23710059 (Which I reviewed...Hrm...). 
> Don't worry about doing anything with the cache, I'll fix that issue today or
> tomorrow, which should make this problem go away.

Okay, great!
Do you think whether it's acceptable to move the processing of the caching
directives after handling the headers? That would also have fixed this issue,
and furthermore allow extensions to control the caching behavior of sites (by
modifying the response headers).

https://codereview.chromium.org/154473002/diff/560001/net/url_request/url_req...
net/url_request/url_request_http_job.cc:1045: if
(GetResponseHeaders()->IsSafeRedirect(location)) {
On 2014/03/20 16:40:28, mmenke wrote:
> On 2014/03/20 16:21:11, robwu wrote:
> > On 2014/03/20 15:22:25, mmenke wrote:
> > > I don't think this should be part of the headers, but rather a value in
the
> > > URLRequestHttpJob.  We'll need to pass another GURL parameter to the
> delegate,
> > > which is a bit of a pain, but I consider this a property of the job,
rather
> > than
> > > a property of the overridden headers.  Know it's a bit of a pain, but
being
> a
> > > safe redirect isn't treated as a fundamental property of the headers.
> > 
> > So, basically the situation from patch set 1?
> > Then I prefer to use a struct over two parameters, because it is easier to
> > extend (e.g. with a toggle for status code, http 302 vs 307).
> 
> We allow overriding the entire headers, so I don't think we'd need the 302 vs
> 307 (Overriding the whole headers for that may be overkill, but I prefer that
to
> adding more cruft to the interface for things the embedder of the stack can
> already do, and again, if you override both, that gives rise to ambiguities).
> 
> I'm weakly opposed to this largely on style grounds - it's not the style the
> network stack currently uses, and this interface does not change frequently
> enough that I think future changes are a big concern.

After re-reading your previous comment, I see that you meant to pass the safe
redirect target via the delegate instead of the headers, and keep the redirect
override on the response headers. Makes sense.

[mmenke, 2014-03-20 17:31:58]

https://codereview.chromium.org/154473002/diff/560001/net/url_request/url_req...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/154473002/diff/560001/net/url_request/url_req...
net/url_request/url_request_http_job.cc:1045: if
(GetResponseHeaders()->IsSafeRedirect(location)) {
On 2014/03/20 17:28:14, robwu wrote:
> On 2014/03/20 16:40:28, mmenke wrote:
> > On 2014/03/20 16:21:11, robwu wrote:
> > > On 2014/03/20 15:22:25, mmenke wrote:
> > > > I don't think this should be part of the headers, but rather a value in
> the
> > > > URLRequestHttpJob.  We'll need to pass another GURL parameter to the
> > delegate,
> > > > which is a bit of a pain, but I consider this a property of the job,
> rather
> > > than
> > > > a property of the overridden headers.  Know it's a bit of a pain, but
> being
> > a
> > > > safe redirect isn't treated as a fundamental property of the headers.
> > > 
> > > So, basically the situation from patch set 1?
> > > Then I prefer to use a struct over two parameters, because it is easier to
> > > extend (e.g. with a toggle for status code, http 302 vs 307).
> > 
> > We allow overriding the entire headers, so I don't think we'd need the 302
vs
> > 307 (Overriding the whole headers for that may be overkill, but I prefer
that
> to
> > adding more cruft to the interface for things the embedder of the stack can
> > already do, and again, if you override both, that gives rise to
ambiguities).
> > 
> > I'm weakly opposed to this largely on style grounds - it's not the style the
> > network stack currently uses, and this interface does not change frequently
> > enough that I think future changes are a big concern.
> 
> After re-reading your previous comment, I see that you meant to pass the safe
> redirect target via the delegate instead of the headers, and keep the redirect
> override on the response headers. Makes sense.

Not sure you're completely following me...  Here's what I'm thinking:

When calling into the network delegate, just have:

network_delegate()->OnHeadersReceived(... &override_headers_,
&allow_unsafe_redirect_url_);

And then here, just have:
if (allow_unsafe_redirect_url_.is_valid() && allow_unsafe_redirect_url_ ==
location)
  return true;

(Doing all the ref clearing that's needed, of course).

[robwu, 2014-03-21 14:39:51]

mmenke, could you review:

net/url_request/url_request_http_job.cc and
net/url_request/url_request_unittest.cc

I have moved the allowed_safe_redirect_url from the headers to the network
delegate API (and had to edit several other files to match this new format),
added some new tests (for the reference fragment) and removed the StopCaching()
code (because you're going to fix that bug).

[mmenke, 2014-03-21 18:15:02]

This looks pretty good.  I'll want to land my CL first, just so we never surface
the bug (Will send it out for review later today, after the trybots pass).

Two merge notes:
I added tests in two files, so I put your code to set a NetworkDelegate redirect
URL in TestNetworkDelegate, so after merging, you should move your override url
code there.  So you can remove the changes to BlockingNetworkDelegate, and get
rid of RedirectOnHeadersReceivedNetworkDelegate.

I put my tests near the existing redirect tests.  I suggest you put your tests
together there as well, just to keep related tests together.

https://codereview.chromium.org/154473002/diff/580001/net/url_request/url_req...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/154473002/diff/580001/net/url_request/url_req...
net/url_request/url_request_http_job.cc:809: allowed_unsafe_redirect_url_ =
GURL();
Should just always clear it here.  Code size and readability matter more here.

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

https://codereview.chromium.org/154473002/diff/580001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:2857: }
This test is indeed probably overkill.

https://codereview.chromium.org/154473002/diff/580001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:2866: GURL
expected_url("data:,url-marked-safe-and-used-in-redirect#fragment");
Suggest a linebreak after the GURLs in all of these, for easier reading.

https://codereview.chromium.org/154473002/diff/580001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:2910: }
I don't think we need this test.  But I do think we should have a test where the
redirect_url has a different fragment from the original url.

https://codereview.chromium.org/154473002/diff/580001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:3257: // preserve POST data.
Both these test are probably overkill as well.  More tests is generally good,
but during refactorings, there's a non-zero costs for extra tests.

If you want async tests, could just turn the other tests into async tests.

[robwu, 2014-03-25 00:03:59]

Dominic, PTAL.
I have changed 307 to 302 redirects at onHeadersReceived, because I cannot
imagine a use case where 307 as default would be more beneficial than 302. With
307 redirects, extension authors risk leaking POST data from one site to another
site, which sounds bad.
Besides, 302 redirects are the most common redirects on the web, so this
probably matches the expectations of (web) developers.

mmenke, please review my changes. There are no functional changes, I just
addressed your comments.

https://codereview.chromium.org/154473002/diff/580001/net/url_request/url_req...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/154473002/diff/580001/net/url_request/url_req...
net/url_request/url_request_http_job.cc:809: allowed_unsafe_redirect_url_ =
GURL();
On 2014/03/21 18:15:04, mmenke wrote:
> Should just always clear it here.  Code size and readability matter more here.

Done.

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

https://codereview.chromium.org/154473002/diff/580001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:2857: }
On 2014/03/21 18:15:04, mmenke wrote:
> This test is indeed probably overkill.

Indeed, removed; This case is already covered by the
NetworkDelegateRedirectRequestOnHeadersReceived test.

https://codereview.chromium.org/154473002/diff/580001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:2866: GURL
expected_url("data:,url-marked-safe-and-used-in-redirect#fragment");
On 2014/03/21 18:15:04, mmenke wrote:
> Suggest a linebreak after the GURLs in all of these, for easier reading.

Done.

https://codereview.chromium.org/154473002/diff/580001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:2910: }
On 2014/03/21 18:15:04, mmenke wrote:
> I don't think we need this test.  But I do think we should have a test where
the
> redirect_url has a different fragment from the original url.

Currently, the test is indeed redundant (it merely confirms the current
behavior).
However, after landing this feature, I will submit another patch for
http://crbug.com/354653, right after landing this patch. Then, the expected
behavior becomes "Do not add a fragment even if the original URL has a
fragment". This is important and must be tested (then expected_redirect_url will
be removed, and the expectation is that r.url() is equal to redirect_url).

https://codereview.chromium.org/154473002/diff/580001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:3257: // preserve POST data.
On 2014/03/21 18:15:04, mmenke wrote:
> Both these test are probably overkill as well.  More tests is generally good,
> but during refactorings, there's a non-zero costs for extra tests.
> 
> If you want async tests, could just turn the other tests into async tests.

You're right. What I actually wanted to do was to test the correctness of the
webRequest API. That is already done with the new tests in the extensions/
namespace, so I've removed these two tests.

[battre, 2014-03-25 00:47:29]

I think that the 307 redirects were intentional so that extensions like HTTPS
everywhere can redirect logins to a SSL domain.

https://codereview.chromium.org/154473002/diff/620001/chrome/common/extension...
File chrome/common/extensions/docs/templates/intros/declarativeWebRequest.html
(right):

https://codereview.chromium.org/154473002/diff/620001/chrome/common/extension...
chrome/common/extensions/docs/templates/intros/declarativeWebRequest.html:202:
stage, then POST requests will be redirected using the GET method. In other
cases
nit: 80 characters

https://codereview.chromium.org/154473002/diff/620001/chrome/common/extension...
chrome/common/extensions/docs/templates/intros/declarativeWebRequest.html:203:
the HTTP method is preserved.
Why is this?

https://codereview.chromium.org/154473002/diff/620001/net/url_request/url_req...
File net/url_request/url_request_test_util.h (right):

https://codereview.chromium.org/154473002/diff/620001/net/url_request/url_req...
net/url_request/url_request_test_util.h:312: GURL allowed_unsafe_redirect_url_;
I think it would be good to add a comment here that explains what the variable
means.

[robwu, 2014-03-25 10:08:14]

On 2014/03/25 00:47:29, battre wrote:
> I think that the 307 redirects were intentional so that extensions like HTTPS
> everywhere can redirect logins to a SSL domain.

onBeforeRedirect still use 307 redirects. I've initially used 307 at
onHeadersReceived to match this behavior. Later, I decided to switch to 302
because I can't think of a use case where preserving POST is preferred.

Although the property names are identical, the expectations differ:
redirectUrl at onBeforeRequest = rewrite the URL (implies only change URL, don't
change anything else)
redirectUrl at onHeadersReceived = redirect URL (implies "follow the redirect";
302 is far more common than 307)

https://codereview.chromium.org/154473002/diff/620001/chrome/common/extension...
File chrome/common/extensions/docs/templates/intros/declarativeWebRequest.html
(right):

https://codereview.chromium.org/154473002/diff/620001/chrome/common/extension...
chrome/common/extensions/docs/templates/intros/declarativeWebRequest.html:202:
stage, then POST requests will be redirected using the GET method. In other
cases
On 2014/03/25 00:47:30, battre wrote:
> nit: 80 characters

Done.

https://codereview.chromium.org/154473002/diff/620001/chrome/common/extension...
chrome/common/extensions/docs/templates/intros/declarativeWebRequest.html:203:
the HTTP method is preserved.
On 2014/03/25 00:47:30, battre wrote:
> Why is this?

onBeforeRequest = 307 redirect, which doesn't change the HTTP method.
onHeadersReceived = 302 redirect, which changes the HTTP method if the original
request was a POST request. I have changed the note to make this difference more
obvious.

https://codereview.chromium.org/154473002/diff/620001/net/url_request/url_req...
File net/url_request/url_request_test_util.h (right):

https://codereview.chromium.org/154473002/diff/620001/net/url_request/url_req...
net/url_request/url_request_test_util.h:312: GURL allowed_unsafe_redirect_url_;
On 2014/03/25 00:47:30, battre wrote:
> I think it would be good to add a comment here that explains what the variable
> means.

Done.

[mmenke, 2014-03-25 15:08:33]

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

https://codereview.chromium.org/154473002/diff/640001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:2728: EXPECT_EQ(0, r.status().error());
This should be net::OK instead of 0.

https://codereview.chromium.org/154473002/diff/640001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:2729: EXPECT_EQ(unsafe_url, r.url());
Suggest checking the body, for completeness.

https://codereview.chromium.org/154473002/diff/640001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:2757: }
On 2014/03/25 00:04:00, robwu wrote:
> On 2014/03/21 18:15:04, mmenke wrote:
> > I don't think we need this test.  But I do think we should have a test where
> the
> > redirect_url has a different fragment from the original url.
> 
> Currently, the test is indeed redundant (it merely confirms the current
> behavior).
> However, after landing this feature, I will submit another patch for
> http://crbug.com/354653, right after landing this patch. Then, the expected
> behavior becomes "Do not add a fragment even if the original URL has a
> fragment". This is important and must be tested (then expected_redirect_url
will
> be removed, and the expectation is that r.url() is equal to redirect_url).

Sorry, there were two parts to my comment (I should learn not to do that):  I
also wanted a test where the unsafe url we force a redirect to has a fragment
(And ideally, the source URL has a different fragment).

Once that's done, I'm happy with this.

[robwu, 2014-03-25 15:59:34]

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

https://codereview.chromium.org/154473002/diff/640001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:2728: EXPECT_EQ(0, r.status().error());
On 2014/03/25 15:08:34, mmenke wrote:
> This should be net::OK instead of 0.

Done.

https://codereview.chromium.org/154473002/diff/640001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:2729: EXPECT_EQ(unsafe_url, r.url());
On 2014/03/25 15:08:34, mmenke wrote:
> Suggest checking the body, for completeness.

Done.

https://codereview.chromium.org/154473002/diff/640001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:2757: }
On 2014/03/25 15:08:34, mmenke wrote:
> On 2014/03/25 00:04:00, robwu wrote:
> > On 2014/03/21 18:15:04, mmenke wrote:
> > > I don't think we need this test.  But I do think we should have a test
where
> > the
> > > redirect_url has a different fragment from the original url.
> > 
> > Currently, the test is indeed redundant (it merely confirms the current
> > behavior).
> > However, after landing this feature, I will submit another patch for
> > http://crbug.com/354653, right after landing this patch. Then, the expected
> > behavior becomes "Do not add a fragment even if the original URL has a
> > fragment". This is important and must be tested (then expected_redirect_url
> will
> > be removed, and the expectation is that r.url() is equal to redirect_url).
> 
> Sorry, there were two parts to my comment (I should learn not to do that):  I
> also wanted a test where the unsafe url we force a redirect to has a fragment
> (And ideally, the source URL has a different fragment).
> 
> Once that's done, I'm happy with this.

Done.

[mmenke, 2014-03-25 20:27:37]

net/ and chrome/browser/net LGTM

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

https://codereview.chromium.org/154473002/diff/690001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:2708: TEST_F(URLRequestTestHTTP,
UnsafeRedirectToWhitelistedUnsafeURL) {
nit:  These should go down with the other unsaferedirect tests.

https://codereview.chromium.org/154473002/diff/690001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:5366: GURL
original_url(test_server_.GetURL("original#fragment"));
Should make it so that the fragment here is not a substring of framgent2, out of
general paranoia (fragment1 will do).

[robwu, 2014-03-25 21:23:18]

Dominic: Using 302 instead of 307 for the implementation of redirectUrl at
onHeadersReceived is a significant change. Could you confirm your previous LGTM?
See my previous comments for the rationale behind this change.

mmenke: Thanks, fixed nits.

[battre, 2014-03-25 23:19:19]

Yes, still LGTM.

Thanks and kudos. Great job!

Best regards,
Dominic

[robwu, 2014-03-25 23:19:53]

The CQ bit was checked by rob@robwu.nl

[commit-bot: I haz the power, 2014-03-25 23:20:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rob@robwu.nl/154473002/740001

[commit-bot: I haz the power, 2014-03-26 00:24:07]

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

[commit-bot: I haz the power, 2014-03-26 00:24:07]

Retried try job too often on android_clang_dbg for step(s) slave_steps
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...

[robwu, 2014-03-26 08:37:17]

The CQ bit was checked by rob@robwu.nl

[commit-bot: I haz the power, 2014-03-26 08:37:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rob@robwu.nl/154473002/990001

[commit-bot: I haz the power, 2014-03-26 10:41:27]

Change committed as 259546