776167 - can't go back to the new tab page

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Jesper Hansen]

724239 seems to have unintentionally blocked the back button from being useful for quickly navigate ones about:newtab items.

Example:
1. Open about:newtab
2. Click item 7 (forum, no new threads)
3. Use back button on mouse or keyboard to get back to about:newtab, 4 button on mouse or backspace or alt+left (depending on setup)
4. Click item 8 (forum, no new threads)
5. well, repeat.

This now instead becomes:
1. Open about:newtab
2. Click item 7 (forum, no new threads)
3. Close tab, requires moving mouse or two finger combo (ctrl+w)
4. Open new tab about:newtab, requires moving mouse or two finger combo (ctrl+t)
5. Click item 8 (forum, no new threads)

Bug 724239 introduced this little fear, let's add some sanity back.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

There was nothing unintentional about it - the entire goal of bug 724239 was disabling the back button from showing up for going back to about:newtab :)

There are pros and cons to both approaches. For the moment, it's not possible to enable the back button for this case securely.

Comment 2

[Jesper Hansen]

Hope someone will come up with something. 
Being able to left click an item and then using back button in case there was nothing new on that page.

The speeddial is a little less speedier.

Comment 4

[troop]

Gavin can you give an example on how this is a security issue, it escapes me.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

The details are in bug 769108, which isn't public yet, but should be soon once the Firefox 15 security advisories get posted.

Comment 11

[Jennifer Morrow [:Boriss] (UX)]

Just want to clarify that this is indeed a UX bug that should be fixed while maintaining security.  As Limi says in Bug 724239#c8, users should be able to go back to New Tab once they've left it.  They've effectively gone from one page to another by clicking a New Tab thumbnail, and to subvert expectations by not allowing them to go back creates a poor experience.  This becomes increasingly true as more functionality comes to New Tab that users may want to return to as they would another page.

Based on Gavin's comment on Bug 724239#c13, the security problem occurs if we're allowing a web site to trigger the loading of a chrome-privileged page.  Can we fix this by making about:newtab not be privileged, as Gavin suggests in Bug 724239#c13?  Or, are there other regressions that would occur from doing so, such that we should solve this problem in another way?

Comment 12

[Marco Bonardo [:mak]]

(In reply to Jennifer Morrow [:Boriss] (Firefox UX) from comment #11)
> Can we fix this by making about:newtab not be privileged, as Gavin
> suggests in Bug 724239#c13?

That's basically the plan, see bug 776167
IIRC Tim said it's feasible, just a lot of work since part of the code must be rewritten.

Comment 13

[Marco Bonardo [:mak]]

ehr sorry, bug 776477.

Comment 14

[Johan C]

Attachment: Patch v1

Attaching a patch with the hope of seeing this fixed. It might not be the best solution, but if you think I'm on the right track I'd be happy continue working on it.

I'm aware that bug 776477 has to be fixed before this can land, but this bug is really a must-fix in my opinion. There are plenty of use-cases where going back to the new tab is easier/faster/better/preferable, and not one of the other major browsers or add-on have this limitation.

The patch:
  The only problem I've found with this patch is that the changes do not take effect in already opened "about:newtab"-tabs.
  STR the above bug:
    1. Click the "Hide the new tab page"-button.
    2. Enter an address and hit the enter-key.
  Expected:
    "about:newtab" remains in the session history.
  Actual:
    "about:newtab" should not be added to the session history.
  
  STR #2 (same bug):
    1. Click the "Show the new tab page"-button.
    2. Enter an address and hit the enter-key.
  Expected:
    "about:newtab" is not added to the session history.
  Actual:
    "about:newtab" should remain in the session history.
  
  Could this be solved with an observing the preference and reloading all "about:newtab"-tabs?

I'm guessing a patch for this bug would also need a test, is that correct?

Comment 16

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 774702 [details] [diff] [review]
Patch v1

Why check the newtabpage.enabled pref? I would just unconditionally allow about:newtab.

In any case it's a bit premature to be worrying about this without a fix for bug 776477. Johan, would you be willing to work with Tim to think up some solutions there? IIRC he had plans to get rid of XUL in about:newtab anyways.

Comment 17

[Johan C]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #16)
> Comment on attachment 774702 [details] [diff] [review]
> Patch v1
> 
> Why check the newtabpage.enabled pref? I would just unconditionally allow
> about:newtab.

Fair enough.

> In any case it's a bit premature to be worrying about this without a fix for
> bug 776477. Johan, would you be willing to work with Tim to think up some
> solutions there? IIRC he had plans to get rid of XUL in about:newtab anyways.

I'd be happy to give it a go.
I've already started by using Tim's patch from bug 843853 (which converts the new tag page to xhtml and the CSS3 flexbox model), but I've run into some odd graphical glitches involving mouseover of the thumbs. I'm investigating this at the moment.

===

If I may ask, why does the new tab page have to be unprivileged? Why is it not secure "go back" to a XUL page from an external address? 

Are the following use cases not security issues as well?
  Use case #1:
  1. Go to an external webpage.
  2. Enter "about:addons" or "about:newtab" into the address bar and go.

  Use case #2 (add-ons):
  1. Install an add-on (i.e. Super Start or Speed Dial)
  2. Add a "dial".
  3. Click the dial.
  4. "Go back".

Thanks Gavin :)

