1459544 - Apply Meta CSP to Content Privileged about:newtab

Status: RESOLVED FIXED

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

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

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

Comment 2

[Chris Karlof [:ckarlof]]

Tim Spurway is the engineering manager for New Tab.

Comment 3

[Tim Spurway [: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.

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

(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 5

[Phabricator Automation]

Attachment: Bug 1459544 - home,newtab,welcome removed from all.js

Comment 6

[Vinothkumar Nagasayanan [:vinoth]]

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 7

[Phabricator Automation]

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

Comment 8

[Vinothkumar Nagasayanan [:vinoth]]

(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 9

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8988468 [details]
Bug 1459544 - home,newtab,welcome removed from all.js

This already has my r+; thanks!

Comment 10

[Vinothkumar Nagasayanan [:vinoth]]

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.

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

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

Comment 12

[Christoph Kerschbaumer [:ckerschb]]

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.

Comment 13

[Christoph Kerschbaumer [:ckerschb]]

Attachment: 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?

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

Comment 14

[:Gijs (he/him)]

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

Comment 15

[Christoph Kerschbaumer [:ckerschb]]

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.

Comment 16

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1459544_shrink_whitelist_for_csp_about_assertions.patch

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!

Comment 17

[Christoph Kerschbaumer [:ckerschb]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9b3b4e1622a08e20ccdcaa3e7d32924fb6252d76

Comment 18

[Pulsebot]

Pushed by [e-mail]:
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

Comment 19

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/084a50d2778a