can't go back to the new tab page

RESOLVED FIXED in Firefox 57

Status

()

RESOLVED FIXED
7 years ago
5 months ago

People

(Reporter: jesper, Assigned: Gijs)

Tracking

(Depends on: 1 bug)

Trunk
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(2 attachments)

(Reporter)

Description

7 years ago
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.
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.
(Reporter)

Comment 2

7 years ago
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.
(Reporter)

Updated

7 years ago
Duplicate of this bug: 784444

Comment 4

7 years ago
Gavin can you give an example on how this is a security issue, it escapes me.
The details are in bug 769108, which isn't public yet, but should be soon once the Firefox 15 security advisories get posted.
(Reporter)

Updated

7 years ago
Duplicate of this bug: 787485
Summary: 724239 blocks back button entirely → can't go back to the new tab page

Updated

7 years ago
Duplicate of this bug: 786992

Updated

6 years ago
Duplicate of this bug: 809921

Updated

6 years ago
Duplicate of this bug: 828461

Updated

6 years ago
Duplicate of this bug: 840937
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?
(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.
ehr sorry, bug 776477.

Comment 14

6 years ago
Created attachment 774702 [details] [diff] [review]
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?
Assignee: nobody → johan.charlez
Status: NEW → ASSIGNED
Attachment #774702 - Flags: feedback?(ttaubert)
Attachment #774702 - Flags: feedback?(gavin.sharp)

Updated

6 years ago
Duplicate of this bug: 893697
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.
Attachment #774702 - Flags: feedback?(ttaubert)
Attachment #774702 - Flags: feedback?(gavin.sharp)

Comment 17

6 years ago
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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 :)
Flags: needinfo?(gavin.sharp)
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).
Flags: needinfo?(gavin.sharp)

Comment 19

6 years ago
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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. :)
Flags: needinfo?(gavin.sharp)
(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.
Flags: needinfo?(gavin.sharp)
Whiteboard: John, Colbert

Comment 21

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

Updated

5 years ago
Assignee: johan.charlez → nobody
Status: ASSIGNED → NEW
Duplicate of this bug: 960567

Updated

5 years ago
Duplicate of this bug: 967321

Updated

5 years ago
Duplicate of this bug: 978659
Duplicate of this bug: 1014925

Updated

4 years ago
Depends on: 1084471
Duplicate of this bug: 1087466
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1211382

Comment 28

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

Updated

3 years ago
Duplicate of this bug: 1178781

Updated

3 years ago
Duplicate of this bug: 1256179
Duplicate of this bug: 1332110

Comment 32

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

Comment 33

2 years ago
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?
Flags: needinfo?(gijskruitbosch+bugs)
Comment hidden (mozreview-request)
(Assignee)

Comment 35

2 years ago
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.
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Component: Tabbed Browser → Document Navigation
Flags: needinfo?(gijskruitbosch+bugs)
Product: Firefox → Core
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.
Attachment #8895776 - Flags: review?(fbraun) → feedback+
(Assignee)

Updated

2 years ago
Attachment #8895776 - Flags: review?(ckerschb)

Comment 37

2 years ago
mozreview-review
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!
Attachment #8895776 - Flags: review?(usarracini) → review+

Comment 38

2 years ago
mozreview-review
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,
Attachment #8895776 - Flags: review?(ckerschb)
Comment hidden (mozreview-request)
Want to explain why we have the system principal check?
Flags: needinfo?(gijskruitbosch+bugs)
(Assignee)

Comment 41

2 years ago
(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?
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(bugs)
yes, thanks. Looking.
Flags: needinfo?(bugs)

Comment 43

2 years ago
mozreview-review
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.
Attachment #8895776 - Flags: review?(bugs) → review+
Comment hidden (mozreview-request)

Comment 46

2 years ago
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/647c08380cd3
allow going back to about:newtab, r=smaug,ursula

Comment 47

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/647c08380cd3
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Depends on: 1415136

Updated

a year ago
See Also: → bug 1423450

Updated

a year ago
See Also: → bug 1419340
Depends on: 1478348
You need to log in before you can comment on or make changes to this bug.