Opened 5 years ago

Closed 5 years ago

#13672 closed enhancement (fixed)

The circuit display dropdown should be optional.

Reported by: yawning Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Keywords: TorBrowserTeam201412R, TorBrowserTeam201412
Cc: arthuredelstein, gk, mikeperry, isis Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

The summary is self explanatory. There should be a way to entirely disable the circuit display including all of the associated control port interactions for people that use exotic configs, or are just plain paranoid.

It more than likely is excessive tinfoil hattery on my part, but I personally don't think that details on my circuits have any business sitting my the address space of my web browser. Note, there is an argument here that anyone that could get at that could also probably query the information themselves via the control port, but making it easier(?) for them doesn't feel great to me.

Child Tickets

Attachments (1)

0001-Bug-13672.-bind-tor-circuit-display-to-a-pref.patch (17.1 KB) - added by arthuredelstein 5 years ago.

Download all attachments as: .zip

Change History (20)

comment:1 Changed 5 years ago by gk

Keywords: tbb-4.5-alpha removed

I think this is a good idea.

comment:2 Changed 5 years ago by isis

Cc: isis added

comment:3 Changed 5 years ago by mikeperry

There's another good reason to make this optional and also auto-disabled in the event of weird control port failures: patch #8405 might not be present in a non-TBB tor client that the user is pointing their Tor Browser at instead of the Tor client we ship. People do this when they want to mix their browsing traffic in with the traffic routed through a relay or bridge that they are running for better traffic analysis resistance, for example.

There's also the Tails+Whonix control port filters, as well as Yawning's new control port filter. We should also gracefully degrade in the presence of those filters rather than barfing errors everywhere.

comment:4 in reply to:  3 Changed 5 years ago by arthuredelstein

Replying to mikeperry:

There's another good reason to make this optional and also auto-disabled in the event of weird control port failures: patch #8405 might not be present in a non-TBB tor client that the user is pointing their Tor Browser at instead of the Tor client we ship.

I do hope #8405 will land soon. It seems to me that an authenticated control port ought to have access to all useful state in the tor process.

There's also the Tails+Whonix control port filters, as well as Yawning's new control port filter. We should also gracefully degrade in the presence of those filters rather than barfing errors everywhere.

Agreed -- sorry for the mess. :) I'm working on a patch.

comment:5 Changed 5 years ago by arthuredelstein

Status: newneeds_review

I've bound the tor circuit display to a boolean pref named "extensions.torbutton.display_circuit". Should we also expose the option with a checkbox in the Torbutton preferences window?

comment:6 in reply to:  5 ; Changed 5 years ago by gk

Replying to arthuredelstein:

I've bound the tor circuit display to a boolean pref named "extensions.torbutton.display_circuit". Should we also expose the option with a checkbox in the Torbutton preferences window?

Where? Personally, I think we should not expose that on the prefs dialog for now. A preference reachable via about:config seems to be enough for this advanced use-case. Wrt your patch: did you attach the latest one? There are conflicts while applying it to Torbutton master and I was wondering whether I am looking at the right one.

comment:7 in reply to:  6 Changed 5 years ago by arthuredelstein

Replying to gk:

Replying to arthuredelstein:

I've bound the tor circuit display to a boolean pref named "extensions.torbutton.display_circuit". Should we also expose the option with a checkbox in the Torbutton preferences window?

Where? Personally, I think we should not expose that on the prefs dialog for now. A preference reachable via about:config seems to be enough for this advanced use-case.

Sounds good to me.

Wrt your patch: did you attach the latest one? There are conflicts while applying it to Torbutton master and I was wondering whether I am looking at the right one.

Sorry -- it looks like my patch for #13671 needs to be applied first.

comment:8 Changed 5 years ago by arthuredelstein

I've now rewritten both patches so that #13672 can be applied before #13671.

comment:9 Changed 5 years ago by gk

The patch looks good. I'd like to have one thing changed:

We should make sure that we call

syncDisplayWithSelectedTab(true); 
collectIsolationData(myController); 

in start() only if there are no errors while calling controller().

comment:10 in reply to:  9 ; Changed 5 years ago by arthuredelstein

Replying to gk:

The patch looks good. I'd like to have one thing changed:

We should make sure that we call

syncDisplayWithSelectedTab(true); 
collectIsolationData(myController); 

in start() only if there are no errors while calling controller().

