Skip to content

feat: always pass nonce & always seal state parameter#390

Merged
nicknisi merged 2 commits intoworkos:nicknisi/pkce-support-fixesfrom
cn-stephen:auth-cleanup
Mar 13, 2026
Merged

feat: always pass nonce & always seal state parameter#390
nicknisi merged 2 commits intoworkos:nicknisi/pkce-support-fixesfrom
cn-stephen:auth-cleanup

Conversation

@cn-stephen
Copy link

This PR enhances the security and reliability of the authentication flow by ensuring the OAuth state parameter is always encrypted (sealed) and verified upon callback. It also introduces structured runtime validation for authentication state data.

refs:

If state is used for carrying application state, and the integrity of its contents is a concern, clients MUST protect state against tampering and swapping.

Key Changes

  • Sealed State Management: Replaced the previous Base64-encoded state parameter with a sealed (encrypted) object using iron-session. This ensures that sensitive data like returnPathname and customState cannot be tampered with, read by the client or guessed by an attacker.

  • Mandatory Nonce & CSRF Protection: A unique nonce is now generated and included in the state for every request, regardless of whether PKCE is enabled. This provides a unified "Defense in Depth" approach to CSRF protection, and simplifies code paths.

  • Runtime Type Safety: Integrated valibot to define a strict StateSchema. This ensures that the data unsealed during the callback is structured correctly before it is used. This is because TypeScript cannot guarantee that unseal will actually return an object in the same shape as was sealed at compile time, so we have to do so at runtime to trust our type system.

  • Simplified Callback Logic: Refactored handleAuth to strictly enforce the presence of the code, state, and PKCE cookie. The callback now automatically destroys the PKCE cookie regardless of success or failure to prevent replay attacks and stale sessions.

BREAKING CHANGE
This PR is breaking the legacy state format. Previously, the library supported a mix of raw strings and Base64-encoded JSON objects. Now, it strictly requires an encrypted, sealed string that matches a single Schema.

Why this is OK

  • People don't use different versions of this library to generate the Auth URL, and handle the callback
  • The differences to URL & cookie handling do not spread anywhere outside this library, they are self-contained
  • Therefore if folks upgrade to a newly released version, all their auth will still "just work"

@cn-stephen cn-stephen requested a review from a team as a code owner March 12, 2026 22:28
@cn-stephen cn-stephen requested review from alisherry and removed request for a team March 12, 2026 22:28
@cn-stephen
Copy link
Author

cc @nicknisi tyvm

@nicknisi
Copy link
Member

nicknisi commented Mar 13, 2026

@cn-stephen Thanks for this. Taking a look now. Two changes I'm considering:

  • Dropping valibot as a dependency. It feels heavy for this check when a simple runtime type guard achieves the same thing. The sealed cookie is written by our own library and encrypted with iron-session, so the only way the shape is wrong is if the cookie password changed or the cookie was corrupted, both of which iron-session already throws on.
  • Keeping the customState outside the sealed blob using the existing dot-separator convention (<sealed>.<customState>) rather than sealing it inside. Today consumers can pass a state string to getSignInUrl and see it raw in the callback URL. Sealing it changes that contract. Maintaining the dot-separator preserves backwards compatibility while still encrypting all the internal state (nonce, returnPathname, codeVerifier).

I'm also going to add graceful PKCE degradation: try/catch around workos.pkce.generate() so that environments where the SDK doesn't support PKCE fall back to nonce-only CSRF protection automatically, instead of breaking. That removes the need for WORKOS_DISABLE_PKCE as a mandatory escape hatch (we can keep it as a force-off).

What do you think of these proposed changes?

@cn-stephen
Copy link
Author

Love it!

My somewhat devil's advocate replies:

  • valibot bundle shakes down it 1.3 KiB <-- https://github.com/open-circle/valibot - What it's providing is if a developer accidentally changes what is sealed to a different shape, right now TS would still compile, but at runtime the code would still pass through getStateFromPKCECookieValue. The runtime behaviour from there would be undefined which feels dangerous in such a security critical path. Theoretically yeah, something would fail, but we're lying to TypeScript and I'd very much rather guard against undefined behaviour

  • Keeping the customState outside the sealed blob <-- this will allow your users to do a "footgun" of putting secure info into the URL that shouldn't be exposed. Because we allow passing arbitrary info in, some might be secure. In case it is, If state is used for carrying application state, and the integrity of its contents is a concern, clients MUST protect state against tampering and swapping.. I really think we should keep the State encrypted always.

Thanks for giving this PR a fair go :)

@nicknisi
Copy link
Member

Fair on both points!

Remove the '/' default for returnPathname in getAuthorizationUrl so
handleAuth({ returnPathname }) isn't silently ignored.

Fix "delete PKCE cookie after failed authentication" test to set the
state param so it actually exercises the auth failure path.
@nicknisi
Copy link
Member

I'm going to go ahead and merge this and get #388 ready for a review to get merged tomorrow. Thank you for this!

Also, we're getting reports that the PKCE-on-by-default change is breaking some customers with custom middleware proxies that don't propagate Set-Cookie headers on redirects. Arguably a gap in their setup, but since we introduced this as a minor release it's not fair to expect everyone to catch it.

The plan is to make PKCE opt-in for now (WORKOS_ENABLE_PKCE) and flip it to on-by-default in v3.

@nicknisi nicknisi merged commit daf6574 into workos:nicknisi/pkce-support-fixes Mar 13, 2026
6 checks passed
nicknisi added a commit that referenced this pull request Mar 13, 2026
Co-authored-by: Nick Nisi <nick.nisi@workos.com>
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