Bug 72674 – Implement Meta referrer

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

Implement Meta referrer

Created attachment 115702 [details]
Patch

This patch implements http://wiki.whatwg.org/wiki/Meta_referrer

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Adam Barth]

*** Bug 70826 has been marked as a duplicate of this bug. ***

[Adam Barth]

Comment on attachment 115702 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=115702&action=review

> Source/WebCore/loader/PingLoader.cpp:96
> +        String referrer;
> +        if (!SecurityPolicy::shouldHideReferrer(pingURL, frame->loader()->outgoingReferrer(), frame->document()->referrerPolicy(), &referrer))
> +            request.setHTTPReferrer(referrer);

Bad indent.

> Source/WebCore/page/SecurityPolicy.cpp:70
> +    default:
> +        ASSERT_NOT_REACHED();

We usually we leave off the default case so the compiler will complain when we forget a case.

> Source/WebKit/chromium/public/WebSecurityPolicy.h:48
> +    enum WebReferrerPolicy {
> +        ReferrerPolicyAlways,
> +        ReferrerPolicyDefault,
> +        ReferrerPolicyNever,
> +        ReferrerPolicyOrigin,
> +    };

Usually we have some compile asserts about matching enum values.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:596
> +    if (m_frame->document())

m_frame->document() can never be null.

> Source/WebKit/chromium/src/WebSecurityPolicy.cpp:99
> +    String tmpReferrer;
> +    bool hideReferrer = SecurityPolicy::shouldHideReferrer(url, referrer, static_cast<SecurityPolicy::ReferrerPolicy>(referrerPolicy), &tmpReferrer);
> +    if (!hideReferrer && referrerToUse)
> +        *referrerToUse = tmpReferrer;
> +    return hideReferrer;

This is slightly inelegant, but I don't see a better way.

> LayoutTests/ChangeLog:4
> +        Implement Meta referrer
> +        https://bugs.webkit.org/show_bug.cgi?id=72674

Should we test the interaction with rel=noreferrer?

> LayoutTests/http/tests/security/resources/referrer-policy-log.html:9
> +    if (document.referrer == "")

Should we check the HTTP header too?  If you make this a PHP script, that's pretty easy to do.

[Adam Barth]

Comment on attachment 115702 [details]
Patch

These tests are a good start, but we should cover the HTTP header and the interaction with rel=noreferrer.

[Nate Chapin]

Comment on attachment 115702 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=115702&action=review

> Source/WebCore/loader/FrameLoader.cpp:1145
> +    if (SecurityPolicy::shouldHideReferrer(url, argsReferrer.isEmpty() ? m_outgoingReferrer : argsReferrer, m_frame->document()->referrerPolicy(), &referrer) || shouldSendReferrer == NeverSendReferrer)
>          referrer = String();

Perhaps consider m_outgoingReferrer when initializing argsReferrer, instead of here?