Comment 18

[:Gavin Sharp [email: [e-mail]]]

In general, if a web page can trigger the loading of a chrome-privileged page, that's a bad thing. The add-ons manager example you list is still somewhat problematic, but the important difference is how frequently the problematic scenario occurs. Users are much more likely to load the new tab page before loading some other random content website (that's the primary purpose of about:newtab - loading other pages) than they are to navigate to that random content website from about:addons (you can't even do that easily since about:addons hides the location bar).

Comment 19

[Johan C]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #18)
> In general, if a web page can trigger the loading of a chrome-privileged
> page, that's a bad thing. The add-ons manager example you list is still
> somewhat problematic, but the important difference is how frequently the
> problematic scenario occurs. Users are much more likely to load the new tab
> page before loading some other random content website (that's the primary
> purpose of about:newtab - loading other pages) than they are to navigate to
> that random content website from about:addons (you can't even do that easily
> since about:addons hides the location bar).

My point was that a fair number of users use add-ons such as Super Start and Speed Dial, which I'm guessing are chrome-privileged. These add-ons work the same way about:newtab does, they holds links to external webpages which can trigger loading of the chrome-privileged add-on page.

So isn't this a bigger security issue, something that should be fixed further down?

I'm using needinfo? because I'm not sure if you can read questions when I'm replying, hope you don't mind. :)

Comment 20

[:Gavin Sharp [email: [e-mail]]]

(No need to needinfo on every comment, I read my bugmail.)

If those add-ons cause similar issues they should be fixed, sure. I'm not sure what kind of "fix it further down" solutions you're envisioning - bug 286651 or bug 652736 are somewhat complicated architectural changes that aren't viable short term solutions. It's possible e10s may force us into a better setup too.

Comment 21

[Jesse Ruderman]

See also bug 664556, "Don't allow navigation from chrome to content".

Comment 28

[donrhummy]

It was mentioned that this can't be allowed for security reasons, but then why can you do this on Firefox Android?

Comment 32

[Ed Lee :Mardak]

Potentially this can be revisited when Activity Stream about:newtab is enabled as it runs in the content process.

Comment 33

[bbell]

The security issues no longer exist on the Activity Stream Version of New tab that's shipping in 57 (Photon). The Recommended content feature of the New Newtab requires the ability for users to return to the Newtab via the Back button after they've tried out an article so they can get to another recommendation. 

Gijs do you know who can help make this change?

Comment 34

[:Gijs (he/him)]

Attachment: Bug 776167 - allow going back to about:newtab,

