feat!: PKCE state binding + storage-owned verifier cookies (authkit-session 0.4.0)#66
feat!: PKCE state binding + storage-owned verifier cookies (authkit-session 0.4.0)#66
Conversation
Consume authkit-session 0.4.0's PKCE primitives across the three URL server functions and the callback route. The adapter now writes the sealed verifier cookie via the middleware's pending-header channel when generating auth URLs, reads it from the callback request, passes it to core for cryptographic verification, and deletes it on every callback exit path (success, error, onError, missing-code, setup-failure). Removes the old base64-plus-dot state format and the `decodeState` helper it depended on. `customState` and `returnPathname` now come directly from the cryptographically-verified sealed blob returned by `authkit.handleCallback`, closing the state-confusion attack vector. Security properties: - Delete-cookie Set-Cookie appended on every response leaving the callback handler, including when `getAuthkit()` itself fails (static fallback broadside covering SameSite=Lax and SameSite=None;Secure). - Error response bodies no longer echo `error.message` / `details` -- operational detail stays server-side via console.error. - `sealedState` is not exposed through the public server-function return types; callers still receive `Promise<string>` (URL only). - Fails loud with actionable error if `authkitMiddleware` context is unavailable when generating auth URLs, rather than silently producing a URL that always fails at callback. BREAKING CHANGES: - Requires @workos/authkit-session@0.4.0. - `decodeState` helper removed from internal and public surface. - `customState`/`returnPathname` on OAuth callbacks are now integrity-protected by the seal; tampered state parameters fail closed with OAuthStateMismatchError instead of silently falling back to a root redirect. Also extracts `parseCookies` from the private `storage.ts` method into a shared `src/server/cookie-utils.ts` module and re-exports `OAuthStateMismatchError` / `PKCECookieMissingError` for adopter error handling.
Collapse the three copy-pasted "inject contextRedirectUri if caller didn't provide one" blocks across getAuthorizationUrl/getSignInUrl/ getSignUpUrl into a single generic helper. Pure refactor: no behavior change, same public return types (`Promise<string>`), PKCE cookie wiring via `writeCookieAndReturn` untouched.
Internal refactor of the TanStack Start adapter to track authkit-session
0.4.0's storage-owned PKCE cookie flow (Phase 2 of the coordinated
refactor in ./docs/ideation/storage-owned-pkce-cookies in authkit-session).
Public adapter surface is unchanged:
- `getSignInUrl`, `getSignUpUrl`, `getAuthorizationUrl` still return
`Promise<string>`.
- `handleCallbackRoute({ onSuccess, onError })` signature unchanged.
Internally:
- `TanStackStartCookieSessionStorage` now implements `getCookie(request,
name)`; the `getSession` override is deleted and inherited from the
base class as a wrapper over `getCookie`.
- Server functions call upstream `createAuthorization`/`createSignIn`/
`createSignUp` and forward each `Set-Cookie` from the returned
`HeadersBag` through `__setPendingHeader` (append-per-value, never
comma-joined).
- Callback handler reads no cookies itself and passes no `cookieValue`
into `handleCallback`. The library emits both the session cookie and
the verifier-delete cookie; the adapter appends each via `.append`.
- Error path uses `authkit.clearPendingVerifier(new Response())` to
obtain the verifier-delete `Set-Cookie`; `STATIC_FALLBACK_DELETE_HEADERS`
is preserved for the case where `getAuthkit()` itself throws.
- `readPKCECookie` is removed from `cookie-utils.ts`; `parseCookies` is
kept as a generic helper used by storage.
Example app additions:
- New `/api/auth/sign-in` route that calls `getSignInUrl` at request
time so the PKCE verifier `Set-Cookie` lands on an actual redirect
response (rather than a page-loader response that doesn't propagate
cookies through TanStack's client-side navigation path).
- `__root.tsx`, `_authenticated.tsx`, and `index.tsx` now route the
sign-in button through that endpoint.
- `package.json` + `pnpm-lock.yaml`: `@workos/authkit-session` is
pinned via `pnpm.overrides` to `link:../authkit-session`. Reverts
once 0.4.0 publishes to npm.
Greptile SummaryThis PR refactors the TanStack Start adapter to consume
Confidence Score: 4/5Safe to merge once the One P1 finding: the
Important Files Changed
Reviews (1): Last reviewed commit: "feat(pkce)!: consume storage-owned verif..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| "overrides": { | ||
| "@workos/authkit-session": "link:../authkit-session" | ||
| } |
There was a problem hiding this comment.
link: override must be removed before merging
The pnpm.overrides entry resolves @workos/authkit-session to link:../authkit-session, which means any environment that only has this repo checked out (including most CI systems and every downstream contributor) will get a hard install failure — ../authkit-session won't exist. The package.json dependencies field correctly specifies "0.4.0", but pnpm still resolves through the override before publishing. This entire block must be removed (or reverted to a registry version) before the PR lands on main.
| "overrides": { | |
| "@workos/authkit-session": "link:../authkit-session" | |
| } | |
| "pnpm": { | |
| "onlyBuiltDependencies": [ | |
| "@parcel/watcher", | |
| "esbuild" | |
| ] | |
| } |
| if (result.headers) { | ||
| for (const [key, value] of Object.entries(result.headers)) { | ||
| if (Array.isArray(value)) { | ||
| for (const v of value) ctx.__setPendingHeader(key, v); | ||
| } else if (typeof value === 'string') { | ||
| ctx.__setPendingHeader(key, value); | ||
| } | ||
| } | ||
| } else if (result.response) { | ||
| // Fallback: storage mutated the Response directly (context-unavailable path). | ||
| for (const value of result.response.headers.getSetCookie()) { | ||
| ctx.__setPendingHeader('Set-Cookie', value); | ||
| } | ||
| } | ||
|
|
||
| return result.url; |
There was a problem hiding this comment.
headers and response are absent
If the upstream library returns an AuthorizationResult with neither headers nor response populated (e.g., due to a future upstream refactor or a version mismatch), the function returns the URL without setting any cookie and without any log or error. The PKCE verifier cookie is silently dropped, and the failure only surfaces later as a state-mismatch error in the callback. A console.warn here would make the root cause immediately obvious.
| return { 'Set-Cookie': setCookie }; | ||
| } | ||
|
|
||
| function appendSessionHeaders(target: Headers, result: any): void { |
There was a problem hiding this comment.
result: any widens the type unnecessarily
appendSessionHeaders accepts result: any, which removes all type-checking for the result.headers, result.response, and result.response.headers property accesses that follow. Given that AuthorizationResult (defined in server-functions.ts) already captures the expected shape, or a similar local interface could be defined here, tightening this type would surface future breaking changes from the upstream library at compile time rather than at runtime.
Consolidate three near-duplicate HeadersBag iteration blocks (forwardAuthorizationCookies, signOut, appendSessionHeaders) behind a single helper in headers-bag.ts. Also drop the `any` parameter on appendSessionHeaders and remove a change-narrating comment.
…aths
`buildVerifierDeleteHeaders` previously read only `result.headers['Set-Cookie']`
from `clearPendingVerifier`, but this adapter's `applyHeaders` override always
returns `{ response }` with the Set-Cookie attached to the response — the
headers bag is empty. Every error path therefore fell back to the static
root-path delete, which doesn't match per-request `redirectUri` overrides and
leaves the verifier cookie in place for the next attempt.
Read from `response.headers.getSetCookie()` first, fall back to the headers
bag, then to the static default as a last resort.
Reported by Codex review.
Prior fix ensured the verifier-delete header was *extracted* correctly
from the adapter's `{ response }` return shape, but the call was still
`clearPendingVerifier(new Response())` with no options — so upstream
computed the delete cookie's `Path` from the config-default
`redirectUri`, ignoring any per-request override set via
`authkitMiddleware({ redirectUri })`. A failed callback on a scoped
path would therefore emit a `Path=/` delete that doesn't match the
browser's scoped cookie, leaving the verifier behind.
Pull the override from middleware context (same source used by
`applyContextRedirectUri` on the sign-in path) and pass it to
`clearPendingVerifier` so the delete's `Path` matches the cookie
`createSignIn` originally set.
Tests now assert the *call arguments* to `clearPendingVerifier`, not
just the return transport — the prior test only proved a mocked scoped
header would propagate.
Reported by Codex review (follow-up to 3d0222f).
nicknisi
changed the title
… or response Previously `forwardAuthorizationCookies` silently returned the URL when the upstream authorization result had neither `headers` nor `response` populated. That path should be impossible under the current `@workos/authkit-session` contract, but if a future refactor or version mismatch ever hit it, the PKCE verifier cookie would be dropped and the failure would only surface later as an opaque state-mismatch error on the callback. Fail loudly at the drop site so the real cause is visible, matching the existing throw for a missing middleware context above.
- Extract `emitHeadersFrom` in headers-bag.ts; both
`forwardAuthorizationCookies` (server-functions.ts) and
`appendSessionHeaders` (server.ts) now share the "HeadersBag or mutated
Response → emit" path instead of hand-rolling it twice. The helper
returns a boolean so each call site keeps its own policy — throw on
empty for PKCE forwarding, silent no-op for session headers.
- Defer `buildVerifierDeleteHeaders` into `errorResponse`. The success
path never used the result, so every happy-path callback was paying
for an extra `clearPendingVerifier` storage round-trip.
- Fix `parseCookies('')` returning `{ '': '' }` — return `{}`. The old
shape would silently mask "no cookie" as "empty cookie" for any caller
that indexed by name.
- Drop a test comment that narrated an upstream shape change; it would
rot the first time upstream shifted again.
`emitHeadersFrom` previously returned `false` when `source.response` was present but produced zero `Set-Cookie` values, causing `forwardAuthorizationCookies` to throw on the happy path. The zero-emit case is legitimate: the storage adapter forwards cookies directly via `ctx.__setPendingHeader` and hands back a bare `new Response()` as a contract stub. The server function should consider "one of headers/response present" as the contract — not "bytes emitted." Regression from de4e5cb. 196 tests passed because all mocks populated `headers`; none exercised the response-present-but-empty shape. Added a regression test across all three authorization URL paths.
Ports workos/authkit-nextjs#402 to this SDK. Adds: - Setup step for a `/api/auth/sign-in` route used as the dashboard's Sign-in endpoint (initiate_login_uri). Required for WorkOS-initiated flows like dashboard impersonation, which otherwise hit the callback with no `state` and fail PKCE/CSRF verification. - Dashboard-configuration list now includes the Sign-in endpoint and links directly to the Redirects page. - Troubleshooting entry for `Missing required auth parameter` pointing back to the setup step.
Replaces the four remaining `getSignInUrl()`-in-loader + `redirect({ href: signInUrl })`
code samples with `redirect({ href: '/api/auth/sign-in' })` so the
documented patterns match the example app and the Sign-In Endpoint step
added in 5e7cef0. The old inline pattern could race the PKCE verifier
cookie against the cross-origin redirect; routing through the dedicated
endpoint keeps both the cookie set and the WorkOS redirect on the same
server response.
Drops the `link:../authkit-session` pnpm override that pinned the SDK to the local sibling workspace. The `clearPendingVerifier(response, options)` signature used by the callback path is now shipped in 0.4.0 on the registry, so the override is no longer required. Side effect: the example's authkit-loader server chunk shrinks from 188 kB to 2.9 kB — the registry package tree-shakes correctly where the workspace link pulled the unbundled module graph.
Review —
|
gjtorikian
left a comment
gjtorikian
left a comment
There was a problem hiding this comment.
I don't think Devin's documentation concerns are worth addressing.
2 participants
Summary
Adopts
@workos/authkit-session@0.4.0's two coupled changes — OAuth state binding (URLstateis sealed and byte-compared against anHttpOnlyverifier cookie) and storage-owned verifier cookies (the library owns the cookie lifecycle through newSessionStorageprimitives). The adapter's public API is unchanged; all rework is internal.Phase 2 of the coordinated three-repo refactor. Spec:
docs/ideation/storage-owned-pkce-cookies/spec-phase-2.mdinauthkit-session.Public adapter API unchanged.
getSignInUrl/getSignUpUrl/getAuthorizationUrlstill returnPromise<string>;handleCallbackRoute({ onSuccess, onError })signature unchanged.What moved internally
Storage — minimal surface, maximum ownership
TanStackStartCookieSessionStoragenow implementsgetCookie(request, name); the oldgetSessionoverride is gone (inherited from the base class as a one-line wrapper overgetCookie).applyHeadersoverride keeps its two-mode behavior (middleware context →__setPendingHeader/ fallback → mutateResponse) so upstream cookie writes flow through middleware-aware infrastructure when available.Server functions — forward, don't serialize
server-functions.tscalls upstreamcreateAuthorization/createSignIn/createSignUpand forwards eachSet-Cookiefrom the returnedHeadersBagthrough__setPendingHeader(append per value — never comma-joined).forEachHeaderBagEntry(bag, emit)helper insrc/server/headers-bag.tsdeduplicates the bag-iteration pattern acrossforwardAuthorizationCookies,appendSessionHeaders, and the pre-existingsignOutpath.Callback — upstream owns the cookie on every exit path
server.tsno longer reads the PKCE cookie or passescookieValueintohandleCallback. The library emits both the session cookie and the verifier-delete cookie as astring[]; the adapter appends each via.appendso they survive as distinct HTTP headers.authkit.clearPendingVerifier(new Response(), { redirectUri })to get the correct verifier-deleteSet-Cookie. TheredirectUricomes from middleware context viagetRedirectUriFromContext()so the delete'sPathmatches whatevercreateSignInoriginally set — this closes a bug where anauthkitMiddleware({ redirectUri: '...' })override would get aPath=/delete that didn't match the scoped cookie the browser actually holds.STATIC_FALLBACK_DELETE_HEADERSis preserved as a safety net for the case wheregetAuthkit()itself throws at setup — upstream instance unavailable meansclearPendingVerifiercan't be called at all.readPKCECookieis removed fromcookie-utils.ts;parseCookiesremains as a generic helper used bystorage.ts.Example app — sign-in as a redirect endpoint
Reworked so the PKCE verifier
Set-Cookielands on an actual redirect response rather than a page-loader response (which doesn't propagate cookies through TanStack's client-side navigation):/api/auth/sign-inroute that callsgetSignInUrlat request time and issues a 307 redirect with the cookie attached.__root.tsx,_authenticated.tsx, andindex.tsxnow route the sign-in button through that endpoint instead of pre-resolving the URL in their loaders.Review-driven fixes included
Two bugs surfaced mid-review and were fixed on-branch:
buildVerifierDeleteHeadersoriginally only readresult.headers['Set-Cookie'], but this adapter'sapplyHeadersreturns{ response }with theSet-Cookieattached to the response — the headers bag is empty in that path. Now readsresponse.headers.getSetCookie()first, falls back to the bag, then to the static default only if both are empty. (commit3d0222f)clearPendingVerifier(new Response())was being called with no options, so upstream silently defaulted to the config-levelredirectUriwhen computing the delete'sPath. Now pulls the per-request override from middleware context. (commit50284ef)Test plan
pnpm test— 196/196 passing (17 files)pnpm run typecheck— cleanpnpm run lint— 0 warnings / 0 errors (oxlint)pnpm run format:check— cleanpnpm run build— clean (tsc)wos-auth-verifieris written on sign-in and cleared on callback (expect 2Set-Cookieheaders on the callback response: session + verifier-delete)Coordination & merge order
@workos/authkit-session@0.4.0publishes to npm. Thepnpm.overrides→link:../authkit-sessionentry is a local-dev convenience, not a publishable dependency. Post-publish, flip it back to^0.4.0(one commit).BREAKING CHANGE: requires
@workos/authkit-session@0.4.0+. Public adapter surface unchanged — only the upstream dep floor moves.