Skip to content

fix(mfa): harden MFA against bypasses found in a security review - #91

Open
bordoni wants to merge 2 commits into
feat/mfa-adminfrom
fix/mfa-security
Open

bordoni wants to merge 2 commits into
feat/mfa-adminfrom
fix/mfa-security

Conversation

@bordoni

@bordoni bordoni commented Oct 18, 2026 •

Copy link
Copy Markdown
Owner

Fixes from a security review of the MFA stack before 1.1.0. Stacked on #90. Part of #28.

The review looked for three kinds of problem: ways to get a session without passing MFA, weak spots in the sign-in code step, and ways a stolen session could change MFA. Each finding below was checked against the code. The WorkOS behavior it depends on was tested against the staging environment.

High

  • Bearer tokens skipped MFA. REST\TokenAuth accepted any valid WorkOS access token for the environment. That includes tokens from another site, a staging copy, or hosted AuthKit, none of which ran this site's MFA. For users who have or need MFA, REST now accepts only tokens whose session (sid) signed in on this site (Mfa\VerifiedSessions). New filters: workos_mfa_accept_bearer_token and the general workos_bearer_token_user.
  • Anyone could reset their own MFA without a code. WordPress grants every role edit_user on itself, so the admin routes (/mfa/required, /mfa/reset) let a hijacked session wipe its factors and clear the require flag. The admin routes and tools now never apply to your own account.
  • XML-RPC multicall skipped the password guard. The application-password exemption checked did_action() for the whole request. One multicall could authenticate an attacker's application password, then the victim's password, and the guard stepped aside. The exemption now covers only the user whose application password it was, in that sign-in.
  • No per-account limit on wrong codes. Someone with the password could keep starting new sign-ins, each allowing 5 guesses, and parallel requests could get past the per-token count. New Mfa\CodeGuard: 5 wrong codes per 15 minutes and 30 a day per account, across sign-in, the account panel and the legacy routes. Only one code check per account runs at a time, under a lock (wp_cache_add, or INSERT IGNORE without an object cache). Right codes are refunded.
  • Factors outside the mirrored flag were ignored. The policy trusted _workos_mfa_enabled. A factor created on hosted AuthKit, in the dashboard, by another app or before this version got none (a password-only sign-in) or enroll (an attacker adds their own app). The policy now reads factors from WorkOS at every sign-in. The flag follows WorkOS both ways: a factor removed in WorkOS turns MFA off in WordPress (activity-log mfa.removed_in_workos, action workos_mfa_removed_in_workos, an email to the user), and users who must use MFA enroll again. A failed WorkOS call never clears it. Checked live on iwos110: factor added and verified in WorkOS → WordPress picked it up → deleted in WorkOS → next password sign-in went straight to wp-admin with MFA off.

Medium

  • The sign-in step now always uses an mfa_token. WorkOS's own mfa_challenge is parked like the plugin's. /auth/mfa/challenge and /auth/mfa/verify refuse requests without a token, so nobody can challenge an arbitrary factor or pair a victim's pending token with their own challenge. A code must come from the factor its challenge belongs to (challenge.authentication_factor_id). That also fixes verifying against the wrong factor when a user has more than one.
  • A stolen session could add a second authenticator. Enrolling on an account that already has a factor now needs a current code from one of them (factor_id + code, else 403 workos_mfa_code_required). This covers the account panel and the legacy /auth/mfa/totp/enroll and /auth/mfa/sms/enroll, which also respect workos_mfa_can_manage now. The sign-in enroll step refuses if a factor has appeared since the policy decided.
  • Re-linking could replace the account's factors. A WP user with MFA is never re-linked by email to a different WorkOS user, whatever the Re-link Protection setting (workos_relink_protected). wp workos user link --force still works.
  • The password guard ignored an always profile. It now also refuses users linked to WorkOS whom the default profile would ask for a code. Unlinked local admins aren't affected.
  • The hosted return URL carried the live token. It now carries a one-time, 60-second resume code. The page strips it with history.replaceState and sends Referrer-Policy: no-referrer.
  • Parked sign-ins could be kept alive. They now expire 10 minutes after the first factor, however often they're touched, and an update never brings back a forgotten record. The parked WorkOS response (refresh token included) is encrypted with a key derived from the token, which the server doesn't store.
  • Sessions and notifications on changes. Adding or removing an authenticator emails the account and signs out its other sessions. An admin reset signs the user out everywhere, clears the stored tokens and revokes the WorkOS session (Mfa\AccountChanges, filter workos_mfa_notify_user).