Review commit: https://reviewboard.mozilla.org/r/167106/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/167106/

Comment 35

[:Gijs (he/him)]

Freddy, let me know if you're not comfortable reviewing the docshell change. I sanity-checked the test, things seem to work correctly both with AS on and off.

Comment 36

[Frederik Braun [:freddy]]

Comment on attachment 8895776 [details]
Bug 776167 - allow going back to about:newtab,

Looks good overall, but I need to cancel this review, I'm not a peer.
Please find someone else (ckerschb or smaug?) for a review.

Comment 37

[Ursula Sarracini (:ursula)]

Comment on attachment 8895776 [details]
Bug 776167 - allow going back to about:newtab,

https://reviewboard.mozilla.org/r/167106/#review172290

Looks good to me, thanks Gijs!

Comment 38

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8895776 [details]
Bug 776167 - allow going back to about:newtab,

https://reviewboard.mozilla.org/r/167106/#review174122

Looks good to me, but I am not a docshell peer, smaug is the right person for this review.

::: docshell/base/nsDocShell.cpp:12396
(Diff revision 1)
>      }
> +    // We only want to add about:newtab if it's not privileged:
> +    if (buf.EqualsLiteral("newtab")) {
> +      if (!aChannel) {
> +        return false;
> +      }

NS_ENSURE_TRUE(aChannel, false);

::: docshell/base/nsDocShell.cpp:12397
(Diff revision 1)
> +    // We only want to add about:newtab if it's not privileged:
> +    if (buf.EqualsLiteral("newtab")) {
> +      if (!aChannel) {
> +        return false;
> +      }
> +      nsCOMPtr<nsIPrincipal> principal;

nit: replace that codeblock with:

nsCOMPtr<nsIPrincipal> resultPrincipal;
rv = nsContentUtils::GetSecurityManager()-> GetChannelResultPrincipal(channel,

Comment 39

[:Gijs (he/him)]

Comment on attachment 8895776 [details]
Bug 776167 - allow going back to about:newtab,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/167106/diff/1-2/

Comment 40

[Olli Pettay [:smaug][[e-mail]]]

Want to explain why we have the system principal check?

Comment 41

[:Gijs (he/him)]

(In reply to Olli Pettay [:smaug] from comment #40)
> Want to explain why we have the system principal check?

Because the old about:newtab (if/when Activity Stream is turned off, which is just a pref flip in about:config) uses system principal, and web pages can use history.back/go(-1) to navigate a page to something that has system principal, and that is/was considered Bad. So I'm making it possible to navigate back if and only if about:newtab doesn't have system principal. Does that make sense?

Comment 42

[Olli Pettay [:smaug][[e-mail]]]

yes, thanks. Looking.

Comment 43

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8895776 [details]
Bug 776167 - allow going back to about:newtab,

https://reviewboard.mozilla.org/r/167106/#review174212

Tests modified a bit, r+

::: browser/base/content/test/general/browser_bug724239.js:22
(Diff revision 2)
> +    BrowserTestUtils.loadURI(browser, "about:newtab");
> +    await BrowserTestUtils.browserLoaded(browser);
> +
> +    BrowserTestUtils.loadURI(browser, "http://example.com");
> +    await BrowserTestUtils.browserLoaded(browser);
> +    let usingActivityStream = Services.prefs.getBoolPref("browser.newtabpage.activity-stream.enabled", false);

It would be nice if we explicitly tested both setups, with and without activity stream enabled or disabled.

Could you perhaps make the change?
First push pref disabled and test and then push pref and test pref enabled.

Comment 44

[:Gijs (he/him)]

Comment on attachment 8895776 [details]
Bug 776167 - allow going back to about:newtab,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/167106/diff/2-3/

Comment 45

[:Gijs (he/him)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=11c398677b3d

Comment 46

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/647c08380cd3
allow going back to about:newtab, r=smaug,ursula

Comment 47

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

https://hg.mozilla.org/mozilla-central/rev/647c08380cd3