Opened 6 years ago

Closed 6 years ago

#11377 closed defect (fixed)

New BridgeDB GimpCaptcha should be case insensitive (or should say it's sensitive)

Reported by: wfn Owned by: isis
Priority: Medium Milestone:
Component: Circumvention/BridgeDB Version:
Severity: Keywords: bridgedb-dist, bridgedb-https, easy, gimp-captcha, bridgedb-0.1.7
Cc: sysrqb, isis Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Sherief reports that the new captcha is case sensitive (it is), whereas the previous (re)captcha was not (the check was done remotely, remote resource must have implicitly done case-insensitive check.)

I assume this is the relevant line: https://gitweb.torproject.org/bridgedb.git/blob/HEAD:/lib/bridgedb/captcha.py#l206

Should probably either do a case-insensitive comparison, or should tell users it's case sensitive (I suppose the former makes more sense.)

Child Tickets

Attachments (1)

11377.patch (927 bytes) - added by wfn 6 years ago.
Patch to make GimpCaptcha check be case insensitive

Download all attachments as: .zip

Change History (6)

comment:1 Changed 6 years ago by Sherief

Cc: sysrqb added
Owner: set to isis
Status: newassigned

comment:2 Changed 6 years ago by isis

Cc: isis added
Keywords: bridgedb-dist bridgedb-https easy gimp-captcha added

I thought that most CAPTCHA system are case sensitive, but I could be wrong. I'd accept a patch for either of the proposed fixes.

comment:3 in reply to:  2 ; Changed 6 years ago by wfn

Status: assignedneeds_review

Replying to isis:

I thought that most CAPTCHA system are case sensitive, but I could be wrong.

Yeah, those captcha systems are not intuitive at all, right? fwiw, I'd have also guessed that most/many of them are case sensitive.

I'd accept a patch for either of the proposed fixes.

Don't know if this helps, but in case it does: I'm linking to a commit (in a separate bug branch, bug11377_gimpcaptcha) (+ attaching its patch file) that makes the check be case insensitive (it's just one line / a few bytes. Doesn't break things.)

https://github.com/wfn/bridgedb/commit/dd9e75ba234d2d4aad90aedb0bf163d8bb13811b

I'm still learning to tame BridgeDB. Running all tests seem to take a long while. But I did run the test_createChallenge_decryptedAnswerMatches() from bridgedb.test.test_captcha.GimpCaptchaTests.

If this goes well, I assume it'd be nice to create a separate test as well, something like test_createChallenge_decryptedAnswerMatchesDifferentCases() or so?

Changed 6 years ago by wfn

Attachment: 11377.patch added

Patch to make GimpCaptcha check be case insensitive

comment:4 in reply to:  3 Changed 6 years ago by isis

Replying to wfn:

Replying to isis:

I thought that most CAPTCHA system are case sensitive, but I could be wrong.

Yeah, those captcha systems are not intuitive at all, right? fwiw, I'd have also guessed that most/many of them are case sensitive.

I'd accept a patch for either of the proposed fixes.

Don't know if this helps, but in case it does: I'm linking to a commit (in a separate bug branch, bug11377_gimpcaptcha) (+ attaching its patch file) that makes the check be case insensitive (it's just one line / a few bytes. Doesn't break things.)

https://github.com/wfn/bridgedb/commit/dd9e75ba234d2d4aad90aedb0bf163d8bb13811b


Awesome, thanks! I'll post my review in a separate comment.

I'm still learning to tame BridgeDB. Running all tests seem to take a long while. But I did run the test_createChallenge_decryptedAnswerMatches() from bridgedb.test.test_captcha.GimpCaptchaTests.


Most of the time spent during testing, as well as most of the time spent when "BridgeDB is down" (i.e. when I reply with "BridgeDB is single-threaded (#5232) and is parsing millions of descriptors"), is within the same bridgedb.Stability.addOrUpdateBridgeHistory() function (see #10724). This function is pretty brutal on CPU and memory, is blocking, and it needs to runs thousands and thousands of times. The algorithm within that function has a time complexity increasing linearithmically, relative to the number of bridges and timestamps already within the database.

tl;dr: I just merged sysrqb's patches for #5232 into the common develop branch, so all test runs and server startups/restarts should be significantly faster now.

If this goes well, I assume it'd be nice to create a separate test as well, something like test_createChallenge_decryptedAnswerMatchesDifferentCases() or so?


Nope, thanks though! The test_createChallenge_decryptedAnswerMatches test will cover it.

comment:5 Changed 6 years ago by isis

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

Okay, merged for 0.1.7, thanks wfn!

Note: See TracTickets for help on using tickets.