masteradhoc · nimesh-xecurify · Aug 3, 2026
diff --git a/CLI/class-two-factor-cli-command.php b/CLI/class-two-factor-cli-command.php
@@ -216,6 +216,20 @@ public function disable( $args, $assoc_args ) {
 	private function disable_single_provider( $user, $provider, $assoc_args ) {
 		$enabled = Two_Factor_Core::get_enabled_providers_for_user( $user );
 
+		// Reject unknown provider keys up front. Without this a typo or the wrong
+		// casing (Two_Factor_TOTP) would fall through to the "not enabled" branch
+		// below and exit zero, while the real provider stayed enabled.
+		if ( ! array_key_exists( $provider, Two_Factor_Core::get_supported_providers_for_user( $user ) ) ) {
+			WP_CLI::error(
+				sprintf(
+					/* translators: 1: provider class name, 2: user login */
+					__( 'Unknown provider "%1$s". Run "wp two-factor list-providers" to see the registered providers, or "wp two-factor disable %2$s" to clear all two-factor state.', 'two-factor' ),
+					$provider,
+					$user->user_login
+				)
+			);
+		}
+
 		if ( ! in_array( $provider, $enabled, true ) ) {
 			WP_CLI::success(
 				sprintf(
@@ -268,6 +282,65 @@ private function disable_single_provider( $user, $provider, $assoc_args ) {
 		}
 	}
 
+	/**
+	 * User meta keys holding two-factor state that a full reset clears.
+	 *
+	 * Two_Factor_Core::USER_PASSWORD_WAS_RESET_KEY is deliberately absent: it
+	 * records a password compromise rather than 2FA configuration and is
+	 * preserved across a reset.
+	 *
+	 * @since 0.17.0
+	 *
+	 * @return string[] Meta key names.
+	 */
+	private function two_factor_state_meta_keys() {
+		$keys = array(
+			Two_Factor_Core::ENABLED_PROVIDERS_USER_META_KEY,
+			Two_Factor_Core::PROVIDER_USER_META_KEY,
+			Two_Factor_Core::USER_META_NONCE_KEY,
+			Two_Factor_Core::USER_RATE_LIMIT_KEY,
+			Two_Factor_Core::USER_FAILED_LOGIN_ATTEMPTS_KEY,
+		);
+
+		if ( class_exists( 'Two_Factor_Totp' ) ) {
+			$keys[] = Two_Factor_Totp::SECRET_META_KEY;
+			$keys[] = Two_Factor_Totp::LAST_SUCCESSFUL_LOGIN_META_KEY;
+		}
+
+		if ( class_exists( 'Two_Factor_Backup_Codes' ) ) {
+			$keys[] = Two_Factor_Backup_Codes::BACKUP_CODES_META_KEY;
+		}
+
+		if ( class_exists( 'Two_Factor_Email' ) ) {
+			$keys[] = Two_Factor_Email::TOKEN_META_KEY;
+			$keys[] = Two_Factor_Email::TOKEN_META_KEY_TIMESTAMP;
+		}
+
+		return $keys;
+	}
+
+	/**
+	 * Whether a user carries any two-factor state a full reset would clear.
+	 *
+	 * Active sessions are intentionally not treated as two-factor state, so a
+	 * reset against an account that never used 2FA stays a true no-op instead of
+	 * logging the user out.
+	 *
+	 * @since 0.17.0
+	 *
+	 * @param WP_User $user Target user.
+	 * @return bool True when there is something to clean up.
+	 */
+	private function has_residual_two_factor_state( $user ) {
+		foreach ( $this->two_factor_state_meta_keys() as $meta_key ) {
+			if ( ! empty( get_user_meta( $user->ID, $meta_key, true ) ) ) {
+				return true;
+			}
+		}
+
+		return false;
+	}
+
 	/**
 	 * Disable all 2FA providers and clean up all residual state for a user.
 	 *
@@ -278,9 +351,12 @@ private function disable_single_provider( $user, $provider, $assoc_args ) {
 	 */
 	private function disable_all_providers( $user, $assoc_args ) {
 		$enabled = Two_Factor_Core::get_enabled_providers_for_user( $user );
-		$raw     = get_user_meta( $user->ID, Two_Factor_Core::ENABLED_PROVIDERS_USER_META_KEY, true );
 
-		if ( empty( $enabled ) && empty( $raw ) ) {
+		// Gate on all residual state, not just the enabled-providers list. An
+		// account can carry a TOTP secret, recovery-code hashes, a login nonce or
+		// an active throttle while that list is empty; returning early here would
+		// report "already disabled" and leave every one of them in place.
+		if ( ! $this->has_residual_two_factor_state( $user ) ) {
 			WP_CLI::success(
 				sprintf(
 					/* translators: %s: user login */
@@ -564,6 +640,19 @@ public function backup_codes( $args, $assoc_args ) {
 			WP_CLI::error( __( 'The Two_Factor_Backup_Codes provider is not available.', 'two-factor' ) );
 		}
 
+		// Confirm the provider can actually be enabled before writing anything.
+		// generate_codes() replaces the stored hashes, so failing afterwards would
+		// destroy the old codes and leave unusable new ones the operator never saw.
+		if ( ! array_key_exists( 'Two_Factor_Backup_Codes', Two_Factor_Core::get_supported_providers_for_user( $user ) ) ) {
+			WP_CLI::error(
+				sprintf(
+					/* translators: %s: user login */
+					__( 'The Two_Factor_Backup_Codes provider is not registered for user %s, so generated codes could not be used. No codes were changed.', 'two-factor' ),
+					$user->user_login
+				)
+			);
+		}
+
 		$raw_count = WP_CLI\Utils\get_flag_value( $assoc_args, 'count', (string) Two_Factor_Backup_Codes::NUMBER_OF_CODES );
 		if ( ! is_scalar( $raw_count ) ) {
 			WP_CLI::error(
@@ -627,10 +716,15 @@ public function backup_codes( $args, $assoc_args ) {
 		// is never offered, so the user is rejected at the 2FA step.
 		$providers_before = Two_Factor_Core::get_enabled_providers_for_user( $user );
 		if ( ! Two_Factor_Core::enable_provider_for_user( $user->ID, 'Two_Factor_Backup_Codes' ) ) {
+			// Unreachable via the pre-flight check above, but if enablement still
+			// fails the freshly written hashes are unusable — drop them rather than
+			// leaving unusable recovery material behind.
+			delete_user_meta( $user->ID, Two_Factor_Backup_Codes::BACKUP_CODES_META_KEY );
+
 			WP_CLI::error(
 				sprintf(
 					/* translators: %s: user login */
-					__( 'Backup codes were generated but the provider could not be enabled for user %s.', 'two-factor' ),
+					__( 'Backup codes could not be enabled for user %s. No codes were stored.', 'two-factor' ),
 					$user->user_login
 				)
 			);

diff --git a/TESTS.md b/TESTS.md
@@ -167,9 +167,9 @@ that capture output for assertions and throw on `error()`/`confirm()`:
 - `status` — output for users with and without 2FA, backup-code count, `--format` passthrough
 - `list-providers` — registered providers listed, `--format` passthrough
 - `enable` — enabling secret-free providers; session destruction on change; refusing TOTP (no stale "Phase 3" pointer) and backup codes; unknown provider and missing-argument errors
-- `disable` (single provider) — removal leaves others intact, session destruction on change, idempotent no-op, confirmation required without `--yes`
-- `disable` (all) — full reset clears providers/throttle/nonce state and destroys sessions, preserves the compromised-password-reset flag, idempotent no-op, stale-meta cleanup guarding the fail-closed email fallback, confirmation required without `--yes`
-- `backup-codes generate` — default and `--count` code counts, regeneration replaces the set, enables the provider so codes are usable at login, session destruction when first enabled, unknown-action and missing-argument errors
+- `disable` (single provider) — removal leaves others intact (TOTP also drops its stored secret), session destruction on change, idempotent no-op, unregistered/mis-cased provider keys fail instead of reporting a false success, confirmation required without `--yes`
+- `disable` (all) — full reset clears providers/throttle/nonce state and destroys sessions, also cleans residual secrets and lockout state when the enabled-providers list is already empty, preserves the compromised-password-reset flag, idempotent no-op, stale-meta cleanup guarding the fail-closed email fallback, confirmation required without `--yes`
+- `backup-codes generate` — default and `--count` code counts, regeneration replaces the set, enables the provider so codes are usable at login, stores no hashes when the provider cannot be enabled, session destruction when first enabled, unknown-action and missing-argument errors
 - `unlock` — clears the login throttle for a rate-limited user; no-op message otherwise
 
 ## Test Helpers

diff --git a/phpcs.xml.dist b/phpcs.xml.dist
@@ -40,6 +40,11 @@
 		<exclude-pattern>tests/providers/*.php</exclude-pattern>
 	</rule>
 
+	<!-- WPCOM_VIP_CLI_Command only exists on the VIP platform; a general-purpose plugin must extend WP_CLI_Command. -->
+	<rule ref="WordPressVIPMinimum.Classes.RestrictedExtendClasses.wp_cli">
+		<exclude-pattern>CLI/*.php</exclude-pattern>
+	</rule>
+
 	<exclude-pattern>*/wordpress/*</exclude-pattern>
 	<exclude-pattern>*/dist/*</exclude-pattern>
 	<exclude-pattern>*/includes/*</exclude-pattern>

diff --git a/tests/cli/class-two-factor-cli-command.php b/tests/cli/class-two-factor-cli-command.php
@@ -433,7 +433,8 @@ public function test_disable_single_totp_clears_stored_secret() {
 		$this->enable_provider( 'Two_Factor_Totp' );
 		$totp = Two_Factor_Totp::get_instance();
 
-		$this->assertTrue( $totp->set_user_totp_key( $this->user->ID, Two_Factor_Totp::generate_key() ) );
+		// update_user_meta() returns the new meta ID (int) on insert, true on update.
+		$this->assertNotFalse( $totp->set_user_totp_key( $this->user->ID, Two_Factor_Totp::generate_key() ) );
 		$this->assertNotSame( '', $totp->get_user_totp_key( $this->user->ID ) );
 
 		$this->command->disable( array( 'cli_test_user', 'Two_Factor_Totp' ), array( 'yes' => true ) );
@@ -467,6 +468,31 @@ public function test_disable_single_provider_not_enabled() {
 		$this->assertStringContainsString( 'no changes made', $this->last_message( 'success' ) );
 	}
 
+	/**
+	 * Disabling an unregistered provider key fails loudly instead of reporting success.
+	 *
+	 * A mis-typed or mis-cased class name must not exit zero with "no changes
+	 * made" while the real provider quietly stays enabled with its secret.
+	 *
+	 * @covers Two_Factor_CLI_Command::disable
+	 */
+	public function test_disable_single_provider_unknown_key_errors() {
+		$this->enable_provider( 'Two_Factor_Totp' );
+
+		$message = $this->assert_command_aborts(
+			function () {
+				$this->command->disable( array( 'cli_test_user', 'Two_Factor_TOTP' ), array( 'yes' => true ) );
+			}
+		);
+
+		$this->assertStringContainsString( 'Two_Factor_TOTP', $message );
+		$this->assertContains(
+			'Two_Factor_Totp',
+			Two_Factor_Core::get_enabled_providers_for_user( $this->user ),
+			'The correctly-cased provider must stay enabled after a failed disable.'
+		);
+	}
+
 	/**
 	 * Disabling a single provider requires confirmation without --yes.
 	 *
@@ -555,6 +581,40 @@ public function test_disable_all_providers_when_already_disabled() {
 		$this->assertStringContainsString( 'already disabled', $this->last_message( 'success' ) );
 	}
 
+	/**
+	 * A full reset clears residual state even when no providers are enabled.
+	 *
+	 * An account can still carry a TOTP secret, recovery-code hashes, a login
+	 * nonce and an active throttle while the enabled-providers list is empty.
+	 * Reporting "already disabled" and returning would leave all of it behind.
+	 *
+	 * @covers Two_Factor_CLI_Command::disable
+	 */
+	public function test_disable_all_providers_clears_residual_state_without_enabled_providers() {
+		$totp = Two_Factor_Totp::get_instance();
+		$totp->set_user_totp_key( $this->user->ID, Two_Factor_Totp::generate_key() );
+		Two_Factor_Backup_Codes::get_instance()->generate_codes( $this->user, array( 'method' => 'replace' ) );
+		update_user_meta( $this->user->ID, Two_Factor_Core::USER_META_NONCE_KEY, array( 'key' => 'nonce-value' ) );
+		update_user_meta( $this->user->ID, Two_Factor_Core::USER_RATE_LIMIT_KEY, time() );
+		update_user_meta( $this->user->ID, Two_Factor_Core::USER_FAILED_LOGIN_ATTEMPTS_KEY, 7 );
+		$this->create_user_session();
+
+		$this->assertEmpty(
+			Two_Factor_Core::get_enabled_providers_for_user( $this->user ),
+			'This scenario requires an empty enabled-providers list.'
+		);
+
+		$this->command->disable( array( 'cli_test_user' ), array( 'yes' => true ) );
+
+		$this->assertSame( '', $totp->get_user_totp_key( $this->user->ID ) );
+		$this->assertSame( 0, Two_Factor_Backup_Codes::codes_remaining_for_user( $this->user ) );
+		$this->assertEmpty( get_user_meta( $this->user->ID, Two_Factor_Core::USER_META_NONCE_KEY, true ) );
+		$this->assertEmpty( get_user_meta( $this->user->ID, Two_Factor_Core::USER_RATE_LIMIT_KEY, true ) );
+		$this->assertEmpty( get_user_meta( $this->user->ID, Two_Factor_Core::USER_FAILED_LOGIN_ATTEMPTS_KEY, true ) );
+		$this->assertSame( 0, $this->count_user_sessions() );
+		$this->assertStringContainsString( 'All 2FA disabled', $this->last_message( 'success' ) );
+	}
+
 	/**
 	 * A full disable clears stale meta even when the provider class no longer exists.
 	 *
@@ -777,7 +837,42 @@ function () {
 	}
 
 	/**
-	 * An unknown backup-codes action aborts with an error.
+	 * No backup-code hashes are stored when the provider cannot be enabled.
+	 *
+	 * Generating first and enabling second leaves unusable hashes behind when
+	 * enablement fails, with no plaintext codes shown to the operator.
+	 *
+	 * @covers Two_Factor_CLI_Command::backup_codes
+	 */
+	public function test_backup_codes_generate_does_not_store_hashes_when_provider_unavailable() {
+		$deregister = function ( $providers ) {
+			unset( $providers['Two_Factor_Backup_Codes'] );
+
+			return $providers;
+		};
+
+		add_filter( 'two_factor_providers', $deregister );
+
+		try {
+			$this->assert_command_aborts(
+				function () {
+					$this->command->backup_codes( array( 'generate', 'cli_test_user' ), array() );
+				}
+			);
+		} finally {
+			remove_filter( 'two_factor_providers', $deregister );
+		}
+
+		$this->assertSame(
+			0,
+			Two_Factor_Backup_Codes::codes_remaining_for_user( $this->user ),
+			'Unusable hashes must not be left behind when the provider cannot be enabled.'
+		);
+		$this->assertEmpty( Two_Factor_Core::get_enabled_providers_for_user( $this->user ) );
+	}
+
+	/**
+	 * An unknown backup-codes action is rejected.
 	 *
 	 * @covers Two_Factor_CLI_Command::backup_codes
 	 */