Skip to content

feat: drop cookie sealing with bidirectional migration#31

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

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

Conversation

@nicknisi
Copy link
Copy Markdown
Member

@nicknisi nicknisi commented May 6, 2026

Summary

  • Adds SessionEncryptionAdapter that wraps iron-webcrypto and auto-detects format on read (Fe26.* → iron unseal, otherwise → JSON parse)
  • Default mode is 'unsealed' — session cookies written as plain JSON
  • PKCE state is always iron-sealed regardless of mode, because the sealed blob appears as the OAuth state URL parameter
  • Runtime session shape validation in decryptSession() rejects malformed cookie values
  • Consumers can pass { mode: 'sealed' } to re-enable encryption

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.

Why this is safe

  • Session cookies (wos-session) are httpOnly, secure, sameSite=lax — cookie attributes prevent client-side access
  • 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
  • decryptSession validates decoded shape at runtime, closing the gap left by removing HMAC integrity

Test plan

  • 27 adapter tests: both modes, cross-mode reads, PKCE TTL guard, unicode, prefix collision, empty/whitespace input, fake Fe26 prefix, error handling
  • 8 session validation tests: string, number, null, array, empty object, missing fields
  • All 276 tests pass, types clean
  • No consumer changes needed (authkit-tanstack-start, authkit-remix3, authkit-sveltekit)
  • Manual: sign in with existing sealed session → confirm no re-auth
  • Manual: sign out + sign back in → verify wos-session cookie is readable JSON
  • Manual: verify PKCE verifier cookie remains iron-sealed during sign-in flow

…ling

Introduces SessionEncryptionAdapter that wraps iron-webcrypto and
auto-detects format on read (Fe26.* prefix → iron, else → JSON).
Write mode is controlled by a `sealed` flag (default: false).

The factory default now writes plain JSON cookies while transparently
reading legacy iron-sealed cookies, enabling zero-downtime migration.
Passing `sealed: true` re-enables encryption for rollback.
@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented May 6, 2026

Greptile Summary

This PR replaces iron-sealed session cookies with plain JSON by default, adding a SessionEncryptionAdapter that auto-detects the cookie format on read (Fe26.* → iron unseal, otherwise → JSON parse) to enable a zero-downtime migration. PKCE verifier cookies remain iron-sealed regardless of mode; cookiePassword is now derived from apiKey + clientId when not explicitly provided, removing it as a required configuration field.

  • SessionEncryptionAdapter: bidirectional read, writes plain JSON in 'unsealed' mode (default) or iron-sealed in 'sealed' mode; PKCE path (TTL > 0) always iron-seals to protect the codeVerifier from appearing in plaintext URLs.
  • isSessionLike guard: runtime shape validation in decryptSession rejects structurally malformed cookie values and throws SessionEncryptionError, closing the integrity gap left by removing HMAC enforcement on session cookies.
  • deriveCookiePassword: deterministic password derived as wos-ck:${apiKey}:${clientId} when no explicit password is configured; validation only enforces the 32-char minimum for explicitly supplied passwords.

Confidence Score: 4/5

The migration logic and PKCE protection are sound, but the unsealed-by-default decision moves refresh tokens into plain-text Cookie headers where server-side logging infrastructure can capture them — this warrants explicit acknowledgment and deployment guidance before merging.

The bidirectional adapter, shape validation, and PKCE sealing are correctly implemented. The main concern is that refreshToken and accessToken now travel as readable JSON in every session request's Cookie header; any CDN, proxy, WAF, or SIEM log pipeline that records raw Cookie values will capture long-lived credentials. The PR describes httpOnly/secure/sameSite as the safety justification, but those attributes only guard against client-side access, not server-side log capture.

src/core/encryption/sessionEncryption.ts — the unsealed write path and its implications for server-side log exposure deserve explicit documentation or a deployment checklist note.

Security Review

  • Plain-text refreshToken exposure in Cookie headers (src/core/encryption/sessionEncryption.ts): switching to plain JSON means the wos-session cookie now travels as a human-readable object containing accessToken, refreshToken, and user PII in every HTTP request. Server-side infrastructure (CDN edge nodes, WAFs, reverse proxies, SIEM log pipelines) that captures raw Cookie headers will log these values. refreshToken is long-lived and can be used for account impersonation if extracted from logs \u2014 the cookie's httpOnly/secure/sameSite attributes do not protect against this vector.

Important Files Changed

