Fix: Users list table fatals (wp_die) when a user's 2FA provider is deregistered#933
Fix: Users list table fatals (wp_die) when a user's 2FA provider is deregistered#933masteradhoc wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes an admin-facing fatal (wp_die()) that could break the entire wp-admin Users list table when a user’s only enabled 2FA provider has been deregistered (e.g., via two_factor_providers), by making provider resolution error-tolerant in shared UI contexts while still failing closed during the affected user’s own login flow.
Changes:
- Updated
get_primary_provider_for_user()to return aWP_Errorinstead of callingwp_die()when a user’s enabled provider(s) are no longer registered. - Hardened admin/UI call sites (e.g., Users list table and user profile 2FA settings rendering) to handle
WP_Errorsafely without truncating the page. - Ensured authentication enforcement remains secure by treating the
WP_Errorstate as “still using 2FA” (prevents failing open).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if ( is_wp_error( $provider ) ) { | ||
| // The user has a provider enabled that's no longer registered on the site. Show a clear, | ||
| // non-fatal indicator instead of erroring out — this must never wp_die(), since that would | ||
| // truncate the Users list table for every admin viewing the page, not just this one user's row. | ||
| return sprintf( '<span class="dashicons-before dashicons-warning">%s</span>', esc_html__( 'Error: legacy 2FA method', 'two-factor' ) ); | ||
| } |
chubes4
left a comment
There was a problem hiding this comment.
Fuzzed this against a disposable path-multisite network (seed 9338501: 5,000 randomized provider states, 500 transition sequences, 1,500 cross-site steps). The stale Users-row behavior generally remains fail-closed and no provider/meta leakage or cross-blog bleed appeared, but one PR-path fatal remains:
class-two-factor-core.php:1144-1152callsarray_diff_key()before checking whether the second availability lookup returnedWP_Error. If a provider is deregistered between resolution calls,login_html()throwsTypeError: array_diff_key(): Argument #1 ($array) must be of type array, WP_Error given. Move theWP_Errorguard ahead ofarray_diff_key()and retain fail-closed login behavior.
Minimized replay: make the selected provider available for the initial provider lookups, remove it via two_factor_providers before the subsequent availability lookup, then invoke login_html().
Current CI/lint blockers also need resolution: PHPStan flags the new is_wp_error() branch against the current return annotation, and PHPCS rejects the new wp_die( WP_Error ) calls as unescaped output. The original Users-list fix otherwise survived the state campaign, including subsequent-row rendering and malicious stale provider values. Happy to rerun seed 9338501 after the guard/type fixes.
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Thanks @chubes4 - ready for another review when you are ;) |
chubes4
left a comment
There was a problem hiding this comment.
Second pass against 4fde20d covered 50,001 randomized provider states across three seeds, 5,000 transition sequences/15,000 cross-site steps, PHP 7.4 + WP 6.9, PHP 8.3 + trunk, PHP 8.5/current, and browser rendering with 64 mixed users. The prior login_html() array_diff_key(WP_Error) TypeError is fixed and CI is green, but provider-filter churn exposed a remaining fail-open authentication schedule:
- With successive
two_factor_providerssnapshots[present, absent, present], the initial supported lookup sees the persisted enabled provider, the availability lookup returns an empty array, and the later enabled lookup sees it again.is_user_using_two_factor()then returns false (class-two-factor-core.php:702-735,:822-875), allowingfilter_authenticate()at:936-953to leave cookies/REST/XML-RPC password auth unrestricted. Reproduced on PHP 7.4/WP 6.9 and PHP 8.3/trunk. Tracked in #943.
The authentication decision needs one consistent provider snapshot or another durable fail-closed signal for the entire request. Add a regression covering this exact call schedule; fixing only UI/error callers is insufficient.
Adjacent pre-existing malformed-meta crashes also remain: enabled-provider scalar/object values (#941) and primary-provider object/array offsets (#944). They may stay separately scoped, but tests for #933 should include stale-row and fail-closed authentication behavior because the PR changes a public return contract and currently adds none.
No cross-blog bleed, malicious-label leakage, unrelated-row truncation, or extra page errors appeared. Browser assertions passed across main/alpha/network user lists and stale profiles. Worst bounded render probe (1,000 rows × 512 providers) remained approximately O(rows × providers), ~18-19s/50MB. Happy to rerun seeds 9338501-9338503 after the auth snapshot fix.
What?
Fixes #932
Prevents the wp-admin Users list table from fataling (
wp_die()) when a user's only enabled 2FA provider has been deregistered.Why?
get_primary_provider_for_user()callswp_die()whenget_available_providers_for_user()returns aWP_Error(which happens when a user's saved provider no longer exists and Email isn't available as a fallback). This is called per-row frommanage_users_custom_column(), so one user's stale data breaks the entire Users list page for any admin viewing it, including unrelated UI like Screen Options.How?
wp_die()call inget_primary_provider_for_user(); it now returnsnullon error instead of halting execution.manage_users_custom_column()to check fornull/WP_Errorbefore calling->get_label(), rendering a safe "legacy/unavailable method" string instead.get_primary_provider_for_user()to confirm none assume a valid object is always returned.Use of AI Tools
AI assistance: Yes
Tool(s): Claude
Model(s): Claude Sonnet 5
Used for: Root cause analysis and initial fix suggestions; final implementation and tests reviewed and edited by me.
Testing Instructions
Two_Factor_Emailfrom registered providers via atwo_factor_providersfilter.Screenshots or screencast
Changelog Entry