Fix the findings of a full second security audit

Four-part audit (OIDC/JWT/crypto, user sync, admin UI, login bypasses) with dynamic PoCs against a real WordPress install; every fix is covered by a regression test. Report: docs/security-audit.md, section 6. Critical/High - Multisite: settings, AJAX actions and certificate download require manage_network_options (site admins could sign in as super admin). - Privileged accounts are only linked (sync and first sign-in) via a matching UPN of a member account, never via the settable mail attribute; the sync never changes their e-mail address; e-mail change notifications stay on. - Button-only mode exempts by credential (application passwords, WP-CLI) instead of request context, closing bypasses through xmlrpc.php and REST login handlers; API requests never receive login cookies. - Multi-tenant mode refuses guest/external identities. Medium/Low - Same message for right and wrong passwords; button-only no longer switches off when the connection breaks; server-side fallback cookie expiry; correct fallback key beats IP lockouts; right-most proxy hop; higher start limit; one object ID per account. - Deactivation sets a random password, revokes application passwords and removes the role (restored on reactivation); disabled people are deactivated even when their mail vanished; duplicate bindings handled. - Sync: abort on empty directory answer, no deprovisioning right after a tenant change, atomic run lock, strict photo path validation. - Certificates: key bundles refused, clean re-exported certificate. - Array-safe sanitising, encoded redirect_to, per-action nonces, escaped role lists, no Graph sleeps during sign-in, warnings for public groups, multi-tenant group rules and missing salts, uninstall clears the token. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
2026-09-23 17:10:30 +00:00 · 2026-09-23 17:10:30 +00:00 · 850f0dcd54
commit 850f0dcd54
parent 791f43a80b
18 changed files with 1908 additions and 1270 deletions
--- a/includes/class-m365-login-sync.php
+++ b/includes/class-m365-login-sync.php
@ -15,8 +15,10 @@ defined( 'ABSPATH' ) || exit;
 class M365_Login_Sync {

 	const CRON_HOOK      = 'm365_login_sync';
-	const LOCK           = 'm365_login_sync_lock';
-	const LOCK_TTL       = 30 * MINUTE_IN_SECONDS;
+	const LOCK           = 'm365_login_sync_lock'; // Option (atomic via add_option), value "token|time".
+	const LOCK_TTL       = 2 * HOUR_IN_SECONDS;
+	const TENANT_OPTION  = 'm365_login_sync_tenant'; // Tenant of the last successful live run.
+	const PHOTO_FILE     = '#^m365-login-avatars/m365-[a-f0-9]{16}-[a-f0-9]{8}\.(?:jpg|png|gif)$#';
 	const REPORT_OPTION  = 'm365_login_sync_report';
 	const META_SYNCED    = '_m365_login_synced'; // Account was created by the sync.
 	const META_DISABLED  = '_m365_login_disabled'; // Time, origin (sync or manual) and reason.
@ -55,6 +57,13 @@ class M365_Login_Sync {
 	 */
 	private $dry = false;

+	/**
+	 * Further accounts bound to the same object ID as the one in linked_users() (oid => user IDs).
+	 *
+	 * @var array
+	 */
+	private $duplicates = array();
+
 	/**
 	 * Constructor.
 	 *
@ -305,11 +314,11 @@ class M365_Login_Sync {
 			'log'      => array(),
 		);

-		if ( get_transient( self::LOCK ) ) {
+		$lock = $this->acquire_lock();
+		if ( '' === $lock ) {
 			$this->log( 'error', __( 'Another sync is still running. Please try again in a few minutes.', 'm365-login' ) );
 			return $this->finish( 'locked', false );
 		}
-		set_transient( self::LOCK, time(), self::LOCK_TTL );

 		if ( function_exists( 'set_time_limit' ) ) {
 			set_time_limit( 0 ); // phpcs:ignore Squiz.PHP.DiscouragedFunctions.Discouraged -- long-running directory sync.
@ -317,21 +326,51 @@ class M365_Login_Sync {
 		wp_raise_memory_limit( 'admin' );
 		require_once ABSPATH . 'wp-admin/includes/user.php';

-		// No "your e-mail/password changed" mails for changes made by the sync.
-		add_filter( 'send_email_change_email', '__return_false', 99 );
+		// No "your password changed" mails for the random passwords of deactivated accounts.
+		// E-mail change notifications stay on: the previous address is told about the change.
 		add_filter( 'send_password_change_email', '__return_false', 99 );

 		try {
 			$status = $this->sync();
 		} finally {
-			remove_filter( 'send_email_change_email', '__return_false', 99 );
 			remove_filter( 'send_password_change_email', '__return_false', 99 );
-			delete_transient( self::LOCK );
+			$this->release_lock( $lock );
 		}

 		return $this->finish( $status, true );
 	}

+	/**
+	 * Takes the run lock atomically (add_option fails when the row exists).
+	 *
+	 * @return string Lock token, or '' when another run holds the lock.
+	 */
+	private function acquire_lock() {
+		$token = wp_generate_password( 20, false );
+		if ( add_option( self::LOCK, $token . '|' . time(), '', 'no' ) ) {
+			return $token;
+		}
+		$held = explode( '|', (string) get_option( self::LOCK, '' ) );
+		if ( isset( $held[1] ) && time() - (int) $held[1] < self::LOCK_TTL ) {
+			return '';
+		}
+		// Stale lock of a crashed run.
+		delete_option( self::LOCK );
+		return add_option( self::LOCK, $token . '|' . time(), '', 'no' ) ? $token : '';
+	}
+
+	/**
+	 * Releases the run lock if this run still holds it.
+	 *
+	 * @param string $token Lock token.
+	 */
+	private function release_lock( $token ) {
+		$held = explode( '|', (string) get_option( self::LOCK, '' ) );
+		if ( $held[0] === $token ) {
+			delete_option( self::LOCK );
+		}
+	}
+
 	/**
 	 * Stores and returns the report.
 	 *
@ -392,7 +431,12 @@ class M365_Login_Sync {
 		}

 		// 2. Create, link and update accounts.
-		$linked   = $this->linked_users();
+		$linked = $this->linked_users();
+		if ( empty( $people ) && ! empty( $linked ) ) {
+			$this->log( 'error', __( 'Microsoft 365 returned no users at all while accounts are linked. Nothing was changed. Check the tenant and the sync groups.', 'm365-login' ) );
+			return 'aborted';
+		}
+
 		$seen     = array();
 		$pending  = array(); // Deactivations/deletions, applied after the safety check.
 		$photo_of = array(); // oid => WP_User whose photo is kept in sync.
@ -404,7 +448,7 @@ class M365_Login_Sync {

 			$result = $this->sync_person( $person, $linked, $memberships );
 			if ( is_array( $result ) ) {
-				$pending[] = $result;
+				$pending = array_merge( $pending, $result );
 			} elseif ( $result instanceof WP_User ) {
 				$photo_of[ $oid ] = $result;
 			}
@ -428,13 +472,22 @@ class M365_Login_Sync {
 				return 'failed';
 			}
 			if ( null !== $action ) {
-				$pending[] = $action;
+				$pending = array_merge( $pending, $action );
 			}
 		}

 		// 4. Safety net: never deactivate or delete a large part of the linked accounts in one go.
 		$pending = array_values( array_filter( $pending, array( $this, 'is_effective_action' ) ) );
-		$limit   = (int) apply_filters( 'm365_login_sync_deprovision_limit', max( 5, (int) ceil( count( $linked ) * 0.2 ) ), count( $linked ) );
+		$tenant  = strtolower( $this->settings->tenant() );
+		$before  = (string) get_option( self::TENANT_OPTION, '' );
+		if ( $pending && '' !== $before && $before !== $tenant ) {
+			$this->log( 'warning', __( 'The tenant ID changed since the last sync. Accounts linked in the old tenant are not found in the new one, so no account was deactivated or deleted in this run. Run the sync again to apply deactivations.', 'm365-login' ) );
+			$pending = array();
+		}
+		if ( ! $this->dry ) {
+			update_option( self::TENANT_OPTION, $tenant, false );
+		}
+		$limit = (int) apply_filters( 'm365_login_sync_deprovision_limit', max( 5, (int) ceil( count( $linked ) * 0.2 ) ), count( $linked ) );
 		if ( count( $pending ) > $limit ) {
 			$this->log(
 				'error',
@ -519,7 +572,7 @@ class M365_Login_Sync {
 	 * @return array oid => user ID.
 	 */
 	private function linked_users() {
-		$users = get_users(
+		$users            = get_users(
 			array(
 				'meta_key'     => M365_Login_Auth::META_OID, // phpcs:ignore WordPress.DB.SlowDBQuery.slow_db_query_meta_key
 				'meta_compare' => 'EXISTS',
@ -527,10 +580,16 @@ class M365_Login_Sync {
 				'number'       => -1,
 			)
 		);
-		$out   = array();
+		$out              = array();
+		$this->duplicates = array();
 		foreach ( $users as $row ) {
 			$oid = strtolower( (string) get_user_meta( (int) $row->ID, M365_Login_Auth::META_OID, true ) );
-			if ( M365_Login_Settings::is_guid( $oid ) ) {
+			if ( ! M365_Login_Settings::is_guid( $oid ) ) {
+				continue;
+			}
+			if ( isset( $out[ $oid ] ) ) {
+				$this->duplicates[ $oid ][] = (int) $row->ID;
+			} else {
 				$out[ $oid ] = (int) $row->ID;
 			}
 		}
@ -556,6 +615,12 @@ class M365_Login_Sync {

 		$user = isset( $linked[ $oid ] ) ? get_userdata( $linked[ $oid ] ) : false;

+		// A linked person disabled in Microsoft 365 is handled before anything else, so a
+		// removed or changed e-mail address during offboarding cannot keep the account alive.
+		if ( $user && ! $enabled ) {
+			return $this->action( (string) $this->settings->get( 'sync_disabled_action' ), $user, 'disabled', __( 'disabled in Microsoft 365', 'm365-login' ), $oid );
+		}
+
 		if ( ! $user && isset( $person['userType'] ) && 'Guest' === $person['userType'] && ! $this->settings->get( 'sync_guests' ) ) {
 			return null; // Guests are not imported (they may still be linked through a sign-in).
 		}
@ -586,6 +651,11 @@ class M365_Login_Sync {
 					$this->skip( sprintf( __( '%s: the WordPress account with this e-mail address is linked to a different Microsoft account, skipped.', 'm365-login' ), $email ) );
 					return null;
 				}
+				if ( ! $this->may_link( $by_mail, $person, $email ) ) {
+					/* translators: %s: e-mail address */
+					$this->skip( sprintf( __( '%s: privileged WordPress account – it is only linked when the Microsoft user principal name equals its e-mail address (member account, no guest). Skipped.', 'm365-login' ), $email ) );
+					return null;
+				}
 				$user = $by_mail;
 				$this->log( 'info', sprintf( /* translators: %s: e-mail address */ __( '%s: existing account linked.', 'm365-login' ), $email ) );
 				$this->count( 'linked' );
@ -601,7 +671,7 @@ class M365_Login_Sync {
 			if ( ! $user ) {
 				return null; // Nothing to create for disabled people.
 			}
-			return $this->action( (string) $this->settings->get( 'sync_disabled_action' ), $user, 'disabled', __( 'disabled in Microsoft 365', 'm365-login' ) );
+			return $this->action( (string) $this->settings->get( 'sync_disabled_action' ), $user, 'disabled', __( 'disabled in Microsoft 365', 'm365-login' ), $oid );
 		}

 		if ( ! $user ) {
@ -626,7 +696,7 @@ class M365_Login_Sync {
 		}

 		$changes = $this->update_profile( $user, $person, $email );
-		if ( $this->manages_roles( $user ) ) {
+		if ( $this->manages_roles( $user ) && ! self::disabled_info( $user->ID ) ) {
 			$changes = array_merge( $changes, $this->update_roles( $user, $this->desired_roles( $oid, $memberships ) ) );
 		}

@ -741,7 +811,10 @@ class M365_Login_Sync {

 		if ( strtolower( $user->user_email ) !== $email ) {
 			$owner = get_user_by( 'email', $email );
-			if ( $owner && $owner->ID !== $user->ID ) {
+			if ( self::is_privileged( $user ) ) {
+				/* translators: 1: current e-mail address, 2: e-mail address in Microsoft 365 */
+				$this->log( 'warning', sprintf( __( '%1$s: the e-mail address in Microsoft 365 changed to %2$s. It is not changed automatically for privileged accounts – update it by hand if intended.', 'm365-login' ), $user->user_email, $email ) );
+			} elseif ( $owner && $owner->ID !== $user->ID ) {
 				/* translators: %s: e-mail address */
 				$this->log( 'warning', sprintf( __( '%s: e-mail address is used by another WordPress account and was not changed.', 'm365-login' ), $email ) );
 			} else {
@ -935,7 +1008,7 @@ class M365_Login_Sync {
 	 *
 	 * @param string $oid     Object ID.
 	 * @param int    $user_id User ID.
-	 * @return array|null|WP_Error Pending action, null for none.
+	 * @return array[]|null|WP_Error Pending actions, null for none.
 	 */
 	private function classify_missing( $oid, $user_id ) {
 		$user = get_userdata( $user_id );
@ -947,15 +1020,15 @@ class M365_Login_Sync {
 		$person = $this->graph->get_user( $oid, array( 'id', 'accountEnabled', 'userType' ) );
 		if ( is_wp_error( $person ) ) {
 			if ( M365_Login_Graph::is_not_found( $person ) ) {
-				return $this->action( (string) $this->settings->get( 'sync_deleted_action' ), $user, 'deleted', __( 'deleted in Microsoft 365', 'm365-login' ) );
+				return $this->action( (string) $this->settings->get( 'sync_deleted_action' ), $user, 'deleted', __( 'deleted in Microsoft 365', 'm365-login' ), $oid );
 			}
 			return $person;
 		}
 		if ( isset( $person['accountEnabled'] ) && false === $person['accountEnabled'] ) {
-			return $this->action( (string) $this->settings->get( 'sync_disabled_action' ), $user, 'disabled', __( 'disabled in Microsoft 365', 'm365-login' ) );
+			return $this->action( (string) $this->settings->get( 'sync_disabled_action' ), $user, 'disabled', __( 'disabled in Microsoft 365', 'm365-login' ), $oid );
 		}
 		if ( $this->settings->sync_scope_groups() && ( ! isset( $person['userType'] ) || 'Guest' !== $person['userType'] || $this->settings->get( 'sync_guests' ) ) ) {
-			return $this->action( (string) $this->settings->get( 'sync_scope_action' ), $user, 'scope', __( 'no longer a member of the sync groups', 'm365-login' ) );
+			return $this->action( (string) $this->settings->get( 'sync_scope_action' ), $user, 'scope', __( 'no longer a member of the sync groups', 'm365-login' ), $oid );
 		}
 		return null;
 	}
@ -967,23 +1040,79 @@ class M365_Login_Sync {
 	 * @param WP_User $user   User.
 	 * @param string  $reason Machine reason.
 	 * @param string  $label  Human reason.
-	 * @return array|null
+	 * @param string  $oid    Object ID (further accounts bound to it get the same action).
+	 * @return array[]|null Pending actions.
 	 */
-	private function action( $what, $user, $reason, $label ) {
+	private function action( $what, $user, $reason, $label, $oid = '' ) {
 		if ( ! in_array( $what, array( 'disable', 'delete' ), true ) ) {
 			return null;
 		}
-		if ( $this->is_protected( $user ) ) {
-			/* translators: 1: e-mail address, 2: reason */
-			$this->skip( sprintf( __( '%1$s: %2$s, but the account is protected (administrator or your own account) and was not changed.', 'm365-login' ), $user->user_email, $label ) );
-			return null;
+		$users = array( $user );
+		if ( '' !== $oid && ! empty( $this->duplicates[ $oid ] ) ) {
+			foreach ( $this->duplicates[ $oid ] as $user_id ) {
+				$other = get_userdata( $user_id );
+				if ( $other && $other->ID !== $user->ID ) {
+					$users[] = $other;
+				}
+			}
 		}
-		return array(
-			'what'   => $what,
-			'user'   => $user,
-			'reason' => $reason,
-			'label'  => $label,
-		);
+		$out = array();
+		foreach ( $users as $target ) {
+			if ( $this->is_protected( $target ) ) {
+				/* translators: 1: e-mail address, 2: reason */
+				$this->skip( sprintf( __( '%1$s: %2$s, but the account is protected (administrator or your own account) and was not changed.', 'm365-login' ), $target->user_email, $label ) );
+				continue;
+			}
+			$out[] = array(
+				'what'   => $what,
+				'user'   => $target,
+				'reason' => $reason,
+				'label'  => $label,
+			);
+		}
+		return $out ? $out : null;
+	}
+
+	/**
+	 * Whether an existing account may be linked to a directory user by e-mail address.
+	 *
+	 * Privileged accounts are only linked through the user principal name of a member
+	 * account: its domain must be verified in the tenant, whereas the "mail" attribute can
+	 * be set to any address by a user or Exchange administrator of the tenant.
+	 *
+	 * @param WP_User $user   Existing account.
+	 * @param array   $person Graph user.
+	 * @param string  $email  Address the account was found by.
+	 * @return bool
+	 */
+	private function may_link( $user, $person, $email ) {
+		if ( ! self::is_privileged( $user ) ) {
+			return true;
+		}
+		$upn   = isset( $person['userPrincipalName'] ) ? strtolower( (string) $person['userPrincipalName'] ) : '';
+		$guest = isset( $person['userType'] ) && 'Guest' === $person['userType'];
+		return ! $guest && '' !== $upn && false === strpos( $upn, '#ext#' ) && strtolower( $user->user_email ) === $upn && $upn === $email;
+	}
+
+	/**
+	 * Accounts with administrative capabilities (they get extra protection against linking by e-mail).
+	 *
+	 * @param WP_User $user User.
+	 * @return bool
+	 */
+	public static function is_privileged( $user ) {
+		$privileged = is_super_admin( $user->ID )
+			|| user_can( $user, 'manage_options' )
+			|| user_can( $user, 'promote_users' )
+			|| user_can( $user, 'edit_users' );
+
+		/**
+		 * Filters whether an account counts as privileged (linked only via a matching user principal name).
+		 *
+		 * @param bool    $privileged Whether the account is privileged.
+		 * @param WP_User $user       User.
+		 */
+		return (bool) apply_filters( 'm365_login_is_privileged_user', $privileged, $user );
 	}

 	/**
@ -1215,7 +1344,7 @@ class M365_Login_Sync {
 				continue;
 			}

-			if ( ! empty( $stored['file'] ) && isset( $stored['etag'] ) && $stored['etag'] === $version && file_exists( self::photo_path( $stored['file'] ) ) ) {
+			if ( ! empty( $stored['file'] ) && isset( $stored['etag'] ) && $stored['etag'] === $version && self::is_photo_file( $stored['file'] ) && file_exists( self::photo_path( $stored['file'] ) ) ) {
 				if ( ! $this->dry ) {
 					$stored['checked'] = time();
 					update_user_meta( $user->ID, self::META_PHOTO, $stored );
@ -1256,7 +1385,7 @@ class M365_Login_Sync {
 				continue;
 			}

-			if ( ! empty( $stored['file'] ) && $stored['file'] !== $file ) {
+			if ( ! empty( $stored['file'] ) && $stored['file'] !== $file && self::is_photo_file( $stored['file'] ) ) {
 				wp_delete_file( self::photo_path( $stored['file'] ) );
 			}
 			update_user_meta(
@ -1388,6 +1517,16 @@ class M365_Login_Sync {
 		return trailingslashit( $uploads['basedir'] ) . ltrim( $file, '/' );
 	}

+	/**
+	 * Whether a stored photo path is one the plugin wrote (no traversal, fixed folder and pattern).
+	 *
+	 * @param mixed $file Relative path from user meta.
+	 * @return bool
+	 */
+	private static function is_photo_file( $file ) {
+		return is_string( $file ) && (bool) preg_match( self::PHOTO_FILE, $file );
+	}
+
 	/**
 	 * Deletes a user's stored photo (also hooked to user deletion).
 	 *
@ -1395,7 +1534,7 @@ class M365_Login_Sync {
 	 */
 	public function delete_photo( $user_id ) {
 		$stored = get_user_meta( $user_id, self::META_PHOTO, true );
-		if ( is_array( $stored ) && ! empty( $stored['file'] ) && 0 === strpos( $stored['file'], self::PHOTO_DIR . '/' ) ) {
+		if ( is_array( $stored ) && ! empty( $stored['file'] ) && self::is_photo_file( $stored['file'] ) ) {
 			wp_delete_file( self::photo_path( $stored['file'] ) );
 		}
 		delete_user_meta( $user_id, self::META_PHOTO );
@ -1431,7 +1570,7 @@ class M365_Login_Sync {
 		}

 		$stored = get_user_meta( $user_id, self::META_PHOTO, true );
-		if ( ! is_array( $stored ) || empty( $stored['file'] ) ) {
+		if ( ! is_array( $stored ) || empty( $stored['file'] ) || ! self::is_photo_file( $stored['file'] ) ) {
 			return $args;
 		}
 		$uploads              = wp_get_upload_dir();
@ -1472,6 +1611,9 @@ class M365_Login_Sync {
 	 * @param string $reason  Machine reason.
 	 */
 	public static function disable( $user_id, $by, $reason = '' ) {
+		$user  = get_userdata( $user_id );
+		$roles = $user ? array_values( $user->roles ) : array();
+
 		update_user_meta(
 			$user_id,
 			self::META_DISABLED,
@ -1479,9 +1621,20 @@ class M365_Login_Sync {
 				'time'   => time(),
 				'by'     => $by,
 				'reason' => $reason,
+				'roles'  => $roles,
 			)
 		);
+
+		// Lock the account for good, also without this plugin: no sessions, no role on this
+		// site, a random password nobody knows and no application passwords.
 		WP_Session_Tokens::get_instance( $user_id )->destroy_all();
+		if ( class_exists( 'WP_Application_Passwords' ) ) {
+			WP_Application_Passwords::delete_all_application_passwords( $user_id );
+		}
+		wp_set_password( wp_generate_password( 64, true, true ), $user_id );
+		if ( $user ) {
+			$user->set_role( '' );
+		}

 		/**
 		 * Fires after an account was deactivated.
@ -1499,8 +1652,19 @@ class M365_Login_Sync {
 	 * @param int $user_id User ID.
 	 */
 	public static function enable( $user_id ) {
+		$info = self::disabled_info( $user_id );
 		delete_user_meta( $user_id, self::META_DISABLED );

+		// Give back the roles taken away on deactivation (the sync may adjust them afterwards).
+		$user = get_userdata( $user_id );
+		if ( $user && empty( $user->roles ) && $info && ! empty( $info['roles'] ) && is_array( $info['roles'] ) ) {
+			foreach ( $info['roles'] as $role ) {
+				if ( is_string( $role ) && get_role( $role ) ) {
+					$user->add_role( $role );
+				}
+			}
+		}
+
 		/**
 		 * Fires after an account was reactivated.
 		 *