Implement new reCAPTCHA interface
ClosedPublic
Actions

Authored by thoughtpolice on Feb 3 2017, 4:26 AM.

Details

Reviewers

epriestley
chad

Group Reviewers

Blessed Reviewers

Maniphest Tasks

T12195: Implement "new" reCAPTCHA UI

Commits

rPab923e0a752b: Implement new reCAPTCHA interface

Summary

Fixes T12195. For the past few years, Recaptcha (now part of Google) has supported
a new, "no captcha" one-click user interface. This new UI is stable, doesn't
require any typing or reading words, and can even work without JavaScript (if
the administrator enables it on the Recaptcha side).

Furthermore, the new Recaptcha has a completely trivial API that can be dealt
with in a few lines of code. Thus, the external recaptcha php library is now
gone.

This API is a complete replacement for the old one, and does not require any
upgrade path for users or Phabricator administrators - public and secret keys
for the "new" Recaptcha UI are the exact same as the "classic" Recaptcha. Any
old Recaptcha keys for a domain will continue to work.

Note that Google is currently testing Yet Another new Captcha API, called
"Invisible reCAPTCHA", that will not require user interaction at all. In fact,
the user will not even be aware there is even a captcha form, as far as I
understand. However, this new API is 1) in beta, 2) requires new Recaptcha keys
(so it cannot be a drop-in replacement), and 3) requires more drastic API
changes, as form submission buttons must instead invoke JavaScript code, rather
than a token being passed along with the form submission. This would require far
more extensive changes to the controllers. Maybe when it's several years old, it
can be considered.

Signed-off-by: Austin Seipp <aseipp@pobox.com>

Test Plan

Created a brand-new Phabricator installation, saw the new Captcha UI
on administrator sign up. Logged out, made 5 invalid login attempts, and saw the
new Captcha UI. Reworked the conditional to invert the condition, etc to test
and make sure the API responded properly.

Diff Detail

Repository

rP Phabricator

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

thoughtpolice created this revision.Feb 3 2017, 4:26 AM

Herald added a reviewer: Blessed Reviewers. · View Herald TranscriptFeb 3 2017, 4:26 AM

Herald added a subscriber: Korvin. · View Herald Transcript

Harbormaster completed remote builds in B15512: Diff 41615.Feb 3 2017, 4:27 AM

Here's what the new UI looks like, when Recaptcha is configured on a newly created server. So much cleaner! (Especially on my HiDPI screen where the jaggedness of the old element was really obvious):

I suppose the current integration already lets Google run arbitrary Javascript so this isn't really placing more trust in them.

Let's do this:

Fix the phutil_safe_html() to render with phutil_tag() instead to close the self-XSS-by-administrators hole.
As a general update, lock the Recaptcha-related config options so they can only be edited from the CLI (setLocked(true)). This brings them more inline with other modern policy around not letting arbitrary administrators configure third-party integrations.

That self-XSS isn't really too threatening, but I'd like to get the fix into the release today. I'll plan to commandeer this and clean up the remaining few bits this afternoon (about 8-10 hours from now) if you don't have a chance to get to it before then.

src/view/form/control/AphrontFormRecaptchaControl.php
31	Slightly more modern if written as `$request->getRemoteAddress()`.
42–56	Render these tags using `phutil_tag()`. As written, administrators can self-XSS by putting HTML in `recaptcha.public-key`. This is pre-existing and very not good.

This revision now requires changes to proceed.Feb 3 2017, 2:11 PM

A couple of minor style notes, because I want to feel useful without actually doing anything.

src/view/form/control/AphrontFormRecaptchaControl.php
38	I think will blow up if anything is returned that isn't valid json, so you might as well just juse `$future->resolveJSON()` which also blows up, but has less code (Although it also blows up for non-200 response, IIRC).
40–44	Maybe just `return (bool)idx($json, 'success')` - `idx` defaults to `null` which is falsy.

Both good catches, thanks!

mark recaptcha config options as locked in the UI
fix a well-aged self-admin-XSS in emitting recaptcha tags
use resolvex to handle non-200 status codes in HTTPSFuture
clean up some tiny dead code

doctordance

These are all done now - minor note, I wanted resolvex, not resolveJSON, which only exists (oddly) in ExecFuture and a few other places?

Harbormaster completed remote builds in B15524: Diff 41627.Feb 3 2017, 7:54 PM

thoughtpolice edited the summary of this revision. (Show Details)Feb 3 2017, 7:55 PM

It would probably be reasonable to put resolveJSON on HTTPFuture, it just feels a little flaky (like we should have some kind of adapter layer or something rather than hard-coding JSON into every Future) so I haven't slapped it on everything.

src/view/form/control/AphrontFormRecaptchaControl.php
57	Just `return $tags;` should work fine, too.

This revision is now accepted and ready to land.Feb 3 2017, 7:57 PM

epriestley awarded a token.Feb 3 2017, 7:57 PM

Ah, I see. Yeah, resolvex basically has the exact semantics I wanted anyway (checking the error code). I'll fix the $tags thing right quick and land.

remove unneeded phutil_implode_html

Even better. Oh, and in my latest test just to make sure nothing horribly broke, Google even asked me to click some pictures and select street signs. My robot nature has been determined.

Harbormaster completed remote builds in B15525: Diff 41628.Feb 3 2017, 8:06 PM

Closed by commit rPab923e0a752b: Implement new reCAPTCHA interface (authored by thoughtpolice, committed by thoughtpolice). · Explain WhyFeb 3 2017, 8:06 PM

This revision was automatically updated to reflect the committed changes.