Closed Bug 1459544 Opened 3 years ago Closed 2 years ago
Apply Meta CSP to Content Privileged about:newtab
No description provided.
Assignee: nobody → cegvinoth
Status: NEW → ASSIGNED
Priority: -- → P2
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 , 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!  https://github.com/mozilla/activity-stream
Tim Spurway is the engineering manager for New Tab.
Flags: needinfo?(ckarlof) → needinfo?(tspurway)
Hey :ckerschb. The activity stream code is currently pulled from  (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.
(In reply to Tim Spurway [:tspurway] from comment #3) > Hey :ckerschb. The activity stream code is currently pulled from  (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.
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!
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.
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.
(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()  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.
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)
Pushed by email@example.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
You need to log in before you can comment on or make changes to this bug.