[georgestephanis]

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:

Reason	Meaning
no_nonce_stored	No pending login for that user at all.
expired	Correct value, past its ten-minute expiration.
mismatch	Value did not match the stored hash.

Each failure:

1. Fires a new two_factor_login_nonce_failed action ($user_id, $reason), for sites that want to route these into an audit log, an IDS, or a rate limiter.

2. Writes a line to the PHP error log, including the validated remote address:

   Two-Factor: login nonce verification failed for user 5 (reason: mismatch, remote address: 203.0.113.10).
   

The error log write defaults on for expired and mismatch, and off for no_nonce_stored.

The first two require a nonce to already be stored for the user, which only happens after a successful password check, so their volume is bounded by real login activity. no_nonce_stored is not bounded — any unauthenticated request carrying a guessed user ID reaches it, so defaulting it on would hand anyone an unthrottled write to the error log. It is also the least informative of the three, firing for every stale bookmark and resubmitted form.

The action fires for all three regardless, so anything routing these into an audit log or intrusion detection system sees everything. The new two_factor_log_login_nonce_failures filter receives the per-reason default and can flip it either way — opt no_nonce_stored in, or turn the whole thing off.

The presented value is never written to the log, only the reason it was rejected.

Notes

• No behavior change to authentication — this is purely additive diagnostics.
• REMOTE_ADDR is 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.
• No User-Agent in the log line, deliberately. Every field written today is an integer, a fixed-vocabulary string, or an IP that passed FILTER_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

• New: 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 (data provider asserting the per-reason default reaching the filter).
• Existing nonce tests unchanged; the test class suppresses the error log write so the suite output stays clean.

---

🤖 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>
Co-authored-by: kasparsd <kasparsd@git.wordpress.org>
Co-authored-by: jeffpaul <jeffpaul@git.wordpress.org>
Co-authored-by: mboynes <mboynes@git.wordpress.org>

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

[georgestephanis]

Scope note: this is about the login nonce, not the second-factor code

Worth 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:

	Login nonce	Second-factor code
Meta key	_two_factor_nonce	_two_factor_email_token, TOTP secret, backup codes
What it proves	This browser cleared the password step for user N	The person holds the second factor
Applies to	Every provider	Its own provider only
Delivered via	Hidden wp-auth-nonce field	Email, authenticator app, printed list
Lifetime	10 minutes, single use	two_factor_token_ttl (15 min default) for email; TOTP is time-window based

Despite the name it is not a wp_create_nonce() value — it is bin2hex( random_bytes( 32 ) ), stored as a hash in user meta with an explicit expiration, and deleted on use. It has to be, because during the interstitial there is no session for a WP nonce to bind to. The revalidation flow, where the user is logged in, uses a real WP nonce (two_factor_revalidate_{$user_id}) instead.

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 paths

flowchart 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

Loading

The two behave quite differently on failure:

	Bad nonce	Bad code
Attempt counter	—	Incremented
Exponential backoff	—	Applied
wp_login_failed	—	Fired
Error log entry	New in this PR	— (see below)
Effect on the user's flow	Ends; back to the password step	Continues; fresh nonce, form re-rendered
Effect on the second factor	None — an emailed code stays valid in the inbox, TOTP is unaffected, backup codes unspent	Consumed only on success

That asymmetry is why the nonce side was the gap worth closing. A wrong code is already counted, throttled, and broadcast on wp_login_failed, which every security plugin consumes. A wrong nonce produced no record of any kind.

The reverse gap — no error log line for provider failures — is real but separate, and arguably already covered by wp_login_failed. Happy to open a follow-up adding a matching log line in process_provider() if reviewers want the symmetry.

Aside, for anyone adding a click-to-login email link

The stale feature/email-code-link branch puts both the code and wp-auth-nonce in a GET query string in the email body. If that idea gets revived it needs a different shape — and not only because get_login_nonce() no longer exists. Only the hash is stored now, and authentication_page() is rendered inside the form, so the provider never receives the plaintext; handing it one would deliberately widen where that value travels.

Putting the nonce in a URL has three practical problems:

• Mail gateways prefetch links. Safe Links, Proofpoint, Mimecast and friends GET every URL in an inbound message. That request completes the flow, consumes both the nonce and the code, and issues the cookie into a throwaway scanner context — the real user then clicks a dead link with no explanation.
• Query strings land in access logs in cleartext, plus any CDN, WAF, or proxy in front. This PR goes out of its way never to write the presented value to a log; a GET link would put it in the webserver log regardless.
• It persists in browser history, and any external asset on the landing page leaks it via Referer.

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]

small early feedback about filter docs and a CI failure.

[georgestephanis]

Rebased onto master now that #974 and #972 have landed, so CI here should finally be meaningful — these tests have never actually executed anywhere.

