Opened 5 years ago

Closed 5 years ago

#11127 closed defect (fixed)

reCaptcha verification is hardcoded to use plaintext HTTP

Reported by: isis Owned by: isis
Priority: High Milestone:
Component: Obfuscation/BridgeDB Version:
Severity: Keywords: bridgedb-https, bridgedb-0.1.5
Cc: sysrqb, isis Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

The reCaptcha API which BridgeDB uses hardcodes the CAPTCHA verification submission to occur over plaintext HTTP. See their source code and grep for 'verify'.

Their API is also blocking. BridgeDB would probably go faster if it were asynchronous.

Child Tickets

Change History (5)

comment:1 Changed 5 years ago by isis

Status: newneeds_review

I wrote a Twisted reCaptcha client which only uses SSL. It also does full certificate chain verification and cert hostname verification on a per-request basis.

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

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!

Last edited 5 years ago by isis (previous) (diff)

comment:2 in reply to:  1 ; Changed 5 years ago by sysrqb

Replying to isis:

I wrote a Twisted reCaptcha client which only uses SSL. It also does 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.

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

Replying to sysrqb:

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


Thanks! :D

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:


Oopsies. That is not supposed to be the same. One of those is supposed to be something else, like a twisted.web.client.ConnectionLost instead of ResponseDone...

Good catch.

2) thee

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


It's expecting an aristocratic protocol, you know? Like twisted.protocols.stateful.HerRoyalHighnessTCP.

Kidding! Thanks, I'll fix that one too. :)

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.


There's now a [ branch] which is exactly these commits, unchanged, but they are rebased on top of the other CAPTCHA branch for #10809. There is one additional commit to use bridgedb.txrecaptcha in bridgedb.HTTPServer and bridgedb.captcha.

All the tests still pass, but we currently have like zero testing for bridgedb.HTTPServer. I am unsure if I should focus time on that before getting started with the database backend improvements.

Last edited 5 years ago by isis (previous) (diff)

comment:4 Changed 5 years ago by isis

Aristocratic typo and duplicated unittest fixed.

comment:5 Changed 5 years ago by isis

Keywords: bridgedb-0.1.5 added
Resolution: fixed
Status: needs_reviewclosed

Merged for version 0.1.5 in this commit.

Note: See TracTickets for help on using tickets.