Write test to make sure newly registered about page asserts if loaded without a CSP

RESOLVED FIXED in Firefox 64

Status

()

enhancement
P2
normal
RESOLVED FIXED
10 months ago
10 months ago

People

(Reporter: ckerschb, Assigned: ckerschb)

Tracking

unspecified
mozilla64
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox64 fixed)

Details

(Whiteboard: [domsecurity-active])

Attachments

(1 attachment, 2 obsolete attachments)

We should write a browser test that registers a new about page. Then we load that about page and make sure that our assertion keeps working correctly. We can annotate that test with 'expected assertions' 1 or something to make sure it fires in such a case.
Assignee: nobody → cegvinoth
Blocks: 1449872
Status: NEW → ASSIGNED
Depends on: 1459544
Priority: -- → P3
Whiteboard: [domsecurity-backlog1]
Posted patch test_assert_about_csp.patch (obsolete) — Splinter Review
I started writing a test but ran into some problems with BrowserTestUtils.registerAboutPage. See also:
https://bugzilla.mozilla.org/show_bug.cgi?id=1459544#c13
Hey Smaug, we would like to make sure that our assertion keeps firing correctly in case an about: page ships without a CSP. In the end it seems that a mochitest is the best solution to achieve that. Please note that the assertion only runs in debug builds anyway, so I guess adding Preferences::GetBool() should be fine.

Thanks!
Attachment #9008707 - Attachment is obsolete: true
Attachment #9009847 - Flags: review?(bugs)
Assignee: cegvinoth → ckerschb
Priority: P3 → P2
Whiteboard: [domsecurity-backlog1] → [domsecurity-active]
Ah, this is not working properly:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e51016ed7cc7cf7f983acfd4f689be5059fdab99&selectedJob=199869214

but then how do we annotate a test where we expect an assertion?
Doesn't expectAssertions help with NS_ASSERTIONs? NS_ASSERTIONs are more like warnings.
Attachment #9009847 - Flags: review?(bugs)
Since this is debug-only code, would you be willing to accept that patch where we branch on the pref and either do NS_ASSERT for the test VS MOZ_ASSERT in the regular case?
Attachment #9009847 - Attachment is obsolete: true
Attachment #9009928 - Flags: review?(bugs)
Comment on attachment 9009928 [details] [diff] [review]
bug_1490977_assert_about_no_csp_assert.patch

>+  if (Preferences::GetBool("csp.overrule_content_privileged_about_uris_without_csp_whitelist")) {
>+    NS_ASSERTION(parsedPolicyStr.Find("default-src") >= 0,
>+     "about: page must contain a CSP including default-src");
odd indentation

But, another option, perhaps a bit nicer, would be to dispatch some event for testing case when the pref is set.
That would mean the method should take nsIDocument* as param and then dispatch
nsContentUtils::DispatchEvent(aDocument, aDocument, NS_LITERAL_STRING("testing-only-about-csp-check"), ...);


However, I can live with this too
Attachment #9009928 - Flags: review?(bugs) → review+
Pushed by mozilla@christophkerschbaumer.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b4ac15e18538
Assert content privileged about page has CSP. r=smaug
Backout by toros@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d48fc7e23aa7
Backed out changeset b4ac15e18538 for assertion failures at build/build/src/dom/base/nsDocument.cpp on a CLOSED TREE
(In reply to Tiberius Oros[:tiberius_oros] from comment #9)
> Push with the failures:
> https://treeherder.mozilla.org/#/jobs?repo=mozilla-
> inbound&revision=b4ac15e185381a8e410b9c9515d15a26a4719520

Ah, once the whitelist is initialized it doesn't get overriden. In that case we are stuck with a whitelist of "". We have to explicitly clear that cache before resettting the old whitelist.
Flags: needinfo?(ckerschb)
Works correctly locally, but let's make sure TRY is happy as well:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=49bfc0fdba4cc6b491a84de5930de176eff6db35
Pushed by mozilla@christophkerschbaumer.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8da145a5caaf
Assert content privileged about page has CSP. r=smaug
https://hg.mozilla.org/mozilla-central/rev/8da145a5caaf
Status: ASSIGNED → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in before you can comment on or make changes to this bug.