Move handling of invalid referrer to the network delegate

Move handling of invalid referrer to the network delegate

We now always abort invalid loads. According to UMA, they don't happen
anymore, so this should only affect future bugs.

Also, move the UMA reporting to the right thread.

BUG=none
R=mmenke@chromium.org,mef@chromium.org,nasko@chromium.org

Committed: https://crrev.com/0e3b3a6c53b408ea11cdab0a46d12ea905782821
Cr-Commit-Position: refs/heads/master@{#295107}

[jochen (gone - plz use gerrit), 2014-09-16 13:18:12]

ptal

Mark, everything

Misha, Nasko: fyi

[jochen (gone - plz use gerrit), 2014-09-16 13:37:44]

jochen@chromium.org changed reviewers:
+ mmenke@chromium.org
- mark@chromium.org

[jochen (gone - plz use gerrit), 2014-09-16 13:37:44]

sorry, got the reviewer list wrong

Matt, please review everything :)

[mmenke, 2014-09-16 15:05:42]

This looks reasonable to me, just wonder about naming.

https://codereview.chromium.org/572273002/diff/20001/net/url_request/url_requ...
File net/url_request/url_request.cc (right):

https://codereview.chromium.org/572273002/diff/20001/net/url_request/url_requ...
net/url_request/url_request.cc:694:
network_delegate_->CanCorrectInvalidReferrerHeader(
I think this function name isn't clear.  We only call this on secure to insecure
transitions, and when referrer policy is
"CLEAR_REFERRER_ON_TRANSITION_FROM_SECURE_TO_INSECURE"...And in that case, we
fail requests rather than clearing the referrer, which seems a bit odd, given
the referrer policy.

I think we should rename "CLEAR_REFERRER_ON_TRANSITION_FROM_SECURE_TO_INSECURE"
to something like "DISALLOW_REFERRER_ON_TRANSITION_FROM_SECURE_TO_INSECURE",
with a comment that the referrer will be cleared in that case, though the
delegate can optionally opt to block the request instead.

I also think "CanCorrectInvalidReferrerHeader" would be muck clearer as
"ShouldBlockRequestWithSecureReferrerToInsecure" or somesuch.

Open to other ideas, just think both of these names could better reflect the new
behavior.

https://codereview.chromium.org/572273002/diff/20001/net/url_request/url_requ...
net/url_request/url_request.cc:699: // when starting the error job.
nit:  Don't use "we" in comments.

[mmenke, 2014-09-16 15:53:57]

https://codereview.chromium.org/572273002/diff/20001/chrome/browser/net/chrom...
File chrome/browser/net/chrome_network_delegate.cc (right):

https://codereview.chromium.org/572273002/diff/20001/chrome/browser/net/chrom...
chrome/browser/net/chrome_network_delegate.cc:827: return false;
Should we only do this for RDH-tracked requests?  Also, can WebRequest
extensions cause requests to end up here?  I can't remember if they modify the
initial URL of requests, or can only set the URL by creating fake redirects.

[jochen (gone - plz use gerrit), 2014-09-16 16:27:57]

https://codereview.chromium.org/572273002/diff/20001/net/url_request/url_requ...
File net/url_request/url_request.cc (right):

https://codereview.chromium.org/572273002/diff/20001/net/url_request/url_requ...
net/url_request/url_request.cc:694:
network_delegate_->CanCorrectInvalidReferrerHeader(
On 2014/09/16 15:05:42, mmenke wrote:
> I think this function name isn't clear.  We only call this on secure to
insecure
> transitions, and when referrer policy is
> "CLEAR_REFERRER_ON_TRANSITION_FROM_SECURE_TO_INSECURE"...And in that case, we
> fail requests rather than clearing the referrer, which seems a bit odd, given
> the referrer policy.

That's not correct. The transition happens here:
URLRequestJob::ComputeRedirectInfo

This code path is taken if somebody created an URLRequest with incorrect initial
data.

> 
> I think we should rename
"CLEAR_REFERRER_ON_TRANSITION_FROM_SECURE_TO_INSECURE"
> to something like "DISALLOW_REFERRER_ON_TRANSITION_FROM_SECURE_TO_INSECURE",
> with a comment that the referrer will be cleared in that case, though the
> delegate can optionally opt to block the request instead.



> 
> I also think "CanCorrectInvalidReferrerHeader" would be muck clearer as
> "ShouldBlockRequestWithSecureReferrerToInsecure" or somesuch.


We might want to add different policies in the future, like origin-on-transition
or something. You could then create an URLRequest with a non-origin referrer
that would then also get handled here.

> 
> Open to other ideas, just think both of these names could better reflect the
new
> behavior.

[jochen (gone - plz use gerrit), 2014-09-16 16:28:49]

On 2014/09/16 at 15:53:57, mmenke wrote:
>
https://codereview.chromium.org/572273002/diff/20001/chrome/browser/net/chrom...
> File chrome/browser/net/chrome_network_delegate.cc (right):
> 
>
https://codereview.chromium.org/572273002/diff/20001/chrome/browser/net/chrom...
> chrome/browser/net/chrome_network_delegate.cc:827: return false;
> Should we only do this for RDH-tracked requests?  Also, can WebRequest
extensions cause requests to end up here?  I can't remember if they modify the
initial URL of requests, or can only set the URL by creating fake redirects.

it should happen for all requests.

extensions are not allowed to mock with the referrer, I think it's automatically
computed for them. I'll double check

[mmenke, 2014-09-16 16:30:45]

On 2014/09/16 16:28:49, jochen wrote:
> On 2014/09/16 at 15:53:57, mmenke wrote:
> >
>
https://codereview.chromium.org/572273002/diff/20001/chrome/browser/net/chrom...
> > File chrome/browser/net/chrome_network_delegate.cc (right):
> > 
> >
>
https://codereview.chromium.org/572273002/diff/20001/chrome/browser/net/chrom...
> > chrome/browser/net/chrome_network_delegate.cc:827: return false;
> > Should we only do this for RDH-tracked requests?  Also, can WebRequest
> extensions cause requests to end up here?  I can't remember if they modify the
> initial URL of requests, or can only set the URL by creating fake redirects.
> 
> it should happen for all requests.
> 
> extensions are not allowed to mock with the referrer, I think it's
automatically
> computed for them. I'll double check

Extensions are allowed to muck with the URL, though...But I'm not sure if they
can do it before a request is created, or only afterwards (Afterwards they can
create bogus redirects, which should be handled.  Before a request is issues,
though, I'm not sure).

[mmenke, 2014-09-16 16:37:15]

https://codereview.chromium.org/572273002/diff/20001/net/url_request/url_requ...
File net/url_request/url_request.cc (right):

https://codereview.chromium.org/572273002/diff/20001/net/url_request/url_requ...
net/url_request/url_request.cc:694:
network_delegate_->CanCorrectInvalidReferrerHeader(
On 2014/09/16 16:27:57, jochen wrote:
> On 2014/09/16 15:05:42, mmenke wrote:
> > I think this function name isn't clear.  We only call this on secure to
> insecure
> > transitions, and when referrer policy is
> > "CLEAR_REFERRER_ON_TRANSITION_FROM_SECURE_TO_INSECURE"...And in that case,
we
> > fail requests rather than clearing the referrer, which seems a bit odd,
given
> > the referrer policy.
> 
> That's not correct. The transition happens here:
> URLRequestJob::ComputeRedirectInfo
> 
> This code path is taken if somebody created an URLRequest with incorrect
initial
> data.

Ahh...  I think that's not at all clear from the source here.  Suppose it could
also be caused by someone changing the referrer policy after start.

> > I think we should rename
> "CLEAR_REFERRER_ON_TRANSITION_FROM_SECURE_TO_INSECURE"
> > to something like "DISALLOW_REFERRER_ON_TRANSITION_FROM_SECURE_TO_INSECURE",
> > with a comment that the referrer will be cleared in that case, though the
> > delegate can optionally opt to block the request instead.
> 
> 
> 
> > 
> > I also think "CanCorrectInvalidReferrerHeader" would be muck clearer as
> > "ShouldBlockRequestWithSecureReferrerToInsecure" or somesuch.
> 
> 
> We might want to add different policies in the future, like
origin-on-transition
> or something. You could then create an URLRequest with a non-origin referrer
> that would then also get handled here.

I still think that "CanCorrectInvalidReferrerHeader" is a very misleading name. 
Just looking at it, I'd assume that false would mean keep the invalid header, or
clear it, but that's not the case.  It's also not clear that "invalid" doesn't
mean !GURL(referrer_).is_valid().

[jochen (gone - plz use gerrit), 2014-09-16 17:03:16]

On 2014/09/16 at 16:30:45, mmenke wrote:
> On 2014/09/16 16:28:49, jochen wrote:
> > On 2014/09/16 at 15:53:57, mmenke wrote:
> > >
> >
https://codereview.chromium.org/572273002/diff/20001/chrome/browser/net/chrom...
> > > File chrome/browser/net/chrome_network_delegate.cc (right):
> > > 
> > >
> >
https://codereview.chromium.org/572273002/diff/20001/chrome/browser/net/chrom...
> > > chrome/browser/net/chrome_network_delegate.cc:827: return false;
> > > Should we only do this for RDH-tracked requests?  Also, can WebRequest
> > extensions cause requests to end up here?  I can't remember if they modify
the
> > initial URL of requests, or can only set the URL by creating fake redirects.
> > 
> > it should happen for all requests.
> > 
> > extensions are not allowed to mock with the referrer, I think it's
automatically
> > computed for them. I'll double check
> 
> Extensions are allowed to muck with the URL, though...But I'm not sure if they
can do it before a request is created, or only afterwards (Afterwards they can
create bogus redirects, which should be handled.  Before a request is issues,
though, I'm not sure).

if the extension changes the URL, we always do an URLRequestRedirectJob

[jochen (gone - plz use gerrit), 2014-09-16 17:04:21]

On 2014/09/16 at 16:37:15, mmenke wrote:
>
https://codereview.chromium.org/572273002/diff/20001/net/url_request/url_requ...
> File net/url_request/url_request.cc (right):
> 
>
https://codereview.chromium.org/572273002/diff/20001/net/url_request/url_requ...
> net/url_request/url_request.cc:694:
network_delegate_->CanCorrectInvalidReferrerHeader(
> On 2014/09/16 16:27:57, jochen wrote:
> > On 2014/09/16 15:05:42, mmenke wrote:
> > > I think this function name isn't clear.  We only call this on secure to
> > insecure
> > > transitions, and when referrer policy is
> > > "CLEAR_REFERRER_ON_TRANSITION_FROM_SECURE_TO_INSECURE"...And in that case,
we
> > > fail requests rather than clearing the referrer, which seems a bit odd,
given
> > > the referrer policy.
> > 
> > That's not correct. The transition happens here:
> > URLRequestJob::ComputeRedirectInfo
> > 
> > This code path is taken if somebody created an URLRequest with incorrect
initial
> > data.
> 
> Ahh...  I think that's not at all clear from the source here.  Suppose it
could also be caused by someone changing the referrer policy after start.

there's a DCHECK() for that in set_referrer_policy(), but yes, that could happen
in a release build


> 
> > > I think we should rename
> > "CLEAR_REFERRER_ON_TRANSITION_FROM_SECURE_TO_INSECURE"
> > > to something like
"DISALLOW_REFERRER_ON_TRANSITION_FROM_SECURE_TO_INSECURE",
> > > with a comment that the referrer will be cleared in that case, though the
> > > delegate can optionally opt to block the request instead.
> > 
> > 
> > 
> > > 
> > > I also think "CanCorrectInvalidReferrerHeader" would be muck clearer as
> > > "ShouldBlockRequestWithSecureReferrerToInsecure" or somesuch.
> > 
> > 
> > We might want to add different policies in the future, like
origin-on-transition
> > or something. You could then create an URLRequest with a non-origin referrer
> > that would then also get handled here.
> 
> I still think that "CanCorrectInvalidReferrerHeader" is a very misleading
name.  Just looking at it, I'd assume that false would mean keep the invalid
header, or clear it, but that's not the case.  It's also not clear that
"invalid" doesn't mean !GURL(referrer_).is_valid().

[jochen (gone - plz use gerrit), 2014-09-16 17:08:27]

btw, which name would you prefer for the NetworkDelegate method?

CancelURLRequestWithPolicyViolatingReferrerHeader() maybe?

[mmenke, 2014-09-16 17:16:49]

On 2014/09/16 17:08:27, jochen wrote:
> btw, which name would you prefer for the NetworkDelegate method?
> 
> CancelURLRequestWithPolicyViolatingReferrerHeader() maybe?

That SGTM.  With that change the CL LGTM.

[jochen (gone - plz use gerrit), 2014-09-16 17:28:56]

The CQ bit was checked by jochen@chromium.org

[commit-bot: I haz the power, 2014-09-16 17:30:05]

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

[commit-bot: I haz the power, 2014-09-16 18:32:24]

Committed patchset #3 (id:40001) as 729bff37c9911da3a8722b0822b001a1df175928

[commit-bot: I haz the power, 2014-09-16 18:36:01]

Patchset 3 (id:??) landed as
https://crrev.com/0e3b3a6c53b408ea11cdab0a46d12ea905782821
Cr-Commit-Position: refs/heads/master@{#295107}