Filename Overview
src/core/encryption/sessionEncryption.ts New SessionEncryptionAdapter that writes plain JSON (unsealed mode default) for session cookies and always iron-seals PKCE state; the unsealed path exposes refresh tokens in HTTP Cookie headers readable by server-side logging infrastructure
src/core/config/ConfigurationProvider.ts Removes cookiePassword from required fields and derives it from apiKey+clientId when absent; validation gap — derived password length is never checked against the 32-char minimum
src/core/AuthKitCore.ts Adds isSessionLike runtime shape validator in decryptSession; cleanly re-throws SessionEncryptionError to avoid double-wrapping; logic is correct
src/service/factory.ts Switches default encryption to SessionEncryptionAdapter wrapping ironWebcryptoEncryption; stateless today so functional behaviour is unchanged
src/core/encryption/sessionEncryption.spec.ts Comprehensive adapter tests covering both modes, cross-mode reads, PKCE TTL guard, unicode, error paths, and prefix collision safety — well structured

Sequence Diagram

sequenceDiagram
    participant Browser
    participant Server
    participant SessionEncryptionAdapter
    participant IronWebcrypto

    Note over Browser,Server: Session write (unsealed mode default)
    Server->>SessionEncryptionAdapter: "sealData(session, {password, ttl:0})"
    SessionEncryptionAdapter-->>Server: JSON.stringify(session)
    Server->>Browser: "Set-Cookie: wos-session={...} (httpOnly, secure)"

    Note over Browser,Server: Session read (bidirectional migration)
    Browser->>Server: "Cookie: wos-session=value"
    Server->>SessionEncryptionAdapter: "unsealData(value, {password})"
    alt value starts with Fe26.
        SessionEncryptionAdapter->>IronWebcrypto: unsealData(value)
        IronWebcrypto-->>SessionEncryptionAdapter: session object
    else plain JSON
        SessionEncryptionAdapter-->>SessionEncryptionAdapter: JSON.parse(value)
    end
    SessionEncryptionAdapter-->>Server: session object
    Server->>Server: isSessionLike(session) validation

    Note over Browser,Server: PKCE flow (always sealed)
    Server->>SessionEncryptionAdapter: "sealData(pkceState, {password, ttl:600})"
    SessionEncryptionAdapter->>IronWebcrypto: "sealData(pkceState, {ttl:600})"
    IronWebcrypto-->>SessionEncryptionAdapter: "Fe26.2*..."
    Server->>Browser: "Set-Cookie: wos-auth-verifier=Fe26.2*..."
Loading

Reviews (4): Last reviewed commit: "feat: make cookiePassword optional, deri..." | Re-trigger Greptile

Comment thread src/core/encryption/sessionEncryption.ts Outdated
Comment thread src/core/encryption/sessionEncryption.ts
Comment thread src/service/factory.ts
Comment on lines +51 to +52
encryptionFactory = () =>
new SessionEncryptionAdapter(ironWebcryptoEncryption),
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 The new default creates a fresh SessionEncryptionAdapter on every encryptionFactory() call, whereas the old default returned the shared ironWebcryptoEncryption singleton. Because SessionEncryptionAdapter is stateless this is functionally equivalent today, but it diverges from the existing pattern and would mask state bugs if the class ever acquired state. Wrapping the construction in once (already imported) matches how the rest of the factory avoids repeated construction.

Suggested change
encryptionFactory = () =>
new SessionEncryptionAdapter(ironWebcryptoEncryption),
encryptionFactory = once(() =>
new SessionEncryptionAdapter(ironWebcryptoEncryption)),

devin-ai-integration[bot]

This comment was marked as resolved.

nicknisi added 3 commits May 6, 2026 12:35
… API

- Always seal when TTL > 0 so PKCE codeVerifier never appears in
  plaintext in the OAuth state URL parameter
- Default mode is 'sealed' for safe rolling deploys (readers first,
  flip to 'unsealed' once all nodes run the adapter)
- Replace positional boolean with options object ({ mode: 'sealed' | 'unsealed' })
- Add runtime session shape validation in decryptSession() — rejects
  arrays, primitives, null, and objects missing required fields
- Add tests: empty/whitespace input, fake Fe26. prefix, invalid session
  shapes, PKCE TTL guard, prefix collision safety
