Implement new reCAPTCHA interface
ClosedPublic

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

Details

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
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
thoughtpolice created this revision.Fri, Feb 3, 4:26 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):

epriestley requested changes to this revision.Fri, Feb 3, 2:11 PM

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.Fri, Feb 3, 2:11 PM
avivey added a subscriber: avivey.Fri, Feb 3, 2:33 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!

thoughtpolice marked 3 inline comments as done.
  • 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
thoughtpolice marked an inline comment as done.Fri, Feb 3, 7:54 PM

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

thoughtpolice edited the summary of this revision. (Show Details)Fri, Feb 3, 7:55 PM
epriestley accepted this revision.Fri, Feb 3, 7:57 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.Fri, Feb 3, 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
thoughtpolice marked an inline comment as done.Fri, Feb 3, 8:06 PM

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.

This revision was automatically updated to reflect the committed changes.

I like when it asks me to choose the waterfalls. :)