fix(security)!: check the csrf token after the route's own middleware

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,

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');

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

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
 

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;

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

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": {

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,

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;