Closed Bug 1163743 Opened 10 years ago Closed 10 years ago

(referrer policy) origin-when-crossorigin should have a hyphen in cross-origin

Categories

(Core :: DOM: Security, defect)

Product:
Core
Component:
DOM: Security
Version:
37 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla41
Tracking Flags:
Tracking Status
firefox41 --- fixed

People

(Reporter: geekboy, Assigned: franziskus)

References

Details

(Keywords: dev-doc-complete, site-compat)

Attachments

(2 files, 4 obsolete files)

replacing origin-when-crossorigin with origin-when-cross-origin
3.67 KB, patch
mcmanus
: review+
geekboy
: feedback+
Details | Diff | Splinter Review
replacing origin-when-crossorigin with origin-when-cross-origin in tests
29.19 KB, patch
geekboy
: review+
Details | Diff | Splinter Review
There was apparently a typo in the referrer policy specification where origin-when-crossorigin should actually be origin-when-cross-origin. See this thread: https://lists.w3.org/Archives/Public/public-webappsec/2015May/0058.html The fix should be easy, just a string replace in some tests and in ReferrerPolicy.h
Assignee: nobody → franziskuskiefer
Flags: needinfo?(mozilla)
Before we remove support for origin-when-crossorigin, we should consider supporting it for legacy (alias origin-when-crossorigin to its hyphen-containing counterpart). This is because for a little while there will be browsers that only understand the version without the extra hyphen and sites can't use both very easily. Should be an easy tweak to the impl patch.
Attachment #8604297 - Attachment is obsolete: true
Flags: needinfo?(mozilla)
Blocks: 1161221
Comment on attachment 8604369 [details] [diff] [review] replacing origin-when-crossorigin with origin-when-cross-origin Review of attachment 8604369 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, once completely ready, we should have mcmanus review those bits.
Attachment #8604369 - Flags: feedback+
CC'ing Sid an removing the needinfo flag.
Flags: needinfo?(mozilla)
Comment on attachment 8604371 [details] [diff] [review] replacing origin-when-crossorigin with origin-when-cross-origin in tests Review of attachment 8604371 [details] [diff] [review]: ----------------------------------------------------------------- Can we factor out the common bits of test_bug704320 and test_bug1163743 so we don't end up having so much duplicate code? ::: dom/base/test/test_bug1163743.html @@ +6,5 @@ > +--> > + > +<head> > + <meta charset="utf-8"> > + <title>Test policies for Bug 704320</title> Please update the bugnumber here.
Attachment #8604371 - Flags: feedback+
Attachment #8605281 - Flags: review?(sstamm)
Comment on attachment 8605282 [details] [diff] [review] replacing origin-when-crossorigin with origin-when-cross-origin in tests Review of attachment 8605282 [details] [diff] [review]: ----------------------------------------------------------------- referrerHelper.js is tailored to one particular bug, please either make it general purpose, leave it in test_bug704320_policyset.html, or name it something like file_bug704320.js to make its purpose clear. r=me with a green try run (you'll have to test it in combination with the other patch, of course) and assuming you address my referrerHelper.js comments. ::: dom/base/test/test_bug704320_policyset.html @@ +8,5 @@ > <head> > <meta charset="utf-8"> > <title>Test policies for Bug 704320</title> > <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script> > + <script type="application/javascript" src="referrerHelper.js"></script> This is the only file that references this script; why did you lift it out? Do you anticipate using it again in the future?
Attachment #8605282 - Flags: review?(sstamm) → review+
Comment on attachment 8605281 [details] [diff] [review] replacing origin-when-crossorigin with origin-when-cross-origin Review of attachment 8605281 [details] [diff] [review]: ----------------------------------------------------------------- Looks good from a referrer policy standpoint. I'm not a necko peer, though, so over to mcmanus for an official review. mcmanus: there was a bug in the spec, they missed a hyphen in the "origin-when-cross-origin" token. We implemented the spec as written, now our impl needs a tweak and we want to support existing users of the old less-hyphenated "origin-when-crossorigin" token.
Attachment #8605281 - Flags: review?(sstamm)
Attachment #8605281 - Flags: review?(mcmanus)
Attachment #8605281 - Flags: feedback+
(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #10) > https://treeherder.mozilla.org/#/jobs?repo=try&revision=dbd0d26968f4 And the try run appears green, so that's cool.
(In reply to Sid Stamm [:geekboy or :sstamm] from comment #13) > Comment on attachment 8605282 [details] [diff] [review] > replacing origin-when-crossorigin with origin-when-cross-origin in tests > > Review of attachment 8605282 [details] [diff] [review]: > ----------------------------------------------------------------- > > referrerHelper.js is tailored to one particular bug, please either make it > general purpose, leave it in test_bug704320_policyset.html, or name it > something like file_bug704320.js to make its purpose clear. > > r=me with a green try run (you'll have to test it in combination with the > other patch, of course) and assuming you address my referrerHelper.js > comments. > > ::: dom/base/test/test_bug704320_policyset.html > @@ +8,5 @@ > > <head> > > <meta charset="utf-8"> > > <title>Test policies for Bug 704320</title> > > <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script> > > + <script type="application/javascript" src="referrerHelper.js"></script> > > This is the only file that references this script; why did you lift it out? > Do you anticipate using it again in the future? referrerHelper.js is used in test_bug1163743.html as well, which is a single test for origin-when-crossorigin. That's why a bug specific name didn't make sense to me. None of these tests will stay like this anyway as I rewrote them for bug 1161221 to enable them on all platforms.
Status: NEW → ASSIGNED
Flags: needinfo?(sstamm)
Ah, I see, makes sense. Great.
Flags: needinfo?(sstamm)
Attachment #8605281 - Flags: review?(mcmanus) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e8f2b5a25918 https://hg.mozilla.org/mozilla-central/rev/591e058a89b9
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
The main CSP article was updated already; I have added this information to https://developer.mozilla.org/en-US/Firefox/Releases/41.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: