Closed Bug 1459544 Opened 2 years ago Closed Last year

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
https://hg.mozilla.org/mozilla-central/rev/084a50d2778a
Status: ASSIGNED → RESOLVED
Closed: Last year
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.