diff --git a/src/VerifierServer/Endpoints/SS14VerifiedEndpoint.php b/src/VerifierServer/Endpoints/SS14VerifiedEndpoint.php index d8a7a9b..ff23e46 100644 --- a/src/VerifierServer/Endpoints/SS14VerifiedEndpoint.php +++ b/src/VerifierServer/Endpoints/SS14VerifiedEndpoint.php @@ -83,7 +83,7 @@ protected function post(ServerRequestInterface|string $request, int|string &$res ? trim(is_array($formData['token']) ? $formData['token'][0] : $formData['token']) : ''; - if (!$bypass_token && ($this->state->getToken() === 'changeme' || $token !== $this->state->getToken())) { + if (!$bypass_token && ($this->state->getToken() === 'changeme' || !hash_equals($this->state->getToken(), $token))) { $response = Response::STATUS_UNAUTHORIZED; $headers = ['Content-Type' => 'text/plain']; $body = 'Unauthorized'; diff --git a/src/VerifierServer/Endpoints/VerifiedEndpoint.php b/src/VerifierServer/Endpoints/VerifiedEndpoint.php index 8b3b580..1e51cd9 100644 --- a/src/VerifierServer/Endpoints/VerifiedEndpoint.php +++ b/src/VerifierServer/Endpoints/VerifiedEndpoint.php @@ -175,7 +175,7 @@ protected function post(ServerRequestInterface|string $request, int|string &$res ? trim(is_array($formData['token']) ? $formData['token'][0] : $formData['token']) : ''; - if (!$bypass_token && ($this->state->getToken() === 'changeme' || $token !== $this->state->getToken())) { + if (!$bypass_token && ($this->state->getToken() === 'changeme' || !hash_equals($this->state->getToken(), $token))) { $response = Response::STATUS_UNAUTHORIZED; $headers = ['Content-Type' => 'text/plain']; $body = 'Unauthorized'; diff --git a/src/VerifierServer/Server.php b/src/VerifierServer/Server.php index e3d525a..94a4d8f 100644 --- a/src/VerifierServer/Server.php +++ b/src/VerifierServer/Server.php @@ -179,7 +179,7 @@ private function __initReactHttpServer(?LoopInterface $loop = null): void ? $loop : Loop::get(), fn($request) => $this->handleReact($request) - ))->on('error', fn(Throwable $e) => $this->logError($e, true)); + ))->on('error', fn(Throwable $e) => $this->logError($e, false)); $this->socket = new SocketServer( "{$this->addr}:{$this->port}", [ @@ -270,6 +270,7 @@ protected function route( return; } + $uri = self::normalizePath($uri); $endpoint = null; $allowed_methods = (isset($this->endpoints[$uri]) && ($endpoint = $this->endpoints[$uri]) instanceof EndpointInterface) ? array_merge($endpoint->getAllowedMethods(), self::DEFAULT_METHODS) @@ -288,9 +289,21 @@ protected function route( //TODO: Check if the method is implemented by the endpoint - $endpoint - ? $endpoint->handle($method, $request, $response, $headers, $body) - : $this->logError(new Exception("Endpoint not found: $uri"), false); + if (! $endpoint) { + if (isset($this->logger)) $this->logger->debug("Endpoint not found: $uri"); + return; + } + $endpoint->handle($method, $request, $response, $headers, $body); + } + + /** + * Collapses a trailing slash so that `/verified/` and `/verified` reach the same endpoint. + * Civilizationbot's default verify_url ends with a slash. + */ + public static function normalizePath(string $uri): string + { + $trimmed = rtrim($uri, '/'); + return $trimmed === '' ? '/' : $trimmed; } /** @@ -314,6 +327,7 @@ public function handleEndpoint( bool $bypass_token = false ): void { + $uri = self::normalizePath($uri); if (isset($this->endpoints[$uri]) && $this->endpoints[$uri] instanceof EndpointInterface) { $this->endpoints[$uri]->handle($method, $request, $response, $headers, $body, $bypass_token); } @@ -321,6 +335,7 @@ public function handleEndpoint( public function getEndpoint(string $uri): ?EndpointInterface { + $uri = self::normalizePath($uri); return (isset($this->endpoints[$uri]) && $this->endpoints[$uri] instanceof EndpointInterface) ? $this->endpoints[$uri] : null; @@ -342,8 +357,16 @@ private function handleReact(ServerRequestInterface $request): ResponseInterface $response = Response::STATUS_NOT_FOUND; $headers = ['Content-Type' => 'text/plain']; $body = "Not Found"; - - $this->route($uri, $method, $request, $response, $headers, $body); + + try { + $this->route($uri, $method, $request, $response, $headers, $body); + } catch (Throwable $e) { + // A failing request must not take the listener down with it. + $this->logError($e, false); + $response = Response::STATUS_INTERNAL_SERVER_ERROR; + $headers = ['Content-Type' => 'text/plain']; + $body = 'Internal Server Error'; + } return new Response( $response, @@ -372,7 +395,7 @@ private function handleResource($client): null throw new Exception("Failed to get client name"); } //var_dump($sender); - $requester = explode($sender, ':'); + $requester = explode(':', $sender); $client_headers = explode(PHP_EOL, trim($request)) ?: []; $firstLine = explode(' ', array_shift($client_headers)); @@ -432,13 +455,11 @@ private function handleResource($client): null /** * Logs an error message with details about the exception. * - * @param Exception $e The exception to log. + * @param Throwable $e The exception to log. * @param bool $fatal Optional. Indicates whether the error is fatal. Defaults to false. * If true, the server will stop after logging the error. - * - * @return string */ - public function logError(Exception $e, bool $fatal = false): void + public function logError(Throwable $e, bool $fatal = false): void { if ($fatal) $this->close(); if (isset($this->logger)) $this->logger->warning(sprintf( @@ -517,7 +538,7 @@ public function getState(): ?PersistentState } public function getSS14State(): ?SS14PersistentState { - return $this->SS14state ?? null; + return $this->ss14state ?? null; } public function getSessions(): array diff --git a/tests/isolated/RequestRobustnessTest.php b/tests/isolated/RequestRobustnessTest.php new file mode 100644 index 0000000..79d0dda --- /dev/null +++ b/tests/isolated/RequestRobustnessTest.php @@ -0,0 +1,100 @@ +records[] = [$level, (string) $message]; + } +} + +final class RequestRobustnessTest extends TestCase +{ + private function server(PersistentState $state): Server + { + $server = new Server('127.0.0.1', 16261); + $server->setState($state); + return $server; + } + + private function react(Server $server, ServerRequest $request): \Psr\Http\Message\ResponseInterface + { + return (new ReflectionMethod(Server::class, 'handleReact'))->invoke($server, $request); + } + + public function testEndpointFailureIsA500AndDoesNotStopTheServer(): void + { + $server = $this->server(new FailingVerificationState('test-token')); + $logger = new RecordingLogger(); + $server->setLogger($logger); + $response = $this->react($server, new ServerRequest('GET', '/verified')); + self::assertSame(500, $response->getStatusCode()); + self::assertSame('Internal Server Error', (string) $response->getBody()); + self::assertNotEmpty($logger->records); + self::assertStringContainsString('storage unavailable', $logger->records[0][1]); + self::assertStringNotContainsString('[FATAL]', $logger->records[0][1]); + $running = new ReflectionProperty(Server::class, 'running'); + $running->setValue($server, true); + $server->logError(new RuntimeException('again'), false); + self::assertTrue($running->getValue($server), 'a request error must not close the listener'); + } + + public function testTrailingSlashReachesTheVerifiedEndpoint(): void + { + $state = new MemoryVerificationState('test-token'); + $state->setVerifyList([['discord' => '123456789012345678', 'ss13' => 'alice']], false); + $server = $this->server($state); + foreach (['/verified', '/verified/', '/verified//', '/'] as $path) { + $response = $this->react($server, new ServerRequest('GET', $path)); + self::assertSame(200, $response->getStatusCode(), $path); + self::assertSame('application/json', $response->getHeaderLine('Content-Type'), $path); + } + self::assertSame(404, $this->react($server, new ServerRequest('GET', '/missing/'))->getStatusCode()); + self::assertNotNull($server->getEndpoint('/verified/')); + + $status = 404; $headers = []; $body = ''; + $server->handleEndpoint('/verified/', 'POST', "token: test-token".PHP_EOL."discord: 234567890123456789".PHP_EOL."ckey: bob", $status, $headers, $body); + self::assertSame(200, $status); + self::assertCount(2, $state->getVerifyList()); + } + + public function testTokenMismatchStillUnauthorized(): void + { + $state = new MemoryVerificationState('test-token'); + $state->setVerifyList([], false); + $server = $this->server($state); + $status = 404; $headers = []; $body = ''; + $server->handleEndpoint('/verified', 'POST', "token: test-tokem".PHP_EOL."discord: 234567890123456789".PHP_EOL."ckey: bob", $status, $headers, $body); + self::assertSame(401, $status); + self::assertSame([], $state->getVerifyList()); + } + + public function testSs14StateAccessor(): void + { + $server = new Server('127.0.0.1', 16261); + self::assertNull($server->getSS14State()); + $ss14 = new SS14PersistentState('test-token'); + $server->setSS14State($ss14); + self::assertSame($ss14, $server->getSS14State()); + self::assertNotNull($server->getEndpoint('/ss14verified/')); + } +}