fix(auth): keep SSO working when the app and API are on different ori… - #1202
fix(auth): keep SSO working when the app and API are on different ori…#1202njbrake wants to merge 5 commits into
Conversation
…gins _Note: this PR description was drafted by Claude via back-and-forth with @njbrake. The reasoning and decisions are his; the prose is Claude's._ Fixes two SSO failures that only appear when the app and the API are served from different origins. Both are invisible in the Compose and ALB setups, where one origin proxies both. **Sign-in returns a bare `NOT_FOUND`.** Better Auth resolves its error redirect as `onAPIError.errorURL || ${baseURL}/error`, and `baseURL` is the API origin, which serves no HTML. Any auth failure lands on a blank 404 instead of the app. Now sets `onAPIError.errorURL` to `APP_URL`. **The OAuth state cookie is rejected.** `__Secure-better-auth.state` is set by the API origin and read back during a redirect initiated from the app origin, so a cross-site `SameSite=Lax` cookie is dropped and sign-in fails. `account.skipStateCookieCheck` is now enabled only when `APP_URL` and the backend origin actually differ, so single-origin deployments keep the check. PKCE still protects the exchange, and the state parameter is still round-tripped through the provider. The cookie is a second layer that a split-origin deployment cannot rely on. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ital0
left a comment
ital0
left a comment
There was a problem hiding this comment.
Thanks for digging into this one. The split-origin failure mode is subtle, and the write-up in the description made it easy to follow. The errorURL change looks good to me as is.
A few things I'd like before merging:
skipStateCookieCheck
This is my main concern. The state cookie is what ties the OAuth callback to the browser that started the flow. With the check skipped, someone with an account on the same IdP could start a login, capture their own callback URL (code + state) without consuming it, and hand it to a victim. The victim's browser completes the flow and ends up logged into the attacker's account, and anything the victim types lands in the attacker's data. PKCE doesn't cover this, since the verifier lives server-side in the verification row: it binds the code exchange to the server, not to the initiating browser.
I know the alternative today is SSO being fully broken on split-origin deploys, so I don't want to block on perfection. Two ideas:
- If our split-origin deploys share a registrable domain (app.example.com + api.example.com), they're actually same-site and SameSite=Lax cookies still work.
advanced.crossSubDomainCookiesmight let us keep the check there and reserve the skip for genuinely cross-site setups. - The current condition compares origins, which is broader than SameSite. localhost:1420 vs localhost:8000 is cross-origin but same-site, so we also drop the check in dev and in the whole test suite. Worth tightening, or at least calling out.
One wording suggestion: the comment says the cookie was "only an additional binding check", which understates the login CSRF window a bit. Naming the trade-off explicitly will help future readers.
Possible redirect loop on persistent errors
The frontend never reads ?error=. In SSO mode the flow becomes: callback error → appUrl/?error=... → auth gate → /sso-redirect → auto-starts SSO on mount → IdP → error again. Recoverable errors (stale state) fix themselves on retry, which is great, but persistent ones (unlinked account, IdP misconfiguration) loop forever with no message. Before this change the bare 404 at least stopped the cycle. Could we point errorURL at a dedicated error route, or have SsoRedirect check for ?error= and show the retry UI it already has?
Tests
One side effect worth knowing: createTestSettings uses localhost:1420 and localhost:8000, so every existing OIDC/SAML integration test now runs with the skip active, and the cookie-check path loses coverage. A small unit test on the shape of the createAuth options (skip present only when origins differ, errorURL set) would document the contract.
- keep the OAuth state cookie check in every deployment: the signed cookie is the only thing binding a callback to the browser that started the flow, so skipping it to accommodate a cross-site split reopened login CSRF. Split-origin deployments must keep APP_URL and BETTER_AUTH_URL same-site instead, which is now documented. - point onAPIError.errorURL at /auth-error rather than the app root: in SSO mode the root sits behind the auth gate and restarts an IdP round-trip on mount, so a persistent failure (unlinked account, IdP misconfiguration) looped forever. The new route is unguarded and terminal, and surfaces the provider error code for support. - cover state binding end to end: missing cookie, an attacker state replayed in a victim browser, single-use consumption, and the redirect target of every failure. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Preview environment deployed 🚀
Stack: Auto-destroys on PR close/merge. Login via the bundled Keycloak realm — |
|
You were right about the state cookie, so I dropped skipStateCookieCheck entirely rather than tightening the predicate. Login CSRF. better-auth 1.6.9's state.mjs confirms your read: under storeStateStrategy: "database" the verification row makes state single-use, but the signed cookie is the only browser binding. New sso-callback-state.test.ts drives the real callback: a fresh state with no cookie, and an attacker's state replayed against a victim's cookie, both stop before the token exchange. I checked that the test discriminates by re-adding the skip, at which point both reach the code exchange and the tests fail. crossSubDomainCookies. It doesn't help the deployment I need, which is Railway. up.railway.app is on the Public Suffix List, so web-x.up.railway.app and api-x.up.railway.app are different registrable domains, not siblings. That also means the session cookie (SameSite=Lax, 7 day) would never reach the app even with state fixed, so the original diff would not have finished the job. I'm moving those services onto subdomains of one custom apex, which makes everything same-site and needs no code. Redirect loop. errorURL now targets a dedicated /auth-error route instead of the app root. Verified in a browser against a production bundle built with VITE_AUTH_MODE=sso: /?error=... redirects into /sso-redirect and drops the error param, while /auth-error holds. The page reads both ?error= and ?state=, since one better-auth path reports state_not_found on the latter, and renders error_description as text. Tests. Those 6 plus 4 on the resolved option shape. Test settings stay cross-origin, which no longer costs coverage now that the skip is gone. Two things from the source dive. SAML never checked this cookie, since parseRelayState passes skipStateCookieCheck: true internally, so this only ever affected OIDC. And the state cookie's Max-Age is 300 while the verification row lives 10 minutes, so an IdP login slower than 5 minutes fails the same way. Pre-existing, so I left it. The same-site requirement is now written up in docs/self-hosting/configuration.md.
|
ital0
left a comment
ital0
left a comment
There was a problem hiding this comment.
Nice work on the follow-up.
A few small things that I believe worth to take a look.
| globalThis.fetch = savedFetch | ||
| process.env.TRUSTED_ORIGINS = savedOrigins | ||
| if (cleanup) { | ||
| await cleanup().catch(() => {}) |
There was a problem hiding this comment.
The repo pattern is one transaction per test with cleanup() in afterEach (backend/docs/testing.md is pretty explicit about it). The shared beforeAll transaction works here since each startFlow() writes fresh rows, but worth either matching the pattern or a short comment on why not. Also, .catch(() => {}) hides rollback failures silently.
| // that started the flow — the verification row makes state single-use but not | ||
| // browser-bound. Skipping the check reopens login CSRF, so split-origin | ||
| // deployments must keep the app and API same-site instead. | ||
| const sameSite = optionsFor({ appUrl: 'https://app.example.com', betterAuthUrl: 'https://api.example.com' }) |
There was a problem hiding this comment.
This runs in the default consumer mode, so options.account is undefined and not.toHaveProperty passes without exercising anything. Pass authMode: 'oidc' so the test would actually catch a SSO-conditional skip coming back. (The callback suite still catches it, so this is belt-and-suspenders, but right now this test is green for free.)
| const replace = mock(() => {}) | ||
| const originalLocation = window.location | ||
|
|
||
| Object.defineProperty(window, 'location', { |
There was a problem hiding this comment.
window.location is replaced globally and never restored; the afterEach only clears the mock. The fake object leaks into other test files running in the same worker. Restoring the original in an afterAll would fix it.
| // Better Auth reports the code as `?error=` on most paths, but a missing state | ||
| // parameter arrives as `?state=state_not_found`. | ||
| const code = searchParams.get('error') || searchParams.get('state') || 'unknown_error' | ||
| const description = searchParams.get('error_description') |
There was a problem hiding this comment.
this pageview goes to PostHog with the full query string as $current_url (usePageTracking appends location.search, and sanitizeUrl only rewrites the /chats/:id pathname). With telemetry enabled, an IdP-supplied error_description lands in analytics. Stripping the query for this route in sanitizeUrl, or clearing the params here after reading them, should cover it.
| <div className="flex flex-col items-center gap-2"> | ||
| <h1 className="text-4xl font-semibold tracking-tight">Sign-in failed</h1> | ||
| <p className="text-muted-foreground">{describeAuthError(code)}</p> | ||
| <p className="text-[length:var(--font-size-xs)] text-muted-foreground">{description || code}</p> |
There was a problem hiding this comment.
When error_description is present, the raw code disappears from the page, though the JSDoc above says the code is always shown so support tickets keep the real signal. Showing both (labeled) would match the comment. Confirmed on the preview deploy: ?error=idp_exploded&error_description=Client+not+registered renders only the description.
|
|
||
| The simplest deployments put the frontend and the backend behind one origin (Compose, the ALB, a k8s ingress). If you split them, `APP_URL` and `BETTER_AUTH_URL` must stay **same-site**: two hostnames under one registrable domain, such as `app.example.com` and `api.example.com`. Different ports or subdomains of a shared parent are fine. | ||
|
|
||
| Hostnames on different registrable domains break sign-in. The auth cookies are `SameSite=Lax`, so a cross-site deployment loses both the OAuth `state` cookie (the callback fails with `state_security_mismatch` after a successful IdP login) and the session cookie (`/get-session` returns 401 from the app origin). Watch out for PaaS hostnames that look like subdomains but are not: `up.railway.app`, `vercel.app`, and similar are on the [Public Suffix List](https://publicsuffix.org/list/), which makes `web.up.railway.app` and `api.up.railway.app` cross-site. Attach custom domains under one apex instead. |
There was a problem hiding this comment.
The redirect actually delivers ?error=state_mismatch (the new callback tests assert exactly that), so an operator searching logs for state_security_mismatch won't find it. Worth using the delivered code here.
- isolate sso-callback-state tests in a per-test transaction so consumed verification rows cannot leak between tests - run the state-cookie assertions in SSO mode, where `account` exists, so they cannot pass vacuously - restore the original window.location after the auth-error suite instead of leaving a global stub for later test files - always render the error code next to the provider description, so support tickets keep the real signal - strip the query string from /auth-error before analytics see it, since error_description is third-party text - correct the documented error code for a cross-site callback failure
|
Thanks @ital0 ! I think Claude and I took care of those remaining issues. And please let me know if you would like the PRs presented in a different way, I know I sent a whole batch of them at once, if you want them submitted more gradually of if you want more or less AI involved in these PRs, let me know :) I saw and used a handful of the |
This deployment was running `skipStateCookieCheck: true`, applied whenever the app and API origins differ, which they do here. Upstream review of thunderbird#1202 established that this reopens login CSRF, and the reasoning holds for us: Under `storeStateStrategy: "database"` the verification row makes `state` single-use, but the signed cookie is the only thing binding a callback to the browser that began the flow. PKCE does not close the gap, because the verifier lives server-side in that same row: it binds the code exchange to the server, not to the initiating browser. So an attacker with an account on the same Keycloak realm could start a login, capture their own unconsumed callback URL, and hand it to a victim, whose browser completes the flow and lands in the attacker's account. Dropping the skip costs nothing here. It was only ever needed for Railway's generated hostnames, which sit on the Public Suffix List and so make sibling subdomains cross-site. This deployment now serves the app and API as subdomains of one apex, which is same-site, so `SameSite=Lax` delivers the state cookie normally. Also brings over the reviewed error-handling half, which never made it back to this branch: `onAPIError.errorURL` targets an unguarded, terminal `/auth-error` page rather than the app root. In SSO mode the root sits behind the auth gate, which redirects to the sign-in route, which starts a fresh IdP round-trip on mount, so a persistent failure looped through the identity provider forever. `sanitizeUrl` strips the query string for that route so an IdP-supplied `error_description` does not reach analytics. The page's retry button targets `/sign-in`, this branch's route, rather than upstream's `/sso-redirect`, which does not exist here. Verified the new tests discriminate: reintroducing the skip fails all three guards, including the attacker-replay case. Frontend 4349 pass, backend 1064 pass, 0 lint errors.
ital0
left a comment
ital0
left a comment
There was a problem hiding this comment.
Thanks for the careful follow-up. The final direction resolves the state-cookie and automatic redirect-loop concerns from my earlier reviews, and the callback tests add useful coverage.
I did one more line-by-line pass and left focused comments on two sibling flows that still fall outside the new landing page, SAML and desktop loopback, plus the remaining trust and privacy boundary around error query data. The other notes are smaller copy, test, and self-hosting documentation corrections.
One general housekeeping item: main now contains broader URL sanitization from #1210, so please preserve that implementation while resolving the current conflict.
The overall direction makes sense to me. I think addressing the sibling flows and query-data handling will make the fix hold across the deployment modes this project supports.
| // /sso-redirect, which starts a new IdP round-trip on mount. A persistent | ||
| // failure (unlinked account, IdP misconfiguration) would loop through the | ||
| // IdP forever. /auth-error is unguarded and terminal. | ||
| onAPIError: { errorURL: `${settings.appUrl}/auth-error` }, |
There was a problem hiding this comment.
onAPIError.errorURL does not cover the SAML result path in @better-auth/sso 1.6.9. processSAMLResponse() ignores relayState.errorURL and sends validation or linking failures to relayState.callbackURL. Thunderbolt sets that to /, so AuthGate starts SSO again and the redirect loop remains for SAML.
| <p className="text-[length:var(--font-size-xs)] text-muted-foreground">Error code: {code}</p> | ||
| </div> | ||
|
|
||
| <Button onClick={() => window.location.replace(isSsoMode() ? '/sso-redirect' : '/')}>Try again</Button> |
There was a problem hiding this comment.
Desktop OIDC failures land on this web page while startSsoFlowLoopback() continues waiting for its localhost callback for up to five minutes. This button starts a normal web flow in the system browser and never settles the original desktop promise, leaving the app pending until timeout.
| } | ||
| })() | ||
|
|
||
| if (queryStrippedRoutes.some((route) => route === pathname)) { |
There was a problem hiding this comment.
This only sanitizes selected event properties. PostHog also records $session_entry_url, and same-origin analytics requests carry the full page URL in Referer, which the backend proxy forwards. error_description can still reach PostHog despite the test asserting that it never does.
| <div className="flex flex-col items-center gap-2"> | ||
| <h1 className="text-4xl font-semibold tracking-tight">Sign-in failed</h1> | ||
| <p className="text-muted-foreground">{describeAuthError(code)}</p> | ||
| {description && <p className="text-[length:var(--font-size-xs)] text-muted-foreground">{description}</p>} |
There was a problem hiding this comment.
/auth-error is public and renders error_description verbatim under Thunderbolt branding. A crafted URL can display arbitrary instructions. React escapes markup, but it does not prevent misleading text. When an IdP omits the description, the SSO plugin can also forward the literal value undefined.
| const staleFlowCodes = new Set([ | ||
| 'invalid_state', | ||
| 'please_restart_the_process', | ||
| 'state_mismatch', |
There was a problem hiding this comment.
state_mismatch does not mean the flow went stale. The callback tests use it for a missing or different-browser cookie, and the docs use it for persistent cross-site configuration. The page therefore reports a timeout and recommends retry for failures that happened immediately and cannot be fixed by retry.
| return 'Your sign-in took too long to complete. Starting over usually fixes this.' | ||
| } | ||
| if (code === 'account_not_linked') { | ||
| return 'This email is already registered with a different sign-in method. Ask your administrator to link the accounts.' |
There was a problem hiding this comment.
Thunderbolt has no administrator account-linking flow, while trusted SSO providers already link matching emails automatically. This message directs the user to an unavailable action and hides the more likely original-sign-in or identity-mapping problem.
|
|
||
| ### Serving the app and API on separate hostnames | ||
|
|
||
| The simplest deployments put the frontend and the backend behind one origin (Compose, the ALB, a k8s ingress). If you split them, `APP_URL` and `BETTER_AUTH_URL` must stay **same-site**: two hostnames under one registrable domain, such as `app.example.com` and `api.example.com`. Different ports or subdomains of a shared parent are fine. |
There was a problem hiding this comment.
This split-origin recipe is incomplete. SameSite is schemeful, so the URLs need the same scheme as well as the same registrable domain. Split origins also depend on CORS_ORIGINS, TRUSTED_ORIGINS, credentialed CORS, and an absolute VITE_THUNDERBOLT_CLOUD_URL pointing at the API. A deployment can follow the current paragraph and still fail sign-in.
|
|
||
| The simplest deployments put the frontend and the backend behind one origin (Compose, the ALB, a k8s ingress). If you split them, `APP_URL` and `BETTER_AUTH_URL` must stay **same-site**: two hostnames under one registrable domain, such as `app.example.com` and `api.example.com`. Different ports or subdomains of a shared parent are fine. | ||
|
|
||
| Hostnames on different registrable domains break sign-in. The auth cookies are `SameSite=Lax`, so a cross-site deployment loses both the OAuth `state` cookie (after a successful IdP login the callback redirects to `APP_URL/auth-error?error=state_mismatch`, and the backend logs `state_security_mismatch`) and the session cookie (`/get-session` returns 401 from the app origin). Watch out for PaaS hostnames that look like subdomains but are not: `up.railway.app`, `vercel.app`, and similar are on the [Public Suffix List](https://publicsuffix.org/list/), which makes `web.up.railway.app` and `api.up.railway.app` cross-site. Attach custom domains under one apex instead. |
There was a problem hiding this comment.
Better Auth 1.6.9 returns 200 null from /get-session when the session cookie is absent, not 401. The documented status points operators at the wrong troubleshooting symptom.
| const savedOrigins = process.env.TRUSTED_ORIGINS | ||
|
|
||
| beforeAll(() => { | ||
| globalThis.fetch = Object.assign(stubbedFetch, { preconnect: () => {} }) as unknown as typeof fetch |
There was a problem hiding this comment.
as unknown as typeof fetch bypasses the fetch contract. The test can continue compiling after the stub stops matching Bun's real fetch signature, which weakens this process-wide boundary.
Resolves the src/lib/posthog.tsx conflict in favour of main's thunderbird#1210 sanitization, which already strips every query string in sanitizeUrl — the route-specific list this branch added is redundant beside it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QUZcKJcXPqNx8kSJCPrbby
- divert `?error=` at the AuthGate to /auth-error, so a SAML linking failure lands on a terminal page instead of looping back through the IdP - add a `/desktop-error/:port` backend route and pass `errorCallbackURL` from desktop-initiate, so a desktop SSO failure reaches the waiting loopback server instead of timing out after five minutes - attribute and truncate the provider's `error_description` on /auth-error, and drop the literal `undefined` Better Auth sends - correct the `state_mismatch` and `account_not_linked` copy - sanitize the `$session_entry_*` and `$set_once` URL properties and stop forwarding `Referer` through the PostHog proxy - correct and expand the split-origin self-hosting docs
ital0
left a comment
ital0
left a comment
There was a problem hiding this comment.
Thanks for the careful follow-up. The latest changes address most of my earlier comments.
I left one remaining inline note on the desktop error callback. In the versions pinned by the backend, OIDC state failures and SAML failures still bypass the loopback error path, leaving the desktop pending until the five-minute timeout.
| method: 'POST', | ||
| headers: { 'Content-Type': 'application/json' }, | ||
| body: JSON.stringify({ providerId: 'sso', callbackURL }), | ||
| body: JSON.stringify({ providerId: 'sso', callbackURL, errorCallbackURL }), |
There was a problem hiding this comment.
Thanks for adding the desktop error callback. I traced this through Better Auth and @better-auth/sso 1.6.9, and two branches still bypass it. An OIDC state failure happens before Better Auth can recover errorCallbackURL, so it falls back to the web /auth-error. SAML stores this value in RelayState, but processSAMLResponse() redirects with callbackURL instead. Neither branch reaches the loopback server, leaving the desktop flow pending until its five-minute timeout.
2 participants
…gins
Note: this PR description was drafted by Claude via back-and-forth with @njbrake. The reasoning and decisions are his; the prose is Claude's.
Fixes two SSO failures that only appear when the app and the API are served from different origins. Both are invisible in the Compose and ALB setups, where one origin proxies both.
Sign-in returns a bare
NOT_FOUND. Better Auth resolves its error redirect asonAPIError.errorURL || ${baseURL}/error, andbaseURLis the API origin, which serves no HTML. Any auth failure lands on a blank 404 instead of the app. Now setsonAPIError.errorURLtoAPP_URL.The OAuth state cookie is rejected.
__Secure-better-auth.stateis set by the API origin and read back during a redirect initiated from the app origin, so a cross-siteSameSite=Laxcookie is dropped and sign-in fails.account.skipStateCookieCheckis now enabled only whenAPP_URLand the backend origin actually differ, so single-origin deployments keep the check.PKCE still protects the exchange, and the state parameter is still round-tripped through the provider. The cookie is a second layer that a split-origin deployment cannot rely on.