I am working on this bug as a part of Outreachy application. I have some queries : 1- Which build system to configure Firefox to? I have the build system for Android(C++) set up. 2- What does the boolean value mReferrerPolicySet in nsIDocument.h[1] represent ? 3- The bug asks to log a warning for deprecating CSP referrer directive. Is CSPPARSERLOG("message");[2] the right way to log warnings ? 4- Please guide me on the approach : a- Adding a metric CSP_REFERRER_POLICY in Histogram.json[3] b- Using boolean mReferrerPolicySet[4] to increment the Telemetry by 1 in nsDocument.cpp[5] c- Logging a warning in nsCSPParser.cpp[6] [1] https://dxr.mozilla.org/mozilla-central/source/dom/base/nsIDocument.h#2861 [2] https://dxr.mozilla.org/mozilla-central/source/dom/security/nsCSPParser.cpp#862 [3] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/Histograms.json#3806 [4] https://dxr.mozilla.org/mozilla-central/source/dom/base/nsIDocument.h#2861 [5] https://dxr.mozilla.org/mozilla-central/source/dom/base/nsDocument.cpp#1450 [6] https://dxr.mozilla.org/mozilla-central/source/dom/security/nsCSPParser.cpp#867

(Please needinfo fbraun@mozilla.com, not the other Frederik Braun :-)) Most of the questions are well-explained on articles in MDN or our wiki, so let me respond with a lot of links :-) 1) https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Source_Code and https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch 2) Looking at dxr.mozilla.org (like you did!), it seems this is an internal state variable to figure out of the document has a referrer policy 3) No, CSPPARSERLOG is initialized here https://dxr.mozilla.org/mozilla-central/source/dom/security/nsCSPParser.cpp#25 and used for internal/debug logging, that is disabled during release. See also NSPR-Logging (now called MOZLOG I think). Logging to the developer console happens in other code, e.g., here https://dxr.mozilla.org/mozilla-central/source/dom/security/nsCSPParser.cpp#258 4) Sorry, I have not worked with telemetry yet. I trust you can figure this out for yourself. If not, I want to encourage you learning to ask the right people and the right questions on IRC chat (https://wiki.mozilla.org/IRC) ;-) - I would be even more excited if you managed to write a very short summary if there isn't any kind of documentation to add gather new telemetry. The folks in our documentation team would instantly like you (and possibly turn this into a great page on MDN) :-)

Seems like Freddy already replied!

Comment on attachment 8796918 [details] [diff] [review] patch.diff Review of attachment 8796918 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/nsDocument.cpp @@ +1455,5 @@ > if (mHasUnsafeEvalCSP) { > Accumulate(Telemetry::CSP_UNSAFE_EVAL_DOCUMENTS_COUNT, 1); > } > + if (mReferrerPolicySet) { > + Accumulate(Telemetry::CSP_REFERRER_POLICY, 1); mReferrerPoliySet is also used if the referrer is set by the actual header, not only by CSP. I think you have to add an additional flag, similar to mHasUnafeEvalCSP. ::: dom/security/nsCSPParser.cpp @@ +867,5 @@ > > // the referrer policy is valid, so go ahead and use it. > + const char16_t* params[] = { mCurToken.get() }; > + logWarningErrorToConsole(nsIScriptError::warningFlag, "ValidReferrerPolicy", > + params, ArrayLength(params)); Bug 1303682 already added a deprecation warning - please remove!

Comment on attachment 8797741 [details] [diff] [review] fix.diff Review of attachment 8797741 [details] [diff] [review]: ----------------------------------------------------------------- Almost, changes within nsDocument.cpp, nsIDocument.h and Histograms.json look good. Please update the code according to my suggestions underneath and flag me again for review. ::: dom/security/nsCSPParser.cpp @@ +566,5 @@ > + if (doc) { > + doc->SetHasReferrerPolicyCSP(true); > + } > + return new nsCSPKeywordSrc(CSP_KeywordToEnum(mCurToken)); > + } Referrer is not a keyword. Please just add the code where you query the doc from the context here [1] and we are good to go - thanks! [1] https://dxr.mozilla.org/mozilla-central/source/dom/security/nsCSPParser.cpp#873 ::: dom/security/nsCSPUtils.h @@ +124,5 @@ > CSP_UNSAFE_INLINE, > CSP_UNSAFE_EVAL, > CSP_NONE, > CSP_NONCE, > + CSP_REFERRER_POLICY, Please discard all changes within nsCSPUtils.h

Adding francois because we need a data-review for gathering telemetry data in the end.

