Opened 6 years ago

Closed 6 years ago

#11231 closed defect (fixed)

BridgeDB's txrecaptcha returns the "No bridges available!" page if 'captcha_response_field' is blank

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

Description

I was writing unittests for the bridgedb.HTTPServer module and discovered this.

What is happening is:

  • bridgedb.HTTPServer.ReCaptchaProtectedResource.render_POST(request) is called with blank strings for the 'captcha_challenge_field' and 'captcha_response_field' POST arguments.
  • bridgedb.HTTPServer.CaptchaProtectedResource.render_POST(request) is called.
  • bridgedb.HTTPServer.ReCaptchaProtectedResource.checkSolution(request) is called.
  • bridgedb.HTTPServer.CaptchaProtectedResource.extractClientSolution(request) is called, and it returns a tuple of ('', ''), which in Python has a boolean value of True.
  • The empty strings return a bridgedb.txrecaptcha.RecaptchaResponse from bridgedb.HTTPServer.ReCaptchaProtectedResource.checkSolution() without hitting the callback function checkResponse().
  • The RecaptchaResponse also evaluates to True, meaning that checkSolution(request) in render_POST() passes, and the server tries to render the RecaptchaResponse object as the list of bridges to give to the client, resulting in the "No bridges available!" webpage.

That sounds confusing. But I have a unittest to prove it happens, and the solution is really simple:

In bridgedb.CaptchaProtectedResource.render_POST():

-        if self.checkSolution(request):
+        if self.checkSolution(request) is True:

Child Tickets

Change History (4)

comment:1 Changed 6 years ago by isis

This commit adds the unittests to reproduce this, and you can see the results of this test run here.

comment:2 Changed 6 years ago by isis

Status: newneeds_review

The unittests and fixes for this ticket are in my fix/11231-blank-captcha-field branch and my fix/11231-additional-bug-fixes branch. 99% of the work is creating unittests, however there is a bugfix in commit acf37dc00b4d9e2b8e3619a56f1711bb503d9e5b for the problem mentioned in this ticket's description.

The other bugfix is in commit b4f88aded4fd6cc60be0129e1b6d0bd708d392c7 and makes the following changes:

Author:     Isis Lovecruft <isis@torproject.org>
| AuthorDate: 6 hours ago
| Commit:     Isis Lovecruft <isis@torproject.org>
| CommitDate: 6 hours ago
| 
|     Fix txrecaptcha.submit() to always return Deferreds.
|     
|     All returned Deferreds callback with RecaptchaResponse objects. This
|     way, we do not need additional code to check if the result was a
|     Deferred (because, in that case, we would need to add a callback
|     function to extract the RecaptchaResponse object from it and check it),
|     or if it directly returned a RecaptchaResponse.
|     
|     This function now *always* returns Deferred.
| ---
|  lib/bridgedb/txrecaptcha.py | 12 ++++++------
|  1 file changed, 6 insertions(+), 6 deletions(-)
| 
| diff --git a/lib/bridgedb/txrecaptcha.py b/lib/bridgedb/txrecaptcha.py
| index e1e3234..2185112 100644
| --- a/lib/bridgedb/txrecaptcha.py
| +++ b/lib/bridgedb/txrecaptcha.py
| @@ -213,12 +213,12 @@ def submit(recaptcha_challenge_field, recaptcha_response_field,
|      :returns: A :api:`~twisted.internet.defer.Deferred` which will callback
|          with a ``recaptcha.RecaptchaResponse`` for the request.
|      """
| -    if not (recaptcha_response_field and
| -            recaptcha_challenge_field and
| -            len(recaptcha_response_field) and
| -            len(recaptcha_challenge_field)):
| -        return RecaptchaResponse(is_valid=False,
| -                                 error_code='incorrect-captcha-sol')
| +    if not (recaptcha_response_field and len(recaptcha_response_field) and
| +            recaptcha_challenge_field and len(recaptcha_challenge_field)):
| +        d = defer.Deferred()
| +        d.addBoth(_ebRequest)  # We want `is_valid=False`
| +        d.errback(failure.Failure(ValueError('incorrect-captcha-sol')))
| +        return d
|  
|      params = urllib.urlencode({
|          'privatekey': _encodeIfNecessary(private_key),
|  
Last edited 6 years ago by isis (previous) (diff)

comment:3 Changed 6 years ago by sysrqb

Nice catch! These look reasonable (and by that I mean that it's probably safe to merge but I still need to review them more thoroughly). Merge these if you're planning to deploy soon.

Last edited 6 years ago by sysrqb (previous) (diff)

comment:4 Changed 6 years ago by isis

Resolution: fixed
Status: needs_reviewclosed

Merged for 0.1.6.

Note: See TracTickets for help on using tickets.