[tbb-bugs] #17568 [Tor Browser]: Clean up tor-control-port.js in Torbutton

[Tor Bug Tracker & Wiki]

#17568: Clean up tor-control-port.js in Torbutton
-------------------------------------------------+-------------------------
 Reporter:  gk                                   |          Owner:  tbb-
     Type:  task                                 |  team
 Priority:  Medium                               |         Status:
Component:  Tor Browser                          |  needs_review
 Severity:  Normal                               |      Milestone:
 Keywords:  tbb-torbutton,                       |        Version:
  TorBrowserTeam201511R                          |     Resolution:
Parent ID:                                       |  Actual Points:
  Sponsor:                                       |         Points:
-------------------------------------------------+-------------------------

Comment (by gk):

 Replying to [comment:6 arthuredelstein]:
 > Replying to [comment:4 gk]:
 > > Here are some things I thought about while reading the code in tor-
 control-port.js. I am confused about other things(, too) (especially while
 looking at the spec + the controller.c file) but these might be more for a
 second clean-up (after I had more time to sort them out).
 > >
 > > 1)
 > > {{{
 > >   // GETCONF with a single argument returns results that look like
 > >   // results from GETINFO with multiple arguments."
 > > }}}
 > > Does not seem to be so:
 > > {{{
 > > [11-10 15:33:46] Torbutton INFO: controlPort << getconf bridge
 > >
 > >
 > > [11-10 15:33:46] Torbutton INFO: controlPort >> 250 Bridge
 > > [11-10 15:33:46] Torbutton INFO: controlPort << getinfo version
 config-file
 > >
 > >
 > > [11-10 15:33:46] Torbutton INFO: controlPort >> 250-version=0.2.7.4-rc
 (git-f55d23e1e66e9b0f)
 > > 250-config-file=/path/to/torbrowser/tor-
 browser/Browser/TorBrowser/Data/Tor/torrc
 > > 250 OK
 > > }}}
 >
 > That's a good point. Right now, we're not parsing getconf lines such as
 `250 Bridge`. We'd need to write some extra code for this. I tried to
 clarify the comment to reflect the current situation.

 Hmm... Did you really mean `250[+ ]key=value`? It should be something like
 `250[ -]key=value` instead, no? At least that is what I would guess
 reading `handle_control_getconf()` in tor's control.c.

 > > 4) I wonder why we only have `getInfoMultiple()` given that both
 GETINFO and GETCONF can have multiple keywords (we use them both with just
 one keyword right now as far as I can see which does not explain the
 different treatment of both to me)
 >
 > I never implemented getConfMultiple, but it should be pretty
 straightforward if we need it.

 Yes, but as I said we don't need `getInfoMultiple()` either at the moment
 as far as I can see. Why do we have this additional code then?

 > Replying to [comment:5 gk]:
 > > {{{
 > >   // matchResult finds a single-line result for `250-` or a multi-line
 one for `250+`.
 > > }}}
 > > is interesting as you are not mentioning the `250 ` case although your
 regex is checking for it. But I think the comment is right here (but not
 the code) as the `250 ` case should not make it that far as I guess
 `EndReplyLine` does not contain a `=` (see the `250 Bridge` example) and
 `key` would therefore be `null`. That's one of the things I still need to
 figure out which is a bit tricky as `ReplyText` is not specified in the
 spec.
 >
 > If you activate a bridge in Tor Browser, such as obfs3, then you get:
 >
 > {{{
 > controlPort << getconf bridge
 >
 > controlPort >> 250-Bridge=obfs3 [ip] [fp]
 > 250-Bridge=obfs3 [ip] [fp]
 > 250-Bridge=obfs3 [ip] [fp]
 > 250-Bridge=obfs3 [ip] [fp]
 > 250 Bridge=obfs3 [ip] [fp]
 > }}}
 >
 > Note the final line without a `-`.

 Thanks, I missed that one.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/17568#comment:7>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online