Skip to content

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

Open
etienne-scherrer wants to merge 1 commit into
marko-php:developfrom
etienne-scherrer:fix/csrf-runs-innermost
Open

etienne-scherrer wants to merge 1 commit into
marko-php:developfrom
etienne-scherrer:fix/csrf-runs-innermost

Conversation

@etienne-scherrer

@etienne-scherrer etienne-scherrer commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

Problem

CsrfMiddleware is global, so it wraps every route middleware. When an admin's session has expired and they submit a form that was still open in a tab, the token no longer matches, and CsrfMiddleware answers 419 before the route's auth middleware can send them to the login page. The visitor sees a token error where a login redirect belongs.

An app can work around it today by giving each protected controller #[WithoutMiddleware(CsrfMiddleware::class)] and listing an empty CsrfMiddleware subclass after its auth middleware.

Change

  • CsrfMiddleware carries #[RunsInnermost] (from fix!: render layouts innermost so route middleware headers and cookies survive #522), so it checks the token after the route's own middleware. A route middleware that denies the request answers first.
  • Remember-me logins keep working. A login inside route middleware, such as a remember-me cookie on an auth route, rotates the token through RotateCsrfTokenOnLogin before the check now runs. CsrfTokenManager therefore also accepts the token the session held before the request first wrote one, for the rest of that request. That gives the same answer the check gave when it ran first. A token issued during the request is never accepted once replaced, and a pre-login token still fails on every later request.
  • CsrfTokenManagerInterface is now a singleton, so CsrfMiddleware and the login observer share one manager. CsrfTokenManager implements ResettableInterface, so a long-running worker forgets the remembered token between requests.
  • New RouteCsrfMiddleware: the same check at the position a route lists it. The admin panel login uses it ahead of its RateLimitMiddleware, so a forged cross-site login is rejected before it uses up a client's attempts.
  • marko/security loads before marko/layout. Both run innermost in module order, and module discovery is alphabetical, so without this LayoutMiddleware would wrap CsrfMiddleware, and a token issued while the layout renders (through csrf_field()) would miss the XSRF-TOKEN cookie on that response.
  • Docs: security.md describes the new position, RouteCsrfMiddleware, and that an early response from route middleware carries no refreshed XSRF-TOKEN. routing.md and layout.md no longer say CSRF wraps everything, admin-panel.md shows the login's RouteCsrfMiddleware, and the interface docblock states the new validate() contract.

SecurityHeadersMiddleware stays outer, so a 419 still carries the security headers.

Depends on #681: in a long-running worker the remembered token is cleared by RequestStateResetter, which today skips a resettable service hidden behind a plugin interface proxy. Without #681, a plugin on CsrfTokenManagerInterface would keep one request's token valid for later requests on the same worker.

Breaking

  • Route middleware now sees a state-changing request before its token is verified, so a route middleware with side effects also runs for requests that end in a 419. A route that needs the old order excludes the global check and lists RouteCsrfMiddleware first:

    #[Middleware([RouteCsrfMiddleware::class, RateLimitMiddleware::class])]
    #[WithoutMiddleware(CsrfMiddleware::class)]
  • A custom CsrfTokenManagerInterface implementation should also accept the token a request arrived with until that request ends, or remember-me logins on auth routes answer 419. A binding that replaces it is now resolved as a singleton.

Tests

Each of these failed against the code without its change, for the expected reason:

  • A POST without a token to a route whose middleware redirects gets the redirect (was 419).
  • A POST whose route middleware logs the visitor in is accepted with the token the page issued, and the response carries the rotated XSRF-TOKEN (was 419).
  • CsrfTokenManager accepts the token the request arrived with after a rotation, no longer after reset(), and the arrival token of the next request after a reset.
  • The module wiring resolves one shared CsrfTokenManager.
  • DependencyResolver and GlobalMiddlewareResolver order CsrfMiddleware before LayoutMiddleware, with layout listed first.
  • RouteCsrfMiddleware listed first checks the token before the route's middleware.
  • A forged POST to /admin/login, routed through the controller's real attributes, gets 419 without a rate limit attempt.

Four guard tests pin down behaviour that already held: a token issued during the request is not accepted once replaced, with or without a token at arrival, and a route middleware that passes the request through still gets 419 without a token and 200 with one.

Checks, in php:8.5-cli with sockets, pcntl, posix, pdo_pgsql, pdo_mysql, pdo_sqlite, gd, imagick, intl, zip and redis:

  • pest --parallel: 14185 passed, 449 skipped, 15 failed. The 15 (GdImageProcessorTest, GdImageProcessorLimitsTest, IntegrationVerificationTest, RepoManagementScriptsTest) fail the same way on a clean develop in that container.
  • phpcs, php-cs-fixer --dry-run, docs:lint and phpstan: clean.

Also checked end to end in an app on marko 0.13 with this branch's security package and its own workaround removed: a logged out POST with a stale token got 302 to /admin/login, a logged in POST without a token 419, and a valid one the normal redirect.

@github-actions github-actions Bot added bug Something isn't working breaking Introduces a breaking change labels Nov 3, 2026
@github-actions github-actions Bot added bug Something isn't working and removed bug Something isn't working labels Nov 3, 2026
@github-actions github-actions Bot added bug Something isn't working and removed bug Something isn't working labels Nov 3, 2026
BREAKING CHANGE: route middleware now runs before the CSRF check, so it also sees requests that end in a 419. A route that needs the token checked first excludes CsrfMiddleware with #[WithoutMiddleware] and lists RouteCsrfMiddleware ahead of its other middleware, as the admin panel login now does. A custom CsrfTokenManagerInterface implementation should accept the token a request arrived with until that request ends, and is now resolved as a singleton.
@github-actions github-actions Bot added bug Something isn't working and removed bug Something isn't working labels Nov 3, 2026
@etienne-scherrer
etienne-scherrer marked this pull request as ready for review November 3, 2026 22:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking Introduces a breaking change bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant