[tbb-bugs] #23136 [Applications/Tor Launcher]: moat integration (fetch bridges for the user)

[Tor Bug Tracker & Wiki]

#23136: moat integration (fetch bridges for the user)
-------------------------------------------+-------------------------------
 Reporter:  mcs                            |          Owner:  brade
     Type:  defect                         |         Status:
                                           |  needs_revision
 Priority:  Very High                      |      Milestone:
Component:  Applications/Tor Launcher      |        Version:
 Severity:  Normal                         |     Resolution:
 Keywords:  TorBrowserTeam201803, ux-team  |  Actual Points:
Parent ID:  #24689                         |         Points:
 Reviewer:                                 |        Sponsor:  Sponsor4
-------------------------------------------+-------------------------------
Changes (by gk):

 * keywords:  TorBrowserTeam201802R, ux-team => TorBrowserTeam201803, ux-
               team
 * status:  needs_review => needs_revision


Comment:

 Okay, three final items:

 16) "in CMETHOD responses" -> "in CMETHOD response"

 17)

 Do you mind flipping
 {{{
         version: this.kMoatVersion,
         type: this.kMoatCheckRequestType,
 }}}
 to
 {{{
         type: this.kMoatCheckRequestType,
         version: this.kMoatVersion,
 }}}
 in `finishFetch()` so that it conforms to the order outlined in the spec?

 18)

 Two points regarding your PT spec compliance:

 a) Setting `TOR_PT_STATE_LOCATION` is required according to the PT spec
 but is missing in `envAddidions`. Is that intentional? It seems it
 benefits from `meek` not caring about that and I wonder what to do. At
 least we should add a comment explaining why we deviate from the spec.

 b)
 {{{
               if (proxyType == "socks4a")
                 this.mMeekClientProxyType = "socks4";
               else if (proxyType == "socks5")
                 this.mMeekClientProxyType = "socks";
               else
                 this.mMeekClientProxyType = proxyType;

 }}}
 is overly lenient IMO. The spec states that the client talks some sort of
 SOCKS. Thus, we should make sure we get either SOCKS4 or SOCKS5 back and
 error out in case anything else shows up. So, probably something like this
 (taking the SOCKS 5 prioritization in the PT spec into account):
 {{{
 if (proxyType == "socks5") {
   this.mMeekClientProxyType = "socks";
 } else if ((proxyType == "socks4a") || (proxyType == "socks4")) {
   this.mMeekClientProxyType = "socks4";
 } else {
   errMsg = "Unexpected proxy type " + proxyType + " in CMETHOD response."
   break;
 }
 }}}

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