Repository navigation
Conversation
When the WordPress Secrets API is available (WordPress 7.2+, or the secrets-api feature plugin), store each user's TOTP shared secret in it, encrypted, instead of in plaintext user meta. Sites without the API behave exactly as before. - Two_Factor_Secrets adapter: the only caller of the Secrets API. Always uses network scope; detects the API at call time. - Lazy, verified migration of existing plaintext secrets, plus `wp two-factor secrets status|migrate|export`. - Fail closed: an unreadable secret never authenticates. Core now forces the fallback provider (or refuses login) when every enrolled provider is registered but unusable, instead of allowing a password-only login. - Admin notice and Site Health test when migrated secrets become unreachable; lifecycle hooks keep working when TOTP is disabled. - Opt out of writes and migration with the two_factor_use_secrets_api filter. Closes WordPress#455. Supersedes WordPress#819.
--user is a WP-CLI global parameter: WP-CLI consumed it to switch the current user and the command never saw it, so 'secrets export --user=X --yes' exported every user's secret to plaintext instead of just X's. Accept the user positionally, matching 'status <user>' and 'backup-codes generate <user>'.
Closed
Two-Factor should never pull a secret back out of the Secrets API into plaintext user meta. Remove Two_Factor_Totp::export_user_totp_key() and the 'wp two-factor secrets export' action. The worst case after removing the Secrets API is that affected users re-enrol their authenticator app: an administrator resets it from the profile, which works while the API is missing. The admin notice and Site Health result now point to restoring the Secrets API or resetting affected users instead of to export.
The dry run ended with "Dry run: Migrated 0, failed 0, skipped N", which is accurate but reads as if nothing would happen. Report "Dry run: would migrate N, skipped M. Nothing was changed." instead, and only list users who actually hold a plaintext secret, so a dry run for a single user without one is reported as a skip.
The Secrets API wraps one root key with the site key (WP_SECRETS_KEY). If that key changes without the API's own rotation, the root key can't be unwrapped and every read and write fails, including writes of brand new secrets. Re-enrolling after an admin reset then returned a generic 500 "re-scan the QR code" db_error, and the reset itself had already left the user with no second factor. - TOTP setup now returns a 503 two_factor_secrets_key_unavailable that says to restore WP_SECRETS_KEY, or two_factor_secrets_write_failed for other Secrets API write failures. Nothing is stored, and there is still no plaintext fallback while the API is present. - While the key is unusable, the profile says to restore the key and offers no reset, and the REST reset is refused, so the user stays enrolled and kept out instead of being left with a password alone. - Two_Factor_Secrets::is_key_unavailable_error() keeps the Secrets API error code inside the adapter.
Storing TOTP secrets with the Secrets API was automatic whenever the API was present. Because migration is one-directional and a changed secrets key makes every stored secret unreadable, an administrator now has to turn it on under Settings > Two-Factor and acknowledge that first. The choice is a network option, since secrets are network scoped; on multisite only a network administrator can change it. Until it is on, nothing is written or migrated, Site Health recommends it with a link to the settings screen, and `wp two-factor secrets migrate` refuses. Turning it off stops new writes and migration only. The two_factor_use_secrets_api filter now receives the administrator's choice as its default, so it can turn storage off or force it on. `wp two-factor secrets status` reports opted_in and enabled in place of filter_enabled.
Changelogs are generated at release time.
The plugin requires PHP 7.4, so the adapter's methods now declare scalar parameter and return types. Methods returning string|null|WP_Error or true|WP_Error keep an untyped return, as 7.4 has no union types. Two_Factor_Totp casts the user ID (and the key) where it calls the adapter, so its long-standing loosely typed public methods keep accepting what they did before.
The feature plugin was mapped into the tests environment by .wp-env.json. It is now wordpress/secrets-api 0.2.1 in require-dev, and the test bootstrap loads it from vendor/, so the tests no longer depend on the wp-env plugin mapping. The package is not on Packagist, so composer.json declares it as a package repository pointing at the tagged zip. A vcs repository is not usable here: the wp-env container has no git, and the GitHub API it would fall back to is rate limited for unauthenticated CI runs.
The filter now receives whether the Secrets API is actually loaded as a second argument. It still cannot force a missing API to count as present.
Two_Factor_Secrets was all static, so tests reached into it through a public $test_overrides array and the two_factor_secrets_api_present filter, and most of them needed the real Secrets API loaded. The logic now lives in Two_Factor_Secrets_Manager, which is constructed with a Two_Factor_Secrets_Store. Two_Factor_Secrets_Api_Store is the only code that calls the Secrets API. Two_Factor_Secrets keeps every public static method, forwarding to a shared manager, so callers are unchanged. Tests install an in-memory store instead, which removes $test_overrides and the filter and lets the secrets tests run without the feature plugin. A small set of tests still exercises the real store.
All Secrets API handling for TOTP lived in Two_Factor_Totp, and the CLI called that class by name, so a custom provider got none of it. Providers now declare the user meta keys that hold secrets through the static Two_Factor_Provider::user_secret_meta_keys(). The storage logic (verified writes, lazy one-way migration, deletion, storage state) moves into Two_Factor_Secrets_Manager, generalised over a slug and a meta key. Two_Factor_Secrets_Lifecycle handles deletion with the user and on uninstall, the unreachable-secrets notice, Site Health and the counts for every declared secret, registered from Two_Factor_Core::add_hooks(). Two_Factor_Totp's public methods keep their signatures and become wrappers. Its slug, marker key and secret name are unchanged, so nothing already stored moves. Two_Factor_Totp::register_secret_lifecycle_hooks() is removed. Two_Factor_Core::get_provider_secrets() lists secrets from every provider class, including ones turned off site-wide, without instantiating any. A duplicate slug is refused. wp two-factor secrets migrate covers every declared secret, and secrets status prints one row per secret.
Let providers declare their secrets; move secrets logic behind an injectable store
Resolve the composer.lock conflict: keep master's phpstan 2.3.0 bump and this branch's wordpress/secrets-api entry, and regenerate the content hash.
✅ WordPress Plugin Check Report
📊 ReportAll checks passed! No errors or warnings found. 🤖 Generated by WordPress Plugin Check Action • Learn more about Plugin Check |
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.
Store TOTP secrets with the WordPress Secrets API
Closes WordPress#455. Supersedes WordPress#819.
Motivation
Two-Factor keeps every user's TOTP shared secret in plaintext user meta (
_two_factor_totp_key,Two_Factor_Totp::SECRET_META_KEY). Anyone who can read the database, a backup, or a SQL injection dump can mint valid codes for every TOTP user.Two earlier attempts encrypted the secret inside Two-Factor itself:
SECURE_AUTH_SALT. It stalled because rotating the salt needed a bespoke re-encryption process.wp-config.phpkeys. In review, @georgestephanis preferred an independent architecture that both Two-Factor and other plugins could share over each plugin building its own. Encrypt TOTP secrets at rest using AES-256-GCM WordPress/two-factor#819 was closed so this integration could replace it.Both made Two-Factor responsible for cryptography and key management.
The WordPress Secrets API removes that burden. It is tracked in Trac #66187 (milestone 7.2), core PR WordPress/wordpress-develop#13759, and is available today as a feature plugin (
ericmann/secrets-api, v0.2.1). This change uses it when present and changes nothing when it is not.Answering the fail-open question from WordPress#819
On WordPress#819, @georgestephanis asked whether a decryption failure after a bad or rotated key could fail open, leaving TOTP unenforced instead of blocking the login. Here it cannot, and that is deliberate:
Two_Factor_Provider::is_enrolled_but_unavailable_for_user()lets core force the fallback in that case, or refuse the login outright withno_available_2fa_methods. Regression tests cover a user whose only method is TOTP.WP_SECRETS_KEY. The user stays locked out until an administrator resets their authenticator app (backup codes still work). We deliberately do not quietly move them to email, because a changed or broken key should be noticed, not papered over.Design summary
Two_Factor_Secrets(class-two-factor-secrets.php) is a static class and the only code that calls the Secrets API. Everything else goes through it. Presence is detected at call time, never when files load.two-factor/totp-{user_id}and always stored with the network functions, on single site and multisite alike. The API has no per-user scope, and site-scope secrets are bound to the blog ID, which would break a user moving between sites._two_factor_totp_key_network(Two_Factor_Totp::SECRET_NETWORK_META_KEY) holds the network ID that owns the secret. It is what tells "not migrated" apart from "migrated but unreadable".get_user_totp_key_state()returns a string,null(no key) orWP_Error(a key exists but cannot be read).get_user_totp_key()keeps its signature and returns''for both non-string states.hash_equals(). The plaintext copy is deleted only after that succeeds. A Secrets API failure never falls back to writing plaintext.no_available_2fa_methodserror if no valid fallback exists. They are never let in with one factor.WP_SECRETS_KEY, or a missing secret row), TOTP stays available and is not replaced by the fallback. The user cannot authenticate and is locked out until an administrator resets them.two_factor_use_secrets_apifilter: opt out of writes and migration. Already-migrated users are still read from the Secrets API while it is present.two_factor_secrets_migrated,two_factor_secrets_migration_failedandtwo_factor_secret_unavailableactions. No hook ever receives a plaintext secret.two_factor_secrets_api_presentis an internal test seam that can only force "absent".is_enrolled_but_unavailable_for_user()and the staticuninstall_user_data(). Core stays free of TOTP logic.Two_Factor_Totp::register_secret_lifecycle_hooks()registers user-deletion cleanup, the admin notice and the Site Health test statically.two-factor.phpalways loads the TOTP class and calls it. The provider itself is not instantiated, so its REST routes stay off while it is disabled.Migration and downgrade story
wp two-factor secrets migrate [<user>] [--batch-size=<n>] [--dry-run]uses the same code path in bounded batches.wp two-factor secrets statusand atotp_storagefield onwp two-factor status <user>(plaintext,secrets-api,unavailable,none).manage_options/manage_network_optionsonly) and a critical Site Health result,two_factor_totp_secret_storage. Detection is a cheap single-row query cached in a site transient.wp two-factor secrets export [<user>] [--batch-size=<n>] [--yes]moves secrets back into user meta (with confirmation) before the API is removed. While the API is active andtwo_factor_use_secrets_apiallows writes, lazy migration moves exported secrets back on the next read. To decommission: return false fromtwo_factor_use_secrets_api, run export, then deactivate the Secrets API.wpmu_delete_useron multisite, so removal from a single site does not delete a network secret). Uninstall removes every secret while the API is available. Secrets left in the store when the API is absent at uninstall are orphaned; there is nothing to reach them with.Compatibility
Requires at leastbump, and no new package dependency. The Secrets API is optional.two_factor_use_secrets_apifilter only. There is no settings UI.Two_Factor_Totpare unchanged.includes/is untouched.Testing notes
env.tests.pluginsin.wp-env.json(ericmann/secrets-api#v0.2.1, with"."repeated because an env-levelpluginskey replaces the root one). CI needs no change because wp-env clones the source itself.WP_SECRETS_KEYis set to a fixed, test-only value so encryption is deterministic.tests/bootstrap.phploadssecrets-api.phpif present (override withTWO_FACTOR_SECRETS_API_FILE); when it is absent the suite still runs and API-dependent tests skip.npm run composer -- test -- --group secrets. Shared helpers live inTwo_Factor_Secrets_UnitTestCase.tests/phpstan/, excluded from PHPUnit because they would redeclare the real functions.Open questions for maintainers
@internalfor now?LOGGED_IN_*salt-rotation hazard acceptable withoutWP_SECRETS_KEY, or should Two-Factor surface a Site Health recommendation to define it?two-factor/totp-{ID}puts one row per TOTP user in the options table. Is that acceptable on large networks, versus waiting for a user-scoped store in the Secrets API?Two_Factor_Provider::is_enrolled_but_unavailable_for_user()is a generic core hook that lets any provider say "enrolled, but currently unusable", so core forces the fallback instead of failing open. Is that the right name and shape for a core-level concept?Two_Factor_Provider::uninstall_user_data()is a bulk, static hook called once during uninstall for data held outside user meta. Would maintainers prefer a different name or a per-user shape?