> Source/WebCore/loader/PingLoader.cpp:97
> -    if (!SecurityPolicy::shouldHideReferrer(pingURL, frame->loader()->outgoingReferrer())) {
> +    if (!SecurityPolicy::shouldHideReferrer(pingURL, frame->loader()->outgoingReferrer(), SecurityPolicy::ReferrerPolicyDefault, 0)) {
>        request.setHTTPHeaderField("Ping-From", frame->document()->url());
> -      if (!sourceOrigin->isSameSchemeHostPort(pingOrigin.get()))
> -        request.setHTTPReferrer(frame->loader()->outgoingReferrer());
> +      if (!sourceOrigin->isSameSchemeHostPort(pingOrigin.get())) {
> +        String referrer;
> +        if (!SecurityPolicy::shouldHideReferrer(pingURL, frame->loader()->outgoingReferrer(), frame->document()->referrerPolicy(), &referrer))
> +            request.setHTTPReferrer(referrer);
> +      }

Can we combine these shouldHideReferrer() calls?

> Source/WebCore/page/SecurityPolicy.h:52
> +
> +    // True if the referrer header should be ommitted according to the policy.
> +    // Otherwise, if referrerToUse is non-0, the value to send as referrer is
> +    // returned.
> +    static bool shouldHideReferrer(const KURL&, const String& referrer, ReferrerPolicy, String* referrerToUse);
>  

Could we rename this and just return a String instead of having the String* input parameter?

(In reply to comment #7)
> (From update of attachment 115702 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=115702&action=review
> 
> > Source/WebCore/loader/FrameLoader.cpp:1145
> > +    if (SecurityPolicy::shouldHideReferrer(url, argsReferrer.isEmpty() ? m_outgoingReferrer : argsReferrer, m_frame->document()->referrerPolicy(), &referrer) || shouldSendReferrer == NeverSendReferrer)
> >          referrer = String();
> 
> Perhaps consider m_outgoingReferrer when initializing argsReferrer, instead of here?
> 
> > Source/WebCore/loader/PingLoader.cpp:97
> > -    if (!SecurityPolicy::shouldHideReferrer(pingURL, frame->loader()->outgoingReferrer())) {
> > +    if (!SecurityPolicy::shouldHideReferrer(pingURL, frame->loader()->outgoingReferrer(), SecurityPolicy::ReferrerPolicyDefault, 0)) {
> >        request.setHTTPHeaderField("Ping-From", frame->document()->url());
> > -      if (!sourceOrigin->isSameSchemeHostPort(pingOrigin.get()))
> > -        request.setHTTPReferrer(frame->loader()->outgoingReferrer());
> > +      if (!sourceOrigin->isSameSchemeHostPort(pingOrigin.get())) {
> > +        String referrer;
> > +        if (!SecurityPolicy::shouldHideReferrer(pingURL, frame->loader()->outgoingReferrer(), frame->document()->referrerPolicy(), &referrer))
> > +            request.setHTTPReferrer(referrer);
> > +      }
> 
> Can we combine these shouldHideReferrer() calls?

The first shouldHideReferrer call is basically abusing this. It's not really about referrers but mainly checks whether it's a transition from encrypted to unencrypted.

The second one checks and applies the referrer policy.

> 
> > Source/WebCore/page/SecurityPolicy.h:52
> > +
> > +    // True if the referrer header should be ommitted according to the policy.
> > +    // Otherwise, if referrerToUse is non-0, the value to send as referrer is
> > +    // returned.
> > +    static bool shouldHideReferrer(const KURL&, const String& referrer, ReferrerPolicy, String* referrerToUse);
> >  
> 
> Could we rename this and just return a String instead of having the String* input parameter?

I tried that, but I didn't find a nice name, and it looked ugly overall

[Alexey Proskuryakov]

Comment on attachment 115702 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=115702&action=review

Looks like a good thing to support. Please find a better name or signature for shouldHideReferrer - this function is not a "should" type any more, since it generates the header. Perhaps it could just return the string, and empty result would signify that the header should not be sent.

> Source/WebCore/page/SecurityPolicy.h:45
> +        ReferrerPolicyOrigin,

This last value is not clear at all, and needs at least a comment.

(In reply to comment #9)
> (From update of attachment 115702 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=115702&action=review
> 
> Looks like a good thing to support. Please find a better name or signature for shouldHideReferrer - this function is not a "should" type any more, since it generates the header. Perhaps it could just return the string, and empty result would signify that the header should not be sent.

what about String generateReferrerHeader(policy, url, referrer)?



> 
> > Source/WebCore/page/SecurityPolicy.h:45
> > +        ReferrerPolicyOrigin,
> 
> This last value is not clear at all, and needs at least a comment.

[Darin Fisher (:fishd, Google)]

Comment on attachment 115702 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=115702&action=review

> Source/WebKit/chromium/public/WebFrame.h:146
>  

nit: preserve two blank lines at end of section.

> Source/WebKit/chromium/public/WebSecurityPolicy.h:43
> +    enum WebReferrerPolicy {

nit: Web prefix is reserved for top-level enums.  Plus, enum values should be named starting with the type name.

enum Foo { FooBar, FooBaz };

Given that you are having to #include WebSecurityPolicy.h into WebFrame.h, perhaps WebReferrerPolicy should just
be a top-level enum (in its own header file)?

Created attachment 115893 [details]
Patch

Adressed all comments and added two new layout tests for rel=noreferrer and redirects

[Adam Barth]

Comment on attachment 115893 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=115893&action=review

LGTM

> Source/WebCore/page/SecurityPolicy.cpp:81
> +        return origin + "/";

I'd add a comment explaining why we're appending "/" here.

[Adam Barth]

@fishd: Would you be willing to review the WebKit API changes?

Created attachment 115943 [details]
Patch

(In reply to comment #14)
> (From update of attachment 115893 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=115893&action=review
> 
> LGTM
> 
> > Source/WebCore/page/SecurityPolicy.cpp:81
> > +        return origin + "/";
> 
> I'd add a comment explaining why we're appending "/" here.

Done

For the record, I'm adding the slash since a security origin is not a canonical URL as it lacks a path, so in order to use it as a referrer header, I'm adding /

I'm also omitting the referrer header if the policy is "origin" and the origin is "null"

And I've decided to always drop the referrer if you navigate using a link with rel=noreferrer

I've added comments to http://wiki.whatwg.org/wiki/Meta_referrer documenting this

[Darin Fisher (:fishd, Google)]

Comment on attachment 115943 [details]
Patch

WebKit API changes LGTM

[WebKit Review Bot]

Comment on attachment 115943 [details]
Patch

Clearing flags on attachment: 115943

Committed r100895: <http://trac.webkit.org/changeset/100895>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/10994615>

(In reply to comment #21)
> <rdar://problem/10994615>

AFAIK <rdar://problem/10565234> also refers to this