Skip to content

feat!: per-flow PKCE verifier cookies + pure URL helpers (0.5.0)#27

Merged
nicknisi merged 21 commits intomainfrom
pkce-per-flow-cookies
Apr 23, 2026
Merged

feat!: per-flow PKCE verifier cookies + pure URL helpers (0.5.0)#27
nicknisi merged 21 commits intomainfrom
pkce-per-flow-cookies

Conversation

@nicknisi
Copy link
Copy Markdown
Member

@nicknisi nicknisi commented Apr 23, 2026

Summary

  • Per-flow PKCE verifier cookies: wos-auth-verifier-<fnv1a>, with suffix derived from the sealed state. Concurrent sign-ins from multiple tabs no longer clobber each other.
  • Breaking: clearPendingVerifier(response, { state })state is now required to identify the per-flow cookie. Guard the call on state presence; skip it when the callback URL is malformed (10-min TTL cleans up orphans).
  • Breaking: CreateAuthorizationResult gains a required cookieName: string field.
  • PKCE_COOKIE_PREFIX and getPKCECookieNameForState are exported from the package root for cookie introspection.

Why the hash is safe

FNV-1a 32-bit is a namespacing mechanism, not a security primitive. Integrity of the sealed blob comes from AEAD in SessionEncryption, and verifyCallbackState still does a constant-time byte comparison of the full state. Collision probability is ~n²/2³² (vanishingly small even with 50 concurrent flows); a collision fails closed via state mismatch.

Dropped before merge

An earlier draft added getAuthorizationUrl / getSignInUrl / getSignUpUrl — pure variants that returned { url, cookieName } without writing a cookie. Removed after review: no concrete consumer, and using them for a document-request redirect silently produces a broken callback with no surfaced sealedState to recover with. The motivating "non-document requests in middleware" case is better solved by calling createSignIn only on the actual sign-in route. Can be reintroduced later with a better-signalled API if a real caller materializes.

Test plan

  • vitest run — 215/215 passing
  • tsc --noEmit clean
  • New cookieName.spec.ts with known-answer FNV-1a vectors (empty string, a, foobar)
  • Concurrent-flow isolation: callback for flow A does not touch flow B's cookie
  • Tampered-cookie, missing-cookie, scheme-agnostic delete, and redirectUri-override cases all rewired under derived names
  • Verified neither authkit-tanstack-start (0.4.0 dep) nor authkit-sveltekit (^0.4.0 dep) consume the dropped pure helpers — name collisions in those adapters are independent wrappers around createSignIn / createSignUp
  • Smoke-test one downstream adapter on the breaking clearPendingVerifier call site before release

nicknisi added 17 commits April 22, 2026 14:51
Design for porting the authkit-nextjs PR #403 fix (per-flow PKCE
cookie names) to authkit-session and its downstream adapters
(authkit-sveltekit, authkit-tanstack-start). Additive to the
core's public API except for a deliberate breaking change to
clearPendingVerifier, which is only called in-tree.
Fix stale line numbers in the evidence section. Resolve contradiction
between CreateAuthorizationResult.cookieName and the clearPendingVerifier
state-based signature. Add explicit adapter rule for missing-state
cleanup (skip, let TTL handle orphans). Replace TanStack's static
fallback delete headers with a state-derived variant. Call out
SvelteKit test fixture updates and add static-fallback test case
for TanStack.
Correct release versions against actual package.json state (session is
already 0.4.0, sveltekit 0.2.0, tanstack 0.6.0). Acknowledge the
clearPendingVerifier break as consumer-facing (it's in public README
and MIGRATION docs) and add README/MIGRATION updates to the scope.
20-task plan across three phases (authkit-session 0.5.0 →
authkit-sveltekit 0.3.0 → authkit-tanstack-react-start 0.7.0).
Each task TDD-structured with exact code blocks, file paths, and
commit messages.
Introduces PKCE_COOKIE_PREFIX and getPKCECookieNameForState(state),
backed by an inline FNV-1a 32-bit hash. Zero new dependencies.
Adds cookieName to GeneratedAuthorizationUrl. Byte-length guard now
measures the actual (longer) on-wire name. Still well under the
3800-byte cap (worst case 26 chars vs 17).
Addresses code-quality feedback on d248daa: oxlint flagged the unused
import as an error. Also hoists the dynamic import in the new test
to a static top-level import for consistency.
Callers that destructure the result now see the PKCE verifier cookie
name that was written. Type-only change; runtime wiring lands next.
createAuthorization, createSignIn, createSignUp now write under the
name returned by generateAuthorizationUrl. Concurrent flows no longer
clobber each other's cookies.
handleCallback now reads and clears the flow-specific cookie identified
by the URL state parameter. Removes the mirrorToLegacyName test helper
that bridged Task 4's partial migration. Concurrent flows are fully
isolated end-to-end: callback for flow A leaves flow B's cookie
untouched.
getAuthorizationUrl, getSignInUrl, getSignUpUrl — same URL that
createAuthorization/createSignIn/createSignUp produce, but no cookie
write. Adapters with loop-prone paths (middleware hooks that fire
on every request) can use these for non-document requests to avoid
HTTP 431 cookie bloat.
BREAKING CHANGE: clearPendingVerifier now takes
{ state: string; redirectUri?: string } (state required). The old
state-less form had no meaning in the per-flow cookie world — there
is no single cookie name to clear without knowing which flow.

