Closed Bug 1021667 Opened 6 years ago Closed 3 years ago

Give Activity Stream URI_SAFE_FOR_UNTRUSTED_CONTENT

Categories

(Firefox :: New Tab Page, defect)

defect
Not set
Points:
2

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+
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.
Points: --- → 2
Whiteboard: p=2
Depends on: 1167601
Ursula or Mike, can you comment as to what's required to get this to work?
Flags: needinfo?(ursulasarracini)
Flags: needinfo?(mconley)
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)
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)
Gijs is this a dupe of bug 1272342?
Flags: needinfo?(gijskruitbosch+bugs)
(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)
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)
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)
(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?
Sounds good! Thanks :)
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)
(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)
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: nobody → usarracini
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)
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
(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)
(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.
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)
Summary: Make Activity Stream unprivileged: ensure about:newtab document runs with null principal (or similar) → Give Activity Stream URI_SAFE_FOR_UNTRUSTED_CONTENT
Attachment #8890867 - Flags: review?(ckerschb)
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+
Blocks: 1385306
Blocks: 1385308
Attachment #8890867 - Flags: superreview?(gijskruitbosch+bugs)
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+
Attachment #8890867 - Flags: review?(allstars.chh) → review?(gijskruitbosch+bugs)
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+
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+
Pushed by usarracini@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6726399b5bbb
Give Activity Stream URI_SAFE_FOR_UNTRUSTED_CONTENT r=ckerschb,Gijs
https://hg.mozilla.org/mozilla-central/rev/6726399b5bbb
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
You need to log in before you can comment on or make changes to this bug.