diff --git a/packages/admin-panel/src/Controller/LoginController.php b/packages/admin-panel/src/Controller/LoginController.php index 7d6a239c9..58afbf473 100644 --- a/packages/admin-panel/src/Controller/LoginController.php +++ b/packages/admin-panel/src/Controller/LoginController.php @@ -12,16 +12,21 @@ use Marko\Routing\Attributes\Get; use Marko\Routing\Attributes\Middleware; use Marko\Routing\Attributes\Post; +use Marko\Routing\Attributes\WithoutMiddleware; use Marko\Routing\Http\Request; use Marko\Routing\Http\Response; use Marko\Security\Contracts\CsrfTokenManagerInterface; +use Marko\Security\Middleware\CsrfMiddleware; +use Marko\Security\Middleware\RouteCsrfMiddleware; use Marko\View\ViewInterface; /** * Logs admins in and out on the admin guard (admin-auth.guard), never the * app's default guard, so only AdminUserProvider users can sign in here. - * The login and logout POST routes are CSRF-protected by the global - * CsrfMiddleware that marko/security registers. + * The logout POST is CSRF-protected by the global CsrfMiddleware that + * marko/security registers. The login POST checks the token with + * RouteCsrfMiddleware ahead of the rate limiter instead, so a forged + * cross-site login cannot use up a client's attempts. * * Login is throttled twice: the admin guard's SessionGuard locks out an * email for a client after repeated failures (authentication.throttle), and @@ -52,7 +57,8 @@ public function showLoginForm( } #[Post(path: '/admin/login')] - #[Middleware(RateLimitMiddleware::class)] + #[Middleware([RouteCsrfMiddleware::class, RateLimitMiddleware::class])] + #[WithoutMiddleware(CsrfMiddleware::class)] #[RateLimit(maxAttempts: 10, decaySeconds: 60, name: 'admin-login')] public function authenticate( Request $request, diff --git a/packages/admin-panel/tests/Unit/Controller/LoginControllerTest.php b/packages/admin-panel/tests/Unit/Controller/LoginControllerTest.php index 7b494fce6..0a9d3b5c4 100644 --- a/packages/admin-panel/tests/Unit/Controller/LoginControllerTest.php +++ b/packages/admin-panel/tests/Unit/Controller/LoginControllerTest.php @@ -8,16 +8,27 @@ use Marko\AdminPanel\Controller\LoginController; use Marko\AdminPanel\Tests\Fixtures\FixedAdminGuardResolver; use Marko\Authentication\Exceptions\TooManyLoginAttemptsException; +use Marko\Core\Container\Container; +use Marko\Core\Container\PreferenceRegistry; use Marko\RateLimiter\Attributes\RateLimit; +use Marko\RateLimiter\Config\RateLimiterConfig; +use Marko\RateLimiter\Contracts\RateLimiterInterface; +use Marko\RateLimiter\Contracts\RateLimitKeyResolverInterface; use Marko\RateLimiter\Middleware\RateLimitMiddleware; +use Marko\RateLimiter\RateLimitResult; use Marko\Routing\Attributes\Middleware; use Marko\Routing\Attributes\WithoutMiddleware; use Marko\Routing\Http\Request; use Marko\Routing\Http\Response; +use Marko\Routing\RouteCollection; +use Marko\Routing\RouteDiscovery; +use Marko\Routing\RouteMatcher; +use Marko\Routing\Router; use Marko\Security\Contracts\CsrfTokenManagerInterface; use Marko\Security\Exceptions\CsrfTokenMismatchException; use Marko\Security\Middleware\CsrfMiddleware; use Marko\Session\Config\SessionConfig; +use Marko\Session\Contracts\SessionInterface; use Marko\Testing\Fake\FakeAuthenticatable; use Marko\Testing\Fake\FakeConfigRepository; use Marko\Testing\Fake\FakeGuard; @@ -115,24 +126,91 @@ function loginCsrfMiddleware(): CsrfMiddleware ); } -it('relies on the global CsrfMiddleware from marko/security for the login and logout routes', function (): void { - $securityRoot = dirname((string) new ReflectionClass(CsrfMiddleware::class)->getFileName(), 3); - $securityModule = require $securityRoot . '/module.php'; +class CountingRateLimiter implements RateLimiterInterface +{ + public int $attempts = 0; - expect($securityModule['globalMiddleware'])->toContain(CsrfMiddleware::class); + public function attempt( + string $key, + int $maxAttempts, + int $decaySeconds, + ): RateLimitResult { + $this->attempts++; + + return new RateLimitResult(allowed: true, remaining: $maxAttempts - $this->attempts); + } + + public function tooManyAttempts( + string $key, + int $maxAttempts, + ): bool { + return false; + } + + public function clear( + string $key, + ): void {} +} - foreach (['authenticate', 'logout'] as $action) { - $method = new ReflectionMethod(LoginController::class, $action); - $routeMiddleware = array_merge(...array_map( - fn (ReflectionAttribute $attribute): array => $attribute->newInstance()->middleware, - $method->getAttributes(Middleware::class), - )); +it('answers a forged login POST with 419 before it uses up a rate limit attempt', function (): void { + $routes = new RouteCollection(); - expect($routeMiddleware)->not->toContain(CsrfMiddleware::class) - ->and($method->getAttributes(WithoutMiddleware::class))->toBeEmpty(); + foreach (new RouteDiscovery()->discoverFromClass(LoginController::class) as $route) { + $routes->add($route); } - expect(new ReflectionClass(LoginController::class)->getAttributes(WithoutMiddleware::class))->toBeEmpty(); + $session = new FakeSession(); + $session->start(); + $rateLimiter = new CountingRateLimiter(); + $container = new Container(new PreferenceRegistry()); + $container->instance(CsrfTokenManagerInterface::class, new LoginStubCsrfTokenManager('test-csrf-token')); + $container->instance(SessionInterface::class, $session); + $container->instance(SessionConfig::class, new SessionConfig(new FakeConfigRepository([ + 'session.cookie.name' => 'marko_session', + 'session.cookie.path' => '/', + 'session.cookie.domain' => '', + 'session.cookie.secure' => true, + ]))); + $container->instance(RateLimitMiddleware::class, new RateLimitMiddleware( + rateLimiter: $rateLimiter, + rateLimitKeyResolver: new class () implements RateLimitKeyResolverInterface + { + public function resolve( + Request $request, + ): string { + return '203.0.113.7'; + } + }, + rateLimiterConfig: new RateLimiterConfig(new FakeConfigRepository()), + )); + $router = new Router( + matcher: new RouteMatcher($routes), + container: $container, + globalMiddleware: [CsrfMiddleware::class], + ); + + $response = $router->handle(new Request( + server: ['REQUEST_METHOD' => 'POST', 'REQUEST_URI' => '/admin/login', 'HTTP_ACCEPT' => 'application/json'], + post: ['email' => 'kira@example.ch', 'password' => 'guess', '_token' => 'forged'], + )); + + expect($response->statusCode())->toBe(419) + ->and($rateLimiter->attempts)->toBe(0); +}); + +it('relies on the global CsrfMiddleware from marko/security for the logout route', function (): void { + $securityRoot = dirname((string) new ReflectionClass(CsrfMiddleware::class)->getFileName(), 3); + $securityModule = require $securityRoot . '/module.php'; + $method = new ReflectionMethod(LoginController::class, 'logout'); + $routeMiddleware = array_merge(...array_map( + fn (ReflectionAttribute $attribute): array => $attribute->newInstance()->middleware, + $method->getAttributes(Middleware::class), + )); + + expect($securityModule['globalMiddleware'])->toContain(CsrfMiddleware::class) + ->and($routeMiddleware)->not->toContain(CsrfMiddleware::class) + ->and($method->getAttributes(WithoutMiddleware::class))->toBeEmpty() + ->and(new ReflectionClass(LoginController::class)->getAttributes(WithoutMiddleware::class))->toBeEmpty(); }); it('rejects a POST to the admin login route when no CSRF token is supplied', function (): void { @@ -296,7 +374,7 @@ function loginCsrfMiddleware(): CsrfMiddleware $middleware = $method->getAttributes(Middleware::class)[0]->newInstance(); $rateLimit = $method->getAttributes(RateLimit::class)[0]->newInstance(); - expect($middleware->middleware)->toBe([RateLimitMiddleware::class]) + expect($middleware->middleware)->toContain(RateLimitMiddleware::class) ->and($rateLimit->maxAttempts)->toBe(10) ->and($rateLimit->decaySeconds)->toBe(60) ->and($rateLimit->name)->toBe('admin-login'); diff --git a/packages/docs-markdown/docs/packages/admin-panel.md b/packages/docs-markdown/docs/packages/admin-panel.md index 7863783d6..6df377ed6 100644 --- a/packages/docs-markdown/docs/packages/admin-panel.md +++ b/packages/docs-markdown/docs/packages/admin-panel.md @@ -26,7 +26,7 @@ The panel registers these routes automatically: | POST | `/admin/logout` | Logout (requires valid CSRF token) | | GET | `/admin` | Dashboard (requires auth; lists only the sections the admin user can access) | -The login and logout `POST` routes are protected by the global `CsrfMiddleware` that [`marko/security`](/docs/packages/security/) registers. The login form receives a CSRF token via the `csrfToken` template variable; the template must include it as a hidden field named `_token`. Requests without a valid token are rejected before authentication logic runs. +The logout `POST` route is protected by the global `CsrfMiddleware` that [`marko/security`](/docs/packages/security/) registers. The login `POST` route lists `RouteCsrfMiddleware` ahead of its rate limiter instead, so a forged cross-site login is rejected before it uses up an attempt. The login form receives a CSRF token via the `csrfToken` template variable; the template must include it as a hidden field named `_token`. Requests without a valid token are rejected before authentication logic runs. Login is throttled in two layers. The admin guard is a `SessionGuard`, so [login throttling](/docs/packages/authentication/#login-throttling) locks out an email for a client after repeated failures. A locked-out attempt re-renders the login form with an error, status `429` and a `Retry-After` header. On top of that, `#[RateLimit(maxAttempts: 10, decaySeconds: 60, name: 'admin-login')]` with [`RateLimitMiddleware`](/docs/packages/ratelimiter/) caps login `POST`s per client IP across all emails. Both need a cache driver. @@ -140,10 +140,11 @@ use Marko\Routing\Attributes\Get; use Marko\Routing\Attributes\Post; #[Get(path: '/admin/login')] // showLoginForm -#[Post(path: '/admin/login')] // authenticate — CSRF required (global CsrfMiddleware) -#[Middleware(RateLimitMiddleware::class)] // 10 login POSTs per minute per client IP +#[Post(path: '/admin/login')] // authenticate: CSRF required (RouteCsrfMiddleware) +#[Middleware([RouteCsrfMiddleware::class, RateLimitMiddleware::class])] // token checked first, then 10 login POSTs per minute per client IP +#[WithoutMiddleware(CsrfMiddleware::class)] // the global check is replaced, not doubled #[RateLimit(maxAttempts: 10, decaySeconds: 60, name: 'admin-login')] -#[Post(path: '/admin/logout')] // logout — CSRF required (global CsrfMiddleware) +#[Post(path: '/admin/logout')] // logout: CSRF required (global CsrfMiddleware) ``` ### DashboardController Routes diff --git a/packages/docs-markdown/docs/packages/layout.md b/packages/docs-markdown/docs/packages/layout.md index 88813bb56..097f55075 100644 --- a/packages/docs-markdown/docs/packages/layout.md +++ b/packages/docs-markdown/docs/packages/layout.md @@ -158,7 +158,7 @@ class NoStoreMiddleware implements MiddlewareInterface } ``` -Global middleware (sessions, CSRF, security headers, page cache) still wraps everything, including the layout. +Global middleware (sessions, security headers, page cache) still wraps everything, including the layout. `CsrfMiddleware` from [`marko/security`](/docs/packages/security/) is also `#[RunsInnermost]`: it runs after your route middleware but still wraps the layout. ## Customization diff --git a/packages/docs-markdown/docs/packages/routing.md b/packages/docs-markdown/docs/packages/routing.md index b42200122..1bd3b4a0d 100644 --- a/packages/docs-markdown/docs/packages/routing.md +++ b/packages/docs-markdown/docs/packages/routing.md @@ -449,7 +449,7 @@ If a route excludes middleware that is neither global nor on the route, boot fai ### Innermost Global Middleware -Global middleware normally wraps every route middleware. A global middleware that turns the controller's result into the final response (layout rendering in [`marko/layout`](/docs/packages/layout/), for example) needs the opposite: it should run directly around the controller, so route middleware sees, and can decorate, the final response. Mark it `#[RunsInnermost]`: +Global middleware normally wraps every route middleware. A global middleware that turns the controller's result into the final response (layout rendering in [`marko/layout`](/docs/packages/layout/), for example) needs the opposite: it should run directly around the controller, so route middleware sees, and can decorate, the final response. The same holds for a check that should only reach requests the route's own middleware let through, such as `CsrfMiddleware` in [`marko/security`](/docs/packages/security/). Mark it `#[RunsInnermost]`: ```php title="app/web/src/Http/Middleware/WrapInFrameMiddleware.php" use Marko\Routing\Attributes\RunsInnermost; diff --git a/packages/docs-markdown/docs/packages/security.md b/packages/docs-markdown/docs/packages/security.md index f6d276a58..03f5fa62e 100644 --- a/packages/docs-markdown/docs/packages/security.md +++ b/packages/docs-markdown/docs/packages/security.md @@ -59,6 +59,23 @@ class FormController The middleware reads the token from the `_token` form field, the `X-CSRF-TOKEN` header, or the `X-XSRF-TOKEN` header. Safe methods (GET, HEAD, OPTIONS) are skipped. +It carries `#[RunsInnermost]`, so it checks the token after the route's own middleware (see [Innermost global middleware](/docs/packages/routing/#innermost-global-middleware)). An auth guard on the route answers a logged out visitor's stale form with its login redirect, not a `419`. When that middleware logs the visitor in, for example from a remember-me cookie, the login rotates the token, and the token the request arrived with is still accepted for that one request. + +Route middleware therefore also sees requests that end in a `419`. A route that needs the token checked before its own middleware, for a rate limiter that should only count verified requests, excludes the global check and lists `RouteCsrfMiddleware` where the check belongs. The admin panel's login route does this: + +```php +use Marko\Security\Middleware\CsrfMiddleware; +use Marko\Security\Middleware\RouteCsrfMiddleware; + +#[Middleware([RouteCsrfMiddleware::class, RateLimitMiddleware::class])] +#[WithoutMiddleware(CsrfMiddleware::class)] +class ContactController +``` + +`#[WithoutMiddleware]` removes middleware by exact class, so `RouteCsrfMiddleware`, a subclass, stays. A `#[Preference]` that replaces `CsrfMiddleware` does not reach `RouteCsrfMiddleware`, so such a route, the admin login included, keeps the shipped check; prefer `RouteCsrfMiddleware` as well if you change how the token is read. + +A response that route middleware returns before reaching `CsrfMiddleware`, such as an auth redirect, does not carry a refreshed `XSRF-TOKEN` cookie. The next response that passes through it does. + It never runs on requests that match no route, because it does not carry `#[RunsOnUnmatched]` (see [Which middleware runs](/docs/packages/routing/#which-middleware-runs)). A `POST` to an unknown path gets a `404`, and a `POST` to a GET-only path gets a `405` with `Allow` --- never a `419` token mismatch. :::caution[Upgrading from per-route CSRF] @@ -167,7 +184,7 @@ In earlier releases, `SecurityHeadersMiddleware` only ran on routes that declare ### Token Rotation on Login and Logout -When [marko/authentication](/docs/packages/authentication/) is installed, the token is regenerated on every `LoginEvent` and `LogoutEvent` (by the `RotateCsrfTokenOnLogin` and `RotateCsrfTokenOnLogout` observers). A token issued before login --- including one an attacker learned by planting a session from a sibling subdomain --- stops validating once the victim logs in, and the next person on a shared machine does not inherit the logged-out user's token. Forms rendered before the login or logout must be reloaded to pick up the new token. Without marko/authentication these events are never dispatched, so nothing changes. +When [marko/authentication](/docs/packages/authentication/) is installed, the token is regenerated on every `LoginEvent` and `LogoutEvent` (by the `RotateCsrfTokenOnLogin` and `RotateCsrfTokenOnLogout` observers). A token issued before login --- including one an attacker learned by planting a session from a sibling subdomain --- stops validating on every request after the one that logged the victim in, and the next person on a shared machine does not inherit the logged-out user's token. Forms rendered before the login or logout must be reloaded to pick up the new token. Without marko/authentication these events are never dispatched, so nothing changes. ### Using the CSRF Token Manager Directly @@ -228,7 +245,7 @@ use Marko\Security\Exceptions\CsrfTokenMismatchException; use Marko\Security\Contracts\CsrfTokenManagerInterface; public function get(): string; // Get token, generating one if none exists -public function validate(string $token): bool; // Validate against stored token +public function validate(string $token): bool; // Validate against the stored token, or the one this request arrived with public function regenerate(): string; // Regenerate token, replacing the previous one ``` @@ -247,6 +264,14 @@ public function __construct( public function handle(Request $request, callable $next): Response; ``` +### RouteCsrfMiddleware + +```php +use Marko\Security\Middleware\RouteCsrfMiddleware; + +class RouteCsrfMiddleware extends CsrfMiddleware {} // the same check, at the position a route lists it +``` + ### CsrfTokenManager ```php diff --git a/packages/security/composer.json b/packages/security/composer.json index e40a79a5d..da3dfe4eb 100644 --- a/packages/security/composer.json +++ b/packages/security/composer.json @@ -14,6 +14,7 @@ "require-dev": { "marko/authentication": "self.version", "marko/cors": "self.version", + "marko/layout": "self.version", "marko/testing": "self.version" }, "suggest": { diff --git a/packages/security/module.php b/packages/security/module.php index a84dd9f88..ba1627475 100644 --- a/packages/security/module.php +++ b/packages/security/module.php @@ -13,9 +13,13 @@ return [ // CsrfMiddleware reads the token from the session, so SessionMiddleware - // (registered globally by the session drivers) must run first. + // (registered globally by the session drivers) must run first. Loading + // before marko/layout keeps CsrfMiddleware around LayoutMiddleware when + // both run innermost, so a token a layout component issues is mirrored to + // the XSRF-TOKEN cookie. 'sequence' => [ 'after' => ['marko/session-file', 'marko/session-database'], + 'before' => ['marko/layout'], ], 'bindings' => [ CsrfTokenManagerInterface::class => function (ContainerInterface $container): CsrfTokenManagerInterface { @@ -26,11 +30,16 @@ ); }, ], + 'singletons' => [ + // Shared, so CsrfMiddleware still accepts the token a request arrived + // with after a login in route middleware rotated it. + CsrfTokenManagerInterface::class, + ], // Every matched route is protected; opt a route out with // #[WithoutMiddleware(CsrfMiddleware::class)] or // #[WithoutMiddleware(SecurityHeadersMiddleware::class)]. - // SecurityHeadersMiddleware comes first so it wraps CsrfMiddleware and - // also decorates the 419 a token mismatch renders. + // CsrfMiddleware runs innermost (#[RunsInnermost]), so SecurityHeadersMiddleware + // wraps it and also decorates the 419 a token mismatch renders. 'globalMiddleware' => [ SecurityHeadersMiddleware::class, CsrfMiddleware::class, diff --git a/packages/security/src/Contracts/CsrfTokenManagerInterface.php b/packages/security/src/Contracts/CsrfTokenManagerInterface.php index 245b151e1..e59343902 100644 --- a/packages/security/src/Contracts/CsrfTokenManagerInterface.php +++ b/packages/security/src/Contracts/CsrfTokenManagerInterface.php @@ -12,7 +12,11 @@ interface CsrfTokenManagerInterface public function get(): string; /** - * Validate a submitted token against the stored token. + * Validate a submitted token for the current request. + * + * CsrfMiddleware checks after the route's own middleware, so a login there + * may already have rotated the token: accept the token the request arrived + * with too, until the request ends. */ public function validate(string $token): bool; diff --git a/packages/security/src/CsrfTokenManager.php b/packages/security/src/CsrfTokenManager.php index d09787eb1..c86df5ea5 100644 --- a/packages/security/src/CsrfTokenManager.php +++ b/packages/security/src/CsrfTokenManager.php @@ -4,12 +4,22 @@ namespace Marko\Security; +use Marko\Core\Contracts\ResettableInterface; use Marko\Encryption\Contracts\EncryptorInterface; use Marko\Security\Contracts\CsrfTokenManagerInterface; use Marko\Session\Contracts\SessionInterface; +use Override; -class CsrfTokenManager implements CsrfTokenManagerInterface +class CsrfTokenManager implements CsrfTokenManagerInterface, ResettableInterface { + /** + * The token the session held before this request first wrote one, kept + * once a write replaced it. + */ + private ?string $arrivedWith = null; + + private bool $hasWritten = false; + /** * @param string $sessionKey Session key holding the token (security.csrf.session_key) */ @@ -30,16 +40,21 @@ public function get(): string return $this->generateToken(); } + /** + * Also accepts the token the request arrived with after a rotation in the + * same request: CsrfMiddleware checks after the route's middleware, and a + * remember-me login there rotates the token before the check. + */ public function validate( string $token, ): bool { $stored = $this->session->get($this->sessionKey); - if ($stored === null) { - return false; + if ($stored !== null && hash_equals((string) $stored, $token)) { + return true; } - return hash_equals((string) $stored, $token); + return $this->arrivedWith !== null && hash_equals($this->arrivedWith, $token); } public function regenerate(): string @@ -47,8 +62,21 @@ public function regenerate(): string return $this->generateToken(); } + #[Override] + public function reset(): void + { + $this->arrivedWith = null; + $this->hasWritten = false; + } + private function generateToken(): string { + if (!$this->hasWritten) { + $existing = $this->session->get($this->sessionKey); + $this->arrivedWith = $existing !== null ? (string) $existing : null; + $this->hasWritten = true; + } + $randomBytes = random_bytes(32); $token = $this->encryptor->encrypt($randomBytes); diff --git a/packages/security/src/Middleware/CsrfMiddleware.php b/packages/security/src/Middleware/CsrfMiddleware.php index 56a791b80..f7b6acb1c 100644 --- a/packages/security/src/Middleware/CsrfMiddleware.php +++ b/packages/security/src/Middleware/CsrfMiddleware.php @@ -4,6 +4,7 @@ namespace Marko\Security\Middleware; +use Marko\Routing\Attributes\RunsInnermost; use Marko\Routing\Exceptions\CookieException; use Marko\Routing\Http\Cookie; use Marko\Routing\Http\Request; @@ -24,7 +25,12 @@ * or the `X-XSRF-TOKEN` header. Whenever the response persists the session, * the current token is mirrored to a readable `XSRF-TOKEN` cookie so SPA * clients (axios, Inertia) can send it back automatically. + * + * It runs innermost, after the route's own middleware, so an auth guard + * answers a logged out visitor's stale form with its login redirect instead + * of a token error. */ +#[RunsInnermost] class CsrfMiddleware implements MiddlewareInterface { public const string COOKIE_NAME = 'XSRF-TOKEN'; diff --git a/packages/security/src/Middleware/RouteCsrfMiddleware.php b/packages/security/src/Middleware/RouteCsrfMiddleware.php new file mode 100644 index 000000000..1125f713a --- /dev/null +++ b/packages/security/src/Middleware/RouteCsrfMiddleware.php @@ -0,0 +1,17 @@ + $sessionCookieConfig + * @param class-string $middlewareClass */ public static function createCsrfMiddleware( CsrfTokenManagerInterface $tokenManager, ?SessionInterface $session = null, array $sessionCookieConfig = [], + string $middlewareClass = CsrfMiddleware::class, ): CsrfMiddleware { - return new CsrfMiddleware( + return new $middlewareClass( tokenManager: $tokenManager, session: $session ?? self::createResumedSession(), sessionConfig: new SessionConfig(new FakeConfigRepository(array_merge([ diff --git a/packages/security/tests/Unit/CsrfGlobalMiddlewareTest.php b/packages/security/tests/Unit/CsrfGlobalMiddlewareTest.php index 957f50aef..365197b7c 100644 --- a/packages/security/tests/Unit/CsrfGlobalMiddlewareTest.php +++ b/packages/security/tests/Unit/CsrfGlobalMiddlewareTest.php @@ -8,12 +8,14 @@ use Marko\Core\Container\PreferenceRegistry; use Marko\Routing\Http\Request; use Marko\Routing\Http\Response; +use Marko\Routing\Middleware\MiddlewareInterface; use Marko\Routing\RouteCollection; use Marko\Routing\RouteDefinition; use Marko\Routing\RouteMatcher; use Marko\Routing\Router; use Marko\Security\Contracts\CsrfTokenManagerInterface; use Marko\Security\Middleware\CsrfMiddleware; +use Marko\Security\Middleware\RouteCsrfMiddleware; use Marko\Security\Tests\Helpers; class GlobalStoredTokenManager implements CsrfTokenManagerInterface @@ -56,9 +58,33 @@ public function webhook(): Response } } +/** + * Stands in for an auth guard: a visitor whose session expired is sent to + * the login page. + */ +class RedirectGuestMiddleware implements MiddlewareInterface +{ + public function handle( + Request $request, + callable $next, + ): Response { + return Response::redirect('/login'); + } +} + +class PassThroughMiddleware implements MiddlewareInterface +{ + public function handle( + Request $request, + callable $next, + ): Response { + return $next($request); + } +} + /** * A router wired the way marko/security's module.php wires it: CsrfMiddleware - * is global and no route opts in with #[Middleware]. + * is global and no route names it in #[Middleware]. */ function globalCsrfRouter(): Router { @@ -82,9 +108,35 @@ function globalCsrfRouter(): Router action: 'webhook', withoutMiddleware: [CsrfMiddleware::class], )); + $routes->add(new RouteDefinition( + method: 'POST', + path: '/admin/settings', + controller: GlobalCsrfController::class, + action: 'update', + middleware: [RedirectGuestMiddleware::class], + )); + $routes->add(new RouteDefinition( + method: 'POST', + path: '/profile/name', + controller: GlobalCsrfController::class, + action: 'update', + middleware: [PassThroughMiddleware::class], + )); + $routes->add(new RouteDefinition( + method: 'POST', + path: '/contact', + controller: GlobalCsrfController::class, + action: 'update', + middleware: [RouteCsrfMiddleware::class, RedirectGuestMiddleware::class], + withoutMiddleware: [CsrfMiddleware::class], + )); $container = new Container(new PreferenceRegistry()); $container->instance(CsrfMiddleware::class, Helpers::createCsrfMiddleware(new GlobalStoredTokenManager())); + $container->instance(RouteCsrfMiddleware::class, Helpers::createCsrfMiddleware( + new GlobalStoredTokenManager(), + middlewareClass: RouteCsrfMiddleware::class, + )); $container->instance(GlobalCsrfController::class, new GlobalCsrfController()); return new Router( @@ -135,6 +187,32 @@ function globalPost( ->and($response->body())->toBe('Received'); }); + it('lets a route middleware answer a POST without a token before the token is checked', function (): void { + $response = globalCsrfRouter()->handle(globalPost('/admin/settings')); + + expect($response->statusCode())->toBe(302) + ->and($response->headers()['Location'] ?? null)->toBe('/login'); + }); + + it('rejects a POST without a token once the route middleware lets it through', function (): void { + $response = globalCsrfRouter()->handle(globalPost('/profile/name')); + + expect($response->statusCode())->toBe(419); + }); + + it('accepts a POST with a token on a route that has its own middleware', function (): void { + $response = globalCsrfRouter()->handle(globalPost('/profile/name', ['HTTP_X_CSRF_TOKEN' => 'stored-token'])); + + expect($response->statusCode())->toBe(200) + ->and($response->body())->toBe('Updated'); + }); + + it('checks the token first on a route that lists a CsrfMiddleware subclass', function (): void { + $response = globalCsrfRouter()->handle(globalPost('/contact')); + + expect($response->statusCode())->toBe(419); + }); + it('issues the XSRF-TOKEN cookie on a GET that resumes a session', function (): void { $response = globalCsrfRouter()->handle(new Request( server: ['REQUEST_METHOD' => 'GET', 'REQUEST_URI' => '/account'], diff --git a/packages/security/tests/Unit/CsrfRotationOnAuthTest.php b/packages/security/tests/Unit/CsrfRotationOnAuthTest.php index 7238d5764..44866b0b7 100644 --- a/packages/security/tests/Unit/CsrfRotationOnAuthTest.php +++ b/packages/security/tests/Unit/CsrfRotationOnAuthTest.php @@ -10,10 +10,19 @@ use Marko\Core\Event\EventDispatcher; use Marko\Core\Event\ObserverDefinition; use Marko\Core\Event\ObserverRegistry; +use Marko\Routing\Http\Request; +use Marko\Routing\Http\Response; +use Marko\Routing\Middleware\MiddlewareInterface; +use Marko\Routing\RouteCollection; +use Marko\Routing\RouteDefinition; +use Marko\Routing\RouteMatcher; +use Marko\Routing\Router; use Marko\Security\Contracts\CsrfTokenManagerInterface; use Marko\Security\CsrfTokenManager; +use Marko\Security\Middleware\CsrfMiddleware; use Marko\Security\Observer\RotateCsrfTokenOnLogin; use Marko\Security\Observer\RotateCsrfTokenOnLogout; +use Marko\Security\Tests\Helpers; use Marko\Testing\Fake\FakeAuthenticatable; use Marko\Testing\Fake\FakeEncryptor; use Marko\Testing\Fake\FakeSession; @@ -54,6 +63,37 @@ function csrfRotatingGuard( ); } +/** + * Logs the visitor in on the way to the controller, the way a remember-me + * cookie does when a route's auth middleware checks the guard. + */ +class LoginOnTheWayMiddleware implements MiddlewareInterface +{ + public function __construct( + private readonly SessionGuard $guard, + ) {} + + public function handle( + Request $request, + callable $next, + ): Response { + $this->guard->login(new FakeAuthenticatable(id: 42)); + + return $next($request); + } +} + +/** + * @noinspection PhpUnused - Action invoked by the router + */ +class RotationCsrfController +{ + public function update(): Response + { + return new Response('Updated'); + } +} + describe('CSRF token rotation on login and logout', function (): void { it('observes LoginEvent and LogoutEvent', function (): void { $loginObserver = new ReflectionClass(RotateCsrfTokenOnLogin::class) @@ -77,6 +117,47 @@ function csrfRotatingGuard( ->and($csrf->get())->not->toBe($preLoginToken); }); + it('accepts the token a POST arrived with when route middleware logs the visitor in', function (): void { + $session = Helpers::createResumedSession(); + $tokenFromThePage = csrfRotationManager($session)->get(); + $csrf = csrfRotationManager($session); + + $routes = new RouteCollection(); + $routes->add(new RouteDefinition( + method: 'POST', + path: '/account/email', + controller: RotationCsrfController::class, + action: 'update', + middleware: [LoginOnTheWayMiddleware::class], + )); + $container = new Container(); + $container->instance( + LoginOnTheWayMiddleware::class, + new LoginOnTheWayMiddleware(csrfRotatingGuard($session, $csrf)), + ); + $container->instance(CsrfMiddleware::class, Helpers::createCsrfMiddleware($csrf, $session)); + $container->instance(RotationCsrfController::class, new RotationCsrfController()); + $router = new Router( + matcher: new RouteMatcher($routes), + container: $container, + globalMiddleware: [CsrfMiddleware::class], + ); + + $response = $router->handle(new Request( + server: [ + 'REQUEST_METHOD' => 'POST', + 'REQUEST_URI' => '/account/email', + 'HTTP_ACCEPT' => 'application/json', + 'HTTP_X_CSRF_TOKEN' => $tokenFromThePage, + ], + cookies: Helpers::resumedSessionCookies(), + )); + + expect($response->statusCode())->toBe(200) + ->and(Helpers::findXsrfCookie($response)?->value())->toBe($csrf->get()) + ->and($csrf->get())->not->toBe($tokenFromThePage); + }); + it('invalidates the logged-in CSRF token on logout', function (): void { $session = new FakeSession(); $session->start(); diff --git a/packages/security/tests/Unit/CsrfTokenManagerTest.php b/packages/security/tests/Unit/CsrfTokenManagerTest.php index 2c93ed407..7e4c6a739 100644 --- a/packages/security/tests/Unit/CsrfTokenManagerTest.php +++ b/packages/security/tests/Unit/CsrfTokenManagerTest.php @@ -2,6 +2,7 @@ declare(strict_types=1); +use Marko\Core\Contracts\ResettableInterface; use Marko\Security\Contracts\CsrfTokenManagerInterface; use Marko\Security\CsrfTokenManager; use Marko\Security\Exceptions\CsrfTokenMismatchException; @@ -111,6 +112,61 @@ ->and($manager->get())->toBe($regenerated); }); + it('keeps accepting the token the request arrived with after a regeneration', function (): void { + $session = new FakeSession(); + $session->set('_csrf_token', 'token-from-the-page'); + $manager = new CsrfTokenManager(session: $session, encryptor: new FakeEncryptor()); + + $manager->regenerate(); + + expect($manager->validate('token-from-the-page'))->toBeTrue(); + }); + + it('accepts no token that was issued during the request and then replaced', function (): void { + $session = new FakeSession(); + $session->set('_csrf_token', 'token-from-the-page'); + $manager = new CsrfTokenManager(session: $session, encryptor: new FakeEncryptor()); + + $issuedDuringTheRequest = $manager->regenerate(); + $manager->regenerate(); + + expect($manager->validate($issuedDuringTheRequest))->toBeFalse(); + }); + + it('accepts no replaced token when the request arrived without one', function (): void { + $manager = new CsrfTokenManager(session: new FakeSession(), encryptor: new FakeEncryptor()); + + $issuedDuringTheRequest = $manager->get(); + $manager->regenerate(); + + expect($manager->validate($issuedDuringTheRequest))->toBeFalse(); + }); + + it('rejects the token the request arrived with once the manager is reset for the next request', function (): void { + $session = new FakeSession(); + $session->set('_csrf_token', 'token-from-the-page'); + $manager = new CsrfTokenManager(session: $session, encryptor: new FakeEncryptor()); + + $manager->regenerate(); + $manager->reset(); + + expect($manager)->toBeInstanceOf(ResettableInterface::class) + ->and($manager->validate('token-from-the-page'))->toBeFalse(); + }); + + it('remembers the token the next request arrived with after a reset', function (): void { + $session = new FakeSession(); + $session->set('_csrf_token', 'token-from-the-first-page'); + $manager = new CsrfTokenManager(session: $session, encryptor: new FakeEncryptor()); + $manager->regenerate(); + $manager->reset(); + + $session->set('_csrf_token', 'token-from-the-second-page'); + $manager->regenerate(); + + expect($manager->validate('token-from-the-second-page'))->toBeTrue(); + }); + it('implements CsrfTokenManagerInterface', function (): void { $session = new FakeSession(); $encryptor = new FakeEncryptor(); diff --git a/packages/security/tests/Unit/SecurityConfigTest.php b/packages/security/tests/Unit/SecurityConfigTest.php index bda76de7f..f838ffb02 100644 --- a/packages/security/tests/Unit/SecurityConfigTest.php +++ b/packages/security/tests/Unit/SecurityConfigTest.php @@ -2,9 +2,14 @@ declare(strict_types=1); +use Marko\Core\Container\BindingRegistry; use Marko\Core\Container\Container; use Marko\Core\Container\PreferenceRegistry; +use Marko\Core\Module\DependencyResolver; +use Marko\Core\Module\GlobalMiddlewareResolver; +use Marko\Core\Module\ModuleManifest; use Marko\Encryption\Contracts\EncryptorInterface; +use Marko\Layout\Middleware\LayoutMiddleware; use Marko\Security\Config\SecurityConfig; use Marko\Security\Contracts\CsrfTokenManagerInterface; use Marko\Security\Middleware\CsrfMiddleware; @@ -94,12 +99,50 @@ it('registers the security middleware globally after the session drivers', function (): void { $module = require dirname(__DIR__, 2) . '/module.php'; - // SecurityHeadersMiddleware is listed first so it wraps CsrfMiddleware - // and also decorates the 419 a token mismatch renders. expect($module['globalMiddleware'])->toBe([SecurityHeadersMiddleware::class, CsrfMiddleware::class]) ->and($module['sequence']['after'])->toContain('marko/session-file', 'marko/session-database'); }); + it('orders CsrfMiddleware before LayoutMiddleware in the global middleware', function (): void { + $manifest = fn (string $name, array $module): ModuleManifest => new ModuleManifest( + name: $name, + version: '1.0.0', + after: $module['sequence']['after'] ?? [], + before: $module['sequence']['before'] ?? [], + globalMiddleware: $module['globalMiddleware'] ?? [], + ); + + // Layout is listed first, as alphabetical discovery would, so only the sequence hint can produce the order. + $modules = new DependencyResolver()->resolve([ + $manifest('marko/layout', require dirname(__DIR__, 3) . '/layout/module.php'), + $manifest('marko/security', require dirname(__DIR__, 2) . '/module.php'), + ]); + $order = new GlobalMiddlewareResolver()->resolve($modules); + + expect(array_search(CsrfMiddleware::class, $order, true)) + ->toBeLessThan(array_search(LayoutMiddleware::class, $order, true)); + }); + + it('shares one CsrfTokenManager between CsrfMiddleware and the login rotation', function (): void { + $module = require dirname(__DIR__, 2) . '/module.php'; + $container = new Container(new PreferenceRegistry()); + $container->instance(SessionInterface::class, new FakeSession()); + $container->instance(EncryptorInterface::class, $this->createStub(EncryptorInterface::class)); + $container->instance(SecurityConfig::class, Helpers::createSecurityConfig([ + 'security.csrf.session_key' => '_csrf_token', + ])); + + new BindingRegistry($container)->registerModule(new ModuleManifest( + name: 'marko/security', + version: '1.0.0', + bindings: $module['bindings'], + singletons: $module['singletons'] ?? [], + )); + + expect($container->get(CsrfTokenManagerInterface::class)) + ->toBe($container->get(CsrfTokenManagerInterface::class)); + }); + it('builds CsrfTokenManager with the configured security.csrf.session_key', function (): void { $module = require dirname(__DIR__, 2) . '/module.php'; $session = new FakeSession();