Comment on attachment 8799249 [details] [diff] [review] new.diff Review of attachment 8799249 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me, but we need a module peer to give an r+ before you can land. I found some tiny nits that I'd like you to fix below, then flag ckerschb for review: ::: dom/security/nsCSPParser.cpp @@ +558,5 @@ > doc->SetHasUnsafeEvalCSP(true); > } > return new nsCSPKeywordSrc(CSP_KeywordToEnum(mCurToken)); > } > + You introduced a newline here. Please remove. ::: toolkit/components/telemetry/Histograms.json @@ +3854,5 @@ > + "alert_emails": ["seceng@mozilla.com"], > + "bug_numbers": [1303685], > + "expires_in_version": "55", > + "kind": "count", > + "description": "Number of unique pages that contain an referrer CSP directive" Nit: "a CSP referrer directive"

Comment on attachment 8799249 [details] [diff] [review] new.diff Review of attachment 8799249 [details] [diff] [review]: ----------------------------------------------------------------- r=me, but please fix freddy's nits. @Francois: Can you do a quick data-review for the telemetry data gathered? Thanks!

Adding Francois to review the new diff.

Comment on attachment 8799471 [details] [diff] [review] final.diff Review of attachment 8799471 [details] [diff] [review]: ----------------------------------------------------------------- Carrying over r+ from Freddy and Me. Francois, can you please do the data-review?

Comment on attachment 8799471 [details] [diff] [review] final.diff Review of attachment 8799471 [details] [diff] [review]: ----------------------------------------------------------------- I'm not sure that the telemetry data collected here is the right one. What question are you trying to answer? Is it: what perecentage of sites with a site policy include the "referrer" directive? If so, I would suggest using a "boolean" type and reporting either "false" when we encounter a CSP policy without "referrer" and "true" when we encounter a CSP policy with a "referrer" directive.

Comment on attachment 8800351 [details] [diff] [review] new.diff Review of attachment 8800351 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/telemetry/Histograms.json @@ +3854,5 @@ > + "alert_emails": ["seceng@mozilla.com"], > + "bug_numbers": [1303685], > + "expires_in_version": "55", > + "kind": "boolean", > + "description": "whether CSP referrer directive is present" Assuming that the probe is in the right place (I'll let Christoph/Freddy confirm), the description needs to be more explicit. Maybe something like: Whether a document with a CSP policy (report-only or enforcing) contains a referrer directive ("true") or not ("false").

Comment on attachment 8800537 [details] [diff] [review] new.diff Review of attachment 8800537 [details] [diff] [review]: ----------------------------------------------------------------- Clearing review, please follow my nit and flag us again - thanks! ::: dom/base/nsDocument.cpp @@ +1454,5 @@ > } > if (mHasUnsafeEvalCSP) { > Accumulate(Telemetry::CSP_UNSAFE_EVAL_DOCUMENTS_COUNT, 1); > } > + Accumulate(Telemetry::CSP_REFERRER_POLICY, mHasReferrerPolicyCSP); This was correct before, now we are reporting for *all* documents which pollutes the result. Please revert.

Comment on attachment 8800793 [details] [diff] [review] final.diff Review of attachment 8800793 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/nsDocument.cpp @@ +1455,5 @@ > if (mHasUnsafeEvalCSP) { > Accumulate(Telemetry::CSP_UNSAFE_EVAL_DOCUMENTS_COUNT, 1); > } > + if (mHasReferrerPolicyCSP) { > + Accumulate(Telemetry::CSP_REFERRER_POLICY, true); This won't keep track of CSP policies that don't use the referrer directive. You should replace this if statement with: Accumulate(Telemetry::CSP_REFERRER_POLICY, mHasReferrerPolicyCSP); so that a telemetry ping is generated regardless of whether or not a referrer directive is present. ::: toolkit/components/telemetry/Histograms.json @@ +3849,5 @@ > "expires_in_version": "55", > "kind": "count", > "description": "Number of unique pages that contain an unsafe-eval CSP directive" > }, > + "CSP_REFERRER_POLICY": { nit: A better name for this would be "CSP_REFERRER_DIRECTIVE".

Comment on attachment 8800982 [details] [diff] [review] final.diff Review of attachment 8800982 [details] [diff] [review]: ----------------------------------------------------------------- Please address the following, then flag Francois for review. ::: dom/base/nsDocument.cpp @@ +1454,5 @@ > } > if (mHasUnsafeEvalCSP) { > Accumulate(Telemetry::CSP_UNSAFE_EVAL_DOCUMENTS_COUNT, 1); > } > + Accumulate(Telemetry::CSP_REFERRER_DIRECTIVE, mHasReferrerPolicyCSP); As Christoph mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=1303685#c19: This reports for *every* document, whether it has the directive. What we want is to report only for the documents that have the directive. Please wrap this in an "if" statement and set the value to 1, like the other cases above.

The reason for not enclosing it in if statement is that CSP_REFERRER_DIRECTIVE is a boolean metric that is true when mHasReferrerPolicyCSP is true and false otherwise. The telemetry page would generate a pie chart of two segments(true and false), where 'true' is the % of documents that have mHasReferrerPolicyCSP true and 'false' is the % of documents that have mHasReferrerPolicyCSP false. This would report for all documents, mark them either true or false. Refer Francois' comment https://bugzilla.mozilla.org/show_bug.cgi?id=1303685#c21 Should I continue with CSP_REFERRER_DIRECTIVE as boolean or count ?