Also narrowed the error log default. Originally every failure was written unconditionally, which is defensible while the nonce deletion is still in place but becomes awkward once #980 removes it: no_nonce_stored is reachable by any unauthenticated request with a guessed user ID, so logging it by default would replace one unthrottled write with another, milder one. It is also the noisiest and least informative reason — every stale bookmark hits it.

So expired and mismatch default on (both require a stored nonce, so real logins bound their volume), no_nonce_stored defaults off, and the filter still reaches all three in either direction. The action is unchanged and fires for everything.

One thing worth being explicit about: this PR does not close #534. That issue asks for an error message for the person who just got silently bounced to the homepage; this writes to the error log, which is a different audience. It is adjacent, not a fix.

[georgestephanis]

Green now, and the first run turned up something worth calling out for review: these tests found a latent bug in an existing test.

test_can_verify_login_nonce expires a nonce by rewriting expiration in usermeta. But create_login_nonce() hashes array( user_id, expiration, key ), and verify_login_nonce() rebuilds that hash from the stored expiration — so editing it changes the hash input, the hashes stop matching, and execution never reaches the expiry check.

Its 'Expired nonce is invalid' assertion has been passing because the hash mismatches, not because the nonce expired. The expiry branch has had no real coverage since f3232d9 introduced hashed storage. Asserting only the return value hid it; asserting the failure reason surfaced it immediately.

The branch is reachable in production — create a nonce, wait past ten minutes, submit the correct key — just not by mutating usermeta. So there is a new create_expired_login_nonce() helper that rehashes for the backdated expiration, and the existing test now uses it alongside the new ones. That fix is in its own commit (cea1ae8) if you would rather it landed separately.

[masteradhoc]

apart from my comments - LGTM!

[Copilot]

🟢 Approval recommended

The change is additive and well-tested, and the only feedback is minor test isolation hardening to avoid hook/filter leakage on failed assertions.

Pull request overview

This PR adds operator-facing diagnostics for failed 2FA login nonce verification in Two_Factor_Core, making nonce failures observable via a new action hook and (optionally) PHP error-log entries to support auditing and troubleshooting without changing authentication behavior.

Changes:

• Emit a new two_factor_login_nonce_failed action with a structured failure reason (no_nonce_stored, expired, mismatch) on nonce verification failures.
• Optionally write a structured PHP error-log line for selected failure reasons, controllable via a new two_factor_log_login_nonce_failures filter.
• Expand PHPUnit coverage to assert action firing behavior and per-reason logging defaults/filtering.

File summaries

File	Description
class-two-factor-core.php	Adds nonce-failure recording (do_action + optional error_log) and calls it from verify_login_nonce() with explicit reasons.
tests/class-two-factor-core.php	Adds helper for creating an actually-expired nonce and new tests covering the new action and logging filter behavior.

Review details

Suppressed comments (1)

tests/class-two-factor-core.php:844

• This test temporarily removes the class-wide suppression filter and adds a recorder filter, but restoration only happens on the happy path. If an assertion fails, the filter state can leak to later tests. Use try/finally to always remove the recorder filter and restore __return_false.

		// Stand in for the class-wide suppression so the default value is observable.
		remove_filter( 'two_factor_log_login_nonce_failures', '__return_false' );
		add_filter( 'two_factor_log_login_nonce_failures', $recorder, 10, 3 );

		$this->assertFalse( Two_Factor_Core::verify_login_nonce( $user_id, $nonce ) );

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

---

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

[Copilot]

🟡 Changes recommended

The new test helper calls ReflectionMethod::setAccessible(true) unconditionally, which can trigger deprecation notices on newer PHP versions and risk CI/test failures.

Get a fresh assessment by requesting another Copilot review.

Review details

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

[masteradhoc]

since 8.1 method has no effect. since 8.5 its deprecated - good call.

[georgestephanis]

Thanks — both addressed.

@masteradhoc: both hooks are now in readme.txt alongside the existing list. The CI failure you hit was the 'expired' → 'mismatch' mismatch, which turned out to be a latent bug in an existing test rather than in the new ones — details in the comment above. Fixed by the create_expired_login_nonce() helper; the suite is green now.

@copilot-pull-request-reviewer: fair point, and taken. test_login_nonce_failure_logging_can_be_filtered is the one that matters — it removes the class-wide error log suppression so the default value is observable, so an assertion failure there left the suppression off and the recorder registered, and the damage would surface in whatever ran next rather than in the test that caused it. That is a genuinely annoying failure to debug.

Restoration is now wrapped in finally, in that test and in the two action tests that register a recorder the same way. Cheap insurance regardless of what the test framework restores between tests.

[masteradhoc]

@georgestephanis #973 (comment) what do you think here?

[georgestephanis]

I think it's mergeable now