Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#13576 closed defect (fixed)

Tor Launcher strips "bridge" from the middle of bridge lines

Reported by: dcf Owned by: brade
Priority: Medium Milestone:
Component: Applications/Tor Launcher Version:
Severity: Keywords: tbb-usability, tbb-4.5-alpha, TorBrowserTeam201504, GeorgKoppen201504R
Cc: mcs, isis, sherief, gk Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

I tried configuring a meek bridge on a URL with "bridge" in the domain name.

bridge meek 0.0.2.0:5 url=https://bridge.example.com/

When I clicked Connect, "bridge" disappeared from the URL and broke the configuration.

bridge meek 0.0.2.0:5 url=https://.example.com/

Child Tickets

Attachments (1)

0001-Bug-13576-When-parsing-bridge-defs-only-zap-leading-.patch (1.3 KB) - added by yoah 4 years ago.
Fix for bug #13576

Download all attachments as: .zip

Change History (12)

comment:1 Changed 4 years ago by mcs

Cc: mcs added

comment:2 Changed 4 years ago by mcs

Cc: isis sherief added
Keywords: tbb-usability TorBrowserTeam201503R added
Status: newneeds_review

Thanks for the patch and sorry for the delay! A fix that also includes some additional small improvements is available here:

https://gitweb.torproject.org/user/brade/tor-launcher.git/commit/?h=bug13576-01&id=78f505c2139341e71538e77a975f7df9f81189d0

Isis and Sherief -- Kathy and I value your opinion on these changes, especially:

  1. "bridge " will no longer be shown in Tor Launcher's custom bridges textbox.
  2. Lines within that textbox will not wrap, which means users may need to scroll horizontally to see long bridge lines (but Kathy and I think that is less confusing than wrapping).

comment:3 Changed 4 years ago by mikeperry

Keywords: TorBrowserTeam201504 added

comment:4 Changed 4 years ago by mikeperry

Keywords: TorBrowserTeam201504R added; TorBrowserTeam201503R removed

comment:5 in reply to:  2 ; Changed 4 years ago by isis

Status: needs_reviewneeds_revision

Replying to mcs:

Thanks for the patch and sorry for the delay! A fix that also includes some additional small improvements is available here:

https://gitweb.torproject.org/user/brade/tor-launcher.git/commit/?h=bug13576-01&id=78f505c2139341e71538e77a975f7df9f81189d0

Isis and Sherief -- Kathy and I value your opinion on these changes, especially:

  1. "bridge " will no longer be shown in Tor Launcher's custom bridges textbox.

Shown in the textbox? Or re-added to the beginning of the line? Because if the "bridge " prefix is being stripped once from the beginning of the line in parseAndValidateBridges(), and then not re-added again in setBridgeListElemValue():

@@ -1406,11 +1407,7 @@ function setBridgeListElemValue(aBridgeArray)
     {
       var s = aBridgeArray[i].trim();
       if (s.length > 0)
-      {
-        if (s.toLowerCase().indexOf("bridge") != 0)
-          s = "bridge " + s;
         bridgeList.push(s);
-      }
     }
   }

Then tor won't be able to set the "Bridge" config line.

  1. Lines within that textbox will not wrap, which means users may need to scroll horizontally to see long bridge lines (but Kathy and I think that is less confusing than wrapping).

I agree. (I also added a similar change recently in BridgeDB because of the obfs4 lines being huge.)

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

Status: needs_revisionneeds_review

Replying to isis:

Shown in the textbox? Or re-added to the beginning of the line? Because if the "bridge " prefix is being stripped once from the beginning of the line in parseAndValidateBridges(), and then not re-added again in setBridgeListElemValue():
...
Then tor won't be able to set the "Bridge" config line.

The lines contained in the textbox are not directly passed to the control port via SETCONF.

TorSetConf() inside src/components/tl-protocol.js uses a JS object to construct a SETCONF command.

For the bridge config., getAndValidateBridgeSettings() inside network-settings.js creates the JS object. It contains two properties: "Bridge" (which is an array that contains the lines from the textbox) and "UseBridges" (which is a Boolean). So the "Bridge" part really comes from the property name and does not depend on what is in the textbox (in the current code, parseAndValidateBridges() removes "bridge" everywhere within the text; this patch changes things to remove it on a line-by-line basis).

Does the above make sense?

  1. Lines within that textbox will not wrap, which means users may need to scroll horizontally to see long bridge lines (but Kathy and I think that is less confusing than wrapping).

I agree. (I also added a similar change recently in BridgeDB because of the obfs4 lines being huge.)

I am glad our approach matches yours.

comment:7 Changed 4 years ago by mcs

Cc: gk added
Keywords: tbb-4.5-alpha added

We should consider taking this patch for TB 4.5, assuming it passes review.

comment:8 Changed 4 years ago by gk

Keywords: GeorgKoppen201504R added

comment:9 in reply to:  6 Changed 4 years ago by isis

Replying to mcs:

The lines contained in the textbox are not directly passed to the control port via SETCONF.

[…]

Does the above make sense?


Yes, thanks for taking the time to explain; I missed where the config option name part was coming from.

This patch looks good to me.

comment:10 Changed 4 years ago by gk

Resolution: fixed
Status: needs_reviewclosed

Looks good. Merged in commit a37371ac9a866612e91a791251104184caad41ca.

comment:11 Changed 4 years ago by mcs

Keywords: TorBrowserTeam201504R removed

The commit for this ticket also fixes #10384.

Note: See TracTickets for help on using tickets.