[tor-bugs] #12932 [BridgeDB]: Transport Key-Value pairs should be space separated

[Tor Bug Tracker & Wiki]

#12932: Transport Key-Value pairs should be space separated
--------------------------+-----------------------------------
     Reporter:  sysrqb    |      Owner:  isis
         Type:  defect    |     Status:  needs_review
     Priority:  blocker   |  Milestone:
    Component:  BridgeDB  |    Version:
   Resolution:            |   Keywords:  pt,bridgedb-dist,easy
Actual Points:            |  Parent ID:  #12130
       Points:            |
--------------------------+-----------------------------------
Changes (by isis):

 * status:  accepted => needs_review


Comment:

 I have fix for this in my `fix/12932-pt-args-spaces` branch which fixes
 this.

 -----
 In addition to the one-char bug pointed out above, there were an
 additional four bugs in the legacy parser,
 `bridgedb.Bridges.parseExtraInfoFile()` which I am about to deprecate:

 {{{
        # get the transport line
        if ID and line.startswith("transport "):
            fields = line[10:].split()
            # [ arglist ] field, optional
            if len(fields) >= 3:
                arglist = fields[2:]               # BUGS 1 and 2
                # parse arglist [k=v,...k=v] as argdict {k:v,...,k:v}
                argdict = {}
                for arg in arglist:
                    try: k,v = arg.split('=')      # BUG 3
                    except ValueError: continue    # BUG 4
                    argdict[k] = v
                    logging.debug("  Parsing Argument: %s: %s", k, v)
 }}}

 '''BUGS''':
   1. This assumes the PT arguments are space-separated in the extrainfo
 descriptor. They are not; they are comma-separated.
   2. This would result in parsing the entire, comma-separated group of PT
 arguments into:
      {{{
      {"key1": "a,key2=b,key3=c"}
      }}}
   3. This would produce a `ValueError`, because there's more than one
 `'='` character. (Meaning that the whole set of arguments would be
 discarded due to Bug !#4.)
   4. The whole set of arguments gets discarded, without even so much as a
 log message, if there was more than one argument.

 These bugs additionally have been fixed, and this portion of the legacy
 parser, `bridgedb.Bridges.parseExtraInfoFile()` now looks like this:

 {{{
        # get the transport line
        if ID and line.startswith("transport "):
            fields = line[10:].split()

            if len(fields) >= 3:
                argdict = {}
                allargs = ','.join(fields[2:])
                for arg in allargs.split(','):
                    try:
                        k, v = arg.split('=')
                    except ValueError:
                        logging.warn("  Couldn't parse K=V from PT arg: %r"
 % arg)
                    else:
                        logging.debug("  Parsed PT Argument: %s: %s" % (k,
 v))
                        argdict[k] = v
 }}}

 These are all bug fixes on a single commit,
 4300329a30f3b6aa3e390b140193dd50faa6e03f, from #4568. And I'm still
 deprecating the entire function anyway (for #9380) because the rest of it
 is likely just as full of bugs.

 -----

 The regression test for this requires additional mocking of `transport
 obfs4` lines in the bridge-extrainfo descriptors. This has been added in
 Leekspin-1.1.0, and BridgeDB's version has been bumped to the newest
 version.

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