Implement PKCE and sealed OAuth state flow#68
Conversation
Co-Authored-By: nick.nisi@workos.com <nick.nisi@workos.com>
Original prompt from nick.nisi@workos.com
|
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
Greptile SummaryThis PR implements PKCE (RFC 7636, S256) and sealed OAuth state on top of the existing AuthKit React Router integration. Confidence Score: 4/5Safe to merge after reviewing the open P2 items; the PKCE + double-submit cookie CSRF implementation is correct and the breaking-change migration path is documented. The core PKCE and CSRF logic is sound: sealed state is used consistently for both the URL parameter and the cookie value, the CSRF check is byte-equal comparison, the codeVerifier is correctly threaded to authenticateWithCode, single-use cookies are cleared on all exit paths, and the issuer claim is now validated in jwtVerify. No P0/P1 bugs found in the changed code. Score is 4 rather than 5 because this is a security-critical breaking change deserving extra human verification, and the three P2 findings are worth the author's attention before a stable release. src/pkce.ts (X-Forwarded-Proto trust), src/authkit-callback-route.ts (callback URL mutation), src/session.ts (jwtVerify audience — addressed with inline comment, verify WorkOS token spec) Important Files Changed
Sequence DiagramsequenceDiagram
participant Browser
participant App as App Server
participant WorkOS
Browser->>App: GET /login
App->>App: pkce.generate() codeVerifier + codeChallenge
App->>App: sealData nonce + codeVerifier + returnPathname = sealedState
App->>App: getPKCECookieString(sealedState) builds Set-Cookie header
App-->>Browser: 302 to WorkOS with Set-Cookie wos-auth-verifier-hash
Browser->>WorkOS: GET /authorize with state=sealedState and code_challenge
WorkOS->>WorkOS: authenticate user
WorkOS-->>Browser: 302 to /callback with code and state=sealedState
Browser->>App: GET /callback with code and state and PKCE cookie
App->>App: readPKCECookie extracts pkceCookieValue
App->>App: CSRF check state equals pkceCookieValue
App->>App: unsealData extracts codeVerifier
App->>WorkOS: authenticateWithCode with code and codeVerifier
WorkOS-->>App: accessToken + refreshToken + user
App->>App: encryptSession and commitSession
App-->>Browser: 302 to returnPathname with session cookie and cleared PKCE cookie
Reviews (9): Last reviewed commit: "Simplify PKCE callback comments and clea..." | Re-trigger Greptile |
| "dependencies": { | ||
| "@workos-inc/node": "^7.41.0", | ||
| "iron-session": "^8.0.1", | ||
| "jose": "^5.2.3" | ||
| "jose": "^5.2.3", | ||
| "tslib": "^2.8.1", | ||
| "valibot": "^1.2.0" | ||
| }, |
There was a problem hiding this comment.
@workos-inc/node dropped from runtime dependencies
@workos-inc/node was removed from dependencies and kept only in devDependencies, but workos.ts does import { WorkOS } from '@workos-inc/node' — a non-type, runtime import. Any consumer who installs this package fresh (without @workos-inc/node in their own project) will get a module-not-found error at runtime.
The PR description says this was a version bump (^7.41.0 → ^8.9.0), not a removal, so this looks like an unintentional regression. It should either be restored to dependencies or, if the intent is for apps to manage the version themselves, explicitly added to peerDependencies.
There was a problem hiding this comment.
Good catch — restored in 7bd2c83. @workos-inc/node@^8.9.0 is back in dependencies, and I also realigned the devDependencies pin from ^8.13.0 to ^8.9.0 so both entries reference the same range.
Co-Authored-By: nick.nisi@workos.com <nick.nisi@workos.com>
Co-Authored-By: nick.nisi@workos.com <nick.nisi@workos.com>
Apply review feedback from the PKCE/CSRF PR: - Derive the PKCE cookie's Secure attribute from the live request protocol when a Request is available, falling back to the configured redirectUri and warning on unparseable URIs. Fixes local dev on http://localhost with an https:// WORKOS_REDIRECT_URI silently dropping the cookie. - Replace the hand-rolled FNV-1a cookie-name fingerprint with a SHA-256 slice from node:crypto. No new deps, shorter, and no collision-tradeoff caveat to document. - Drop the misleading 'SameSite=None for iframes' comment on a code path that hardcodes Lax. Iframe/embed flows aren't supported; if we ever add them, it belongs in its own change. - Let getSignInUrl/getSignUpUrl accept the incoming Request as a second argument so loaders can thread it through transparently. - Stop emitting an empty Set-Cookie: '' header from switchToOrganization when refreshSession returns no cookie — omit the header entirely. Co-Authored-By: nick.nisi@workos.com <nick.nisi@workos.com>
getSignInUrl/getSignUpUrl now return { url, headers } and the Set-Cookie
must travel on the redirect that starts the OAuth flow. The old pattern
of loading a URL into page data and rendering it in a <Link> no longer
works — the cookie and the URL must leave the server on the same
response.
Replace the broken example with dedicated /login and /signup redirect
routes, call that pattern out as the way to offer sign-in/sign-up from
any page, and add a 'Migrating from 0.4.x' section documenting the
return-shape change, the required redirect-route pattern, the new
request threading for Secure detection, and the @workos-inc/node ^8.9.0
minimum. Also add CHANGELOG.md to track behavioral changes.
Co-Authored-By: nick.nisi@workos.com <nick.nisi@workos.com>
Co-Authored-By: nick.nisi@workos.com <nick.nisi@workos.com>
- Raise engines.node floor to >=20.15.0 to match @workos-inc/node@^8.13.x,
whose engines declaration was stricter than ours. Users on 20.0-20.14
would otherwise see unsupported-engine install failures despite our
metadata advertising support.
- Honor X-Forwarded-Proto (leftmost value when chained) ahead of
request.url when deriving the PKCE cookie's Secure attribute, so
deployments that terminate TLS upstream and forward http:// internally
still emit Secure cookies for the public https:// site.
- Thread the per-call redirectUri override from getAuthorizationUrl
through to resolveSecure so a caller passing
getAuthorizationUrl({ redirectUri }) without a Request gets a cookie
whose Secure attribute matches the override's scheme rather than the
unrelated global WORKOS_REDIRECT_URI.
Co-Authored-By: nick.nisi@workos.com <nick.nisi@workos.com>
Co-Authored-By: nick.nisi@workos.com <nick.nisi@workos.com>
Port two defensive improvements from the nicknisi/pkce branch:
1. Introduce sanitizeReturnPathname, a shared utility that restricts a
user-supplied return target to a same-origin pathname. Rejects
absolute URLs, protocol-relative paths, backslash-prefixed paths,
CRLF header-injection attempts, dot-segment traversal, URL-encoded
bypasses, and oversized inputs; returns '/' on rejection.
2. Apply the sanitizer in two places:
- getAuthorizationUrl sanitizes returnPathname before sealing so a
hostile caller cannot plant a malicious return target in the
encrypted state.
- authkit-callback-route sanitizes both the sealed-state value and
the configured default independently on the callback response, so
a tampered sealed state cannot erase a legitimate default.
3. Replace the pathname-or-pathname+search branching in the callback
with a single URL-based reconstruction that preserves pathname,
search params, and fragment together. /dashboard#section now stays
/dashboard#section instead of being percent-encoded.
Coverage: the new return-pathname.spec.ts mirrors the sanitization
suite from nicknisi/pkce (accepted paths; rejected absolute URLs,
protocol-relative, CRLF, dot segments, encoded bypasses, oversized,
non-string inputs). The callback spec adds tests for fragment
preservation, search+fragment together, and fall-back to the
configured default when a hostile sealed state is received.
Co-Authored-By: nick.nisi@workos.com <nick.nisi@workos.com>
sanitizeReturnPathname returns '/' both on legitimate '/' input and on rejection, which made the previous callback logic treat any sealed- state value of '/' as a rejection and fall through to the configured returnPathname option. A caller who explicitly asked to return home would instead land on the handler's default (e.g. '/dashboard'). Detect rejection by comparing the sanitized result against the raw input: state is only rejected when sanitization produced '/' from a non-'/' input. Adds a callback test covering an explicit '/' in the sealed state with a non-root configured option. Co-Authored-By: nick.nisi@workos.com <nick.nisi@workos.com>
Co-Authored-By: nick.nisi@workos.com <nick.nisi@workos.com>
Co-Authored-By: nick.nisi@workos.com <nick.nisi@workos.com>
Co-Authored-By: nick.nisi@workos.com <nick.nisi@workos.com>
Co-Authored-By: nick.nisi@workos.com <nick.nisi@workos.com>
Inline the one-call `clearPKCECookie` wrapper, trim over-narrated comments on the callback handler and returnPathname disambiguation, fix the inaccurate "legacy name" comment in getPKCECleanupCookieStrings (the bare name has always been a prefix only), drop an unused spread, and remove narrative preambles that duplicated the test titles.
Per CHANGELOG policy, pre-1.0 minor bumps signal breaking changes. The PKCE/CSRF release in #68 introduced two: getSignInUrl/getSignUpUrl/ getAuthorizationUrl now return { url, headers }, and the engines.node floor moved to >=20.15.0 to match @workos-inc/node ^8.13.x. Cuts the [Unreleased] section in CHANGELOG.md to [0.11.0] - 2026-04-27 and refreshes the lockfile (sync the engines bump that was previously left dirty in the working tree).
Summary
Implements a PKCE-based authorization flow for
authkit-react-router, adapted to React Router v7's loader-based architecture.What changed
getAuthorizationUrlnow generates acodeVerifier/codeChallengepair viaworkos.pkce.generate()and threads the verifier through toauthenticateWithCodeon callback.{ nonce, codeVerifier, customState?, returnPathname? }is encrypted withiron-sessionusing the configuredcookiePassword(10-minute TTL).stateis sent both as a URL parameter and stored in a flow-specific HTTP-only cookiewos-auth-verifier-<hash>. The callback requires the cookie to be present and byte-equal to thestateparameter before exchanging the code.Secureattribute is derived from the live request protocol when aRequestis threaded through, falling back to the configuredredirectUri. This avoids minting aSecurecookie that the browser would drop during local development.API changes (breaking, documented in CHANGELOG.md and the migration guide)
getSignInUrl,getSignUpUrl, andgetAuthorizationUrlnow return{ url, headers }instead of a barestring. TheSet-Cookieinheadersmust ride the same response that sends the user to WorkOS so the callback can recover the verifier and validate the returned state. They also accept an optionalRequestso the cookie'sSecureattribute matches the live protocol.Internal callsites (
authkitLoader,switchToOrganization,refreshSession) already propagate the new cookie; only user-facing routes that call these helpers directly need to adapt. The README's old "render a sign-in URL in a<Link>" pattern has been replaced with dedicated/loginand/signupredirect routes because the cookie and the URL need to leave the server on the same response.switchToOrganizationno longer emits an emptySet-Cookie: ''header whenrefreshSessionreturns without one — the header is now omitted entirely.Dependency bumps
@workos-inc/node:^7.41.0→^8.9.0(required forworkos.pkce.generate())valibot^1.2.0(state schema validation)tslib^2.8.1(required by valibot'simportHelpers)Review & Testing Checklist for Human
wos-auth-verifier-*cookie appears on the redirect to AuthKit and that the callback completes without a 500. Also try a second flow in a different tab/window to confirm the flow-specific cookie naming doesn't collide.code+statepair when the matching cookie is absent.Securebehavior: run the example app onhttp://localhostwithWORKOS_REDIRECT_URI=https://…and confirm thewos-auth-verifier-*cookie is set without theSecureattribute so the browser keeps it across the redirect.getSignInUrl,getSignUpUrl, orgetAuthorizationUrlthat treat the result as astring. They need to destructure{ url, headers }and passheaderstoredirect()/ the response.@workos-inc/nodev8 is OK in your deployment (it's ESM-first with a CJS shim; should be a drop-in but worth verifying in staging).Notes
{ url, headers }return shape, the redirect-route pattern, threadingrequest, and the@workos-inc/node ^8.9.0minimum.workos.spec.tswas rewritten to checkworkos.options.*directly rather than mocking@workos-inc/node, mirroring the approach inauthkit-nextjs. The previousjest.mock('@workos-inc/node', …)pattern didn't survive the v8 upgrade (ESM/CJS dual-build changes how Jest resolves the module).SameSite=Lax(notStrict) on purpose becauseStrictwould strip the cookie on the cross-site redirect back fromapi.workos.com.Link to Devin session: https://app.devin.ai/sessions/f5fd3b5da62544c7b6c41a6a18bd4314