[tor-bugs] #11127 [BridgeDB]: reCaptcha verification is hardcoded to use plaintext HTTP

[Tor Bug Tracker & Wiki]

#11127: reCaptcha verification is hardcoded to use plaintext HTTP
--------------------------+----------------------------
     Reporter:  isis      |      Owner:  isis
         Type:  defect    |     Status:  needs_review
     Priority:  major     |  Milestone:
    Component:  BridgeDB  |    Version:
   Resolution:            |   Keywords:  bridgedb-https
Actual Points:            |  Parent ID:
       Points:            |
--------------------------+----------------------------

Comment (by sysrqb):

 Replying to [comment:1 isis]:
 > I wrote
 [https://gitweb.torproject.org/user/isis/bridgedb.git/shortlog/refs/heads/fix/11127
 -recaptcha-ssl a Twisted reCaptcha client which only uses SSL]. It also
 does
 [https://gitweb.torproject.org/user/isis/bridgedb.git/blob/refs/heads/fix/11127
 -recaptcha-ssl:/lib/bridgedb/crypto.py#l83 full certificate chain
 verification and cert hostname verification] on a per-request basis.
 >

 Mmmm

 > It's much faster. And it has full unittest coverage. :)
 >

 Mmmmm

 > I left the methods for creating the
 `bridgedb.crypto.SSLVerifyingContextFactory` and
 `twisted.web.client.Agent` separate from the main reCaptcha API functions,
 so we can use them for other requests. (For example, there is another
 blocking HTTP request in `bridgedb.[R|c]aptcha.Recaptcha.get()`, which
 obtains the CAPTCHA image and challenge string from the reCaptcha server,
 that could easily benefit from this as well.) Possible this SSL client-
 side stuff should be separate somewhere, but for now I just put it all in
 `bridgedb.txrecaptcha` (except for
 `bridgedb.crypto.SSLVerifyingContextFactory`.
 >
 > Please review!

 I guess I should actually tell you that I reviewed it!

 {{{
 Ran 263 tests in 215.672s

 PASSED (skips=1, successes=262)
 }}}

 It looks good, I don't have any blockers on the code. A couple minor
 comments on the unit test.

 1) These appear to be the same test, but maybe I'm missing something:
 {{{
     def test_trueResponse(self):
         """A valid API response which states 'true' should result in
         ``RecaptchaResponse.is_valid`` being ``True``.
         """
         responseBody = "true\nsome-reason-or-another\n"
         response = self._test(responseBody, ResponseDone)
         self.assertIsInstance(response, txrecaptcha.RecaptchaResponse)
         self.assertTrue(response.is_valid)
         self.assertEqual(response.error_code, "some-reason-or-another")

     def test_responseDone(self):
         """A valid response body with a ``ResponseDone`` should result in
         ``RecaptchaResponse.is_valid`` which is ``True``.
         """
         responseBody = "true\nsome-reason-or-another\n"
         response = self._test(responseBody, ResponseDone)
         self.assertIsInstance(response, txrecaptcha.RecaptchaResponse)
         self.assertTrue(response.is_valid)
         self.assertEqual(response.error_code, "some-reason-or-another")
 }}}

 2) thee
 {{{
     def test_cbRequest(self):
         """Send a :class:`MockResponse` and check that thee resulting
 protocol
 }}}

 I'm unable to test it, but as soon as the remainder of the patch with an
 implementation in CaptchaProtectedResource looks good, and think it's
 mergable.

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