Closed Bug 1459544 Opened 7 years ago Closed 6 years ago

Apply Meta CSP to Content Privileged about:newtab

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: ckerschb, Assigned: vinoth)

References

Details

(Whiteboard: [domsecurity-active])

Attachments

(1 file, 2 obsolete files)

No description provided.
Blocks: 1449872
Assignee: nobody → cegvinoth
Status: NEW → ASSIGNED
Priority: -- → P2
Whiteboard: [domsecurity-active]
Hey Chris, we are currently applying a CSP to all of our content privileged about pages (See Meta Bug 1449872). Can you please connect us with the right folks working on about:newtab (activity stream). It seems currently about about:newtab pulls all the code from [1], right? Currently we have an assertion that waves through content privileged about: pages that don't yet have a strong CSP applied and ultimately we would like to remove that assertion entirely so that all content privileged about pages need to ship with a CSP. Thanks! [1] https://github.com/mozilla/activity-stream
Flags: needinfo?(ckarlof)
Tim Spurway is the engineering manager for New Tab.
Flags: needinfo?(ckarlof) → needinfo?(tspurway)
Hey :ckerschb. The activity stream code is currently pulled from [1] (from comment1). We have a current bug that is scheduled for Fx62 that is relevant here (bug 1448359), which, in effect would 'turn on' basic CSP for about:home and about:newtab. We have some ideas on the 'strong' CSP part and could set up a meeting to discuss.
Flags: needinfo?(tspurway)
Depends on: 1448359
(In reply to Tim Spurway [:tspurway] from comment #3) > Hey :ckerschb. The activity stream code is currently pulled from [1] (from > comment1). > > We have a current bug that is scheduled for Fx62 that is relevant here (bug > 1448359), which, in effect would 'turn on' basic CSP for about:home and > about:newtab. > > We have some ideas on the 'strong' CSP part and could set up a meeting to > discuss. Yeah, I a meeting sounds good. :jkt already left some notes here: https://bugzilla.mozilla.org/show_bug.cgi?id=1448359#c2 Probably we should move the discussion over to Bug 1448359 anyway.
Comment on attachment 8988468 [details] Bug 1459544 - home,newtab,welcome removed from all.js Please review the patch and let me know if changes are needed.
Attachment #8988468 - Flags: review?(ckerschb)
Comment on attachment 8988468 [details] Bug 1459544 - home,newtab,welcome removed from all.js Christoph Kerschbaumer [:ckerschb] has approved the revision. https://phabricator.services.mozilla.com/D1861
Attachment #8988468 - Flags: review+
(In reply to Phabricator Automation from comment #7) > Comment on attachment 8988468 [details] > Bug 1459544 - home,newtab,welcome removed from all.js > > Christoph Kerschbaumer [:ckerschb] has approved the revision. > > https://phabricator.services.mozilla.com/D1861 Ya I will do a TRY push and check. also will fix the nit. Thanks.
Comment on attachment 8988468 [details] Bug 1459544 - home,newtab,welcome removed from all.js This already has my r+; thanks!
Attachment #8988468 - Flags: review?(ckerschb)
Hey k88hudson, In somecases about:newtab is loaded but the content of the newtab is a blank document. Hence assertion failure happens when we try to check for CSP from META tags. Few test files that trigger the assertion are, 1) browser_bug609700.js 2) browser_startup_images.js 3) browser_bug1025195_switchToTabHavingURI_aOpenParams.js 4) browser_remoteness_flip_on_restore.js So I have two quick questions, 1) What are the cases where about:newtab contents are overridden with about:blank contents but still the URI is about:newtab ? 2) Apart from aboutNewTabServer.js any other file implements the overriding behaviour ? Please let me know incase you know any details about this.
Flags: needinfo?(khudson)
It seems we should only assert in case nothing stopped the document load on purpose. Probably we can use mParserAborted to realize that. Seems to work (at least locally): https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c40b9f364b08dde54f21196569b0548475d9fe7
If that turns out to work then we need to write a test where we register a new about page that does *not* have a meta CSP attached and then annotate that test with expectedAssertions 1 or some similar test to verify our assertion keeps working correctly.
Attached patch about_page_test.patch (obsolete) — Splinter Review
Hey Gijs, when you factored out the about page registration and added BrowserTestUtils.registerAboutPage() [1] did browser_e10s_about_page_triggeringprincipal.js still work correctly? I am about to add a test making sure that we assert in case a new registered about page does not ship with a CSP. I run the attached test and get the following error (see stacktrace underneath). I get the same error when running browser_e10s_about_page_triggeringprincipal.js. It seems the problem is related to the flag Ci.nsIAboutModule.URI_MUST_LOAD_IN_CHILD. If I remove that, then for now at least my test runs to completion and doesn't throw the NS_ERROR_MALFORMED_URI exception. Seems surprising to me that browser_e10s_about_page_triggeringprincipal.js works on TRY given that it doesn't locally. Any idea what might be wrong here? Or am I just missing something? [1] https://hg.mozilla.org/mozilla-central/rev/38740bd3a50e#l2.21 0:27.04 GECKO(26695) [Child 26878, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x804B000A: file /home/ckerschb/source/mc/netwerk/base/nsNetUtil.cpp, line 235 0:27.11 INFO Console message: [JavaScript Error: "[Exception... "Component returned failure code: 0x804b000a (NS_ERROR_MALFORMED_URI) [nsIWebNavigation.loadURIWithOptions]" nsresult: "0x804b000a (NS_ERROR_MALFORMED_URI)" location: "JS frame :: resource://gre/actors/WebNavigationChild.jsm :: loadURI/< :: line 112" data: no]"] loadURI/<@resource://gre/actors/WebNavigationChild.jsm:112:14 _wrapURIChangeCall@resource://gre/actors/WebNavigationChild.jsm:63:7 loadURI@resource://gre/actors/WebNavigationChild.jsm:111:5 receiveMessage@resource://gre/actors/WebNavigationChild.jsm:43:9 receiveMessage@resource://gre/modules/ActorManagerChild.jsm:91:39 MessageListener.receiveMessage*init@resource://gre/modules/ActorManagerChild.jsm:40:7 attach@resource://gre/modules/ActorManagerChild.jsm:229:48 @chrome://global/content/browser-content.js:14:1
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #13) > Created attachment 9007994 [details] [diff] [review] > about_page_test.patch > > Hey Gijs, > > when you factored out the about page registration and added > BrowserTestUtils.registerAboutPage() [1] did > browser_e10s_about_page_triggeringprincipal.js still work correctly? It was 18 months ago, I really don't recall, unfortunately. I assume so... from looking at it very very quickly, I imagine that the issue is with the fact that we're trying to load the content script for the registrations on-demand, and then immediately send a message. Presumably the load of the script races with the message and thus we never register the about: page in the child. One possible way around this would be to move the child-process registering thing to a content process script that we always load earlier, e.g. content-utils.js . An easy way to see if that is indeed the issue would be to just cut/paste the contents of mozilla-central/testing/mochitest/BrowserTestUtils/content/content-about-page-utils.js into content-utils.js and see if that fixes the issue for you locally.
Flags: needinfo?(gijskruitbosch+bugs)
Blocks: 1490977
Comment on attachment 9007994 [details] [diff] [review] about_page_test.patch (In reply to :Gijs (he/him) from comment #14) > One possible way around this would be to move the child-process registering > thing to a content process script that we always load earlier, e.g. > content-utils.js . An easy way to see if that is indeed the issue would be > to just cut/paste the contents of > mozilla-central/testing/mochitest/BrowserTestUtils/content/content-about- > page-utils.js into content-utils.js and see if that fixes the issue for you > locally. Thanks Gijs, but your idea did not fix the problem. I didn't manage to receive a message within content-utils.js for registering the about page. We have to dig a little deeper to find out where the problem is. I filed Bug 1490977 to write that test and to figure the problem.
Attachment #9007994 - Attachment is obsolete: true
Attachment #8988468 - Attachment is obsolete: true
Smaug, as discussed the other day it makes more sense to only assert that an about page has indeed a CSP applied if the document load was not cancelled. Thanks!
Attachment #9008708 - Flags: review?(bugs)
Attachment #9008708 - Flags: review?(bugs) → review+
Pushed by mozilla@christophkerschbaumer.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/084a50d2778a Only assert that about page has CSP if nothing stopped the load of the doc. r=smaug
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Depends on: 1500061
Flags: needinfo?(khudson)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: