Fix email resend during 2FA rate limiting

[dknauss]

Summary

In process_provider() (line 1692), pre_process_authentication() is called before the rate-limit check (line 1702). So a rate-limited user clicking "Resend Code" can bypass the rate limit and get a fresh token emailed — defeating the purpose of rate limiting.

This patch adds a guard in providers/class-two-factor-email.php so resend requests do not generate a fresh code while the user is already rate-limited. (It lets the request fall through to the existing error path rather than adding a second error.) It also adds a regression test in tests/class-two-factor-core.php.

Specific Changes

• Block email resend requests while the user is inside the existing two-factor retry delay.
• Let the request fall through to Two_Factor_Core::process_provider() so it returns the normal two_factor_too_fast error instead of generating a fresh code.
• Add a regression test covering the resend path during rate limiting.

Testing

• php -l providers/class-two-factor-email.php
• php -l tests/class-two-factor-core.php
• vendor/bin/phpcs -q providers/class-two-factor-email.php tests/class-two-factor-core.php (shows two pre-existing warnings in tests/class-two-factor-core.php unrelated to this change)

Closes #847

[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: dknauss <dpknauss@git.wordpress.org>
Co-authored-by: kasparsd <kasparsd@git.wordpress.org>

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

[dknauss]

One design note in case maintainers would rather enforce this in core than in the email provider:

I chose the narrow provider-level guard here because issue #847 is specifically about Two_Factor_Email::pre_process_authentication() generating a fresh code while the user is already inside the existing retry delay, and this patch fixes that behavior with the smallest surface-area change.

That said, I can see a reasonable argument for a core-level policy in Two_Factor_Core::process_provider(): if resend/pre-processing actions should never bypass the retry-delay gate, moving the rate-limit check ahead of pre_process_authentication() would make that rule explicit for all providers instead of only fixing the email path.

I did not take that route in this PR because it would change the contract for every provider that uses pre_process_authentication() for non-validate actions, and I wanted to keep #847 scoped to the confirmed email behavior. If you prefer the broader core policy, I’m happy to rework the patch in that direction.

[dknauss]

If a core-level version would be more useful, the patch shape I had in mind is:

1. In Two_Factor_Core::process_provider(), move the existing is_user_rate_limited( $user ) check ahead of $provider->pre_process_authentication( $user ).
2. Keep the current two_factor_too_fast error path unchanged, so rate-limited resend/pre-processing requests surface the same user-facing message as rate-limited verification attempts.
3. Keep the regression coverage, but retarget the assertion as a core policy test: when rate limited, a resend-capable provider must not be allowed to rotate state before the two_factor_too_fast error is returned.

I did not implement that here because it broadens behavior for every provider that uses pre_process_authentication(), whereas this PR only fixes the confirmed email-provider case from #847. But if that broader policy is preferable, I can rework the PR around that shape.

[dknauss]

Further thoughts on the security concern around the old token remaining valid for its full 15-minute TTL (class-two-factor-email.php:159)... An attacker who intercepted/guessed token A could still use it even after the user is rate-limited. The rate limit blocks new code generation but doesn't invalidate the existing code. My commit 80a84c7 doesn't address this.

Recommendation: Invalidate the existing token when rate-limit is hit, or at minimum on each failed validation attempt.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[kasparsd]

This is a great suggestion. I think your proposal to fix this in the plugin core would be a better approach since it would apply equally to all providers.

Would you be able to create a pull request that implements that?

[dknauss]

This is a great suggestion. I think your proposal to fix this in the plugin core would be a better approach since it would apply equally to all providers.

Would you be able to create a pull request that implements that?

Yep, I'll do that.

[dknauss]

New PR: #859

[dknauss]

Follow-up on the above: I implemented this recommendation in PR #917 (originally #859) but then reverted it after testing revealed a worse problem.

Deleting the token when the rate-limit fires causes login_html() → authentication_page() to immediately regenerate and email a new code on every blocked POST — because authentication_page() checks for a token and sends one if none is found. The resend button itself, the verify button, every blocked submission: each one silently triggered a new email. The flooding vector just moved one step later in the call chain.

The replay concern in the original recommendation is also covered by the existing TTL: the default two_factor_email_token_ttl and the max rate-limit cap are both 15 minutes, so a captured token expires at roughly the same time the lockout lifts. Token deletion is unnecessary and counterproductive here.

The resend button visibility during lockout is addressed separately in PR #921.