To get a better idea of CSP usage we should add some telemetry. This bug adds telemetry for: a) how many sites use CSP b) how many use unsafe-inline c) how many use unsafe-eval

Review commit: https://reviewboard.mozilla.org/r/37641/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/37641/

Comment on attachment 8725794 [details] MozReview Request: Bug 1252829 - CSP Telemetry, r=ckerschb, p=bsmedberg https://reviewboard.mozilla.org/r/37641/#review34477 Looks good to me - thanks, but please answer my question underneath before landing. ::: toolkit/components/telemetry/Histograms.json:3298 (Diff revision 1) > + }, I don't know what low, high, and n_buckets is, also I don't know what 'opt-out' means in that context. Do you know or did you copy those from MixedContent, SafeBrowsing or other related Telementry code?

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #2) > Comment on attachment 8725794 [details] > MozReview Request: Bug 1252829 - CSP Telemetry, r?ckerschb > > I don't know what low, high, and n_buckets is, also I don't know what > 'opt-out' means in that context. Do you know or did you copy those from > MixedContent, SafeBrowsing or other related Telementry code? so if the rest is ok, we should get better numbers for the histogram. I'd suggest something like [1]. That should give us a good idea of the distribution of pages. Higher than 1000 is probably not interesting looking at [2]. But this also depends on what exactly we're interested in. Thoughts? [1] https://telemetry.mozilla.org/histogram-simulator/#low=0&high=1000&n_buckets=20&kind=exponential&generate=log-normal [2] https://telemetry.mozilla.org/new-pipeline/dist.html#!cumulative=0&end_date=2016-03-03&keys=__none__!__none__!__none__&max_channel_version=nightly%252F47&measure=PLACES_PAGES_COUNT&min_channel_version=null&product=Firefox&sanitize=1&sort_keys=submissions&start_date=2016-01-25&table=0&trim=1&use_submission_date=0

Actually, I took a closer look and I think we have to update a few things: > "alert_emails": ["seceng@mozilla.com"], Shouldn't that be: seceng-telemetry@mozilla.com > "expires_in_version": "55", Probably we don't want that to expire ever, can't we use |"expires_in_version": "never",|? > "kind": "exponential", > "low": 1000, > "high": 150000, > "n_buckets": 20, > "releaseChannelCollection": "opt-out", Again, I don't fully know what those parameters mean. We know that only around 1% of sites use CSP. If you take the Alexa Top 1million sites. The other thing I just realized is > if (mHasCSP) { > Accumulate(Telemetry::CSP_PAGES_COUNT, 1); > } shouldn't we have an |else| branch here that does something like |Accumulate(Telemetry::CSP_PAGES_COUNT, 0);|? Probably someone who has done Telemetry more often than me has a better understanding of how the accumulation happens. Please don't land before we have sorted those issues - thanks!

Clearing my needinfo for now.

Comment on attachment 8725794 [details] MozReview Request: Bug 1252829 - CSP Telemetry, r=ckerschb, p=bsmedberg Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37641/diff/1-2/

https://reviewboard.mozilla.org/r/37641/#review35859 ::: dom/base/nsDocument.cpp:1591 (Diff revision 2) > + Accumulate(Telemetry::CSP_PAGES_COUNT, 0); When using `"count"` probes here, you only need to `Accumulate(..., 1)` and don't need this. ::: toolkit/components/telemetry/Histograms.json:3286 (Diff revision 2) > + "kind": "exponential", From the `Accumulate(..., 1)` code, you actually want to use `"kind": "count"` here. ::: toolkit/components/telemetry/Histograms.json:3290 (Diff revision 2) > + "releaseChannelCollection": "opt-out", Is collecting this data from the beta channel sufficient? Unless there is a specific need to collect this from (nearly) all release users, let's leave out this line (it defaults to "opt-in").

Comment on attachment 8725794 [details] MozReview Request: Bug 1252829 - CSP Telemetry, r=ckerschb, p=bsmedberg Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37641/diff/2-3/

Thanks Georg for the good feedback. I updated the patch as discussed.

So this should be fine. Ally, can you do the data collection review?

https://reviewboard.mozilla.org/r/37641/#review35919 ::: dom/base/nsDocument.cpp:1591 (Diff revision 3) > + Accumulate(Telemetry::CSP_PAGES_COUNT, 0); You should remove the 0 count here.

not my day, forgot to update that. Will wait for data collection review from ally before uploading a new version.

maybe Benjamin has a minute for this data collection review.

https://reviewboard.mozilla.org/r/37641/#review36393 ::: toolkit/components/telemetry/Histograms.json:3287 (Diff revision 3) > + "CSP_PAGES_COUNT": { > + "alert_emails": ["seceng@mozilla.com"], > + "bug_numbers": [1252829], > + "expires_in_version": "55", > + "kind": "count", > + "description": "Number of unique pages that contain a CSP" "pages" here is a little ambiguous. Does this count all loaded document (including frames/iframes), or just top-level documents? I suggest using "documents" instead of "pages" and clarifying that. That way it's clear that in-page navigation such as .pushState or .replace aren't counted in the totals. Is it important for you to know the ratio of "documents with CSP" to "total documents" or is this count useful on its own?

Comment on attachment 8725794 [details] MozReview Request: Bug 1252829 - CSP Telemetry, r=ckerschb, p=bsmedberg data-review=me in any case

So, I am fine with that. Again, what we really want to know is how many (toplevel) sites use CSP, how many of those use unsafe-inline and unsafe-eval. It seems we can query the total number of sites from the mixed content telemetry reports as well. Please incorporate Benjamin's suggestions and we are good to go. Finally getting some stats on CSP.

Comment on attachment 8725794 [details] MozReview Request: Bug 1252829 - CSP Telemetry, r=ckerschb, p=bsmedberg Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37641/diff/3-4/

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #4) > Actually, I took a closer look and I think we have to update a few things: > > "alert_emails": ["seceng@mozilla.com"], > Shouldn't that be: seceng-telemetry@mozilla.com I think seceng will not be happy to receive those emails. I suppose you wanted to update that before landing, no? > > "expires_in_version": "55", > Probably we don't want that to expire ever, can't we use > |"expires_in_version": "never",|? Why do we only care about CSP telemetry only up to FF55? Didn't we agree on using 'never'?

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #20) > (In reply to Christoph Kerschbaumer [:ckerschb] from comment #4) > > Actually, I took a closer look and I think we have to update a few things: > > > "alert_emails": ["seceng@mozilla.com"], > > Shouldn't that be: seceng-telemetry@mozilla.com > > I think seceng will not be happy to receive those emails. I suppose you > wanted to update that before landing, no? Ah, missed that. seceng@ is what most other probes have as well. I don't know who gets those e-mails anyway. > > > "expires_in_version": "55", > > Probably we don't want that to expire ever, can't we use > > |"expires_in_version": "never",|? > > Why do we only care about CSP telemetry only up to FF55? Didn't we agree on > using 'never'? 'never' is not accepted by telemetry folks anymore. 55 should be fine for a while. We have to update then once we hit 54 latest.