1303682 - Add deprecation warning before removing 'referrer' directive from CSP

Status: RESOLVED FIXED

(Core :: DOM: Security, defect, P3)

Description

[Christoph Kerschbaumer [:ckerschb]]

Please see:
https://bugzilla.mozilla.org/show_bug.cgi?id=1302449#c1

Comment 1

[Samriddhi]

Attachment: Added deprecation warning for referrer-directive

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

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

Comment 3

[Samriddhi]

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

Comment 4

[Samriddhi]

Attachment: Change in file nsCSPParse.cpp and csp.properties

Comment 5

[Christoph Kerschbaumer [: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.

Comment 6

[Samriddhi]

Attachment: Updated warning message

Comment 7

[Christoph Kerschbaumer [: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?

Comment 8

[Samriddhi]

Attachment: Updated reviews

Comment 9

[Samriddhi]

(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 10

[Christoph Kerschbaumer [:ckerschb]]

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!

Comment 11

[Samriddhi]

Attachment: Updated commit message

Comment 12

[Christoph Kerschbaumer [: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 :-)

Comment 13

[Samriddhi]

(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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/64465dd73b97
Add deprecation warning before removing 'referrer' directive from CSP. r=ckerschb

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/64465dd73b97

Comment 16

[Kohei Yoshino [:kohei]]

Posted the site compatibility doc: https://www.fxsitecompat.com/en-CA/docs/2017/csp-referrer-directive-has-been-deprecated/