Add deprecation warning before removing 'referrer' directive from CSP

RESOLVED FIXED in Firefox 52

Status

()

defect
P3
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: ckerschb, Assigned: j.samriddhi13)

Tracking

({dev-doc-needed, site-compat})

unspecified
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

(Whiteboard: [domsecurity-backlog1])

Attachments

(1 attachment, 4 obsolete attachments)

Blocks: 1302449
Priority: -- → P3
Whiteboard: [domsecurity-backlog1]
Summary: Add deprecation warning and telemetry before removing 'referrer' directive from CSP → Add deprecation warning before removing 'referrer' directive from CSP
Assignee

Comment 1

3 years ago
Attachment #8795410 - Flags: review?(ckerschb)
Assignee: nobody → j.samriddhi13
Status: NEW → ASSIGNED
Comment on attachment 8795410 [details] [diff] [review]
Added deprecation warning for referrer-directive

Hey Samriddhy, you would have to generate a *.patch which I can review. Please have a look at 'Creating a Patch' [1] which provides some guidance on how to do that. You would have to do something like:
> hg export . > my-change.patch
and upload that patch to bugzilla which I can then review.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch
Attachment #8795410 - Flags: review?(ckerschb)
Assignee

Comment 3

3 years ago
Hey, sorry, I know I messed up things, was just working on the same, will fix that soon.
Assignee

Comment 4

3 years ago
Attachment #8795726 - Flags: review?(ckerschb)
Comment on attachment 8795726 [details] [diff] [review]
Change in file nsCSPParse.cpp and csp.properties

Review of attachment 8795726 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good otherwise, please update my nit and flag me again so we can get this landed. thanks!

::: dom/locales/en-US/chrome/security/csp.properties
@@ +83,1 @@
>  

Please make that match the other deprecation warning we already have:
> deprecatedDirective = Directive ‘%1$S’ has been deprecated. Please use directive ‘%2$S’ instead.

So Please update to:
deprecatedReferrerDirective = Referrer Directive ‘%1$S’ has been deprecated. Please use the Referrer-Policy header instead.
Attachment #8795726 - Flags: review?(ckerschb) → feedback+
Attachment #8795410 - Attachment is obsolete: true
Assignee

Comment 6

3 years ago
Posted patch Updated warning message (obsolete) — Splinter Review
Attachment #8795832 - Flags: review?(ckerschb)
Comment on attachment 8795832 [details] [diff] [review]
Updated warning message

Review of attachment 8795832 [details] [diff] [review]:
-----------------------------------------------------------------

It seems you have re-export the patch because csp.properties appears twice in the patch. Please fix and re-flag me for review!

::: dom/locales/en-US/chrome/security/csp.properties
@@ +77,4 @@
>  # LOCALIZATION NOTE (ignoringReportOnlyDirective):
>  # %1$S is the directive that is ignored in report-only mode.
>  ignoringReportOnlyDirective = Ignoring sandbox directive when delivered in a report-only policy ‘%1$S’
> +# LOCALIZATION NOTE (deprecatedReferrerDirective):

Looks good!

@@ +78,5 @@
>  # %1$S is the directive that is ignored in report-only mode.
>  ignoringReportOnlyDirective = Ignoring sandbox directive when delivered in a report-only policy ‘%1$S’
> +# LOCALIZATION NOTE (deprecatedReferrerDirective):
> +# %1$S is the value of the deprecated Referrer Directive.
> +deprecatedReferrerDirective = Support for Referrer Directive ‘%1$S’ is going to be removed soon.

Why is that part still here?
Attachment #8795832 - Flags: review?(ckerschb)
Assignee

Comment 8

3 years ago
Posted patch Updated reviews (obsolete) — Splinter Review
Attachment #8796150 - Flags: review?(ckerschb)
Assignee

Comment 9

3 years ago
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #7)
> Comment on attachment 8795832 [details] [diff] [review]
> Updated warning message
> 
> Review of attachment 8795832 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It seems you have re-export the patch because csp.properties appears twice
> in the patch. Please fix and re-flag me for review!
> 
> ::: dom/locales/en-US/chrome/security/csp.properties
> @@ +77,4 @@
> >  # LOCALIZATION NOTE (ignoringReportOnlyDirective):
> >  # %1$S is the directive that is ignored in report-only mode.
> >  ignoringReportOnlyDirective = Ignoring sandbox directive when delivered in a report-only policy ‘%1$S’
> > +# LOCALIZATION NOTE (deprecatedReferrerDirective):
> 
> Looks good!
> 
> @@ +78,5 @@
> >  # %1$S is the directive that is ignored in report-only mode.
> >  ignoringReportOnlyDirective = Ignoring sandbox directive when delivered in a report-only policy ‘%1$S’
> > +# LOCALIZATION NOTE (deprecatedReferrerDirective):
> > +# %1$S is the value of the deprecated Referrer Directive.
> > +deprecatedReferrerDirective = Support for Referrer Directive ‘%1$S’ is going to be removed soon.
> 
> Why is that part still here?

I combined the last two changesets to get the patch, that was the reason for redundant lines. Updated with new patch. Thanks!
Comment on attachment 8796150 [details] [diff] [review]
Updated reviews

Review of attachment 8796150 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, one thing I just realized, you haven't set a correct commit message. Something link:
> Bug 1303682: Add deprecation warning before removing 'referrer' directive from CSP. r=ckerschb

Please also mark older patches as 'obsolete' whenever you update a new version of a patch. Thanks!
Attachment #8796150 - Flags: review?(ckerschb)
Assignee

Comment 11

3 years ago
Attachment #8795726 - Attachment is obsolete: true
Attachment #8795832 - Attachment is obsolete: true
Attachment #8796150 - Attachment is obsolete: true
Attachment #8796172 - Flags: review?(ckerschb)
Comment on attachment 8796172 [details] [diff] [review]
Updated commit message

Review of attachment 8796172 [details] [diff] [review]:
-----------------------------------------------------------------

Awesome! Whenever you are ready you can now set 'checkin-needed' as a Keyword and someone from the sheriffs will check in the code for you. Congrats :-)
Attachment #8796172 - Flags: review?(ckerschb) → review+
Assignee

Updated

3 years ago
Keywords: checkin-needed
Assignee

Comment 13

3 years ago
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #12)
> Comment on attachment 8796172 [details] [diff] [review]
> Updated commit message
> 
> Review of attachment 8796172 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Awesome! Whenever you are ready you can now set 'checkin-needed' as a
> Keyword and someone from the sheriffs will check in the code for you.
> Congrats :-)

Added the keyword 'checkin-needed'. Thanks!

Comment 14

3 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/64465dd73b97
Add deprecation warning before removing 'referrer' directive from CSP. r=ckerschb
Keywords: checkin-needed

Comment 15

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/64465dd73b97
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.