Session cookies are written as plain JSON by default. PKCE state
remains iron-sealed (TTL > 0 guard). Consumers can pass
{ mode: 'sealed' } to re-enable encryption.
Comment on lines +6 to +15
/**
* `'sealed'` — always encrypt via iron-webcrypto (current behavior).
* `'unsealed'` — write plain JSON for session cookies. PKCE state
* (TTL > 0) is always sealed regardless of this setting because
* the sealed blob appears in the OAuth `state` URL parameter.
*
* Default: `'sealed'` — deploy readers first, flip to `'unsealed'`
* once all nodes run the adapter.
*/
mode?: 'sealed' | 'unsealed';
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 The JSDoc for mode says the default is 'sealed', but the constructor assignment on line 37 (options.mode ?? 'unsealed') defaults to 'unsealed'. A consumer reading this doc to plan a rolling migration would believe they can deploy readers first with sealed behavior, then flip — but the adapter already writes JSON out of the box. The stated migration strategy ("deploy readers first, flip to unsealed once all nodes run the adapter") only makes sense if the initial default is sealed.

Suggested change
/**
* `'sealed'` always encrypt via iron-webcrypto (current behavior).
* `'unsealed'` write plain JSON for session cookies. PKCE state
* (TTL > 0) is always sealed regardless of this setting because
* the sealed blob appears in the OAuth `state` URL parameter.
*
* Default: `'sealed'` deploy readers first, flip to `'unsealed'`
* once all nodes run the adapter.
*/
mode?: 'sealed' | 'unsealed';
/**
* `'sealed'` always encrypt via iron-webcrypto (current behavior).
* `'unsealed'` write plain JSON for session cookies. PKCE state
* (TTL > 0) is always sealed regardless of this setting because
* the sealed blob appears in the OAuth `state` URL parameter.
*
* Default: `'unsealed'` session cookies are written as plain JSON.
* Pass `{ mode: 'sealed' }` to re-enable iron-webcrypto encryption.
*/
mode?: 'sealed' | 'unsealed';

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 new potential issue.

View 7 additional findings in Devin Review.

Open in Devin Review

Comment on lines +12 to +13
* Default: `'sealed'` — deploy readers first, flip to `'unsealed'`
* once all nodes run the adapter.
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.

🟡 JSDoc states default mode is 'sealed' but implementation defaults to 'unsealed'

The JSDoc comment on the mode property at src/core/encryption/sessionEncryption.ts:12 states Default: 'sealed', but the constructor at line 37 actually defaults to 'unsealed' (options.mode ?? 'unsealed'). This discrepancy means developers reading the documentation in their IDE will believe sessions are encrypted by default, when in reality session cookies are stored as plain JSON without encryption or integrity protection. The createAuthService factory at src/service/factory.ts:52 uses the adapter with no explicit mode, inheriting the 'unsealed' default.

Suggested change
* Default: `'sealed'` deploy readers first, flip to `'unsealed'`
* once all nodes run the adapter.
* Default: `'unsealed'` session cookies are written as plain JSON.
* Pass `'sealed'` to re-enable iron-webcrypto encryption.
Open in Devin Review

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

WORKOS_COOKIE_PASSWORD is no longer required. When not provided, a
sealing password is derived from the already-required apiKey and
clientId. This eliminates an extra env var for most setups.

Explicitly setting WORKOS_COOKIE_PASSWORD still works and takes
precedence — useful for cookie continuity across API key rotations.
Comment on lines +40 to +48
async sealData(
data: unknown,
options: { password: string; ttl?: number | undefined },
): Promise<string> {
if (this.mode === 'sealed' || (options.ttl != null && options.ttl > 0)) {
return this.ironEncryption.sealData(data, options);
}
return JSON.stringify(data);
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 security Plain-JSON session cookies expose refreshToken to server-side log infrastructure

The PR's safety argument (httpOnly, secure, sameSite=lax) only prevents client-side JavaScript access. It does not protect against server-side log capture: CDN edge logs, WAF/SIEM systems, reverse proxies, and load balancers routinely log raw Cookie request headers at DEBUG or AUDIT level. Once a request hits those systems, the Cookie: wos-session={"accessToken":"eyJ...","refreshToken":"rt_...","user":{...}} header is written in plain text.

refreshToken is long-lived — an attacker who extracts it from a log entry can impersonate the user until the token is revoked. Consider at minimum documenting that callers must sanitize Cookie headers from any logging infrastructure, or offer a way to enable sealed mode by default for security-sensitive deployments.

Rule Used: Do not log sensitive fields like access_token, ref... (source)

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.

1 participant