diff --git a/README.md b/README.md index 77f1a1c..eaaa6ed 100644 --- a/README.md +++ b/README.md @@ -106,6 +106,73 @@ $request->psr; // the PSR-7 request, for everything else is an ordinary thing to receive from the open internet, and a handler replying `400` reads better than one wrapped in a `try`. +## Middleware + +```php +#[Route(Method::POST, '/api/links', middleware: [VerifySignature::class])] +final readonly class CreateLink { /* ... */ } +``` + +```php +final readonly class VerifySignature implements Middleware +{ + public function __invoke(Request $request, callable $next): Response + { + return $this->valid($request) ? $next($request) : Response::unauthorized(); + } +} +``` + +The first one listed is outermost. Returning an answer instead of calling +`$next` stops the request there — the handler never runs, and neither does any +middleware behind it. That is the whole reason this exists: a check on whether +the caller may be asking has to be able to stop the work, not merely disapprove +of it afterwards. + +Middleware are built by the container, so they may take whatever they need. + +## Deciding who is asking + +There are no sessions here, and that is deliberate — see below. What there is +covers what a bot's HTTP layer actually needs: + +```php +$request->cookie('session'); // off this request's Cookie header +$request->bearerToken(); // from "Authorization: Bearer ..." + +Secret::matches($configured, $request->bearerToken() ?? ''); +Secret::signed($request->body(), $request->header('X-Signature') ?? '', $secret); + +return Response::noContent()->withCookie(new Cookie('session', $id, secure: true)); +``` + +`Secret` compares in constant time. `===` on a secret stops at the first byte +that differs, and that difference is measurable over enough requests — which is +how a signature gets guessed one byte at a time by somebody with a loop and +patience. `signed()` takes the **raw** body, because re-encoding changes bytes +and the signature is over what was actually sent. + +Cookies go out on the response, with `HttpOnly`, `SameSite=Lax` and `Path=/` by +default. `Secure` is left to you: a bot behind a plain-HTTP proxy would +otherwise set a cookie the browser never returns, which is a bug that looks like +a login loop. + +## Never use PHP sessions here + +`session_start()` and `$_SESSION` are unsafe in this server, and not in a subtle +way. PHP keeps **one** session per process. Under PHP-FPM that is fine because +the process *is* the request; here the process serves thousands of requests over +weeks, so the first caller's session becomes the process's session and every +caller after them reads it. With fibers, two requests interleave inside it. + +The same goes for `$_GET`, `$_POST`, `$_COOKIE` and `$_SERVER`: they hold +whatever the CLI process started with, not the request being served. And +`setcookie()` and `header()` write nowhere — the answer is the `Response`. + +For the same reason a **handler must not keep state on itself**. It is built by +the container, which may hand back the same instance next time; the request is +given as an argument precisely so that nothing has to be stashed. + ## Configuration `app/config/http.config.php`: diff --git a/src/Attributes/Route.php b/src/Attributes/Route.php index 2841403..8ce9ff1 100644 --- a/src/Attributes/Route.php +++ b/src/Attributes/Route.php @@ -27,8 +27,14 @@ #[Attribute(Attribute::TARGET_CLASS | Attribute::IS_REPEATABLE)] final readonly class Route { + /** + * @param list> $middleware + * run in the order given, outermost first; any of them may answer + * instead of letting the handler run + */ public function __construct( public Method $method, public string $path, + public array $middleware = [], ) {} } diff --git a/src/Compiler/RouteCompiler.php b/src/Compiler/RouteCompiler.php index 1c5b2d3..45254f7 100644 --- a/src/Compiler/RouteCompiler.php +++ b/src/Compiler/RouteCompiler.php @@ -39,6 +39,7 @@ public function compile(ClassReflector $class, Route $route): RouteDefinition parameters: $parameters, handler: $class->getName(), invoke: $class->getMethod('__invoke'), + middleware: $route->middleware, ); } diff --git a/src/Definitions/RouteDefinition.php b/src/Definitions/RouteDefinition.php index 209af79..1a1f4e2 100644 --- a/src/Definitions/RouteDefinition.php +++ b/src/Definitions/RouteDefinition.php @@ -17,6 +17,7 @@ * segment captured under its own name * @param list $parameters the segment names, in the order the path * declares them + * @param list> $middleware */ public function __construct( public Method $method, @@ -25,6 +26,7 @@ public function __construct( public array $parameters, public string $handler, public MethodReflector $invoke, + public array $middleware = [], ) {} /** diff --git a/src/Http/Cookie.php b/src/Http/Cookie.php new file mode 100644 index 0000000..ba3548c --- /dev/null +++ b/src/Http/Cookie.php @@ -0,0 +1,67 @@ +name . '=' . rawurlencode($this->value)]; + + if ($this->expiresAt !== null) { + $parts[] = 'Expires=' . gmdate('D, d M Y H:i:s \G\M\T', $this->expiresAt); + $parts[] = 'Max-Age=' . max(0, $this->expiresAt - time()); + } + + $parts[] = 'Path=' . $this->path; + + if ($this->domain !== null) { + $parts[] = 'Domain=' . $this->domain; + } + + if ($this->secure) { + $parts[] = 'Secure'; + } + + if ($this->httpOnly) { + $parts[] = 'HttpOnly'; + } + + $parts[] = 'SameSite=' . $this->sameSite->value; + + return implode('; ', $parts); + } +} diff --git a/src/Http/Middleware.php b/src/Http/Middleware.php new file mode 100644 index 0000000..7ab7b6c --- /dev/null +++ b/src/Http/Middleware.php @@ -0,0 +1,23 @@ +psr->hasHeader($name) ? $this->psr->getHeaderLine($name) : null; } + /** + * A cookie the browser sent, read off this request's own header. + * + * Never $_COOKIE: in a long-running server the superglobals hold whatever + * the process was started with, not what the person on the other end of + * this socket sent. + */ + public function cookie(string $name): ?string + { + $cookies = $this->psr->getCookieParams(); + $value = $cookies[$name] ?? null; + + return is_scalar($value) ? (string) $value : null; + } + + /** + * The token from an "Authorization: Bearer ..." header. + * + * Only Bearer: anything else is a different scheme with different rules, + * and quietly treating it as a token would accept credentials meant for + * something else entirely. + */ + public function bearerToken(): ?string + { + $header = $this->header('Authorization'); + + if ($header === null || !preg_match('/\ABearer\s+(\S+)\z/i', $header, $found)) { + return null; + } + + return $found[1]; + } + public function body(): string { return (string) $this->psr->getBody(); diff --git a/src/Http/Response.php b/src/Http/Response.php index d688105..32111ef 100644 --- a/src/Http/Response.php +++ b/src/Http/Response.php @@ -15,13 +15,34 @@ */ final readonly class Response { + /** + * @param array $headers + * @param list $cookies + */ private function __construct( public int $status, public string $body, - /** @var array */ public array $headers, + public array $cookies = [], ) {} + /** + * The same answer, with a cookie set on it. + * + * On the response rather than through setcookie(): there are no headers to + * send in a long-running server, and a cookie belongs to one answer rather + * than to the process that happened to build it. + */ + public function withCookie(Cookie $cookie): self + { + return new self($this->status, $this->body, $this->headers, [...$this->cookies, $cookie]); + } + + public function withHeader(string $name, string $value): self + { + return new self($this->status, $this->body, [...$this->headers, $name => $value], $this->cookies); + } + /** * @param array $headers */ @@ -65,6 +86,19 @@ public static function unauthorized(string $message = 'Unauthorized'): self public function toReact(): ReactResponse { - return new ReactResponse($this->status, $this->headers, $this->body); + $headers = $this->headers; + + /* + * Set-Cookie is the one header that may legitimately appear more than + * once, so the values go out as a list rather than being joined. + */ + if ($this->cookies !== []) { + $headers['Set-Cookie'] = array_map( + static fn(Cookie $cookie) => $cookie->header(), + $this->cookies, + ); + } + + return new ReactResponse($this->status, $headers, $this->body); } } diff --git a/src/Http/SameSite.php b/src/Http/SameSite.php new file mode 100644 index 0000000..0370a17 --- /dev/null +++ b/src/Http/SameSite.php @@ -0,0 +1,17 @@ +invoke->invokeArgs( - $this->container->get($route->handler), - $this->argumentsFor($route, $request), - ); + return $this->through($route)($request); } catch (Throwable $throwable) { /* * Contained, and deliberately vague to the caller: whatever went * wrong is in the log, and an exception message is not something to * hand to whoever is on the other end of the socket. */ - $this->logger->error( + $this->logger()->error( 'Route ' . $route->method->value . ' ' . $route->path . ' failed: ' . $throwable->getMessage(), ['exception' => $throwable], @@ -98,8 +96,64 @@ private function call(RouteDefinition $route, Request $request): Response return Response::json(['error' => 'Internal Server Error'], 500); } + } + + /** + * The handler, wrapped in its middleware. + * + * Built from the inside out so that the first one listed ends up outermost, + * which is the order anyone reading the attribute expects — and the order + * that matters when one of them is the check that says whether the caller + * may be here at all. + * + * @return callable(Request): Response + */ + private function through(RouteDefinition $route): callable + { + /* + * Normalised here rather than after the middleware have run, so that + * one of them wrapping the answer is handed a Response and not + * whatever a handler happened to return. + */ + $next = function (Request $request) use ($route): Response { + $answer = $route->invoke->invokeArgs( + $this->container->get($route->handler), + $this->argumentsFor($route, $request), + ); + + return $answer instanceof Response ? $answer : Response::noContent(); + }; + + foreach (array_reverse($route->middleware) as $middleware) { + $inner = $next; + $next = fn(Request $request): Response => $this->middleware($middleware)($request, $inner); + } + + return $next; + } + + private function middleware(string $class): Middleware + { + $middleware = $this->container->get($class); + + if (!$middleware instanceof Middleware) { + throw new RuntimeException($class . ' must implement ' . Middleware::class); + } + + return $middleware; + } - return $answer instanceof Response ? $answer : Response::noContent(); + /** + * Resolved when something goes wrong rather than in the constructor. + * + * Discovery builds this router while the container is still being + * assembled, before the initializers that provide the logger have + * themselves been found — asking for one up front makes the whole + * application fail to boot. + */ + private function logger(): Logger + { + return $this->container->get(Logger::class); } /** diff --git a/tests/Fixtures/First.php b/tests/Fixtures/First.php new file mode 100644 index 0000000..bfc0e56 --- /dev/null +++ b/tests/Fixtures/First.php @@ -0,0 +1,23 @@ +withHeader('X-First', 'yes'); + } +} diff --git a/tests/Fixtures/Guarded.php b/tests/Fixtures/Guarded.php new file mode 100644 index 0000000..baf4661 --- /dev/null +++ b/tests/Fixtures/Guarded.php @@ -0,0 +1,20 @@ + */ + public static array $steps = []; + + public static function reset(): void + { + self::$steps = []; + } +} diff --git a/tests/Unit/AuthPrimitivesTest.php b/tests/Unit/AuthPrimitivesTest.php new file mode 100644 index 0000000..d836ccd --- /dev/null +++ b/tests/Unit/AuthPrimitivesTest.php @@ -0,0 +1,188 @@ +request(['Cookie' => 'session=abc; theme=dark']); + + $this->assertSame('abc', $request->cookie('session')); + $this->assertSame('dark', $request->cookie('theme')); + $this->assertNull($request->cookie('nothing')); + } + + public function test_a_request_with_no_cookies_reads_none(): void + { + $this->assertNull($this->request()->cookie('session')); + } + + public function test_a_bearer_token_is_read(): void + { + $this->assertSame('abc123', $this->request(['Authorization' => 'Bearer abc123'])->bearerToken()); + $this->assertSame('abc123', $this->request(['Authorization' => 'bearer abc123'])->bearerToken()); + } + + /** + * Another scheme has different rules, and treating it as a token would + * accept credentials meant for something else entirely. + */ + #[DataProvider('notBearer')] + public function test_anything_that_is_not_a_bearer_token_is_not_read(string $header): void + { + $this->assertNull($this->request(['Authorization' => $header])->bearerToken()); + } + + /** + * @return array + */ + public static function notBearer(): array + { + return [ + 'basic auth' => ['Basic dXNlcjpwYXNz'], + 'no scheme' => ['abc123'], + 'empty' => [''], + 'bearer with nothing after it' => ['Bearer'], + 'bearer with a space in the token' => ['Bearer abc 123'], + ]; + } + + public function test_a_cookie_goes_out_with_safe_defaults(): void + { + $header = new Cookie('session', 'abc')->header(); + + $this->assertStringContainsString('session=abc', $header); + $this->assertStringContainsString('HttpOnly', $header); + $this->assertStringContainsString('SameSite=Lax', $header); + $this->assertStringContainsString('Path=/', $header); + } + + /** + * Left to the caller: a bot behind a plain-HTTP proxy would otherwise set a + * cookie the browser never sends back, which is a bug that looks like a + * login loop. + */ + public function test_secure_is_not_set_unless_asked_for(): void + { + $this->assertStringNotContainsString('Secure', new Cookie('a', 'b')->header()); + $this->assertStringContainsString('Secure', new Cookie('a', 'b', secure: true)->header()); + } + + public function test_a_value_is_encoded(): void + { + $this->assertStringContainsString('a=b%20c%3Bd', new Cookie('a', 'b c;d')->header()); + } + + public function test_forgetting_a_cookie_expires_it(): void + { + $header = Cookie::forget('session')->header(); + + $this->assertStringContainsString('session=', $header); + $this->assertStringContainsString('Max-Age=0', $header); + } + + public function test_same_site_can_be_tightened(): void + { + $this->assertStringContainsString( + 'SameSite=Strict', + new Cookie('a', 'b', sameSite: SameSite::Strict)->header(), + ); + } + + /** + * Set-Cookie is the one header that may legitimately appear more than once. + */ + public function test_two_cookies_are_sent_as_two_headers(): void + { + $react = Response::noContent() + ->withCookie(new Cookie('a', '1')) + ->withCookie(new Cookie('b', '2')) + ->toReact(); + + $this->assertCount(2, $react->getHeader('Set-Cookie')); + } + + public function test_an_answer_can_carry_a_header(): void + { + $response = Response::noContent()->withHeader('X-Trace', 'abc'); + + $this->assertSame('abc', $response->toReact()->getHeaderLine('X-Trace')); + } + + public function test_a_matching_secret_is_accepted(): void + { + $this->assertTrue(Secret::matches('s3cret', 's3cret')); + $this->assertFalse(Secret::matches('s3cret', 's3cres')); + } + + /** + * An unset secret must never match, or a bot with no secret configured + * accepts every request that sends an empty one. + */ + public function test_an_empty_secret_matches_nothing(): void + { + $this->assertFalse(Secret::matches('', '')); + $this->assertFalse(Secret::matches('', 'anything')); + } + + public function test_a_correctly_signed_body_is_accepted(): void + { + $body = '{"event":"ping"}'; + $signature = hash_hmac('sha256', $body, 'shared'); + + $this->assertTrue(Secret::signed($body, $signature, 'shared')); + } + + public function test_a_body_that_was_tampered_with_is_refused(): void + { + $signature = hash_hmac('sha256', '{"amount":1}', 'shared'); + + $this->assertFalse(Secret::signed('{"amount":1000}', $signature, 'shared')); + } + + public function test_a_signature_under_another_secret_is_refused(): void + { + $body = '{"event":"ping"}'; + + $this->assertFalse(Secret::signed($body, hash_hmac('sha256', $body, 'theirs'), 'ours')); + } + + public function test_a_missing_signature_or_secret_is_refused(): void + { + $this->assertFalse(Secret::signed('body', '', 'shared')); + $this->assertFalse(Secret::signed('body', 'anything', '')); + } +} diff --git a/tests/Unit/MiddlewareTest.php b/tests/Unit/MiddlewareTest.php new file mode 100644 index 0000000..738d4ac --- /dev/null +++ b/tests/Unit/MiddlewareTest.php @@ -0,0 +1,127 @@ +logger = new RecordingLogger(); + } + + private function router(string ...$classes): Router + { + $container = new GenericContainer(); + $container->singleton(Logger::class, $this->logger); + + $router = new Router($container); + $compiler = new RouteCompiler(); + + foreach ($classes as $class) { + $reflector = new ClassReflector($class); + + foreach ($reflector->getAttributes(Route::class) as $route) { + $router->add($compiler->compile($reflector, $route)); + } + } + + return $router; + } + + private function get(Router $router, string $path): Response + { + return $router->handle(new ServerRequest('GET', 'http://bot.test' . $path)); + } + + /** + * The first one listed is outermost, which is the order anyone reading the + * attribute expects — and the order that matters when one of them decides + * whether the caller may be here at all. + */ + public function test_middleware_run_outermost_first_and_unwind(): void + { + $response = $this->get($this->router(Guarded::class), '/guarded'); + + $this->assertSame('through', $response->body); + $this->assertSame( + ['first:before', 'second:before', 'handler', 'first:after'], + Trail::$steps, + ); + } + + public function test_middleware_may_change_the_answer_on_the_way_out(): void + { + $response = $this->get($this->router(Guarded::class), '/guarded'); + + $this->assertSame('yes', $response->toReact()->getHeaderLine('X-First')); + } + + /** + * The whole point. A handler that is never reached cannot half-do the work + * it was asked for, which is what makes this the right place for a check on + * whether the caller may ask at all. + */ + public function test_middleware_answering_stops_the_handler_running(): void + { + $response = $this->get($this->router(Refused::class), '/refused'); + + $this->assertSame(401, $response->status); + $this->assertSame(['refused'], Trail::$steps); + } + + /** + * And it stops the ones after it too, not just the handler. + */ + public function test_middleware_answering_stops_the_ones_behind_it(): void + { + $this->get($this->router(Refused::class), '/refused'); + + $this->assertNotContains('second:before', Trail::$steps); + } + + public function test_a_route_without_middleware_is_unaffected(): void + { + $this->assertSame(200, $this->get($this->router(Health::class), '/health')->status); + $this->assertSame([], Trail::$steps); + } + + /** + * Named on a route but not actually middleware: a 500 with the reason in + * the log, rather than a call to something that has no idea what $next is. + */ + public function test_something_that_is_not_middleware_is_refused(): void + { + $response = $this->get($this->router(Mislabelled::class), '/mislabelled'); + + $this->assertSame(500, $response->status); + $this->assertTrue($this->logger->has('must implement')); + } +} diff --git a/tests/Unit/RouterTest.php b/tests/Unit/RouterTest.php index 5c7f0f0..342b267 100644 --- a/tests/Unit/RouterTest.php +++ b/tests/Unit/RouterTest.php @@ -47,7 +47,7 @@ private function router(string ...$classes): Router $container = new GenericContainer(); $container->singleton(Logger::class, $this->logger); - $router = new Router($container, $this->logger); + $router = new Router($container); $compiler = new RouteCompiler(); foreach ($classes as $class) { @@ -223,6 +223,32 @@ public function test_a_path_that_could_never_match_is_refused(): void $this->router(Relative::class); } + /** + * Discovery builds the router while the container is still being assembled, + * before the initializers that provide the logger have themselves been + * found. Asking for one in the constructor made every application using + * this plugin fail to boot — including ones with no routes at all. + */ + public function test_it_can_be_built_before_the_container_has_a_logger(): void + { + $router = new Router(new GenericContainer()); + + $this->assertSame([], $router->all()); + $this->assertSame(404, $this->get($router, '/anything')->status); + } + + /** + * And the logger is still there when something actually goes wrong. + */ + public function test_a_failure_is_logged_once_the_container_can_provide_one(): void + { + $router = $this->router(Boom::class); + + $this->get($router, '/boom'); + + $this->assertTrue($this->logger->has('the database is on fire')); + } + public function test_it_lists_what_it_serves(): void { $this->assertCount(2, $this->router(Health::class, MyGuild::class)->all());