Add diagnostics for failed login nonce verification - #973
Add diagnostics for failed login nonce verification#973georgestephanis wants to merge 3 commits into
Conversation
Failed login nonce verification is currently silent: the request is redirected to the home page and nothing is recorded anywhere. An administrator has no way to tell whether a user's report of being bounced back to the login screen was a stale tab, two sessions racing each other, or something that deserves a closer look. #534 covers the user-facing half of that gap; this covers the operator-facing half. verify_login_nonce() now reports every failure and distinguishes three cases: no pending login for that user, a correct-but-expired value, and a value that did not match. Each failure fires a new `two_factor_login_nonce_failed` action, so sites can route these into an audit log, an IDS, or a rate limiter, and writes a line to the PHP error log by default so they are visible with no configuration. The error log write can be turned off with the `two_factor_log_login_nonce_failures` filter. The presented value is never written to the log. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Scope note: this is about the login nonce, not the second-factor codeWorth spelling out, since "nonce" and "code" get used interchangeably in discussion and this PR only touches one of them. Completing a second factor requires two independent secrets:
Despite the name it is not a This PR adds logging to the nonce check only. It is provider-agnostic — a TOTP-only user has a login nonce too — but a wrong TOTP code never reaches the code path this PR touches. The two failure pathsflowchart TD
A["Request to validate_2fa"] --> B{"verify_login_nonce()"}
B -->|fails| C["Stored nonce discarded<br/><i>(existing behavior)</i>"]
C --> D["<b>NEW:</b> record reason<br/>+ fire two_factor_login_nonce_failed"]
D --> E["Redirect to home_url()<br/>flow ends, no code examined"]
B -->|passes| F{"provider->validate_authentication()"}
F -->|fails| G["Increment USER_FAILED_LOGIN_ATTEMPTS_KEY<br/>stamp USER_RATE_LIMIT_KEY"]
G --> H["Fire wp_login_failed"]
H --> I["Mint a fresh nonce,<br/>re-render form — user continues"]
F -->|passes| J["Delete nonce, clear counters,<br/>issue auth cookie"]
style D fill:#dbf5dc,stroke:#2da44e,color:#1a1a1a
The two behave quite differently on failure:
That asymmetry is why the nonce side was the gap worth closing. A wrong code is already counted, throttled, and broadcast on The reverse gap — no error log line for provider failures — is real but separate, and arguably already covered by Aside, for anyone adding a click-to-login email linkThe stale Putting the nonce in a URL has three practical problems:
The shape that survives all three: an opaque single-use token in the link, landing on an interstitial with a POST-only confirm button, with nothing consumed until the user actually clicks it. |
masteradhoc
left a comment
masteradhoc
left a comment
There was a problem hiding this comment.
small early feedback about filter docs and a CI failure.
| * @param int $user_id The user ID the nonce was presented for. | ||
| * @param string $reason One of 'no_nonce_stored', 'expired', or 'mismatch'. | ||
| */ | ||
| do_action( 'two_factor_login_nonce_failed', $user_id, $reason ); |
There was a problem hiding this comment.
Please add this to the readme.txt - every other public hook in the plugin is listed there already.
| * @param int $user_id The user ID the nonce was presented for. | ||
| * @param string $reason One of 'no_nonce_stored', 'expired', or 'mismatch'. | ||
| */ | ||
| if ( ! apply_filters( 'two_factor_log_login_nonce_failures', true, $user_id, $reason ) ) { |
There was a problem hiding this comment.
Same ask for readme.txt
| * @param bool $expire_nonce Whether to backdate the nonce's expiration. | ||
| * @param bool $send_valid_key Whether to present the real key or a bogus one. | ||
| */ | ||
| public function test_failed_login_nonce_fires_action( $expected_reason, $create_nonce, $expire_nonce, $send_valid_key ) { |
There was a problem hiding this comment.
Currently results in a Failure:
1) Test_ClassTwoFactorCore::test_failed_login_nonce_fires_action with data set "past expiration" ('expired', true, true, true)
The failure action fires once with the expected user and reason
Failed asserting that two arrays are identical.
--- Expected
+++ Actual
@@ @@
Array &0 (
0 => Array &1 (
0 => 8
- 1 => 'expired'
+ 1 => 'mismatch'
)
)
/var/www/html/wp-content/plugins/two-factor/tests/class-two-factor-core.php:762
3 participants
Description
Failed login nonce verification is currently silent.
validate_login_form_2fa()redirects to the home page and nothing is recorded anywhere, so when a user reports being bounced back to the login screen there is no way for an administrator to tell what happened.#534 describes the user-facing half of this — the person logging in gets no explanation. This PR covers the operator-facing half: making the failures visible in the log so they can be diagnosed and, where the volume warrants it, acted on. It does not close #534, which is also asking for an error message on the login form.
A handful of these are entirely routine (a stale browser tab, the back button, two sessions on a shared account racing each other, as in #534). A sustained run of them against a single account is a different signal, and today nothing distinguishes the two.
What changed
verify_login_nonce()now reports every failure, distinguishing three cases:no_nonce_storedexpiredmismatchEach failure:
Fires a new
two_factor_login_nonce_failedaction ($user_id,$reason), for sites that want to route these into an audit log, an IDS, or a rate limiter.Writes a line to the PHP error log, including the validated remote address:
Logging is on by default so the failures are visible without any configuration. Sites that would rather not carry the volume, or that handle the action themselves, can opt out with the new
two_factor_log_login_nonce_failuresfilter.The presented value is never written to the log, only the reason it was rejected.
Notes
REMOTE_ADDRis unreliable behind a proxy or load balancer; the docblock says so, and the action hook is the better place for sites that can resolve provenance properly.User-Agentin the log line, deliberately. Every field written today is an integer, a fixed-vocabulary string, or an IP that passedFILTER_VALIDATE_IP— nothing attacker-controlled. A raw UA would be the first, and a newline in one lets a caller forge additional lines that look exactly like ours. It is also trivially spoofed, roughly triples the line length, and pairs with the IP as a browser fingerprint. Sites that want it can capture it from the action hook with their own sanitization and retention rules.Partially addresses #534.
Testing
test_failed_login_nonce_fires_action(data provider covering all three reasons),test_successful_login_nonce_does_not_fire_action,test_login_nonce_failure_logging_can_be_filtered.