Opened 4 years ago

Closed 3 years ago

#15852 closed defect (fixed)

Remove/synchronize Torbutton SOCKS pref logic

Reported by: mikeperry Owned by: brade
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-4.5-regression, tbb-torbutton-conversion, TorBrowserTeam201609R
Cc: brade, mcs, gk, arthuredelstein Actual Points:
Parent ID: Points:
Reviewer: Sponsor: SponsorU

Description

In 4.5 (#14630), we removed the Torbutton settings window because I assumed that users could use the Firefox proxy setting instead. Unfortunately, Torbutton still has code to check if its SOCKS settings match the browser proxy settings, and to disable itself if these are out of sync.

We should remove this old code so that it is easier for people to point TBB at a different Tor instance. The code is a toggle-era relic, and we have a control port check anyway.

We should also ensure that if the Tor check fails, Torbutton's features aren't actually disabled, and that the difference is only a cosmetic warning. Right now, I fear that features are actually disabled, due to other toggle-era checks.

Child Tickets

Change History (26)

comment:1 Changed 4 years ago by mcs

Cc: brade mcs added

comment:2 Changed 4 years ago by gk

Cc: gk added

comment:3 Changed 4 years ago by cypherpunks

Slightly related: #14864

comment:4 Changed 4 years ago by mikeperry

Keywords: TorBrowserTeam201505 removed

comment:5 Changed 4 years ago by arthuredelstein

Cc: arthuredelstein added

comment:6 Changed 4 years ago by mikeperry

Keywords: tbb-torbutton-conversion added

comment:7 Changed 3 years ago by gk

Sponsor: SponsorU

comment:8 Changed 3 years ago by gk

Keywords: TorBrowserTeam201608 added

Getting important SponsorU things on our August radar.

comment:9 Changed 3 years ago by mcs

Owner: changed from tbb-team to brade
Severity: Normal
Status: newassigned

Making Kathy the owner since she and I started to work on this ticket already.

comment:10 Changed 3 years ago by mcs

I filed a related ticket: #19929

comment:11 Changed 3 years ago by mcs

Keywords: TorBrowserTeam201608R added; TorBrowserTeam201608 removed
Status: assignedneeds_review

Sometimes when you pull on a thread, an entire sweater begins to unravel. Kathy and I removed the code that dealt with the long gone proxy preferences, and that led us to to clean up more Torbutton code. The following branch contains 4 commits that are ready for review:
https://gitweb.torproject.org/user/brade/torbutton.git/log/?h=bug15852-01

These commits can be smooshed together before merging into master, but it seemed better to keep them separate for review purposes.

Please let us know if we removed something that should be kept. In our judgment, all of the code that we removed was either not being used or it could have confusing (and possible harmful) effects.

comment:12 Changed 3 years ago by gk

Status: needs_reviewneeds_revision

commit 573bb7fb3b22cfc28d3dce01e2378d1b181e6d43 looks good.

re commit 6244aadf7cfd944e8b53aabd1da52fe40df89f96:

1) It seems we don't need to add/remove an observer for network.proxy anymore in torbutton,js?
2) Maybe add 3. Reset Security Slider settings" in torbutton_prefs_reset_defaults()`?
3) I have the feeling that

// XXX: Hack for TBB people who alternate between transproxy and non

in startup-observer.js can go as well or did I miss something?

commit ab40a9b20bab2d2608ad528830338c24fcf98e32 looks good.
commit aa5cceae245efca766d5287d6754fac9c589b89e looks good.

So, just the nits above + squashing the commits into a single one and we are good here, thanks!

comment:13 in reply to:  12 ; Changed 3 years ago by mcs

Replying to gk:

commit 573bb7fb3b22cfc28d3dce01e2378d1b181e6d43 looks good.

re commit 6244aadf7cfd944e8b53aabd1da52fe40df89f96:

1) It seems we don't need to add/remove an observer for network.proxy anymore in torbutton,js?

You are right. We will remove that observer.

2) Maybe add 3. Reset Security Slider settings" in torbutton_prefs_reset_defaults()`?

