diff --git a/display.php b/display.php index a07de23..2515583 100644 --- a/display.php +++ b/display.php @@ -7,7 +7,7 @@ if ($queue) session_cache_limiter('nocache'); else session_cache_limiter('none'); session_set_cookie_params(array('lifetime' => 0, 'path' => '/', 'domain' => cookie_domain(), 'secure' => TRUE, 'httponly' => TRUE, 'samesite' => 'Lax')); session_start(); -if (isset($_SESSION['editor'])) { +if (isset($_SESSION['editor']) && editor_exists($_SESSION['editor'])) { $is_admin = TRUE; $username = $_SESSION['editor']; } else { diff --git a/open_db.lib.php b/open_db.lib.php index ce7ddbd..6b3d615 100644 --- a/open_db.lib.php +++ b/open_db.lib.php @@ -36,6 +36,22 @@ function open_db($state) { } } +/* + * TRUE if an editor account named $username exists. A session or a + * persistent login of an editor whose account was removed ends with it. + */ +function editor_exists($username) { + global $db; + if (!is_string($username) || $username === '') + return FALSE; + $opened = open_db('open'); + $result = $db->execute_query("SELECT username FROM editors WHERE username = ?", array($username)) or db_fail(); + $row = $result->fetch_assoc(); + if ($opened) + open_db('close'); + return (bool)$row; +} + /* * Stop the page after a database error. The details, with the file and * line of the failed call, go to the PHP error log; the visitor only diff --git a/publish/addeditor.php b/publish/addeditor.php index 3520cac..05c2e21 100644 --- a/publish/addeditor.php +++ b/publish/addeditor.php @@ -6,19 +6,22 @@ $added = FALSE; $csrf_failed = FALSE; $hash_failed = FALSE; $exists = FALSE; +$invalid = array(); if (!isset($_SESSION['editor'])) { header("Location: /publish/login"); return; } else if (isset($_POST['adduser']) && !csrf_valid()) { $csrf_failed = TRUE; +} else if (isset($_POST['adduser']) && ($invalid = new_editor_problems($_POST['username'] ?? NULL, $_POST['password'] ?? NULL))) { + // Nothing is stored; the page lists the problems } else if (isset($_POST['adduser'])) { require_once('../open_db.lib.php'); open_db('open'); $table = "editors"; - $username = (string)($_POST['username'] ?? ''); - $password = password_hash((string)($_POST['password'] ?? ''), password_algorithm()); - $email = (string)($_POST['email'] ?? ''); + $username = $_POST['username']; + $password = password_hash($_POST['password'], password_algorithm()); + $email = is_string($_POST['email'] ?? NULL) ? $_POST['email'] : ''; $found = $db->execute_query("SELECT username FROM $table WHERE username = ?", array($username)); $query = "INSERT INTO $table SET `username` = ?, `password` = ?, `email` = ?"; if ($found && $found->fetch_assoc()) { @@ -28,13 +31,33 @@ if (!isset($_SESSION['editor'])) { if (password_hash_is_stored($username, $password)) { $added = TRUE; } else { - $db->execute_query("DELETE FROM $table WHERE username = ?", array($username)); + remove_editor($username); $hash_failed = TRUE; } $is_admin = TRUE; } open_db('close'); } + +/* + * The problems with a new username and password, as messages; an empty + * list means that both are fine. The persistent login cookie holds + * "username|key", so a username cannot contain "|". + */ +function new_editor_problems($username, $password) { + $problems = array(); + if (!is_string($username) || trim($username) === '') + $problems[] = 'Please enter a username.'; + else if ($username !== trim($username) || strlen($username) > 64 || preg_match('/[|\p{Cc}]/u', $username) !== 0) + $problems[] = 'A username has at most 64 characters, with no spaces at the ends and no "|".'; + if (!is_string($password) || mb_strlen($password, 'UTF-8') < 12) + $problems[] = 'A password needs at least 12 characters.'; + else if (strlen($password) > 1024) + $problems[] = 'A password has at most 1024 characters.'; + else if (is_string($username) && strcasecmp($password, $username) === 0) + $problems[] = 'The password cannot be the username.'; + return $problems; +} ?> @@ -67,6 +90,9 @@ if (!isset($_SESSION['editor'])) {

An editor with that username already exists.

+ +

+

The editor was not added: the editors.password column is too short for the password hash. Widen it to VARCHAR(255).

@@ -77,12 +103,12 @@ if (!isset($_SESSION['editor'])) {
- +
- +
diff --git a/session.lib.php b/session.lib.php index 3448949..2311bd2 100644 --- a/session.lib.php +++ b/session.lib.php @@ -10,6 +10,10 @@ if (isset($_SERVER['HTTPS'])) { session_set_cookie_params(array('lifetime' => 0, 'path' => '/', 'domain' => cookie_domain(), 'secure' => TRUE, 'httponly' => TRUE, 'samesite' => 'Lax')); session_start(); + // An editor whose account was removed is logged out + if (isset($_SESSION['editor']) && !editor_exists($_SESSION['editor'])) + unset($_SESSION['editor']); + if (isset($_SESSION['editor'])) { $is_admin = TRUE; $username = $_SESSION['editor']; @@ -29,7 +33,8 @@ if (isset($_SERVER['HTTPS'])) { /* * Persistent logins: the cookie holds "username|key", and the database * holds only a SHA-256 of the key, so a copy of the table cannot be used - * to log in. Each device has its own row. + * to log in. Each device has its own row. A key works once, for a year + * after it was made, and only while the editor account exists. */ function persistent_cookie() { $cookie_data = $_COOKIE['PersistentAuth'] ?? ''; @@ -54,7 +59,8 @@ function check_persistent_login() { $key_hash = persistent_key_hash($key); open_db('open'); - $query = "SELECT * FROM $table WHERE username = ? AND session = ? LIMIT 1"; + $query = "SELECT p.username, p.session FROM $table p JOIN editors e ON e.username = p.username" . + " WHERE p.username = ? AND p.session = ? AND p.created > DATE_SUB(now(), INTERVAL 1 YEAR) LIMIT 1"; $getid = $db->execute_query($query, array($username, $key_hash)) or db_fail(); $row = $getid->fetch_assoc(); open_db('close'); @@ -66,9 +72,24 @@ function check_persistent_login() { create_persistent_login($username); return true; } + // A wrong, expired or revoked key: remove it and its cookie + delete_persistent_login(); return false; } +/* + * Remove an editor account, with its persistent logins. Its sessions end + * at their next request, when editor_exists() fails. + */ +function remove_editor($username) { + global $db; + $opened = open_db('open'); + $db->execute_query("DELETE FROM persistent_sessions WHERE username = ?", array($username)) or db_fail(); + $db->execute_query("DELETE FROM editors WHERE username = ?", array($username)) or db_fail(); + if ($opened) + open_db('close'); +} + function create_persistent_login($username) { $validity = strtotime('+1 year'); // Time to keep the session valid global $db, $is_admin; diff --git a/tests/php/Pages/AddEditorTest.php b/tests/php/Pages/AddEditorTest.php index b2c3832..3de5bd5 100644 --- a/tests/php/Pages/AddEditorTest.php +++ b/tests/php/Pages/AddEditorTest.php @@ -1,6 +1,7 @@ post('/publish/addeditor', $fields + array('adduser' => '1', 'username' => 'newbie', 'password' => 'pass word', + return $this->post('/publish/addeditor', $fields + array('adduser' => '1', 'username' => 'newbie', 'password' => 'a long pass phrase', 'email' => 'new@example.org', 'csrf_token' => $this->token)); } @@ -38,7 +39,7 @@ class AddEditorTest extends PageTestCase { $this->assertStringContainsString('

Added editor newbie

', $answer['body']); $row = $this->tables()['editors'][1]; $this->assertSame(array('newbie', 'new@example.org'), array($row['username'], $row['email'])); - $this->assertTrue(password_verify('pass word', $row['password'])); + $this->assertTrue(password_verify('a long pass phrase', $row['password'])); } public function testTheTokenIsChecked(): void { @@ -54,10 +55,41 @@ class AddEditorTest extends PageTestCase { } public function testAHashThatDoesNotFitIsNotKept(): void { - $this->setTables(array('editors' => array()), 40); + $this->setTables(array('editors' => array(array('username' => 'ed', 'password' => 'x', 'email' => ''))), 40); $answer = $this->add(); $this->assertStringContainsString('the editors.password column is too short', $answer['body']); - $this->assertSame(array(), $this->tables()['editors']); + $this->assertSame(array('ed'), array_column($this->tables()['editors'], 'username')); + $this->assertCount(1, $this->queries('/^DELETE FROM persistent_sessions WHERE username = \?$/')); + } + + public static function badAccounts(): array { + return array( + 'no username' => array(array('username' => ''), 'Please enter a username.'), + 'a blank username' => array(array('username' => ' '), 'Please enter a username.'), + 'an array username' => array(array('username' => array('x')), 'Please enter a username.'), + 'spaces at the ends' => array(array('username' => ' newbie'), 'no spaces at the ends'), + 'a bar' => array(array('username' => 'new|bie'), 'no spaces at the ends'), + 'a long username' => array(array('username' => str_repeat('n', 65)), 'at most 64 characters'), + 'no password' => array(array('password' => ''), 'A password needs at least 12 characters.'), + 'a short password' => array(array('password' => 'eleven char'), 'A password needs at least 12 characters.'), + 'an array password' => array(array('password' => array('a long pass phrase')), 'A password needs at least 12 characters.'), + 'a huge password' => array(array('password' => str_repeat('p', 1025)), 'A password has at most 1024 characters.'), + 'the username' => array(array('username' => 'newbie-editor', 'password' => 'NEWBIE-EDITOR'), 'The password cannot be the username.'), + ); + } + + #[DataProvider('badAccounts')] + public function testABadAccountIsRefused(array $fields, string $message): void { + $answer = $this->add($fields); + $this->assertSame(200, $answer['status']); + $this->assertStringContainsString(htmlspecialchars($message, ENT_QUOTES), $answer['body']); + $this->assertStringNotContainsString('Added editor', $answer['body']); + $this->assertCount(1, $this->tables()['editors']); + $this->assertCount(0, $this->queries('/^INSERT INTO editors/')); + } + + public function testTwelveCharactersAreEnough(): void { + $this->assertStringContainsString('Added editor newbie', $this->add(array('password' => 'twelve chars'))['body']); } public function testTheUsernameIsEscaped(): void { diff --git a/tests/php/Pages/FeedAndImageTest.php b/tests/php/Pages/FeedAndImageTest.php index 5150726..49d67e2 100644 --- a/tests/php/Pages/FeedAndImageTest.php +++ b/tests/php/Pages/FeedAndImageTest.php @@ -68,6 +68,12 @@ class FeedAndImageTest extends PageTestCase { $this->assertSame(200, $this->get('/display?id=3')['status']); } + public function testARemovedEditorSeesNoHiddenImages(): void { + $this->loginAs(); + $this->setTables(array('xxx' => $this->tables()['xxx'])); + $this->assertSame(404, $this->get('/display?id=3')['status']); + } + public function testQueuedImagesNeedPermission(): void { $this->assertSame(401, $this->get('/display?id=9&queue=1')['status']); $this->startSession(array('allow_queue_image' => '8')); diff --git a/tests/php/Pages/LoginTest.php b/tests/php/Pages/LoginTest.php index 19065be..d955685 100644 --- a/tests/php/Pages/LoginTest.php +++ b/tests/php/Pages/LoginTest.php @@ -181,6 +181,34 @@ class LoginTest extends PageTestCase { $this->assertStringNotContainsString('/publish/logout', $body); } + public function testAnExpiredPersistentKeyDoesNotLogIn(): void { + $this->setTables(array('editors' => $this->tables()['editors'], 'persistent_sessions' => array( + array('username' => 'ed', 'session' => hash('sha256', 'key'), 'created' => gmdate('Y-m-d H:i:s', strtotime('-1 year -1 day')))))); + $this->cookies['PersistentAuth'] = 'ed|key'; + $body = $this->get('/')['body']; + $this->assertStringNotContainsString('/publish/logout', $body); + $this->assertArrayNotHasKey('editor', $this->session()); + // The expired key and its cookie are removed + $this->assertArrayNotHasKey('PersistentAuth', $this->cookies); + $this->assertSame(array(), $this->tables()['persistent_sessions']); + } + + public function testAPersistentKeyOfARemovedAccountDoesNotLogIn(): void { + $this->setTables(array('persistent_sessions' => array( + array('username' => 'ed', 'session' => hash('sha256', 'key'), 'created' => gmdate('Y-m-d H:i:s'))))); + $this->cookies['PersistentAuth'] = 'ed|key'; + $this->assertStringNotContainsString('/publish/logout', $this->get('/')['body']); + $this->assertArrayNotHasKey('editor', $this->session()); + $this->assertArrayNotHasKey('PersistentAuth', $this->cookies); + } + + public function testARemovedAccountEndsItsSession(): void { + $this->loginAs(); + $this->setTables(array()); + $this->assertRedirect('/publish/login', $this->get('/publish/edit')); + $this->assertArrayNotHasKey('editor', $this->session()); + } + public function testLogoutNeedsTheToken(): void { $this->loginAs(); $this->assertRedirect('/', $this->get('/publish/logout?token=wrong')); diff --git a/tests/php/Support/PageTestCase.php b/tests/php/Support/PageTestCase.php index 9d3dfdb..0a72088 100644 --- a/tests/php/Support/PageTestCase.php +++ b/tests/php/Support/PageTestCase.php @@ -124,8 +124,17 @@ abstract class PageTestCase extends \PHPUnit\Framework\TestCase { return $id; } - // An editor's session, with a CSRF token; returns the token + /* + * An editor's session, with a CSRF token; returns the token. The pages + * log out an editor without an account, so this adds the account to the + * editors table if it is missing. + */ protected function loginAs(string $username = 'ed', array $data = array()): string { + $saved = unserialize(file_get_contents(self::$state . '/tables.ser')); + if (!in_array($username, array_column($saved['tables']['editors'], 'username'), TRUE)) { + $saved['tables']['editors'][] = array('username' => $username, 'password' => '', 'email' => ''); + file_put_contents(self::$state . '/tables.ser', serialize($saved)); + } $token = str_repeat('c0ffee', 10) . 'c0de'; $this->startSession($data + array('editor' => $username, 'csrf_token' => $token)); return $token; diff --git a/tests/php/Support/SiteData.php b/tests/php/Support/SiteData.php index c08e753..fb288d1 100644 --- a/tests/php/Support/SiteData.php +++ b/tests/php/Support/SiteData.php @@ -75,6 +75,22 @@ class SiteData { return $row['username'] === $params[0] && $row['session'] === $params[1]; })), 0, 1); }); + FakeDb::on('/^SELECT p\.username, p\.session FROM persistent_sessions p JOIN editors e ON e\.username = p\.username\s+WHERE p\.username = \? AND p\.session = \? AND p\.created > DATE_SUB\(now\(\), INTERVAL 1 YEAR\)/', function ($sql, $params) { + $editors = array_column(self::$tables['editors'], 'username'); + $limit = gmdate('Y-m-d H:i:s', strtotime('-1 year')); + return array_slice(array_values(array_map(function ($row) { + return array('username' => $row['username'], 'session' => $row['session']); + }, array_filter(self::$tables['persistent_sessions'], function ($row) use ($params, $editors, $limit) { + return $row['username'] === $params[0] && $row['session'] === $params[1] + && in_array($row['username'], $editors, TRUE) && ($row['created'] ?? $limit) > $limit; + }))), 0, 1); + }); + FakeDb::on('/^DELETE FROM persistent_sessions WHERE username = \?$/', function ($sql, $params) { + self::$tables['persistent_sessions'] = array_values(array_filter(self::$tables['persistent_sessions'], function ($row) use ($params) { + return $row['username'] !== $params[0]; + })); + return TRUE; + }); FakeDb::on('/^INSERT INTO persistent_sessions \(username, session\) VALUES \(\?, \?\)/', function ($sql, $params) { self::$tables['persistent_sessions'][] = array('username' => $params[0], 'session' => $params[1], 'created' => gmdate('Y-m-d H:i:s')); return TRUE; diff --git a/tests/php/Unit/SessionTest.php b/tests/php/Unit/SessionTest.php index ecffa65..2483640 100644 --- a/tests/php/Unit/SessionTest.php +++ b/tests/php/Unit/SessionTest.php @@ -127,6 +127,7 @@ class SessionTest extends TestCase { } public function testCheckPersistentLoginLogsInAndSwapsTheKey(): void { + SiteData::$tables['editors'][] = array('username' => 'ed', 'password' => '', 'email' => ''); session_start(); $before = session_id(); SiteData::$tables['persistent_sessions'][] = array('username' => 'ed', 'session' => persistent_key_hash('key'), 'created' => gmdate('Y-m-d H:i:s')); @@ -139,6 +140,52 @@ class SessionTest extends TestCase { $this->assertNotSame(persistent_key_hash('key'), $rows[0]['session'], 'the used key is gone'); } + public function testCheckPersistentLoginRefusesAnExpiredKey(): void { + SiteData::$tables['editors'][] = array('username' => 'ed', 'password' => '', 'email' => ''); + SiteData::$tables['persistent_sessions'][] = array('username' => 'ed', 'session' => persistent_key_hash('key'), + 'created' => gmdate('Y-m-d H:i:s', strtotime('-1 year -1 day'))); + $_COOKIE['PersistentAuth'] = 'ed|key'; + $this->assertFalse(check_persistent_login()); + $this->assertArrayNotHasKey('editor', $_SESSION ?? array()); + $this->assertSame(array(), SiteData::$tables['persistent_sessions']); + } + + public function testCheckPersistentLoginRefusesAKeyWithoutAnAccount(): void { + SiteData::$tables['persistent_sessions'][] = array('username' => 'ed', 'session' => persistent_key_hash('key'), 'created' => gmdate('Y-m-d H:i:s')); + $_COOKIE['PersistentAuth'] = 'ed|key'; + $this->assertFalse(check_persistent_login()); + $this->assertSame(array(), SiteData::$tables['persistent_sessions']); + } + + public function testEditorExists(): void { + SiteData::$tables['editors'][] = array('username' => 'ed', 'password' => '', 'email' => ''); + $this->assertTrue(editor_exists('ed')); + $this->assertFalse(editor_exists('ann')); + $this->assertFalse(editor_exists('')); + $this->assertFalse(editor_exists(array('ed'))); + $this->assertNull($GLOBALS['db'], 'the connection is closed again'); + } + + public function testRemoveEditorRevokesItsPersistentLogins(): void { + SiteData::$tables['editors'] = array(array('username' => 'ed', 'password' => '', 'email' => ''), + array('username' => 'ann', 'password' => '', 'email' => '')); + SiteData::$tables['persistent_sessions'] = array( + array('username' => 'ed', 'session' => persistent_key_hash('one'), 'created' => gmdate('Y-m-d H:i:s')), + array('username' => 'ed', 'session' => persistent_key_hash('two'), 'created' => gmdate('Y-m-d H:i:s')), + array('username' => 'ann', 'session' => persistent_key_hash('three'), 'created' => gmdate('Y-m-d H:i:s'))); + remove_editor('ed'); + $this->assertSame(array('ann'), array_column(SiteData::$tables['editors'], 'username')); + $this->assertSame(array('ann'), array_column(SiteData::$tables['persistent_sessions'], 'username')); + } + + public function testARemovedEditorIsLoggedOutOverHttps(): void { + $code = 'Xxx\Tests\Support\SiteData::install(); $_SERVER["HTTPS"] = "on"; session_id("abc123"); session_start(); $_SESSION["editor"] = "ed"; session_write_close();' + . ' $_COOKIE[session_name()] = "abc123"; include XXX_ROOT . "/session.lib.php";' + . ' echo json_encode(array($is_admin, $_SESSION["editor"] ?? NULL));'; + $result = PhpProcess::runBare($code, array(), array('session.save_path' => $this->tmp, 'session.use_cookies' => '0', 'session.cache_limiter' => '')); + $this->assertSame('[false,null]', $result['stdout'], $result['stderr']); + } + public function testDeletePersistentLoginRemovesThisKeyAndOldKeys(): void { SiteData::$tables['persistent_sessions'] = array( array('username' => 'ed', 'session' => persistent_key_hash('mine'), 'created' => gmdate('Y-m-d H:i:s')), @@ -160,7 +207,7 @@ class SessionTest extends TestCase { // The top of session.lib.php runs on every page; it needs HTTPS and a session public function testSessionStartsOnlyOverHttps(): void { - $code = '$_SERVER["HTTPS"] = %s; session_id("abc123"); session_start(); $_SESSION["editor"] = "ed"; session_write_close();' + $code = 'Xxx\Tests\Support\SiteData::install(array("editors" => array(array("username" => "ed")))); $_SERVER["HTTPS"] = %s; session_id("abc123"); session_start(); $_SESSION["editor"] = "ed"; session_write_close();' . ' $_COOKIE[session_name()] = "abc123"; include XXX_ROOT . "/session.lib.php";' . ' echo json_encode(array($is_admin, $username ?? NULL, session_status() === PHP_SESSION_ACTIVE));'; $ini = array('session.save_path' => $this->tmp, 'session.use_cookies' => '0', 'session.cache_limiter' => ''); @@ -176,7 +223,7 @@ class SessionTest extends TestCase { public function testPersistentCookieLogsInOverHttps(): void { $store = $this->tmp . '/tables.json'; - file_put_contents($store, json_encode(array('persistent_sessions' => array( + file_put_contents($store, json_encode(array('editors' => array(array('username' => 'ed')), 'persistent_sessions' => array( array('username' => 'ed', 'session' => persistent_key_hash('key'), 'created' => gmdate('Y-m-d H:i:s')))))); $code = 'Xxx\Tests\Support\SiteData::install(json_decode(file_get_contents(' . var_export($store, TRUE) . '), TRUE));' . ' $_SERVER["HTTPS"] = "on"; $_COOKIE["PersistentAuth"] = "ed|key"; include XXX_ROOT . "/session.lib.php";'