Skip to content

refactor: drop cookie sealing with bidirectional migration#424

Open
nicknisi wants to merge 5 commits into
mainfrom
feat/drop-session-sealing
Open

refactor: drop cookie sealing with bidirectional migration#424
nicknisi wants to merge 5 commits into
mainfrom
feat/drop-session-sealing

Conversation

@nicknisi
Copy link
Copy Markdown
Member

@nicknisi nicknisi commented May 7, 2026

Closes #425

Summary

  • Session cookies (wos-session) are now written as plain JSON instead of iron-sealed blobs
  • Reader auto-detects format via Fe26. prefix — existing iron-sealed cookies are transparently unsealed
  • On next token refresh, legacy cookies are rewritten as plain JSON (zero-downtime migration)
  • PKCE state remains iron-sealed regardless — those values appear in OAuth state URL parameters
  • Malformed cookie values are now gracefully handled (return no session instead of throwing)

Why this is safe

  • Session cookies are httpOnly, secure, sameSite=lax — browser JS cannot read them
  • PKCE verifier cookies remain iron-sealed — the sealed blob appears in URLs where plaintext would expose the codeVerifier
  • Iron seals are unambiguously detectable via Fe26. prefix — no collision with JSON (JSON.stringify always starts with {, [, or ")

Migration behavior

Seamless. The adapter reads both formats, so existing users with iron-sealed cookies are never rejected. On next token refresh the cookie is rewritten as JSON. No re-auth, no downtime.

Mirrors the approach in workos/authkit-session#31.

capture_20260507_145357
Open in Devin Review

@nicknisi nicknisi requested a review from a team as a code owner May 7, 2026 21:55
Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 1 potential issue.

View 4 additional findings in Devin Review.

Open in Devin Review

Comment thread src/session.ts
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Raw JSON in Set-Cookie header breaks when session data contains semicolons

After the migration from sealData (which produces URL-safe base64) to JSON.stringify, the raw JSON output is placed directly into a Set-Cookie header at line 319 without URL-encoding. If any session field (e.g., user's firstName, lastName, email, or metadata) contains a semicolon (;), the browser's cookie parser will interpret it as a cookie attribute delimiter, truncating the cookie value. This produces malformed JSON that fails to parse on subsequent requests, causing session loss.

Demonstration of the truncation

For a user with firstName: "John; Drop", the Set-Cookie header becomes:

wos-session={"accessToken":"eyJ...","user":{"firstName":"John; Drop"}}; Path=/; HttpOnly; ...

The browser parses the value as {"accessToken":"eyJ...","user":{"firstName":"John — everything after the first ; in the JSON is treated as cookie attributes. The stored cookie is malformed JSON that decryptSession cannot parse.

Note that saveSession (src/session.ts:640) uses nextCookies.set() which internally URL-encodes the value via the cookie package's serialize function, so it is not affected. Only the middleware refresh path at line 319, which manually constructs the raw header string, is vulnerable.

(Refers to line 319)

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — this was a real gap. The saveSession path was safe (nextCookies.set URL-encodes internally), but this middleware refresh path built the header string manually.

Fixed in 91654ee — the value is now wrapped in encodeURIComponent(). The cookie read side (request.cookies.get() / nextCookies.get()) auto-decodes, so decryptSession receives raw JSON as expected.

For reference, authkit-session was already safe here — its serializeCookie helper uses encodeURIComponent(value) at the serialization layer.

@nicknisi nicknisi requested review from cmatheson and gjtorikian May 7, 2026 22:06
nicknisi added 4 commits May 7, 2026 15:18
Session cookies are now written as plain JSON instead of iron-sealed
blobs. The reader detects the Fe26. prefix to transparently unseal
legacy cookies, so existing sessions migrate on next refresh with
zero downtime. PKCE state remains iron-sealed since those values
appear in OAuth URL parameters.
JSON values can contain semicolons (e.g. user names) which the
browser interprets as cookie attribute delimiters, truncating the
value. The saveSession path was safe (nextCookies.set URL-encodes),
but the middleware refresh path built the header string manually.
@nicknisi nicknisi changed the title feat: drop cookie sealing with bidirectional migration refactor: drop cookie sealing with bidirectional migration May 7, 2026
@nicknisi nicknisi force-pushed the feat/drop-session-sealing branch from e336657 to 748ce46 Compare May 7, 2026 22:19
When not set, a password is derived from WORKOS_API_KEY and
WORKOS_CLIENT_ID. Explicitly setting the env var still takes
precedence — useful for cookie continuity across API key rotations
or sharing sessions across domains.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

WORKOS_COOKIE_PASSWORD doc inconsistency

1 participant