bug 40656: remove fingerprintable preferences

Merge Info

• Related Issues

  • #40656

• Backport Timeline

  • Immediate - patchsets for critical bug fixes or other major blocker (e.g. fixes for a 0-day exploit) OR patchsets with trivial changes which do not need testing (e.g. fixes for typos or fixes easily verified in a local developer build)
  • Next Minor Stable Release - patchset that needs to be verified in nightly before backport
  • Eventually - patchset that needs to be verified in alpha before backport
  • No Backport - patchset for the next major stable

• Upstream Merging

  • Merge to base-browser - typically for !fixups to patches in the base-browser branch, though sometimes new patches as well
    • NOTE: if your changeset includes patches to both base-browser and tor-browser please please make separate merge requests for each part

• Issue Tracking

  • 12.0.4

Change Description

Removed fingerprintable preferences from about:preferences

• browser.download.useDownloadDir
• signon.rememberSignons
• browser.formfill.enable
• [already] signon.autofillForms
• browser.search.suggest.enabled
  • urlBarSuggestion
  • browser.urlbar.showSearchSuggestionsFirst
  • showSearchSuggestionsPrivateWindows
• network.http.windows-sso.enabled

"In addition to that, we need to make sure sync.inc.xhtml is not visible."

• it is, it's default hidden, and the show function doesn't get called unless entity.fxaccounts.enabled is true - is set to fasle in base profile

"Then, from #40899, we should review: "

• [already] Picture in picture
  • media.videocontrols.picture-in-picture.enabled
    • hides pref and master disable of feature
• Media keys controls
  • per IRC - should be fine to leave
• Extension recommendations
• Phishing and malware protection (in the privacy panel)
• [TODO seperate TB only MR] Default browser (only Tor Browser, not S131)
• Saved passwords and similar amenities
  • "My decision: discuss about keeping these features, and possibly add a manual page about the risks of having them"

requested review from @pierov

assigned to @dan

assigning to Pier as he did most of the prep work and documenting these issues.

I wasn't sure about the removal style so leaned into commenting it all out. I documented in the JS liberally, but not the .html.incs, just on personal asthetics and because at least the html was paired with #ifndef and you could look at the MR it's in for final context, but unsure if that's what you had in mind, so please let me know if you'd like this either structured or documented differently

I'm a little bit quizzed about the final UX for the password management.

We'll keep showing the saved password button, which could be a good idea, because users might have enabled it in the past, and now have saved passwords, so they should have a way to know it.

However, seeing this button with the feature disabled could be confusing from a first-time user point of view.

An idea could be showing it only if there are effectively saved passwords.

In general, I'm not sure on the final UX that we'll manage: if we don't lock these preferences but hide them, users that enabled them in the past, won't have a way to disable them again.

The same could be said for all the preferences, but for password the problem is particularly delicate.

yeah that was my thinking. good idea about that additional hidding if there are no saved passwords on hand. I'll take a look into adding that

so looking a bit more into this, if we want to hide it, I prolly need to add

 this._storage = Cc[
      "@mozilla.org/login-manager/storage/default;1"
    ].createInstance(Ci.nsILoginManagerStorage);

access to a storage for the login manager to privacy.js so I can get a count of saved passwords. Are we ok adding that to the settings page? Not sure how intrusive we want this PR to be?

added, it hides it

In general, I'm not sure on the final UX that we'll manage: if we don't lock these preferences but hide them, users that enabled them in the past, won't have a way to disable them again.

The same could be said for all the preferences, but for password the problem is particularly delicate.

@donuts some UX opinion here?

This is something I was expecting to look into as part of #27605, but not until later in the year. It's important enough that the UX needs to be thought through properly imo.

To be clear – removing minor prefs is totally fine, but changing major features should go through the UX pipeline first.

To be clear – removing minor prefs is totally fine

I think that also minor prefs could lead to an unwanted result: the impossibility to go back to the sane defaults, or us needing to force them (confusing for the user).

Do you want us to pause this MR and get back to it after you have an opinion about this?

Yeah, potentially – the passwords manager one stood out to me because we know it's pretty common for users to enable it. I wonder if we should come up with more of a formal "deprecation" process for high-use features like this in Tor Browser, with a grace period, migration strategy, warnings etc. for existing users.

Related question: are there any prefs here that can't wait, and that you'd like to hide urgently for safety reasons?

The worst one if Windows SSO, which has been already handled and locked down.

It was planned for the last alpha, but probably we were still on holiday and screwed up with tags .

The reason to disable it is linking: basically you're deanonymizing yourself if you auth with Windows SSO automatically (not sure how it works, I've never used this feature).

Right, I think that one will be fine!

I don't like "fingerprintable preferences", because we are dealing with many different kind of preferences, including disk-leak.

Maybe should we rename the issue, and then update the patch?

What do y'all think the tradeoffs are between commenting out via multiple // single-line comments vs /* ... */ pairs

My first instinct is that single-line comments will be easier to maintain w/ respect to rebases (or at least, more likely to cause merge conflicts when things change, which we may want to inspect) but I don't actually know.

I haven't had to handle the rebases so I'm less sure. My initial think was that if we wanted to avoid just deleting things cus that made merges/rebases hard to follow and liklier to conflict and more hassle, multi line comments were a good approach. For example if mozilla adjusted something covered by this, we'd still cover it and not show it automatically in merge/rebase with out having to revisit it.

On the other hand, if you want us to have to revisit these then yeah, single line comments will be much more likely to trigger that. At that point, whats the trade off between single line commenting it all out, and straight up deleting it?

I love that in C++ we can #ifdef/#ifndef.

I see a big danger for single-line comments: you risk to accept the commented version, instead of realizing that you should comment again. (FWIW it's something that happens and happened also with non-commented code).

Recently, I've replacing JS code only with a comment to the related issue. Deleting code forces you to re-evaluate what you're deleting, which isn't necessarily a bad thing, but makes the rebase slower.

The final achievement should always be uplift, but it won't likely happen for changes like this one.

I don't like "fingerprintable preferences"

@pierov do you just mean for this patch/ticket (and comments in code), so the tor blog release notes don't confuse users?

If so then just call it "troublesome preferences & UI settings [disk, fingerprint, linkification]"

"remove problematic stuff" :P

I don't care about release notes too much.

I care more of code documentation .

I also think we could bypass some prefs in code when RFP is enabled - e.g. RFP ignores general.**.override prefs, pretty sure it ignores a few others as well

so for example, when we patch RFP to make regional locale to match language, we would also ignore "use system formatting" (which is a pref), and so on - here's one - 1818894: RFP: harden network information protection

added 1 commit

• 7b5d9777 - bug 40656: remove fingerprintable preferences

Compare with previous version

added 1 commit

• 77f89a16 - bug 40656: remove fingerprintable preferences

Compare with previous version

added 1 commit

• d2af20fd - bug 40656: remove fingerprintable preferences

Compare with previous version

changed the description

@dan @pierov what happened with this MR? This i about removing prefs from about:preferences if I'm unerstanding correctly, so I presume that got done in some other MRs for mullvad-browser#34 (closed) ?

I think we want to do something also for Tor Browser, but I think we don't have a clean UX to go back to our safe defaults if we hide them.

Correct, Tor Browser will require some UX. I'm treating this as the UX ticket for what I'm presuming will amount to a redesign of about:preferences#privacy: #27605

Ok, that ticket is scheduled for 13.5 stable so let's close this MR an await the distant future.

closed