Fix. 2FA. Flow updates. by alexandergull · Pull Request #761 · CleanTalk/security-malware-firewall

alexandergull · 2026-10-23T10:14:10Z

https://app.doboard.com/1/task/60469

Copilot

🟡 Changes recommended

Concurrency races and multibyte user-agent handling leave replay protection and attempt limits bypassable.

3 open issues

What changed in this PR

This PR hardens the plugin’s two-factor authentication flow by binding code verification to a successful password check and adding replay protection and per-user attempt limits.

Changes:

Adds password-step tokens, replay checks, lockout handling, and related login messages.
Strengthens secret and email-code generation and restricts access to activated application secrets.
Adds regression tests for authentication, setup, and rate limiting.

File	Description
`tests/lib/CleantalkSP/SpbctWP/RateLimiter/TestSpbcUserRateLimiter.php`	Tests per-user limits, resets, IP changes, and long headers.
`tests/lib/CleantalkSP/SpbctWP/G2FA/TestGoogleAuthenticator.php`	Tests code generation and secret format.
`tests/Inc/TwoFactorAuthTest.php`	Covers password-step binding, replay rejection, lockouts, and setup.
`security-malware-firewall.php`	Defines authentication attempt limits and lock duration.
`lib/CleantalkSP/SpbctWP/SpbcRateLimit/SpbcUserRateLimiter.php`	Adds user-keyed rate limiting and counter reset support.
`lib/CleantalkSP/SpbctWP/G2FA/GoogleAuthenticator.php`	Generates longer secrets using secure randomness.
`inc/spbc-auth.php`	Integrates authentication safeguards and updates setup endpoints.

_{Review effort: Balanced}

Truncating UA by bytes could split a multibyte character; wpdb rejects queries with invalid text and the limiter fails open, so the per-user 2FA attempts cap could be bypassed. Keep only printable ASCII. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Concurrent requests with the same step token could overwrite each other's fail counter and both pass the TOTP replay check. Now each attempt is reserved by compare-and-swap of the token record before the code check, and a correct code logs in only if this request deletes the reserved record. The TOTP time step is marked used after the claim. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

🟡 Changes recommended

Concurrent requests can restore consumed password-step tokens or replace a newly activated authenticator secret.

5 open issues

_{Review effort: Balanced}

Copilot

🟡 Changes recommended

Replay protection remains non-atomic, password resets leave pending login tokens valid, and successful-login cleanup can invalidate a newer challenge.

3 open issues

1 previously missed issue

In code that hasn't changed since last review

Avoid deleting a concurrent login's fresh step token

inc/spbc-auth.php:1416

Both successful authentication branches have already deleted the matching step record with spbc_2fa__step_token_claim(). Calling spbc_2fa__step_token_destroy() here deletes whatever record currently exists for the user, including a fresh token issued by a concurrent password login after that claim. This invalidates the other browser's new challenge. Clear only this browser's step cookie here instead of deleting user meta again.

4 resolved since last review

_{Review effort: Balanced}

A step token and e-mail code issued for the old password could still complete the login after the password reset. Revoke both on after_password_reset. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

The step record is already consumed by the atomic claim, so deleting user meta again on success could revoke a fresh challenge issued to a password login in another browser. Clear only this browser's cookie. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Fix. 2FA. Flow updates. * Fix. 2FA. Keep user rate limiter UA charset-safe. Truncating UA by bytes could split a multibyte character; wpdb rejects queries with invalid text and the limiter fails open, so the per-user 2FA attempts cap could be bypassed. Keep only printable ASCII. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Fix. 2FA. Atomic step token attempts and claim. Concurrent requests with the same step token could overwrite each other's fail counter and both pass the TOTP replay check. Now each attempt is reserved by compare-and-swap of the token record before the code check, and a correct code logs in only if this request deletes the reserved record. The TOTP time step is marked used after the claim. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Fix. 2FA. Revoke pending challenge on password reset. A step token and e-mail code issued for the old password could still complete the login after the password reset. Revoke both on after_password_reset. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Fix. 2FA. Keep concurrent login's fresh step token on success. The step record is already consumed by the atomic claim, so deleting user meta again on success could revoke a fresh challenge issued to a password login in another browser. Clear only this browser's cookie. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Fix. 2FA. Flow updates.

cc59c28

alexandergull requested a balanced review from Copilot October 23, 2026 10:14

Copilot started reviewing on behalf of alexandergull October 23, 2026 10:14

View session

Copilot AI reviewed Oct 23, 2026

View reviewed changes

Comment thread inc/spbc-auth.php Outdated

Comment thread inc/spbc-auth.php Outdated

Comment thread lib/CleantalkSP/SpbctWP/SpbcRateLimit/SpbcUserRateLimiter.php Outdated

alexandergull and others added 2 commits October 23, 2026 15:40

alexandergull requested a balanced review from Copilot October 23, 2026 10:49

Copilot started reviewing on behalf of alexandergull October 23, 2026 10:49

View session

Copilot AI reviewed Oct 23, 2026

View reviewed changes

Comment thread inc/spbc-auth.php Outdated

Comment thread inc/spbc-auth.php

alexandergull requested a balanced review from Copilot October 23, 2026 10:57

Copilot started reviewing on behalf of alexandergull October 23, 2026 10:58

View session

Copilot AI reviewed Oct 23, 2026

View reviewed changes

Comment thread inc/spbc-auth.php

Comment thread inc/spbc-auth.php

alexandergull and others added 2 commits October 23, 2026 16:27

Fix. 2FA. Revoke pending challenge on password reset.

efcdf5b

A step token and e-mail code issued for the old password could still complete the login after the password reset. Revoke both on after_password_reset. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

alexandergull requested review from Glomberg and svedge October 23, 2026 11:42

Glomberg approved these changes Oct 23, 2026

View reviewed changes

svedge approved these changes Oct 26, 2026

View reviewed changes

AntonV1211 merged commit d35fbdb into dev Oct 26, 2026
8 checks passed

Navigation Menu

Repository navigation

Fix. 2FA. Flow updates. - #761

Fix. 2FA. Flow updates.#761
AntonV1211 merged 5 commits into
devfrom
2fa-sec-flow.ag

alexandergull commented Oct 23, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

alexandergull commented Oct 23, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants