Opened 5 years ago

Closed 5 years ago

#13882 closed defect (fixed)

Circuit display fails to show new bridge settings

Reported by: arthuredelstein Owned by: arthuredelstein
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Keywords: tbb-torbutton, tbb-circuit-display, TorBrowserTeam201502R, PearlCrescent201502R
Cc: brade, mcs Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description (last modified by arthuredelstein)

Right now, tor-circuit-display.js uses bridges getinfo config-text to query the current bridges, which is inefficient and a little ugly. Also the circuit display doesn't show circuits with a new bridge if bridge settings have been changed. (Updating title to reflect this). Usinggetconf bridge to read bridges in tor circuit display will allow us to fix this.

Child Tickets

Change History (11)

comment:1 Changed 5 years ago by gk

Component: TorbuttonTor Browser
Keywords: tbb-torbutton added

comment:2 Changed 5 years ago by arthuredelstein

Keywords: tbb-circuit-display added

comment:3 Changed 5 years ago by arthuredelstein

Description: modified (diff)
Summary: Use `getconf bridge` to read bridges in tor circuit displayCircuit display fails to show new bridge settings

comment:4 Changed 5 years ago by arthuredelstein

Keywords: TorBrowserTeam201502R added
Status: newneeds_review

comment:5 Changed 5 years ago by mcs

Keywords: PearlCrescent201502R added

comment:6 Changed 5 years ago by mcs

Kathy and I reviewed your patch. This looks like a nice improvement! We have a few questions / comments:

  • In the getBridge function, are you certain that bridge.ID will be upper case?
  • In info.bridgeParser, do we need to add parser support for IPv6 addresses?
  • Will "vanilla" ever show up in the circuit display? I could not make that happen during testing. Tor transport (aka vanilla bridges) resulted in a generic display that looked like "(Unknown country)(IP unknown)".
  • For meek, I expected the circuit display to show 'Bridge: meek" or similar but I saw the same generic display as for Tor bridges.
  • Inside info.getMultipleResponseValues, I think you can remove "filter(utils.identity)" (unless I misunderstand something, it does not change the array that is returned).

Also, this may already be covered in a different ticket, but after I did "New Tor Circuit for this Site" I had no circuit display.

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

Replying to mcs:

Kathy and I reviewed your patch. This looks like a nice improvement! We have a few questions / comments:

Thanks for the review!

  • In the getBridge function, are you certain that bridge.ID will be upper case?

Fixed.

  • In info.bridgeParser, do we need to add parser support for IPv6 addresses?

Likely a useful thing to add in the future. I am adding a ticket: #14939

  • Will "vanilla" ever show up in the circuit display? I could not make that happen during testing. Tor transport (aka vanilla bridges) resulted in a generic display that looked like "(Unknown country)(IP unknown)".

I made a modification so that a "vanilla" bridge just shows the word "(Bridge)" after the country name. For example, I am now seeing Japan (Bridge). I'm not sure why you have seen "Unknown country (IP unknown)".

  • For meek, I expected the circuit display to show 'Bridge: meek" or similar but I saw the same generic display as for Tor bridges.

Yes, meek is a special problem and I need to work on it some more. I'm putting it in a separate ticket so as not to hold up the rest of this fix: #14937.

  • Inside info.getMultipleResponseValues, I think you can remove "filter(utils.identity)" (unless I misunderstand something, it does not change the array that is returned).

.filter(utils.identity) was added to drop items that are null.

Also, this may already be covered in a different ticket, but after I did "New Tor Circuit for this Site" I had no circuit display.

Both my patches for #9442 and #14866 are necessary to get this working.

comment:9 Changed 5 years ago by mcs

Cc: brade mcs added

comment:10 in reply to:  8 Changed 5 years ago by mcs

Replying to arthuredelstein:

And here's the new patch:
https://github.com/arthuredelstein/torbutton/commit/de8cb7a32e0d2a70812322ce1d6a1f4627ef33f0

Everything you said makes sense and the patch looks good.

comment:11 Changed 5 years ago by mikeperry

Resolution: fixed
Status: needs_reviewclosed

Ok, I merged this. I'd be lying if I said I fully understood all of the Task.js usage, though. Thanks for the review, mcs+brade!

Note: See TracTickets for help on using tickets.