Add email verification before activation#1
Open
faisalahammad wants to merge 193 commits into
Open
Conversation
Bumps [svgo](https://github.com/svg/svgo) from 3.3.2 to 3.3.3. - [Release notes](https://github.com/svg/svgo/releases) - [Commits](svg/svgo@v3.3.2...v3.3.3) --- updated-dependencies: - dependency-name: svgo dependency-version: 3.3.3 dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [simple-git](https://github.com/steveukx/git-js/tree/HEAD/simple-git) from 3.28.0 to 3.33.0. - [Release notes](https://github.com/steveukx/git-js/releases) - [Changelog](https://github.com/steveukx/git-js/blob/main/simple-git/CHANGELOG.md) - [Commits](https://github.com/steveukx/git-js/commits/simple-git@3.33.0/simple-git) --- updated-dependencies: - dependency-name: simple-git dependency-version: 3.33.0 dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
* update anchor link * update anchor link
* Improves test coverage and reliability Adds extensive unit tests across various core functionalities and provider methods to increase overall code coverage. Enhances the test suite's robustness by introducing a redirect interception mechanism and refining cookie-blocking assertions, preventing test termination and ensuring precise verification. Excludes trivial methods from code coverage reports where direct testing is impractical or unnecessary. * phpcbf fixes * fixes as per comments * Refines test reliability and correctness Ensures anonymous functions are properly removed from hooks in tests. Adjusts filter return values and assertions for greater clarity and accuracy. Corrects a minor typo in a test assertion message. * Refines test suite for improved accuracy Enhances several tests to be more robust and accurately reflect method behavior. - Improves the check for plugin cookie blocking by explicitly verifying the `__return_false` callback. - Ensures a clean test environment for `collect_auth_cookie_tokens` by resetting its static property and directly asserting its populated state. - Corrects the `is_supported_for_user` test to check for global provider registration, aligning with the method's intended logic. * Apply suggestion from @Copilot Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Refines test suite stability and robustness Ensures proper cleanup of authentication cookie filters after tests to prevent unintended side effects on subsequent test runs. Updates email verification regex to use a universal newline matcher, improving test robustness against varied email content formatting. * Ensures API auth test isolation Applies process isolation and global state disabling to the API authentication test. This prevents global state contamination and ensures consistent test results. Adds a guard to prevent redefinition of the `XMLRPC_REQUEST` constant, making the test more robust and reliable when running in various environments or alongside other tests that might set the constant. * Update tests/class-two-factor-core.php Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * phpcs fixes --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
* fix typos * make emails translateable * fix phpstan issue
* Move inline JavaScript to external script files Extract 6 inline script blocks from 4 PHP files into 5 new external JS files under providers/js/, improving CSP compatibility and enabling proper WordPress script dependency management. New files: - totp-admin-qrcode.js: QR code generation for TOTP setup - totp-admin.js: TOTP setup form, checkbox focus, reset key - backup-codes-admin.js: Backup code generation, copy, download - two-factor-login.js: Shared login page authcode clear/focus - two-factor-login-authcode.js: Numeric-only enforcement, auto-submit The customizer messenger one-liner uses wp_add_inline_script() instead of a raw <script> tag. Fixes WordPress#812 * Fix QR code not rendering after resetting authenticator app The "Reset authenticator app" button's AJAX response injects HTML with a QR code placeholder, but the QR code generator script only runs on page load and never re-triggers. Additionally, the qrcode library wasn't loaded when TOTP was already configured. Add qr-code-generator as a dependency of totp-admin script and generate the QR code in JavaScript after the reset response HTML is inserted. * Fix vars-on-top lint errors in totp-admin.js Move all var declarations to the top of their respective function scopes. * Fix issues from Copilot review on external JS migration - Add $hook_suffix parameter to backup codes enqueue_assets() for PHP 8+ - Replace trimStart() with ES5-compatible regex in login authcode - Fix submit button disable using querySelector instead of form.submit - Guard responseJSON access in TOTP admin fail handler - Fix SVG ARIA: use setAttribute with role="img" instead of DOM properties - Localize QR code aria-label string for translation - Move wp_add_inline_script before login_footer action
Add documentation for development and testing: AGENTS.md describes the Two-Factor plugin development environment (Docker/@wordpress/env), build/test/lint commands, architecture and provider patterns, login flow, provider registration, key user meta, REST API, and coding standards. TESTS.md documents how to run the PHPUnit suite inside the wp-env container, coverage, filtering, and provides an overview of the test files and what each test class covers. CLAUDE.md is a simple pointer to AGENTS.md.
Add AGENTS.md, TESTS.md and CLAUDE.md
* add settings page and required functions * add class-two-factor-settings.php * PR Refresh * PR refresh * refresh PR * add docs * add since * fix since version number * update readme to reflect new settings * reverse logic * reverse logic * update settings_action_link test * change to correct StringContains * add additional cap check
don’t error on non-string values such as arrays, etc.
faisalahammad
force-pushed
the
fix/778-email-verification-before-activation
branch
from
abc10ef to
2515e10
Compare
📝 WalkthroughWalkthroughRelease v0.15.0: removed the FIDO U2F provider and libraries, refactored core authentication and provider management, added site-wide provider settings UI, introduced Email provider REST verification, moved inline JS to enqueued scripts, and expanded unit tests and documentation. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant LoginForm as Login Form
participant Core as Two_Factor_Core
participant Provider as Provider (Email/TOTP)
participant Session as Session/Cookies
User->>LoginForm: Submit username/password
LoginForm->>Core: authenticate filter (single-arg)
Core->>Session: Disable auth cookies (send_auth_cookies filter)
Core-->>LoginForm: Allow core auth to proceed
LoginForm->>Core: wp_login action (post-auth)
Core->>Provider: get_primary_provider_for_user() / authentication_page(user)
Provider-->>LoginForm: Render 2FA UI (email/TOTP prompt)
User->>LoginForm: Submit 2FA code
LoginForm->>Provider: validate_authentication(user)
alt Valid Code
Provider->>Session: Set 2FA session marker
Session->>User: Enable auth cookies, redirect to dashboard
else Invalid Code
Provider-->>LoginForm: Show error, re-prompt
end
sequenceDiagram
participant Admin as Admin User
participant EmailUI as Email Provider UI
participant REST as REST API
participant EmailProvider as Two_Factor_Email
participant Mail as Email Service
Admin->>EmailUI: Open user profile 2FA
EmailUI->>REST: POST /email (generate token)
REST->>EmailProvider: rest_setup_email()
EmailProvider->>Mail: Send verification email (action='verification_setup')
Mail-->>Admin: Email with code
Admin->>EmailUI: Enter code
EmailUI->>REST: POST /email (validate code)
REST->>EmailProvider: rest_setup_email() (validate)
EmailProvider->>EmailUI: { verified: true }
EmailUI->>REST: POST /email?enable_provider=true
REST->>EmailProvider: enable provider and persist VERIFIED_META_KEY
EmailProvider-->>EmailUI: { verified: true, enabled: true }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/deploy.yml (1)
47-56:⚠️ Potential issue | 🔴 CriticalReplace
10up/action-wordpress-plugin-asset-updatewith10up/action-wordpress-plugin-deployon line 49.The "WordPress.org deploy" step on line 47-56 is using the wrong action. It currently uses
10up/action-wordpress-plugin-asset-update, but should use10up/action-wordpress-plugin-deploy. Thegenerate-zip: trueinput on line 52 is only available in the deploy action, not the asset-update action. Both actions are separate, and line 39 correctly uses asset-update for asset updates while line 49 should use deploy for deployments.Proposed fix
- name: WordPress.org deploy id: deploy - uses: 10up/action-wordpress-plugin-asset-update@2480306f6f693672726d08b5917ea114cb2825f7 # v2.2.0 + uses: 10up/action-wordpress-plugin-deploy@8b2ae52c7cb2f272565d2c0c4f3355450e12dee1 # v2.2.0 if: startsWith( github.ref, 'refs/tags/' )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/deploy.yml around lines 47 - 56, The workflow step with id "deploy" is using the wrong GitHub Action; replace the uses value "10up/action-wordpress-plugin-asset-update@2480306f6f693672726d08b5917ea114cb2825f7" with "10up/action-wordpress-plugin-deploy" so the deploy step supports the generate-zip input; keep the step id "deploy", the if condition, generate-zip: true, and existing env variables (BUILD_DIR, SVN_USERNAME, SVN_PASSWORD) unchanged.
🧹 Nitpick comments (3)
providers/js/two-factor-login.js (1)
4-9: Empty catch block silently swallows errors.While the try/catch is needed to handle cases where
#authcodedoesn't exist, the empty catch block will also hide genuine runtime errors. Consider checking for element existence explicitly instead:🔧 Suggested improvement
- var d; - try { - d = document.getElementById( 'authcode' ); - d.value = ''; - d.focus(); - } catch ( e ) {} + var authcodeInput = document.getElementById( 'authcode' ); + if ( authcodeInput ) { + authcodeInput.value = ''; + authcodeInput.focus(); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@providers/js/two-factor-login.js` around lines 4 - 9, The empty catch hides real errors; replace the try/catch by retrieving the element with document.getElementById('authcode') into variable d, check if d is non-null before setting d.value = '' and calling d.focus(), and if you still need to guard unexpected exceptions wrap only the DOM access in a minimal catch that logs the error (e.g., console.error) rather than swallowing it; target the code around variable d and the calls to d.value and d.focus().user-edit.css (1)
63-66: Use an explicit padding value for better browser compatibility.The
revertkeyword is not supported in IE11 or older browser versions (Edge <84, Chrome <84, Firefox <67, Safari <9.1). If legacy browser support is required, consider replacingpadding: revert;with an explicit padding value (e.g.,padding: 0;or the intended value).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@user-edit.css` around lines 63 - 66, The CSS rule for .two-factor-backup-codes-list-wrap .two-factor-backup-codes-unused-codes uses padding: revert which is unsupported in legacy browsers; replace padding: revert with the explicit padding value you intend (e.g., padding: 0; or another pixel/REM value) so that the rule works consistently across IE11 and older browser versions.CREDITS.md (1)
16-16: Consider splitting contributors into one-per-line entries.This would reduce future merge conflicts and make review/history diffs much easier to read.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CREDITS.md` at line 16, The contributor list in CREDITS.md is a single long comma-separated line which causes merge conflicts; split each contributor entry like "[Name (`@handle`)](url)" onto its own line so each contributor occupies a single line, preserving the exact text/links and original ordering, remove inline trailing commas between entries (use one entry per line optionally ending with a comma or none consistently), and ensure the final sentence punctuation remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@class-two-factor-core.php`:
- Around line 651-673: The code forces 'Two_Factor_Email' into
$enabled_providers when the provider class exists, but
Two_Factor_Email::is_available_for_user() may return false (e.g., unverified
email), leaving $configured_providers empty and causing 2FA to fail open; change
the logic in the block that inspects $enabled_providers so it checks
Two_Factor_Email::is_available_for_user( $user ) (or the provider's availability
method) before adding it to $enabled_providers, and if no providers are truly
available return the WP_Error as currently done; additionally, update callers
that assume an array result to handle a WP_Error return from this code path
(check for is_wp_error() before treating the return as an array).
In `@package.json`:
- Around line 16-17: The lint:js npm script currently only targets
./Gruntfile.js so JS files under providers/js/ (e.g., backup-codes-admin.js,
totp-admin.js, two-factor-login.js, etc.) are not linted; update the "lint:js"
script in package.json to include the providers/js/ directory by invoking
wp-scripts lint-js with both ./Gruntfile.js and ./providers/js/ so the WordPress
ESLint config in .eslintrc.json is applied to those files.
In `@providers/class-two-factor-email.php`:
- Around line 153-176: rest_setup_email currently writes a bare truthy flag to
self::VERIFIED_META_KEY which allows Email 2FA to remain trusted after the
user_email changes; change the flow so after successful verification (in
rest_setup_email using generate_and_email_token and validate_token) you store
the verified email (or a cryptographic hash of it) into self::VERIFIED_META_KEY
instead of true, and update is_available_for_user and pre_user_options_update to
compare that stored value to the current user_email (or its hash) and treat
mismatches as unverified (or clear the meta); also ensure any code paths that
should invalidate verification on email change will remove the VERIFIED_META_KEY
so re-verification is required.
In `@providers/class-two-factor-totp.php`:
- Around line 620-628: The return value in get_authcode_valid_ticktime() uses
self::DEFAULT_TIME_STEP_SEC, causing wrong timestamps when a non-default
$time_step is used; update the return to multiply the matched tick ($log_time)
by the actual $time_step parameter (not self::DEFAULT_TIME_STEP_SEC) so the
method returns the correct timestamp for the caller's time step (ensure
references to self::calc_totp, $time_step, and $log_time are used to find the
code to change).
In `@providers/js/backup-codes-admin.js`:
- Around line 23-48: The click handler on
'.button-two-factor-backup-codes-generate' calls wp.apiRequest but lacks error
handling; add a .catch() to the wp.apiRequest promise to handle failures from
the REST API, log the error (or include error.message) and provide user feedback
(e.g. show an inline error message in '.two-factor-backup-codes-wrapper' or use
alert/notice), and ensure any UI state changes (like showing the wrapper or
clearing collections) are only done on success or are reverted/cleared on
failure; locate the wp.apiRequest call inside the click handler and attach the
error handler to it, referencing response processing code that updates
'.two-factor-backup-codes-unused-codes', '.two-factor-backup-codes-count' and
'#two-factor-backup-codes-download-link'.
In `@providers/js/totp-admin-qrcode.js`:
- Around line 17-34: The qr initialization (qrGenerator) assumes DOM nodes exist
and only checks for readyState === 'complete', which can cause exceptions;
update qrGenerator to safely query and null-check the container
('#two-factor-qr-code'), the anchor and the generated SVG before accessing
properties or calling methods (e.g., ensure the anchor exists before setting
innerHTML and ensure svg is non-null before setAttribute/appendChild), and
change the startup check to run immediately when document.readyState !==
'loading' (so both 'interactive' and 'complete' are treated as ready) otherwise
attach the DOMContentLoaded listener; reference qrGenerator, the
'#two-factor-qr-code' selector, and the DOMContentLoaded listener when making
these changes.
In `@providers/js/totp-admin.js`:
- Around line 33-37: The click handler attached to checkbox currently assumes
document.getElementById('two-factor-totp-authcode') exists and calls .focus(),
which can throw if the element was removed; update the handler (the anonymous
function bound to checkbox.addEventListener('click', ...') to first query the
element into a variable, check it is non-null (and optionally that it has a
focus method), and only then call .focus() so clicks no longer break when the
authcode field is absent.
- Around line 41-94: The current direct bindings on $('.totp-submit').click(...)
and $('.button.reset-totp-key').click(...) become inert after
`#two-factor-totp-options` is replaced; change them to delegated handlers using a
stable ancestor (e.g., document or a closer container) like
$(document).on('click', '.totp-submit', function(e){...}) and
$(document).on('click', '.button.reset-totp-key', function(e){...}) preserving
the existing handler bodies (preventDefault, wp.apiRequest calls, and DOM
updates) and remove the original direct .click(...) calls so newly injected
controls remain interactive; reference the existing symbols
twoFactorTotpAdmin.restPath, generateQrCode, and selectors
'#two-factor-totp-key', '#two-factor-totp-authcode', '#totp-setup-error',
'#enabled-Two_Factor_Totp', and '#two-factor-totp-options'.
In `@readme.md`:
- Line 4: The license badge in readme.md currently points to the wrong
repository (WordPress/ai); update the badge URL reference in the badges line to
the correct repository (e.g., WordPress/two-factor) so the license image and
link use the repo's actual license file; locate the license badge fragment that
references "WordPress/ai.svg" and replace it with the correct repository slug
(for consistency with the other WordPress badge URLs).
In `@readme.txt`:
- Around line 58-60: The "FIDO U2F Security Keys" subsection heading and its
note currently live inside the available-methods section but contradict other
docs; remove the entire "FIDO U2F Security Keys" subsection (the heading "###
FIDO U2F Security Keys" and its content) from the available-methods list, or
alternatively relocate that short note into the changelog/FAQ file under a clear
entry about removed features; update any table-of-contents entries referencing
"FIDO U2F Security Keys" to reflect its removal.
In `@tests/class-two-factor-core.php`:
- Around line 349-364: The test assumes Two_Factor_Email is available but
Two_Factor_Email::is_available_for_user() now requires the user be verified;
update the test to mark the test user as verified before asserting the fallback.
Specifically, after update_user_meta(...) set whatever verification flag/method
your system uses for Two_Factor_Email (e.g. mark the user as verified via the
verification meta or helper used elsewhere in tests) so that
Two_Factor_Core::get_available_providers_for_user( $user ) can legitimately
return 'Two_Factor_Email'; ensure you reference the same verification mechanism
used by Two_Factor_Email::is_available_for_user().
- Around line 2546-2555: This test relies on
Two_Factor_Core::add_settings_action_link() only adding the settings link for
users who can manage_options, so make the capability precondition explicit by
creating or switching to a user with that capability before calling
add_settings_action_link; e.g., create a user with the administrator role (via
the test factory) or add the manage_options capability to a test user, call
wp_set_current_user() to make them the current user, run
Two_Factor_Core::add_settings_action_link( $links ), then perform the existing
assertions and optionally reset the current user after the test.
In `@tests/providers/class-two-factor-email.php`:
- Around line 216-223: The test assertions in
tests/providers/class-two-factor-email.php are still checking the old “Enter …
log in” fragments against $GLOBALS['phpmailer']->Body; update the assertions
inside the block using $this->assertStringContainsString to match the current
login-email copy (replace the 'Enter'/'log in' expected fragments with the new
phrase(s) used by the login branch) while keeping the existing IP check for
'127.0.0.1' and the username check using $user->user_login so the test verifies
the actual message content produced by the updated template.
In `@two-factor.php`:
- Around line 69-82: The provider-restriction filters are currently added on
init too late (after Two_Factor_Core bootstraps providers); move the add_filter
calls for two_factor_filter_enabled_providers and
two_factor_filter_enabled_providers_for_user so they are registered at file-load
time (i.e. outside two_factor_register_admin_hooks) so they run before the
'init' hook that triggers Two_Factor_Core::get_providers(); also guard those
top-level add_filter registrations to skip them during uninstall (check
defined('WP_UNINSTALL_PLUGIN') or an equivalent uninstall flag) so uninstall
discovery isn't affected—leave the admin_menu registration inside
two_factor_register_admin_hooks and keep the add_action('init',
'two_factor_register_admin_hooks') as-is.
---
Outside diff comments:
In @.github/workflows/deploy.yml:
- Around line 47-56: The workflow step with id "deploy" is using the wrong
GitHub Action; replace the uses value
"10up/action-wordpress-plugin-asset-update@2480306f6f693672726d08b5917ea114cb2825f7"
with "10up/action-wordpress-plugin-deploy" so the deploy step supports the
generate-zip input; keep the step id "deploy", the if condition, generate-zip:
true, and existing env variables (BUILD_DIR, SVN_USERNAME, SVN_PASSWORD)
unchanged.
---
Nitpick comments:
In `@CREDITS.md`:
- Line 16: The contributor list in CREDITS.md is a single long comma-separated
line which causes merge conflicts; split each contributor entry like "[Name
(`@handle`)](url)" onto its own line so each contributor occupies a single line,
preserving the exact text/links and original ordering, remove inline trailing
commas between entries (use one entry per line optionally ending with a comma or
none consistently), and ensure the final sentence punctuation remains unchanged.
In `@providers/js/two-factor-login.js`:
- Around line 4-9: The empty catch hides real errors; replace the try/catch by
retrieving the element with document.getElementById('authcode') into variable d,
check if d is non-null before setting d.value = '' and calling d.focus(), and if
you still need to guard unexpected exceptions wrap only the DOM access in a
minimal catch that logs the error (e.g., console.error) rather than swallowing
it; target the code around variable d and the calls to d.value and d.focus().
In `@user-edit.css`:
- Around line 63-66: The CSS rule for .two-factor-backup-codes-list-wrap
.two-factor-backup-codes-unused-codes uses padding: revert which is unsupported
in legacy browsers; replace padding: revert with the explicit padding value you
intend (e.g., padding: 0; or another pixel/REM value) so that the rule works
consistently across IE11 and older browser versions.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 74708907-cfb2-4b0b-9b76-d07fa6762531
⛔ Files ignored due to path filters (7)
.wordpress-org/screenshot-1.pngis excluded by!**/*.png.wordpress-org/screenshot-2.pngis excluded by!**/*.png.wordpress-org/screenshot-3.pngis excluded by!**/*.png.wordpress-org/screenshot-4.pngis excluded by!**/*.png.wordpress-org/screenshot-5.pngis excluded by!**/*.pngcomposer.lockis excluded by!**/*.lockpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (44)
.github/workflows/deploy.yml.github/workflows/props-bot.yml.github/workflows/test.yml.wordpress-org/blueprints/blueprint.jsonAGENTS.mdCHANGELOG.mdCLAUDE.mdCREDITS.mdRELEASING.mdTESTS.mdclass-two-factor-core.phpcomposer.jsonincludes/Google/u2f-api.jsincludes/Yubico/U2F.phppackage.jsonphpcs.xml.distproviders/class-two-factor-backup-codes.phpproviders/class-two-factor-dummy.phpproviders/class-two-factor-email.phpproviders/class-two-factor-fido-u2f-admin-list-table.phpproviders/class-two-factor-fido-u2f-admin.phpproviders/class-two-factor-fido-u2f.phpproviders/class-two-factor-provider.phpproviders/class-two-factor-totp.phpproviders/css/fido-u2f-admin.cssproviders/js/backup-codes-admin.jsproviders/js/fido-u2f-admin-inline-edit.jsproviders/js/fido-u2f-admin.jsproviders/js/fido-u2f-login.jsproviders/js/totp-admin-qrcode.jsproviders/js/totp-admin.jsproviders/js/two-factor-login-authcode.jsproviders/js/two-factor-login.jsreadme.mdreadme.txtsettings/class-two-factor-settings.phptests/class-two-factor-core.phptests/providers/class-two-factor-backup-codes.phptests/providers/class-two-factor-email.phptests/providers/class-two-factor-fido-u2f.phptests/providers/class-two-factor-provider.phptests/providers/class-two-factor-totp.phptwo-factor.phpuser-edit.css
💤 Files with no reviewable changes (10)
- providers/css/fido-u2f-admin.css
- providers/js/fido-u2f-login.js
- providers/js/fido-u2f-admin.js
- providers/class-two-factor-fido-u2f-admin-list-table.php
- providers/js/fido-u2f-admin-inline-edit.js
- tests/providers/class-two-factor-fido-u2f.php
- providers/class-two-factor-fido-u2f-admin.php
- includes/Google/u2f-api.js
- includes/Yubico/U2F.php
- providers/class-two-factor-fido-u2f.php
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 12 file(s) based on 14 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (4)
class-two-factor-core.php (1)
765-770:⚠️ Potential issue | 🟠 MajorDon’t
wp_die()fromget_primary_provider_for_user().This accessor is also used by
is_user_using_two_factor()and the Users/profile rendering paths in this file. For a user whose only configured provider disappeared and can’t fall back to Email, this branch now takes down those admin flows instead of behaving like “no primary provider”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@class-two-factor-core.php` around lines 765 - 770, get_primary_provider_for_user() currently calls wp_die() when $available_providers is a WP_Error, which aborts unrelated admin flows; instead, treat this case as "no primary provider." In the is_wp_error($available_providers) branch inside get_primary_provider_for_user(), remove the wp_die() call and return null (optionally log the error with error_log or trigger_error for diagnostics), so callers like is_user_using_two_factor() and profile rendering continue to behave as if no primary provider is configured.providers/class-two-factor-email.php (2)
159-173:⚠️ Potential issue | 🟠 MajorBind setup tokens to the address they were sent to.
This endpoint accepts any valid user token and then marks the current
user_emailas verified. If the address changes between “send code” and “submit code”, a code delivered to the old mailbox will verify the new mailbox. Store the target email (or its hash) alongside the setup token and reject mismatches before writingVERIFIED_META_KEY.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@providers/class-two-factor-email.php` around lines 159 - 173, Bind setup tokens to the email they were sent to by persisting the target address (or a hash) with the setup token and checking it on validation: update generate_and_email_token( $user, 'verification_setup' ) to store the intended email alongside the token, modify validate_token( $user_id, $code ) (or add a validate_token_for_email helper) to load the token metadata and ensure the stored email matches $user->user_email before returning true, and only call update_user_meta( $user_id, self::VERIFIED_META_KEY, $user->user_email ) when the token-email pair matches; if they differ, return an appropriate WP_Error (e.g., 'email_mismatch') instead of marking verified.
590-603:⚠️ Potential issue | 🟠 MajorThe legacy exemption is still too broad.
is_available_for_user()now treats any currently-enabled Email provider as legacy. After Line 677 clearsVERIFIED_META_KEY, a user who was genuinely verified once can change their profile email and keep Email 2FA active on the new unverified address. This needs a dedicated migration/legacy marker, or a preserved last-verified address, instead of using current enabled state as the proxy.Also applies to: 673-689
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@providers/class-two-factor-email.php` around lines 590 - 603, is_available_for_user() currently treats any enabled 'Two_Factor_Email' as a legacy exemption, which allows a previously-verified user to change their profile email and keep Email 2FA active; instead, add and check a dedicated migration/legacy marker or preserve the last verified address in user meta (e.g., create a new user meta key like self::LEGACY_VERIFIED_MARKER or self::LAST_VERIFIED_EMAIL) during your migration, update the migration to populate that field rather than relying on Two_Factor_Core::ENABLED_PROVIDERS_USER_META_KEY, and change is_available_for_user() to consult that new meta (compare preserved last-verified email to $user->user_email or check the explicit legacy marker) before returning true for legacy users.tests/class-two-factor-core.php (1)
2097-2114:⚠️ Potential issue | 🟠 MajorMark Email verified before expecting it to be selectable.
This test still enables
Two_Factor_Emailas if it were immediately available. The new production rule requires the verified-email meta for non-legacy users, so seed that fixture first.🧪 Suggested fix
// Enable Email provider as well. + update_user_meta( $user->ID, Two_Factor_Email::VERIFIED_META_KEY, $user->user_email ); Two_Factor_Core::enable_provider_for_user( $user->ID, 'Two_Factor_Email' );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/class-two-factor-core.php` around lines 2097 - 2114, The test enables Two_Factor_Email and then asserts it can be selected, but production requires the verified-email user meta for non-legacy users; before calling Two_Factor_Core::enable_provider_for_user or before applying the filter, set the verified-email meta for $user->ID (e.g. via update_user_meta/add_user_meta) to a truthy/expected value so Two_Factor_Email is treated as verified and becomes selectable when Two_Factor_Core::get_primary_provider_for_user is called.
🧹 Nitpick comments (1)
providers/js/backup-codes-admin.js (1)
3-21: Copy handler implementation looks good with minor enhancement opportunity.The clipboard fallback logic is correct. However,
navigator.clipboard.writeTextreturns a Promise, and errors (e.g., permission denied) are silently swallowed. Consider adding.catch()for robustness, though this is optional since copy failures are generally non-critical.♻️ Optional: Handle clipboard promise rejection
if ( navigator.clipboard && navigator.clipboard.writeText ) { - navigator.clipboard.writeText( csvCodes ); + navigator.clipboard.writeText( csvCodes ).catch( function() { + // Fallback silently handled; user can retry. + } ); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@providers/js/backup-codes-admin.js` around lines 3 - 21, The click handler bound to '.button-two-factor-backup-codes-copy' calls navigator.clipboard.writeText(csvCodes) but ignores its returned Promise; update the handler to attach a .catch(...) to navigator.clipboard.writeText(csvCodes) (and optionally a .then(...) for success) so permission or write errors are handled (e.g., log via console.error or show a UI message) while keeping the existing textarea fallback intact; locate the code around the click callback where csvCodes is read and modify the navigator.clipboard.writeText call to handle rejections.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@class-two-factor-core.php`:
- Around line 1066-1069: The second is_wp_error() check is unreachable because
get_available_providers_for_user( $user ) is already tested and wp_die() is
called on error; remove the redundant is_wp_error() branch (and any associated
else/empty code) so only the first guard remains; update any subsequent logic
that assumed the removed branch to rely on the single post-guard flow
(references: get_available_providers_for_user, is_wp_error, wp_die).
In `@providers/js/backup-codes-admin.js`:
- Line 65: The file ends with the closing IIFE token "}( jQuery ) );" but is
missing a trailing newline; add a single newline character at the end of
providers/js/backup-codes-admin.js (after the "}( jQuery ) );" line) so the file
terminates with a newline to satisfy the linter.
- Around line 40-42: The loop in providers/js/backup-codes-admin.js inserts
response.codes[i] directly into an HTML string, creating an XSS risk; change the
append logic to create a new li element, set its text content using jQuery
.text() (rather than string concatenation), add the
"two-factor-backup-codes-token" class, and then append it to $codesList so
response.codes is safely escaped (refer to the loop that iterates over
response.codes and the $codesList variable).
- Around line 59-62: The lint failure is due to using console in
providers/js/backup-codes-admin.js without declaring it as a global; fix by
either adding "console" to the file's globals comment at the top or by adding a
local ESLint directive before the debug line (e.g., disable no-console for that
statement) and then restoring linting after if needed; locate the snippet that
calls console.error('Backup codes generation failed:', error) and apply one of
these two fixes so the linter no longer flags console as undefined.
In `@providers/js/totp-admin-qrcode.js`:
- Line 46: The file ends with the closing IIFE shown as "}() );" but is missing
a trailing newline; open providers/js/totp-admin-qrcode.js and add a single
newline character after the closing IIFE (the "}() );" sequence) so the file
ends with a newline to satisfy the JS/CSS lint job.
In `@providers/js/totp-admin.js`:
- Around line 21-25: Rename the block-scoped declarations to use let/const and
move their declarations to the appropriate enclosing scope: replace var
ariaLabel with const or let ariaLabel (computed once from
twoFactorTotpAdmin.qrCodeAriaLabel) and ensure authCodeInput is declared with
let/const in the same containing scope where it’s referenced (instead of inside
inner blocks), update any assignments accordingly, and add a final trailing
newline to the file; reference symbols: ariaLabel, authCodeInput,
twoFactorTotpAdmin, svg, title.
In `@tests/providers/class-two-factor-email.php`:
- Around line 303-306: The test sets the legacy boolean marker for verification
but production expects the verified email string; update the fixtures in
tests/providers/class-two-factor-email.php (e.g., in
test_is_available_for_user_verified and the other failing case) to write the
user's actual email into Two_Factor_Email::VERIFIED_META_KEY (use
$user->user_email) instead of true so that
Two_Factor_Email::is_available_for_user sees a matching verified email.
In `@two-factor.php`:
- Around line 156-158: The current bypass uses only is_admin() + $_GET['page']
which is spoofable; change the condition to verify the actual admin screen via
get_current_screen() (use get_current_screen() and check $screen->id or
$screen->base for the real two-factor settings screen) before returning
$providers so the bypass only runs on the genuine two-factor settings screen
rather than any request with page=two-factor-settings.
---
Duplicate comments:
In `@class-two-factor-core.php`:
- Around line 765-770: get_primary_provider_for_user() currently calls wp_die()
when $available_providers is a WP_Error, which aborts unrelated admin flows;
instead, treat this case as "no primary provider." In the
is_wp_error($available_providers) branch inside get_primary_provider_for_user(),
remove the wp_die() call and return null (optionally log the error with
error_log or trigger_error for diagnostics), so callers like
is_user_using_two_factor() and profile rendering continue to behave as if no
primary provider is configured.
In `@providers/class-two-factor-email.php`:
- Around line 159-173: Bind setup tokens to the email they were sent to by
persisting the target address (or a hash) with the setup token and checking it
on validation: update generate_and_email_token( $user, 'verification_setup' ) to
store the intended email alongside the token, modify validate_token( $user_id,
$code ) (or add a validate_token_for_email helper) to load the token metadata
and ensure the stored email matches $user->user_email before returning true, and
only call update_user_meta( $user_id, self::VERIFIED_META_KEY, $user->user_email
) when the token-email pair matches; if they differ, return an appropriate
WP_Error (e.g., 'email_mismatch') instead of marking verified.
- Around line 590-603: is_available_for_user() currently treats any enabled
'Two_Factor_Email' as a legacy exemption, which allows a previously-verified
user to change their profile email and keep Email 2FA active; instead, add and
check a dedicated migration/legacy marker or preserve the last verified address
in user meta (e.g., create a new user meta key like self::LEGACY_VERIFIED_MARKER
or self::LAST_VERIFIED_EMAIL) during your migration, update the migration to
populate that field rather than relying on
Two_Factor_Core::ENABLED_PROVIDERS_USER_META_KEY, and change
is_available_for_user() to consult that new meta (compare preserved
last-verified email to $user->user_email or check the explicit legacy marker)
before returning true for legacy users.
In `@tests/class-two-factor-core.php`:
- Around line 2097-2114: The test enables Two_Factor_Email and then asserts it
can be selected, but production requires the verified-email user meta for
non-legacy users; before calling Two_Factor_Core::enable_provider_for_user or
before applying the filter, set the verified-email meta for $user->ID (e.g. via
update_user_meta/add_user_meta) to a truthy/expected value so Two_Factor_Email
is treated as verified and becomes selectable when
Two_Factor_Core::get_primary_provider_for_user is called.
---
Nitpick comments:
In `@providers/js/backup-codes-admin.js`:
- Around line 3-21: The click handler bound to
'.button-two-factor-backup-codes-copy' calls
navigator.clipboard.writeText(csvCodes) but ignores its returned Promise; update
the handler to attach a .catch(...) to navigator.clipboard.writeText(csvCodes)
(and optionally a .then(...) for success) so permission or write errors are
handled (e.g., log via console.error or show a UI message) while keeping the
existing textarea fallback intact; locate the code around the click callback
where csvCodes is read and modify the navigator.clipboard.writeText call to
handle rejections.
🪄 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: 6197a901-7641-48c3-9432-c3004c88eb80
📒 Files selected for processing (12)
class-two-factor-core.phppackage.jsonproviders/class-two-factor-email.phpproviders/class-two-factor-totp.phpproviders/js/backup-codes-admin.jsproviders/js/totp-admin-qrcode.jsproviders/js/totp-admin.jsreadme.mdreadme.txttests/class-two-factor-core.phptests/providers/class-two-factor-email.phptwo-factor.php
🚧 Files skipped from review as they are similar to previous changes (3)
- package.json
- readme.txt
- readme.md
Comment on lines
1066
to
+1069
| $available_providers = self::get_available_providers_for_user( $user ); | ||
| if ( is_wp_error( $available_providers ) ) { | ||
| wp_die( $available_providers ); | ||
| } |
There was a problem hiding this comment.
Remove the unreachable second is_wp_error() check.
The earlier guard already exits on WP_Error, so this branch is dead code and is what the PHP lint job is complaining about.
🧹 Suggested cleanup
$available_providers = self::get_available_providers_for_user( $user );
if ( is_wp_error( $available_providers ) ) {
wp_die( $available_providers );
}
$backup_providers = array_diff_key( $available_providers, array( $provider_key => null ) );
$interim_login = isset( $_REQUEST['interim-login'] ); // phpcs:ignore WordPress.Security.NonceVerification.Recommended
-
- if ( is_wp_error( $available_providers ) ) {
- // If it returned an error, the configured methods don't exist, and it couldn't swap in a replacement.
- wp_die( $available_providers );
- }Also applies to: 1075-1078
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@class-two-factor-core.php` around lines 1066 - 1069, The second is_wp_error()
check is unreachable because get_available_providers_for_user( $user ) is
already tested and wp_die() is called on error; remove the redundant
is_wp_error() branch (and any associated else/empty code) so only the first
guard remains; update any subsequent logic that assumed the removed branch to
rely on the single post-guard flow (references:
get_available_providers_for_user, is_wp_error, wp_die).
Comment on lines
+40
to
+42
| for ( i = 0; i < response.codes.length; i++ ) { | ||
| $codesList.append( '<li class="two-factor-backup-codes-token">' + response.codes[ i ] + '</li>' ); | ||
| } |
There was a problem hiding this comment.
Potential XSS vulnerability: escape response data before inserting into HTML.
response.codes[i] is concatenated directly into an HTML string. If the server response is compromised or contains unexpected characters, this could lead to XSS. Use jQuery's .text() to safely set content.
🛡️ Proposed fix
// Append the codes.
for ( i = 0; i < response.codes.length; i++ ) {
- $codesList.append( '<li class="two-factor-backup-codes-token">' + response.codes[ i ] + '</li>' );
+ $codesList.append( $( '<li class="two-factor-backup-codes-token"></li>' ).text( response.codes[ i ] ) );
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for ( i = 0; i < response.codes.length; i++ ) { | |
| $codesList.append( '<li class="two-factor-backup-codes-token">' + response.codes[ i ] + '</li>' ); | |
| } | |
| for ( i = 0; i < response.codes.length; i++ ) { | |
| $codesList.append( $( '<li class="two-factor-backup-codes-token"></li>' ).text( response.codes[ i ] ) ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@providers/js/backup-codes-admin.js` around lines 40 - 42, The loop in
providers/js/backup-codes-admin.js inserts response.codes[i] directly into an
HTML string, creating an XSS risk; change the append logic to create a new li
element, set its text content using jQuery .text() (rather than string
concatenation), add the "two-factor-backup-codes-token" class, and then append
it to $codesList so response.codes is safely escaped (refer to the loop that
iterates over response.codes and the $codesList variable).
Comment on lines
+59
to
+62
| // Log for debugging. | ||
| if ( console && console.error ) { | ||
| console.error( 'Backup codes generation failed:', error ); | ||
| } |
There was a problem hiding this comment.
Fix linting errors: console is not defined.
The pipeline is failing because console is not declared in the globals comment. Either add console to the globals declaration or use an ESLint disable comment for debugging statements.
🔧 Option 1: Add console to globals (line 1)
-/* global twoFactorBackupCodes, wp, navigator, document, jQuery */
+/* global twoFactorBackupCodes, wp, navigator, document, jQuery, console */🔧 Option 2: Use ESLint disable comment
// Log for debugging.
- if ( console && console.error ) {
+ if ( typeof console !== 'undefined' && console.error ) { // eslint-disable-line no-console
console.error( 'Backup codes generation failed:', error );
}🧰 Tools
🪛 GitHub Actions: Test
[error] 60-60: ESLint (lint-js) error: 'console' is not defined. (no-undef)
🪛 GitHub Check: Lint JS & CSS
[failure] 61-61:
'console' is not defined
[failure] 61-61:
Unexpected console statement
[failure] 60-60:
'console' is not defined
[failure] 60-60:
Unexpected console statement
[failure] 60-60:
'console' is not defined
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@providers/js/backup-codes-admin.js` around lines 59 - 62, The lint failure is
due to using console in providers/js/backup-codes-admin.js without declaring it
as a global; fix by either adding "console" to the file's globals comment at the
top or by adding a local ESLint directive before the debug line (e.g., disable
no-console for that statement) and then restoring linting after if needed;
locate the snippet that calls console.error('Backup codes generation failed:',
error) and apply one of these two fixes so the linter no longer flags console as
undefined.
| } | ||
| } ); | ||
| } ); | ||
| }( jQuery ) ); No newline at end of file |
There was a problem hiding this comment.
Add missing newline at end of file.
Linter requires a newline at EOF.
🔧 Proposed fix
}( jQuery ) );
+📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| }( jQuery ) ); | |
| }( jQuery ) ); | |
🧰 Tools
🪛 GitHub Check: Lint JS & CSS
[failure] 65-65:
Newline required at end of file but not found
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@providers/js/backup-codes-admin.js` at line 65, The file ends with the
closing IIFE token "}( jQuery ) );" but is missing a trailing newline; add a
single newline character at the end of providers/js/backup-codes-admin.js (after
the "}( jQuery ) );" line) so the file terminates with a newline to satisfy the
linter.
| } else { | ||
| window.addEventListener( 'DOMContentLoaded', qrGenerator ); | ||
| } | ||
| }() ); No newline at end of file |
There was a problem hiding this comment.
Add the missing trailing newline.
The JS/CSS lint job is still failing on this file because it does not end with a newline.
🧰 Tools
🪛 GitHub Check: Lint JS & CSS
[failure] 46-46:
Newline required at end of file but not found
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@providers/js/totp-admin-qrcode.js` at line 46, The file ends with the closing
IIFE shown as "}() );" but is missing a trailing newline; open
providers/js/totp-admin-qrcode.js and add a single newline character after the
closing IIFE (the "}() );" sequence) so the file ends with a newline to satisfy
the JS/CSS lint job.
Comment on lines
+21
to
+25
| var ariaLabel = ( typeof twoFactorTotpAdmin !== 'undefined' && twoFactorTotpAdmin && twoFactorTotpAdmin.qrCodeAriaLabel ) ? twoFactorTotpAdmin.qrCodeAriaLabel : 'Authenticator App QR Code'; | ||
| title = document.createElement( 'title' ); | ||
| svg.setAttribute( 'role', 'img' ); | ||
| svg.setAttribute( 'aria-label', ariaLabel ); | ||
| title.innerText = ariaLabel; |
There was a problem hiding this comment.
This file still fails the JS lint job.
ariaLabel and authCodeInput are declared inside blocks, and the file is still missing a trailing newline.
🧹 Suggested cleanup
var generateQrCode = function( totpUrl ) {
var $qrLink = $( '#two-factor-qr-code a' ),
+ ariaLabel,
qr,
svg,
title;
@@
svg = $qrLink.find( 'svg' )[ 0 ];
if ( svg ) {
- var ariaLabel = ( typeof twoFactorTotpAdmin !== 'undefined' && twoFactorTotpAdmin && twoFactorTotpAdmin.qrCodeAriaLabel ) ? twoFactorTotpAdmin.qrCodeAriaLabel : 'Authenticator App QR Code';
+ ariaLabel = ( typeof twoFactorTotpAdmin !== 'undefined' && twoFactorTotpAdmin && twoFactorTotpAdmin.qrCodeAriaLabel ) ? twoFactorTotpAdmin.qrCodeAriaLabel : 'Authenticator App QR Code';
title = document.createElement( 'title' );
svg.setAttribute( 'role', 'img' );
svg.setAttribute( 'aria-label', ariaLabel );
@@
if ( checkbox ) {
checkbox.addEventListener( 'click', function( e ) {
+ var authCodeInput;
if ( e.target.checked ) {
- var authCodeInput = document.getElementById( 'two-factor-totp-authcode' );
+ authCodeInput = document.getElementById( 'two-factor-totp-authcode' );
if ( authCodeInput ) {
authCodeInput.focus();
}
}
} );
}
}( jQuery ) );Also applies to: 36-38, 98-98
🧰 Tools
🪛 GitHub Check: Lint JS & CSS
[failure] 21-21:
All 'var' declarations must be at the top of the function scope
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@providers/js/totp-admin.js` around lines 21 - 25, Rename the block-scoped
declarations to use let/const and move their declarations to the appropriate
enclosing scope: replace var ariaLabel with const or let ariaLabel (computed
once from twoFactorTotpAdmin.qrCodeAriaLabel) and ensure authCodeInput is
declared with let/const in the same containing scope where it’s referenced
(instead of inside inner blocks), update any assignments accordingly, and add a
final trailing newline to the file; reference symbols: ariaLabel, authCodeInput,
twoFactorTotpAdmin, svg, title.
Comment on lines
+303
to
+306
| public function test_is_available_for_user_verified() { | ||
| $user = new WP_User( self::factory()->user->create() ); | ||
| update_user_meta( $user->ID, Two_Factor_Email::VERIFIED_META_KEY, true ); | ||
| $this->assertTrue( $this->provider->is_available_for_user( $user ) ); |
There was a problem hiding this comment.
These fixtures still use the old boolean verification marker.
Production now stores the verified email address and compares it to $user->user_email, so writing true here is why both assertions fail.
🧪 Suggested fix
public function test_is_available_for_user_verified() {
$user = new WP_User( self::factory()->user->create() );
- update_user_meta( $user->ID, Two_Factor_Email::VERIFIED_META_KEY, true );
+ update_user_meta( $user->ID, Two_Factor_Email::VERIFIED_META_KEY, $user->user_email );
$this->assertTrue( $this->provider->is_available_for_user( $user ) );
}
@@
public function test_pre_user_options_update_allows_verified() {
$user_id = self::factory()->user->create();
+ $user = get_user_by( 'id', $user_id );
// Set up verified state.
- update_user_meta( $user_id, Two_Factor_Email::VERIFIED_META_KEY, true );
+ update_user_meta( $user_id, Two_Factor_Email::VERIFIED_META_KEY, $user->user_email );Also applies to: 390-395
🧰 Tools
🪛 GitHub Actions: Test
[error] 306-306: Test failure in Tests_Two_Factor_Email::test_is_available_for_user_verified: failed asserting that false is true.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/providers/class-two-factor-email.php` around lines 303 - 306, The test
sets the legacy boolean marker for verification but production expects the
verified email string; update the fixtures in
tests/providers/class-two-factor-email.php (e.g., in
test_is_available_for_user_verified and the other failing case) to write the
user's actual email into Two_Factor_Email::VERIFIED_META_KEY (use
$user->user_email) instead of true so that
Two_Factor_Email::is_available_for_user sees a matching verified email.
Comment on lines
+156
to
+158
| // On the settings page itself, show all providers so admins can change the selection. | ||
| if ( is_admin() && isset( $_GET['page'] ) && 'two-factor-settings' === $_GET['page'] ) { | ||
| return $providers; |
There was a problem hiding this comment.
Gate the bypass on the real settings screen.
$_GET['page'] alone is spoofable on other admin/profile requests. Right now profile.php?page=two-factor-settings will skip the site-enabled provider filter too, which lets users opt back into providers the site admin disabled.
🛠️ Possible fix
function two_factor_filter_enabled_providers( $providers ) {
$site_enabled = two_factor_get_enabled_providers_option();
+ $page = isset( $_GET['page'] ) ? sanitize_key( wp_unslash( $_GET['page'] ) ) : '';
+ global $pagenow;
@@
- if ( is_admin() && isset( $_GET['page'] ) && 'two-factor-settings' === $_GET['page'] ) {
+ if (
+ is_admin() &&
+ current_user_can( 'manage_options' ) &&
+ 'options-general.php' === $pagenow &&
+ 'two-factor-settings' === $page
+ ) {
return $providers;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@two-factor.php` around lines 156 - 158, The current bypass uses only
is_admin() + $_GET['page'] which is spoofable; change the condition to verify
the actual admin screen via get_current_screen() (use get_current_screen() and
check $screen->id or $screen->base for the real two-factor settings screen)
before returning $providers so the bypass only runs on the genuine two-factor
settings screen rather than any request with page=two-factor-settings.
…-on-save Unlinked contributors: WordMessie. Co-authored-by: kasparsd <kasparsd@git.wordpress.org> Co-authored-by: masteradhoc <masteradhoc@git.wordpress.org> Co-authored-by: georgestephanis <georgestephanis@git.wordpress.org> Co-authored-by: dknauss <dpknauss@git.wordpress.org> Co-authored-by: crstauf <crstauf@git.wordpress.org> Co-authored-by: simonwheatley <simonwheatley@git.wordpress.org>
faisalahammad
force-pushed
the
fix/778-email-verification-before-activation
branch
from
4bab1d9 to
412d2fe
Compare
Implements a verification step for the Email provider. Users must verify their email address before the Email 2FA method can be enabled. Legacy users who already have Email 2FA enabled are unaffected. Changes: - Add REST API endpoints for email verification (POST/DELETE /two-factor/1.0/email) - Add VERIFIED_META_KEY to track verified email addresses - Update is_available_for_user() to require verification (with legacy fallback) - Add pre_user_options_update() to prevent enabling without verification - Add email-admin.js for verification UI interactions - Add is_wp_error() guards for get_available_providers_for_user() calls - Add comprehensive REST API and unit tests Fixes WordPress#778
faisalahammad
force-pushed
the
fix/778-email-verification-before-activation
branch
from
412d2fe to
a0f6973
Compare
- Fix enqueue_assets() @SInCE version: 0.10.0 → 0.16.0 - Add missing @SInCE 0.16.0 to register_rest_routes() - Add missing @SInCE 0.16.0 to rest_setup_email() - Add missing @SInCE 0.16.0 to rest_delete_email() - Add missing @SInCE 0.16.0 to pre_user_options_update() - Fix test_user_two_factor_rest_setup_email_valid_code: replace undefined is_provider_enabled_for_user() with in_array check - Fix test_user_can_delete_email_verification: set verified meta before enabling provider - Fix test_admin_can_delete_email_for_others: add enable_provider_for_user call - Fix test_generate_and_email_token_login_context_correct_args: match assertions to actual email body text - Fix test_other_sessions_destroyed_when_enabling_2fa: add verified meta before enabling Email 2FA - Fix test_user_options (backup codes): update assertion for wp_scripts data
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
9 participants
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.
Description
This PR implements a verification step for the Email provider before it can be activated as a Two-Factor Authentication method.
Previously, the Email provider could be enabled without confirming that the user had access to the email address. This could lead to a lockout if the email associated with the account was incorrect or inaccessible.
This change aligns the Email provider's activation flow with the TOTP provider effectively requiring a successful code verification before the
Two_Factor_Emailprovider is added to the user's enabled providers list.Changes
POST /two-factor/1.0/email: Handles sending verification codes and validating them.DELETE /two-factor/1.0/email: Handles resetting the verification status (if needed).Tw_Factor_Email::is_available_for_user()now returnstrueonly if the user has verified their email (checked via_two_factor_email_verifieduser meta).pre_user_options_updatehook to prevent the Email provider from being enabled via the standard profile form save unless the user is verified.How to Test
New User (Fresh Setup)
Legacy User (Existing Setup)
Technical Details
Two_Factor_Emailregister_rest_routes()rest_setup_email()rest_delete_email()pre_user_options_update()user_options(): Updated to render the verification UI.is_available_for_user(): Added verification check (with legacy fallback).generate_and_email_token(): Updated to accept an$actionargument ('login' vs 'verification_setup') to send context-appropriate emails.VERIFIED_META_KEY:_two_factor_email_verifiedChecklist
Fixes WordPress#778
Summary by CodeRabbit
Breaking Changes
New Features
Documentation
Chores