Closed Bug 1490977 Opened 6 years ago Closed 6 years ago

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


(Core :: DOM: Security, enhancement, P2)




Tracking Status
firefox64 --- fixed


(Reporter: ckerschb, Assigned: ckerschb)



(Whiteboard: [domsecurity-active])


(1 file, 2 obsolete files)

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
Depends on: 1459544
Priority: -- → P3
Whiteboard: [domsecurity-backlog1]
Attached patch test_assert_about_csp.patch (obsolete) — Splinter Review
I started writing a test but ran into some problems with BrowserTestUtils.registerAboutPage. See also:
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.

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:

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.
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]

>+  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
Assert content privileged about page has CSP. r=smaug
Backout by
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:
> 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:
Pushed by
Assert content privileged about page has CSP. r=smaug
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in before you can comment on or make changes to this bug.