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.' }; } }