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
Mike West
2014-10-11 03:37:00 PDT
Created attachment 239671 [details]
Patch
ap@: Mind taking a look at this patch? 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 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 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 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
Created attachment 239679 [details]
Patch
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 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. (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). Created attachment 239702 [details]
Patch
Comment on attachment 239702 [details]
Patch
Carrying over ap's r+.
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 (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 on attachment 239702 [details] Patch Clearing flags on attachment: 239702 Committed r174640: <http://trac.webkit.org/changeset/174640> All reviewed patches have been landed. Closing bug. |