Bug 72674 - Implement Meta referrer
Summary: Implement Meta referrer
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: jochen
URL:
Keywords: InRadar
: 70826 (view as bug list)
Depends on: 73913
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-17 15:45 PST by jochen
Modified: 2012-03-06 11:40 PST (History)
7 users (show)

See Also:


Attachments
Patch (43.22 KB, patch)
2011-11-17 15:46 PST, jochen
no flags Details | Formatted Diff | Diff
Patch (52.21 KB, patch)
2011-11-18 15:33 PST, jochen
no flags Details | Formatted Diff | Diff
Patch (52.35 KB, patch)
2011-11-19 02:27 PST, jochen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description jochen 2011-11-17 15:45:06 PST
Implement Meta referrer
Comment 1 jochen 2011-11-17 15:46:06 PST
Created attachment 115702 [details]
Patch
Comment 2 jochen 2011-11-17 15:47:14 PST
This patch implements http://wiki.whatwg.org/wiki/Meta_referrer
Comment 3 WebKit Review Bot 2011-11-17 15:48:25 PST
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment 4 Adam Barth 2011-11-17 15:52:28 PST
*** Bug 70826 has been marked as a duplicate of this bug. ***
Comment 5 Adam Barth 2011-11-17 16:03:43 PST
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.
Comment 6 Adam Barth 2011-11-17 16:04:27 PST
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.
Comment 7 Nate Chapin 2011-11-17 16:08:56 PST
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?
Comment 8 jochen 2011-11-17 16:12:59 PST
(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
Comment 9 Alexey Proskuryakov 2011-11-17 16:13:51 PST
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.
Comment 10 jochen 2011-11-17 16:23:35 PST
(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.
Comment 11 Darin Fisher (:fishd, Google) 2011-11-17 19:34:08 PST
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)?
Comment 12 jochen 2011-11-18 15:33:53 PST
Created attachment 115893 [details]
Patch
Comment 13 jochen 2011-11-18 15:35:27 PST
Adressed all comments and added two new layout tests for rel=noreferrer and redirects
Comment 14 Adam Barth 2011-11-18 19:04:34 PST
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.
Comment 15 Adam Barth 2011-11-18 19:05:02 PST
@fishd: Would you be willing to review the WebKit API changes?
Comment 16 jochen 2011-11-19 02:27:39 PST
Created attachment 115943 [details]
Patch
Comment 17 jochen 2011-11-19 02:30:33 PST
(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
Comment 18 Darin Fisher (:fishd, Google) 2011-11-20 07:25:31 PST
Comment on attachment 115943 [details]
Patch

WebKit API changes LGTM
Comment 19 WebKit Review Bot 2011-11-21 02:30:13 PST
Comment on attachment 115943 [details]
Patch

Clearing flags on attachment: 115943

Committed r100895: <http://trac.webkit.org/changeset/100895>
Comment 20 WebKit Review Bot 2011-11-21 02:30:20 PST
All reviewed patches have been landed.  Closing bug.
Comment 21 Radar WebKit Bug Importer 2012-03-06 10:41:34 PST
<rdar://problem/10994615>
Comment 22 jochen 2012-03-06 11:40:17 PST
(In reply to comment #21)
> <rdar://problem/10994615>

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