1329457 - Unable to add about:* pages as a about:newtab tile

Status: VERIFIED FIXED

(Firefox :: New Tab Page, defect)

Description

[Fanolian]

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:53.0) Gecko/20100101 Firefox/53.0
Build ID: 20170107030205

Steps to reproduce:

1. In a new profile in Nightly, open about:newtab.
2. Try to drag about:crashes from bookmark[1], or any about: pages, into about:newtab as a new tile.

[1] Bookmarks > Firefox Nightly Resources > All your crashes


Actual results:

I cannot add about:crashes as a new tile. about:crashes opens in the tab instead.


Expected results:

A new tile linking to about:crashes is created.


I mark this bug as confidential because it should be regressed by bug 1309310, a security bug which I don't have the right to access, according to Mozregression.
Is this an intended change?

Comment 1

[Fanolian]

From Mozregression:

Last good Nightly: 2016-11-11
First bad Nightly: 2016-11-12
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=d38d06f85ef59c5dbb5d4a1a8d895957a78714de&tochange=fc104971a4db41e38808e6412bc32e1900172f14

Further bisections:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=d728a7fdade20d7fbfd8c18e46974abd7fd150e6&tochange=fb5ae665310e4f60464ccd43b2626fdfc2087a67

Suspect: bug 1309310

Comment 2

[Fanolian]

(I cannot add bug 1309310 into "Tracking" > "Blocks" field since I don't have the permission to access)

If I have added an about:crashes tile prior fixing bug 1309310 (2016-11-11 build or earlier) and open that profile in recent Nightly, the tile is still present but I cannot open the link. Browser Console shows such messages when I click the tile:

Security Error: Content at about:newtab may not load or link to about:crashes.

NS_ERROR_DOM_BAD_URI: Component returned failure code: 0x805303f4 [nsIWebNavigation.loadURIWithOptions]  browser.js:876
	_loadURIWithFlags chrome://browser/content/browser.js:876:7
	loadURIWithFlags chrome://browser/content/tabbrowser.xml:7568:13
	openLinkIn chrome://browser/content/utilityOverlay.js:380:5
	onE10sAboutNewTab chrome://browser/content/browser.js:3067:7
	handleEvent chrome://browser/content/browser.js:2830:7

Comment 3

[:Gijs (he/him)]

Yeah, this wasn't an intentional regression. It might be tricky to fix correctly, though. I'll keep needinfo and think about this more during the week.

Comment 4

[:Gijs (he/him)]

(FWIW, I don't think this is severe enough to block 51, and we're weeks away from its release, so I think this is effectively going to be WONTFIX for 51.)

Comment 5

[:Gijs (he/him)]

(In reply to Fanolian from comment #2)
> (I cannot add bug 1309310 into "Tracking" > "Blocks" field since I don't
> have the permission to access)
> 
> If I have added an about:crashes tile prior fixing bug 1309310 (2016-11-11
> build or earlier) and open that profile in recent Nightly, the tile is still
> present but I cannot open the link. Browser Console shows such messages when
> I click the tile:
> 
> Security Error: Content at about:newtab may not load or link to
> about:crashes.
> 
> NS_ERROR_DOM_BAD_URI: Component returned failure code: 0x805303f4
> [nsIWebNavigation.loadURIWithOptions]  browser.js:876
> 	_loadURIWithFlags chrome://browser/content/browser.js:876:7
> 	loadURIWithFlags chrome://browser/content/tabbrowser.xml:7568:13
> 	openLinkIn chrome://browser/content/utilityOverlay.js:380:5
> 	onE10sAboutNewTab chrome://browser/content/browser.js:3067:7
> 	handleEvent chrome://browser/content/browser.js:2830:7

Is the regression window for this the same?

I'm asking because about:newtab is (currently) privileged, and so loading another page should always work. This looks like an instance of bug 1329032 to me.

I can fix the addition of the tile pretty easily, but of course if it then doesn't work that doesn't actually gain us anything...

Comment 6

[:Gijs (he/him)]

(In reply to :Gijs from comment #5)
> (In reply to Fanolian from comment #2)
> > (I cannot add bug 1309310 into "Tracking" > "Blocks" field since I don't
> > have the permission to access)
> > 
> > If I have added an about:crashes tile prior fixing bug 1309310 (2016-11-11
> > build or earlier) and open that profile in recent Nightly, the tile is still
> > present but I cannot open the link. Browser Console shows such messages when
> > I click the tile:
> > 
> > Security Error: Content at about:newtab may not load or link to
> > about:crashes.
> > 
> > NS_ERROR_DOM_BAD_URI: Component returned failure code: 0x805303f4
> > [nsIWebNavigation.loadURIWithOptions]  browser.js:876
> > 	_loadURIWithFlags chrome://browser/content/browser.js:876:7
> > 	loadURIWithFlags chrome://browser/content/tabbrowser.xml:7568:13
> > 	openLinkIn chrome://browser/content/utilityOverlay.js:380:5
> > 	onE10sAboutNewTab chrome://browser/content/browser.js:3067:7
> > 	handleEvent chrome://browser/content/browser.js:2830:7
> 
> Is the regression window for this the same?

So the answer is no. Fairly sure that that bit is a "dupe" of 1329032 then. See also the last few comments there.

I'll fix the addition of the tiles in here as that is relatively straightforward.

Comment 7

[Christoph Kerschbaumer [:ckerschb}]

(In reply to :Gijs from comment #6)
> So the answer is no. Fairly sure that that bit is a "dupe" of 1329032 then.
> See also the last few comments there.

This is almost certainly a dup of Bug 1329032. Question is, does Bug 1329032 also need to be security sensitive?

Comment 8

[:Gijs (he/him)]

Attachment: Patch v0.1

bz, picking you because you suggested the checkLoadURI call in bug 765628. AIUI we're mostly concerned with inheriting the system principal and everything else is fair game, but it feels like I shouldn't use the system principal lightly here, so I'd appreciate a sanity check. :-)

It's kind of odd, actually, because as I'm attaching this, I'm realizing that file: links already don't pass these tests, and didn't before 1309310. But bug 765628 is explicitly concerned about allowing this.

Restoring this possibility (assuming it wasn't broken ever since 765628, which I haven't checked) makes sense given that if you do the following:

1. open fx with a new profile
2. navigate to a file:// folder
3. bookmark
4. refresh a couple of times
5. open new tab

it becomes the first tile (hi, frecency).

So clearly we're OK with linking to these things, so we should also allow adding those links. 

Or so I thought, until I noticed this code:

https://dxr.mozilla.org/mozilla-central/source/browser/components/newtab/PlacesProvider.jsm#187

and then wondered why that worked. So it turns out if you restart the browser and then open about:newtab, the file:/// link is gone. If you then visit it once, it comes back, because the frecency and title change notifications don't do the same checkLoadURI.

This patch fixes that inconsistency, too.

Note that about:newtab still has the system principal, but the checks happen from a JSM so I can't directly reference the document's contentPrincipal.

Comment 9

[:Gijs (he/him)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #7)
> (In reply to :Gijs from comment #6)
> > So the answer is no. Fairly sure that that bit is a "dupe" of 1329032 then.
> > See also the last few comments there.
> 
> This is almost certainly a dup of Bug 1329032. Question is, does Bug 1329032
> also need to be security sensitive?

No. The only reason this is sec-sensitive is the first part of the problem (being able to drag about: URLs into about:newtab) being a regression from bug 1309310, and *that* bug is sec-sensitive.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8825085 [details] [diff] [review]
Patch v0.1

r=me as far as it goes, but:

1) It might be good to have someone familiar with the Places code review those changes and make sure all consumers are OK with them.

2) The two _doCheckLoadURI functions should probably have comments pointing out that their behavior should match, if they can't be shared.

3) An explicit comment about how the idea is to filter out principal-inheriting URIs, not anything else, is probably a good thing to have.

Comment 11

[:Gijs (he/him)]

Attachment: Patch v0.2

Dão, can you doublecheck what I've done here given Boris' earlier comment?

Given that this wants uplift and it's very late in beta, I'm keeping the PlacesProvider.LinkChecker stuff for backwards compat for add-ons etc. (from looking at m-c, I didn't see any other consumers). We need to take a broom through some of this in a different bug.

Comment 12

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ba8ba11e2bc7080a420192084aaebbfe5d757a67

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ba8ba11e2bc7

Comment 14

[:Gijs (he/him)]

Comment on attachment 8825362 [details] [diff] [review]
Patch v0.2

Approval Request Comment
[Feature/Bug causing the regression]: bug 1309310
[User impact if declined]: we sporadically don't put about: pages on the new tab page, and don't let the user do it manually
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: not yet
[Needs manual test from QE? If yes, steps to reproduce]: 
1. open about:crashes
2. bookmark it
3. open a new tab
4. drag the bookmark to the new tab page

AR: no new pinned tile is created
Expected: a pinned tile is created

Note that on Nightly, because of bug 1329032, clicking the tile won't work there, but it will work on aurora/beta.

[List of other uplifts needed for the feature/fix]: n/a
[Is the change risky?]: no
[Why is the change risky/not risky?]: the code change is relatively simple and well-understood. The patch is slightly larger because it deduplicates 2 identical implementations of the same object ("LinkChecker") and updates a test.
[String changes made/needed]: no.

Comment 15

[Julien Cristau [:jcristau]]

Comment on attachment 8825362 [details] [diff] [review]
Patch v0.2

let about:newtab include other about:* tiles, aurora52+

FWIW I verified that this didn't work in yesterday's nightly, and worked in today's (but as expected clicking the tile then doesn't do anything).

Comment 16

[Fanolian]

I can verify that the issue is fixed in 2017-01-11 build. Thanks.

Comment 17

[Gerry Chang [:gchang]]

Comment on attachment 8825362 [details] [diff] [review]
Patch v0.2

Fix a regression. Beta51+. Should be in 51 beta 14.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing for uplift.

Comment 19

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/0688f0bd4f534fd3f6d6cae3006c21966fd340bc
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/8a89dd47e7847c27ffe04b12832f0cdba7262b56

Comment 20

[Andrei Vaida [:avaida]]

Flagging this for verification, str available in Comment 14.

Also updating status for 53 based on Comment 16.

Comment 21

[Bogdan Maris, Desktop Test Engineering]

I was able to reproduce on an old version of Nightly from 2017-01-07, and I verified that this issue is fixed on latest builds (Fx 51.0 RC build 2, latest Developer Edition 52.0a2 and latest Nightly 53.0a1) across platforms (Windows 10 64bit, Ubuntu 16.04 32bit and macOS 10.12.2).

Comment 22

[Fanolian]

(In reply to :Gijs from comment #6)
> (In reply to :Gijs from comment #5)
> > (In reply to Fanolian from comment #2)
> > > (I cannot add bug 1309310 into "Tracking" > "Blocks" field since I don't
> > > have the permission to access)
> > > 
> > > If I have added an about:crashes tile prior fixing bug 1309310 (2016-11-11
> > > build or earlier) and open that profile in recent Nightly, the tile is still
> > > present but I cannot open the link. Browser Console shows such messages when
> > > I click the tile:
> > > 
> > > Security Error: Content at about:newtab may not load or link to
> > > about:crashes.
> > > 
> > > NS_ERROR_DOM_BAD_URI: Component returned failure code: 0x805303f4
> > > [nsIWebNavigation.loadURIWithOptions]  browser.js:876
> > > 	_loadURIWithFlags chrome://browser/content/browser.js:876:7
> > > 	loadURIWithFlags chrome://browser/content/tabbrowser.xml:7568:13
> > > 	openLinkIn chrome://browser/content/utilityOverlay.js:380:5
> > > 	onE10sAboutNewTab chrome://browser/content/browser.js:3067:7
> > > 	handleEvent chrome://browser/content/browser.js:2830:7
> > 
> > Is the regression window for this the same?
> 
> So the answer is no. Fairly sure that that bit is a "dupe" of 1329032 then.
> See also the last few comments there.
> 
> I'll fix the addition of the tiles in here as that is relatively
> straightforward.

FYI, left-clicking an about:* tile in about:newtab still fails in 2017-01-19 build which contains the fix for bug 1329032.
Should I wait for bug 1331686 or file a new bug? (If so, should that new bug be security-sensitive too?)

Comment 23

[Christoph Kerschbaumer [:ckerschb}]

(In reply to Fanolian from comment #22)
> FYI, left-clicking an about:* tile in about:newtab still fails in 2017-01-19
> build which contains the fix for bug 1329032.
> Should I wait for bug 1331686 or file a new bug? (If so, should that new bug
> be security-sensitive too?)

With left-clicking you mean a regular 'click' to open a link in the current tab? As far as I can tell that should have been fixed together with bug 1329032. Potentially it gets fixed with Bug 1331686, but I doubt it. I already put initial patches up for feedback/review for Bug 1331686 - you could apply them and see if it works, if not, please feel free to file a new bug (please also mark it tracking for 53). I think it doesn't need to be a security sensitive bug though.