Particularly, this is eventually going to change to require you to submit a response to a specific challenge we issued earlier, not just a valid response for the current timestep. Until it does, a lot of the other checks can't be completely effective.

I think this will probably happen reasonably often (log out + log in within a couple minutes, two browsers, or desktop + mobile) in the wild and probably doesn't mean anything ~100% of the time. It could mean something bad but I'm wary about crying wolf and we can't really give the user very good next steps other than "tell someone", and that someone is just going to ignore them after the first three times it happens I think.

(It's maybe worth looking at more awareness around sessions in general, e.g. login alerts, at some point -- those are actionable, and could do a better job of covering this. "Bind session to IP" also has a task somewhere and would help.)

Just that using the omnipotent viewer is inherently somewhat risky/dangerous since some future change could accidentally use it somewhere inappropriate. This has caused almost no actual damage historically, but T13223 is a rare example of a mild policy issue arising from inappropriate use of the omnipotent viewer.

I think "the risk that the omnipotent viewer goes somewhere it shouldn't" and "the risk that the policy system hides a challenge" are both virtually zero, but the former is maybe slightly riskier?

There's some reasonable argument that challenges shouldn't be a policy object, but, today, you can't get normal query behavior unless you extend PolicyAwareQuery and implement the policy interface.

Making them a policy object is also somewhat good if, say, a third party writes a "Any Object Debugger/Inspector" app without thinking about it too carefully, where you put in a PHID and get out object properties (very old versions of Phabricator had this, but it proved difficult to make both secure and useful once policies got more sophisticated), since you can get them indirectly with ObjectQuery.

Is it because this request to generate a challenge is coming in via a GET request?

Yeah -- the write guard is for the case where you load the form for the first time and it says "hey, answer this MFA challenge".

Sometimes that's in response to an actual form POST, but sometimes it's a GET or not a CSRF'd POST. One example is that when you GET into the "VCS Password" settings page, we immediately prompt you for MFA (admittedly, this is a little weird).

magic herbs that ward off evil CSRF spirits

A lot of the way it's hooked up is pretty magic, but the actual implementation is as dumb as rocks.

We tell the query layer "before you do any write, call this method" (in AphrontApplicationConfiguration):

$write_guard = new AphrontWriteGuard(array($request, 'validateCSRF'));

This object is magic and registers itself with the query layer. A more modern implementation would probably be a little less magic, but this was written in like 2011.

The query layer tests each query to see if it starts with "SELECT", "SHOW", or "EXPLAIN". If it doesn't, it determines that it's probably a write and calls the callback:

// NOTE: The opening "(" allows queries in the form of:
//
//   (SELECT ...) UNION (SELECT ...)
$is_write = !preg_match('/^[(]*(SELECT|SHOW|EXPLAIN)\s/', $raw_query);
if ($is_write) {
  // ....
  AphrontWriteGuard::willWrite();
}

The callback checks that $_REQUEST has a valid CSRF token. If it doesn't, it throws an exception.

Why are we calling save() instead of letting newResultFromIssuedChallenges implementations handle that (especially if those implementations might have their own special form of persistence)?

Mostly, just so that we don't have to have a lot of unguarded write boilerplate in every single implementation.

Challenge has a freeform $properties property, so my expectation is that any reasonable MFA factor implementation doesn't need other persistence. If that turns out to be untrue, maybe this was a bad call.

In theory, the write guard mechanism works with any service that does writes, although I'm not sure we ever bothered to implement it in any others. There's normally no way to do a service write without also doing a database write, and service writes are normally not interesting/dangerous (e.g., uploading data to S3 or whatever, I guess?).

Nothing client side / in JS right now. There's one in the background job thing (bulk editor / bulk exporter) but it "animates" by reloading the page. This is probably not terribly difficult to build and the current UI isn't super hot so maybe I'll take a stab at it.

Yeah, I just didn't want to break this by accident between now and when I (hopefully) make it vanish.

It occurs to me that the code in the Countdown app is probably up to the challenge for just showing a little timer.