Apply meta tag referrer policy for preloaded requests

Apply meta tag referrer policy for preloaded requests

When a referrer policy is discovered in a meta tag during a preload
scan, apply that policy to the document so that it will be used for
subsequent preloaded resource loads.

BUG=508310
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=198992

[estark, 2015-07-14 04:45:34]

estark@chromium.org changed reviewers:
+ jochen@chromium.org

[estark, 2015-07-14 04:45:35]

jochen, could you please take a look? Thanks!

https://codereview.chromium.org/1235563004/diff/40001/LayoutTests/http/tests/...
File
LayoutTests/http/tests/security/resources/referrer-policy-conflicting-policies.html
(right):

https://codereview.chromium.org/1235563004/diff/40001/LayoutTests/http/tests/...
LayoutTests/http/tests/security/resources/referrer-policy-conflicting-policies.html:18:
<script>
I think (?) this change preserves what the test is trying to test (setting a
document policy after a request is created). But something seems a little
funky... after making this change, I looked at the original commit that
introduced this test
(https://chromium.googlesource.com/chromium/blink/+/b500b3f99d86bd5ff36764ca83...)
and tried to verify that the test still tests what it's supposed to test by
undoing the FrameFetchContext change in b500b3f99d. That is, I changed
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
to use m_document->referrerPolicy() instead of request.referrerPolicy() and was
hoping to see the assertion fail on this test... but it didn't fail, either
before or after my change here. So I'm not sure the test was testing what it was
supposed to test. I wonder if we should replace it with a unit test for
FrameFetchContext::addAdditionalRequestHeaders()?

[jochen (gone - plz use gerrit), 2015-07-14 14:29:36]

what about adding an String m_referrerPolicy to CachedDocumentParameters, then
you don't need to pass the String* around all the time.

I'd then also add an String m_referrerPolicy to PreloadRequest and use that to
generate a referrer in PreloadRequest::resourceRequest to set the referrer on
the returned request.

Probably requires factoring out a method somewhere that parses a string to a
referrer policy if we don't have that already?

that avoids setting the referrer policy on the document (which might be wrong,
after all, it's just the preload scanner that discovered the policy)

[estark, 2015-07-14 22:24:30]

On 2015/07/14 14:29:36, jochen wrote:
> what about adding an String m_referrerPolicy to CachedDocumentParameters, then
> you don't need to pass the String* around all the time.
> 
> I'd then also add an String m_referrerPolicy to PreloadRequest and use that to
> generate a referrer in PreloadRequest::resourceRequest to set the referrer on
> the returned request.
> 
> Probably requires factoring out a method somewhere that parses a string to a
> referrer policy if we don't have that already?
> 
> that avoids setting the referrer policy on the document (which might be wrong,
> after all, it's just the preload scanner that discovered the policy)

Ah, yes, that makes sense; PTAL at patch set 8.

[Yoav Weiss, 2015-07-15 08:14:47]

yoav@yoav.ws changed reviewers:
+ yoav@yoav.ws

[Yoav Weiss, 2015-07-15 08:14:47]

All in all, the preloadScanner parts look good.

https://codereview.chromium.org/1235563004/diff/140001/Source/core/html/parse...
File Source/core/html/parser/HTMLPreloadScanner.cpp (right):

https://codereview.chromium.org/1235563004/diff/140001/Source/core/html/parse...
Source/core/html/parser/HTMLPreloadScanner.cpp:564:
m_cssScanner.setReferrerPolicy(m_documentParameters->referrerPolicy);
Could you split that part into a helper function? e.g. "handleReferrer"

Optional: We could also DRY the code even more by merging "referrer" and
"viewport" logic.

[jochen (gone - plz use gerrit), 2015-07-15 13:43:54]

lgtm from my side, thx!

https://codereview.chromium.org/1235563004/diff/140001/Source/core/html/parse...
File Source/core/html/parser/PreloadRequest.h (right):

https://codereview.chromium.org/1235563004/diff/140001/Source/core/html/parse...
Source/core/html/parser/PreloadRequest.h:59: RequestType requestType, const
ReferrerPolicy& referrerPolicy)
you can pass ReferrerPolicy by value, it's just an int

[estark, 2015-07-15 21:58:56]

https://codereview.chromium.org/1235563004/diff/140001/Source/core/html/parse...
File Source/core/html/parser/HTMLPreloadScanner.cpp (right):

https://codereview.chromium.org/1235563004/diff/140001/Source/core/html/parse...
Source/core/html/parser/HTMLPreloadScanner.cpp:564:
m_cssScanner.setReferrerPolicy(m_documentParameters->referrerPolicy);
On 2015/07/15 08:14:47, Yoav Weiss wrote:
> Could you split that part into a helper function? e.g. "handleReferrer"
> 
> Optional: We could also DRY the code even more by merging "referrer" and
> "viewport" logic.

Done.

[Yoav Weiss, 2015-07-15 22:19:45]

Thanks! :) LGTM from my end as well % a minor style nit.

https://codereview.chromium.org/1235563004/diff/180001/Source/core/html/parse...
File Source/core/html/parser/PreloadRequest.h (right):

https://codereview.chromium.org/1235563004/diff/180001/Source/core/html/parse...
Source/core/html/parser/PreloadRequest.h:59: RequestType requestType, const
ReferrerPolicy referrerPolicy)
style nit: having these two in the same line feels out-of-style with the rest of
the parameters.

[estark, 2015-07-15 22:31:40]

Thanks Jochen and Yoav.

https://codereview.chromium.org/1235563004/diff/180001/Source/core/html/parse...
File Source/core/html/parser/PreloadRequest.h (right):

https://codereview.chromium.org/1235563004/diff/180001/Source/core/html/parse...
Source/core/html/parser/PreloadRequest.h:59: RequestType requestType, const
ReferrerPolicy referrerPolicy)
On 2015/07/15 22:19:45, Yoav Weiss wrote:
> style nit: having these two in the same line feels out-of-style with the rest
of
> the parameters.

Done.

[estark, 2015-07-15 22:31:53]

The CQ bit was checked by estark@chromium.org

[estark, 2015-07-15 22:31:53]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
yoav@yoav.ws
Link to the patchset: https://codereview.chromium.org/1235563004/#ps200001
(title: "style fix")

[commit-bot: I haz the power, 2015-07-15 22:32:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1235563004/200001

[commit-bot: I haz the power, 2015-07-16 00:19:18]

Committed patchset #11 (id:200001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=198992

[patrick.toomey, 2015-07-16 00:40:30]

patrick.toomey@github.com changed reviewers:
+ patrick.toomey@github.com

[patrick.toomey, 2015-07-16 00:40:31]

This test seems to verify that a subresource after the meta tag does not include
the referrer. But, I don't see a test that verifies that any resource that is
sourced before the meta tag does include referrer. I haven't looked through the
spec, but both Firefox and Safari seem to consider the meta tag policy
positionally (i.e. it is only active for resources that are defined after the
meta tag sets the policy).

[jochen (gone - plz use gerrit), 2015-07-16 05:20:03]

the spec doesn't really talk about pre-fetching.

safari doesn't do referrer handling during pre-fetching, since it gets the
header right, I guess it just doesn't prefetch at all.

firefox discards prefetched resources that were fetched with the wrong header
afaik. I don't think that's a reasonable approach, because if you were worried
about not leaking the referrer, it leaked already anyways, so why not just use
the cached entry. Also, just use CSP to set the referrer.

anyways, adding more tests seems reasonable.