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:

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

Mike West

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

Attachments
Patch (15.00 KB, patch) 2014-10-11 05:00 PDT, Mike West	no flags	Details Formatted Diff Diff
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2 (581.00 KB, application/zip) 2014-10-11 06:04 PDT, Build Bot	no flags	Details
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion (601.88 KB, application/zip) 2014-10-11 06:28 PDT, Build Bot	no flags	Details
Patch (13.87 KB, patch) 2014-10-11 10:18 PDT, Mike West	no flags	Details Formatted Diff Diff
Patch (14.41 KB, patch) 2014-10-12 04:41 PDT, Mike West	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Mike West

Comment 1 2014-10-11 05:00:44 PDT

Created attachment 239671 [details] Patch

Mike West

Comment 2 2014-10-11 05:01:10 PDT

ap@: Mind taking a look at this patch?

Build Bot

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

Build Bot

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

Build Bot

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

Build Bot

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

Mike West

Comment 7 2014-10-11 10:18:26 PDT

Created attachment 239679 [details] Patch

jochen

Comment 8 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 :)

Alexey Proskuryakov

Comment 9 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.

Mike West

Comment 10 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).

Mike West

Comment 11 2014-10-12 04:41:14 PDT

Created attachment 239702 [details] Patch

Mike West

Comment 12 2014-10-12 04:44:44 PDT

Comment on attachment 239702 [details] Patch Carrying over ap's r+.

WebKit Commit Bot

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

Mike West

Comment 14 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.

WebKit Commit Bot

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

WebKit Commit Bot

Comment 16 2014-10-12 05:55:53 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.