Repository navigation
fix(security)!: check the csrf token after the route's own middleware - #680
Open
etienne-scherrer wants to merge 1 commit into
Open
etienne-scherrer wants to merge 1 commit into
etienne-scherrer wants to merge 1 commit into
Conversation
etienne-scherrer
force-pushed
the
fix/csrf-runs-innermost
branch
from
November 3, 2026 21:55
2cb56f8 to
e50135e
Compare
etienne-scherrer
force-pushed
the
fix/csrf-runs-innermost
branch
from
November 3, 2026 22:08
e50135e to
921b164
Compare
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.
etienne-scherrer
force-pushed
the
fix/csrf-runs-innermost
branch
from
November 3, 2026 22:15
921b164 to
d884cbf
Compare
etienne-scherrer
marked this pull request as ready for review
November 3, 2026 22:27
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
CsrfMiddlewareis 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, andCsrfMiddlewareanswers419before 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 emptyCsrfMiddlewaresubclass after its auth middleware.Change
CsrfMiddlewarecarries#[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.RotateCsrfTokenOnLoginbefore the check now runs.CsrfTokenManagertherefore 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.CsrfTokenManagerInterfaceis now a singleton, soCsrfMiddlewareand the login observer share one manager.CsrfTokenManagerimplementsResettableInterface, so a long-running worker forgets the remembered token between requests.RouteCsrfMiddleware: the same check at the position a route lists it. The admin panel login uses it ahead of itsRateLimitMiddleware, so a forged cross-site login is rejected before it uses up a client's attempts.marko/securityloads beforemarko/layout. Both run innermost in module order, and module discovery is alphabetical, so without thisLayoutMiddlewarewould wrapCsrfMiddleware, and a token issued while the layout renders (throughcsrf_field()) would miss theXSRF-TOKENcookie on that response.security.mddescribes the new position,RouteCsrfMiddleware, and that an early response from route middleware carries no refreshedXSRF-TOKEN.routing.mdandlayout.mdno longer say CSRF wraps everything,admin-panel.mdshows the login'sRouteCsrfMiddleware, and the interface docblock states the newvalidate()contract.SecurityHeadersMiddlewarestays outer, so a419still 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 onCsrfTokenManagerInterfacewould 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 listsRouteCsrfMiddlewarefirst:A custom
CsrfTokenManagerInterfaceimplementation should also accept the token a request arrived with until that request ends, or remember-me logins on auth routes answer419. 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:
419).XSRF-TOKEN(was419).CsrfTokenManageraccepts the token the request arrived with after a rotation, no longer afterreset(), and the arrival token of the next request after a reset.CsrfTokenManager.DependencyResolverandGlobalMiddlewareResolverorderCsrfMiddlewarebeforeLayoutMiddleware, with layout listed first.RouteCsrfMiddlewarelisted first checks the token before the route's middleware./admin/login, routed through the controller's real attributes, gets419without 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
419without a token and200with one.Checks, in
php:8.5-cliwith 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 cleandevelopin that container.phpcs,php-cs-fixer --dry-run,docs:lintandphpstan: 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
302to/admin/login, a logged in POST without a token419, and a valid one the normal redirect.