Bug 137635

Summary: Referrer Policy: Update <meta name="referrer"> values to match the spec
Product: WebKit Reporter: Mike West <mkwst>
Component: WebCore Misc.Assignee: Mike West <mkwst>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, buildbot, commit-queue, esprehn+autocc, jochen, kangil.han, rniwa
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2
none
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion
none
Patch
none
Patch none

Description Mike West 2014-10-11 03:37:00 PDT
http://w3c.github.io/webappsec/specs/referrer-policy/#referrer-policy-delivery-meta defines different keywords than we originally implemented. We should support them.
Comment 1 Mike West 2014-10-11 05:00:44 PDT
Created attachment 239671 [details]
Patch
Comment 2 Mike West 2014-10-11 05:01:10 PDT
ap@: Mind taking a look at this patch?
Comment 3 Build Bot 2014-10-11 06:04:31 PDT
Comment on attachment 239671 [details]
Patch

Attachment 239671 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5084248897748992

New failing tests:
http/tests/security/referrer-on-client-redirect.html
Comment 4 Build Bot 2014-10-11 06:04:35 PDT
Created attachment 239673 [details]
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-13  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 5 Build Bot 2014-10-11 06:28:04 PDT
Comment on attachment 239671 [details]
Patch

Attachment 239671 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5384132104290304

New failing tests:
http/tests/security/referrer-on-client-redirect.html
Comment 6 Build Bot 2014-10-11 06:28:12 PDT
Created attachment 239675 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 7 Mike West 2014-10-11 10:18:26 PDT
Created attachment 239679 [details]
Patch
Comment 8 jochen 2014-10-11 10:27:25 PDT
Comment on attachment 239679 [details]
Patch

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

lg

Alexey, do you think adopting to the standard requires a PSA to webkit-dev?

Eventually, implementing origin-when-cross-origin will require plumbing the setting down to the network stack (similar to always and origin already do)

> LayoutTests/http/tests/security/referrer-policy-no-referrer-when-downgrade.html:1
> +<html>

<!DOCTYPE html> plz :)
Comment 9 Alexey Proskuryakov 2014-10-11 22:44:40 PDT
Comment on attachment 239679 [details]
Patch

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

Thank you!

> Alexey, do you think adopting to the standard requires a PSA to webkit-dev?

I don't know if it's formally required, however I think that it would be helpful to give people a heads up.

> Source/WebCore/ChangeLog:10
> +        defines different keywords than we originally implemented. We should
> +        support them.

...because this is the only version of the spec that is actively maintained?

Anyone can publish thousands of specs on github, which means that the existence of a spec alone has no impact on what browsers do. Knowing your history with referrer policy, I can figure out more, yet it's helpful to have a more complete rationale in ChangeLog.

> Source/WebCore/dom/Document.cpp:3048
> +    if (equalIgnoringCase(policy, "no-referrer") || equalIgnoringCase(policy, "never"))
> +        setReferrerPolicy(ReferrerPolicyNever);
> +    else if (equalIgnoringCase(policy, "unsafe-url") || equalIgnoringCase(policy, "always"))
> +        setReferrerPolicy(ReferrerPolicyAlways);

I think that a comment explaining that we support multiple spec versions would help here. Otherwise, it will be very difficult to figure out for someone looking at this code.
Comment 10 Mike West 2014-10-12 04:37:05 PDT
(In reply to comment #9)
> (From update of attachment 239679 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=239679&action=review
> 
> Thank you!

Thank _you_!

> > Alexey, do you think adopting to the standard requires a PSA to webkit-dev?
> 
> I don't know if it's formally required, however I think that it would be helpful to give people a heads up.

I'm not sure this CL in particular warrants an announcement, but I'm happy to point folks to the spec on webkit-dev in the hopes that someone wants to do lots of work to implement the bits that aren't yet in WebKit (or Blink, for that matter).

> > Source/WebCore/ChangeLog:10
> > +        defines different keywords than we originally implemented. We should
> > +        support them.
> 
> ...because this is the only version of the spec that is actively maintained?
> 
> Anyone can publish thousands of specs on github, which means that the existence of a spec alone has no impact on what browsers do. Knowing your history with referrer policy, I can figure out more, yet it's helpful to have a more complete rationale in ChangeLog.

I've added links to the working draft published by the W3C, as an appeal to authority if nothing else. The main reasons that the working group decided on new names were related to clarity; "always" (arguably) didn't make it clear that the full URL was always sent, and "default" was seen as too historically-based. The new names are meant to convey in themselves what practical impact the policy has.

> I think that a comment explaining that we support multiple spec versions would help here. Otherwise, it will be very difficult to figure out for someone looking at this code.

The spec actually defines all these keywords, it simply labels some of them as legacy. We won't be able to get rid of them easily, given that we've been shipping them for years, but we can guide folks towards the new names (which, incidentally, are the only names supported in CSP level 2).
Comment 11 Mike West 2014-10-12 04:41:14 PDT
Created attachment 239702 [details]
Patch
Comment 12 Mike West 2014-10-12 04:44:44 PDT
Comment on attachment 239702 [details]
Patch

Carrying over ap's r+.
Comment 13 WebKit Commit Bot 2014-10-12 04:45:51 PDT
Comment on attachment 239702 [details]
Patch

Rejecting attachment 239702 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 239702, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.

Full output: http://webkit-queues.appspot.com/results/4551953602314240
Comment 14 Mike West 2014-10-12 05:26:04 PDT
(In reply to comment #13)
> (From update of attachment 239702 [details])
> Rejecting attachment 239702 [details] from commit-queue.
> 
> Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 239702, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit
> 
> ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.
> 
> Full output: http://webkit-queues.appspot.com/results/4551953602314240

Ah, right, I was supposed to update the ChangeLog entry. It's been a while. :(

Thanks for the r+/cq+, Jochen.
Comment 15 WebKit Commit Bot 2014-10-12 05:55:48 PDT
Comment on attachment 239702 [details]
Patch

Clearing flags on attachment: 239702

Committed r174640: <http://trac.webkit.org/changeset/174640>
Comment 16 WebKit Commit Bot 2014-10-12 05:55:53 PDT
All reviewed patches have been landed.  Closing bug.