search

achiurizo / rack-recaptcha

Rack Middleware for CAPTCHA verification via Recaptcha API
twitter.com/arthur_chiu
MIT License
151 stars 23 forks source link

Request for error messages in _call #7

Closed eric-hu closed 13 years ago

eric-hu commented 13 years ago

I noticed my app was rejecting all recaptcha's. When I looked into the rack-recaptcha source, I noticed that the IF statement on line 32 (reproduced below) doesn't do anything if any of the params are set up incorrectly.

In particular, setting up the paths is a bit finicky and it would really streamline using this gem if it would report some errors. This doesn't seem too difficult and I think I could make the changes if you don't have the time, but I just wanted to make sure this is the way you'd want it done. I was thinking of just generating some error messages and putting them in request.env['recaptcha.msg'], since you're already putting the callback messages in there.

if request.params[CHALLENGE_FIELD] and
    request.params[RESPONSE_FIELD] and (not @paths or @paths.include?(request.path))
achiurizo commented 13 years ago

after looking at the conditional, i can't recall why the paths needed to be checked. I've removed them and released 0.4.0 that doesn't requires paths any longer. Checking to see if both recaptcha params seems sufficient enough to decide whether or not the request is meant for interception by rack-recaptcha.

if you still want to go ahead and add in your error messages, I am not against it. Just send me a pull request and add tests and i'll get them merged. thanks

eric-hu commented 13 years ago

As I understand, you were checking paths to allow for a whitelist approach. If I setup a captcha to work on a page like /users/login, without path checking it could work on a page like /users/edit. You may be right that this is unnecessary--I can't think of how a malicious user could abuse this to submit a captcha to a specific path. I assume you were checking @paths first so that you didn't a nil.include? error. I realize now that I could've just left @paths as nil to take a non-whitelist approach from the start. If you don't think the path checking is necessary then I'll close the ticket

achiurizo commented 13 years ago

Yeah the paths checking doesn't seem neccessary. This allows more flexibility with rack-recaptcha and removes the extra hassle of checking paths.

eric-hu commented 13 years ago

yeah, I agree. Thanks for being so responsive on your gem!

achiurizo commented 13 years ago

no problem, hope rack-recaptcha has been of use to you