Bug 72674

Created attachment 115702 [details] Patch

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

Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.

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

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 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 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

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.

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

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.

@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

Comment on attachment 115943 [details] Patch WebKit API changes LGTM

Comment on attachment 115943 [details] Patch Clearing flags on attachment: 115943 Committed r100895: <http://trac.webkit.org/changeset/100895>

All reviewed patches have been landed. Closing bug.

<rdar://problem/10994615>

(In reply to comment #21) > <rdar://problem/10994615> AFAIK <rdar://problem/10565234> also refers to this