I've updated the patch again. If start() throws an error, it will be caught and reported at in the try { bindPrefAndInit(...) }... part. Note: the anonymous error callback function is called asynchronously, which means it will only run after start() returns.

comment:11 in reply to:  10 ; Changed 5 years ago by gk

Replying to arthuredelstein:

Replying to gk:

The patch looks good. I'd like to have one thing changed:

We should make sure that we call

syncDisplayWithSelectedTab(true); 
collectIsolationData(myController); 

in start() only if there are no errors while calling controller().

I've updated the patch again. If start() throws an error, it will be caught and reported at in the try { bindPrefAndInit(...) }... part. Note: the anonymous error callback function is called asynchronously, which means it will only run after start() returns.

Ok, yes. This means that syncDisaplayWithSelectedTab(true) is called even if it (later) turns out we would not have needed that due to the error handler getting called. Seems to be the price for having asynchronous things which is okay to me.

With two more things addressed I am happy::

-if (line.match(/^\d\d\d /) && should get rid of the whitespace (git is complaining about)

-If one clicks on "New Identity" and toggles the pref afterwards weird things are happening while Tor Browser complains about gBrowser.tabContainer being undefined:

20:30 < arthuredelstein> I guess the whole tor-circuit-display.js file is tied to 
                         the current chrome window. With New Identity we destroy 
                         that window and create another. But the preference binding 
                         for the old chrome window is still hanging around. So when 
                         you toggle the pref, it's looking for the old gBrowser's 
                         tabContainer and not finding it.

comment:12 in reply to:  11 Changed 5 years ago by arthuredelstein

Replying to gk:

Replying to arthuredelstein:

Replying to gk:

The patch looks good. I'd like to have one thing changed:

We should make sure that we call

syncDisplayWithSelectedTab(true); 
collectIsolationData(myController); 

in start() only if there are no errors while calling controller().

I've updated the patch again. If start() throws an error, it will be caught and reported at in the try { bindPrefAndInit(...) }... part. Note: the anonymous error callback function is called asynchronously, which means it will only run after start() returns.

Ok, yes. This means that syncDisaplayWithSelectedTab(true) is called even if it (later) turns out we would not have needed that due to the error handler getting called. Seems to be the price for having asynchronous things which is okay to me.

With two more things addressed I am happy::

-if (line.match(/^\d\d\d /) && should get rid of the whitespace (git is complaining about)

-If one clicks on "New Identity" and toggles the pref afterwards weird things are happening while Tor Browser complains about gBrowser.tabContainer being undefined:

20:30 < arthuredelstein> I guess the whole tor-circuit-display.js file is tied to 
                         the current chrome window. With New Identity we destroy 
                         that window and create another. But the preference binding 
                         for the old chrome window is still hanging around. So when 
                         you toggle the pref, it's looking for the old gBrowser's 
                         tabContainer and not finding it.

I've fixed both issues and done some code cleanup. Note this patch needs to be applied before the forthcoming patch for #13671, which I am still working on.

Changed 5 years ago by arthuredelstein

comment:13 Changed 5 years ago by gk

Looks good to me now, thanks! One tiny request: If you are attaching patches, could you keep the old versions? I may get the new versions faster reviewed if I can diff them to the older versions.

comment:14 in reply to:  13 Changed 5 years ago by arthuredelstein

Replying to gk:

Looks good to me now, thanks! One tiny request: If you are attaching patches, could you keep the old versions? I may get the new versions faster reviewed if I can diff them to the older versions.

Sure! I do wish there were a way to mark the old versions as obsolete, because it can be a confusing when there are several attached versions.

Thanks for the reviews!

comment:15 Changed 5 years ago by mikeperry

Keywords: TorBrowserTeam2014R added

comment:16 Changed 5 years ago by mikeperry

Keywords: TorBrowserTeam201411R added; TorBrowserTeam2014R removed

comment:17 Changed 5 years ago by mikeperry

Keywords: TorBrowserTeam201412R added; TorBrowserTeam201411R removed

comment:18 Changed 5 years ago by mikeperry

Keywords: TorBrowserTeam201412 added; TorBrowserTeam201411 removed

comment:19 Changed 5 years ago by gk

Resolution: fixed
Status: needs_reviewclosed

Fixed in commit bdc987b01c977c177d567eb850ce4c98d02e3538 which will make it into 4.5-alpha-2.

Note: See TracTickets for help on using tickets.