From cab1f8ec807df293e5cc175abd37efe69cac3628 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 6 Nov 2026 11:40:52 +0000 Subject: [PATCH] Validate records before storing them and compare ckeys after normalisation Any POST with a token used to be stored as sent, including empty fields, Discord mentions and a second spelling of an already registered key (alice / Al_ice). Civilizationbot's read-only consumers reject the entire hosted list when one record has a Discord id that is not 17 to 20 digits, an empty key, or two keys that collide after normalisation, so a single bad write could take verification down for every desktop instance. Inserts now return 400 Bad Request for a malformed Discord id or key, keys are stored in normalised ckey form (the same rule as the bot's sanitizeInput), and the duplicate check compares existing records after the same normalisation. SS14 names keep their spelling but must be printable and bounded. Deletion and token handling are unchanged. The README now documents the fields, record shape and status codes the bot depends on. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_016WTP2S4qKEFT8a6a6yFEL4 --- README.md | 47 +++++++ .../Endpoints/SS14VerifiedEndpoint.php | 26 +++- .../Endpoints/VerifiedEndpoint.php | 68 ++++++++-- tests/isolated/RecordValidationTest.php | 118 ++++++++++++++++++ tests/isolated/VerificationContractTest.php | 36 +++--- 5 files changed, 266 insertions(+), 29 deletions(-) create mode 100644 tests/isolated/RecordValidationTest.php diff --git a/README.md b/README.md index 43466da..336b576 100644 --- a/README.md +++ b/README.md @@ -68,6 +68,53 @@ Verifier-Server is a web server designed to handle BYOND user verification. - `GET /verified`: Retrieve the list of verified users. - `POST /verified`: Add or delete a verified user. Requires a valid token. +- `GET /ss14verified` and `POST /ss14verified`: the same contract for Space Station 14 + accounts, with an `ss14` field instead of `ckey`. + +A trailing slash is accepted (`/verified/`), and `/` is an alias of `/verified`. + +## HTTP contract + +This is the contract [Civilizationbot](https://github.com/Valgorithms/Civilizationbot) +relies on for `approveme`; keep it backward compatible. + +### Records + +```json +[{"discord": "123456789012345678", "ss13": "alice", "create_time": "2026-01-01 12:00:00"}] +``` + +Each record links exactly one Discord user id to one BYOND ckey. A `ckey` is +stored in its normalised form: lowercased, with `.`, `_`, `-`, `+` and spaces +removed, which is how BYOND and the bot compare keys. + +### `POST /verified` + +Body: `application/x-www-form-urlencoded` (the legacy `Field: value` header +form is still accepted for in-process callers). Fields: + +| Field | Required | Meaning | +| --------- | -------- | ------- | +| `token` | yes | Must equal the configured `TOKEN`. | +| `discord` | insert | Discord user id, 17 to 20 digits. | +| `ckey` | insert | BYOND key, normalised before storage. | +| `method` | no | `delete` removes the record matching `ckey`, else `discord`. Anything else inserts. | + +Status codes: + +| Status | Meaning | +| ------ | ------- | +| 200 | Inserted (body: the full list) or deleted (body: the removed records). | +| 400 | `discord` or `ckey` is malformed. Nothing was stored. | +| 401 | Missing or wrong `token`, or the server still uses the default token. | +| 403 | The Discord id or the (normalised) ckey is already registered. | +| 404 | `delete` found no matching record. | +| 405 | Unsupported HTTP method. | + +### `GET /verified` + +Returns the full list as JSON. There is no read token; do not host the +server on a public interface unless that is intended. ## Running Tests diff --git a/src/VerifierServer/Endpoints/SS14VerifiedEndpoint.php b/src/VerifierServer/Endpoints/SS14VerifiedEndpoint.php index d8a7a9b..a1480d1 100644 --- a/src/VerifierServer/Endpoints/SS14VerifiedEndpoint.php +++ b/src/VerifierServer/Endpoints/SS14VerifiedEndpoint.php @@ -50,6 +50,24 @@ class SS14VerifiedEndpoint extends VerifiedEndpoint public function __construct(protected &$state) {} + /** SS14 user names keep their spelling; they only need to be printable and bounded. */ + protected function validateRecord(string $discord, string &$account): ?string + { + if (preg_match(self::DISCORD_ID_PATTERN, $discord) !== 1) { + return 'discord must be a Discord user id (17 to 20 digits)'; + } + $account = trim($account); + if ($account === '' || strlen($account) > 256 || preg_match('/[\x00-\x1F\x7F]/', $account) === 1) { + return 'ss14 must be a printable user name of at most 256 bytes'; + } + return null; + } + + protected function findAccount(string $account, array $list): int|string|false + { + return $this->findIndex('ss14', $account, $list); + } + /** * Handles POST requests by parsing the request data and performing actions based on the method type. * @@ -132,7 +150,13 @@ protected function post(ServerRequestInterface|string $request, int|string &$res */ protected function __post(array &$list, string $ss14, string $discord, int|string &$response, array &$headers, string &$body): void { - if ($this->findIndex('discord', $discord, $list) !== false || $this->findIndex('ss14', $ss14, $list) !== false) { + if ($problem = $this->validateRecord($discord, $ss14)) { + $response = Response::STATUS_BAD_REQUEST; + $headers = ['Content-Type' => 'text/plain']; + $body = "Bad Request: $problem"; + return; + } + if ($this->findIndex('discord', $discord, $list) !== false || $this->findAccount($ss14, $list) !== false) { $response = Response::STATUS_FORBIDDEN; $headers = ['Content-Type' => 'text/plain']; $body = 'Forbidden'; diff --git a/src/VerifierServer/Endpoints/VerifiedEndpoint.php b/src/VerifierServer/Endpoints/VerifiedEndpoint.php index 8b3b580..546adfa 100644 --- a/src/VerifierServer/Endpoints/VerifiedEndpoint.php +++ b/src/VerifierServer/Endpoints/VerifiedEndpoint.php @@ -48,25 +48,66 @@ */ class VerifiedEndpoint extends Endpoint { + /** Discord user ids are snowflakes: 17 to 20 decimal digits. */ + public const DISCORD_ID_PATTERN = '/^[0-9]{17,20}$/D'; + /** A normalised BYOND ckey: lowercase letters and digits only. */ + public const CKEY_PATTERN = '/^[a-z0-9]{1,256}$/D'; + private ?array $lookupSnapshot = null; private array $lookupColumns = []; /** Snapshot comparison also detects changes through SS14's reference accessor. */ - protected function findIndex(string $column, string $value, ?array $list = null): int|string|false + protected function findIndex(string $column, string $value, ?array $list = null, ?callable $normalize = null): int|string|false { $list = $list ?? $this->state->getVerifyList(); if ($this->lookupSnapshot !== $list) { $this->lookupSnapshot = $list; $this->lookupColumns = []; } - if (!array_key_exists($column, $this->lookupColumns)) { + $cacheKey = $normalize ? "$column#normalized" : $column; + if (!array_key_exists($cacheKey, $this->lookupColumns)) { $values = []; foreach ($list as $index => $item) { - if (array_key_exists($column, $item)) $values[$index] = $item[$column]; + if (!array_key_exists($column, $item)) continue; + $values[$index] = $normalize && is_string($item[$column]) ? $normalize($item[$column]) : $item[$column]; } - $this->lookupColumns[$column] = $values; + $this->lookupColumns[$cacheKey] = $values; + } + return array_search($normalize ? $normalize($value) : $value, $this->lookupColumns[$cacheKey]); + } + + /** + * Reduces a BYOND key to its ckey form the same way Civilizationbot's + * sanitizeInput() and HostedVerificationSource::validate() do, so that + * two spellings of one account can never both be registered. + */ + public static function normalizeCkey(string $key): string + { + return str_replace(['.', '_', '-', '+', ' '], '', strtolower(trim($key))); + } + + /** + * Validates and canonicalises a record before it is stored. + * + * @return string|null A human-readable problem, or null when the record is acceptable. + * `$account` is replaced with its canonical spelling on success. + */ + protected function validateRecord(string $discord, string &$account): ?string + { + if (preg_match(self::DISCORD_ID_PATTERN, $discord) !== 1) { + return 'discord must be a Discord user id (17 to 20 digits)'; } - return array_search($value, $this->lookupColumns[$column]); + $account = self::normalizeCkey($account); + if (preg_match(self::CKEY_PATTERN, $account) !== 1) { + return 'ckey must be a BYOND key (letters and digits after normalisation)'; + } + return null; + } + + /** Index of a record whose account matches `$account` after normalisation, or false. */ + protected function findAccount(string $account, array $list): int|string|false + { + return $this->findIndex('ss13', $account, $list, [self::class, 'normalizeCkey']); } public function __construct(protected &$state) @@ -210,10 +251,11 @@ protected function post(ServerRequestInterface|string $request, int|string &$res /** * Handles the default verification process. * - * This method checks if the provided `ckey` or `discord` already exists in the list. - * If either exists, it sets the response to a 403 Forbidden status. - * If neither exists, it adds the new entry to the list, writes the updated list to a JSON file, - * updates the state, and sets the response to a 200 OK status. + * This method first validates the record (400 Bad Request when the Discord id or ckey + * is malformed), then checks if the provided `ckey` or `discord` already exists in the list, + * comparing ckeys after normalisation. If either exists, it sets the response to a + * 403 Forbidden status. If neither exists, it adds the new entry to the list, writes the + * updated list to a JSON file, updates the state, and sets the response to a 200 OK status. * * @param array $list The list of existing entries. * @param string $ckey The ckey to be verified. @@ -224,7 +266,13 @@ protected function post(ServerRequestInterface|string $request, int|string &$res */ protected function __post(array &$list, string $ckey, string $discord, int|string &$response, array &$headers, string &$body): void { - if ($this->findIndex('discord', $discord, $list) !== false || $this->findIndex('ss13', $ckey, $list) !== false) { + if ($problem = $this->validateRecord($discord, $ckey)) { + $response = Response::STATUS_BAD_REQUEST; + $headers = ['Content-Type' => 'text/plain']; + $body = "Bad Request: $problem"; + return; + } + if ($this->findIndex('discord', $discord, $list) !== false || $this->findAccount($ckey, $list) !== false) { $response = Response::STATUS_FORBIDDEN; $headers = ['Content-Type' => 'text/plain']; $body = 'Forbidden'; diff --git a/tests/isolated/RecordValidationTest.php b/tests/isolated/RecordValidationTest.php new file mode 100644 index 0000000..827f303 --- /dev/null +++ b/tests/isolated/RecordValidationTest.php @@ -0,0 +1,118 @@ + 'application/x-www-form-urlencoded'], http_build_query($fields + ['token' => 'test-token'])); + $status = 404; $headers = []; $body = ''; + $endpoint->handle('POST', $request, $status, $headers, $body); + return [$status, $body]; + } + + private function endpoint(array $list = []): VerifiedEndpoint + { + $state = new ValidationMemoryState('test-token'); + $state->setVerifyList($list, false); + return new VerifiedEndpoint($state); + } + + /** @dataProvider malformedRecords */ + public function testMalformedRecordsAreRejectedWith400(array $fields, string $problem): void + { + $endpoint = $this->endpoint(); + [$status, $body] = $this->post($endpoint, $fields); + self::assertSame(400, $status); + self::assertStringContainsString($problem, $body); + self::assertSame([], $endpoint->getState()->getVerifyList()); + } + + public function malformedRecords(): array + { + return [ + 'no fields' => [[], 'discord'], + 'short discord' => [['discord' => '123', 'ckey' => 'alice'], 'discord'], + 'non-numeric discord' => [['discord' => '12345678901234567x', 'ckey' => 'alice'], 'discord'], + 'mention' => [['discord' => '<@123456789012345678>', 'ckey' => 'alice'], 'discord'], + 'empty ckey' => [['discord' => '123456789012345678', 'ckey' => ''], 'ckey'], + 'ckey of separators only' => [['discord' => '123456789012345678', 'ckey' => '_ -.'], 'ckey'], + 'control characters' => [['discord' => '123456789012345678', 'ckey' => "al\x01ice"], 'ckey'], + 'unicode' => [['discord' => '123456789012345678', 'ckey' => 'alíce'], 'ckey'], + ]; + } + + public function testKeysAreStoredAsNormalisedCkeys(): void + { + $endpoint = $this->endpoint(); + [$status, $body] = $this->post($endpoint, ['discord' => '123456789012345678', 'ckey' => ' Al_ice-Smith.Jr ']); + self::assertSame(200, $status); + self::assertSame('alicesmithjr', json_decode($body, true)[0]['ss13']); + self::assertSame('alicesmithjr', $endpoint->getState()->getVerifyList()[0]['ss13']); + self::assertSame(0, $endpoint->getIndex('', 'alicesmithjr')); + } + + public function testAnotherSpellingOfARegisteredKeyIsADuplicate(): void + { + $endpoint = $this->endpoint([['discord' => '123456789012345678', 'ss13' => 'alice']]); + [$status] = $this->post($endpoint, ['discord' => '234567890123456789', 'ckey' => 'Al_ice']); + self::assertSame(403, $status); + $endpoint = $this->endpoint([['discord' => '123456789012345678', 'ss13' => 'Al Ice']]); + [$status] = $this->post($endpoint, ['discord' => '234567890123456789', 'ckey' => 'alice']); + self::assertSame(403, $status, 'legacy records are compared after normalisation too'); + self::assertCount(1, $endpoint->getState()->getVerifyList()); + [$status] = $this->post($endpoint, ['discord' => '234567890123456789', 'ckey' => 'bob']); + self::assertSame(200, $status); + self::assertCount(2, $endpoint->getState()->getVerifyList()); + } + + public function testLegacyHeaderRequestsAreValidatedToo(): void + { + $endpoint = $this->endpoint(); + $status = 404; $headers = []; $body = ''; + $endpoint->handle('POST', "token: test-token".PHP_EOL."discord: 42".PHP_EOL."ckey: alice", $status, $headers, $body); + self::assertSame(400, $status); + $endpoint->handle('POST', "token: test-token".PHP_EOL."discord: 123456789012345678".PHP_EOL."ckey: Alice", $status, $headers, $body); + self::assertSame(200, $status); + self::assertSame('alice', $endpoint->getState()->getVerifyList()[0]['ss13']); + } + + public function testDeletionDoesNotRequireAValidRecord(): void + { + $endpoint = $this->endpoint([['discord' => '123456789012345678', 'ss13' => 'alice']]); + [$status] = $this->post($endpoint, ['method' => 'delete', 'discord' => 'alice', 'ckey' => 'alice']); + self::assertSame(200, $status); + self::assertSame([], $endpoint->getState()->getVerifyList()); + } + + public function testSs14NamesKeepTheirSpellingButMustBePrintable(): void + { + $state = new ValidationMemorySS14State('test-token'); + $state->setVerifyList([], false); + $endpoint = new SS14VerifiedEndpoint($state); + [$status] = $this->post($endpoint, ['discord' => '123456789012345678', 'ss14' => 'Space Captain_42']); + self::assertSame(200, $status); + self::assertSame('Space Captain_42', $state->getVerifyList()[0]['ss14']); + [$status] = $this->post($endpoint, ['discord' => '234567890123456789', 'ss14' => "bad\x00name"]); + self::assertSame(400, $status); + [$status] = $this->post($endpoint, ['discord' => '123', 'ss14' => 'Someone']); + self::assertSame(400, $status); + [$status] = $this->post($endpoint, ['discord' => '234567890123456789', 'ss14' => 'Space Captain_42']); + self::assertSame(403, $status); + } +} diff --git a/tests/isolated/VerificationContractTest.php b/tests/isolated/VerificationContractTest.php index d7550f9..a5c2fc9 100644 --- a/tests/isolated/VerificationContractTest.php +++ b/tests/isolated/VerificationContractTest.php @@ -24,16 +24,16 @@ public function testRegistrationAndDuplicateContracts(bool $ss14): void $state->setVerifyList([], false); $endpoint = $ss14 ? new SS14VerifiedEndpoint($state) : new VerifiedEndpoint($state); $field = $ss14 ? 'ss14' : 'ckey'; - $request = new ServerRequest('POST', '/verified', ['Content-Type' => 'application/x-www-form-urlencoded; charset=UTF-8'], http_build_query(['token' => 'test-token', 'discord' => '123', $field => 'alice'])); + $request = new ServerRequest('POST', '/verified', ['Content-Type' => 'application/x-www-form-urlencoded; charset=UTF-8'], http_build_query(['token' => 'test-token', 'discord' => '100000000000000123', $field => 'alice'])); $status = 404; $headers = []; $body = ''; $endpoint->handle('POST', $request, $status, $headers, $body); self::assertSame(200, $status); self::assertCount(1, json_decode($body, true)); - self::assertSame(0, $endpoint->getIndex('123')); + self::assertSame(0, $endpoint->getIndex('100000000000000123')); $endpoint->handle('POST', $request, $status, $headers, $body); self::assertSame(403, $status); self::assertCount(1, $state->getVerifyList()); - $request = new ServerRequest('POST', '/verified', ['Content-Type' => 'application/x-www-form-urlencoded'], 'token=wrong&discord=456&'.$field.'=bob'); + $request = new ServerRequest('POST', '/verified', ['Content-Type' => 'application/x-www-form-urlencoded'], 'token=wrong&discord=100000000000000456&'.$field.'=bob'); $endpoint->handle('POST', $request, $status, $headers, $body); self::assertSame(401, $status); self::assertCount(1, $state->getVerifyList()); @@ -41,14 +41,14 @@ public function testRegistrationAndDuplicateContracts(bool $ss14): void public function testDuplicateCkeyCannotLinkAnotherDiscord(): void { $state = new OfflineVerificationState('test-token'); - $state->setVerifyList([['discord' => '123', 'ss13' => 'alice']], false); + $state->setVerifyList([['discord' => '100000000000000123', 'ss13' => 'alice']], false); $endpoint = new VerifiedEndpoint($state); $status = 404; $headers = []; $body = ''; - $request = new ServerRequest('POST', '/verified', ['Token' => 'test-token', 'Discord' => '456', 'Ckey' => 'alice']); + $request = new ServerRequest('POST', '/verified', ['Token' => 'test-token', 'Discord' => '100000000000000456', 'Ckey' => 'alice']); $endpoint->handle('POST', $request, $status, $headers, $body); self::assertSame(403, $status); self::assertCount(1, $state->getVerifyList()); - $request = new ServerRequest('POST', '/verified', ['Token' => 'wrong', 'Content-Type' => 'application/x-www-form-urlencoded'], 'token=test-token&discord=456&ckey=bob'); + $request = new ServerRequest('POST', '/verified', ['Token' => 'wrong', 'Content-Type' => 'application/x-www-form-urlencoded'], 'token=test-token&discord=100000000000000456&ckey=bob'); $endpoint->handle('POST', $request, $status, $headers, $body); self::assertSame(200, $status); self::assertCount(2, $state->getVerifyList()); @@ -62,9 +62,9 @@ public function testLegacyHeadersAndDiscordOnlyDeletion(): void $state->setVerifyList([], false); $endpoint = new VerifiedEndpoint($state); $status = 404; $headers = []; $body = ''; - $endpoint->handle('POST', "token: test-token".PHP_EOL."discord: 123".PHP_EOL."ckey: alice", $status, $headers, $body); + $endpoint->handle('POST', "token: test-token".PHP_EOL."discord: 100000000000000123".PHP_EOL."ckey: alice", $status, $headers, $body); self::assertSame(200, $status); - $endpoint->handle('POST', "token: test-token".PHP_EOL."discord: 123".PHP_EOL."method: delete", $status, $headers, $body); + $endpoint->handle('POST', "token: test-token".PHP_EOL."discord: 100000000000000123".PHP_EOL."method: delete", $status, $headers, $body); self::assertSame(200, $status); self::assertSame([], $state->getVerifyList()); } @@ -72,24 +72,24 @@ public function testLegacyHeadersAndDiscordOnlyDeletion(): void public function testSparseKeysAndLookupPrecedence(): void { $state = new OfflineVerificationState('test-token'); - $state->setVerifyList([7 => ['discord' => '123', 'ss13' => 'alice'], 12 => ['discord' => '456', 'ss13' => 'bob']], false); + $state->setVerifyList([7 => ['discord' => '100000000000000123', 'ss13' => 'alice'], 12 => ['discord' => '100000000000000456', 'ss13' => 'bob']], false); $endpoint = new VerifiedEndpoint($state); - self::assertSame(12, $endpoint->getIndex('123', 'bob')); - self::assertSame(7, $endpoint->getIndex('123', 'absent')); + self::assertSame(12, $endpoint->getIndex('100000000000000123', 'bob')); + self::assertSame(7, $endpoint->getIndex('100000000000000123', 'absent')); self::assertFalse($endpoint->getIndex('', '')); self::assertFalse($endpoint->getIndex('absent')); $ss14 = new OfflineSS14VerificationState('test-token'); - $ss14->setVerifyList([7 => ['discord' => '123', 'ss14' => 'alice'], 12 => ['discord' => '456', 'ss14' => 'bob']], false); + $ss14->setVerifyList([7 => ['discord' => '100000000000000123', 'ss14' => 'alice'], 12 => ['discord' => '100000000000000456', 'ss14' => 'bob']], false); $endpoint = new SS14VerifiedEndpoint($ss14); - self::assertSame(7, $endpoint->getIndex('123', 'bob')); - self::assertSame(['discord' => '456', 'ss14' => 'bob'], $endpoint->fetch('ss14', 'bob')); + self::assertSame(7, $endpoint->getIndex('100000000000000123', 'bob')); + self::assertSame(['discord' => '100000000000000456', 'ss14' => 'bob'], $endpoint->fetch('ss14', 'bob')); self::assertNull($endpoint->fetch('ss14', 'absent')); $live = &$ss14->getVerifyList(); - $live[7]['discord'] = '999'; - self::assertFalse($endpoint->getIndex('123')); - self::assertSame(7, $endpoint->getIndex('999')); + $live[7]['discord'] = '100000000000000999'; + self::assertFalse($endpoint->getIndex('100000000000000123')); + self::assertSame(7, $endpoint->getIndex('100000000000000999')); $ss14->setVerifyList([], false); - self::assertFalse($endpoint->getIndex('123')); + self::assertFalse($endpoint->getIndex('100000000000000123')); self::assertNull($endpoint->fetch('ss14', 'bob')); } }