Closed Bug 1118285 Opened 10 years ago Closed 9 years ago

The browser.newtab.url preference is abused and should be removed

Categories

(Firefox :: New Tab Page, defect, P2)

Product:
Firefox
Component:
New Tab Page
Type:
defect
Points:
5

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 41
Tracking Flags:
Tracking Status
firefox41 --- fixed

People

(Reporter: florian, Assigned: nhnt11)

References

Details

(Keywords: addon-compat, Whiteboard: [hijacking][fxsearch])

Attachments

(3 files, 9 obsolete files)

Screenshot of abused new tab page
30.27 KB, image/png
Details
Patch v5
19.26 KB, patch
florian
: review+
Details | Diff | Splinter Review
Talos Patch v1.1
8.49 KB, patch
nhnt11
: review+
Details | Diff | Splinter Review 
The browser.newtab.url preference has no exposed UI, is not really supported, and is abused by search hijackers. We should remove it and encourage people using a non-default new tab page to install an add-on instead.
Flags: firefox-backlog+
While I'm always a fan of less footguns, browser.newtab.url was introduced to have any API at all for add-ons to configure the new tab page. Before that every single add-on I found overrode BrowserOpenTab() by copying the current source code and replacing the URL in it. And I think there might be a few people wanting to have their own custom new tab page, without having to write an add-on only for that - given they know how to do that.
We can provide an extension hook that isn't a pref relatively easily, right?
Attached patch potential patch (obsolete) — — Splinter Review 
This is what I'm thinking. This moves things away from pref/xpcom land, but still lets addons override. We'll probably want to additional expose an xpcom way to retrieve the current URL (but not set it) to replace the nsDocShell use of the pref.
Flags: qe-verify-
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #2)
> We can provide an extension hook that isn't a pref relatively easily, right?

Would suggesting that add-ons can override the chrome URL "chrome://browser/content/newtab/newTab.xul" be enough?
That's a much less friendly hook than what my patch proposes, and makes it difficult to use an arbitrary web page as your new tab page.
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #2)
> We can provide an extension hook that isn't a pref relatively easily, right?

