From 5365f7d3c824eda7d343e66ddf4ae2008783deed Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 28 Oct 2026 15:58:28 +0000 Subject: [PATCH 1/4] 1.9.79: a new password ends the user's other sessions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #325 revoked the remember-me keys on a new password and left every browser already signed in signed in, so a thief who had used a stolen cookie kept the session it made after the owner changed or reset the password. Measured live on #326's head: after a change on Personal settings, a reset, PUT /users/{id} and Account › Users, the other browsers stayed signed in. 202_users gains user_session_generation (INT UNSIGNED NOT NULL DEFAULT 0), in the table definition and the 1.9.78 -> 1.9.79 rung. A counter, not a time: no same-second race, no clock to skew between PHP and the database. - The sign-in (LOGIN_SELECT, and the cookie's SELECT *) stores the user's generation in the session (begin_user_session()). - SessionGeneration::bump() counts it up, in the writer's transaction, wherever the user's remember-me keys are revoked: a new password (PasswordChange::store(), Account › Users, PUT /users/{id}, MysqlUserRepository::update()), a switch-off (a session idle through the switch-off would otherwise go on once the user is switched on again: measured), and the removal's soft delete. A failed count rolls the change back. The rehash at sign-in does not count. - connect.php's User read (UserStanding::READ) reads the generation in the same statement; AUTH ends a session holding another at that request, with the sign-out's teardown but without revoking keys again (the change revoked them; keys made since are a later sign-in's). A failed read is the error page. Before the upgrade has run the column is missing: the read retries without it (that request only) and compares nothing. - A session from before 1.9.79 holds no generation and is read as 0, every user's value until a change, so the upgrade signs nobody out. - Personal settings keeps the changing browser's session: it takes the new generation after the commit (keepSessionAfterPasswordChange()). An admin's change (Account › Users, PUT) ends the user's sessions, not the admin's. RememberMeKeysRevokedTest holds every password change and every standing writer to ending the sessions, the rehash to not, and every write of the generation to counting it up by one; SessionSignInPathsTest holds the session's generation to the sign-in and keepSessionAfterPasswordChange(). SessionGenerationIntegrationTest runs two browsers through each writer, the rehash, a session from before the upgrade, a failed read, a failed count and the window before the upgrade; SessionGenerationUpgrade- IntegrationTest runs the rung. tests/live/password-ends-sessions.sh drives it over HTTP. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01VRoS2SViuxaucFaKWsFnLr --- 202-account/account.php | 16 +- 202-config/Database/Tables/UserTables.php | 7 + 202-config/User.class.php | 16 +- 202-config/User/MysqlUserRepository.php | 4 +- 202-config/User/PasswordChange.php | 20 +- 202-config/User/SessionGeneration.php | 74 +++ 202-config/User/UserDataPurge.php | 9 +- 202-config/User/UserStanding.php | 59 ++- 202-config/functions-account-ui.php | 8 +- 202-config/functions-auth.php | 62 ++- 202-config/functions-upgrade.php | 6 + api/v3/Controllers/UsersController.php | 8 +- .../Auth/SessionGenerationIntegrationTest.php | 425 ++++++++++++++++++ tests/Auth/SessionSignInPathsTest.php | 34 +- tests/Auth/fixtures/session-user-runner.php | 22 +- ...essionGenerationUpgradeIntegrationTest.php | 152 +++++++ tests/User/RememberMeKeysRevokedTest.php | 166 ++++++- tests/User/UserManagementSaveTest.php | 32 +- tests/live/password-ends-sessions.sh | 209 +++++++++ 19 files changed, 1272 insertions(+), 57 deletions(-) create mode 100644 202-config/User/SessionGeneration.php create mode 100644 tests/Auth/SessionGenerationIntegrationTest.php create mode 100644 tests/Upgrade/SessionGenerationUpgradeIntegrationTest.php create mode 100755 tests/live/password-ends-sessions.sh diff --git a/202-account/account.php b/202-account/account.php index c9a520dfe..85ada1a8e 100755 --- a/202-account/account.php +++ b/202-account/account.php @@ -723,11 +723,12 @@ $new_hash = hash_user_pass($newPass); $passSaved = false; $passError = ''; + $generation = null; try { - // The password and every remember-me key of the user, this - // browser's too (a stolen cookie is a copy of one), in one - // transaction; the session goes on. - \Prosper202\User\PasswordChange::store( + // The password, every remember-me key of the user (this + // browser's too: a stolen cookie is a copy of one) and every + // session of theirs, in one transaction. + $generation = \Prosper202\User\PasswordChange::store( new \Prosper202\Database\Connection($db), (int) ($_SESSION['user_own_id'] ?? 0), $new_hash, @@ -741,9 +742,12 @@ prosper_log('account', 'Failed to update password: ' . $passError); $pageFlashes[] = ['kind' => 'bad', 'text' => 'Your password could not be changed. Your old password still works; try again.']; } else { - // Its key is gone with the rest: the cookie would only be refused. + // This session goes on, at the generation the change made; every + // other one ends at its next request. Its key is gone with the + // rest: the cookie would only be refused. + AUTH::keepSessionAfterPasswordChange((int) $generation); AUTH::forgetRememberMeCookie(); - p202_account_flash('ok', 'Your password is changed. Browsers kept signed in now need it to sign in again; this one stays signed in.'); + p202_account_flash('ok', 'Your password is changed. Every other browser signed in as you is signed out, and needs it to sign in again; this one stays signed in.'); p202_account_redirect('202-account/account.php#password'); } } diff --git a/202-config/Database/Tables/UserTables.php b/202-config/Database/Tables/UserTables.php index d69bbd843..eeaeb1dd7 100644 --- a/202-config/Database/Tables/UserTables.php +++ b/202-config/Database/Tables/UserTables.php @@ -32,6 +32,12 @@ public static function getDefinitions(): array ]; } + /** + * user_session_generation counts the changes that end every session of + * the user (a new password, a switch-off, a removal): a session holds the + * value it began with, and connect.php ends one that holds another + * (Prosper202\User\SessionGeneration). From 1.9.79, added by its rung. + */ public static function users(): SchemaDefinition { return SchemaBuilder::fromRawSql( @@ -62,6 +68,7 @@ public static function users(): SchemaDefinition `secret_key` char(48) DEFAULT NULL, `user_mods_lb` tinyint(1) unsigned NOT NULL DEFAULT '0', `p202_customer_api_key` char(60) DEFAULT NULL, + `user_session_generation` int(10) unsigned NOT NULL DEFAULT '0', PRIMARY KEY (`user_id`), UNIQUE KEY `user_name_2` (`user_name`), KEY `user_name` (`user_name`,`user_pass`), diff --git a/202-config/User.class.php b/202-config/User.class.php index 7cfef799e..13403d7c8 100755 --- a/202-config/User.class.php +++ b/202-config/User.class.php @@ -11,6 +11,9 @@ class User /** Whether that user may act, read with the row; null when no row was read. */ private ?\Prosper202\User\UserStanding $standing = null; + /** The user's session generation, read with the row; null when there is none to compare. */ + private ?int $generation = null; + public function __construct($user_id) { try { @@ -47,7 +50,7 @@ public function __construct($user_id) // installed, so no account (index.php and the setup pages load // connect.php before the tables exist). try { - $standing = \Prosper202\User\UserStanding::read(self::$db, $id); + [$standing, $generation] = \Prosper202\User\UserStanding::readWithGeneration(self::$db, $id); } catch (\Prosper202\Database\Exceptions\QueryException $e) { if ($e->getCode() !== 1146) { throw $e; @@ -56,6 +59,7 @@ public function __construct($user_id) } $this->userId = $id; $this->standing = $standing; + $this->generation = $generation; // A user who may not act holds no permission, whatever their roles. if ($standing->canAct()) { $this->loadRoles($id); @@ -73,6 +77,16 @@ public function standingOf(int $userId): ?\Prosper202\User\UserStanding return $this->userId === $userId ? $this->standing : null; } + /** + * The user's session generation, as read when this object was built, for + * that user only; null when none was read (see standingOf()), or the + * database has no such column yet (before the 1.9.79 upgrade). + */ + public function generationOf(int $userId): ?int + { + return $this->userId === $userId ? $this->generation : null; + } + protected function loadRoles($user_id) { // Ensure $db is a valid mysqli object diff --git a/202-config/User/MysqlUserRepository.php b/202-config/User/MysqlUserRepository.php index 5822f55cc..635966b72 100644 --- a/202-config/User/MysqlUserRepository.php +++ b/202-config/User/MysqlUserRepository.php @@ -110,7 +110,8 @@ public function update(int $id, array $data): void || (array_key_exists('user_active', $data) && !UserStanding::isActive($data['user_active'])); // Switched off, or given a new password, the user's remember-me keys - // go in the same transaction (RememberMeKeys). + // and open sessions end in the same transaction (RememberMeKeys, + // SessionGeneration). $this->conn->transaction(function () use ($sets, $types, $values, $id, $revokes): void { $stmt = $this->conn->prepareWrite( 'UPDATE 202_users SET ' . implode(', ', $sets) . ' WHERE user_id = ?' @@ -120,6 +121,7 @@ public function update(int $id, array $data): void $stmt->close(); if ($revokes) { RememberMeKeys::revokeAll($this->conn, $id); + SessionGeneration::bump($this->conn, $id); } }); } diff --git a/202-config/User/PasswordChange.php b/202-config/User/PasswordChange.php index 3cde8a187..2c862d0e2 100644 --- a/202-config/User/PasswordChange.php +++ b/202-config/User/PasswordChange.php @@ -9,8 +9,9 @@ /** * A new password for a user, from Personal settings (the user's own) or the - * reset link (202-pass-reset.php): the password and the end of every - * remember-me key of the user, in one transaction. + * reset link (202-pass-reset.php): the password, the end of every remember-me + * key of the user and of every session they have open (SessionGeneration), + * in one transaction. * * The password changed and the keys stayed, so a stolen remember-me cookie * went on signing in after its owner changed the password to lock the thief @@ -25,19 +26,22 @@ private function __construct() } /** - * Store $hash as the user's password and delete every remember-me key of - * theirs; with $consumesResetKey, also clear the reset key that allowed it. - * The caller holds no transaction; nothing is kept unless all of it is. + * Store $hash as the user's password, delete every remember-me key of + * theirs and end every session they have open; with $consumesResetKey, + * also clear the reset key that allowed it. The caller holds no + * transaction; nothing is kept unless all of it is. Answers the user's new + * session generation, which the browser that made the change takes to keep + * its own session (AUTH::keepSessionAfterPasswordChange()). * * @throws RuntimeException when nothing was stored (a QueryException for a * failed statement, RuntimeException for a user who is not there) */ - public static function store(Connection $conn, int $userId, string $hash, bool $consumesResetKey): void + public static function store(Connection $conn, int $userId, string $hash, bool $consumesResetKey): int { $sql = $consumesResetKey ? 'UPDATE 202_users SET user_pass = ?, user_pass_key = NULL, user_pass_time = 0 WHERE user_id = ?' : 'UPDATE 202_users SET user_pass = ? WHERE user_id = ?'; - $conn->transaction(static function () use ($conn, $sql, $userId, $hash): void { + return $conn->transaction(static function () use ($conn, $sql, $userId, $hash): int { $stmt = $conn->prepareWrite($sql); $conn->bind($stmt, 'si', [$hash, $userId]); if ($conn->executeUpdate($stmt) !== 1) { @@ -46,6 +50,8 @@ public static function store(Connection $conn, int $userId, string $hash, bool $ throw new RuntimeException('Storing the password of user ' . $userId . ' changed no row'); } RememberMeKeys::revokeAll($conn, $userId); + + return SessionGeneration::bump($conn, $userId); }); } } diff --git a/202-config/User/SessionGeneration.php b/202-config/User/SessionGeneration.php new file mode 100644 index 000000000..9711b5bc4 --- /dev/null +++ b/202-config/User/SessionGeneration.php @@ -0,0 +1,74 @@ +prepareWrite(self::BUMP); + $conn->bind($stmt, 'i', [$userId]); + if ($conn->executeUpdate($stmt) !== 1) { + throw new RuntimeException('Ending the sessions of user ' . $userId . ' changed no row'); + } + $stmt = $conn->prepareWrite(self::READ); + $conn->bind($stmt, 'i', [$userId]); + $row = $conn->fetchOne($stmt); + } catch (QueryException $e) { + throw new QueryException( + 'Ending the sessions of user ' . $userId . ' failed: ' . $e->getMessage(), + (int) $e->getCode(), + $e + ); + } catch (\mysqli_sql_exception $e) { + throw new QueryException( + 'Ending the sessions of user ' . $userId . ' failed: ' . $e->getMessage() + . ' [errno ' . (int) $e->getCode() . ']', + (int) $e->getCode(), + $e + ); + } + $generation = $row['user_session_generation'] ?? null; + if (!is_numeric($generation)) { + throw new RuntimeException('Ending the sessions of user ' . $userId . ': the new generation was not read'); + } + + return (int) $generation; + } +} diff --git a/202-config/User/UserDataPurge.php b/202-config/User/UserDataPurge.php index 5f176a568..64a2fe9ba 100644 --- a/202-config/User/UserDataPurge.php +++ b/202-config/User/UserDataPurge.php @@ -153,7 +153,6 @@ final class UserDataPurge /** Taken first in the delete's transaction: see deleteUser(). */ public const LOCK_USER = 'SELECT user_id FROM 202_users WHERE user_id = ? FOR UPDATE'; - /** What lets the user act at all through the API; revoked with the rest. */ private const ACCESS_STATEMENTS = [ 'DELETE FROM 202_api_keys WHERE user_id = ?', @@ -254,7 +253,13 @@ public function deleteUser(int $userId): void $this->run($sql, $userId); } $unlinked = (new AppDataPurge($this->db))->purgeUser($userId); - $this->run('UPDATE 202_users SET user_deleted = 1 WHERE user_id = ?', $userId); + // Every open session of the user ends with the delete + // (SessionGeneration), so a row restored by hand stays signed out. + $this->run( + 'UPDATE 202_users SET user_deleted = 1,' + . ' user_session_generation = user_session_generation + 1 WHERE user_id = ?', + $userId + ); return [$exportFileNames, $unlinked]; }); diff --git a/202-config/User/UserStanding.php b/202-config/User/UserStanding.php index 2a730007a..f99765edd 100644 --- a/202-config/User/UserStanding.php +++ b/202-config/User/UserStanding.php @@ -26,8 +26,20 @@ enum UserStanding case Deleted; case Missing; - /** The row of the user, read from the primary: a replica's copy can predate the switch-off. */ - public const string READ = 'SELECT user_active, user_deleted FROM 202_users WHERE user_id = ? LIMIT 1'; + /** + * The row of the user, read from the primary: a replica's copy can predate + * the switch-off. With the session generation (SessionGeneration), in the + * same read. + */ + public const string READ = 'SELECT user_active, user_deleted, user_session_generation' + . ' FROM 202_users WHERE user_id = ? LIMIT 1'; + + /** + * The same, on a database whose upgrade has not run yet: the files are + * deployed first, and 202_users has no user_session_generation until the + * 1.9.79 rung adds it. + */ + public const string READ_BEFORE_UPGRADE = 'SELECT user_active, user_deleted FROM 202_users WHERE user_id = ? LIMIT 1'; /** @param array|null $row the user's row (user_active, user_deleted), null for none */ public static function ofRow(?array $row): self @@ -59,16 +71,51 @@ private static function is(mixed $value, int $want): bool * @throws QueryException with the server's errno as its code where it has one */ public static function read(\mysqli $db, int $userId): self + { + return self::readWithGeneration($db, $userId)[0]; + } + + /** + * The user's standing and session generation, in one read. The generation + * is null when there is none to compare: no row, or a database the 1.9.79 + * upgrade has not reached yet (then one more read, without the column, + * on that request only). A read that fails otherwise throws. + * + * @return array{self, ?int} + * @throws QueryException with the server's errno as its code where it has one + */ + public static function readWithGeneration(\mysqli $db, int $userId): array { if ($userId <= 0) { - return self::Missing; + return [self::Missing, null]; + } + try { + $row = self::fetch($db, self::READ, $userId); + } catch (QueryException $e) { + // 1054: unknown column. Only this one is read without it. + if ($e->getCode() !== 1054 || !str_contains($e->getMessage(), 'user_session_generation')) { + throw $e; + } + return [self::ofRow(self::fetch($db, self::READ_BEFORE_UPGRADE, $userId)), null]; } + $generation = $row['user_session_generation'] ?? null; + + return [self::ofRow($row), is_numeric($generation) ? (int) $generation : null]; + } + + /** + * @return array|null + * @throws QueryException + */ + private static function fetch(\mysqli $db, string $sql, int $userId): ?array + { $failed = 'Reading whether user ' . $userId . ' may act failed'; try { $conn = new Connection($db); - $stmt = $conn->prepareWrite(self::READ); + $stmt = $conn->prepareWrite($sql); $conn->bind($stmt, 'i', [$userId]); - $row = $conn->fetchOne($stmt); + + return $conn->fetchOne($stmt); } catch (QueryException $e) { throw new QueryException($failed . ': ' . $e->getMessage(), self::errno($e), $e); } catch (\mysqli_sql_exception $e) { @@ -79,8 +126,6 @@ public static function read(\mysqli $db, int $userId): self $e ); } - - return self::ofRow($row); } /** Whether a value written to user_active leaves the user active. */ diff --git a/202-config/functions-account-ui.php b/202-config/functions-account-ui.php index 751617276..6dd43d60c 100644 --- a/202-config/functions-account-ui.php +++ b/202-config/functions-account-ui.php @@ -343,9 +343,10 @@ function p202_account_save_currency(\Prosper202\Database\Connection $conn, int $ * without them; it must not open a transaction of its own. * * An edit that switches the user off, or sets their password, deletes their - * remember-me keys in the same transaction (RememberMeKeys): a browser kept - * signed in must not sign back in when they are switched on again, nor a - * stolen cookie outlive the password an admin reset. + * remember-me keys and ends their open sessions in the same transaction + * (RememberMeKeys, SessionGeneration): a browser kept signed in must not sign + * back in when they are switched on again, nor a stolen cookie or session + * outlive the password an admin reset. * * @param array $userSet 202_users column => value; the * column names are the caller's own literals, never the request's @@ -385,6 +386,7 @@ function p202_account_save_user(\Prosper202\Database\Connection $conn, ?int $edi $userId = $editUserId; if ($revokes) { \Prosper202\User\RememberMeKeys::revokeAll($conn, $userId); + \Prosper202\User\SessionGeneration::bump($conn, $userId); } $stmt = $conn->prepareWrite('DELETE FROM `202_user_role` WHERE `user_id` = ?'); $conn->bind($stmt, 'i', [$userId]); diff --git a/202-config/functions-auth.php b/202-config/functions-auth.php index e1fde6f8b..41c6b28c7 100755 --- a/202-config/functions-auth.php +++ b/202-config/functions-auth.php @@ -72,7 +72,7 @@ class AUTH // cookie) guessing the password toward a full account takeover. public const MAX_PASSWORD_REAUTH_FAILS = 5; - private const string LOGIN_SELECT = 'SELECT u.user_id, u.user_name, u.user_pass, u.user_api_key, u.user_stats202_app_key, u.user_timezone, u.user_mods_lb, u.install_hash, u.p202_customer_api_key, up.user_id AS pref_user_id, up.user_slack_incoming_webhook FROM 202_users u LEFT JOIN 202_users_pref up ON up.user_id = u.user_id WHERE u.user_name = ? AND u.user_deleted = 0 AND u.user_active = 1 LIMIT 1'; + private const string LOGIN_SELECT = 'SELECT u.user_id, u.user_name, u.user_pass, u.user_api_key, u.user_stats202_app_key, u.user_timezone, u.user_mods_lb, u.install_hash, u.p202_customer_api_key, u.user_session_generation, up.user_id AS pref_user_id, up.user_slack_incoming_webhook FROM 202_users u LEFT JOIN 202_users_pref up ON up.user_id = u.user_id WHERE u.user_name = ? AND u.user_deleted = 0 AND u.user_active = 1 LIMIT 1'; private static bool $passwordColumnChecked = false; private static bool $sessionHeartbeatRefreshed = false; @@ -146,7 +146,8 @@ public static function logged_in() /** * connect.php, once it has read the session's user (new User()): a - * session whose user may no longer act is ended there, before the page + * session whose user may no longer act, or which began before the user's + * password changed (sessionUserStands()), is ended there, before the page * reads anything from it, so no page shows or acts on it whether or not * it asks for a sign-in. Nothing is read here: where connect.php read no * row (the install, key and upgrade pages, which sign nobody in, or a @@ -165,12 +166,14 @@ public static function endSessionIfItsUserCannotAct(?\User $user): void } /** - * Whether the session's user may act (UserStanding), from the row $read + * Whether the session's user may act (UserStanding), and the session began + * at the user's current generation (SessionGeneration), from the row $read * (connect.php's User) holds for this request or, when it holds none for * this user, from the database now; never from an answer kept from an * earlier request, which would outlive the switch-off it is there to see. - * A session whose user may not act is ended. A read that fails throws: it - * is neither "may act" nor "signed out". + * A session whose user may not act is ended, and so is one from before a + * new password, a switch-off or a removal. A read that fails throws: it is + * neither "may act" nor "signed out". */ private static function sessionUserStands(?\User $read): bool { @@ -179,16 +182,58 @@ private static function sessionUserStands(?\User $read): bool self::endSession(null); return false; } - $standing = $read?->standingOf($userId) - ?? \Prosper202\User\UserStanding::read(DB::getInstance()->getConnection(), $userId); + $standing = $read?->standingOf($userId); + $generation = $read?->generationOf($userId); + if ($standing === null) { + [$standing, $generation] = \Prosper202\User\UserStanding::readWithGeneration( + DB::getInstance()->getConnection(), + $userId + ); + } if (!$standing->canAct()) { self::endSession($userId); return false; } + // The change that counted the generation up revoked the user's keys + // in its own transaction; keys made since are a later sign-in's. + if ($generation !== null && self::sessionGeneration() !== $generation) { + self::endSession(null); + return false; + } return true; } + /** + * The generation this session began at; 0 for a session from before + * 1.9.79, which holds none (every user is at 0 until a change), and -1, + * which no row holds, for a value no sign-in wrote. + */ + private static function sessionGeneration(): int + { + $held = $_SESSION['user_session_generation'] ?? 0; + + return is_int($held) && $held >= 0 ? $held : -1; + } + + /** + * This browser changed its own password (Personal settings): it keeps its + * session by taking the generation the change counted up to, which every + * other session of the user does not hold. After the commit, in the same + * request. + */ + public static function keepSessionAfterPasswordChange(int $generation): void + { + $writer = static function () use ($generation): void { + $_SESSION['user_session_generation'] = $generation; + }; + if (function_exists('withWritableSession')) { + withWritableSession($writer); + } else { + $writer(); + } + } + /** * The user the session is signed in as. begin_user_session() sets * user_id and user_own_id to the same id; a session where either is @@ -503,6 +548,9 @@ public static function begin_user_session(array $user_row): void $_SESSION['user_timezone'] = $user_row['user_timezone'] ?? 'UTC'; $_SESSION['user_mods_lb'] = $user_row['user_mods_lb'] ?? 0; $_SESSION['account_owner_id'] = $accountOwnerId; + // The generation the user is at; a later change counts it up, and + // connect.php ends a session still holding this one. + $_SESSION['user_session_generation'] = (int) ($user_row['user_session_generation'] ?? 0); }; if (function_exists('withWritableSession')) { diff --git a/202-config/functions-upgrade.php b/202-config/functions-upgrade.php index a9deea94d..6c0646f1c 100755 --- a/202-config/functions-upgrade.php +++ b/202-config/functions-upgrade.php @@ -4475,6 +4475,12 @@ private static function ladder($time_from) // (Prosper202\Click\ClickRecord). \Prosper202\Database\Tables\TrackingTables::google(), \Prosper202\Database\Tables\UserTables::usersPref(), + // 202_users gains user_session_generation, which a new + // password, a switch-off or a removal counts up so that + // connect.php ends the user's other sessions + // (Prosper202\User\SessionGeneration). 0 for everyone, which + // a session from before the upgrade, holding none, reads as. + \Prosper202\Database\Tables\UserTables::users(), ], [], ['202_users_pref' => ['user_pref_privacy']]); if ($step_ok) { diff --git a/api/v3/Controllers/UsersController.php b/api/v3/Controllers/UsersController.php index dceefb71a..0c454f20a 100644 --- a/api/v3/Controllers/UsersController.php +++ b/api/v3/Controllers/UsersController.php @@ -281,9 +281,10 @@ public function update(int $id, array $payload, ?int $actorUserId = null): array $types .= 'i'; // Switched off, or given a new password, the user's remember-me keys - // go in the same transaction: a browser kept signed in must not sign - // back in when the user is switched on again, nor a stolen cookie - // outlive the password (RememberMeKeys). + // and open sessions end in the same transaction: a browser kept + // signed in must not sign back in when the user is switched on again, + // nor a stolen cookie or session outlive the password (RememberMeKeys, + // SessionGeneration). $revokes = $switchesOff || $changesPassword; $this->transaction(function () use ($sets, $types, $binds, $id, $revokes): void { $stmt = $this->prepare('UPDATE 202_users SET ' . implode(', ', $sets) . ' WHERE user_id = ?'); @@ -292,6 +293,7 @@ public function update(int $id, array $payload, ?int $actorUserId = null): array $stmt->close(); if ($revokes) { \Prosper202\User\RememberMeKeys::revokeAll(new Connection($this->db), $id); + \Prosper202\User\SessionGeneration::bump(new Connection($this->db), $id); } }); diff --git a/tests/Auth/SessionGenerationIntegrationTest.php b/tests/Auth/SessionGenerationIntegrationTest.php new file mode 100644 index 000000000..39240a76c --- /dev/null +++ b/tests/Auth/SessionGenerationIntegrationTest.php @@ -0,0 +1,425 @@ +|null 202_version as this class found it, put back after */ + private static ?array $versions = null; + + public static function setUpBeforeClass(): void + { + $root = dirname(__DIR__, 2); + if (!is_file($root . '/202-config.php')) { + self::markTestSkipped('No 202-config.php: connect.php would exit before any page ran.' + . ' tests/run-integration-suites.sh writes one.'); + } + if (!self::connectScratch()) { + return; + } + require_once $root . '/202-config/version.php'; + require_once $root . '/202-config/functions-auth.php'; + $found = self::$db->query('SELECT version FROM 202_version'); + $rows = $found instanceof \mysqli_result ? $found->fetch_all(MYSQLI_ASSOC) : []; + self::$versions = array_column($rows, 'version'); + self::q('DELETE FROM 202_version'); + self::row('202_version', ['version' => PROSPER202_VERSION]); + (new DataSeeder(self::$db))->seed(); + self::$sessions = sys_get_temp_dir() . '/p202-session-generation-' . getmypid(); + if (!is_dir(self::$sessions) && !mkdir(self::$sessions, 0700)) { + throw new \RuntimeException('could not create ' . self::$sessions); + } + } + + public static function tearDownAfterClass(): void + { + if (self::$db !== null) { + self::cleanUp(); + if (self::$versions !== null) { + self::q('DELETE FROM 202_version'); + foreach (self::$versions as $version) { + self::row('202_version', ['version' => $version]); + } + } + } + if (self::$sessions !== '' && is_dir(self::$sessions)) { + array_map('unlink', glob(self::$sessions . '/sess_*') ?: []); + rmdir(self::$sessions); + } + } + + protected function setUp(): void + { + self::requireScratch(); + self::cleanUp(); + // USER an Admin, ADMIN a Super user, who may edit Admins. + foreach ([self::USER => 2, self::ADMIN => 1] as $user => $role) { + self::q("INSERT INTO 202_users SET user_id = $user, user_name = 'gen$user', user_fname = 'Gen'," + . " user_lname = 'User', user_email = 'gen$user@example.com', user_dash_email = ''," + . ' user_pass = ' . self::str(password_hash(self::PASSWORD, PASSWORD_DEFAULT)) . ',' + . " user_timezone = 'UTC', user_time_register = 0, install_hash = '', user_hash = ''," + . " p202_customer_api_key = 'session-generation'"); + self::row('202_users_pref', ['user_id' => $user]); + self::row('202_user_role', ['user_id' => $user, 'role_id' => $role]); + } + } + + private static function cleanUp(): void + { + foreach ([self::USER, self::ADMIN] as $user) { + foreach (['202_api_keys', '202_auth_keys', '202_user_role', '202_users_pref', '202_users'] as $table) { + self::q("DELETE FROM $table WHERE user_id = $user"); + } + } + } + + private static function generation(int $user): int + { + $result = self::$db->query("SELECT user_session_generation FROM 202_users WHERE user_id = $user"); + self::assertInstanceOf(\mysqli_result::class, $result); + + return (int) $result->fetch_row()[0]; + } + + /** + * One request; what it printed and what it left. + * + * @param array $env + * @return array{output: string, result: array} + */ + private static function request(string $target, array $env = []): array + { + $env += [ + 'P202_SU_USER' => (string) self::USER, + 'P202_SU_SESSION_DIR' => self::$sessions, + ]; + $runner = __DIR__ . '/fixtures/session-user-runner.php'; + $proc = proc_open( + [PHP_BINARY, '-d', 'display_errors=stderr', $runner, $target], + [1 => ['pipe', 'w'], 2 => ['pipe', 'w']], + $pipes, + null, + $env + getenv() + ); + self::assertIsResource($proc); + $stdout = (string) stream_get_contents($pipes[1]); + $stderr = (string) stream_get_contents($pipes[2]); + fclose($pipes[1]); + fclose($pipes[2]); + $exit = proc_close($proc); + self::assertNotSame(2, $exit, "the runner could not start the request:\n$stdout$stderr"); + $at = strrpos($stdout, "\n@@RESULT "); + self::assertNotFalse($at, "the request left no result line:\n$stdout$stderr"); + $result = json_decode(substr($stdout, $at + strlen("\n@@RESULT ")), true, 8, JSON_THROW_ON_ERROR); + + return ['output' => substr($stdout, 0, $at) . $stderr, 'result' => $result]; + } + + /** + * A browser signed in as $user, at the generation given (none: a session + * from before 1.9.79): its session id. + */ + private static function browser(int $user = self::USER, ?int $generation = 0): string + { + $env = ['P202_SU_USER' => (string) $user]; + if ($generation !== null) { + $env['P202_SU_GENERATION'] = (string) $generation; + } + $signedIn = self::request('call:booted', $env); + self::assertNull($signedIn['result']['uncaught'], $signedIn['output']); + self::assertContains('user_id', $signedIn['result']['answer']['identity'], 'the browser is signed in'); + + return $signedIn['result']['session_id']; + } + + /** + * The browser's next request: whether it is still signed in, and the + * session id it was left with. + * + * @return array{bool, string} + */ + private static function next(string $session, int $user = self::USER): array + { + $next = self::request('call:booted', ['P202_SU_SESSION_ID' => $session, 'P202_SU_USER' => (string) $user]); + self::assertNull($next['result']['uncaught'], $next['output']); + + return [in_array('user_id', $next['result']['answer']['identity'], true), $next['result']['session_id']]; + } + + /** What Personal settings' password form posts, with the session's token. */ + private static function passwordForm(): string + { + return json_encode([ + 'change_user_pass' => '1', + 'token' => 'SESSION', + 'user_pass' => self::PASSWORD, + 'new_user_pass' => self::NEW_PASSWORD, + 'retype_new_user_pass' => self::NEW_PASSWORD, + ], JSON_THROW_ON_ERROR); + } + + public function testAPasswordChangeEndsTheOtherSessionAndKeepsTheChangingOne(): void + { + $changing = self::browser(); + $other = self::browser(); + + $change = self::request('202-account/account.php', [ + 'P202_SU_SESSION_ID' => $changing, + 'P202_SU_POST' => self::passwordForm(), + ]); + $r = $change['result']; + self::assertNull($r['uncaught'], $change['output']); + self::assertSame(303, $r['status'], 'the password is changed'); + self::assertSame(1, self::generation(self::USER), 'the user\'s generation is counted up'); + self::assertSame($changing, $r['session_id'], 'the changing browser keeps its session'); + self::assertSame(1, $r['generation'], 'at the new generation'); + + [$otherIn, $otherId] = self::next($other); + self::assertFalse($otherIn, 'the other browser is signed out at its next request'); + self::assertNotSame($other, $otherId, 'with a new session id, as a sign-out'); + [$changingIn, $changingId] = self::next($changing); + self::assertTrue($changingIn, 'the changing browser is still signed in on its next request'); + self::assertSame($changing, $changingId); + } + + public function testAResetEndsEverySession(): void + { + $one = self::browser(); + $two = self::browser(); + $key = str_repeat('ef', 32); + self::q('UPDATE 202_users SET user_pass_key = ' . self::str($key) . ', user_pass_time = ' . time() + . ' WHERE user_id = ' . self::USER); + + $reset = self::request('202-pass-reset.php', [ + 'P202_SU_SESSION' => 'anonymous', + 'P202_SU_GET' => json_encode(['key' => $key], JSON_THROW_ON_ERROR), + 'P202_SU_POST' => json_encode( + ['token' => 'SESSION', 'user_pass' => self::NEW_PASSWORD, 'verify_user_pass' => self::NEW_PASSWORD], + JSON_THROW_ON_ERROR + ), + ]); + self::assertNull($reset['result']['uncaught'], $reset['output']); + self::assertStringContainsString('Your password has been reset.', $reset['output']); + self::assertSame(1, self::generation(self::USER)); + self::assertFalse(self::next($one)[0], 'a browser signed in before the reset is signed out'); + self::assertFalse(self::next($two)[0], 'and so is the other'); + } + + public function testAnAdminChangeThroughTheApiEndsTheUsersSessionsNotTheAdmins(): void + { + $users = self::browser(); + $admins = self::browser(self::ADMIN); + + (new \Api\V3\Controllers\UsersController(self::$db)) + ->update(self::USER, ['user_pass' => self::NEW_PASSWORD], self::ADMIN); + + self::assertSame(1, self::generation(self::USER)); + self::assertSame(0, self::generation(self::ADMIN)); + self::assertFalse(self::next($users)[0], 'the user\'s browser is signed out'); + self::assertTrue(self::next($admins, self::ADMIN)[0], 'the admin\'s is not'); + } + + public function testAnAdminChangeOnAccountUsersEndsTheUsersSessionsNotTheAdmins(): void + { + $users = self::browser(); + $admins = self::browser(self::ADMIN); + + $edit = self::request('202-account/user-management.php', [ + 'P202_SU_SESSION_ID' => $admins, + 'P202_SU_USER' => (string) self::ADMIN, + 'P202_SU_GET' => json_encode(['edit_user_id' => (string) self::USER], JSON_THROW_ON_ERROR), + 'P202_SU_POST' => json_encode([ + 'token' => 'SESSION', + 'user_id' => (string) self::USER, + 'user_fname' => 'Gen', + 'user_lname' => 'User', + 'user_email' => 'gen' . self::USER . '@example.com', + 'user_name' => 'gen' . self::USER, + 'user_password' => self::NEW_PASSWORD, + 'user_password2' => self::NEW_PASSWORD, + 'user_role' => '2', + 'user_active' => 'on', + ], JSON_THROW_ON_ERROR), + ]); + self::assertNull($edit['result']['uncaught'], $edit['output']); + self::assertSame(303, $edit['result']['status'], 'the edit is saved'); + self::assertSame(1, self::generation(self::USER)); + self::assertFalse(self::next($users)[0], 'the user\'s browser is signed out'); + self::assertTrue(self::next($admins, self::ADMIN)[0], 'the admin\'s is not'); + } + + /** The same password, stored again under a newer hash as the user signs in, ends nothing. */ + public function testARehashAtSignInEndsNoSession(): void + { + $open = self::browser(); + self::q("UPDATE 202_users SET user_pass = MD5(CONCAT('202', MD5(CONCAT('" . self::PASSWORD . "', '202'))))" + . ' WHERE user_id = ' . self::USER); + + $signIn = self::request('call:authenticate', [ + 'P202_SU_SESSION' => 'none', + 'P202_SU_NAME' => 'gen' . self::USER, + 'P202_SU_PASS' => self::PASSWORD, + ]); + self::assertNull($signIn['result']['uncaught'], $signIn['output']); + self::assertTrue($signIn['result']['answer'], 'the legacy password signs in, and is rehashed'); + self::assertSame(0, self::generation(self::USER), 'nothing is counted up'); + self::assertTrue(self::next($open)[0], 'the browser signed in before stays signed in'); + } + + /** A session from before 1.9.79 holds no generation: it is read as 0, and a change ends it. */ + public function testASessionFromBeforeTheUpgradeStaysUntilAChange(): void + { + $old = self::browser(self::USER, null); + self::assertTrue(self::next($old)[0], 'it stays signed in through the upgrade'); + + (new \Api\V3\Controllers\UsersController(self::$db)) + ->update(self::USER, ['user_pass' => self::NEW_PASSWORD], self::ADMIN); + self::assertFalse(self::next($old)[0], 'a new password ends it'); + } + + /** + * A switch-off counts the generation up too: a session that made no + * request while the user was off would otherwise go on once they are on + * again (the standing check sees only the request that comes while off). + */ + public function testASessionIdleThroughASwitchOffIsEndedWhenTheUserIsOnAgain(): void + { + $idle = self::browser(); + $controller = new \Api\V3\Controllers\UsersController(self::$db); + $controller->update(self::USER, ['user_active' => 0], self::ADMIN); + $controller->update(self::USER, ['user_active' => 1], self::ADMIN); + + self::assertSame(1, self::generation(self::USER), 'the switch-off counted up; the switch-on did not'); + self::assertFalse(self::next($idle)[0], 'the idle browser is signed out at its next request'); + } + + /** + * A read that fails is neither answer: the request is the database error + * page and the session, stale or not, is left as it was for a read that + * can answer. + */ + public function testAFailedReadNeitherEndsNorKeepsTheSession(): void + { + $stale = self::browser(); + self::q('UPDATE 202_users SET user_session_generation = 4 WHERE user_id = ' . self::USER); + try { + self::q('ALTER TABLE 202_users DROP COLUMN user_deleted'); + $failed = self::request('call:booted', ['P202_SU_SESSION_ID' => $stale]); + } finally { + self::q("ALTER TABLE 202_users ADD COLUMN user_deleted int(1) NOT NULL DEFAULT '0' AFTER user_active"); + } + $r = $failed['result']; + self::assertStringStartsWith( + 'Prosper202\\Database\\Exceptions\\QueryException: Reading whether user ' . self::USER . ' may act failed', + (string) $r['uncaught'], + $failed['output'] + ); + self::assertSame(500, $r['status'], 'the database error page'); + self::assertFileExists(self::$sessions . '/sess_' . $stale, 'the session was not ended'); + + self::assertFalse(self::next($stale)[0], 'a read that answers ends it'); + } + + /** + * Sessions that cannot be ended are not a password change: the count + * fails with the password in one transaction, and nothing is stored. + */ + public function testAPasswordWhoseSessionsCannotBeEndedIsNotStored(): void + { + $stored = static function (): string { + $result = self::$db->query('SELECT user_pass FROM 202_users WHERE user_id = ' . self::USER); + self::assertInstanceOf(\mysqli_result::class, $result); + + return (string) $result->fetch_row()[0]; + }; + $before = $stored(); + $writers = [ + 'Personal settings and the reset (PasswordChange::store)' => static function (): void { + \Prosper202\User\PasswordChange::store( + new \Prosper202\Database\Connection(self::$db), + self::USER, + password_hash(self::NEW_PASSWORD, PASSWORD_DEFAULT), + false + ); + }, + 'PUT /users/{id}' => static function (): void { + (new \Api\V3\Controllers\UsersController(self::$db)) + ->update(self::USER, ['user_pass' => self::NEW_PASSWORD], self::ADMIN); + }, + ]; + foreach ($writers as $name => $write) { + $threw = null; + try { + self::q('ALTER TABLE 202_users DROP COLUMN user_session_generation'); + $write(); + } catch (\Throwable $e) { + $threw = $e; + } finally { + self::q('ALTER TABLE 202_users ADD COLUMN user_session_generation int(10) unsigned NOT NULL' + . " DEFAULT '0'"); + } + self::assertNotNull($threw, "$name: a password whose sessions were not ended was answered as stored"); + $chain = ''; + for ($e = $threw; $e !== null; $e = $e->getPrevious()) { + $chain .= $e->getMessage() . "\n"; + } + self::assertStringContainsString('Ending the sessions of user ' . self::USER . ' failed', $chain, $name); + self::assertSame($before, $stored(), "$name: the old password stands"); + } + } + + /** + * Between deploying 1.9.79's files and running its upgrade, 202_users has + * no generation: the user is read without it, nothing is compared, and + * every request still runs. + */ + public function testBeforeTheUpgradeTheGenerationIsNotAskedAndSessionsRun(): void + { + try { + self::q('ALTER TABLE 202_users DROP COLUMN user_session_generation'); + // A generation no row holds: nothing compares it. + $session = self::browser(self::USER, 3); + $before = self::request('call:booted', ['P202_SU_SESSION_ID' => $session]); + } finally { + self::q("ALTER TABLE 202_users ADD COLUMN user_session_generation int(10) unsigned NOT NULL DEFAULT '0'"); + } + self::assertNull($before['result']['uncaught'], $before['output']); + self::assertContains('user_id', $before['result']['answer']['identity'], 'still signed in'); + self::assertTrue($before['result']['answer']['may_change_settings'], 'with the user\'s roles'); + } +} diff --git a/tests/Auth/SessionSignInPathsTest.php b/tests/Auth/SessionSignInPathsTest.php index 513d03c4d..a5e5d2d55 100644 --- a/tests/Auth/SessionSignInPathsTest.php +++ b/tests/Auth/SessionSignInPathsTest.php @@ -54,6 +54,14 @@ final class SessionSignInPathsTest extends TestCase /** The keys that say who the session is signed in as. */ private const IDENTITY = ['user_id', 'user_own_id', 'user_name', 'session_fingerprint']; + /** + * The generation a session began at (SessionGeneration): written by the + * sign-in, and by AUTH::keepSessionAfterPasswordChange() for the browser + * that changed its own password. Written anywhere else, a stale session + * could be given the current one and outlive the change. + */ + private const GENERATION = 'user_session_generation'; + /** * Files that open a session other than connect.php or after it, each with why * no session's identity is read there unchecked. @@ -334,6 +342,27 @@ public static function writeShapes(): array "class X { function begin_user_session() { \$_SESSION['user_id'] = 1; } }", true, ], + 'the generation, by a page' => ["\$_SESSION['user_session_generation'] = 7;", true], + 'the generation, counted up by a page' => ["\$_SESSION['user_session_generation']++;", true], + 'the generation, by begin_user_session' => [ + "class AUTH { public static function begin_user_session(array \$r): void {" + . " \$_SESSION['user_session_generation'] = 1; } }", + false, + ], + 'the generation, by keepSessionAfterPasswordChange' => [ + "class AUTH { public static function keepSessionAfterPasswordChange(int \$g): void {" + . " \$w = static function () use (\$g): void { \$_SESSION['user_session_generation'] = \$g; }; } }", + false, + ], + 'the generation, by another method of AUTH' => [ + "class AUTH { public static function other(int \$g): void {" + . " \$_SESSION['user_session_generation'] = \$g; } }", + true, + ], + 'the generation, handed to AUTH\'s writer' => [ + self::writer("self::writeSessionValue('user_session_generation', 5);"), + true, + ], ]; } @@ -559,6 +588,7 @@ private static function identityWrites(string $source): array // is handed: held below to a literal that is not an identity key. $writer = self::functionBody($t, 'AUTH', 'updateSession'); $writeValue = self::functionBody($t, 'AUTH', 'writeSessionValue'); + $keep = self::functionBody($t, 'AUTH', 'keepSessionAfterPasswordChange'); $inside = static fn (int $i, ?array $range): bool => $range !== null && $i > $range[0] && $i < $range[1]; $foreachTargets = []; foreach ($t as $i => $x) { @@ -586,7 +616,7 @@ private static function identityWrites(string $source): array if ( !is_array($key) || $key[0] !== T_CONSTANT_ENCAPSED_STRING || self::text($t[$i + 3] ?? '') !== ',' - || in_array(substr($key[1], 1, -1), self::IDENTITY, true) + || in_array(substr($key[1], 1, -1), [...self::IDENTITY, self::GENERATION], true) ) { $writes[] = $x[2]; } @@ -634,6 +664,8 @@ private static function identityWrites(string $source): array } if ($key === null || in_array($key, self::IDENTITY, true)) { $writes[] = $x[2]; + } elseif ($key === self::GENERATION && !$inside($i, $keep)) { + $writes[] = $x[2]; } } diff --git a/tests/Auth/fixtures/session-user-runner.php b/tests/Auth/fixtures/session-user-runner.php index d54177368..ba8b2a5f9 100644 --- a/tests/Auth/fixtures/session-user-runner.php +++ b/tests/Auth/fixtures/session-user-runner.php @@ -26,7 +26,10 @@ * P202_SU_POST (a JSON object: a POST; "token": "SESSION" sends the * session's), P202_SU_AJAX=1 (X-Requested-With, as jQuery sends it) or * fetch (Sec-Fetch-Dest: empty, as a browser marks fetch()), - * P202_SU_NAME, P202_SU_PASS, P202_SU_PERMISSION (call:may) + * P202_SU_NAME, P202_SU_PASS, P202_SU_PERMISSION (call:may), + * P202_SU_GENERATION (the session generation a new signed-in session + * holds; none when unset), P202_SU_SESSION_ID (resume the session an + * earlier request left, as the same browser's next request) * * Prints what the page printed, then one line: "@@RESULT " and a JSON object * with the status, whether 202-access-denied.php answered, the session's id @@ -78,7 +81,15 @@ $runnerToken = 'p202sessionuserrunnertoken000000'; session_save_path($sessionDir); -if (getenv('P202_SU_SESSION') === 'anonymous') { +$resume = (string) getenv('P202_SU_SESSION_ID'); +if ($resume !== '') { + // The same browser's next request: the session as an earlier request left it. + if (!is_file($sessionDir . '/sess_' . $resume)) { + fwrite(STDERR, "no session '$resume' to resume\n"); + exit(2); + } + session_id($resume); +} elseif (getenv('P202_SU_SESSION') === 'anonymous') { session_id('p202su' . bin2hex(random_bytes(8))); session_start(); $_SESSION = ['token' => $runnerToken]; @@ -101,6 +112,12 @@ 'valid_key' => true, 'token' => $runnerToken, ]; + // The generation the sign-in stored; none, as a session from before + // 1.9.79 holds, unless one is given. + $generation = getenv('P202_SU_GENERATION'); + if (is_string($generation) && $generation !== '') { + $_SESSION['user_session_generation'] = (int) $generation; + } session_write_close(); } if (($post['token'] ?? null) === 'SESSION') { @@ -131,6 +148,7 @@ 'session_id' => session_id(), 'session_keys' => array_keys($_SESSION ?? []), 'token' => $_SESSION['token'] ?? null, + 'generation' => $_SESSION['user_session_generation'] ?? null, 'cookie_kept' => isset($_COOKIE['remember_me']), 'answer' => $answer, 'uncaught' => $uncaught, diff --git a/tests/Upgrade/SessionGenerationUpgradeIntegrationTest.php b/tests/Upgrade/SessionGenerationUpgradeIntegrationTest.php new file mode 100644 index 000000000..e77589e75 --- /dev/null +++ b/tests/Upgrade/SessionGenerationUpgradeIntegrationTest.php @@ -0,0 +1,152 @@ + 1.9.79 rung adds 202_users.user_session_generation + * (Prosper202\User\SessionGeneration) as the installer makes it: an unsigned + * int, NOT NULL, DEFAULT 0, so every user already there is at 0, the value a + * session from before the upgrade (which holds none) is read as, and no one + * is signed out by the upgrade. Afterwards the table is the fresh install's + * (the comparison tests/live/upgrade-equals-install.sh makes), and a second + * run changes nothing. + * + * The rung runs as the upgrade page runs it (UPGRADE::upgrade_databases(), + * fixtures/ladder-runner.php) from a stored 1.9.78, on a 202_users without + * the column. + * + * @group integration + */ +final class SessionGenerationUpgradeIntegrationTest extends TestCase +{ + use \Tests\Report\ScratchReportDatabase; + + private const EXISTING = 990951; + + /** @var list|null 202_version as this class found it, put back after */ + private static ?array $versions = null; + + private static bool $backfillWasEmpty = false; + + public static function setUpBeforeClass(): void + { + if (!self::connectScratch()) { + return; + } + $found = self::$db->query('SELECT version FROM 202_version'); + $rows = $found instanceof \mysqli_result ? $found->fetch_all(MYSQLI_ASSOC) : []; + self::$versions = array_column($rows, 'version'); + $marks = (int) self::$db->query('SELECT COUNT(*) FROM 202_reclick_backfill')->fetch_row()[0]; + self::$backfillWasEmpty = $marks === 0; + } + + public static function tearDownAfterClass(): void + { + if (self::$db === null) { + return; + } + self::q('DELETE FROM 202_users WHERE user_id = ' . self::EXISTING); + if (self::$backfillWasEmpty) { + self::q('DELETE FROM 202_reclick_backfill'); + } + self::q('DELETE FROM 202_version'); + foreach (self::$versions ?? [] as $version) { + self::row('202_version', ['version' => $version]); + } + self::$db->close(); + self::$db = null; + } + + protected function setUp(): void + { + self::requireScratch(); + self::q('DELETE FROM 202_users WHERE user_id = ' . self::EXISTING); + $has = self::$db->query("SHOW COLUMNS FROM 202_users LIKE 'user_session_generation'"); + if ($has instanceof \mysqli_result && $has->num_rows === 0) { + self::q("ALTER TABLE 202_users ADD COLUMN user_session_generation int(10) unsigned NOT NULL DEFAULT '0'"); + } + } + + /** The table's DDL, less the AUTO_INCREMENT counter, which inserts move. */ + private static function shape(): string + { + $ddl = (string) self::$db->query('SHOW CREATE TABLE 202_users')->fetch_row()[1]; + + return (string) preg_replace('/ AUTO_INCREMENT=\d+/', '', $ddl); + } + + /** @return array{string, string} what the runner printed, and what was logged */ + private static function upgrade(): array + { + $proc = proc_open( + [PHP_BINARY, '-d', 'display_errors=stderr', '-d', 'error_log=', __DIR__ . '/fixtures/ladder-runner.php'], + [1 => ['pipe', 'w'], 2 => ['pipe', 'w']], + $pipes, + null, + getenv() + ); + self::assertIsResource($proc); + $out = (string) stream_get_contents($pipes[1]); + $err = (string) stream_get_contents($pipes[2]); + fclose($pipes[1]); + fclose($pipes[2]); + self::assertNotSame(2, proc_close($proc), "the runner could not set up: $err"); + + return [trim($out), $err]; + } + + private static function stored(string $version): void + { + self::q('DELETE FROM 202_version'); + self::row('202_version', ['version' => $version]); + } + + /** @param array{type: string, nullable: bool, default: ?string} $shape */ + private static function assertTheInstallersColumn(array $shape): void + { + // MySQL 8 writes no display width (int unsigned), MariaDB does. + self::assertMatchesRegularExpression('/^int(\(10\))? unsigned$/', $shape['type']); + self::assertSame([false, '0'], [$shape['nullable'], $shape['default']]); + } + + public function testTheRungAddsTheGenerationAtZeroAsTheInstallerMakesIt(): void + { + $generation = static fn (): array => ColumnShape::of(self::$db, '202_users', 'user_session_generation'); + self::assertStringContainsString('`user_session_generation`', UserTables::users()->createStatement); + self::assertTheInstallersColumn($generation()); + $fresh = self::shape(); + + // A 1.9.78 install's table, and a user on it. + self::q('ALTER TABLE 202_users DROP COLUMN user_session_generation'); + self::q('INSERT INTO 202_users SET user_id = ' . self::EXISTING . ", user_name = 'genupgrade'," + . " user_email = 'genupgrade@example.com', user_dash_email = '', user_pass = 'x', user_timezone = 'UTC'," + . " user_time_register = 0, install_hash = '', user_hash = ''"); + self::stored('1.9.78'); + + [$out, $err] = self::upgrade(); + + self::assertSame('RETURNED true', $out, $err); + self::assertStringNotContainsString('Prosper202 upgrade', $err, 'a rung that converged logs nothing'); + self::assertSame('1.9.79', (string) self::$db->query('SELECT version FROM 202_version')->fetch_row()[0]); + self::assertTheInstallersColumn($generation()); + self::assertSame($fresh, self::shape(), 'the upgraded table is the installed one'); + $at = self::$db->query('SELECT user_session_generation FROM 202_users WHERE user_id = ' . self::EXISTING); + self::assertSame('0', (string) $at->fetch_row()[0], 'a user already there is at 0, as old sessions read'); + + // Run again (a retry of a step that failed elsewhere): nothing moves. + self::q('UPDATE 202_users SET user_session_generation = 2 WHERE user_id = ' . self::EXISTING); + self::stored('1.9.78'); + [$out, $err] = self::upgrade(); + self::assertSame('RETURNED true', $out, $err); + self::assertStringNotContainsString('Prosper202 upgrade', $err); + self::assertSame($fresh, self::shape()); + $at = self::$db->query('SELECT user_session_generation FROM 202_users WHERE user_id = ' . self::EXISTING); + self::assertSame('2', (string) $at->fetch_row()[0], 'a generation counted up is kept'); + } +} diff --git a/tests/User/RememberMeKeysRevokedTest.php b/tests/User/RememberMeKeysRevokedTest.php index edcc3f444..f4f2456b4 100644 --- a/tests/User/RememberMeKeysRevokedTest.php +++ b/tests/User/RememberMeKeysRevokedTest.php @@ -87,6 +87,15 @@ final class RememberMeKeysRevokedTest extends TestCase 'api/v3/Controllers/UsersController.php::update' => self::CHANGE, ]; + /** Every statement that can write user_session_generation, path::function => why it only counts it up. */ + private const GENERATION_WRITERS = [ + '202-config/User/MysqlUserRepository.php::update' => 'a SET list of its allowed fields and the password', + '202-config/User/SessionGeneration.php::(file)' => 'SessionGeneration::BUMP, the constant bump() runs: + 1', + '202-config/User/UserDataPurge.php::deleteUser' => 'the soft delete, + 1', + '202-config/functions-account-ui.php::p202_account_save_user' => 'the page\'s own column literals', + 'api/v3/Controllers/UsersController.php::update' => 'PROFILE_FIELDS and the password', + ]; + /** The pages that sign a browser out. */ private const SIGN_OUTS = ['202-account/signout.php', '202-account/account.php']; @@ -97,7 +106,7 @@ public function testEveryWriterOfAPasswordRevokesTheKeysUnlessItIsARehash(): voi $found = []; $wrong = []; foreach ($files as $path => $source) { - foreach (self::writers($source, ['user_pass']) as [$function, $revokes, $line]) { + foreach (self::writers($source, ['user_pass']) as [$function, $revokes, $line, $bumps]) { $key = "$path::$function"; $found[$key] = true; $kind = self::PASSWORD_WRITERS[$key] ?? null; @@ -109,6 +118,10 @@ public function testEveryWriterOfAPasswordRevokesTheKeysUnlessItIsARehash(): voi $wrong[] = "$key (line $line): listed as " . ($isChange ? 'a change' : 'a rehash') . ($revokes ? ', and calls revokeAll()' : ', and does not call revokeAll()'); } + if ($bumps !== $isChange) { + $wrong[] = "$key (line $line): listed as " . ($isChange ? 'a change' : 'a rehash') + . ($bumps ? ', and ends the sessions' : ', and does not end the sessions (SessionGeneration)'); + } } } $found = array_keys($found); @@ -116,8 +129,9 @@ public function testEveryWriterOfAPasswordRevokesTheKeysUnlessItIsARehash(): voi $listed = array_keys(self::PASSWORD_WRITERS); sort($listed); self::assertSame([], $wrong, "A password writer does not do what it is listed as:\n " . implode("\n ", $wrong) - . "\nA new password calls Prosper202\\User\\RememberMeKeys::revokeAll() in the same transaction" - . ' (PasswordChange::store() does both); a rehash of the password just verified at sign-in must not.'); + . "\nA new password calls Prosper202\\User\\RememberMeKeys::revokeAll() and" + . ' Prosper202\\User\\SessionGeneration::bump() in the same transaction (PasswordChange::store() does' + . ' all three); a rehash of the password just verified at sign-in must do neither.'); self::assertSame($listed, $found, 'The writers of user_pass changed. A new one is a change, which revokes' . " the user's remember-me keys in its transaction, or a rehash of the password the user has just" . ' signed in with, which must not; list it in PASSWORD_WRITERS as which, and remove one that no longer' @@ -130,13 +144,17 @@ public function testEveryWriterThatLeavesAUserUnableToActRevokesTheirKeys(): voi self::assertGreaterThan(500, count($files), 'the walk read the served tree'); $found = []; $unrevoked = []; + $unended = []; foreach ($files as $path => $source) { - foreach (self::writers($source, self::STANDING) as [$function, $revokes, $line]) { + foreach (self::writers($source, self::STANDING) as [$function, $revokes, $line, $bumps]) { $key = "$path::$function"; $found[$key] = true; if (!$revokes && !array_key_exists($key, self::REVOKED_OTHERWISE)) { $unrevoked[] = "$key (line $line)"; } + if (!$bumps) { + $unended[] = "$key (line $line)"; + } } } $found = array_keys($found); @@ -145,10 +163,108 @@ public function testEveryWriterThatLeavesAUserUnableToActRevokesTheirKeys(): voi . " and do not delete the user's remember-me keys:\n " . implode("\n ", $unrevoked) . "\nCall Prosper202\\User\\RememberMeKeys::revokeAll() in the same transaction when the write leaves the" . ' user unable to act (UserStanding::isActive()).'); + self::assertSame([], $unended, "These write user_active or user_deleted, or a SET list built at run time," + . " and do not end the user's open sessions:\n " . implode("\n ", $unended) + . "\nCall Prosper202\\User\\SessionGeneration::bump() beside revokeAll(), or count" + . ' user_session_generation up in the statement: a session idle through a switch-off would otherwise' + . ' go on once the user is switched on again.'); self::assertSame(self::WRITERS, $found, 'The writers of user_active and user_deleted changed: a new one revokes' . ' the keys and goes in WRITERS, and one that no longer writes them leaves it.'); } + /** + * Every statement that writes user_session_generation counts it up by + * one: set to any other value (back to 0, or to what an old session + * holds) it would bring the sessions a change ended back. Run-time SET + * lists that could name it are pinned with the fields they write. + */ + public function testTheSessionGenerationOnlyCountsUp(): void + { + $found = []; + $wrong = []; + foreach (SourceScan::phpFiles() as $path => $source) { + foreach (self::writers($source, ['user_session_generation']) as [$function, , $line, , $sql]) { + $found["$path::$function"] = true; + if (!self::onlyCountsTheGenerationUp($sql)) { + $wrong[] = "$path::$function (line $line)"; + } + } + } + $found = array_keys($found); + sort($found); + $listed = array_keys(self::GENERATION_WRITERS); + sort($listed); + self::assertSame([], $wrong, "These set user_session_generation to something other than itself + 1:\n " + . implode("\n ", $wrong) . "\nCount it up with Prosper202\\User\\SessionGeneration::bump()."); + self::assertSame($listed, $found, 'The writers of user_session_generation changed: list a new one in' + . ' GENERATION_WRITERS with why it only counts it up.'); + } + + /** Whether every assignment of user_session_generation in assembled SQL is itself + 1. */ + private static function onlyCountsTheGenerationUp(string $sql): bool + { + $assigned = preg_match_all('/`?user_session_generation`?\s*=/i', $sql); + $countedUp = preg_match_all( + '/`?user_session_generation`?\s*=\s*`?user_session_generation`?\s*\+\s*1\b/i', + $sql + ); + + return $assigned === $countedUp; + } + + /** + * Whether a writer ends the sessions, and whether it only counts the + * generation up, executed on planted sources. + * + * @dataProvider generationShapes + */ + public function testTheScanReadsHowAWriterEndsTheSessions(string $code, bool $ends, bool $countsUp): void + { + $writers = self::writers(" */ + public static function generationShapes(): array + { + $wrap = static fn (string $body): string => 'function f($conn, $id) { ' . $body . ' }'; + $off = "\$sql = 'UPDATE 202_users SET user_active = 0 WHERE user_id = ?';"; + + return [ + 'neither' => [$wrap($off), false, true], + 'bump()' => [$wrap($off . ' \\Prosper202\\User\\SessionGeneration::bump($conn, $id);'), true, true], + 'bump(), imported' => [ + "use Prosper202\\User\\SessionGeneration;\n" . $wrap($off . ' SessionGeneration::bump($conn, $id);'), + true, + true, + ], + 'another class' => [$wrap($off . ' MySessionGeneration::bump($conn, $id);'), false, true], + 'a method of the name' => [$wrap($off . ' $generation->bump($conn, $id);'), false, true], + 'counted up in the statement' => [ + $wrap("\$sql = 'UPDATE 202_users SET user_active = 0," + . " user_session_generation = user_session_generation + 1 WHERE user_id = ?';"), + true, + true, + ], + 'set to 0' => [ + $wrap("\$sql = 'UPDATE 202_users SET user_active = 1, user_session_generation = 0';"), + false, + false, + ], + 'counted down' => [ + $wrap("\$sql = 'UPDATE 202_users SET user_session_generation = user_session_generation - 1';"), + false, + false, + ], + 'counted up by two' => [ + $wrap("\$sql = 'UPDATE 202_users SET user_session_generation = user_session_generation + 2';"), + false, + false, + ], + ]; + } + public function testTheDeleteCascadeRevokesTheKeys(): void { self::assertContains(RememberMeKeys::REVOKE_ALL, UserDataPurge::statements()); @@ -385,10 +501,12 @@ public static function signOutShapes(): array /** * The writers of any of $columns a source holds: [function, whether that - * function revokes, line]. + * function revokes the keys, line, whether it ends the sessions (calls + * SessionGeneration::bump() or counts the generation up in the statement), + * the statement's SQL as assembled]. * * @param list $columns - * @return list + * @return list */ private static function writers(string $source, array $columns): array { @@ -418,12 +536,23 @@ private static function writers(string $source, array $columns): array } } [$name, $open, $close] = $scope ?? ['(file)', 0, count($t)]; - $writers[] = [$name, self::revokesBetween($t, $open, $close, $source), $x[2]]; + $sql = self::assemble($t, $start); + $bumps = self::callsBetween($t, $open, $close, $source, 'Prosper202\\User\\SessionGeneration', 'bump') + || self::countsTheGenerationUp($sql); + $writers[] = [$name, self::revokesBetween($t, $open, $close, $source), $x[2], $bumps, $sql]; } return $writers; } + /** Whether assembled SQL counts the session generation up, in its own SET list. */ + private static function countsTheGenerationUp(string $sql): bool + { + $countUp = '/(?:SET|,)\s*`?user_session_generation`?\s*=\s*`?user_session_generation`?\s*\+\s*1\b/i'; + + return preg_match($countUp, $sql) === 1; + } + /** * Whether assembled SQL (run-time pieces as \0) writes one of $columns of * 202_users, or a SET list it cannot read. @@ -552,21 +681,38 @@ private static function stringStart(array $t, int $i): int */ private static function revokesBetween(array $t, int $open, int $close, string $source): bool { + return self::callsBetween($t, $open, $close, $source, 'Prosper202\\User\\RememberMeKeys', 'revokeAll'); + } + + /** + * Whether a call to $class::$method() sits between two tokens, the class + * name read as PHP resolves it. + * + * @param list $t + */ + private static function callsBetween( + array $t, + int $open, + int $close, + string $source, + string $class, + string $method + ): bool { [$namespace, $imports] = self::names($source); for ($i = $open; $i < $close; $i++) { $x = $t[$i]; if ( !is_array($x) || self::text($t[$i + 1] ?? '') !== '::' - || strcasecmp(self::text($t[$i + 2] ?? ''), 'revokeAll') !== 0 || self::text($t[$i + 3] ?? '') !== '(' + || strcasecmp(self::text($t[$i + 2] ?? ''), $method) !== 0 || self::text($t[$i + 3] ?? '') !== '(' ) { continue; } - $class = match ($x[0]) { + $named = match ($x[0]) { T_NAME_FULLY_QUALIFIED => ltrim($x[1], '\\'), T_STRING => $imports[strtolower($x[1])] ?? ($namespace === '' ? $x[1] : $namespace . '\\' . $x[1]), default => null, }; - if ($class !== null && strcasecmp($class, 'Prosper202\\User\\RememberMeKeys') === 0) { + if ($named !== null && strcasecmp($named, $class) === 0) { return true; } } diff --git a/tests/User/UserManagementSaveTest.php b/tests/User/UserManagementSaveTest.php index 3bcd85b88..7b2073d25 100644 --- a/tests/User/UserManagementSaveTest.php +++ b/tests/User/UserManagementSaveTest.php @@ -29,6 +29,21 @@ protected function setUp(): void require_once dirname(__DIR__, 2) . '/202-config/functions-account-ui.php'; } + /** + * A database holding user 9, which an edit locks; the edit sets a + * password, so it counts the user's session generation up (one row) and + * reads the new one. + */ + private static function editable(): FakeMysqliConnection + { + $db = new FakeMysqliConnection(); + $db->whenQueryContainsReturnRows('FOR UPDATE', [['user_id' => 9]]); + $db->whenQueryContainsAffectedRows('user_session_generation + 1', 1); + $db->whenQueryContainsReturnRows(\Prosper202\User\SessionGeneration::READ, [['user_session_generation' => 1]]); + + return $db; + } + public function testACreateWritesTheUserItsPreferencesAndItsRoleAndCommitsOnce(): void { $db = new FakeMysqliConnection(); @@ -78,16 +93,20 @@ public function testACreateThatFailsAnywhereLeavesNothing(string $failing): void public function testAnEditReplacesTheRoleOfAUserWhoIsStillThere(): void { - $db = new FakeMysqliConnection(); - $db->whenQueryContainsReturnRows('FOR UPDATE', [['user_id' => 9]]); + $db = self::editable(); $id = p202_account_save_user(new Connection($db), 9, self::USER, 4); self::assertSame(9, $id); $order = array_map(static fn ($s): string => strtok($s->sql, ' ') . ' ' . (preg_match('/`?(202_\w+)`?/', $s->sql, $m) ? $m[1] : ''), $db->statements); - self::assertSame(['SELECT 202_users', 'UPDATE 202_users', 'DELETE 202_auth_keys', 'DELETE 202_user_role', 'INSERT 202_user_role'], $order, - 'lock the user, write it, delete its remember-me keys (the edit sets a password), and replace its role,' - . ' so a user with no role row gets one'); + self::assertSame( + ['SELECT 202_users', 'UPDATE 202_users', 'DELETE 202_auth_keys', 'UPDATE 202_users', 'SELECT 202_users', + 'DELETE 202_user_role', 'INSERT 202_user_role'], + $order, + 'lock the user, write it, delete its remember-me keys and end its sessions (the edit sets a password),' + . ' and replace its role, so a user with no role row gets one' + ); self::assertSame([9], $db->statementsContaining(\Prosper202\User\RememberMeKeys::REVOKE_ALL)[0]->boundValues); + self::assertSame([9], $db->statementsContaining(\Prosper202\User\SessionGeneration::BUMP)[0]->boundValues); $update = $db->statementsContaining('UPDATE `202_users` SET')[0]; self::assertStringEndsWith('WHERE `user_id` = ? AND `user_deleted` != 1', $update->sql, 'a removed user is not written'); self::assertSame(9, $update->boundValues[count($update->boundValues) - 1]); @@ -147,8 +166,7 @@ public function testACreatesFirstRowsAreWrittenInsideItsTransactionAndAFailureTh self::assertTrue($db->rollbackCalled, 'the account was rolled back with it'); self::assertFalse($db->commitCalled); - $db = new FakeMysqliConnection(); - $db->whenQueryContainsReturnRows('FOR UPDATE', [['user_id' => 9]]); + $db = self::editable(); $called = false; p202_account_save_user(new Connection($db), 9, self::USER, 4, static function () use (&$called): void { $called = true; diff --git a/tests/live/password-ends-sessions.sh b/tests/live/password-ends-sessions.sh new file mode 100755 index 000000000..4ef739dc2 --- /dev/null +++ b/tests/live/password-ends-sessions.sh @@ -0,0 +1,209 @@ +#!/bin/bash +# Live pass: a new password ends every other browser signed in as the user. +# +# A new password revoked the user's remember-me keys (#325) and left every +# browser already signed in signed in, so a thief who had used a stolen +# cookie kept that session after the owner changed the password. A session +# now holds the user's session generation from its sign-in; a new password, +# a switch-off or a removal counts it up, and connect.php ends a session that +# holds another at its next request. +# +# The pass signs in as the admin (P202_USER), adds a second Admin through +# Account › Users, and signs that user in in browsers of their own (cookie +# jars, without "Keep me signed in", so only the session can keep them in). +# A browser is signed in when Personal settings answers 200 with its form, +# and signed out when it is sent to the sign-in page. Then: +# +# - two browsers; the user changes the password on Personal settings in one: +# that one stays signed in, on the same session id, and the other is +# signed out at its next request, with a new session id; +# - two browsers; a reset through 202-lost-pass.php and 202-pass-reset.php +# (the key read from the database, as the email cannot be read here) signs +# both out; +# - the admin sets the password through PUT /api/v3/users/{id} +# (P202_API_KEY) and then on Account › Users' edit form: each signs the +# user's browser out, and the admin's own stays signed in; +# - a sign-in that rehashes a legacy MD5 password signs nobody out; +# - a browser that makes no request while the user is switched off and on +# again through PUT is signed out at its next request. +# +# It adds users named "rkse…" and removes them. +BASE=${P202_BASE:-http://127.0.0.1:8097} +DB=${P202_DB:-p202_test} +DB_USER=${P202_DB_USER:-root} +DB_PASS=${P202_DB_PASS:-} +DB_HOST=${P202_DB_HOST:-} +DB_PORT=${P202_DB_PORT:-} +P202_USER=${P202_USER:-evalci} +P202_PASS=${P202_PASS:-} +P202_API_KEY=${P202_API_KEY:-} + +if [ -z "$P202_PASS" ] || [ -z "$P202_API_KEY" ]; then + echo "P202_PASS and P202_API_KEY are required: this pass signs in as $P202_USER and calls PUT /users as it." >&2 + exit 2 +fi +HERE="$(dirname "${BASH_SOURCE[0]}")" +# shellcheck source=tests/live/guard.sh +. "$HERE/guard.sh" +p202_require_scratch_db "$DB" || exit 2 + +MYSQL_ARGS=(-u "$DB_USER") +[ -n "$DB_PASS" ] && MYSQL_ARGS+=("-p$DB_PASS") +[ -n "$DB_HOST" ] && MYSQL_ARGS+=(-h "$DB_HOST" --protocol=TCP) +[ -n "$DB_PORT" ] && MYSQL_ARGS+=(-P "$DB_PORT") +Q() { mysql "${MYSQL_ARGS[@]}" -N "$DB" -e "$1"; } + +OUT=$(mktemp -d) +ADMIN="$OUT/admin.jar" +PASS=0; FAIL=0 +say() { printf '\n\033[1m== %s\033[0m\n' "$1"; } +ok() { PASS=$((PASS+1)); printf ' \033[32mPASS\033[0m %s\n' "$1"; } +bad() { FAIL=$((FAIL+1)); printf ' \033[31mFAIL\033[0m %s\n' "$1"; } +eq() { if [ "$1" = "$2" ]; then ok "$3"; else bad "$3 (got '$1' want '$2')"; fi; } + +# GET with a jar; the body to $3, "status redirect" to stdout. +get() { curl -sS -b "$1" -c "$1" -o "$3" -w '%{http_code} %{redirect_url}' "$BASE/$2"; } +submit() { # JAR PAGE MARKER URL OUT [name=value ...] + local jar="$1" page="$2" marker="$3" url="$4" out="$5"; shift 5 + python3 "$HERE/form-body.py" "$page" "$marker" "$@" > "$OUT/.body" || { echo "000 -"; return; } + curl -sS -b "$jar" -c "$jar" -o "$out" -w '%{http_code} %{redirect_url}' --data-binary @"$OUT/.body" \ + -H 'Content-Type: application/x-www-form-urlencoded' "$BASE/$url" +} +sid() { awk '$6=="PHPSESSID"{print $7}' "$1" | tail -1; } +# Sign USER in through the sign-in form, for this browser's session only. +sign_in() { # JAR USER PASS + rm -f "$1" + get "$1" 202-login.php "$OUT/login.html" > /dev/null + submit "$1" "$OUT/login.html" user_name 202-login.php "$OUT/signed-in.html" "user_name=$2" "user_pass=$3" +} +SIGNIN="$BASE/202-login.php?redirect=%2F202-account%2Faccount.php" +# Whether a browser is signed in: Personal settings with its form, or sent to sign in. +state() { # JAR + local answer + answer=$(get "$1" 202-account/account.php "$OUT/state.html") + case "$answer" in + "200 ") grep -q 'name="new_user_pass"' "$OUT/state.html" && echo in || echo "200 without the form" ;; + "302 $SIGNIN") echo out ;; + *) echo "$answer" ;; + esac +} +browser() { # JAR PASS: a browser of the user, signed in + local code + code=$(sign_in "$1" "$NAME" "$2") + [ "${code%% *}" = "302" ] || echo "sign-in answered $code" >&2 +} +generation() { Q "SELECT user_session_generation FROM 202_users WHERE user_id = '$USER_ID'"; } +signs_in_with() { # PASS: "302" signs in, "200" draws the form again + local code + code=$(sign_in "$OUT/try.jar" "$NAME" "$1") + echo "${code%% *}" +} + +STAMP=$(date +%s) +NAME="rkse$STAMP" +UPASS="rkse-$STAMP-pass" +NEW1="rkse-$STAMP-new1" +NEW2="rkse-$STAMP-new2" +NEW3="rkse-$STAMP-new3" +NEW4="rkse-$STAMP-new4" +cleanup() { + Q "DELETE k FROM 202_auth_keys k JOIN 202_users u ON u.user_id = k.user_id WHERE u.user_name LIKE 'rkse%'" 2>/dev/null + Q "DELETE k FROM 202_api_keys k JOIN 202_users u ON u.user_id = k.user_id WHERE u.user_name LIKE 'rkse%'" 2>/dev/null + Q "DELETE u, r, p FROM 202_users u LEFT JOIN 202_user_role r ON r.user_id = u.user_id LEFT JOIN 202_users_pref p ON p.user_id = u.user_id WHERE u.user_name LIKE 'rkse%'" 2>/dev/null + rm -rf "$OUT" +} +trap cleanup EXIT + +say "the admin signs in and adds a second Admin" +code=$(sign_in "$ADMIN" "$P202_USER" "$P202_PASS") +eq "${code%% *}" "302" "the admin's sign-in redirects" +eq "$(get "$ADMIN" 202-account/user-management.php "$OUT/users.html" | cut -d' ' -f1)" "200" "Account › Users answers 200" +code=$(submit "$ADMIN" "$OUT/users.html" user_fname 202-account/user-management.php "$OUT/added.html" \ + "user_fname=Session" "user_lname=Ends" "user_email=$NAME@example.com" "user_name=$NAME" \ + "user_password=$UPASS" "user_password2=$UPASS" "user_role=2" "user_active=on") +eq "${code%% *}" "303" "the add form redirects" +USER_ID=$(Q "SELECT user_id FROM 202_users WHERE user_name = '$NAME' AND user_deleted = 0 AND user_active = 1") +eq "$(printf '%s' "$USER_ID" | grep -cE '^[0-9]+$')" "1" "the user $NAME is stored, active (id $USER_ID)" +eq "$(generation)" "0" "at generation 0" + +say "two browsers; the password changed on Personal settings in one" +browser "$OUT/one.jar" "$UPASS" +browser "$OUT/two.jar" "$UPASS" +eq "$(state "$OUT/one.jar")" "in" "the changing browser is signed in" +eq "$(state "$OUT/two.jar")" "in" "the other browser is signed in" +ONE=$(sid "$OUT/one.jar"); TWO=$(sid "$OUT/two.jar") +get "$OUT/one.jar" 202-account/account.php "$OUT/account.html" > /dev/null +code=$(submit "$OUT/one.jar" "$OUT/account.html" new_user_pass 202-account/account.php "$OUT/changed.html" \ + "user_pass=$UPASS" "new_user_pass=$NEW1" "retype_new_user_pass=$NEW1") +eq "${code%% *}" "303" "the password form redirects" +eq "$(generation)" "1" "the user's generation is counted up" +eq "$(state "$OUT/one.jar")" "in" "the changing browser stays signed in" +eq "$(sid "$OUT/one.jar")" "$ONE" "on the same session" +eq "$(state "$OUT/two.jar")" "out" "the other browser is sent to sign in at its next request" +[ "$(sid "$OUT/two.jar")" != "$TWO" ] && ok "with a new session id" || bad "the other browser kept its session id" +eq "$(state "$OUT/two.jar")" "out" "and stays signed out" +eq "$(signs_in_with "$UPASS")" "200" "the old password is refused" +eq "$(signs_in_with "$NEW1")" "302" "the new one signs in" + +say "two browsers; a reset through the lost-password page" +browser "$OUT/three.jar" "$NEW1" +browser "$OUT/four.jar" "$NEW1" +eq "$(state "$OUT/three.jar")" "in" "a browser is signed in" +eq "$(state "$OUT/four.jar")" "in" "and another" +get "$OUT/reset.jar" 202-lost-pass.php "$OUT/lost.html" > /dev/null +code=$(submit "$OUT/reset.jar" "$OUT/lost.html" user_email 202-lost-pass.php "$OUT/asked.html" \ + "user_name=$NAME" "user_email=$NAME@example.com") +KEY=$(Q "SELECT user_pass_key FROM 202_users WHERE user_id = '$USER_ID'") +eq "$(printf '%s' "$KEY" | grep -cE '^[0-9a-f]{64}$')" "1" "a reset key is stored (read from the database)" +get "$OUT/reset.jar" "202-pass-reset.php?key=$KEY" "$OUT/reset.html" > /dev/null +submit "$OUT/reset.jar" "$OUT/reset.html" verify_user_pass "202-pass-reset.php?key=$KEY" "$OUT/reset-done.html" \ + "user_pass=$NEW2" "verify_user_pass=$NEW2" > /dev/null +grep -q 'Your password has been reset.' "$OUT/reset-done.html" && ok "the reset page says the password is reset" \ + || bad "the reset page does not say the password is reset" +eq "$(generation)" "2" "the user's generation is counted up" +eq "$(state "$OUT/three.jar")" "out" "the first browser is sent to sign in" +eq "$(state "$OUT/four.jar")" "out" "and so is the second" + +say "the admin sets the password through PUT /api/v3/users/{id}" +browser "$OUT/five.jar" "$NEW2" +eq "$(state "$OUT/five.jar")" "in" "the user's browser is signed in" +eq "$(curl -sS -o "$OUT/put.json" -w '%{http_code}' -X PUT -H "Authorization: Bearer $P202_API_KEY" \ + -H 'Content-Type: application/json' --data "{\"user_pass\": \"$NEW3\"}" "$BASE/api/v3/users/$USER_ID")" "200" \ + "PUT user_pass answers 200" +eq "$(state "$OUT/five.jar")" "out" "the user's browser is sent to sign in" +eq "$(get "$ADMIN" 202-account/user-management.php "$OUT/admin.html" | cut -d' ' -f1)" "200" "the admin's own stays signed in" + +say "the admin sets the password on Account › Users" +browser "$OUT/six.jar" "$NEW3" +eq "$(state "$OUT/six.jar")" "in" "the user's browser is signed in" +get "$ADMIN" "202-account/user-management.php?edit_user_id=$USER_ID" "$OUT/edit.html" > /dev/null +code=$(submit "$ADMIN" "$OUT/edit.html" user_fname "202-account/user-management.php?edit_user_id=$USER_ID" "$OUT/edited.html" \ + "user_password=$NEW4" "user_password2=$NEW4" "user_active=on") +eq "${code%% *}" "303" "the edit form, with a new password, redirects" +eq "$(state "$OUT/six.jar")" "out" "the user's browser is sent to sign in" +eq "$(get "$ADMIN" 202-account/user-management.php "$OUT/admin.html" | cut -d' ' -f1)" "200" "the admin's own stays signed in" + +say "a sign-in that rehashes the password signs nobody out" +browser "$OUT/seven.jar" "$NEW4" +eq "$(state "$OUT/seven.jar")" "in" "a browser is signed in" +BEFORE=$(generation) +Q "UPDATE 202_users SET user_pass = MD5(CONCAT('202', MD5(CONCAT('$NEW4', '202')))) WHERE user_id = '$USER_ID'" +eq "$(signs_in_with "$NEW4")" "302" "the legacy password signs in" +eq "$(Q "SELECT LEFT(user_pass, 4) FROM 202_users WHERE user_id = '$USER_ID'")" "\$2y\$" "and is stored again, rehashed" +eq "$(generation)" "$BEFORE" "the generation is not counted up" +eq "$(state "$OUT/seven.jar")" "in" "the browser signed in before stays signed in" + +say "a browser idle while the user is switched off and on again" +browser "$OUT/eight.jar" "$NEW4" +eq "$(state "$OUT/eight.jar")" "in" "a browser is signed in" +put_active() { + curl -sS -o "$OUT/put.json" -w '%{http_code}' -X PUT -H "Authorization: Bearer $P202_API_KEY" \ + -H 'Content-Type: application/json' --data "{\"user_active\": $1}" "$BASE/api/v3/users/$USER_ID" +} +eq "$(put_active 0)" "200" "PUT user_active 0 answers 200" +eq "$(put_active 1)" "200" "PUT user_active 1 answers 200" +eq "$(state "$OUT/eight.jar")" "out" "the idle browser is sent to sign in at its next request" +eq "$(signs_in_with "$NEW4")" "302" "the user signs in again as usual" + +printf '\n%d passed, %d failed\n' "$PASS" "$FAIL" +[ "$FAIL" -eq 0 ] From 5d3f57d12abdb40e3bf1f76dcbeb5c311022c226 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 28 Oct 2026 16:05:59 +0000 Subject: [PATCH 2/4] 1.9.79: docs, changelogs and two more session cases for the session generation The users API page, the OpenAPI spec and the CLI guide say a new password and a switch-off sign out every browser signed in as the user; the 1.9.79 changelog lines say so, after 1.9.79's own, and #325's line no longer says a signed-in browser stays signed in. tests/live/README.md lists password-ends-sessions.sh, and CLAUDE.md #5 says what the session was. SessionGenerationIntegrationTest also holds a sign-in after a change to the new generation, and an ended session to revoking no key a later sign-in made. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01VRoS2SViuxaucFaKWsFnLr --- 202-config/changelogs.json | 5 ++- CLAUDE.md | 11 ++++++ changelogs.txt | 3 +- docs/cli.md | 2 +- docs/openapi.yaml | 5 ++- documentation/api/14-users.md | 14 ++++--- .../Auth/SessionGenerationIntegrationTest.php | 37 +++++++++++++++++++ tests/live/README.md | 1 + 8 files changed, 66 insertions(+), 12 deletions(-) diff --git a/202-config/changelogs.json b/202-config/changelogs.json index bd92529f9..97b0aaa6d 100644 --- a/202-config/changelogs.json +++ b/202-config/changelogs.json @@ -73,8 +73,9 @@ "Fixed: Security. A user switched off or removed in My Account, Users stayed signed in on every browser that already was. Their next request now signs them out, and nothing it asked for is done", "Fixed: Security. Signing out left the browser's remember-me key valid, so a copy of its cookie still signed in for up to 14 days, and switching a user off left all of theirs valid, so a remembered browser signed back in once the user was switched on again. Signing out now deletes that browser's key (the user's other remembered browsers stay signed in), and switching a user off in My Account, Users or through the API deletes every one of theirs", "Fixed: A sync writes a redirector's rules on the other install whole or not at all. When the other install refused one (since this version, a rule with no criterion), its redirector was left with its own rules removed and only the synced rules before the refused one", - "Fixed: Security. Changing a password left every browser kept signed in able to sign in again by its cookie, a stolen copy included. A new password (Personal settings, the reset link, My Account, Users, or the API) now ends them all; the browser making the change on Personal settings stays signed in. A browser already signed in stays signed in until it signs out or its session ends", - "Fixed: The first page opened with only a remember-me cookie (Keep me signed in) ran without the user's roles, so a page such as My Account, Users sent the user away. It has them now" + "Fixed: Security. Changing a password left every browser kept signed in able to sign in again by its cookie, a stolen copy included. A new password (Personal settings, the reset link, My Account, Users, or the API) now ends them all; the browser making the change on Personal settings stays signed in. A browser already signed in is signed out too", + "Fixed: The first page opened with only a remember-me cookie (Keep me signed in) ran without the user's roles, so a page such as My Account, Users sent the user away. It has them now", + "Fixed: Security. Changing or resetting a password now signs out every other browser signed in as the user, not only those kept signed in; the browser making the change on Personal settings stays signed in. Switching a user off ends their sessions for good, also one that was idle meanwhile" ] }, "1.9.78": { diff --git a/CLAUDE.md b/CLAUDE.md index 69476c680..b1b8875d8 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -378,6 +378,17 @@ setting it; the other tables that keep `updated_at` are not held, and some of their writers do not (a model's default cleared, an app's icon stored). Writing a stamped row means writing its stamp. +And a session is a credential the password does not reach: revoking the +keys left every browser already signed in signed in, so whoever had used a +stolen cookie kept that session through the owner's new password (measured +live). Ending something that holds no state on the server needs state to +compare: each session holds the user's session generation from its +sign-in, every change that ends sign-ins counts the row's up in its +transaction, and connect.php compares the two in the read it already makes. +A counter, not a time, so no same-second race and no clock to skew. +`RememberMeKeysRevokedTest` holds every key-revoking writer to counting it +up, the rehash to not, and every write of the column to `+ 1`. + ### 6. Empty response rendering for void operations DELETE/204 responses return empty arrays. Rendering an empty array produces no output. Void operations (delete, remove, revoke) need explicit success messages, not render calls. diff --git a/changelogs.txt b/changelogs.txt index 270bc4228..3fb432468 100755 --- a/changelogs.txt +++ b/changelogs.txt @@ -123,8 +123,9 @@ Version 1.9.79 -- Fixed: Security. A user switched off in My Account › Users, or removed, kept every page, AJAX request and form of a browser already signed in, for as long as they kept using it: the sign-in, the remember-me cookie and the API keys refused them, but a session already signed in was never asked again, and each request kept it alive. Every signed-in request now reads whether its user may still act, in the read that already loads the user's role (no extra query): when the user is switched off, removed or no longer there, the session ends on that request as a sign-out ends it (a new session id, the remember-me cookie cleared) and the request is answered as signed out, with nothing it asked for done. Every remember-me key the user holds is revoked then, so a browser kept signed in does not sign back in when the user is switched on again (switching the user off now revokes them too, below). A user switched back on signs in again as usual. The sign-in, the remember-me cookie and the session now ask one rule: active is user_active 1 and not removed is user_deleted 0, so a value no version writes is a user who may not act (the sign-in let a user_deleted of 2 through). A read of the user that fails is the database error page, never read as signed in or as signed out. A request a page's script makes (jQuery's, or fetch()) that is not signed in, for this reason or because its session expired, is answered 401 "You are signed out. Sign in again, then reload the page.", which the page shows as a failed request; it was sent to the sign-in page, and the page drew the sign-in page where the answer belonged -- Fixed: Security. A remember-me cookie ("Keep me signed in") went on signing in after what should have ended it. Signing out cleared the cookie in that browser and kept the stored key it names, so a copy of the cookie (a copied or synced browser profile, a backup of one) signed in for the rest of its 14 days. Signing out now deletes that browser's key, and so does the sign-out Personal Settings makes after too many wrong current passwords. It deletes that browser's key only: the same user's other remembered browsers are sign-ins of their own and stay signed in (switching the user off ends them all). If the key cannot be deleted, the browser is still signed out, its session ended and its cookie cleared, and the sign-out ends on the database error page, which records the error; the key then stays valid until it expires or the user is switched off. Switching a user off (My Account › Users, PUT /users/{id}, p202 user update --user-active 0) kept every remember-me key of theirs, so a remembered browser that made no request while the user was off signed back in once they were switched on again. Switching off now deletes every key the user holds, in the same transaction as the switch-off: a switch-off whose keys cannot be deleted is not saved (the page says nothing about the user changed, the API answers 500). Removing a user already deleted their keys -- Fixed: A sync (p202 sync and re-sync, the sync jobs) writes a redirector's rules on the target as one set (PUT /rotators/{id}/rules), which the target takes whole or refuses with nothing written. It removed the target redirector's rules and then posted the source's one at a time, so a rule the target refused stopped it with the target holding only the rules before that one: on a re-sync the target's own rules were gone. Since this version refuses a rule with no criterion, a source rule stored without one, which had synced, did exactly that. The refusal names the source rule (give it a criterion; the value ALL keeps it on every visitor). A target from before 1.9.79, which has no rule set route, is written to one rule at a time as before --- Fixed: Security. A new password left every remember-me cookie ("Keep me signed in") of the user signing in, so a stolen cookie went on working after its owner changed the password to lock the thief out. Changing your password on Personal settings, a reset through the lost-password link, an admin setting a password on My Account › Users, and PUT /users/{id} with user_pass (p202 user update --set-password) now delete every remember-me key of the user in the same transaction as the password: each browser kept signed in, and any copy of its cookie, has to sign in with the new password. On Personal settings the browser making the change stays signed in; its own key goes with the rest, as a stolen cookie is a copy of one, and its cookie is cleared, so it asks for the new password at its next sign-in. A new password whose keys cannot be deleted is not saved: the old one still works, and Personal settings, the reset page, Account › Users and the API (500) say the change failed. A sign-in that stores the same password again under a newer hash (one an older version stored as MD5) is not a new password and keeps every remembered browser. A browser already signed in stays signed in until it signs out or its session ends +-- Fixed: Security. A new password left every remember-me cookie ("Keep me signed in") of the user signing in, so a stolen cookie went on working after its owner changed the password to lock the thief out. Changing your password on Personal settings, a reset through the lost-password link, an admin setting a password on My Account › Users, and PUT /users/{id} with user_pass (p202 user update --set-password) now delete every remember-me key of the user in the same transaction as the password: each browser kept signed in, and any copy of its cookie, has to sign in with the new password. On Personal settings the browser making the change stays signed in; its own key goes with the rest, as a stolen cookie is a copy of one, and its cookie is cleared, so it asks for the new password at its next sign-in. A new password whose keys cannot be deleted is not saved: the old one still works, and Personal settings, the reset page, Account › Users and the API (500) say the change failed. A sign-in that stores the same password again under a newer hash (one an older version stored as MD5) is not a new password and keeps every remembered browser. A browser already signed in is signed out at its next request too (see below) -- Fixed: The first page a browser opened with only its remember-me cookie ("Keep me signed in", after the browser was closed) ran without the user's roles: the page signed the browser in after the app had read the user, so the app had read nobody. A page that asks for a permission refused it (My Account › Users sent even the Super user to the account page), and the menu left out what the user's role gives. The cookie now signs the browser in before the user is read, so that first page has the user's roles, as every page after it does. A browser holding a remember-me cookie and no session is now signed in by it on any page, not only on one that asks for a sign-in; a signed-in browser reads nothing more for carrying the cookie +-- Fixed: Security. A new password ended the remember-me cookies but left every browser already signed in as the user signed in, so whoever had used a stolen cookie, or was signed in on a shared computer, kept that session after the owner changed or reset the password. Changing the password on Personal settings, a reset through the lost-password link, an admin setting it on My Account › Users, and PUT /users/{id} with user_pass (p202 user update --set-password) now sign out every browser signed in as the user at its next request, as a sign-out does; the browser that changed its own password on Personal settings stays signed in, and an admin who sets another user's password stays signed in too. Switching a user off, and removing one, do the same, so a browser that made no request while the user was switched off no longer goes on once they are switched on again. A sign-in that stores the same password again under a newer hash signs nobody out. Browsers signed in before the upgrade to 1.9.79 stay signed in through it. The upgrade adds a column to 202_users (user_session_generation, counted up by each of these changes); until it has run, nothing is compared and every signed-in browser goes on as before. A change whose sessions cannot be ended is not saved: the old password still works, and the page or the API (500) says the change failed Version 1.9.78 -- Fixed: On a new install running MariaDB, clicks that arrived at the same moment could go unrecorded (the visitor was still redirected, or a landing page showed an error). They are all recorded now: the report rollup of a single click reads that click through its index instead of scanning the click table diff --git a/docs/cli.md b/docs/cli.md index de5ca5c67..cc4179d10 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -1653,7 +1653,7 @@ p202 user update 1 --user-fname "Jane" --user-lname "Doe" p202 user delete 2 ``` -When creating a user without `--user-pass`, the CLI asks for the password without echo (twice, so a typo is caught), or reads it as one line of stdin when piped. To change a password on update, use `--set-password` (the same prompt or pipe); changing your **own** password also needs `--current-password`, read first. A new password ends every browser the user is kept signed in on ("Keep me signed in"): their remember-me keys are deleted with it. A `--user-pass` value works on both but stays in shell history. +When creating a user without `--user-pass`, the CLI asks for the password without echo (twice, so a typo is caught), or reads it as one line of stdin when piped. To change a password on update, use `--set-password` (the same prompt or pipe); changing your **own** password also needs `--current-password`, read first. A new password ends every browser the user is signed in on, those kept signed in ("Keep me signed in") included: their sessions end at their next request and their remember-me keys are deleted with it. A `--user-pass` value works on both but stays in shell history. | Flag | Required (create) | Description | |-------------------|-------------------|---------------------| diff --git a/docs/openapi.yaml b/docs/openapi.yaml index db6a6b9bf..590b9fd5b 100644 --- a/docs/openapi.yaml +++ b/docs/openapi.yaml @@ -14685,8 +14685,9 @@ components: maxLength: 72 description: >- A new password. Every remember-me key the user holds is deleted - in the same write, so a browser kept signed in has to sign in - with it; a key that cannot be deleted leaves the old password + in the same write, and every browser signed in as the user is + signed out at its next request, so each has to sign in with it; + a key or session that cannot be ended leaves the old password and answers 500. current_password: type: string diff --git a/documentation/api/14-users.md b/documentation/api/14-users.md index 7a12037bd..2931b9b60 100644 --- a/documentation/api/14-users.md +++ b/documentation/api/14-users.md @@ -73,12 +73,12 @@ API keys are masked after the first 8 characters in list responses. The full key | ----- | ---- | -------- | ----------- | | `user_name` | string | Yes | Username, 1-50 characters; another account's (in any case) is `409`. Changing it on `PUT` renames the account, as the Users page does: it needs a user you may manage, even when that is you | | `user_email` | string | Yes | Email address, at most 100 characters; another account's is `422` | -| `user_pass` | string | Yes (create) | Password, 8-72 characters (bcrypt reads only the first 72 bytes), hashed server-side. On an update, every remember-me key the user holds is deleted in the same write, so no browser kept signed in ("Keep me signed in") signs in with a cookie from before the new password; a key that cannot be deleted leaves the old password and answers `500` | +| `user_pass` | string | Yes (create) | Password, 8-72 characters (bcrypt reads only the first 72 bytes), hashed server-side. On an update, every remember-me key the user holds is deleted in the same write, so no browser kept signed in ("Keep me signed in") signs in with a cookie from before the new password, and every browser signed in as the user is signed out at its next request; a key or session that cannot be ended leaves the old password and answers `500` | | `current_password` | string | When you change your **own** `user_pass` | Your present password, as Personal settings asks for it: a key that is not the password must not be enough to take over the sign-in. Missing or wrong is `422` naming `current_password`. A Super user or Admin resetting another user's password does not send it | | `user_fname` | string | No | First name, at most 50 characters | | `user_lname` | string | No | Last name, at most 50 characters | | `user_timezone` | string | No | A PHP time zone name such as `America/New_York`, as Personal settings offers them (default: UTC); anything else is `422` | -| `user_active` | integer | No | `1` can sign in and use its API keys (default), `0` cannot: its keys answer `401`, and a browser it is signed in on is signed out at its next request. Every remember-me key the user holds is deleted in the same write, so no browser kept signed in signs back in when the user is `1` again; a key that cannot be deleted leaves the user as they were and answers `500`. Any other value is `422` rather than read as `0` | +| `user_active` | integer | No | `1` can sign in and use its API keys (default), `0` cannot: its keys answer `401`, and a browser it is signed in on is signed out at its next request, also one that makes no request until the user is `1` again. Every remember-me key the user holds is deleted in the same write, so no browser kept signed in signs back in when the user is `1` again; a key that cannot be deleted leaves the user as they were and answers `500`. Any other value is `422` rather than read as `0` | ## Preference Fields @@ -312,7 +312,9 @@ curl -X PUT https://your-domain.com/api/v3/users/1 \ echo, at a terminal or as two lines of piped stdin (current first). A new password, set here, on Personal settings, on Account › Users or through -the reset link, deletes every remember-me key of the user: a browser kept -signed in, or a copy of its cookie, has to sign in with the new password. -Personal settings keeps the browser making the change signed in. A rehash -of the same password as the user signs in is not a change and keeps them. +the reset link, deletes every remember-me key of the user and signs out every +browser signed in as them at its next request: a browser kept signed in, a +copy of its cookie, or a session someone already has, has to sign in with the +new password. Personal settings keeps the browser making the change signed +in; an admin's change keeps the admin's. A rehash of the same password as the +user signs in is not a change and ends nothing. diff --git a/tests/Auth/SessionGenerationIntegrationTest.php b/tests/Auth/SessionGenerationIntegrationTest.php index 39240a76c..31bd55b83 100644 --- a/tests/Auth/SessionGenerationIntegrationTest.php +++ b/tests/Auth/SessionGenerationIntegrationTest.php @@ -283,6 +283,43 @@ public function testAnAdminChangeOnAccountUsersEndsTheUsersSessionsNotTheAdmins( self::assertTrue(self::next($admins, self::ADMIN)[0], 'the admin\'s is not'); } + /** A sign-in after a change begins at the new generation, and stays signed in. */ + public function testASignInAfterAChangeStaysSignedIn(): void + { + (new \Api\V3\Controllers\UsersController(self::$db)) + ->update(self::USER, ['user_pass' => self::NEW_PASSWORD], self::ADMIN); + + $signIn = self::request('202-login.php', [ + 'P202_SU_SESSION' => 'anonymous', + 'P202_SU_POST' => json_encode( + ['token' => 'SESSION', 'user_name' => 'gen' . self::USER, 'user_pass' => self::NEW_PASSWORD], + JSON_THROW_ON_ERROR + ), + ]); + $r = $signIn['result']; + self::assertNull($r['uncaught'], $signIn['output']); + self::assertContains('user_id', $r['session_keys'], 'signed in'); + self::assertSame(1, $r['generation'], 'at the generation the change made'); + self::assertTrue(self::next($r['session_id'])[0], 'and still signed in on its next request'); + } + + /** + * Ending a session from before the change revokes no key: the change + * revoked the user's keys in its own transaction, and a key made since + * is a later sign-in's. + */ + public function testAnEndedSessionLeavesTheKeysOfALaterSignIn(): void + { + $old = self::browser(); + (new \Api\V3\Controllers\UsersController(self::$db)) + ->update(self::USER, ['user_pass' => self::NEW_PASSWORD], self::ADMIN); + self::row('202_auth_keys', ['user_id' => self::USER, 'auth_key' => 'laterkey', 'expires' => time() + 86400]); + + self::assertFalse(self::next($old)[0], 'the old session is ended'); + $kept = self::$db->query('SELECT COUNT(*) FROM 202_auth_keys WHERE user_id = ' . self::USER); + self::assertSame(1, (int) $kept->fetch_row()[0], 'the later sign-in\'s key is kept'); + } + /** The same password, stored again under a newer hash as the user signs in, ends nothing. */ public function testARehashAtSignInEndsNoSession(): void { diff --git a/tests/live/README.md b/tests/live/README.md index 463b2e022..3471f871d 100644 --- a/tests/live/README.md +++ b/tests/live/README.md @@ -131,6 +131,7 @@ the passes sourced it: `legacy-pixels.sh` (2 failures), `identity-graph.sh` | `session-ends-with-user.sh` | A signed-in session whose user is switched off, removed, or whose row is gone: the admin (`P202_USER`) adds a second Admin through Account › Users, signs it in with "Keep me signed in" in a jar of its own, then switches it off through the edit form, removes it through the list, and deletes its row. Each time the user's next page is sent to sign in with a new session id, the session's file (`P202_SESSION_DIR`, default `/var/lib/php/sessions`; '' skips that check) and the remember-me cookie and keys gone; Personal settings' add-a-key form, posted with its own token, mints nothing; the Overview's AJAX fragment, the update banner's `fetch()` and the chat pass answer 401; the sign-in page draws; and the admin's session is unaffected. Switched back on, the user signs in again. Writes users named `sxend…` and removes them. | | `remember-me-revoked.sh` | A remember-me cookie stops signing in once what made it is over: the admin (`P202_USER`) adds a second Admin through Account › Users and signs it in with "Keep me signed in" in jars of its own, copying each browser's cookie, and a cookie is tried as a browser holding only it opens the sign-in page (sent on to the app, or the form drawn). A copied cookie is refused once its browser signs out, its key alone deleted, while the user's other remembered browser still signs in; after the user is switched off and on again through the edit form, and again through `PUT /api/v3/users/{id}`, every cookie from before is refused and every key gone, and the user signs in again with a new cookie that works; after the user is removed and their row restored, the old cookie is refused. Writes users named `rkend…` and removes them with their keys. Needs `P202_PASS` and `P202_API_KEY`. | | `password-change-revokes.sh` | A new password ends every remember-me cookie of the user, and a rehash does not: the admin (`P202_USER`) adds a second Admin through Account › Users and signs it in with "Keep me signed in" in jars of its own, copying each browser's cookie. The user changes the password on Personal settings in one remembered browser: a copy of that browser's cookie and the other browser's are refused, every key is gone, and the changing browser stays signed in (Personal settings answers, its cookie cleared); the old password is refused and the new one signs in. The same for a reset through `202-lost-pass.php` and `202-pass-reset.php` (the key read from the database, as its email cannot be read here), for `PUT /api/v3/users/{id}` with `user_pass` and for Account › Users' edit form. Then the stored password is put back to the legacy salted MD5 of the same password, a sign-in rehashes it, and the remembered browser still signs in. Writes users named `rkpw…` and removes them with their keys. Needs `P202_PASS` and `P202_API_KEY`. | +| `password-ends-sessions.sh` | A new password ends every other browser signed in as the user: the admin (`P202_USER`) adds a second Admin through Account › Users and signs it in in browsers of its own (without "Keep me signed in", so only the session keeps them in). The user changes the password on Personal settings in one of two: that one stays signed in on the same session id, the other is sent to sign in at its next request with a new session id, and `202_users.user_session_generation` is counted up. The same for a reset through `202-lost-pass.php` and `202-pass-reset.php` (both browsers signed out; the key read from the database), `PUT /api/v3/users/{id}` with `user_pass` and Account › Users' edit form (the user's browser signed out, the admin's not); a sign-in that rehashes a legacy MD5 password signs nobody out; and a browser idle while the user is switched off and on again through `PUT` is signed out at its next request. Writes users named `rkse…` and removes them. Needs `P202_PASS` and `P202_API_KEY`. | | `remember-me-first-request.sh` | The first request a remember-me cookie signs in has the user's roles: `P202_USER` (a role with add_users) signs in with "Keep me signed in", keeps only the `remember_me` cookie (the session cookie dropped), and opens Account › Users as the first request: it is served with its add-a-user form and the chrome's User Management and Settings entries, and the browser is given a session; the next request on that session is served too, and the sign-in page sends a browser holding only the cookie on to the app. The first request has no session to carry the licence's verdict, so serve the instance with a licence the service accepts or with the service unreachable. Adds one remember-me key and removes it. Needs `P202_PASS`. | | `signin-not-completed.sh` | A sign-in whose password is right but which a database failure stops: with a trigger refusing an UPDATE of the pass's own two users (asserted to have landed), the right password past the per-user throttle limit is told each time that the sign-in could not be completed, logged as `login_success = 2` and never throttled, as is a legacy MD5 password whose rehash is refused; with the trigger gone it signs in at once; Account › Settings shows those attempts as Not completed, `GET /system/login-log` answers `status: not_completed` (`login_success` false) and `?status=` selects them, as `p202 system login-log --status` does with `P202_BIN`; then ten wrong passwords still lock the user out, the right one included. Creates its own users, key and trigger and removes them, with every log row of theirs, on any exit; touches no other user, so it may run against a shared instance (it does not source `guard.sh`). | | `dni-own-account.sh` | Account › API integrations as an account other than user 1 (`P202_USER`, whose role has access_to_api_integrations): the page lists the account's own Direct Network Integrations and not user 1's, holds no network's API key anywhere (no Reveal attribute, no poller data, no install hash) and shows a saved key by its last four; an edit holds no key, a save that leaves the key empty keeps it and one that types a key replaces it, a keep-the-key save naming user 1's network or moving to another network is refused with nothing changed, and the progress endpoint asks about the account's own importing networks only. Writes its own rows (networkId `yy12-*`) directly, since a test instance cannot reach the hosted DNI service, and removes them; the saves authenticate against licence-stub.sh, which answers every DNI call. Needs `P202_PASS`. | From 1a0e95cebcbda3c213ab1e5ff2b6baf4fdad031e Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 28 Oct 2026 16:15:43 +0000 Subject: [PATCH 3/4] 1.9.79: read the three request values 1.9.79's own merges left cast 1.9.79's PHPStan job has been red since the merges at 15:46Z. #308 extended ForbidNumericCastOfRequestParamRule to the pages and baselined the casts already there. Three PRs merged beside it each added a cast that its baseline does not count, and each PR passed on its own: - Personal settings (#283) cast the same-IP fallback after checking it is a key of its choices. It now stores 1 for '1' and 0 for '0', the two keys the check admits. - Setup > Offline Conversions (#293) cast a feed id after a regex test. It now reads it with PostedId::read(). - Setup > Traffic Sources (#271) cast the edited account's id to read its stored postbacks. The page's own ownership check compares the raw value in SQL, where MySQL reads '12abc' as 12. So a save from ?edit_ppc_account_id=12abc changed account 12 (one of the account's own, not the one named) and held that account's postbacks. Measured on a live instance: the old page saved the tampered edit onto account 1. The id is now read exactly, and anything else is refused in the page's words before anything is read or written. The baseline counts are unchanged: each file now holds only the casts they counted. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01VRoS2SViuxaucFaKWsFnLr --- 202-account/account.php | 2 +- 202-config/changelogs.json | 3 ++- changelogs.txt | 1 + tracking202/setup/offline_conversions.php | 2 +- tracking202/setup/ppc_accounts.php | 8 ++++++-- 5 files changed, 11 insertions(+), 5 deletions(-) diff --git a/202-account/account.php b/202-account/account.php index 30062fe59..f1a1c059f 100755 --- a/202-account/account.php +++ b/202-account/account.php @@ -332,7 +332,7 @@ 'user_pref_dynamic_bid' => (int)$_POST['user_bid'], 'user_pref_ad_settings' => (string)$_POST['user_pref_ad_settings'], 'user_pref_privacy' => (string)$_POST['user_pref_privacy'], - 'user_pref_same_ip_fallback' => (int)$_POST['user_pref_same_ip_fallback'], + 'user_pref_same_ip_fallback' => (string)$_POST['user_pref_same_ip_fallback'] === '1' ? 1 : 0, 'user_daily_email' => (string)$_POST['user_daily_email'], ]; diff --git a/202-config/changelogs.json b/202-config/changelogs.json index bd92529f9..eff62e814 100644 --- a/202-config/changelogs.json +++ b/202-config/changelogs.json @@ -74,7 +74,8 @@ "Fixed: Security. Signing out left the browser's remember-me key valid, so a copy of its cookie still signed in for up to 14 days, and switching a user off left all of theirs valid, so a remembered browser signed back in once the user was switched on again. Signing out now deletes that browser's key (the user's other remembered browsers stay signed in), and switching a user off in My Account, Users or through the API deletes every one of theirs", "Fixed: A sync writes a redirector's rules on the other install whole or not at all. When the other install refused one (since this version, a rule with no criterion), its redirector was left with its own rules removed and only the synced rules before the refused one", "Fixed: Security. Changing a password left every browser kept signed in able to sign in again by its cookie, a stolen copy included. A new password (Personal settings, the reset link, My Account, Users, or the API) now ends them all; the browser making the change on Personal settings stays signed in. A browser already signed in stays signed in until it signs out or its session ends", - "Fixed: The first page opened with only a remember-me cookie (Keep me signed in) ran without the user's roles, so a page such as My Account, Users sent the user away. It has them now" + "Fixed: The first page opened with only a remember-me cookie (Keep me signed in) ran without the user's roles, so a page such as My Account, Users sent the user away. It has them now", + "Fixed: Setup › Traffic Sources refuses an account edit whose link names the account with more than its id (12abc), where it saved the change to account 12" ] }, "1.9.78": { diff --git a/changelogs.txt b/changelogs.txt index 270bc4228..82d6002e3 100755 --- a/changelogs.txt +++ b/changelogs.txt @@ -125,6 +125,7 @@ Version 1.9.79 -- Fixed: A sync (p202 sync and re-sync, the sync jobs) writes a redirector's rules on the target as one set (PUT /rotators/{id}/rules), which the target takes whole or refuses with nothing written. It removed the target redirector's rules and then posted the source's one at a time, so a rule the target refused stopped it with the target holding only the rules before that one: on a re-sync the target's own rules were gone. Since this version refuses a rule with no criterion, a source rule stored without one, which had synced, did exactly that. The refusal names the source rule (give it a criterion; the value ALL keeps it on every visitor). A target from before 1.9.79, which has no rule set route, is written to one rule at a time as before -- Fixed: Security. A new password left every remember-me cookie ("Keep me signed in") of the user signing in, so a stolen cookie went on working after its owner changed the password to lock the thief out. Changing your password on Personal settings, a reset through the lost-password link, an admin setting a password on My Account › Users, and PUT /users/{id} with user_pass (p202 user update --set-password) now delete every remember-me key of the user in the same transaction as the password: each browser kept signed in, and any copy of its cookie, has to sign in with the new password. On Personal settings the browser making the change stays signed in; its own key goes with the rest, as a stolen cookie is a copy of one, and its cookie is cleared, so it asks for the new password at its next sign-in. A new password whose keys cannot be deleted is not saved: the old one still works, and Personal settings, the reset page, Account › Users and the API (500) say the change failed. A sign-in that stores the same password again under a newer hash (one an older version stored as MD5) is not a new password and keeps every remembered browser. A browser already signed in stays signed in until it signs out or its session ends -- Fixed: The first page a browser opened with only its remember-me cookie ("Keep me signed in", after the browser was closed) ran without the user's roles: the page signed the browser in after the app had read the user, so the app had read nobody. A page that asks for a permission refused it (My Account › Users sent even the Super user to the account page), and the menu left out what the user's role gives. The cookie now signs the browser in before the user is read, so that first page has the user's roles, as every page after it does. A browser holding a remember-me cookie and no session is now signed in by it on any page, not only on one that asks for a sign-in; a signed-in browser reads nothing more for carrying the cookie +-- Fixed: Setup › Traffic Sources saved an account edit on whatever account the link's id began with: a save from ?edit_ppc_account_id=12abc (a link edited by hand or built by a script) changed account 12, one of the account's own and not the one named, and took its stored postbacks as the edited account's. The id is read exactly now; anything else is refused with "Pick the traffic source account to edit from the list." and nothing is changed Version 1.9.78 -- Fixed: On a new install running MariaDB, clicks that arrived at the same moment could go unrecorded (the visitor was still redirected, or a landing page showed an error). They are all recorded now: the report rollup of a single click reads that click through its index instead of scanning the click table diff --git a/tracking202/setup/offline_conversions.php b/tracking202/setup/offline_conversions.php index 11b8150c1..0e0085105 100644 --- a/tracking202/setup/offline_conversions.php +++ b/tracking202/setup/offline_conversions.php @@ -128,7 +128,7 @@ } } } elseif ($action === 'replace_key' || $action === 'delete_feed') { - $feedId = preg_match('/\A[1-9][0-9]{0,9}\z/D', (string) ($_POST['feed_id'] ?? '')) === 1 ? (int) $_POST['feed_id'] : 0; + $feedId = \Prosper202\Http\PostedId::read($_POST['feed_id'] ?? null) ?? 0; if ($action === 'replace_key') { $key = $feedId > 0 ? $feedStore->replaceKey($userId, $feedId, $now) : null; if ($key !== null) { diff --git a/tracking202/setup/ppc_accounts.php b/tracking202/setup/ppc_accounts.php index eebf941fc..9ecd5e298 100644 --- a/tracking202/setup/ppc_accounts.php +++ b/tracking202/setup/ppc_accounts.php @@ -177,9 +177,13 @@ static function () use ($sourceConn, $sourceId, $newName, $ownerId): ?string { // A Postback URL the server would refuse to call is refused here, as // the API refuses it; what the account's pixels hold now is taken as // it is. - $heldPostbacks = $editing + $editAccountKey = $editing ? \Prosper202\Http\PostedId::read($_GET['edit_ppc_account_id'] ?? null) : null; + if ($editing && $editAccountKey === null) { + $error['wrong_user'] = 'Pick the traffic source account to edit from the list.'; + } + $heldPostbacks = $editAccountKey !== null ? (new \Prosper202\Setup\AccountPixels(new \Prosper202\Database\Connection($db))) - ->storedPostbacks((int) $_SESSION['user_id'], (int) $_GET['edit_ppc_account_id']) + ->storedPostbacks((int) $_SESSION['user_id'], $editAccountKey) : []; \Prosper202\Setup\AccountPixels::refusePostbacks($pixelRows, $heldPostbacks, $error); From 9a2553f302f1e0a5f0ca6e4e477433793bf2f45f Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 29 Oct 2026 02:39:38 +0000 Subject: [PATCH 4/4] 1.9.79: changelogs.json is JSON again, and no tracked file may hold a conflict marker 81c601fa (#327's squash) took a merge an agent left unresolved: 202-config/changelogs.json held <<<<<<<, ======= and >>>>>>> around two 1.9.79 entries, so the file the in-app release notes read stopped being JSON. Both entries are kept, 1.9.79's first, as each side wrote it. Nothing CI runs would have refused it: PHP lint reads PHP, and no test decoded the file. NoMergeConflictMarkersTest holds every tracked text file to having no conflict-marker line, and every tracked JSON file to parsing. With the marked file put back, both tests fail on it; the tree is otherwise clean. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01VRoS2SViuxaucFaKWsFnLr --- 202-config/changelogs.json | 5 +- tests/Release/NoMergeConflictMarkersTest.php | 83 ++++++++++++++++++++ 2 files changed, 84 insertions(+), 4 deletions(-) create mode 100644 tests/Release/NoMergeConflictMarkersTest.php diff --git a/202-config/changelogs.json b/202-config/changelogs.json index d3fad95a3..4095333a9 100644 --- a/202-config/changelogs.json +++ b/202-config/changelogs.json @@ -76,11 +76,8 @@ "Fixed: Security. Changing a password left every browser kept signed in able to sign in again by its cookie, a stolen copy included. A new password (Personal settings, the reset link, My Account, Users, or the API) now ends them all; the browser making the change on Personal settings stays signed in. A browser already signed in stays signed in until it signs out or its session ends", "Fixed: The first page opened with only a remember-me cookie (Keep me signed in) ran without the user's roles, so a page such as My Account, Users sent the user away. It has them now", "Fixed: Setup › Traffic Sources refuses an account edit whose link names the account with more than its id (12abc), where it saved the change to account 12", -<<<<<<< HEAD + "Fixed: p202 report losers and winners with --aff-campaign-id priced every click at a payout of 0 when they could not read the campaign's payout (a server error, a refused key, an unreachable server); they now fail with the error and what to do next. An answer with no payout in it is an error in every command that reads one, and a campaign whose payout is 0 is still priced at 0", "Fixed: The Redirector page no longer drops every destination but the first of a rule made through the API with several destinations and Split test off when you save; it shows the rule as the split test the redirect links make of it, with each destination's weight" -======= - "Fixed: p202 report losers and winners with --aff-campaign-id priced every click at a payout of 0 when they could not read the campaign's payout (a server error, a refused key, an unreachable server); they now fail with the error and what to do next. An answer with no payout in it is an error in every command that reads one, and a campaign whose payout is 0 is still priced at 0" ->>>>>>> origin/1.9.79 ] }, "1.9.78": { diff --git a/tests/Release/NoMergeConflictMarkersTest.php b/tests/Release/NoMergeConflictMarkersTest.php new file mode 100644 index 000000000..c059c71a2 --- /dev/null +++ b/tests/Release/NoMergeConflictMarkersTest.php @@ -0,0 +1,83 @@ +{7}|\|{7})(?: |$)|^={7}$/m'; + + public function testNoTrackedFileHoldsAConflictMarker(): void + { + $found = []; + foreach (self::trackedFiles() as $path) { + $source = self::read($path); + if ($source === null) { + continue; + } + if (SourceScan::countMatches(self::MARKER, $source, $path, $matches) > 0) { + $found[] = $path . ': ' . implode(' | ', array_unique($matches[0])); + } + } + self::assertSame([], $found, 'A tracked file holds a merge conflict marker: a merge was committed unresolved.'); + } + + public function testEveryTrackedJsonFileParses(): void + { + $broken = []; + $read = 0; + foreach (self::trackedFiles() as $path) { + if (!str_ends_with($path, '.json')) { + continue; + } + $read++; + try { + json_decode((string) self::read($path), true, 512, JSON_THROW_ON_ERROR); + } catch (\JsonException $e) { + $broken[] = $path . ': ' . $e->getMessage(); + } + } + self::assertGreaterThan(0, $read, 'no tracked JSON file was read'); + self::assertSame([], $broken, 'A tracked JSON file does not parse.'); + } + + /** @return list */ + private static function trackedFiles(): array + { + $root = SourceScan::repoRoot(); + if (!is_dir($root . '/.git') && !is_file($root . '/.git')) { + self::markTestSkipped('not a git checkout'); + } + $out = shell_exec('git -C ' . escapeshellarg($root) . ' ls-files -z'); + self::assertIsString($out, 'git ls-files did not run'); + $files = array_values(array_filter(explode("\0", $out), static fn(string $f): bool => $f !== '')); + self::assertGreaterThan(100, count($files), 'git ls-files listed almost nothing'); + + return $files; + } + + /** A tracked text file's content, or null for a binary, a deleted file or a symbolic link. */ + private static function read(string $path): ?string + { + $full = SourceScan::repoRoot() . '/' . $path; + if (is_link($full) || !is_file($full)) { + return null; + } + $source = file_get_contents($full); + self::assertIsString($source, 'could not read ' . $path); + + return str_contains(substr($source, 0, 8000), "\0") ? null : $source; + } +}