fix(oidc): security + robustness hardening from codex review rounds 1-2

Round 1:
- bind SSO identities to (external_issuer, external_subject): OIDC only
  guarantees sub uniqueness within an issuer, so a sub-only lookup let a
  newly configured IdP's user inherit an old IdP's admin account on
  subject collision; migration 162 gains external_issuer + composite
  unique index (unmerged migration, edited in place)
- fetch UserInfo (with sub cross-check) when the ID token carries no
  email — spec-compliant providers may serve email/profile claims only
  there; ID-token claims win on merge
- allowlist /admin/sso/login + /callback in maintenance mode, or
  SSO-only (JIT) admins are locked out exactly when they need in
- strip reserved keys (oidc_client_secret, setup_token) from BOTH
  generic settings reads (GET / and GET /:type)

Round 2:
- redirect_uri prefers API_URL (the API's public origin — where the
  state cookie lives); final redirects absolute to the frontend base;
  login button builds its URL via buildResourceUrl — split-origin
  deployments (absolute VITE_API_URL) work end to end
- PUT /sso validates the MERGED resulting state (partial update cannot
  blank issuer/client while enabled=true survives; enabling requires a
  derivable redirect URI)
- openid scope forced into oidc_scopes on save
- discovery-cache key includes a secret fingerprint (multi-worker
  secret rotation)
- email→admin linking claims the row atomically (conditional update on
  external_subject IS NULL) — concurrent first-time callbacks with the
  same verified email but different subjects can't both authenticate