Low

  • A confirm with another factor's challenge now fails instead of marking the pending factor confirmed.
  • Unconfirmed-factor cleanup keeps tracking a factor WorkOS couldn't delete, and enrollments run one at a time per account, so a leftover can't count as confirmed.
  • Headless mode refuses an MFA user before storing tokens or firing workos_user_authenticated.
  • The admin panel applies its changes on profile_update, after core validates the form, and shows a reset WorkOS refused as a notice.
  • MFA route errors no longer pass on WorkOS's response body.
  • status() returns must_use (flag, role or filter), and the panel's "only app" warning uses it.
  • Factor lists are read strictly from data.

Checked against WorkOS staging (a throwaway factor on the demo user, deleted afterwards)

  • 3 wrong codes lock a challenge (one_time_code_too_many_attempts), after which even the right code fails.
  • A challenge verifies once (authentication_challenge_previously_verified), and a used code fails on a new challenge.
  • Codes ±2 steps (about ±60 s) are accepted, so 5 codes are valid at any moment. That is why the per-account daily cap matters.
  • Verify responses include challenge.authentication_factor_id.

Not changed (follow-ups)

  • TokenAuth::is_rest_request() matches /wp-json/ anywhere in the URI, and TokenAuth doesn't check deactivation. Neither is specific to MFA.
  • The activity log is off by default, so mfa.* events aren't recorded unless it's turned on.
  • Enrolling at sign-in still trusts whoever passed the first factor. The owner now gets an email; it's documented under Known limits in docs/mfa.md.

Contract changes (all inside unreleased 1.1.0 work, apart from the two routes)

  • /auth/mfa/challenge and /auth/mfa/verify need mfa_token. Before this stack they never worked end to end against WorkOS.
  • PUT /users/{id}/mfa/required and POST …/mfa/reset refuse your own id.
  • POST /users/{id}/mfa/enroll needs factor_id + code when a factor exists.

Verified

  • New MfaSecurityTest: 20 tests, at least one per fix, written as the attack.
  • Existing tests updated for the contract changes above.
  • Local WPUnit: 885 tests, with the same 11 environment-only failures as clean main. PHPCS, PHPStan, tsc and the webpack build pass.
  • Live on the throwaway demo site, against WorkOS staging, with this branch: enrolling at sign-in, and the sign-in challenge (wrong code refused, right code signs in), both end signed in.
  • Docs: a new "Security model" section in docs/mfa.md, plus docs/rate-limiting.md and CHANGELOG.

- Bearer tokens from other apps on the WorkOS environment no longer act
  for users with MFA; only sessions that signed in here (VerifiedSessions).
- Admin MFA routes and tools never apply to your own account (edit_user
  is granted on yourself to every role).
- Adding a second authenticator needs a current code from an existing one,
  on the account panel and the legacy TOTP/SMS routes.
- Wrong codes capped per account (5/15 min, 30/day) under a lock
  (CodeGuard); a code must come from its challenge's factor.
- Sign-in /challenge and /verify need an mfa_token; WorkOS mfa_challenge
  is parked too, so the pending token never reaches the browser.
- Policy reads factors from WorkOS at every sign-in; the enabled flag is
  sticky, so vanished factors mean enroll, not password-only.
- Users with MFA are never re-linked by email; app-password exemption is
  per user (XML-RPC multicall); guard follows an 'always' default profile.
- Parked sign-ins: fixed expiry, encrypted response, one-time resume code
  in the hosted return URL.
- Factor changes email the account and sign out other sessions; admin
  reset signs out everywhere and revokes the WorkOS session.
- Headless refuses before storing tokens; admin panel applies after save;
  MFA errors drop WorkOS response bodies.
@bordoni
bordoni added this pull request to stack #87 October 18, 2026 20:39
@bordoni bordoni added this to the 1.1.0 milestone Oct 18, 2026
WorkOS is the source of truth for a user's factors. When WorkOS answers
with none, list_factors() clears _workos_mfa_enabled instead of keeping
it, logs mfa.removed_in_workos, fires workos_mfa_removed_in_workos and
emails the user. A failed WorkOS call never clears the flag, and the
require flag and roles are untouched, so required users enroll again.
@bordoni
bordoni marked this pull request as ready for review October 19, 2026 05:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant