Only clear the login nonce once it has expired

[georgestephanis]

Fixes the destructive half of verify_login_nonce(): an unrecognized nonce no longer discards the pending one.

The behavior

verify_login_nonce() currently deletes the stored login nonce on any verification failure, including a simple mismatch:

if ( $hashes_match && time() < $login_nonce['expiration'] ) {
    return true;
}

// Require a fresh nonce if verification fails.
self::delete_login_nonce( $user_id );

Its one caller, validate_login_form_2fa(), runs that check before process_provider() — which is where rate limiting lives. So the destructive path is the only one in the flow with nothing throttling it, and it needs no authentication to reach. Anyone can end another user's in-progress 2FA login by submitting a garbage nonce with a guessed user ID. The victim isn't locked out, but their half-finished login dies and they start over at the password prompt.

Why the delete was there

Not as a considered defense. The original implementation collapsed both failure cases into one branch:

if ( $nonce != $login_nonce['key'] || time() > $login_nonce['expiration'] ) {
    self::delete_login_nonce( $user_id );
    return false;
}

Deleting an expired nonce is reasonable garbage collection. Deleting on mismatch came along for the ride. The 2021 hashing refactor (f3232d9) split the condition apart and added the comment "Require a fresh nonce if verification fails" — describing the existing behavior rather than arguing for it.

Why dropping it costs nothing

It isn't brute-forceable. create_login_nonce() uses bin2hex( random_bytes( 32 ) ) — 256 bits, inside a ten minute window.

The nonce alone isn't sufficient. Even a correct one only reaches the second factor prompt; process_provider() still demands the real TOTP code, email code, or backup code. A guessed nonce puts an attacker exactly where knowing the password would.

Rotation already covers it. On a failed second factor, validate_login_form_2fa() calls create_login_nonce() again and overwrites the stored value. The "one attempt per prompt" property delete-on-mismatch would supposedly provide is already provided by rotation.

There's no replay surface to protect. The stored hash binds user_id and expiration, so a nonce can't be reused for another user or have its window extended.

The change

if ( $hashes_match ) {
    if ( time() < $login_nonce['expiration'] ) {
        return true;
    }

    // Expired nonces can never succeed again; clear them.
    self::delete_login_nonce( $user_id );

    return false;
}

// A value we never issued. Leave the pending nonce alone.
return false;

verify_login_nonce() has exactly one caller and it treats every non-true return identically, so nothing downstream shifts.

Tests

test_invalid_nonce_deletes_valid_nonce locks in the behavior being removed, so it inverts:

• Renamed to test_invalid_nonce_preserves_valid_nonce; the second assertion flips to assertTrue, plus a usermeta check that the pending nonce survived.
• New test_expired_nonce_is_deleted covers the retained delete path.
• test_can_verify_login_nonce drops a now-stale "Must create a new one since incorrect nonces deletes them" workaround.

Stacked on #973

This branches off #973 and reuses its log_login_nonce_failure() call sites, so the reason codes stay accurate across the split. Review and merge #973 first — the diff shown here includes its commits.

---

🤖 Generated with Claude Code

[github-actions]

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: georgestephanis <georgestephanis@git.wordpress.org>
Co-authored-by: masteradhoc <masteradhoc@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[Copilot]

🔵 Needs a closer look

As implemented, an expired stored nonce may remain in usermeta (and continue triggering mismatch logging) when presented with a wrong key; expiry cleanup should not depend on providing the correct nonce value.

Pull request overview

This PR adjusts Two_Factor_Core’s login nonce verification behavior to avoid deleting a pending (valid) login nonce on nonce mismatch, reducing the ability for unauthenticated requests to disrupt in-progress 2FA logins. It also adds/updates tests around nonce failure reasons, logging hooks, and the preserved-on-mismatch behavior (stacked with the diagnostics from #973).

Changes:

• Update verify_login_nonce() to delete stored nonces only in the expired path, and add reasoned diagnostics via two_factor_login_nonce_failed + two_factor_log_login_nonce_failures.
• Add/adjust PHPUnit coverage for mismatch preservation, expired nonce deletion, and per-reason logging defaults.
• Add a test-suite-level filter to suppress nonce-failure error log output during tests.

File summaries

File	Description
class-two-factor-core.php	Refines login nonce verification semantics and adds structured logging/action hooks for failure diagnostics.
tests/class-two-factor-core.php	Updates and extends nonce verification tests (mismatch preservation, expiry cleanup, and diagnostics/logging behavior).

Review details

Suppressed comments (1)

class-two-factor-core.php:1383

• Expired login nonces are only deleted when the correct key is presented (the hashes_match branch). If the nonce has expired but the request presents an incorrect/garbage key (the common case for abandoned logins and any probing), the code logs mismatch and leaves an already-expired nonce in usermeta indefinitely (until the next successful password phase overwrites it). That also means mismatch logging can continue long after expiry. Consider checking expiration up front and deleting/logging expired regardless of whether the key matches, then doing the hash comparison only for non-expired nonces.

		$unverified_nonce = array(
			'user_id'    => $user_id,
			'expiration' => $login_nonce['expiration'],
			'key'        => $nonce,

• Files reviewed: 2/2 changed files
• Comments generated: 0
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[georgestephanis]

Good catch — adopted.

Gating the expiry cleanup on hashes_match was wrong, and the case you describe is the common one: someone starts a login, abandons it, and the stale nonce then sits in usermeta until their next password success overwrites it. Worse, every probe against that user logged mismatch instead of expired, so the reason code was actively misleading about what was in the database.

Checking expiry first is also just simpler, and it means the hash is never computed for a nonce that cannot succeed:

if ( time() >= $login_nonce['expiration'] ) {
    self::log_login_nonce_failure( $user_id, 'expired' );
    self::delete_login_nonce( $user_id );

    return false;
}

$unverified_hash = self::hash_login_nonce( $unverified_nonce );

if ( $unverified_hash && hash_equals( $login_nonce['key'], $unverified_hash ) ) {
    return true;
}

self::log_login_nonce_failure( $user_id, 'mismatch' );

return false;

Worth being explicit that this does not weaken what the PR is for. Deleting an expired nonce is not destructive — it was already dead, and no request could have used it. The behavior being removed is deleting a live nonce because someone presented the wrong value against it, which is what let an unauthenticated request end another user's in-progress login. Expiry cleanup and mismatch preservation are independent, and checking expiry first is what keeps them that way.

Coverage added for exactly the case you named: a new 'past expiration, any value' data set asserting the reason is expired rather than mismatch when the key is wrong, and test_expired_nonce_is_deleted now runs a second pass with a garbage key to confirm cleanup does not depend on the caller knowing it.

[Copilot]

🟡 Changes recommended

Address log/hook flooding and clarify nonce failure-reason semantics.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (1)

class-two-factor-core.php:1387

• This early expiry check reports expired for every submitted value once the stored nonce has expired, including a wrong key. However, log_login_nonce_failure() documents expired as the correct value being past expiration, and consumers of its action/filter can rely on that distinction. Since the new tests intentionally cover an arbitrary value as expired, update that reason contract and related documentation, or preserve the hash check before assigning the reason.

		if ( time() >= $login_nonce['expiration'] ) {
			self::log_login_nonce_failure( $user_id, 'expired' );

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Lite

[masteradhoc]

LGTM!