The circuit display dropdown should be optional.
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.
Activity
There's another good reason to make this optional and also auto-disabled in the event of weird control port failures: patch #8405 (moved) 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.
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 (moved) 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 (moved) 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.
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.
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 (moved) needs to be applied first.
I've now rewritten both patches so that #13672 (moved) can be applied before #13671 (moved).
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 callingcontroller().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 afterstart()returns.-
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 afterstart()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.tabContainerbeing 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. 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 callingcontroller().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 afterstart()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.tabContainerbeing 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 (moved), which I am still working on.
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!