Closed
Bug 1021667
Opened 10 years ago
Closed 7 years ago
Give Activity Stream URI_SAFE_FOR_UNTRUSTED_CONTENT
Categories
(Firefox :: New Tab Page, defect)
Firefox
New Tab Page
Tracking
()
RESOLVED
FIXED
Firefox 56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: Gijs, Assigned: ursula)
References
Details
Attachments
(1 file)
After moving to HTML and making things e10s-friendly, I believe we could hit the switches in aboutRedirector. At a minimum, we should be able to add URI_SAFE_FOR_UNTRUSTED_CONTENT, I think?
Flags: firefox-backlog+
Comment 1•10 years ago
|
||
Some more stuff we will have to do then is to fix the thumbnails, they're not accessible from web content ATM and we should check if there's a protocol flag that covers our use case. We should also be able to revert the changes from bug 765628 and remove the link checker code. ... and revert the changes from bug 724239 to enable the back button after clicking one of the tiles. There should be one or more bugs for that but I can't find them right now.
Updated•10 years ago
|
Points: --- → 2
Whiteboard: p=2
Reporter | ||
Comment 2•9 years ago
|
||
Ursula or Mike, can you comment as to what's required to get this to work?
Flags: needinfo?(ursulasarracini)
Flags: needinfo?(mconley)
Comment 3•9 years ago
|
||
The work to run about:newtab in the content process was landed in a project branch on GitHub, as the content services team has decided to focus on attempting to host the newtab content remotely. I would imagine that making newtab truly unprivileged is contingent on that work.
Flags: needinfo?(mconley)
Assignee | ||
Comment 4•8 years ago
|
||
Like Mike said, there's work being done on hosting about:newtab remotely, and using WebChannel.jsm to make it unprivileged. The GitHub repos doing that work are here: 1. https://github.com/mozilla/newtab-dev (for all chrome work) 2. https://github.com/mozilla/remote-newtab (for all content work)
Flags: needinfo?(ursulasarracini)
Assignee | ||
Comment 5•7 years ago
|
||
Gijs is this a dupe of bug 1272342?
Flags: needinfo?(gijskruitbosch+bugs)
Reporter | ||
Comment 6•7 years ago
|
||
(In reply to Ursula Sarracini (:ursula) from comment #5) > Gijs is this a dupe of bug 1272342? Maybe. The unfortunate thing here is that there are two somewhat (but not entirely) orthogonal concepts here: 1) does about:newtab run in the content process 2) is about:newtab's document unprivileged My understanding is that with activity stream the answer to (1) is yes (and that is bug 1272342, AIUI?). I don't know if activity stream changes the answer for (2). Ideally it would, but I don't know that it does...
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(usarracini)
Reporter | ||
Comment 7•7 years ago
|
||
I'm updating the summary - I used to misunderstand URI_SAFE_FOR_UNTRUSTED_CONTENT, and that flag alone might not be enough to ensure the content runs with the right principal.
Summary: Unprivileged about:newtab: Toggle relevant bits in aboutRedirector → Unprivileged about:newtab: ensure about:newtab document runs with null principal (or similar)
Assignee | ||
Comment 8•7 years ago
|
||
So for activity stream, bug 1365643 recently landed which I believe should take care of (1), but I don't think (2) (which should be to give activity stream URI_SAFE_FOR_UNTRUSTED_CONTENT) can properly happen for activity stream until we do some work for the thumbnails (bug 1184701). As for the current about:newtab, bug 1272342 was originally filed to flip it so that it runs in content (1), but because we're going to effectively remove the current about:newtab and replace it with activity stream soon (as mentioned in bug 1021654) there really isn't much point in doing this work for current about:newtab right now. Does that make sense?
Flags: needinfo?(usarracini)
Reporter | ||
Comment 9•7 years ago
|
||
(In reply to Ursula Sarracini (:ursula) from comment #8) > So for activity stream, bug 1365643 recently landed which I believe should > take care of (1), but I don't think (2) (which should be to give activity > stream URI_SAFE_FOR_UNTRUSTED_CONTENT) can properly happen for activity > stream until we do some work for the thumbnails (bug 1184701). As for the > current about:newtab, bug 1272342 was originally filed to flip it so that it > runs in content (1), but because we're going to effectively remove the > current about:newtab and replace it with activity stream soon (as mentioned > in bug 1021654) there really isn't much point in doing this work for current > about:newtab right now. Does that make sense? Yep! Sounds like we can keep this open to deal with eventually un-privileging the activity stream document?
Assignee | ||
Comment 10•7 years ago
|
||
Sounds good! Thanks :)
Assignee | ||
Comment 11•7 years ago
|
||
Gijs, mind if I re-purpose this bug's summary to be something along the lines of "Give Activity Stream URI_SAFE_FOR_UNTRUSTED_CONTENT"? We should probably do this before landing it in 56
Flags: needinfo?(gijskruitbosch+bugs)
Reporter | ||
Comment 12•7 years ago
|
||
(In reply to Ursula Sarracini (:ursula) from comment #11) > Gijs, mind if I re-purpose this bug's summary to be something along the > lines of "Give Activity Stream URI_SAFE_FOR_UNTRUSTED_CONTENT"? We should > probably do this before landing it in 56 Nope, feel free to do so. Note that you will also need to ensure we actually downgrade the principal to a codebase or null principal - URI_SAFE_FOR_UNTRUSTED_CONTENT by itself may or may not be enough (you'd need to test).
Flags: needinfo?(gijskruitbosch+bugs)
Updated•7 years ago
|
Assignee | ||
Updated•7 years ago
|
Summary: Unprivileged about:newtab: ensure about:newtab document runs with null principal (or similar) → Make Activity Stream unprivileged: ensure about:newtab document runs with null principal (or similar)
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → usarracini
Assignee | ||
Comment 13•7 years ago
|
||
So after setting the flag URI_SAFE_FOR_UNTRUSTED_CONTENT for about:newtab, the test: browser/components/originattributes/test/browser/browser_firstPartyIsolation_aboutPages.js fails on test_aboutURL for not recognizing that about:newtab has a firstPartyDomain set. I'm not sure if we want to downgrade the principal for about:newtab to null, since it may need special privileges for UI tour and stuff. Really what we want is just to get the firstPartyDomain bit sorted out. Ollie, would you happen to know what needs to be done to get about:newtab working in that context?
Flags: needinfo?(bugs)
Comment 14•7 years ago
|
||
Why would about:newtab then not use null principal? What principal would it have with URI_SAFE_FOR_UNTRUSTED_CONTENT?
(In reply to Ursula Sarracini (:ursula) from comment #13) Hi Ursula, Is your patch just adding URI_SAFE_FOR_UNTRUSTED_CONTENT for newtab in http://searchfox.org/mozilla-central/rev/8a61c71153a79cda2e1ae7d477564347c607cc5f/browser/components/about/AboutRedirector.cpp#93 If so, I can see the error GECKO(26614) | [Child 26769] WARNING: Not same origin error!: file /home/allstars/src/gecko-dev/dom/base/nsJSEnvironment.cpp, line 658 GECKO(26614) | JavaScript error: resource://activity-stream/data/content/activity-stream.bundle.js, line 555: TypeError: this.sendAsyncMessage is not a function 101 INFO Console message: [JavaScript Warning: "Property contained reference to invalid variable. Error in parsing value for ‘height’. Falling back to ‘initial’." {file: "chrome://browser/skin/browser.css" line: 807 column: 25879 source: " var(--toolbarbutton-height)"}] Can you bypass about:newtab in the if statment in http://searchfox.org/mozilla-central/source/browser/components/originattributes/test/browser/browser_firstPartyIsolation_aboutPages.js#157 And file another bug for this? I'll look into it but might wait until next week. Thanks
Assignee | ||
Comment 16•7 years ago
|
||
(In reply to Yoshi Huang [:allstars.chh] from comment #15) > (In reply to Ursula Sarracini (:ursula) from comment #13) > Hi Ursula, > Is your patch just adding URI_SAFE_FOR_UNTRUSTED_CONTENT for newtab in > http://searchfox.org/mozilla-central/rev/ > 8a61c71153a79cda2e1ae7d477564347c607cc5f/browser/components/about/ > AboutRedirector.cpp#93 Yes, but it would go here instead: http://searchfox.org/mozilla-central/rev/8a61c71153a79cda2e1ae7d477564347c607cc5f/browser/components/about/AboutRedirector.cpp#226-227, just because we want to make sure that we're only setting that flag when activity-stream is enabled, and not for the old about:newtab > If so, I can see the error > GECKO(26614) | [Child 26769] WARNING: Not same origin error!: file > /home/allstars/src/gecko-dev/dom/base/nsJSEnvironment.cpp, line 658 > GECKO(26614) | JavaScript error: > resource://activity-stream/data/content/activity-stream.bundle.js, line 555: > TypeError: this.sendAsyncMessage is not a function > 101 INFO Console message: [JavaScript Warning: "Property contained reference > to invalid variable. Error in parsing value for ‘height’. Falling back to > ‘initial’." {file: "chrome://browser/skin/browser.css" line: 807 column: > 25879 source: " var(--toolbarbutton-height)"}] The error I get from applying the patch is: 69 INFO loading page about:newtab, origin is about:newtab 70 INFO principal [xpconnect wrapped (nsISupports, nsIPrincipal, nsISerializable)] Buffered messages finished 71 INFO TEST-UNEXPECTED-FAIL | browser/components/originattributes/test/browser/browser_firstPartyIsolation_aboutPages.js | The about page should have firstPartyDomain set - "" == "about.ef2a7dd5-93bc-417f-a698-142c3116864f.mozilla" - > Can you bypass about:newtab in the if statment in > http://searchfox.org/mozilla-central/source/browser/components/ > originattributes/test/browser/browser_firstPartyIsolation_aboutPages.js#157 > And file another bug for this? I'll look into it but might wait until next > week. > > Thanks Yeah, I can bypass about:newtab in the test and then file a follow up bug, but I'd like to know a little more about what the correct principal for about:newtab should be. So I have a couple of question that maybe Ollie or Yoshi could shed some light on? 1. From my understanding, based on the comment here: https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/about/nsIAboutModule.idl#26-32, setting URI_SAFE_FOR_UNTRUSTED_CONTENT forces the principal to be null, but when I access about:newtab's principal (by doing something like gBrowser.selectedBrowser.contentPrincipal) it doesn't give any indication that it actually did set the principal to null, vs for example about:blank which has a ' isNullPrincipal: true' attribute if I do the same. Is that understanding correct? 2. If about:newtab needs to be able to let UI tour access parts of the browser code like opening drop downs for you, is it still true that that will work with a null principal for about:newtab? 3. Does giving about:newtab a null principal fix the browser_firstPartyIsolation_aboutPages.js failure I mentioned above, or does there need to be more work done around about:newtab's firstPartyDomain also?
Flags: needinfo?(allstars.chh)
Reporter | ||
Comment 17•7 years ago
|
||
(In reply to Ursula Sarracini (:ursula) from comment #16) > 1. From my understanding, based on the comment here: > https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/about/ > nsIAboutModule.idl#26-32, setting URI_SAFE_FOR_UNTRUSTED_CONTENT forces the > principal to be null, No, the comment states: If [the flag is set], the about: protocol handler will enforce that * the principal of channels created for it be based on their * originalURI or URI (depending on the channel flags) So instead, it'll be based on the URI at which the activity stream content resides. You'd have to explicitly force it to be null if that was what you wanted to do. > but when I access about:newtab's principal (by doing > something like gBrowser.selectedBrowser.contentPrincipal) it doesn't give > any indication that it actually did set the principal to null, vs for > example about:blank which has a ' isNullPrincipal: true' attribute if I do > the same. Is that understanding correct? I think so. Its `baseDomain` and `isCodebasePrincipal` flags might help narrow down what principal you're getting, exactly. Assuming it's a codebase principal for the resource: or chrome: URI, rather than something with isSystemPrincipal:true, it's working correctly. This distinction was why I suggested in comment #12 that just the flag might not be enough. > 2. If about:newtab needs to be able to let UI tour access parts of the > browser code like opening drop downs for you, is it still true that that > will work with a null principal for about:newtab? I don't think the permissions database will work for a null principal, because when creating a null principal (Services.scriptSecurityManager.createNullPrincipal) you can't control the exact principal you get (you get a random, 'opaque' origin), so you can't pre-load permissions for it the way we normally do for UITour. > 3. Does giving about:newtab a null principal fix the > browser_firstPartyIsolation_aboutPages.js failure I mentioned above, or does > there need to be more work done around about:newtab's firstPartyDomain also? I don't know the answer to this because I'm not familiar with the test.
Assignee | ||
Comment 18•7 years ago
|
||
So after chatting with bholly and bz about the next steps for this, here's what we're going to do: 1. Re-purpose this bug to be just set activity stream to be URI_SAFE_FOR_UNTRUSTED_CONTENT only 2. Set activity stream to be URI_SAFE_FOR_UNTRUSTED_CONTENT and fix browser_firstPartyIsolation_aboutPages.js to exempt about:newtab 3. File a follow up bug for browser_firstPartyIsolation_aboutPages.js to sort out the firstPartyDomain stuff 4. File a follow up bug to set activity stream's principal to null properly
Flags: needinfo?(bugs)
Flags: needinfo?(allstars.chh)
Assignee | ||
Updated•7 years ago
|
Summary: Make Activity Stream unprivileged: ensure about:newtab document runs with null principal (or similar) → Give Activity Stream URI_SAFE_FOR_UNTRUSTED_CONTENT
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8890867 -
Flags: review?(ckerschb)
Comment 20•7 years ago
|
||
mozreview-review |
Comment on attachment 8890867 [details] Bug 1021667 - Give Activity Stream URI_SAFE_FOR_UNTRUSTED_CONTENT https://reviewboard.mozilla.org/r/162086/#review167718 That sounds reasonable to me. Please make sure the necessary follow up bugs (linked to this one) are filed before landing. Please note that I am not a browser/ peer. Maybe have gijs rubberstamp it before landing. thanks!
Attachment #8890867 -
Flags: review?(ckerschb) → review+
Assignee | ||
Updated•7 years ago
|
Attachment #8890867 -
Flags: superreview?(gijskruitbosch+bugs)
Reporter | ||
Comment 21•7 years ago
|
||
Comment on attachment 8890867 [details] Bug 1021667 - Give Activity Stream URI_SAFE_FOR_UNTRUSTED_CONTENT We don't really use superreview anymore, and I'm not a superreviewer, but this change looks good to me. :-) I'll see if I can get https://www.mozilla.org/en-US/about/governance/policies/reviewers/ updated somehow.
Attachment #8890867 -
Flags: superreview?(gijskruitbosch+bugs) → superreview+
Reporter | ||
Updated•7 years ago
|
Attachment #8890867 -
Flags: review?(allstars.chh) → review?(gijskruitbosch+bugs)
Reporter | ||
Comment 22•7 years ago
|
||
mozreview-review |
Comment on attachment 8890867 [details] Bug 1021667 - Give Activity Stream URI_SAFE_FOR_UNTRUSTED_CONTENT https://reviewboard.mozilla.org/r/162086/#review167848
Attachment #8890867 -
Flags: review?(gijskruitbosch+bugs) → review+
Reporter | ||
Comment 23•7 years ago
|
||
Comment on attachment 8890867 [details] Bug 1021667 - Give Activity Stream URI_SAFE_FOR_UNTRUSTED_CONTENT Moving this to mozreview and such so this can just land. :-)
Attachment #8890867 -
Flags: superreview+
Comment 24•7 years ago
|
||
Pushed by usarracini@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/6726399b5bbb Give Activity Stream URI_SAFE_FOR_UNTRUSTED_CONTENT r=ckerschb,Gijs
Comment 25•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6726399b5bbb
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
You need to log in
before you can comment on or make changes to this bug.
Description
•