The proposed patch looks fine to me but are we assuming that creating an add-on is too much hassle for malicious parties to hijack newtab pages? It makes it a little harder than adding a line to the prefs file of course but it's still far from being difficult. It does also prevent people from simply changing their newtab pages without an add-on to do that - which I think is a fair tradeoff but doesn't hurt to explicitly mention.
(In reply to Tim Taubert [:ttaubert] from comment #6)
> (In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #2)
> > We can provide an extension hook that isn't a pref relatively easily, right?
> 
> The proposed patch looks fine to me but are we assuming that creating an
> add-on is too much hassle for malicious parties to hijack newtab pages? It
> makes it a little harder than adding a line to the prefs file of course but
> it's still far from being difficult.

I think the assumption is that malicious add-ons can be blocklisted, whereas random preference changes can't.
(In reply to Florian Quèze [:florian] [:flo] from comment #7)
> I think the assumption is that malicious add-ons can be blocklisted, whereas
> random preference changes can't.

True. I don't think blocklisting add-ons has turned out to be a very effective solution to those kind of problems in the past but I'm not opposing the general direction here.

My dream solution would probably be to have an API like the proposed one coupled with defaulting to only accept signed add-ons from AMO.
Multi-pronged approach, this is just one prong :) It moves us in the right direction and adds a few hurdles to jump through in the interim.
Points: --- → 5
So, signed addons are coming :)  We observe newtab override being a problem in the wild.  Time to revisit?
Hello, I'm going to be working on this.

So I'm looking at adding a new function to nsIWebBrowserChrome3.idl and nsIXULBrowserWindow.idl similar to shouldLoadURI (https://mxr.mozilla.org/mozilla-central/source/embedding/browser/nsIWebBrowserChrome3.idl#47, https://mxr.mozilla.org/mozilla-central/source/xpfe/appshell/nsIXULBrowserWindow.idl#62), and have the browser.js implementation of XULBrowserWindow do the work.

Should this function be a delegation of ShouldAddToSessionHistory, in the sense that the XULBrowserWindow implementation would return a boolean (which will be used as-is by nsDocShell), or should it be specific to getting the newtab URL (i.e. return a string)?

Or are you expecting a totally different approach? :)
Assignee: nobody → nhnt11
Flags: needinfo?(gavin.sharp)
Attached patch Continue Gavin's patch (obsolete) — — Splinter Review 
This continues Gavin's earlier patch, and follows the approach in the previous comment.
I don't know yet how to test this out though. I suppose it also needs a test which I will figure out next.
Let me know if the approach is fine.
I've requested f? from Gavin but if I should be requesting from someone else please let me know/redirect the f?.
Thanks.
Attachment #8608921 - Flags: feedback?(gavin.sharp)
Attached patch Continue Gavin's patch v1.1 (obsolete) — — Splinter Review 
Forgot to add NewTabURL.jsm to moz.build.
Attachment #8608921 - Attachment is obsolete: true
Attachment #8608921 - Flags: feedback?(gavin.sharp)
Attachment #8608923 - Flags: feedback?(gavin.sharp)
Attached patch Continue Gavin's patch v1.2 (obsolete) — — Splinter Review 
The changes from Gavin's patch didn't get applied (bitrot that I didn't notice). This patch fixes that.
Attachment #8608923 - Attachment is obsolete: true
Attachment #8608923 - Flags: feedback?(gavin.sharp)
Attachment #8608976 - Flags: feedback?(gavin.sharp)
Comment on attachment 8608976 [details] [diff] [review]
Continue Gavin's patch v1.2

Looks reasonable to me! Thanks for picking this one up.

Perhaps smaug can review the nsDocShell/nsIXULBrowserWindow changes.
Flags: needinfo?(gavin.sharp)
Attachment #8608976 - Flags: feedback?(gavin.sharp) → feedback+
Comment on attachment 8608976 [details] [diff] [review]
Continue Gavin's patch v1.2

Review of attachment 8608976 [details] [diff] [review]:
-----------------------------------------------------------------

I think Gavin meant to set a review flag on smaug in comment 15 for the cpp/idl changes.

::: browser/base/content/utilityOverlay.js
@@ +9,5 @@
>  Components.utils.import("resource://gre/modules/PrivateBrowsingUtils.jsm");
>  Components.utils.import("resource:///modules/RecentWindow.jsm");
>  
>  XPCOMUtils.defineLazyGetter(this, "BROWSER_NEW_TAB_URL", function () {
> +  return NewTabURL.get();

Shouldn't this become a normal getter instead of a lazy one? I'm surprised the browser_bug767836_perwindowpb.js test can pass if we keep using the value returned by NewTabURL.get the first time. Am I missing something?

::: browser/modules/NewTabURL.jsm
@@ +26,5 @@
> +  override: function (newURL) {
> +    this._url = newURL;
> +    this._overridden = true;
> +  },
> +  

nit: please avoid adding trailing whitespace.

@@ +31,5 @@
> +  reset: function () {
> +    this._url = "about:newtab";
> +    this._overridden = false;
> +  }
> +}

nit: missing ';' here.
Attachment #8608976 - Flags: review?(bugs)
(In reply to Florian Quèze [:florian] [:flo] from comment #16)
> ::: browser/base/content/utilityOverlay.js

> >  XPCOMUtils.defineLazyGetter(this, "BROWSER_NEW_TAB_URL", function () {
> > +  return NewTabURL.get();
> 
> Shouldn't this become a normal getter instead of a lazy one?

Yes, good catch.
Comment on attachment 8608976 [details] [diff] [review]
Continue Gavin's patch v1.2

Reviewing only .idl and .h/.cpp
>+++ b/embedding/browser/nsIWebBrowserChrome3.idl
>@@ -42,9 +42,12 @@ interface nsIWebBrowserChrome3 : nsIWebB
update uuid of the interfaces you change.



>+NS_IMETHODIMP nsContentTreeOwner::ShouldAddToSessionHistory(nsIDocShell *aDocShell,
>+                                                            nsIURI *aURI,
>+                                                            bool *_retval)
NS_IMETHODIMP
nsContentTreeOwner::ShouldAddToSessionHistory(nsIDocShell* aDocShell,
                                              nsIURI* aURI,
                                              bool* aRetVal)




>+  if (xulBrowserWindow)
>+    return xulBrowserWindow->ShouldAddToSessionHistory(aDocShell, aURI, _retval);

always {} with if. (I know you're dealing with some ancient .cpp file, but still)


>+++ b/xpfe/appshell/nsIXULBrowserWindow.idl
>@@ -57,15 +57,18 @@ interface nsIXULBrowserWindow : nsISuppo
update uuid
Attachment #8608976 - Flags: review?(bugs) → review+
this new functionality - will spoil functionality of my preferred set "browser.newtab.url;about:newtab" ?
Attached patch Patch v2 (obsolete) — — Splinter Review 
Addressed the previous review comments. This patch gets rid of BROWSER_NEW_TAB_URL and just uses NewTabURL.get() everywhere. I ran the three tests that seem to be relevant to this and they all pass. I'm going to push it to try though, and post the link here once I do.
flo, requesting review from you for the JS changes. There's nothing new in CPP/IDL land except for addressing smaug's review comments.
Attachment #8544732 - Attachment is obsolete: true
Attachment #8608976 - Attachment is obsolete: true
Attachment #8610838 - Flags: review?(florian)
Comment on attachment 8610838 [details] [diff] [review]
Patch v2

Review of attachment 8610838 [details] [diff] [review]:
-----------------------------------------------------------------

While I like replacing BROWSER_NEW_TAB_URL with NewTabURL.get() that is shorter and more explicit, given https://mxr.mozilla.org/addons/search?string=BROWSER_NEW_TAB_URL I'm afraid you'll still need to define a getter for BROWSER_NEW_TAB_URL to avoid breaking add-ons.
Attachment #8610838 - Flags: review?(florian) → feedback+
Attached patch Patch v3 (obsolete) — — Splinter Review 
This patch adds the getter, but still replaces all usages of BROWSER_NEW_TAB_URL with NewTabURL.get(). If you would like me to revert that and keep using BROWSER_NEW_TAB_URL everywhere (to preserve blame?), let me know.
Attachment #8610838 - Attachment is obsolete: true
Attachment #8611581 - Flags: review?(florian)
Comment on attachment 8611581 [details] [diff] [review]
Patch v3

Review of attachment 8611581 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/utilityOverlay.js
@@ +14,2 @@
>  
> +__defineGetter__("BROWSER_NEW_TAB_URL", function() NewTabURL.get());

Other places using __defineGetter__ in similar contexts seem to prefix it with "this."

I think expression closures are deprecated; maybe replace it with an arrow function here?

Please add a comment above the getter saying BROWSER_NEW_TAB_URL is deprecated but kept for add-on compatibility.
Attachment #8611581 - Flags: review?(florian) → review+
Did you forget to paste a try server link?
It's unfortunate timing to take away this pref right when people are up in arms about Tiles -- the conspiracy minded will see nefarious motives :-(  [Yes, I know the tiles page can be blanked as a feature of the page itself and that there's a different extension hook in this patch, but when have facts ever gotten in the way of a good conspiracy theory?]
(In reply to Daniel Veditz [:dveditz] from comment #25)
> It's unfortunate timing to take away this pref right when people are up in
> arms about Tiles -- the conspiracy minded will see nefarious motives :-( 
> [Yes, I know the tiles page can be blanked as a feature of the page itself
> and that there's a different extension hook in this patch, but when have
> facts ever gotten in the way of a good conspiracy theory?]

Also, the patch changes interface uuids, so we won't uplift it. This won't be released before Firefox 41; by then people will possibly have found other topics to make conspiracy theories about.
Attached patch Patch v4 (obsolete) — — Splinter Review 
Addressed review comments, carrying forward r+.

Green try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ca43fcec503
Attachment #8611581 - Attachment is obsolete: true
Attachment #8613609 - Flags: review+
Attached patch Patch v4 for real (obsolete) — — Splinter Review 
My exporting alias was busted, the previous patch was the wrong one.
This is the correct one.
Attachment #8613609 - Attachment is obsolete: true
Attachment #8613619 - Flags: review+
(In reply to Nihanth Subramanya [:nhnt11] from comment #27)

> Green try:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ca43fcec503

This try run has only xpcshell tests. The tests your patch touches are browser mochitests.
Comment on attachment 8613619 [details] [diff] [review]
Patch v4 for real

Found a few issues with this patch (thanks adw). I'll upload a new patch soon.
Attachment #8613619 - Flags: review+ → review-
Attached patch Patch v5 — — Splinter Review 
BROWSER_NEW_TAB_URL was doing some logic related to private browsing. This patch brings that back, and the getter is no longer replaced everywhere by NewTabURL.get() or marked as deprecated.

Also, fixes a typo that was causing test failures (this._overriden vs this._overridden).

I also added a small test for NewTabURL.jsm.

try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=09f7bde89948
Attachment #8613619 - Attachment is obsolete: true
Attachment #8614432 - Flags: review?(florian)
Comment on attachment 8614432 [details] [diff] [review]
Patch v5

Review of attachment 8614432 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks for catching this. I wonder if the private browsing case we failed to handle the first time needs testing.
Attachment #8614432 - Flags: review?(florian) → review+
Rank: 21
Priority: -- → P2
Whiteboard: [hijacking][fxsearch]
(In reply to Wes Kocher (:KWierso) from comment #34)
> Backed out for being the likely cause of talos failures:
> https://treeherder.mozilla.org/logviewer.html#?job_id=3383001&repo=fx-team

Joel, can you help us make sense of this svgr log?  The timeout happens between these two log lines:

15:49:26     INFO -  DEBUG : command line: C:\slave\test\build\application\firefox\firefox -profile c:\users\cltbld~1.t-w\appdata\local\temp\tmp1gistq\profile -tp file:\C:\slave\test\build\venv\lib\site-packages\talos\page_load_test\tart\tart.manifest -tpchrome -tpnoisy -tploadnocache -tpcycles 1 -tppagecycles 25

16:49:26     INFO - Automation Error: mozprocess timed out after 3600 seconds running ['C:\\slave\\test\\build\\venv\\Scripts\\talos', '--noisy', '--debug', '-v', '--executablePath', 'C:\\slave\\test\\build\\application\\firefox\\firefox', '--title', 'T-W864-IX-166', '--symbolsPath', 'https://queue.taskcluster.net/v1/task/d2LbrQONRyGHs-n-WpI6Vg/artifacts/public/build/firefox-41.0a1.en-US.win64.crashreporter-symbols.zip', '--activeTests', u'tsvgx:tsvgr_opacity:tart:tscrollx:cart', '--results_url', 'http://graphs.mozilla.org/server/collect.cgi', '--output', 'talos.yml', '--branchName', 'Fx-Team-Non-PGO', '--datazilla-url', 'https://datazilla.mozilla.org/talos', '--authfile', 'C:\\slave\\test\\oauth.txt', '--webServer', 'localhost']

There are three JS errors in WebappManager.jsm, but they also appear in passing svgr runs so they're probably unrelated.  But after those errors, there are big stacks that start like this:

> 15:49:21     INFO -  console.error:
> 15:49:21     INFO -    Message: Error: Connection closed before committing the transaction.
> 15:49:21     INFO -    Stack:
> 15:49:21     INFO -      ConnectionData.prototype<.executeTransaction/promise</transactionPromise<@resource://gre/modules/Sqlite.jsm:610:1
> 15:49:21     INFO -  TaskImpl_run@resource://gre/modules/Task.jsm:314:40

I suspect we'll have to run svgr locally to see if we can reproduce.
Status: NEW → ASSIGNED
Flags: needinfo?(nhnt11) → needinfo?(jmaher)
there isn't much there- locally this should be obvious- this is on all platforms.

Probably something with the initialization phase where we load getinfo.html and quite.  We do have an addon talos-powers which allows us to quit the browser.  Most likely what is wrong here is that we are using browser.newtab.url in tart/cart:
http://hg.mozilla.org/build/talos/file/tip/talos/page_load_test/tart/addon/content/tart.js

we probably need to fix this.
Flags: needinfo?(jmaher)
Thanks Joel!  I should have known that this bug would need to take the talos tests into account given that they use browser.newtab.url.  Nihanth will work on fixing them and flag you for review if that's OK.  We'll need to land those fixes at the same time.
cool!  We can land the talos changes, then adjust testing/talos/talos.json to point to the new talos revision while landing these changes again.  it is pretty simple.
oh, I will be on holiday tomorrow, but around on Friday!  ping :wlach if there is anything urgent in my absence.
Attached patch Talos Patch (obsolete) — — Splinter Review 
Patch for TART tests. The tests passed locally.
Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2c62e5bcc53a
Attachment #8621147 - Flags: review?(jmaher)
Comment on attachment 8621147 [details] [diff] [review]
Talos Patch

Review of attachment 8621147 [details] [diff] [review]:
-----------------------------------------------------------------

::: talos/page_load_test/tart/addon/content/tart.js
@@ +415,5 @@
>  
>    _startTest: function() {
>  
>      // Save prefs and states which will change during the test, to get restored when done.
> +    var origNewtabUrl =     NewTabURL.get();

Is this variable still used anywhere?
I tried this talos patch on try and it didn't work- NewTabURL is undefined.  Is there a chance I need the other patches from the bug as well?
(In reply to Joel Maher (:jmaher) from comment #42)
> I tried this talos patch on try and it didn't work- NewTabURL is undefined. 
> Is there a chance I need the other patches from the bug as well?

You need both patches to land at once.
Comment on attachment 8621147 [details] [diff] [review]
Talos Patch

Review of attachment 8621147 [details] [diff] [review]:
-----------------------------------------------------------------

thanks :florian for catching the unused variable origNewTabUrl- r=me with that removed.
Attachment #8621147 - Flags: review?(jmaher) → review+
note, once you land that patch on the talos repo, we can update testing/talos/talos.json:
https://dxr.mozilla.org/mozilla-central/source/testing/talos/talos.json?from=talos.json&case=true#1

and then bump the revision that is referenced there to point to the latest revision on talos so it picks up your patch.
Doing this will be highly annoying to a lot of us who manually set newtab.url to about:blank, and will essentially guarantee that those users assume you're pushing ads and/or features they don't want on them.

Maybe you guys could instead just make it so that browser.newtab.url can only be set by signed/certified addons OR the built-in Firefox UIs? That way users could still at least have a setting to trip in about:config without having to install an addon to remove a feature (which seems illogical and will almost certainly cause yet more grumbling from that perspective).

While you're at it, perhaps you could combine the preferences for browser.newtab.url and browser.startup.homepage, so the UI preference for "default home page" sets both (since startup.homepage is basically useless these days compared to newtab.url).
(In reply to Thomas from comment #46)
> Doing this will be highly annoying to a lot of us who manually set
> newtab.url to about:blank, and will essentially guarantee that those users
> assume you're pushing ads and/or features they don't want on them.

You can disable the tiles with just 2 clicks: https://support.mozilla.org/en-US/kb/new-tab-page-show-hide-and-customize-top-sites#w_how-do-i-turn-the-new-tab-page-off

> Maybe you guys could instead just make it so that browser.newtab.url can
> only be set by signed/certified addons OR the built-in Firefox UIs? That way
> users could still at least have a setting to trip in about:config without
> having to install an addon to remove a feature (which seems illogical and
> will almost certainly cause yet more grumbling from that perspective).

Installing an add-on will only be needed if you want to have a custom new tab page. Disabling doesn't require an add-on.

> While you're at it, perhaps you could combine the preferences for
> browser.newtab.url and browser.startup.homepage, so the UI preference for
> "default home page" sets both (since startup.homepage is basically useless
> these days compared to newtab.url).

I think there's some effort underway to merge the default home page and the default new tab page, but I don't know the details.
>You can disable the tiles with just 2 clicks

Thanks.


>Installing an add-on will only be needed if you want to have a custom new tab page. Disabling doesn't require an add-on.

I still fear that this will annoy more users than necessary, but I don't want to pollute this ticket with debates. I do see a Custom New Tab addon that seems to allow this, at least.


>I think there's some effort underway to merge the default home page and the default new tab page, but I don't know the details.

Thanks again, I'll hunt around when I have more time.
Attached patch Talos Patch v1.1 — — Splinter Review 
Removed unused variable, carrying forward r+.
Attachment #8621147 - Attachment is obsolete: true
Attachment #8621704 - Flags: review+
Dave, there are some usages of this pref in build/partner-repacks as well (https://mxr.mozilla.org/build/search?string=browser.newtab.url). Should I be updating those in this bug too? AFAIK I should, but I was told I might want to ask you to be sure.
Flags: needinfo?(dtownsend)
(In reply to Nihanth Subramanya [:nhnt11] from comment #50)
> Dave, there are some usages of this pref in build/partner-repacks as well
> (https://mxr.mozilla.org/build/search?string=browser.newtab.url). Should I
> be updating those in this bug too? AFAIK I should, but I was told I might
> want to ask you to be sure.

I would expect the answer to be yes, maybe Mike can confirm?
Flags: needinfo?(dtownsend) → needinfo?(mconnor)
Don't worry about those examples. They're all contained within partner-maintained add-ons.  If we're deprecating the pref, we should be notifying add-on devs that it's no longer something they can use.  Are we making this official policy in terms of "yes you can/no you can't do this" in general?  If the answer is "no you can't" we should make sure that's in the add-on guidelines+automated checks (cc Kev/Jorge).

What's the expected timeline for this?
Flags: needinfo?(mconnor)
(In reply to Mike Connor [:mconnor] from comment #52)
> Don't worry about those examples. They're all contained within
> partner-maintained add-ons.  If we're deprecating the pref, we should be
> notifying add-on devs that it's no longer something they can use.  Are we
> making this official policy in terms of "yes you can/no you can't do this"
> in general?  If the answer is "no you can't" we should make sure that's in
> the add-on guidelines+automated checks (cc Kev/Jorge).
> 
> What's the expected timeline for this?

We won't uplift the patch, it changes uuids, so we expect the change to be in Firefox 41.
(In reply to Florian Quèze [:florian] [:flo] from comment #0) 
> We should remove it and encourage people using a non-default new tab page to install an add-on
> instead.

For other bugzilla readers: I created a add-on to override the new tab page:
https://addons.mozilla.org/de/firefox/addon/new-tab-override/
(In reply to Greg from comment #56)
> when this newtab.url change lands, will I still be able to set
> newtabs to "blank"?

There is still a visible option on the new tab page to set a blank page for new tabs, there is no need to set "browser.newtab.url" or to install an add-on if you want a blank page.
There really needs to be a notification here when the value is changed so an add-on can know when another add-on set it (or when the user reset it).

Otherwise, all an add-on will do is simply take the new tab page at startup and we'll be in the same bad shape that Chrome is in.

The add-on needs to maintain an internal state ("I own the tab page") and then unset that state when another add-on (or the user) resets the new tab page.
(In reply to yonezpt from comment #59)

> Mozilla will be enforcing add-on
> signing in the future which will prevent this vulnerability and allow a safe
> user customization.

Add-on signing will prevent the new mechanism introduced here from being abused. It won't prevent the prefs.js file in the user's profile folder from being edited by third party software.

The purpose of this bug is to prevent the new tab page from being changed against the user's intent. If you think the UI needs to be changed to allow more customization options for the new tab page, please file this as a new bug.
(In reply to Mike Kaply [:mkaply] from comment #61)

I don't really understand the use case for your proposed notification (why would a user intentionally install 2 different add-ons both attempting to change the new tab page?), but if you think a notification is needed, I don't see a problem with adding one (in a new bug though).
(In reply to yonezpt from comment #59)
> At worst allow these two alternative options: Set new tab pages to blank and
> set new tab pages to homepage.

Bug 850284 deals with adding a UI option to show your home page when opening a new tab.
(In reply to Florian Quèze [:florian] [:flo] from comment #62)
> (In reply to yonezpt from comment #59)
> 
> > Mozilla will be enforcing add-on
> > signing in the future which will prevent this vulnerability and allow a safe
> > user customization.
> 
> Add-on signing will prevent the new mechanism introduced here from being
> abused. It won't prevent the prefs.js file in the user's profile folder from
> being edited by third party software.
> 
> The purpose of this bug is to prevent the new tab page from being changed
> against the user's intent. If you think the UI needs to be changed to allow
> more customization options for the new tab page, please file this as a new
> bug.

Thanks, Florian. In that case I disagree with the intent to remove this option as a direct result of the reason behind it. Completely removing features due to security flaws shouldn't be the first option, in fact they should be the last. This decision will affect many users negatively, I suggest (if I can) finding an alternative solution for this problem, such as allowing it to be modified as easily as the homepage instead of just leaving it hidden and hard to find for novice users, stored in files that are vulnerable to third party software access/modification.
(In reply to Florian Quèze [:florian] [:flo] from comment #63)
> (In reply to Mike Kaply [:mkaply] from comment #61)
> 
> I don't really understand the use case for your proposed notification (why
> would a user intentionally install 2 different add-ons both attempting to
> change the new tab page?), but if you think a notification is needed, I
> don't see a problem with adding one (in a new bug though).

There are a number of add-ons that offer new tab as one of multiple feature that could be turned on and off.

You need to provide a way for those add-ons that change the new tab page to coexist.

In addition, if Firefox provides a way to reset the new tab page in prefs, an add-on that is changing the new tab page needs to know when that happens so it doesn't take the new tab page the next time it starts up.

If you don't provide this, you're just going to have a bunch of add-ons that always take the new tab page at startup.
Depends on: 1178152
(In reply to marnick.leau from comment #66)
> Can't you implement this with a clear textbox in the UI

Again, this bug isn't the right place to discuss UI changes. If you think UI changes are needed, please file a new bug, where UX people can give it the appropriate attention.
https://hg.mozilla.org/mozilla-central/rev/a03993fdddd8
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Blocks: 1179058
(In reply to dave from comment #71)
> ....WHAT is this? 
> https://addons.mozilla.org/en-US/firefox/addon/new-tab-override/  I can have
> custom new tab content for the low price of a $3 recommended donation and
> external code that auto-updates?

I don't know what do you mean. It's free like every add-on on addons.mozilla.org.
Depends on: 1180320
Depends on: 1178837
Depends on: 1180864
Is there a plan to do any public messaging about this page? After reading the rationale here it makes sense, but given that it's making it harder to switch away from a page full of potential ads, this will *definitely* be interpreted the wrong way.
I think a SUMO page should be the maximum we do.  It's a relatively obscure pref, aggressive outreach... meh.

That said, our approach to deprecating hidden prefs is kinda busted.  We can/should do this more systemically.
There's already a SUMO article at https://support.mozilla.org/en-US/kb/new-tab-page-show-hide-and-customize-top-sites#w_how-do-i-turn-the-new-tab-page-off

Who do we need to reach out to, to ensure the paragraph starting with "You can also turn this feature off completely" gets removed (at least for Firefox 41+) ?
(In reply to Florian Quèze [:florian] [:flo] (PTO until August 3rd) from comment #77)
> There's already a SUMO article at
> https://support.mozilla.org/en-US/kb/new-tab-page-show-hide-and-customize-top-sites#w_how-do-i-turn-the-new-tab-page-off
> 
> Who do we need to reach out to, to ensure the paragraph starting with "You
> can also turn this feature off completely" gets removed (at least for
> Firefox 41+) ?

Bug 1164010, comment 3. It doesn't seem possible to target articles for Firefox 41 yet. I tried wrapping the paragraph in {for not fx41}{/for} but it was ignored. The version selection options also say “Version 31… Version 40” but “Firefox 41”.

There are two related article threads:
https://support.mozilla.org/en-US/kb/new-tab-page-show-hide-and-customize-top-sites/discuss/6195
https://support.mozilla.org/en-US/kb/new-tab-page-show-hide-and-customize-top-sites/discuss/6156
the new tab page is still hijackable through a pref - what's prohibiting adware of running their own ad/tile server for about:newtab by overwriting the respective values?
(In reply to philipp from comment #79)
> the new tab page is still hijackable through a pref - what's prohibiting
> adware of running their own ad/tile server for about:newtab by overwriting
> the respective values?

I'm assuming this comment is about the browser.newtabpage.directory.source preference.

Ed, are there plans to restrict modifications to that preference (or why do we need to have that preference in the first place)? I see in bug 1157810 a linksURLModified boolean has been added that explicitly waives some checks we otherwise do.
Flags: needinfo?(edilee)
ok, i just assumed by the presence of those prefs that they'd be freely customizable - wasn't aware that there are further checks in the background...
(In reply to Florian Quèze [:florian] [:flo] from comment #77)
> There's already a SUMO article at
> https://support.mozilla.org/en-US/kb/new-tab-page-show-hide-and-customize-
> top-sites#w_how-do-i-turn-the-new-tab-page-off
> 
> Who do we need to reach out to, to ensure the paragraph starting with "You
> can also turn this feature off completely" gets removed (at least for
> Firefox 41+) ?

Hi Florian, I'm the content manager at SUMO. We've added the markup to hide this conditionally for > version 41.
(In reply to Florian Quèze [:florian] [:flo] from comment #47)
> (In reply to Thomas from comment #46)
> > Doing this will be highly annoying to a lot of us who manually set
> > newtab.url to about:blank, and will essentially guarantee that those users
> > assume you're pushing ads and/or features they don't want on them.
> 
> You can disable the tiles with just 2 clicks:
> https://support.mozilla.org/en-US/kb/new-tab-page-show-hide-and-customize-
> top-sites#w_how-do-i-turn-the-new-tab-page-off

No, you can't, you can only hide some parts of new tab page.

(In reply to Sören Hentzschel from comment #57)
> (In reply to Greg from comment #56)
> > when this newtab.url change lands, will I still be able to set
> > newtabs to "blank"?
> 
> There is still a visible option on the new tab page to set a blank page for
> new tabs, there is no need to set "browser.newtab.url" or to install an
> add-on if you want a blank page.

There is no such option, there is an option to hide some elements but for sure not to open blank pages (like about:blank blank is).
> There is no such option, there is an option to hide some elements but for sure not to open blank pages (like about:blank blank is).

Neither is there an option to display your own custom page, for example, a local page.

There is an extension which allows you to do that.  Now that extensions are signed (and presumably there is some revocation mechanism), I'm more inclined to accept an extension as a valid way to provide the desired functionality.

Dev edition will continue to support unsigned extensions.  Smart.
Depends on: 1196737
Doesn't Bob49 have a point? Can't these same addons just abuse browser.startup.homepage in the same manner?
Also, in FF 41 all addons have to be signed iirc. Doesn't that resolve this "problem?"
(In reply to johnsc301 from comment #89)
> Can't these same addons just abuse
> browser.startup.homepage in the same manner?

The homepage is configurable with a user visible UI. This bug is unrelated to the homepage.

Also, the abuse we are avoiding here is abuse made by other malware writing directly to the user's profile; not changes made by add-ons.

> Also, in FF 41 all addons have to be signed iirc. Doesn't that resolve this
> "problem?"

It's part of the fix. Now that add-ons are signed, we can disable add-ons that have abusive behaviors. With the change made in this bug, changing the newtab page requires an add-on being installed; which means disabling the add-on will be enough to revert the change. Disabling an add-on wouldn't have reverted any unwanted change before, when the new tab url was just stored as a preference.
If the difference between the proposed treatment of browser.newtab.url and the current treatment of browser.startup.homepage is a matter of whether or not there's a user-visible interface for its configuration, wouldn't the better answer here be to provide a configuration interface for browser.newtab.url?  That way, users are able to see whether or not something's set their new tab page without their permission (which is - presumably - why browser.startup.homepage hasn't received the same treatment yet), and at the same time users now have even more ability to customize their online experience as they see fit.

As someone who's done plenty of malware removal over the course of a decade, I've yet to see malware actually use browser.newtab.url; they generally opt for ad-injecting addons or tweaking browser.startup.homepage in my observation (or bypass Firefox entirely and redirect victims to a malicious proxy on an OS level).  I think just providing an easily-accessible and easily-checkable setting in the Preferences/Settings UI would be more than enough to address the "browser.newtab.url preference is abused" aspect; removing it entirely is just going to be a hassle for users (I'd rather not have to write my own addon just to retain my browser's current behavior of opening DuckDuckGo on each new tab - especially if that means having to go through the hassle of signing said addon in the near-future) without any appreciable benefit in the fight against malware.
(In reply to northrup from comment #91)
> If the difference between the proposed treatment of browser.newtab.url and
> the current treatment of browser.startup.homepage is a matter of whether or
> not there's a user-visible interface for its configuration, wouldn't the
> better answer here be to provide a configuration interface for
> browser.newtab.url?

The homeage page is something we expect lots of (if not 'most') users to customize. The url of the new tab page is something we expect a tiny fraction of our users to be interested in customizing. This is why there is an add-on API. If we didn't expect anybody to want to customize it, it would be hardcoded. If we expected most people to want to customize it, there would be visible UI to tweak it.

> I've yet to see malware actually use browser.newtab.url

Now that we removed the pref, I don't expect you to see any :-).

If you are curious, the abuse of that pref that I observed was by a piece of software that called itself "Search Protect by Conduit". It was replacing the new tab page by a page full of ads and reverting the pref to its original value in about:config wasn't enough, as a pref observer was used to ensure the ads page stayed there.
(In reply to Florian Quèze [:florian] [:flo] from comment #93)
> (In reply to northrup from comment #91)
> > I've yet to see malware actually use browser.newtab.url
> 
> Now that we removed the pref, I don't expect you to see any :-).
> 
> If you are curious, the abuse of that pref that I observed was by a piece of
> software that called itself "Search Protect by Conduit". It was replacing
> the new tab page by a page full of ads and reverting the pref to its
> original value in about:config wasn't enough, as a pref observer was used to
> ensure the ads page stayed there.

Alright then.  My point, though, was that homepage alterations and ad injections are much more common among Firefox-targeting malware in my various years of hands-on experience dealing with these sorts of things, and that this feels very minor in comparison.  It's interesting that there's some malware targeting the new tab page, but it's usually the homepage that's the juicier target, and would be susceptible to the same attack (using a pref observer to keep it set to a specific malicious page).

Whatever the case, what's done is done, so it's probably too late to make further recommendations here.  Just seems like removing the pref entirely is overkill.  At least there are indeed addons reenabling this functionality now.
(In reply to Ryan S. Northrup from comment #94)

> it's usually the homepage
> that's the juicier target, and would be susceptible to the same attack

I agree, but currently don't have any good idea of what to do about it.

I think users are more likely to notice an unwanted homepage and revert it on their own, but it's certainly not a good solution. Obviously we can't make the homepage harder to customize for users.
Actually, the relevant bug for the homepage pref is bug 1051082.
Certainly there are many options which we don't expect most users to customize, yet they exist and are important (e.g. privacy settings).  I'd reckon the number of users who customize their new tab page is more than those who have been hijacked.  Additionally, in the latter group, at least the hijacking was obvious whenever they used their browser; now it may be masked and take longer to discover, compromising more of the user's system.  Locking down the browser to protect users from themselves seems a terrible approach.
(In reply to merfius from comment #97)
> I'd
> reckon the number of users who customize their new tab page is more than
> those who have been hijacked.

I haven't seen the numbers (haven't been involved with this change either), but I'd be surprised if your guess is correct.
As I understand it, somebody decided to remove a setting from about:config.

The first time I knew that had happened was when my custom new tab page stopped working after an upgrade.

Myself and others who did not like the change posted on this bug and others.  The change only affected power users and (supposedly?) malware victims.  Power users were offended! (see profanity in 'hidden' messages above. Note use of 'advocacy' tag.)

The change occurred at the same time some new-tab page came around, a page which apparently contains some third party content.  A whole separate group of users were offended about *that*.

I do not build my own firefox.  (I do build my own emacs.)  That's really what this comes down to, near as I can tell.  Some people 'own' firefox, they are committers.  The rest of us are just watching.  Being offended doesn't change anything.  Especially on a "RESOLVED FIXED" bug.

If we really care about this decision (and others lately), we should grab a copy of the source, revert some commits, merge in the good commits that happened after the ones we removed, build the thing, and distribute a binary.  Yeah, I know how stupid this recommendation is.  After all, little nit-picks like this are why the about:config system exists.  Ahem.
May i ask what would be wrong with making that feature a GUI option?
or how about we just go a step farther and create a reset 'common adware abused settings' button (under privacy settings)?
or maybe even both
Depends on: 1207463
There is now the problem, that browser addons can override the newtab page (works okay with different addons), but not about:newtab. This means, the page which is shown after you close the last tab is now the default about:newtab instead of about:blank, which was configured there.
(In reply to alex from comment #101)
> There is now the problem, that browser addons can override the newtab page
> (works okay with different addons), but not about:newtab. This means, the
> page which is shown after you close the last tab is now the default
> about:newtab instead of about:blank, which was configured there.

Are you saying there's a place where about:newtab is hardcoded and doesn't respect the page set using the NewTabURL.jsm API? If so, please file a bug.
I think so.

Steps to reproduce:
- set browser.tabs.closeWindowWithLastTab=false
- close all tabs with Middleclick on the tab.

Then you get about:newtab (with empty urlbar), which is not changed by different "change newtab page" extensions.
(In reply to alex from comment #104)
> I think so.
> 
> Steps to reproduce:
> - set browser.tabs.closeWindowWithLastTab=false
> - close all tabs with Middleclick on the tab.
> 
> Then you get about:newtab (with empty urlbar), which is not changed by
> different "change newtab page" extensions.

I cannot reproduce this. I set the pref you mentioned to false, then overrode my newtab url with the extension linked earlier in this bug. Closing all tabs via middle-click opens the custom URL, not about:newtab.
okay, with this (https://addons.mozilla.org/en-US/firefox/addon/new-tab-override/) extension it works. The other one i tried did not work.
(In reply to dave from comment #99)
> As I understand it, somebody decided to remove a setting from about:config.
> 
> The first time I knew that had happened was when my custom new tab page
> stopped working after an upgrade.

Ditto

I can appreciate that changing the newtab homepage is presumably now harder for malware authors to achieve (they need to write an addon and 
get the user to install it rather than whatever they were doing previously) and will become harder still when addon signing becomes a thing.

However as the functionality was useful (saved maybe 30sec/day at my workplace per person - the work we do isn't of earthshattering importance, but there is a *lot* of it (way too much given the current staffing levels...but I digress), and those seconds do add up) I have to wonder if there might not have been a more elegant solution.

Perhaps something along the lines of how firefox handles popup windows - when a new tab is opened and the value of browser.newtab.url is something other than about:newtab, show a message to the user 'This looks like it might be dodgy, is it what you want to happen?'. It could give them the option of whitelisting the current url (no further popups unless browser.newtab.url is changed to another, non-about:newtab url) or resetting the newtab url to about:newtab (possibly including a 'learn more' button linking to a page explaining how that the change might have happened (make the user aware/more aware that malware can do things like that and how to spot/avoid malware) and telling them how to change the newtab url to something other than the default if that is what they want to do).

Given that I've used firefox since it was called firebird/phoenix and only found out that changing the newtab page was possible a couple of months ago, it is quite possible that there would be people who only find out about this option after having had their newtab page hijacked, but then want to use it.

Anyhow I do realise that at this stage this is a completely moot point - if I took/had the time to be involved with the project I might have been able to make this suggestion when it could have mattered if the right people thought it was a good way to go - if only there were more hours in the day :/.
I'd love to be all diplomatic here, but the newtab url feature has always been a feature that has resulted in unwanted privacy violations for customers.  You could say that users should use private browsing mode when they want things to be private, but they are not that well informed.  And they shouldn't have to be in order to use a web browser without advertising the sites they visit most when they open a new tab.

Having worked on many computers to remove malware infections, the biggest problem with the newtab url page is that customers do not realize that it is there, and when someone opens a new tab, it embarrasses them.  The amount of malware I have encountered that abuses the feature is minimal in comparison.  Where I see most of the malware affecting Firefox is the extension system.  This is often compromised when the user wants to get functionality they don't currently have, and they install a piece of software or an extension after blindly googling.

It also robs the user of their time.  Many of us still use good ole' bookmarks in the bookmarks toolbar to go to frequently visited sites.  We don't need a list generated for us based on whatever a developer thought would be a good idea.  And certainly changing one that has been changed to "about:blank", which has never been associated with malware, is disrespectful of the users' obvious choices.

This change should be reverted.  Blank page when about:blank is entered.  No gear at the top right, no processing of anything besides what is necessary to create a new tab.  No extensions required to restore functionality.  I probably have to remove a few dozen malicious extensions a week from customers' computers.  I always check about:config on infected computers.  I'm sure I have seen newtab hijacked, but not enough to remember when.
(In reply to Matthew Pirrone from comment #108)
> Having worked on many computers to remove malware infections, the biggest
> problem with the newtab url page is that customers do not realize that it is
> there, and when someone opens a new tab, it embarrasses them.

So they should hide them with the gear. This is way more accessible than a hidden pref. Now the fact that the page doesn't become completely blank (because the gear remains) may very well annoy you, but that's independent from the straw man argument you've constructed above.
Some observations, some of which have already been made by myself and others:

1. Power users are annoyed that this setting was removed.
2. Committers are deaf to complaints about this issue.

Some observations that might be new:
1. Look, here are some legit users of this setting: https://bugzilla.mozilla.org/show_bug.cgi?id=757455
2. This bug has a "whiteboard" tag of "hijacking".  What do you want to bet that there is nothing a user can post on a bug thusly tagged which will convince committers to change course?  That's probably good policy.
3. Although there is no reason to doubt that some malware abused this setting, no concrete example has been provided.  Concrete examples of legitimate use have been provided.

It's a shame.

I have an idea for a solution, but it is a lot of work.

How about building a user-settings mechanism which cannot be trivially hijacked: that is, do not provide an API for it.  Only accept user input via GUI.  A screen like that would be a good place to keep this setting, and others.
Ok, that's enough of that.

I'm locking this bug to editbugs-privileged only accounts. Though I'm reluctant to do this, the decision in this bug has been made and eight of the last ten comments have been flagged for various reasons. Advocacy for new features or policy change belongs either in new bugs or other forums. 

If you'd like to discuss my decision to do this, feel free to email me directly.
Restrict Comments: true
Depends on: 1214045
Depends on: 1219520
Blocks: 1219520
No longer depends on: 1219520
Depends on: 1271727
Depends on: 1426438
See Also: → 1452144
browser.newtabpage.directory.source is no more, but it was also intended to allow a custom set of tiles to be shown instead of those from Mozilla for profiles that didn't have enough history to show the user's own tiles.
Flags: needinfo?(edilee)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: