Nightly reports incorrect CSP violations when layout.css.servo.enabled is enabled. Steps to reproduce: 1. enable layout.css.servo.enabled 2. open JS console 3. go to https://duckduckgo.com 4. now go to https://tocco.ch (by typing it in the address bar) 5. observe the CSP errors in the JS console (they do not appear when servo.enabled = false) JS console output: Content Security Policy: The page’s settings observed the loading of a resource at https://duckduckgo.com/font/ProximaNova-Sbold-webfont.woff (“font-src https://www.tocco.ch data: https://fonts.google.com https://fonts.gstatic.com https://maxcdn.bootstrapcdn.com”). A CSP report is being sent. Content Security Policy: The page’s settings observed the loading of a resource at https://duckduckgo.com/font/ProximaNova-Reg-webfont.woff (“font-src https://www.tocco.ch data: https://fonts.google.com https://fonts.gstatic.com https://maxcdn.bootstrapcdn.com”). A CSP report is being sent. CSP reports: { "csp-report": { "violated-directive": "font-src https://www.tocco.ch data: https://fonts.google.com https://fonts.gstatic.com https://maxcdn.bootstrapcdn.com", "referrer": "", "original-policy": "default-src https://www.tocco.ch; style-src https://www.tocco.ch 'unsafe-inline' data: https://fonts.googleapis.com https://maxcdn.bootstrapcdn.com; font-src https://www.tocco.ch data: https://fonts.google.com https://fonts.gstatic.com https://maxcdn.bootstrapcdn.com; script-src https://www.tocco.ch 'unsafe-inline' 'unsafe-eval' https://ssl.google-analytics.com https://s7.addthis.com https://m.addthis.com https://m.addthisedge.com https://graph.facebook.com https://api-public.addthis.com https://maps.googleapis.com https://www.linkedin.com https://analytics.google.com https://www.google.com https://ssl.gstatic.com; connect-src https://www.tocco.ch https://s7.addthis.com https://m.addthis.com; img-src * data:; child-src https://www.tocco.ch https://manual.tocco.ch https://s7.addthis.com https://docs.google.com https://www.youtube.com https://calendar.google.com https://www.google.com https://www.facebook.com; report-uri https://csp-reports.tocco.ch/r", "document-uri": "https://www.tocco.ch/", "blocked-uri": "https://duckduckgo.com" }, "x-source-info": { "timestamp": "2017-07-26T22:27:53.614739Z", "user-agent": "Mozilla/5.0 (Windows NT 6.1; rv:50.0) Gecko/20100101 Firefox/50.0", "path": "/r", "client-address": "155.4.230.97", "host": "csp-reports.tocco.ch" } } { "csp-report": { "violated-directive": "font-src https://www.tocco.ch data: https://fonts.google.com https://fonts.gstatic.com https://maxcdn.bootstrapcdn.com", "referrer": "", "original-policy": "default-src https://www.tocco.ch; style-src https://www.tocco.ch 'unsafe-inline' data: https://fonts.googleapis.com https://maxcdn.bootstrapcdn.com; font-src https://www.tocco.ch data: https://fonts.google.com https://fonts.gstatic.com https://maxcdn.bootstrapcdn.com; script-src https://www.tocco.ch 'unsafe-inline' 'unsafe-eval' https://ssl.google-analytics.com https://s7.addthis.com https://m.addthis.com https://m.addthisedge.com https://graph.facebook.com https://api-public.addthis.com https://maps.googleapis.com https://www.linkedin.com https://analytics.google.com https://www.google.com https://ssl.gstatic.com; connect-src https://www.tocco.ch https://s7.addthis.com https://m.addthis.com; img-src * data:; child-src https://www.tocco.ch https://manual.tocco.ch https://s7.addthis.com https://docs.google.com https://www.youtube.com https://calendar.google.com https://www.google.com https://www.facebook.com; report-uri https://csp-reports.tocco.ch/r", "document-uri": "https://www.tocco.ch/", "blocked-uri": "https://duckduckgo.com" }, "x-source-info": { "timestamp": "2017-07-26T22:27:53.655327Z", "user-agent": "Mozilla/5.0 (Windows NT 6.1; rv:50.0) Gecko/20100101 Firefox/50.0", "path": "/r", "client-address": "155.4.230.97", "host": "csp-reports.tocco.ch" } }

Thank you :) I can confirm the different console outputs (related to CSP) in Nightly 56 x64 20170726100241 @ Debian Testing.