Comment on attachment 8800982 [details] [diff] [review] final.diff Right. Sorry about the misunderstanding :( Francois, please review.

Also, datareview+.

does not apply cleanly, could you take a look, thanks

This is the same patch on the latest firefox code. Do I need to get it reviewed again ?

Comment on attachment 8802177 [details] [diff] [review] patch.diff Review of attachment 8802177 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/nsDocument.cpp @@ +1438,5 @@ > } > if (mHasUnsafeEvalCSP) { > Accumulate(Telemetry::CSP_UNSAFE_EVAL_DOCUMENTS_COUNT, 1); > } > + Accumulate(Telemetry::CSP_REFERRER_DIRECTIVE, mHasReferrerPolicyCSP); I think I already pointed out that this is incorrect. This will report for *all* documents whether we set a referrer Policy or not. The information we want to gather is how many pages that have a CSP policy use the referrer directive, which is quite different, no?

Comment on attachment 8802177 [details] [diff] [review] patch.diff Review of attachment 8802177 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/nsDocument.cpp @@ +1443,1 @@ > } Probably best if you just move the Accumulate part within if (mHasCSP) { ... Accumulate(Telemetry::CSP_REFERRER_DIRECTIVE, mHasReferrerPolicyCSP); }

refer https://bugzilla.mozilla.org/show_bug.cgi?id=1303685#c25 and https://bugzilla.mozilla.org/show_bug.cgi?id=1303685#c26

(In reply to tuhina chatterjee from comment #32) > refer https://bugzilla.mozilla.org/show_bug.cgi?id=1303685#c25 and > https://bugzilla.mozilla.org/show_bug.cgi?id=1303685#c26 Please see comment 31 - thanks!

Sorry, I should have responded to this earlier. I missed Christoph's comment the first time and then assumed from Freddy's reply that it was now clear. (In reply to Christoph Kerschbaumer [:ckerschb] from comment #30) > ::: dom/base/nsDocument.cpp > @@ +1438,5 @@ > > } > > if (mHasUnsafeEvalCSP) { > > Accumulate(Telemetry::CSP_UNSAFE_EVAL_DOCUMENTS_COUNT, 1); > > } > > + Accumulate(Telemetry::CSP_REFERRER_DIRECTIVE, mHasReferrerPolicyCSP); > > I think I already pointed out that this is incorrect. This will report for > *all* documents whether we set a referrer Policy or not. The information we > want to gather is how many pages that have a CSP policy use the referrer > directive, which is quite different, no? The original implementation was to use a count that was only counting the documents that have the directive. I'm not entirely sure whether or not that kind of usage is supported in telemetry because I haven't really seen it, but more importantly, I don't think it's going to tell you anything useful. For example, if the number you get is "50". What does that tell you? It could just be the same person reloading a single page 50 times. In that case, usage of that directive is actually almost zero. However, if those 50 are actually out of about 500 CSP policies, then it's a big deal. That's why I suggested using a boolean and looking at the ratio of "CSP policies with referrer" / "all documents with a CSP policy" I think that will give you the number you need: "The referrer directive is used in 0.00001% of CSP policies for documents that have one."

(In reply to François Marier [:francois] from comment #34) > That's why I suggested using a boolean and looking at the ratio of "CSP > policies with referrer" / "all documents with a CSP policy" Sorry for the lag of response. I was out at a conference. Anyway, I agree - I suppose that's what I suggested within comment 31. If the page has a CSP we collect whether the referrer directive is in use.

Final diff

Comment on attachment 8807605 [details] [diff] [review] final_review_patch.diff Review of attachment 8807605 [details] [diff] [review]: ----------------------------------------------------------------- datareview+ provided you change the email address ::: toolkit/components/telemetry/Histograms.json @@ +3874,5 @@ > "kind": "count", > "description": "Number of unique pages that contain an unsafe-eval CSP directive" > }, > + "CSP_REFERRER_DIRECTIVE": { > + "alert_emails": ["seceng@mozilla.com"], Use "seceng-telemetry@mozilla.com" instead.

Comment on attachment 8807605 [details] [diff] [review] final_review_patch.diff Review of attachment 8807605 [details] [diff] [review]: ----------------------------------------------------------------- You don't need my review if you have Francois'.

Tuhina, it seems this bug is almost ready to land. Do you wanna push it over the finishing line and get it landed?

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #39) > Tuhina, it seems this bug is almost ready to land. Do you wanna push it over > the finishing line and get it landed? OK, this bug is basically done, let me finish that up.

Pushed by mozilla@christophkerschbaumer.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/dfb7a1208ee7 Add telemetry for CSP referrer directive. r=ckerschb,francois