Callers on callback paths have URL state in hand. Bailouts with no
state should skip the call entirely; the 10-minute PKCE TTL handles
orphans.
Exposes PKCE_COOKIE_PREFIX and getPKCECookieNameForState for custom
adapters. PKCE_COOKIE_NAME remains as back-compat alias.
Update README.md and MIGRATION.md to reflect the 0.5.0 signature
change on clearPendingVerifier and introduce the new pure URL-
generation methods.
Per-flow PKCE verifier cookies. See MIGRATION.md#050--per-flow-pkce-cookies
and docs/superpowers/specs/2026-04-22-per-flow-pkce-cookies-design.md.
`createAuthService` returns a hand-rolled proxy cast to `AuthService`,
which silently omitted `getAuthorizationUrl` / `getSignInUrl` /
`getSignUpUrl`. Consumers saw them in autocomplete but hit
`TypeError: not a function` at runtime. Add the forwarders and cover
the factory surface with typeof checks + an end-to-end test.
@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented Apr 23, 2026

Greptile Summary

This PR replaces the single static wos-auth-verifier PKCE verifier cookie with per-flow cookies suffixed by an FNV-1a 32-bit hash of the sealed state (wos-auth-verifier-<fnv1a>), so concurrent sign-ins from multiple tabs no longer clobber each other. The breaking changes — clearPendingVerifier now requires options.state, and CreateAuthorizationResult gains a required cookieName field — are clearly documented in MIGRATION.md with before/after examples.

The implementation is consistent end-to-end: generateAuthorizationUrl derives the name from sealedState, handleCallback re-derives it from the callback URL's state parameter (same sealed blob), and clearPendingVerifier follows the same derivation. Absent-state paths (cookieName = null) correctly skip cookie I/O and rely on the 10-minute TTL, which is the right fail-safe. The pure URL helpers flagged in a prior review were dropped before merge.

Confidence Score: 5/5

Safe to merge — no P0/P1 findings; all changed paths are covered by 215 passing tests including concurrent-flow isolation, tamper, missing-cookie, scheme-agnostic delete, and redirectUri-override scenarios.

All remaining findings are at most P2. The hash derivation is correct (verified by known-answer vectors), the cookie read/write/clear paths are consistent, null-guarded absent-state paths are intentional, and the breaking changes are thoroughly documented. The prior concern about pure URL helpers is resolved by their removal.

No files require special attention.

Important Files Changed

Filename Overview
src/core/pkce/cookieName.ts New module introducing FNV-1a 32-bit hash and per-flow cookie name derivation; implementation is correct and verified against known-answer vectors.
src/core/pkce/generateAuthorizationUrl.ts Derives cookieName from sealedState via getPKCECookieNameForState and threads it through the size check, serialization, and return value; logic is sound.
src/service/AuthService.ts Correctly plumbs per-flow cookieName through createAuthorization, handleCallback, and clearPendingVerifier; null-guarded paths (absent state) are intentional and correctly fall back to TTL cleanup.
src/core/session/types.ts Flattens GetAuthorizationUrlResult into CreateAuthorizationResult and adds required cookieName: string; breaking change is well-documented in MIGRATION.md.
src/index.ts Exports PKCE_COOKIE_PREFIX and getPKCECookieNameForState for downstream adapter introspection; PKCE_COOKIE_NAME correctly removed as a public export.
src/core/pkce/cookieName.spec.ts Thorough known-answer tests for FNV-1a 32-bit (empty string offset basis, single char, multi-char) plus determinism and prefix-constant checks.
src/service/AuthService.spec.ts Comprehensive test updates: per-flow cookie isolation, concurrent flow non-interference, tampered/missing cookie scenarios, scheme-agnostic delete, and redirectUri threading all rewired to derived cookie names.
src/core/pkce/cookieOptions.ts Removes the static PKCE_COOKIE_NAME export; the cookie-options helper itself is unchanged.
MIGRATION.md Well-structured 0.5.0 migration section covering the state-required clearPendingVerifier change, removed exports, and updated firewall/proxy checklist.

Reviews (5): Last reviewed commit: "chore: drop PKCE_COOKIE_NAME and GetAuth..." | Re-trigger Greptile

Comment thread src/service/AuthService.ts Outdated
Remove `getAuthorizationUrl` / `getSignInUrl` / `getSignUpUrl`. The
pure variants were added on speculation — no concrete consumer exists,
and Greptile flagged a real footgun: a caller who uses them for a
document-request redirect gets a broken callback with no surfaced
`sealedState` to recover with. The "wasted cookie write on non-document
requests" motivation is micro-optimization that adopters can solve via
middleware hygiene (call `createSignIn` only on the actual sign-in
route) rather than a parallel API surface. Per-flow cookie naming and
the `clearPendingVerifier({ state })` breakage remain — those are the
load-bearing fixes in 0.5.0.
@nicknisi nicknisi requested a review from gjtorikian April 23, 2026 02:09
Comment thread src/index.ts Outdated
Comment thread src/core/session/types.ts Outdated
Both are dead weight after the 0.5.0 changes:

- `PKCE_COOKIE_NAME` pointed at `'wos-auth-verifier'`, which no cookie
  on the wire has anymore. Nothing in `src/` uses it. Verified neither
  authkit-tanstack-start nor authkit-sveltekit imports it.
- `GetAuthorizationUrlResult` was a one-field interface that only
  existed as a shared base for the pure URL helpers we reverted.
  Inlined into `CreateAuthorizationResult`.

Per-flow cookie name is still derivable via `PKCE_COOKIE_PREFIX` +
`getPKCECookieNameForState(state)`.
@nicknisi nicknisi merged commit 64e82e3 into main Apr 23, 2026
6 checks passed
@gjtorikian gjtorikian deleted the pkce-per-flow-cookies branch April 23, 2026 18:08
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.

2 participants