Thanks for the bug report, Peter. It looks like this is due to the pre-emptive calling of FontFaceSet::IsFontLoadAllowed we added in part 4 of bug 1376964. I guess it wasn't obvious to me that the NS_CheckContentLoadPolicy call not only does a check, but could result in CSP reports being sent. I guess what I want is a way to call NS_CheckContentLoadPolicy, and to store away the CSP report that would be made due to the CSP failure, so that later on, from the style worker threads, if we look up the cached IsFontLoadAllowed result and find that it failed due to a CSP check, that we can make the report at that time.

Comment on attachment 8891630 [details] Bug 1384741 - Part 1: Add facility to buffer up CSP violation reports. Chrisoph, can you tell me if the changes I'm making to the CSP code seems reasonable to you? I want to capture all of the reports that the nsCSPContext might make while under the FontFaceSet::IsFontLoadAllowed call, so that I can dispatch their runnables later if a page actually tries to make such a request.

I initially thought I'd just pass an argument all the way down from NS_CheckContentLoadPolicy, but I wasn't sure that would work if redirects are involved, since it seemed tricky to ensure that argument would get into CSPService::AsyncOnChannelRedirect.

Comment on attachment 8891630 [details] Bug 1384741 - Part 1: Add facility to buffer up CSP violation reports. (In reply to Cameron McCormack (:heycam) from comment #5) > Chrisoph, can you tell me if the changes I'm making to the CSP code seems > reasonable to you? Yes, that all seems reasonable to me. Please also make sure to check with a dom/ peer, but as I said, looks good to me. (In reply to Cameron McCormack (:heycam) from comment #6) > I initially thought I'd just pass an argument all the way down from > NS_CheckContentLoadPolicy, but I wasn't sure that would work if redirects > are involved, since it seemed tricky to ensure that argument would get into > CSPService::AsyncOnChannelRedirect. Unfortunately too many addons are still relying NS_CheckContentLoadPolicy(), hence updating that macro is not going to work. In other words, you are using the right approach.

Obviously a testcase would be nice to have for that particular usecase. Also JFYI: there are numerous CSP tests, you can run them with: ./mach test dom/security/test/csp

Comment on attachment 8894387 [details] Bug 1384741 - Part 3: Buffer up CSP violation reports when pre-emptively checking cached font loads, and dispatch them when trying to use cached fonts. https://reviewboard.mozilla.org/r/165572/#review170842 ::: gfx/thebes/gfxUserFontSet.cpp:1408 (Diff revision 1) > + gfxUserFontSet* aUserFontSet) const > +{ > + LoadResultEntry* entry = mAllowedFontSets.GetEntry(aUserFontSet); > + MOZ_ASSERT(entry, "UpdateAllowedFontSets should have been called and " > + "added an entry to mAllowedFontSets"); > + if (!entry->mViolations.IsEmpty()) { Can we assert that `entry->mAllowed == entry->mViolations.IsEmpty()` here? i.e. if allowed, there are no violations; if not, at least one should have been recorded.

Comment on attachment 8894388 [details] Bug 1384741 - Part 4: Test that we don't send CSP violation reports for cached fonts we don't actually use. https://reviewboard.mozilla.org/r/165574/#review170942

Comment on attachment 8891630 [details] Bug 1384741 - Part 1: Add facility to buffer up CSP violation reports. https://reviewboard.mozilla.org/r/162730/#review172460 ::: dom/base/nsIDocument.h:878 (Diff revision 2) > + > + /** > + * Called when a CSP violation is encountered that would generate a report > + * while buffering is enabled. > + */ > + void ReportCSPViolation(nsIRunnable* aReportingRunnable) Maybe this should be called BufferCSPViolation? ::: dom/base/nsIDocument.h:881 (Diff revision 2) > + * while buffering is enabled. > + */ > + void ReportCSPViolation(nsIRunnable* aReportingRunnable) > + { > + MOZ_ASSERT(mBufferingCSPViolations); > + mBufferedCSPViolations.AppendElement(aReportingRunnable); Could there be enough of these in flight that we could OOM here? If so, it might be better to drop reports on the floor on OOM than crash...

Comment on attachment 8894386 [details] Bug 1384741 - Part 2: Allow file_report_chromescript.js to listen for more than one CSP violation report. https://reviewboard.mozilla.org/r/165570/#review172462

Comment on attachment 8894387 [details] Bug 1384741 - Part 3: Buffer up CSP violation reports when pre-emptively checking cached font loads, and dispatch them when trying to use cached fonts. https://reviewboard.mozilla.org/r/165572/#review170842 > Can we assert that `entry->mAllowed == entry->mViolations.IsEmpty()` here? i.e. if allowed, there are no violations; if not, at least one should have been recorded. We might have recorded mAllowed = false for a non-CSP violation related reason. Or the CSP directives might not have indicated that reports are sent. But I can assert that if the array is non-empty, then mAllowed must be false.

Comment on attachment 8894387 [details] Bug 1384741 - Part 3: Buffer up CSP violation reports when pre-emptively checking cached font loads, and dispatch them when trying to use cached fonts. https://reviewboard.mozilla.org/r/165572/#review170842 > We might have recorded mAllowed = false for a non-CSP violation related reason. Or the CSP directives might not have indicated that reports are sent. But I can assert that if the array is non-empty, then mAllowed must be false. Actually, I think that's not even possible to assert, since we might only have CSP violation reports enabled, but the load is still allowed.

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. hg error in cmd: hg rebase -s c81087bbf0d3 -d cbacd472fc22: rebasing 413121:c81087bbf0d3 "Bug 1384741 - Part 1: Add facility to buffer up CSP violation reports. r=bz" merging dom/base/nsDocument.cpp merging dom/base/nsIDocument.h merging dom/security/nsCSPContext.cpp warning: conflicts while merging dom/base/nsDocument.cpp! (edit, then use 'hg resolve --mark') warning: conflicts while merging dom/base/nsIDocument.h! (edit, then use 'hg resolve --mark') unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment on attachment 8891630 [details] Bug 1384741 - Part 1: Add facility to buffer up CSP violation reports. We should uplift this to beta for the pref experiment to avoid annoying web site operators with spurious CSP report violations. Approval Request Comment [Feature/Bug causing the regression]: bug 1376964 [User impact if declined]: spurious CSP violation reports can be sent if stylo is enabled [Is this code covered by automated tests?]: yes [Has the fix been verified in Nightly?]: no, just landed on autoland [Needs manual test from QE? If yes, steps to reproduce]: no [List of other uplifts needed for the feature/fix]: all four patches on this bug [Is the change risky?]: not very [Why is the change risky/not risky?]: it only affects stylo, and we have tests [String changes made/needed]: none

Pushed by cmccormack@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/41c3fa41a268 Part 1: Add facility to buffer up CSP violation reports. r=bz https://hg.mozilla.org/integration/autoland/rev/de7dba78e6fe Part 2: Allow file_report_chromescript.js to listen for more than one CSP violation report. r=bz https://hg.mozilla.org/integration/autoland/rev/549366daed98 Part 3: Buffer up CSP violation reports when pre-emptively checking cached font loads, and dispatch them when trying to use cached fonts. r=jfkthame https://hg.mozilla.org/integration/autoland/rev/4d5fc5ec7769 Part 4: Test that we don't send CSP violation reports for cached fonts we don't actually use. r=jfkthame

Pushed by bholley@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/1a3ab2692bc9 Followup bustage fix. r=me

https://hg.mozilla.org/mozilla-central/rev/41c3fa41a268 https://hg.mozilla.org/mozilla-central/rev/de7dba78e6fe https://hg.mozilla.org/mozilla-central/rev/549366daed98 https://hg.mozilla.org/mozilla-central/rev/4d5fc5ec7769 https://hg.mozilla.org/mozilla-central/rev/1a3ab2692bc9

Comment on attachment 8891630 [details] Bug 1384741 - Part 1: Add facility to buffer up CSP violation reports. Avoid unnecessary CSP warnings. Please uplift for beta 3 to support the upcoming stylo experiment on beta.

Needs rebasing.

try push for rebased patches on beta: https://treeherder.mozilla.org/#/jobs?repo=try&revision=728218fad66f5c09385588b979507788eca44b39

remote: https://hg.mozilla.org/releases/mozilla-beta/rev/2ca70aa40eae0268d6abf65175aa93a7c14fa7a1 remote: https://hg.mozilla.org/releases/mozilla-beta/rev/ce4bac7cc01ee52323fbd9b118a338622cfc068c remote: https://hg.mozilla.org/releases/mozilla-beta/rev/083d55339a1dff04531da5aca017191bac5f42a3 remote: https://hg.mozilla.org/releases/mozilla-beta/rev/578f340363a37e0887038e39a970e6b83cd00563 remote: https://hg.mozilla.org/releases/mozilla-beta/rev/87ae6494d5b1fff0b2cd388c6d639e29f438fe77