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)
Core
DOM: Security
Tracking
()
RESOLVED
FIXED
mozilla64
Tracking | Status | |
---|---|---|
firefox64 | --- | fixed |
People
(Reporter: ckerschb, Assigned: vinoth)
References
Details
(Whiteboard: [domsecurity-active])
Attachments
(1 file, 2 obsolete files)
2.44 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Reporter | ||
Updated•7 years ago
|
Assignee: nobody → cegvinoth
Status: NEW → ASSIGNED
Priority: -- → P2
Whiteboard: [domsecurity-active]
Reporter | ||
Comment 1•7 years ago
|
||
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)
Comment 2•7 years ago
|
||
Tim Spurway is the engineering manager for New Tab.
Flags: needinfo?(ckarlof) → needinfo?(tspurway)
Comment 3•7 years ago
|
||
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)
Reporter | ||
Comment 4•6 years ago
|
||
(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•6 years ago
|
||
Assignee | ||
Comment 6•6 years ago
|
||
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 7•6 years ago
|
||
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+
Assignee | ||
Comment 8•6 years ago
|
||
(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.
Reporter | ||
Comment 9•6 years ago
|
||
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)
Assignee | ||
Comment 10•6 years ago
|
||
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)
Reporter | ||
Comment 11•6 years ago
|
||
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
Reporter | ||
Comment 12•6 years ago
|
||
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.
Reporter | ||
Comment 13•6 years ago
|
||
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)
Comment 14•6 years ago
|
||
(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)
Reporter | ||
Comment 15•6 years ago
|
||
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
Reporter | ||
Updated•6 years ago
|
Attachment #8988468 -
Attachment is obsolete: true
Reporter | ||
Comment 16•6 years ago
|
||
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)
Reporter | ||
Comment 17•6 years ago
|
||
Updated•6 years ago
|
Attachment #9008708 -
Flags: review?(bugs) → review+
Comment 18•6 years ago
|
||
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
Comment 19•6 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox64:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Updated•5 years ago
|
Flags: needinfo?(khudson)
You need to log in
before you can comment on or make changes to this bug.
Description
•