Tests: mock IdP gains userinfo endpoint + email-via-userinfo-only mode;
new cases pin the userinfo merge and issuer-collision non-inheritance;
redirect assertions updated for absolute URLs. 13/13.
This commit is contained in:
Paul Nothaft
2026-07-16 09:41:48 +02:00
parent ac1838fbd7
commit 7f7d38a57f
8 changed files with 242 additions and 44 deletions
@@ -33,6 +33,8 @@ class MockOidcProvider {
this.nextUser = { sub: 'user-1', email: '[email protected]', email_verified: true };
// Test hooks:
this.tamperNonce = false; // sign the ID token with a WRONG nonce
this.emailViaUserinfoOnly = false; // omit email from the ID token; serve it on /userinfo
this.accessTokens = new Map(); // access_token -> user (for /userinfo)
this.server = null;
this.issuer = null;
}
@@ -81,6 +83,7 @@ class MockOidcProvider {
issuer: this.issuer,
authorization_endpoint: `${this.issuer}/authorize`,
token_endpoint: `${this.issuer}/token`,
userinfo_endpoint: `${this.issuer}/userinfo`,
jwks_uri: `${this.issuer}/jwks`,
response_types_supported: ['code'],
subject_types_supported: ['public'],
@@ -128,13 +131,18 @@ class MockOidcProvider {
}
const { sub, ...extraClaims } = stored.user;
// Spec-compliant providers may keep profile/email claims OFF the ID
// token and serve them from /userinfo only — this hook simulates that.
const idTokenClaims = this.emailViaUserinfoOnly ? {} : extraClaims;
const idToken = this.signIdToken({
sub,
nonce: this.tamperNonce ? 'tampered-nonce' : stored.nonce,
extraClaims,
extraClaims: idTokenClaims,
});
const accessToken = crypto.randomBytes(16).toString('base64url');
this.accessTokens.set(accessToken, stored.user);
return json(200, {
access_token: crypto.randomBytes(16).toString('base64url'),
access_token: accessToken,
token_type: 'Bearer',
expires_in: 300,
id_token: idToken,
@@ -143,6 +151,13 @@ class MockOidcProvider {
return undefined;
}
if (url.pathname === '/userinfo') {
const auth = req.headers.authorization || '';
const user = this.accessTokens.get(auth.replace(/^Bearer\s+/i, ''));
if (!user) return json(401, { error: 'invalid_token' });
return json(200, { ...user });
}
return json(404, { error: 'not_found' });
}
}
+65 -8
View File
@@ -99,7 +99,7 @@ describe('OIDC SSO (#798)', () => {
idp.setNextUser({ sub: 'sub-jit-1', email: '[email protected]', email_verified: true });
const res = await ssoRoundTrip();
expect(res.headers.location).toBe('/admin/dashboard');
expect(res.headers.location).toBe('http://localhost:5199/admin/dashboard');
const adminCookie = (res.headers['set-cookie'] || []).find((c) => c.startsWith('admin_token='));
expect(adminCookie).toBeTruthy();
@@ -122,7 +122,7 @@ describe('OIDC SSO (#798)', () => {
it('matches repeat logins by sub even when the email changed at the IdP', async () => {
idp.setNextUser({ sub: 'sub-jit-1', email: '[email protected]', email_verified: true });
const res = await ssoRoundTrip();
expect(res.headers.location).toBe('/admin/dashboard');
expect(res.headers.location).toBe('http://localhost:5199/admin/dashboard');
// No second row — resolved via external_subject.
expect(await db('admin_users').where({ email: '[email protected]' }).first()).toBeFalsy();
@@ -145,7 +145,7 @@ describe('OIDC SSO (#798)', () => {
idp.setNextUser({ sub: 'sub-local-1', email: '[email protected]', email_verified: true });
const res = await ssoRoundTrip();
expect(res.headers.location).toBe('/admin/dashboard');
expect(res.headers.location).toBe('http://localhost:5199/admin/dashboard');
const row = await db('admin_users').where({ id: localId }).first();
expect(row.external_subject).toBe('sub-local-1');
@@ -180,18 +180,18 @@ describe('OIDC SSO (#798)', () => {
await db('admin_users').where({ id: agentCookies.jitAdminId }).update({ is_active: 0 });
idp.setNextUser({ sub: 'sub-jit-1', email: '[email protected]', email_verified: true });
const res = await ssoRoundTrip();
expect(res.headers.location).toBe('/admin/login?sso_error=inactive');
expect(res.headers.location).toBe('http://localhost:5199/admin/login?sso_error=inactive');
await db('admin_users').where({ id: agentCookies.jitAdminId }).update({ is_active: 1 });
});
it('rejects a callback without the state cookie', async () => {
const res = await ssoRoundTrip({ mutateState: 'drop' });
expect(res.headers.location).toBe('/admin/login?sso_error=state');
expect(res.headers.location).toBe('http://localhost:5199/admin/login?sso_error=state');
});
it('rejects a forged state cookie (wrong signing key)', async () => {
const res = await ssoRoundTrip({ mutateState: 'forge' });
expect(res.headers.location).toBe('/admin/login?sso_error=state');
expect(res.headers.location).toBe('http://localhost:5199/admin/login?sso_error=state');
});
it('rejects an ID token whose nonce does not match', async () => {
@@ -199,7 +199,7 @@ describe('OIDC SSO (#798)', () => {
idp.setNextUser({ sub: 'sub-nonce', email: '[email protected]', email_verified: true });
const res = await ssoRoundTrip();
idp.tamperNonce = false;
expect(res.headers.location).toBe('/admin/login?sso_error=idp');
expect(res.headers.location).toBe('http://localhost:5199/admin/login?sso_error=idp');
expect(await db('admin_users').where({ email: '[email protected]' }).first()).toBeFalsy();
});
@@ -207,7 +207,7 @@ describe('OIDC SSO (#798)', () => {
await oidcService.saveOidcSettings({ oidc_autoprovision: false });
idp.setNextUser({ sub: 'sub-new-user', email: '[email protected]', email_verified: true });
const res = await ssoRoundTrip();
expect(res.headers.location).toBe('/admin/login?sso_error=not_provisioned');
expect(res.headers.location).toBe('http://localhost:5199/admin/login?sso_error=not_provisioned');
expect(await db('admin_users').where({ email: '[email protected]' }).first()).toBeFalsy();
await oidcService.saveOidcSettings({ oidc_autoprovision: true });
});
@@ -227,4 +227,61 @@ describe('OIDC SSO (#798)', () => {
await request(app).get('/api/auth/admin/sso/login').expect(404);
await oidcService.saveOidcSettings({ oidc_enabled: true });
});
it('merges email from the UserInfo endpoint when the ID token omits it', async () => {
idp.emailViaUserinfoOnly = true;
idp.setNextUser({ sub: 'sub-userinfo', email: '[email protected]', email_verified: true });
const res = await ssoRoundTrip();
idp.emailViaUserinfoOnly = false;
expect(res.headers.location).toBe('http://localhost:5199/admin/dashboard');
const row = await db('admin_users').where({ email: '[email protected]' }).first();
expect(row).toBeTruthy();
expect(row.external_subject).toBe('sub-userinfo');
});
it('binds identities per ISSUER — a sub collision on a new IdP must not inherit the old account', async () => {
// The JIT admin from the first test is bound to (issuer A, 'sub-jit-1').
const boundAdmin = await db('admin_users').where({ id: agentCookies.jitAdminId }).first();
expect(boundAdmin.external_issuer).toBe(idp.issuer);
// Same sub, DIFFERENT issuer: a second IdP the instance switches to.
const idp2 = new MockOidcProvider();
await idp2.start();
try {
await oidcService.saveOidcSettings({
oidc_issuer_url: idp2.issuer,
oidc_client_id: idp2.clientId,
oidc_client_secret: idp2.clientSecret,
});
idp2.setNextUser({ sub: 'sub-jit-1', email: '[email protected]', email_verified: true });
const loginRes = await request(app).get('/api/auth/admin/sso/login').expect(302);
const stateCookie = (loginRes.headers['set-cookie'] || [])
.find((c) => c.startsWith('oidc_state=')).split(';')[0];
const idpRes = await fetch(loginRes.headers.location, { redirect: 'manual' });
const back = new URL(idpRes.headers.get('location'));
const res = await request(app)
.get(`${back.pathname}?${back.searchParams.toString()}`)
.set('Cookie', stateCookie)
.expect(302);
expect(res.headers.location).toBe('http://localhost:5199/admin/dashboard');
// A NEW row bound to issuer B — the issuer-A admin is untouched and
// its role was not inherited.
const collider = await db('admin_users').where({ email: '[email protected]' }).first();
expect(collider).toBeTruthy();
expect(collider.id).not.toBe(agentCookies.jitAdminId);
expect(collider.external_issuer).toBe(idp2.issuer);
const original = await db('admin_users').where({ id: agentCookies.jitAdminId }).first();
expect(original.external_issuer).toBe(idp.issuer);
} finally {
await idp2.stop();
await oidcService.saveOidcSettings({
oidc_issuer_url: idp.issuer,
oidc_client_id: idp.clientId,
oidc_client_secret: idp.clientSecret,
});
}
});
});