OK.

3) I have the feeling that

// XXX: Hack for TBB people who alternate between transproxy and non

in startup-observer.js can go as well or did I miss something?

Do you mean just remove the comment or do you want the code to be removed? Kathy and I thought we should keep the code so that existing behavior is not modified for users who rely on setting the env variables.

Thanks for reviewing all of this!

comment:14 in reply to:  13 Changed 3 years ago by gk

Replying to mcs:

Replying to gk:

3) I have the feeling that

// XXX: Hack for TBB people who alternate between transproxy and non

in startup-observer.js can go as well or did I miss something?

Do you mean just remove the comment or do you want the code to be removed? Kathy and I thought we should keep the code so that existing behavior is not modified for users who rely on setting the env variables.

Just the comment. It seemed to me, if at all, then it only applied to the lines you removed.

Thanks for reviewing all of this!

Sure. :)

comment:15 Changed 3 years ago by mcs

Status: needs_revisionneeds_review

comment:16 Changed 3 years ago by gk

Resolution: fixed
Status: needs_reviewclosed

Thanks! This is commit 1b41636c9351b46bbcb2094a65c65a4407b60a37 on torbutton master now.

comment:17 Changed 3 years ago by mcs

Resolution: fixed
Status: closedreopened

comment:18 Changed 3 years ago by mcs

Status: reopenedneeds_review

While testing some other things, Kathy and I noticed a change in behavior compared to older versions of Torbutton: prior to the commit that fixed this ticket, the network.proxy.socks and network.proxy.socks_port prefs were not reset in the absence of env vars. We should probably restore the old behavior.

Here is a fixup patch:
https://gitweb.torproject.org/user/brade/torbutton.git/commit/?h=bug15852-03&id=1648da8461dfed9975ac83fdb03aa745b6c8d49b

comment:19 Changed 3 years ago by gk

Resolution: fixed
Status: needs_reviewclosed

Merged (commit 1648da8461dfed9975ac83fdb03aa745b6c8d49b on master), thanks.

comment:20 Changed 3 years ago by legind

Is there any reason why the config options

extensions.torbutton.socks_host
extensions.torbutton.socks_port

were not removed with this update? Are they still being used anywhere?

comment:21 in reply to:  20 Changed 3 years ago by gk

Replying to legind:

Is there any reason why the config options

extensions.torbutton.socks_host
extensions.torbutton.socks_port

were not removed with this update? Are they still being used anywhere?

Looking at Torbutton master it seems to me they are gone as well? Grepping in different ways does not show instances of them anymore. Where do you still see them?

comment:22 Changed 3 years ago by mcs

Some outdated and unneeded Torbutton preferences are still included with default values via the extension-overrides.js file that is part of the Tor Browser profile. See:
https://gitweb.torproject.org/builders/tor-browser-bundle.git/tree/Bundle-Data/linux/Data/Browser/profile.default/preferences/extension-overrides.js

Kathy and I have prepared a patch to remove all of them, which we will post to #20111 very soon.

comment:23 in reply to:  22 Changed 3 years ago by mcs

Resolution: fixed
Status: closedreopened

Replying to mcs:

Kathy and I have prepared a patch to remove all of them, which we will post to #20111 very soon.

On second thought, the patch belongs in this ticket. Here it is:
https://gitweb.torproject.org/user/brade/tor-browser-bundle.git/commit/?h=bug15852-01&id=5a7d9c36cd6178fb87f3ea5598b400392120f589

Georg, please let me know if you would rather have a new ticket for this followup fix.

comment:24 Changed 3 years ago by mcs

Status: reopenedneeds_review

comment:25 Changed 3 years ago by gk

Keywords: TorBrowserTeam201609R added; TorBrowserTeam201608R removed

comment:26 Changed 3 years ago by gk

Resolution: fixed
Status: needs_reviewclosed

Looks good and got applied to master (commit 0a5e21b00533960317352ac27db996258544eac6) and hardened-builds (commit 36842fddafcf79bab6911def2ec4133826bc28f9).

Note: See TracTickets for help on using tickets.