Skip to content

fix(express)!: clear cookies with the set-path attributes and drop GET state changes#103

Merged
Bccorb merged 1 commit into
mainfrom
fix/cookie-clearing-and-csrf
Jul 20, 2026
Merged

fix(express)!: clear cookies with the set-path attributes and drop GET state changes#103
Bccorb merged 1 commit into
mainfrom
fix/cookie-clearing-and-csrf

Conversation

@Bccorb

@Bccorb Bccorb commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Closes #96
Closes #97

#96: logout did not clear cookies in cross-site deployments (release blocker)

clearSessionCookie/clearAllCookies called res.clearCookie(name, { domain, path: "/" }) with no secure/sameSite, while setSessionCookie in the same file sets both and defaults to Secure; SameSite=None. Express's clearCookie only forces expires, so the clearing header was rejected by browsers in a cross-site response context, which is exactly the deployment the defaults target.

Because requireAuth verifies the cookie locally and never consults the auth API, a 204 logout revoked the session upstream while the signed cookie survived in the browser and stayed fully valid for adopter routes until its own TTL expired.

Both helpers now take CookieSignerOptions and pass { secure, sameSite, domain, path: "/" }, so clearing mirrors setting. All three call sites are updated: handlers/logout.ts, handlers/me.ts, and the clear-cookies branch in middleware/ensureCookies.ts.

#97: CSRF via GET /logout and GET /magic-link

Both were state-changing simple requests. With SameSite=None and no CSRF or Origin check, an <img src> on any page could revoke all of a victim's sessions or flood their mailbox with magic links.

  • GET /logout is removed. DELETE /logout and DELETE /logout/all already cover it and it was documented as deprecated. DELETE forces a preflight, so this vector is fully closed.
  • GET /magic-link becomes POST /magic-link. The issue states it already has a POST sibling; it does not. GET /magic-link was the only route that requests a magic link, and the handler takes no body or query (identity comes from the pre-auth cookie). Removing it outright would have deleted magic-link login, so it is converted to POST instead, which the issue lists as the alternative fix.

Note on residual risk: POST removes the zero-interaction <img src> vector, but a scripted cross-site form POST with Content-Type: text/plain is still a simple request and can reach the route. Fully closing magic-link CSRF needs the Origin check below.

Follow-up (deliberately not in this PR)

An Origin/Sec-Fetch-Site allowlist check in the router when sameSite === "none". That covers the remaining POST/DELETE surface (MFA disable, credential deletion), which widens for adopters running cors({ origin: true, credentials: true }), and closes the residual magic-link vector noted above.

Tests

The reason #96 shipped is that cookieSecurity.test.js filtered to set-cookie headers on the SET path only and never asserted anything about clearing.

Added to cookieSecurity.test.js: the clear path emits Secure and SameSite=None with an epoch Expires, tracks cookieSecure: false down to SameSite=Lax, and honors an explicit cookieSameSite override.

Verified these are not vacuous. Reverting only the cookie.ts change makes the three new tests fail while all five pre-existing set-path tests still pass, which reproduces the original coverage gap.

Added to logoutRoutes.test.js: GET /logout and GET /magic-link no longer route, and POST /magic-link still proxies upstream. The GET /magic-link case sends a valid pre-auth cookie on purpose, since without it the cookie middleware answers 400 before routing and the test would pass whether or not the route existed.

Updated messagingDelivery.test.js to POST. The upstream fetch to the auth API remains GET; only the adapter's inbound verb changed.

Verification

pnpm build   clean
pnpm test    core 15 suites / 60 tests passed
             express 21 suites / 89 tests passed

Breaking change

Changeset is minor, matching the 0.x breaking channel used by #89, #92, and #93. Adopters calling GET /auth/logout move to DELETE /auth/logout/all; adopters calling GET /auth/magic-link move to POST /auth/magic-link. README updated. This also affects the client SDKs, which need the same verb changes.

…T state changes

clearSessionCookie and clearAllCookies emitted a clearing header with no
secure or sameSite while setSessionCookie sets both, defaulting to
Secure; SameSite=None. Express's clearCookie only forces expires, so the
clearing header was rejected by browsers in a cross-site response, which
is the deployment the defaults target. Since requireAuth verifies the
cookie locally and never consults the auth API, a 204 logout revoked the
session upstream while the signed cookie stayed valid for adopter routes
until its own TTL expired. Both clear helpers now take the CookieSigner
options so clearing mirrors setting.

BREAKING CHANGE: GET /logout and GET /magic-link are removed. Both were
state-changing simple requests, so with SameSite=None and no CSRF or
Origin check an img tag on any page could revoke every session or flood
a mailbox with magic links. GET /logout was already deprecated in favor
of DELETE /logout/all. GET /magic-link had no non-GET sibling, so it
becomes POST /magic-link rather than being dropped.

The existing cookie test only inspected the set path, which is why the
clearing defect shipped. The clear path is now asserted directly.

Closes #96
Closes #97
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

security(express): CSRF via GET /logout and GET /magic-link security(express): logout does not clear cookies in cross-site deployments

1 participant