From 5afb6d1efa056ac094781b056bc6ef908e703a9a Mon Sep 17 00:00:00 2001 From: vinc3nati Date: Tue, 18 Aug 2026 18:42:14 +0530 Subject: [PATCH 1/3] feat: handle replayed OIDC callbacks and improve auth error handling --- .changeset/oauth-callback-back-button.md | 5 ++ packages/frontend/src/App.tsx | 29 ++++++++--- packages/frontend/src/authStatusSearch.ts | 29 +++++++++++ .../frontend/tests/authStatusSearch.test.ts | 30 ++++++++++++ packages/trueforge/src/apis/auth.ts | 9 +++- .../trueforge/tests/unit/apis/auth.test.ts | 48 +++++++++++++++++++ 6 files changed, 143 insertions(+), 7 deletions(-) create mode 100644 .changeset/oauth-callback-back-button.md create mode 100644 packages/frontend/tests/authStatusSearch.test.ts diff --git a/.changeset/oauth-callback-back-button.md b/.changeset/oauth-callback-back-button.md new file mode 100644 index 000000000..2876f8b0e --- /dev/null +++ b/.changeset/oauth-callback-back-button.md @@ -0,0 +1,5 @@ +--- +'@truefoundry/trueforge': patch +--- + +Treat a replayed OIDC callback (browser Back after a successful login) as already signed-in instead of `/?error=login_failed`, and ignore that stale query when a session is still valid. diff --git a/packages/frontend/src/App.tsx b/packages/frontend/src/App.tsx index 41db8fbbc..464f2fc41 100644 --- a/packages/frontend/src/App.tsx +++ b/packages/frontend/src/App.tsx @@ -9,7 +9,7 @@ import { useEffect, useMemo, useState } from 'react'; import { AuthErrorScreen } from './AuthErrorScreen'; import { createAuthAwareFetch } from './authFetch'; import { probeSession, type SessionState } from './authSession'; -import { parseAuthErrorReason } from './authStatusSearch'; +import { parseAuthErrorReason, shouldShowAuthErrorScreen, stripAuthErrorSearch } from './authStatusSearch'; import { GetStartedScreen } from './GetStartedScreen'; import { LogoutButton } from './LogoutButton'; @@ -29,10 +29,9 @@ export function App() { // Gate boot on a non-redirecting `/me` probe: unauthenticated users see the // welcome screen instead of being bounced to login by the auth-aware fetch. + // Probe even when the URL has `?error=` — a replayed callback can leave that + // query on a session that is still valid. useEffect(() => { - if (authError != null) { - return; - } const state = { cancelled: false }; void probeSession().then(result => { if (!state.cancelled) { @@ -42,7 +41,25 @@ export function App() { return () => { state.cancelled = true; }; - }, [authError]); + }, []); + + useEffect(() => { + if (session !== 'authenticated') { + return; + } + if (parseAuthErrorReason(window.location.search) == null) { + return; + } + window.history.replaceState( + window.history.state, + '', + stripAuthErrorSearch({ + pathname: window.location.pathname, + search: window.location.search, + hash: window.location.hash, + }), + ); + }, [session]); useEffect(() => { if (session !== 'authenticated') { @@ -98,7 +115,7 @@ export function App() { const overrides: SlotOverrides = useMemo(() => ({ ShellActionsActionSlot: LogoutButton }), []); - if (authError != null) { + if (authError != null && shouldShowAuthErrorScreen({ authError, session })) { return ( diff --git a/packages/frontend/src/authStatusSearch.ts b/packages/frontend/src/authStatusSearch.ts index ae420378e..bedead589 100644 --- a/packages/frontend/src/authStatusSearch.ts +++ b/packages/frontend/src/authStatusSearch.ts @@ -1,3 +1,5 @@ +import type { SessionState } from './authSession'; + /** Reads `/?error=` from OIDC login failures. Returns null when not an error landing. */ export function parseAuthErrorReason(search: string): string | null { const reason = new URLSearchParams(search).get('error')?.trim(); @@ -6,3 +8,30 @@ export function parseAuthErrorReason(search: string): string | null { } return reason; } + +/** True only for a real login failure: `?error=` present and no valid session. */ +export function shouldShowAuthErrorScreen({ + authError, + session, +}: { + authError: string | null; + session: SessionState | 'checking'; +}): boolean { + return authError != null && session === 'unauthenticated'; +} + +/** Path + search + hash with the OIDC `error` query removed. */ +export function stripAuthErrorSearch({ + pathname, + search, + hash, +}: { + pathname: string; + search: string; + hash: string; +}): string { + const params = new URLSearchParams(search); + params.delete('error'); + const nextSearch = params.toString(); + return `${pathname}${nextSearch === '' ? '' : `?${nextSearch}`}${hash}`; +} diff --git a/packages/frontend/tests/authStatusSearch.test.ts b/packages/frontend/tests/authStatusSearch.test.ts new file mode 100644 index 000000000..85ff3ecee --- /dev/null +++ b/packages/frontend/tests/authStatusSearch.test.ts @@ -0,0 +1,30 @@ +import assert from 'node:assert/strict'; +import { describe, it } from 'node:test'; +import { parseAuthErrorReason, shouldShowAuthErrorScreen, stripAuthErrorSearch } from '../src/authStatusSearch'; + +describe('authStatusSearch', () => { + it('parseAuthErrorReason reads a non-empty error query', () => { + assert.equal(parseAuthErrorReason('?error=login_failed'), 'login_failed'); + assert.equal(parseAuthErrorReason('?error=%20'), null); + assert.equal(parseAuthErrorReason(''), null); + }); + + it('shouldShowAuthErrorScreen only for unauthenticated error landings', () => { + assert.equal(shouldShowAuthErrorScreen({ authError: 'login_failed', session: 'unauthenticated' }), true); + assert.equal(shouldShowAuthErrorScreen({ authError: 'login_failed', session: 'checking' }), false); + assert.equal(shouldShowAuthErrorScreen({ authError: 'login_failed', session: 'authenticated' }), false); + assert.equal(shouldShowAuthErrorScreen({ authError: null, session: 'unauthenticated' }), false); + }); + + it('stripAuthErrorSearch drops error and keeps other query and hash', () => { + assert.equal(stripAuthErrorSearch({ pathname: '/', search: '?error=login_failed', hash: '' }), '/'); + assert.equal( + stripAuthErrorSearch({ + pathname: '/sessions/abc', + search: '?error=login_failed&tab=1', + hash: '#composer', + }), + '/sessions/abc?tab=1#composer', + ); + }); +}); diff --git a/packages/trueforge/src/apis/auth.ts b/packages/trueforge/src/apis/auth.ts index b6e0ab881..64484a32f 100644 --- a/packages/trueforge/src/apis/auth.ts +++ b/packages/trueforge/src/apis/auth.ts @@ -4,7 +4,7 @@ import type { Configuration } from 'openid-client'; import type { Logger } from 'winston'; import { clearAuthCookie, ID_TOKEN_COOKIE, OAUTH_STATE_COOKIE, readOAuthStateCookie } from '../auth/cookies'; import { resolveUserContext } from '../auth/identity'; -import { authMiddleware } from '../auth/middleware'; +import { authMiddleware, resolveAuthUser } from '../auth/middleware'; import { buildLoginAuthorization, exchangeAuthorizationCode, getOidcVerify, safeReturnTo } from '../auth/oidc'; import { authLoginRoute, authLogoutRoute, meRoute, oAuthCallbackRoute } from '../routes/authRoutes'; import type { MeResponse } from '../schemas/auth'; @@ -50,6 +50,10 @@ export function createAuthRouter(params: { oidcClient: Configuration | undefined clearAuthCookie({ context: c, name: OAUTH_STATE_COOKIE }); if (pending?.state !== query.state || query.error || !query.code) { + // If already authenticated, redirect home instead of showing an error. + if (await resolveAuthUser(c)) { + return c.redirect('/', 302); + } // Only reflect a non-empty IdP `error_description` when the IdP returned an error. // No fallback to the `error` code — blank/missing descriptions use the default. // Our own validation failures (state mismatch / missing code) stay generic too. @@ -69,6 +73,9 @@ export function createAuthRouter(params: { oidcClient: Configuration | undefined return c.redirect(safeReturnTo(pending.return_to), 302); } catch (error) { params.logger.error('Failed to exchange authorization code', extractErrorLogFields(error)); + if (await resolveAuthUser(c)) { + return c.redirect(safeReturnTo(pending.return_to), 302); + } const reason = error instanceof Error ? error.message : 'login_failed'; return c.redirect(oauthErrorRedirect(reason), 302); } diff --git a/packages/trueforge/tests/unit/apis/auth.test.ts b/packages/trueforge/tests/unit/apis/auth.test.ts index fee01ddcd..b2dc24bc1 100644 --- a/packages/trueforge/tests/unit/apis/auth.test.ts +++ b/packages/trueforge/tests/unit/apis/auth.test.ts @@ -303,6 +303,54 @@ describe('auth router (auth enabled)', () => { expect(res.headers.get('location')).toBe('/?error=login_failed'); }); + it('GET /callback replays home when already authenticated and the state cookie is spent', async () => { + const token = await createIdToken(); + const res = await createAuthRouter({ oidcClient, logger }).request('/callback?code=abc&state=spent', { + redirect: 'manual', + headers: { Cookie: `${ID_TOKEN_COOKIE}=${token}` }, + }); + expect(res.status).toBe(302); + expect(res.headers.get('location')).toBe('/'); + }); + + it('GET /callback replays home when already authenticated even if the IdP returned an error', async () => { + const token = await createIdToken(); + const res = await createAuthRouter({ oidcClient, logger }).request( + '/callback?state=any&error=access_denied&error_description=user%20cancelled', + { + redirect: 'manual', + headers: { Cookie: `${ID_TOKEN_COOKIE}=${token}` }, + }, + ); + expect(res.status).toBe(302); + expect(res.headers.get('location')).toBe('/'); + }); + + it('GET /callback keeps the existing session when code exchange fails', async () => { + const token = await createIdToken(); + const router = createAuthRouter({ oidcClient, logger }); + const loginRes = await router.request('/login?return_to=/sessions/abc123', { redirect: 'manual' }); + const stateCookieRaw = cookieValue(setCookies(loginRes), STATE_COOKIE) ?? ''; + const authorizationUrl = new URL(loginRes.headers.get('location') ?? ''); + const state = authorizationUrl.searchParams.get('state') ?? ''; + + const fetchStub = globalThis.fetch; + const failingFetch: typeof fetch = async (input, init) => { + if (String(input) === `${ISSUER}/token` && init?.method === 'POST') { + return new Response('invalid_grant', { status: 400 }); + } + return fetchStub(input, init); + }; + globalThis.fetch = failingFetch; + + const res = await router.request(`/callback?code=abc&state=${state}&iss=${encodeURIComponent(ISSUER)}`, { + redirect: 'manual', + headers: { Cookie: `${STATE_COOKIE}=${stateCookieRaw}; ${ID_TOKEN_COOKIE}=${token}` }, + }); + expect(res.status).toBe(302); + expect(res.headers.get('location')).toBe('/sessions/abc123'); + }); + it('POST /logout clears id_token even when no cookie is present', async () => { const res = await createAuthRouter({ oidcClient, logger }).request('/logout', { method: 'POST', From 4a2e038722304f288b8658ad80499300ea473faf Mon Sep 17 00:00:00 2001 From: Vinit Date: Wed, 19 Aug 2026 16:48:02 +0530 Subject: [PATCH 2/3] Update packages/frontend/src/App.tsx Co-authored-by: sayan-truefoundry <136362719+sayan-truefoundry@users.noreply.github.com> --- packages/frontend/src/App.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/frontend/src/App.tsx b/packages/frontend/src/App.tsx index 464f2fc41..efe39c2a1 100644 --- a/packages/frontend/src/App.tsx +++ b/packages/frontend/src/App.tsx @@ -115,7 +115,7 @@ export function App() { const overrides: SlotOverrides = useMemo(() => ({ ShellActionsActionSlot: LogoutButton }), []); - if (authError != null && shouldShowAuthErrorScreen({ authError, session })) { + if (shouldShowAuthErrorScreen({ authError, session })) { return ( From 711ad3c84a3bbea7e7cea3754563573eb3bd92c5 Mon Sep 17 00:00:00 2001 From: vinc3nati Date: Wed, 19 Aug 2026 17:23:10 +0530 Subject: [PATCH 3/3] Refactor auth error handling in App component --- packages/frontend/src/App.tsx | 5 +++-- packages/frontend/src/authStatusSearch.ts | 9 ++++++--- packages/frontend/tests/authStatusSearch.test.ts | 8 ++++---- 3 files changed, 13 insertions(+), 9 deletions(-) diff --git a/packages/frontend/src/App.tsx b/packages/frontend/src/App.tsx index efe39c2a1..8c1b348c4 100644 --- a/packages/frontend/src/App.tsx +++ b/packages/frontend/src/App.tsx @@ -115,10 +115,11 @@ export function App() { const overrides: SlotOverrides = useMemo(() => ({ ShellActionsActionSlot: LogoutButton }), []); - if (shouldShowAuthErrorScreen({ authError, session })) { + const authErrorReason = shouldShowAuthErrorScreen({ authError, session }); + if (authErrorReason != null) { return ( - + ); } diff --git a/packages/frontend/src/authStatusSearch.ts b/packages/frontend/src/authStatusSearch.ts index bedead589..ae7831535 100644 --- a/packages/frontend/src/authStatusSearch.ts +++ b/packages/frontend/src/authStatusSearch.ts @@ -9,15 +9,18 @@ export function parseAuthErrorReason(search: string): string | null { return reason; } -/** True only for a real login failure: `?error=` present and no valid session. */ +/** Returns the error reason when the auth error screen should show; otherwise null. */ export function shouldShowAuthErrorScreen({ authError, session, }: { authError: string | null; session: SessionState | 'checking'; -}): boolean { - return authError != null && session === 'unauthenticated'; +}): string | null { + if (authError == null || session !== 'unauthenticated') { + return null; + } + return authError; } /** Path + search + hash with the OIDC `error` query removed. */ diff --git a/packages/frontend/tests/authStatusSearch.test.ts b/packages/frontend/tests/authStatusSearch.test.ts index 85ff3ecee..4d3754935 100644 --- a/packages/frontend/tests/authStatusSearch.test.ts +++ b/packages/frontend/tests/authStatusSearch.test.ts @@ -10,10 +10,10 @@ describe('authStatusSearch', () => { }); it('shouldShowAuthErrorScreen only for unauthenticated error landings', () => { - assert.equal(shouldShowAuthErrorScreen({ authError: 'login_failed', session: 'unauthenticated' }), true); - assert.equal(shouldShowAuthErrorScreen({ authError: 'login_failed', session: 'checking' }), false); - assert.equal(shouldShowAuthErrorScreen({ authError: 'login_failed', session: 'authenticated' }), false); - assert.equal(shouldShowAuthErrorScreen({ authError: null, session: 'unauthenticated' }), false); + assert.equal(shouldShowAuthErrorScreen({ authError: 'login_failed', session: 'unauthenticated' }), 'login_failed'); + assert.equal(shouldShowAuthErrorScreen({ authError: 'login_failed', session: 'checking' }), null); + assert.equal(shouldShowAuthErrorScreen({ authError: 'login_failed', session: 'authenticated' }), null); + assert.equal(shouldShowAuthErrorScreen({ authError: null, session: 'unauthenticated' }), null); }); it('stripAuthErrorSearch drops error and keeps other query and hash', () => {