From e084ad424c1f8ee85b61a0dba2aa8acc0b94e9a0 Mon Sep 17 00:00:00 2001 From: Anso Date: Fri, 7 Aug 2026 23:04:27 -0400 Subject: [PATCH] fix(sso): forward RFC 9207 iss parameter in OIDC callback (#1785) * fix(sso): forward RFC 9207 iss parameter in OIDC callback The Custom OIDC callback only forwarded code and state from the query string to the token exchange, silently dropping the iss parameter that issuer-identification-aware providers (Keycloak 22+, and others) add to the redirect. openid-client rejects the exchange as an invalid response once discovery advertises support for that parameter, so login failed for any such provider. * fix(sso): forward RFC 9207 iss parameter in OIDC callback The Custom OIDC callback only forwarded code and state from the query string to the token exchange, silently dropping the iss parameter that issuer-identification-aware providers add to the redirect. openid-client rejects the exchange as an invalid response once discovery advertises support for that parameter (confirmed on Keycloak 26), so login failed for any such provider. Also logs the underlying openid-client error cause on callback failure instead of only the generic message it collapses specific validation errors into, since that cause carries the actual diagnosis. * refactor(sso): dedupe iss-forwarding test setup Extracts the shared callback-with-stubbed-service setup used by both new regression tests into one helper. --- backend/src/__tests__/sso.test.ts | 82 +++++++++++++++++++++++++++++- backend/src/routes/sso.ts | 3 +- backend/src/services/SSOService.ts | 25 +++++++-- 3 files changed, 103 insertions(+), 7 deletions(-) diff --git a/backend/src/__tests__/sso.test.ts b/backend/src/__tests__/sso.test.ts index e4fd35f2..8ddbedc4 100644 --- a/backend/src/__tests__/sso.test.ts +++ b/backend/src/__tests__/sso.test.ts @@ -1,4 +1,4 @@ -import { describe, it, expect, beforeAll, afterAll, vi } from 'vitest'; +import { describe, it, expect, beforeAll, afterAll, vi, type MockInstance } from 'vitest'; import { setupTestDb, cleanupTestDb, TEST_JWT_SECRET } from './helpers/setupTestDb'; import supertest from 'supertest'; import jwt from 'jsonwebtoken'; @@ -110,6 +110,86 @@ describe('SSO OIDC Callback', () => { expect(res.status).toBe(302); expect(res.headers.location).toContain('User'); }); + + // Drives a callback request with a valid state cookie, returning the stubbed + // handleOIDCCallback spy so the caller can assert on the params it received. + async function callbackWithStubbedService(provider: string, query: Record): Promise { + const { SSOService } = await import('../services/SSOService'); + const { CryptoService } = await import('../services/CryptoService'); + + const stateCookie = CryptoService.getInstance().encrypt(JSON.stringify({ + state: 'test-state', + codeVerifier: 'test-verifier', + provider, + })); + + const spy = vi + .spyOn(SSOService.getInstance(), 'handleOIDCCallback') + .mockResolvedValue({ success: false, error: 'stubbed for iss-forwarding assertion' }); + + await supertest(app) + .get(`/api/auth/sso/oidc/${provider}/callback`) + .query(query) + .set('Cookie', `sencho_sso_state=${stateCookie}`); + + return spy; + } + + it('forwards the RFC 9207 iss query parameter to SSOService.handleOIDCCallback', async () => { + const spy = await callbackWithStubbedService('oidc_custom', { + code: 'test-code', + state: 'test-state', + iss: 'https://idp.example.com/realms/master', + }); + + expect(spy).toHaveBeenCalledWith( + 'oidc_custom', + expect.any(String), + expect.objectContaining({ code: 'test-code', state: 'test-state', iss: 'https://idp.example.com/realms/master' }), + 'test-state', + 'test-verifier', + ); + + spy.mockRestore(); + }); + + it('omits iss from the forwarded params when the provider does not send one', async () => { + const spy = await callbackWithStubbedService('oidc_google', { code: 'test-code', state: 'test-state' }); + + expect(spy).toHaveBeenCalledWith( + 'oidc_google', + expect.any(String), + expect.objectContaining({ code: 'test-code', state: 'test-state', iss: undefined }), + 'test-state', + 'test-verifier', + ); + + spy.mockRestore(); + }); +}); + +describe('SSOService.buildTokenExchangeUrl', () => { + it('sets the iss query parameter when provided', async () => { + const { SSOService } = await import('../services/SSOService'); + const url = SSOService.getInstance().buildTokenExchangeUrl( + 'http://sencho.example.com/api/auth/sso/oidc/oidc_custom/callback', + { code: 'test-code', state: 'test-state', iss: 'https://idp.example.com/realms/master' }, + ); + expect(url.searchParams.get('code')).toBe('test-code'); + expect(url.searchParams.get('state')).toBe('test-state'); + expect(url.searchParams.get('iss')).toBe('https://idp.example.com/realms/master'); + }); + + it('omits the iss query parameter when not provided', async () => { + const { SSOService } = await import('../services/SSOService'); + const url = SSOService.getInstance().buildTokenExchangeUrl( + 'http://sencho.example.com/api/auth/sso/oidc/oidc_google/callback', + { code: 'test-code', state: 'test-state' }, + ); + expect(url.searchParams.get('code')).toBe('test-code'); + expect(url.searchParams.get('state')).toBe('test-state'); + expect(url.searchParams.has('iss')).toBe(false); + }); }); describe('SSO User Provisioning', () => { diff --git a/backend/src/routes/sso.ts b/backend/src/routes/sso.ts index 274216e9..218ab7c6 100644 --- a/backend/src/routes/sso.ts +++ b/backend/src/routes/sso.ts @@ -126,6 +126,7 @@ ssoRouter.get('/oidc/:provider/callback', ssoRateLimiter, async (req: Request, r const provider = String(req.params.provider); const code = String(req.query.code || ''); const state = String(req.query.state || ''); + const iss = req.query.iss ? String(req.query.iss) : undefined; const oidcError = req.query.error ? String(req.query.error) : ''; const error_description = req.query.error_description ? String(req.query.error_description) : ''; @@ -168,7 +169,7 @@ ssoRouter.get('/oidc/:provider/callback', ssoRateLimiter, async (req: Request, r const result = await SSOService.getInstance().handleOIDCCallback( provider, callbackUrl, - { code, state }, + { code, state, iss }, statePayload.state, statePayload.codeVerifier, ); diff --git a/backend/src/services/SSOService.ts b/backend/src/services/SSOService.ts index ac53ddc9..34e56b93 100644 --- a/backend/src/services/SSOService.ts +++ b/backend/src/services/SSOService.ts @@ -387,10 +387,23 @@ export class SSOService { return { url: url.href, state, codeVerifier }; } + /** Builds the URL passed to authorizationCodeGrant() for response validation. + * Providers that support RFC 9207 issuer identification (confirmed on + * Keycloak 26; likely others) include `iss` in the callback. openid-client + * requires it once the provider's discovery metadata advertises support, so + * it must be forwarded here or the callback fails with "invalid response". */ + public buildTokenExchangeUrl(callbackUrl: string, params: { code: string; state: string; iss?: string }): URL { + const url = new URL(callbackUrl); + url.searchParams.set('code', params.code); + url.searchParams.set('state', params.state); + if (params.iss) url.searchParams.set('iss', params.iss); + return url; + } + public async handleOIDCCallback( provider: string, callbackUrl: string, - params: { code: string; state: string }, + params: { code: string; state: string; iss?: string }, expectedState: string, codeVerifier: string ): Promise { @@ -402,9 +415,7 @@ export class SSOService { try { const oidcConfig = await this.getOIDCConfig(provider, config); - const currentUrl = new URL(callbackUrl); - currentUrl.searchParams.set('code', params.code); - currentUrl.searchParams.set('state', params.state); + const currentUrl = this.buildTokenExchangeUrl(callbackUrl, params); const tokens = await authorizationCodeGrant(oidcConfig, currentUrl, { pkceCodeVerifier: codeVerifier, @@ -473,7 +484,11 @@ export class SSOService { }; } catch (err) { const message = err instanceof Error ? err.message : 'OIDC authentication failed'; - console.error('[SSO] OIDC callback error:', message); + // openid-client collapses specific validation failures (e.g. a missing or + // mismatched "iss" parameter) into this generic message; the cause carries + // the actual reason and is essential for diagnosing callback failures. + const cause = err instanceof Error && err.cause instanceof Error ? ` (${err.cause.message})` : ''; + console.error('[SSO] OIDC callback error:', message + cause); return { success: false, error: 'Authentication failed. Please try again.' }; } }