feature: 2fa with authenticator apps#1852
Open
sapayth wants to merge 4 commits intoweDevsOfficial:developfrom
Open
feature: 2fa with authenticator apps#1852sapayth wants to merge 4 commits intoweDevsOfficial:developfrom
sapayth wants to merge 4 commits intoweDevsOfficial:developfrom
Conversation
WalkthroughAdds a complete Two‑Factor Authentication subsystem (TOTP method, QR rendering, secret encryption, per‑user storage, rate limiting, enrollment/login controllers, registry/interface, admin/account UI and assets), generalizes settings card selector for gateways/methods, updates assets/styles/scripts, raises minimum PHP to 7.4, and wires the 2FA manager into plugin bootstrap. Changes2FA Core & Storage
TOTP Method & QR
Method Interface & Registry
Enrollment AJAX & Login Flow
Account & Admin UI Integration
Settings, Card Selector & Template Hooks
Assets Registration & Bootstrap
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 PHPStan (2.1.51)PHPStan was skipped because the sandbox runner could not parse its output. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (4)
Lib/WeDevs_Settings_API.php (1)
1050-1065: Deduplicate dependency-field lookup to prevent drift.The same fallback selector chain is repeated twice; extracting it into one helper inside
script()will reduce maintenance bugs.Refactor sketch
+ function findDependencyField(field_name) { + var $dep = $("input[id*='"+ field_name +"'], select[id*='"+ field_name +"']"); + if ($dep.length === 0) $dep = $("input[name*='["+ field_name +"]'], select[name*='["+ field_name +"]']"); + if ($dep.length === 0) $dep = $("input[id*='["+ field_name +"]'], select[id*='["+ field_name +"]']"); + if ($dep.length === 0) $dep = $("input[id*='"+ field_name +"'], select[id*='"+ field_name +"']"); + return $dep; + } ... - var $depends_on = $("input[id*='"+ field_name +"'], select[id*='"+ field_name +"']"); - ... + var $depends_on = findDependencyField(field_name); ... - var $depends_on = $("input[id*='"+ field_name +"'], select[id*='"+ field_name +"']"); - ... + var $depends_on = findDependencyField(field_name);Also applies to: 1125-1140
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Lib/WeDevs_Settings_API.php` around lines 1050 - 1065, The repeated fallback selector chain that builds $depends_on from field_name (the four jQuery lookups using $("input[id*='"+ field_name +"']" etc.) should be extracted into a single helper inside script(), e.g. a function like resolveDependencyField(field_name) that returns the jQuery collection; replace both duplicated blocks (the occurrences around $depends_on and the second block at lines noted in the review) with calls to that helper so the lookup logic is centralized and not duplicated. Ensure the helper reproduces the same selector priority and returns an empty collection if nothing is found.includes/functions/settings-options.php (1)
37-41: Consider using a more distinctive icon for the 2FA section.The
dashicons-idicon is already used by the "My Account" section (line 30). Consider usingdashicons-lockordashicons-shieldto visually distinguish the 2FA security section.🎨 Proposed change
[ 'id' => 'wpuf_2fa', 'title' => __( '2FA', 'wp-user-frontend' ), - 'icon' => 'dashicons-id', + 'icon' => 'dashicons-shield', ],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@includes/functions/settings-options.php` around lines 37 - 41, The 2FA section uses the same dashicons-id icon as the My Account section; update the icon in the settings array for the entry with 'id' => 'wpuf_2fa' to a more distinctive security glyph (e.g., 'dashicons-lock' or 'dashicons-shield') by replacing the 'icon' value for that array element so the 2FA/security section is visually distinct.includes/TwoFactor/QR_Renderer.php (1)
5-9: Remove unused import.
GDLibRendereris imported on line 5 but never used in the class. OnlyImageRenderer,SvgImageBackEnd,RendererStyle, andWriterare utilized.🧹 Proposed fix
namespace WeDevs\Wpuf\TwoFactor; -use BaconQrCode\Renderer\GDLibRenderer; use BaconQrCode\Renderer\ImageRenderer; use BaconQrCode\Renderer\Image\SvgImageBackEnd; use BaconQrCode\Renderer\RendererStyle\RendererStyle; use BaconQrCode\Writer;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@includes/TwoFactor/QR_Renderer.php` around lines 5 - 9, Remove the unused import GDLibRenderer from the top of the QR_Renderer.php file: locate the use statement "use BaconQrCode\Renderer\GDLibRenderer;" and delete it so only ImageRenderer, SvgImageBackEnd, RendererStyle, and Writer remain; verify no other references to GDLibRenderer in the class (e.g., in methods of QR_Renderer) before committing.includes/TwoFactor/TOTP_Method.php (1)
24-24:SECRET_LENGTH_BYTESis the wrong unit here.
Google2FA::generateSecretKey()uses a Base32 character count, and the library documents 16 chars as 80 bits and 32 chars as 160 bits. By inference, passing20here is about 100 bits of entropy, so both the constant name and the “160 bits” comment are misleading. Either rename this to characters or bump it to32if 160-bit secrets are the goal. (github.com)Suggested fix
- public const SECRET_LENGTH_BYTES = 20; // 160 bits per RFC 6238 §5. + public const SECRET_LENGTH_CHARS = 32; // 160 bits of entropy with Base32 output. ... - return $this->google2fa->generateSecretKey( self::SECRET_LENGTH_BYTES ); + return $this->google2fa->generateSecretKey( self::SECRET_LENGTH_CHARS );Also applies to: 68-70
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@includes/TwoFactor/TOTP_Method.php` at line 24, The constant SECRET_LENGTH_BYTES in TwoFactor/TOTP_Method.php is misnamed and misdocumented: Google2FA expects a Base32 character count, not raw bytes, so either rename SECRET_LENGTH_BYTES to SECRET_LENGTH_CHARS and keep the value 20 (and update the comment to reflect ~100 bits of entropy), or if you actually want 160-bit secrets set the constant to 32 (characters) and update the comment to "32 base32 chars ≈160 bits per RFC 6238 §5"; apply the same rename/value/comment change wherever the constant is used (also update the occurrences referenced around lines 68–70). Ensure function calls that pass this constant to Google2FA::generateSecretKey() reflect the renamed constant name if you rename it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@assets/js/wpuf-2fa-account.js`:
- Around line 34-43: The failure branch for the ajax action wpuf_2fa_totp_start
writes errors into the hidden container .wpuf-2fa-totp-enrollment (updating
.wpuf-2fa-message) without revealing it, so users see no feedback; modify the
failure handling in the $.post(...).done(...) and the other failure path (lines
around 59-61) to call showMessage on a visible container (or ensure the
enrollment panel is shown first) — e.g., ensure you call
$root.find('.wpuf-2fa-totp-enrollment').show() or target a visible selector
before invoking showMessage so errors from wpuf_2fa_totp_start are visible to
users.
In `@includes/TwoFactor/Crypto.php`:
- Around line 26-44: In the encrypt method (function encrypt) ensure you check
the return value of openssl_encrypt for failure before building the payload: if
openssl_encrypt returns false, throw a clear exception (e.g., RuntimeException)
or return an error indicator instead of continuing to base64_encode a boolean;
update the function to validate $ciphertext !== false, handle/log the error and
only construct and return the STORAGE_VERSION.iv.tag.ciphertext payload when
$ciphertext is valid.
In `@includes/TwoFactor/Login_Controller.php`:
- Around line 119-130: After calling wp_set_auth_cookie($user_id, $remember)
fire the standard WordPress login hook so integrations see the 2FA login:
retrieve the WP_User for $user_id (e.g. via get_userdata($user_id)) and call
do_action('wp_login', $user->user_login, $user) immediately after
wp_set_auth_cookie and before wp_safe_redirect/exit; keep the existing
delete_transient(...) behavior intact.
In `@includes/TwoFactor/TOTP_Method.php`:
- Around line 145-160: The verify_against_secret() method currently returns only
true/false and discards the matched TOTP step; change it to return the matched
step (e.g., integer step) on success (or false/null on failure) so callers can
persist it, then update Enrollment_Controller::ajax_confirm() to capture that
returned step and set META_TOTP_LAST_STEP for the user (instead of activating
the secret without recording the step); keep the existing rate_limiter checks
(is_locked, record_failure, clear) and only return the step after clearing the
rate limiter so the exact confirming code cannot be replayed.
In `@Lib/WeDevs_Settings_API.php`:
- Around line 422-430: In the foreach over $args['options'] (the block iterating
$option), guard access to $option['admin_label'] before reading it: replace the
direct index access with a safe lookup (e.g., isset($option['admin_label']) or
null coalescing) and provide a sensible default when missing; update the
assignment to $admin_label so it never triggers a PHP notice for
incomplete/filtered option entries while preserving existing behavior for
present labels.
In `@wpuf-functions.php`:
- Around line 955-958: Update the docblock so it accurately reflects shipped
defaults by removing or rewording the claim that Email OTP and SMS OTP "ship as
pro previews" and instead state that only TOTP is included in the default
registry (exposed as "totp"); adjust the comment text near the gateway-selector
card description to reference only the functional default method "totp" and note
Email/SMS OTP as future/pro-planned if desired.
In `@wpuf.php`:
- Line 60: The PHP version boundary check is incorrect: the class property
$min_php = '7.4' is declared but the runtime check uses a <= comparison that
rejects 7.4; change the conditional (the check around lines 95–99) to use a
strict less-than comparison (or use version_compare($current_php,
$this->min_php, '<')) so that PHP 7.4 is accepted; update the method performing
this check (referenced by $min_php and the existing <= comparison) accordingly
and ensure any related error message reflects the corrected boundary logic.
---
Nitpick comments:
In `@includes/functions/settings-options.php`:
- Around line 37-41: The 2FA section uses the same dashicons-id icon as the My
Account section; update the icon in the settings array for the entry with 'id'
=> 'wpuf_2fa' to a more distinctive security glyph (e.g., 'dashicons-lock' or
'dashicons-shield') by replacing the 'icon' value for that array element so the
2FA/security section is visually distinct.
In `@includes/TwoFactor/QR_Renderer.php`:
- Around line 5-9: Remove the unused import GDLibRenderer from the top of the
QR_Renderer.php file: locate the use statement "use
BaconQrCode\Renderer\GDLibRenderer;" and delete it so only ImageRenderer,
SvgImageBackEnd, RendererStyle, and Writer remain; verify no other references to
GDLibRenderer in the class (e.g., in methods of QR_Renderer) before committing.
In `@includes/TwoFactor/TOTP_Method.php`:
- Line 24: The constant SECRET_LENGTH_BYTES in TwoFactor/TOTP_Method.php is
misnamed and misdocumented: Google2FA expects a Base32 character count, not raw
bytes, so either rename SECRET_LENGTH_BYTES to SECRET_LENGTH_CHARS and keep the
value 20 (and update the comment to reflect ~100 bits of entropy), or if you
actually want 160-bit secrets set the constant to 32 (characters) and update the
comment to "32 base32 chars ≈160 bits per RFC 6238 §5"; apply the same
rename/value/comment change wherever the constant is used (also update the
occurrences referenced around lines 68–70). Ensure function calls that pass this
constant to Google2FA::generateSecretKey() reflect the renamed constant name if
you rename it.
In `@Lib/WeDevs_Settings_API.php`:
- Around line 1050-1065: The repeated fallback selector chain that builds
$depends_on from field_name (the four jQuery lookups using $("input[id*='"+
field_name +"']" etc.) should be extracted into a single helper inside script(),
e.g. a function like resolveDependencyField(field_name) that returns the jQuery
collection; replace both duplicated blocks (the occurrences around $depends_on
and the second block at lines noted in the review) with calls to that helper so
the lookup logic is centralized and not duplicated. Ensure the helper reproduces
the same selector priority and returns an empty collection if nothing is found.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c2618ed8-4e05-49d4-8e1b-d3802dde1aa9
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (26)
Lib/WeDevs_Settings_API.phpassets/css/admin.cssassets/js/admin/settings.jsassets/js/wpuf-2fa-account.jsassets/less/admin.lesscomposer.jsonincludes/Assets.phpincludes/Free/Simple_Login.phpincludes/TwoFactor/Account_Section.phpincludes/TwoFactor/Admin_User_Profile.phpincludes/TwoFactor/Crypto.phpincludes/TwoFactor/Enrollment_Controller.phpincludes/TwoFactor/Login_Controller.phpincludes/TwoFactor/Method_Interface.phpincludes/TwoFactor/Method_Registry.phpincludes/TwoFactor/QR_Renderer.phpincludes/TwoFactor/Rate_Limiter.phpincludes/TwoFactor/TOTP_Method.phpincludes/TwoFactor/Two_Factor_Manager.phpincludes/TwoFactor/User_Storage.phpincludes/functions/settings-options.phpphpcs.xml.distreadme.txttemplates/dashboard/security.phpwpuf-functions.phpwpuf.php
2FA framework: TOTP authenticator support, multi-method registry, enrollment / challenge / verify flow, security dashboard section, and extracted login-stage CSS/JS to dedicated wpuf-2fa-login assets. Login audit (docs/login-process-audit.md, Phases 1-3): - Reconciled the two Turnstile settings on `login_form_turnstile`. - Removed dead Turnstile error string and the `form_align` dead pipeline. - New filters: `wpuf_login_turnstile_verify`, `wpuf_login_form_layout_class`, `wpuf_login_layouts`, `wpuf_login_form_template_args`. - New actions: `wpuf_login_show_errors`, `wpuf_login_show_messages`. - Layout class applied server-side (no FOUC); single source of truth for the layout list. - Server-side rendering of customizable labels / placeholders / button / remember-me / lost-password / form heading — fixes a11y, page caches, and silent shortcode-page heading.
- Fire wp_login action after auth cookie in 2FA stage 2 so integrations listening for logins (audit logs, tracking) see successful 2FA logins. - Guard openssl_encrypt() failure in TwoFactor\Crypto::encrypt() to avoid base64-encoding a false return. - Use strict less-than for the PHP version gate so the declared 7.4 minimum is actually accepted. - isset() guard on admin_label in settings API multiselect-card to avoid notices for filter-extended option arrays.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
includes/TwoFactor/Login_Controller.php (1)
154-166:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFire
wp_loginhook to preserve WordPress authentication chain compatibility.After
wp_set_auth_cookie(), thewp_loginaction is not fired. Core'swp_signon()fires this hook after setting the auth cookie, so integrations and plugins listening for user logins will miss successful 2FA authentications, breaking audit logs, tracking, and conditional logic that depends on this hook.Suggested fix
// Success: consume token, set the auth cookie, redirect. delete_transient( self::TRANSIENT_KEY . $token ); $remember = ! empty( $challenge['remember'] ); wp_set_auth_cookie( $user_id, $remember ); + $user = get_userdata( $user_id ); + if ( $user instanceof \WP_User ) { + do_action( 'wp_login', $user->user_login, $user ); + } + $redirect = ! empty( $challenge['final_redirect'] ) ? $challenge['final_redirect'] : home_url( '/' );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@includes/TwoFactor/Login_Controller.php` around lines 154 - 166, After calling wp_set_auth_cookie($user_id, $remember) in Login_Controller, fire the core wp_login action so other plugins see the successful login: retrieve the WP_User via get_userdata($user_id) (or wp_get_current_user() if preferred), then call do_action('wp_login', $user->user_login, $user) immediately after wp_set_auth_cookie and before wp_safe_redirect/exit; keep the existing delete_transient(self::TRANSIENT_KEY . $token) behavior intact.
🧹 Nitpick comments (1)
includes/TwoFactor/Login_Controller.php (1)
220-223: 💤 Low valueApply consistent sanitization to
$_POST['rememberme'].For consistency with coding guidelines, apply
wp_unslash()before casting. While the boolean cast makes this safe in practice, consistent sanitization patterns are preferred.Suggested fix
// phpcs:ignore WordPress.Security.NonceVerification.Missing - $remember = isset( $_POST['rememberme'] ) ? (bool) $_POST['rememberme'] : false; + $remember = ! empty( $_POST['rememberme'] );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@includes/TwoFactor/Login_Controller.php` around lines 220 - 223, Update the sanitization for the remember-me input in Login_Controller by applying wp_unslash() to $_POST['rememberme'] before casting to boolean so $remember is set consistently (i.e., replace the current direct isset/ cast with an isset check that uses wp_unslash($_POST['rememberme']) and then (bool) on that value); keep the existing phpcs ignore comment if needed and mirror the pattern used for $final_redirect to maintain consistent sanitization style.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@includes/TwoFactor/Admin_User_Profile.php`:
- Around line 36-39: Replace the current_user_can('manage_options') check in
render_section with current_user_can(wpuf_admin_role()) and ensure the
corresponding form handler (the admin reset handler around lines 166-176) also
uses current_user_can(wpuf_admin_role()) and check_ajax_referer() for CSRF
protection; additionally, when reading user_id from $_POST in the handler, use
wp_unslash($_POST['user_id']) then absint() to sanitize, and apply similar
wp_unslash()+appropriate sanitization for any other superglobal inputs touched
by the handler.
---
Duplicate comments:
In `@includes/TwoFactor/Login_Controller.php`:
- Around line 154-166: After calling wp_set_auth_cookie($user_id, $remember) in
Login_Controller, fire the core wp_login action so other plugins see the
successful login: retrieve the WP_User via get_userdata($user_id) (or
wp_get_current_user() if preferred), then call do_action('wp_login',
$user->user_login, $user) immediately after wp_set_auth_cookie and before
wp_safe_redirect/exit; keep the existing delete_transient(self::TRANSIENT_KEY .
$token) behavior intact.
---
Nitpick comments:
In `@includes/TwoFactor/Login_Controller.php`:
- Around line 220-223: Update the sanitization for the remember-me input in
Login_Controller by applying wp_unslash() to $_POST['rememberme'] before casting
to boolean so $remember is set consistently (i.e., replace the current direct
isset/ cast with an isset check that uses wp_unslash($_POST['rememberme']) and
then (bool) on that value); keep the existing phpcs ignore comment if needed and
mirror the pattern used for $final_redirect to maintain consistent sanitization
style.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 81e5d2f8-2e79-4119-aa0c-b250e5fd4c07
📒 Files selected for processing (17)
assets/css/wpuf-2fa-login.cssassets/js/wpuf-2fa-account.jsassets/js/wpuf-2fa-login.jsincludes/Assets.phpincludes/Free/Simple_Login.phpincludes/TwoFactor/Account_Section.phpincludes/TwoFactor/Admin_User_Profile.phpincludes/TwoFactor/Default_Card_Renderer.phpincludes/TwoFactor/Enrollment_Controller.phpincludes/TwoFactor/Login_Controller.phpincludes/TwoFactor/Method_Interface.phpincludes/TwoFactor/TOTP_Method.phpincludes/TwoFactor/Two_Factor_Manager.phpincludes/TwoFactor/User_Storage.phptemplates/dashboard/security.phptemplates/login-form.phpwpuf-functions.php
✅ Files skipped from review due to trivial changes (1)
- assets/css/wpuf-2fa-login.css
🚧 Files skipped from review as they are similar to previous changes (5)
- includes/Assets.php
- includes/TwoFactor/Two_Factor_Manager.php
- templates/dashboard/security.php
- includes/TwoFactor/Account_Section.php
- assets/js/wpuf-2fa-account.js
Comment on lines
+36
to
+39
| public function render_section( $user ) { | ||
| if ( ! current_user_can( 'manage_options' ) ) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Align admin reset guardrails with project security policy.
Use wpuf_admin_role() capability checks in both UI + handler, sanitize user_id from $_POST with wp_unslash() + absint(), and switch to check_ajax_referer() for the form handler guard.
🔧 Suggested patch
public function render_section( $user ) {
- if ( ! current_user_can( 'manage_options' ) ) {
+ if ( ! current_user_can( wpuf_admin_role() ) ) {
return;
}
@@
public function handle_reset() {
- $target_user_id = isset( $_POST['user_id'] ) ? (int) $_POST['user_id'] : 0;
+ $target_user_id = isset( $_POST['user_id'] ) ? absint( wp_unslash( $_POST['user_id'] ) ) : 0;
- if ( ! current_user_can( 'manage_options' ) ) {
+ if ( ! current_user_can( wpuf_admin_role() ) ) {
wp_die( esc_html__( 'You do not have permission to reset 2FA for this user.', 'wp-user-frontend' ), 403 );
}
- $nonce = isset( $_POST['_wpnonce'] ) ? sanitize_key( wp_unslash( $_POST['_wpnonce'] ) ) : '';
-
- if ( ! wp_verify_nonce( $nonce, self::ACTION . '_' . $target_user_id ) ) {
+ if ( ! check_ajax_referer( self::ACTION . '_' . $target_user_id, '_wpnonce', false ) ) {
wp_die( esc_html__( 'Security check failed.', 'wp-user-frontend' ), 403 );
}As per coding guidelines, "Include check_ajax_referer() and current_user_can(wpuf_admin_role()) permission checks in every form and AJAX handler" and "Sanitize all superglobal access ($_POST, $_GET, $_REQUEST, $_SERVER) with wp_unslash() and appropriate sanitization functions".
Also applies to: 166-176
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@includes/TwoFactor/Admin_User_Profile.php` around lines 36 - 39, Replace the
current_user_can('manage_options') check in render_section with
current_user_can(wpuf_admin_role()) and ensure the corresponding form handler
(the admin reset handler around lines 166-176) also uses
current_user_can(wpuf_admin_role()) and check_ajax_referer() for CSRF
protection; additionally, when reading user_id from $_POST in the handler, use
wp_unslash($_POST['user_id']) then absint() to sanitize, and apply similar
wp_unslash()+appropriate sanitization for any other superglobal inputs touched
by the handler.
…_authenticator_app
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@includes/TwoFactor/Login_Controller.php`:
- Around line 307-316: The code treats any \WP_Error from
issue_and_mint_challenge() as a failure and always calls push_login_error(), but
issue_and_mint_challenge() returns a WP_Error with code 'wpuf_2fa_required' on
success; update the guard to only call push_login_error() for the genuine
failure code (e.g. check is_wp_error($result) && $result->get_error_code() ===
'wpuf_2fa_issue_failed') so the challenge prompt returned as 'wpuf_2fa_required'
is not rendered as an error; locate references to issue_and_mint_challenge and
push_login_error in Login_Controller and adjust the conditional accordingly.
- Around line 258-261: Simple_Login::process_login currently only early-returns
for wpuf_2fa_token, so submissions from the stage-1.5 method-choice form
(wpuf_2fa_selector_token) fall through and trigger a "Username is required"
error; update the early-return check in process_login to also detect
wpuf_2fa_selector_token (e.g., change the existing if that checks wpuf_2fa_token
to also check wpuf_2fa_selector_token or add a separate early-return) so the
handler returns immediately when either token is present and does not attempt to
extract $_POST['log'] or $_POST['pwd'].
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8bf2f58d-0159-47c9-8a9a-e43a4d95e53d
📒 Files selected for processing (4)
Lib/WeDevs_Settings_API.phpincludes/TwoFactor/Crypto.phpincludes/TwoFactor/Login_Controller.phpwpuf.php
🚧 Files skipped from review as they are similar to previous changes (3)
- wpuf.php
- Lib/WeDevs_Settings_API.php
- includes/TwoFactor/Crypto.php
Comment on lines
+258
to
+261
| public function maybe_handle_method_choice() { | ||
| // phpcs:ignore WordPress.Security.NonceVerification.Missing | ||
| if ( empty( $_POST['wpuf_2fa_selector_token'] ) || empty( $_POST['wpuf_login'] ) ) { | ||
| return; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 1. Check Simple_Login::process_login() for any wpuf_2fa_selector_token guard
echo "=== Simple_Login early-return guards ==="
rg -n "wpuf_2fa_selector_token|2fa_selector" --type php
# 2. Check whether the login form template strips (not just hides) credential fields
echo ""
echo "=== Login template 2FA field handling ==="
fd -e php -g '*login*' | xargs grep -l "2fa" 2>/dev/null
fd -e php -g '*login*' | xargs grep -n "wpuf_2fa_selector\|wpuf_login\|username\|password" 2>/dev/null | head -60
# 3. Confirm JS approach (hide vs remove credential fields)
echo ""
echo "=== 2FA login JS: field hiding strategy ==="
fd -e js -g '*2fa*' | xargs cat 2>/dev/null | head -80Repository: weDevsOfficial/wp-user-frontend
Length of output: 11321
🏁 Script executed:
#!/bin/bash
# Get the full process_login() method implementation
echo "=== Simple_Login::process_login() full method ==="
sed -n '594,750p' includes/Free/Simple_Login.php
echo ""
echo "=== Check for login form template handling ==="
# Look for template files
fd -e php -g '*form*' -o -g '*template*' | grep -i login | head -20Repository: weDevsOfficial/wp-user-frontend
Length of output: 6636
🏁 Script executed:
#!/bin/bash
# Check what happens after line 594 in process_login
echo "=== View context around process_login (lines 590-720) ==="
sed -n '590,720p' includes/Free/Simple_Login.phpRepository: weDevsOfficial/wp-user-frontend
Length of output: 5368
🏁 Script executed:
#!/bin/bash
# View the authenticate filter implementation in Login_Controller
echo "=== Login_Controller authenticate filter (line 195 context) ==="
sed -n '190,210p' includes/TwoFactor/Login_Controller.php
echo ""
echo "=== Check hook registration ==="
rg -n "add_filter.*authenticate" includes/TwoFactor/Login_Controller.php
echo ""
echo "=== Verify stage 1.5 form structure ==="
sed -n '570,600p' includes/TwoFactor/Login_Controller.phpRepository: weDevsOfficial/wp-user-frontend
Length of output: 2520
Add early-return guard for wpuf_2fa_selector_token in Simple_Login::process_login().
Simple_Login::process_login() early-returns only for wpuf_2fa_token (stage 2) but not for wpuf_2fa_selector_token (stage 1.5). When stage 1.5 submissions (method choice) POST to this handler, they bypass the early-return and proceed to extract credentials. Since the stage 1.5 form contains only the selector token and method buttons—not username/password fields—both $_POST['log'] and $_POST['pwd'] are absent, defaulting to empty strings. The handler then exits at lines 650–651 with a spurious "Username is required" error, which displays alongside the 2FA code input.
Add an early-return guard for wpuf_2fa_selector_token immediately after the existing wpuf_2fa_token check (around line 605):
if ( ! empty( $_POST['wpuf_2fa_token'] ) || ! empty( $_POST['wpuf_2fa_selector_token'] ) ) {
return;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@includes/TwoFactor/Login_Controller.php` around lines 258 - 261,
Simple_Login::process_login currently only early-returns for wpuf_2fa_token, so
submissions from the stage-1.5 method-choice form (wpuf_2fa_selector_token) fall
through and trigger a "Username is required" error; update the early-return
check in process_login to also detect wpuf_2fa_selector_token (e.g., change the
existing if that checks wpuf_2fa_token to also check wpuf_2fa_selector_token or
add a separate early-return) so the handler returns immediately when either
token is present and does not attempt to extract $_POST['log'] or $_POST['pwd'].
Comment on lines
+307
to
+316
| $result = $this->issue_and_mint_challenge( | ||
| $user_id, | ||
| $method, | ||
| ! empty( $selection['remember'] ), | ||
| ! empty( $selection['final_redirect'] ) ? $selection['final_redirect'] : home_url( '/' ) | ||
| ); | ||
|
|
||
| if ( is_wp_error( $result ) ) { | ||
| $this->push_login_error( $result->get_error_message() ); | ||
| } |
There was a problem hiding this comment.
is_wp_error( $result ) is always true — challenge prompt shows as error styling on stage 1.5 success.
issue_and_mint_challenge() is typed @return \WP_Error and unconditionally returns one: 'wpuf_2fa_issue_failed' on failure, 'wpuf_2fa_required' on success. So the if ( is_wp_error( $result ) ) guard at line 314 always executes, pushing the challenge-prompt message ("Enter the 6-digit code from your authenticator app…") through push_login_error() on the success path. That message then renders inside <div class="wpuf-error"> (per show_errors() in Simple_Login), giving an instructional string the visual treatment of an error.
Narrow the guard to the genuine failure code so the prompt is only pushed on send failure:
🐛 Proposed fix
- if ( is_wp_error( $result ) ) {
+ if ( is_wp_error( $result ) && $result->get_error_code() === 'wpuf_2fa_issue_failed' ) {
$this->push_login_error( $result->get_error_message() );
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@includes/TwoFactor/Login_Controller.php` around lines 307 - 316, The code
treats any \WP_Error from issue_and_mint_challenge() as a failure and always
calls push_login_error(), but issue_and_mint_challenge() returns a WP_Error with
code 'wpuf_2fa_required' on success; update the guard to only call
push_login_error() for the genuine failure code (e.g. check is_wp_error($result)
&& $result->get_error_code() === 'wpuf_2fa_issue_failed') so the challenge
prompt returned as 'wpuf_2fa_required' is not rendered as an error; locate
references to issue_and_mint_challenge and push_login_error in Login_Controller
and adjust the conditional accordingly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
closes #1506
closes #1507
Summary
Adds first-class Two-Factor Authentication to WP User Frontend's free edition. Users can secure their accounts with a standards-compliant authenticator app (Google Authenticator, 1Password, Authy, etc.) and are challenged for a 6-digit code on the WPUF login form after their password is verified.
The feature is off by default and is gated behind a new admin settings section (WPUF Settings → 2FA). Once enabled, an authenticated user gets a new Security tab inside
[wpuf_account]to enroll, regenerate, or disable their authenticator. Admins get a Reset 2FA action onUsers → Edit Userfor lockout recovery, with an audit trail.The MVP architecture is multi-method aware (TOTP today; Email OTP / SMS OTP can plug in later via
wpuf_2fa_register_methods) but ships only TOTP.Spec:
docs/totp.md.What's new
User-facing
[wpuf_account](only visible when 2FA is enabled globally).That code didn't match.,Your sign-in session expired.,Too many attempts.) instead of generic WP errors.Admin-facing
Enable Two-Factor Authenticationtoggle2FA Methodsselector (card grid; TOTP today)App Display Name— issuer label shown inside the user's authenticator app, defaults to the site nameUsers → Edit Usershowing enrollment status, recent activity (audit log), and a Reset 2FA button (administrators only, capabilitymanage_options).Architecture / extensibility
WeDevs\Wpuf\TwoFactor\(includes/TwoFactor/):Two_Factor_Manager— service container, accessed aswpuf()->two_factorTOTP_Methodimplements a genericMethod_Interface, registered throughMethod_Registryso future methods slot in without rewriting the challenge controllerEnrollment_Controller,Login_Controller,Account_Section,Admin_User_ProfileUser_Storage(encrypted secret + replay step + audit log + enrollment timestamp)Crypto— AUTH_KEY-derived encryption for the at-rest secretRate_Limiter— 5 failures → 15-minute soft lock, per userQR_Renderer— server-side QR generation (secrets never leave the site)method_selectorsettings field type added toLib/WeDevs_Settings_API.php(extracted fromgateway_selector; both share a privaterender_card_selector()with their own BEM namespaces). Reusable for future selectors (social login, etc.) without further duplication.wpuf_2fa_register_methods,wpuf_2fa_methods(filter),wpuf_2fa_totp_reset(action). Spec lists more hooks reserved for future phases.Summary by CodeRabbit
New Features
Improvements