From 554bcfd2973d26d2a0fc18f158adc0034c903a84 Mon Sep 17 00:00:00 2001 From: Minxi Hou Date: Tue, 29 Sep 2026 05:01:11 +0000 Subject: [PATCH] fix(auth): never sign anyone in through OIDC without an allowlist The OIDC callback treated an empty oidcAllowedSubjects as "let every account of the identity provider in", and it minted the dashboard admin session for whoever that was. The settings route was meant to prevent the empty state, but it only ran when the request body itself set oidcEnabled to true. With OIDC already on, a later update that sent oidcAllowedSubjects: [] (a key that needs no password confirmation) passed, and so did any state that did not come through that route. Check the state the update would produce instead: reject it when OIDC would be enabled and the allowlist would hold no non-blank entry, whether the request changes the switch, the list or both. The callback now also counts an allowlist without a usable entry as not configured, before it contacts the provider, so an instance already in that state stops admitting everybody. Password login is unaffected. Signed-off-by: Minxi Hou --- .../fixes/oidc-empty-allowlist-fail-closed.md | 1 + docs/architecture/AUTHZ_GUIDE.md | 9 +- docs/openapi.yaml | 6 +- src/app/api/auth/oidc/callback/route.ts | 37 ++++---- src/app/api/settings/route.ts | 5 +- tests/unit/oidc-callback.test.ts | 38 +++++++- .../oidc-settings-allowlist-guard.test.ts | 87 +++++++++++++++++++ 7 files changed, 153 insertions(+), 30 deletions(-) create mode 100644 changelog.d/fixes/oidc-empty-allowlist-fail-closed.md create mode 100644 tests/unit/oidc-settings-allowlist-guard.test.ts diff --git a/changelog.d/fixes/oidc-empty-allowlist-fail-closed.md b/changelog.d/fixes/oidc-empty-allowlist-fail-closed.md new file mode 100644 index 000000000000..205983b776ad --- /dev/null +++ b/changelog.d/fixes/oidc-empty-allowlist-fail-closed.md @@ -0,0 +1 @@ +- **fix(auth):** the OIDC callback no longer signs anyone in when `oidcAllowedSubjects` is empty, and the settings route rejects any update that would leave OIDC enabled with an empty allowlist, not only the one that enables it diff --git a/docs/architecture/AUTHZ_GUIDE.md b/docs/architecture/AUTHZ_GUIDE.md index 0a4b39683655..26fa12c1fce9 100644 --- a/docs/architecture/AUTHZ_GUIDE.md +++ b/docs/architecture/AUTHZ_GUIDE.md @@ -64,10 +64,11 @@ supplemented: - `GET /api/auth/oidc/callback` validates `state`, exchanges the authorization code, and verifies the ID token's signature via the issuer's JWKS (`jose`'s `createRemoteJWKSet`, cached per JWKS URI) with `issuer`/`audience` - checks. An optional `oidcAllowedSubjects` allowlist matches the token's - `sub` claim or its `email` claim — the email claim is only honored when - `email_verified === true`, so an unverified email at the IdP can never pass - the gate. + checks. The `oidcAllowedSubjects` allowlist is required: it matches the + token's `sub` claim or its `email` claim — the email claim is only honored + when `email_verified === true`, so an unverified email at the IdP can never + pass the gate — and with no entries the callback refuses every login + (`not_configured`). - On success it mints the **exact same** 30-day `auth_token` JWT the password login issues (`src/app/api/auth/login/route.ts`), so the rest of the dashboard session pipeline (auto-refresh, cookie flags) is unchanged — diff --git a/docs/openapi.yaml b/docs/openapi.yaml index a581c1155ada..faf28721528b 100644 --- a/docs/openapi.yaml +++ b/docs/openapi.yaml @@ -5903,9 +5903,9 @@ paths: summary: Complete OIDC login for the dashboard admin gate description: | Validates the `state` cookie, exchanges the authorization `code` for tokens, - verifies the ID token against the issuer's JWKS (audience = client id), and — - if `oidcAllowedSubjects` is configured — checks the token's `sub`/`email` against - that allowlist. On success it mints the same 30-day `auth_token` dashboard-session + verifies the ID token against the issuer's JWKS (audience = client id), and + checks the token's `sub`/`email` against `oidcAllowedSubjects`, which must hold at + least one entry (an empty list ends in `not_configured`). On success it mints the same 30-day `auth_token` dashboard-session JWT used by password login and redirects to `/dashboard`. parameters: - name: code diff --git a/src/app/api/auth/oidc/callback/route.ts b/src/app/api/auth/oidc/callback/route.ts index 85a73dbdb849..e94ca4044661 100644 --- a/src/app/api/auth/oidc/callback/route.ts +++ b/src/app/api/auth/oidc/callback/route.ts @@ -85,7 +85,13 @@ export async function GET(request: Request) { ? settings.oidcRedirectPath : "/api/auth/oidc/callback"; - if (!enabled || !rawIssuer || !clientId || !clientSecret) { + // Without an allowlist every account at the identity provider would be let in as the + // dashboard admin, so an empty list counts as not configured. + const allowed = Array.isArray(settings.oidcAllowedSubjects) + ? settings.oidcAllowedSubjects.filter((v: unknown) => typeof v === "string" && v.trim() !== "") + : []; + + if (!enabled || !rawIssuer || !clientId || !clientSecret || allowed.length === 0) { return NextResponse.redirect(new URL("/login?oidc_error=not_configured", originEarly)); } @@ -190,23 +196,18 @@ export async function GET(request: Request) { audience: clientId, }); - // Optional subject / email whitelist - const allowed = Array.isArray(settings.oidcAllowedSubjects) ? settings.oidcAllowedSubjects : []; - if (allowed.length > 0) { - const sub = typeof payload.sub === "string" ? payload.sub : ""; - const emailVerified = (payload as Record).email_verified === true; - const email = - emailVerified && typeof (payload as Record).email === "string" - ? ((payload as Record).email as string).toLowerCase() - : ""; - const ok = allowed.some((v: unknown) => { - if (typeof v !== "string") return false; - if (v === sub) return true; - return email !== "" && v.toLowerCase() === email; - }); - if (!ok) { - return NextResponse.redirect(new URL("/login?oidc_error=subject_not_allowed", originEarly)); - } + const sub = typeof payload.sub === "string" ? payload.sub : ""; + const emailVerified = (payload as Record).email_verified === true; + const email = + emailVerified && typeof (payload as Record).email === "string" + ? ((payload as Record).email as string).toLowerCase() + : ""; + const ok = allowed.some((v: string) => { + if (v === sub) return true; + return email !== "" && v.toLowerCase() === email; + }); + if (!ok) { + return NextResponse.redirect(new URL("/login?oidc_error=subject_not_allowed", originEarly)); } } catch { return NextResponse.redirect(new URL("/login?oidc_error=id_token_invalid", originEarly)); diff --git a/src/app/api/settings/route.ts b/src/app/api/settings/route.ts index abb0c32e7188..1671162d4186 100644 --- a/src/app/api/settings/route.ts +++ b/src/app/api/settings/route.ts @@ -323,13 +323,14 @@ export async function PATCH(request: Request) { }) as typeof body.modelLockout; } - if (body.oidcEnabled === true) { + if (body.oidcEnabled !== undefined || body.oidcAllowedSubjects !== undefined) { const current = await getSettings(); + const enabled = body.oidcEnabled ?? current.oidcEnabled === true; const subjects = Array.isArray(body.oidcAllowedSubjects) ? (body.oidcAllowedSubjects as unknown[]) : ((current.oidcAllowedSubjects as unknown[] | undefined) ?? []); const hasAtLeastOne = subjects.some((s) => typeof s === "string" && s.trim().length > 0); - if (!hasAtLeastOne) { + if (enabled && !hasAtLeastOne) { emitSettingsFailureAudit(request, actor, "OIDC_ALLOWED_SUBJECTS_REQUIRED", attemptedKeys); return NextResponse.json( { diff --git a/tests/unit/oidc-callback.test.ts b/tests/unit/oidc-callback.test.ts index 783aec0419ec..57caba190935 100644 --- a/tests/unit/oidc-callback.test.ts +++ b/tests/unit/oidc-callback.test.ts @@ -75,7 +75,7 @@ async function setupFullOidcSettings() { oidcClientId: "client-oidc-test", oidcClientSecret: "secret-oidc-test", oidcRedirectPath: "/api/auth/oidc/callback", - oidcAllowedSubjects: [], + oidcAllowedSubjects: ["user-123"], }); } @@ -518,7 +518,7 @@ test("OIDC callback handles issuer with trailing slash in settings and token (#1 oidcClientId: "client-oidc-authentik", oidcClientSecret: "secret-oidc-authentik", oidcRedirectPath: "/api/auth/oidc/callback", - oidcAllowedSubjects: [], + oidcAllowedSubjects: ["authentik-user-1"], }); const { idToken, jwks } = await createSignedIdToken({ @@ -583,7 +583,7 @@ test("OIDC callback handles issuer mismatch on trailing slash between settings a oidcClientId: "client-oidc-authentik-mismatch", oidcClientSecret: "secret-oidc-authentik-mismatch", oidcRedirectPath: "/api/auth/oidc/callback", - oidcAllowedSubjects: [], + oidcAllowedSubjects: ["authentik-user-2"], }); // Token signed with trailing slash (common with Authentik discovery) @@ -634,3 +634,35 @@ test("OIDC callback handles issuer mismatch on trailing slash between settings a globalThis.fetch = originalFetch; } }); + +for (const [label, subjects] of [ + ["an empty allowlist", []], + ["an allowlist of blank entries", ["", " "]], +] as const) { + test(`OIDC callback refuses to sign anyone in with ${label} (not_configured)`, async () => { + await setupFullOidcSettings(); + await localDb.updateSettings({ oidcAllowedSubjects: [...subjects] }); + + const testState = "state-empty-allowlist"; + capturedCookies["oidc_state"] = { value: testState }; + + const originalFetch = globalThis.fetch; + let outboundCalls = 0; + globalThis.fetch = (async () => { + outboundCalls += 1; + throw new Error("the identity provider must not be contacted"); + }) as typeof fetch; + try { + const response = await callbackRoute.GET( + new Request(`http://localhost/api/auth/oidc/callback?code=foo&state=${testState}`) + ); + + assert.equal(response.status, 307); + assert.ok((response.headers.get("location") || "").includes("oidc_error=not_configured")); + assert.equal(outboundCalls, 0); + assert.equal(capturedCookies["auth_token"], undefined); + } finally { + globalThis.fetch = originalFetch; + } + }); +} diff --git a/tests/unit/oidc-settings-allowlist-guard.test.ts b/tests/unit/oidc-settings-allowlist-guard.test.ts new file mode 100644 index 000000000000..d2ac72eebf59 --- /dev/null +++ b/tests/unit/oidc-settings-allowlist-guard.test.ts @@ -0,0 +1,87 @@ +import test from "node:test"; +import assert from "node:assert/strict"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { makeManagementSessionRequest } from "../helpers/managementSession.ts"; + +const TEST_DATA_DIR = fs.mkdtempSync(path.join(os.tmpdir(), "omniroute-oidc-settings-guard-")); +process.env.DATA_DIR = TEST_DATA_DIR; + +const core = await import("../../src/lib/db/core.ts"); +const settingsDb = await import("../../src/lib/db/settings.ts"); +const settingsRoute = await import("../../src/app/api/settings/route.ts"); + +test.beforeEach(() => { + core.resetDbInstance(); + fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true, maxRetries: 5, retryDelay: 100 }); + fs.mkdirSync(TEST_DATA_DIR, { recursive: true }); +}); + +test.after(() => { + core.resetDbInstance(); + fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true, maxRetries: 5, retryDelay: 100 }); +}); + +async function patch(body: Record) { + const response = await settingsRoute.PATCH( + await makeManagementSessionRequest("http://localhost/api/settings", { + method: "PATCH", + body, + }) + ); + return { status: response.status, body: (await response.json()) as any }; +} + +async function seedEnabled(subjects: string[]) { + await settingsDb.updateSettings({ + oidcEnabled: true, + oidcIssuer: "https://idp.test", + oidcClientId: "client", + oidcClientSecret: "secret", + oidcAllowedSubjects: subjects, + }); +} + +test("emptying the allowlist while OIDC stays enabled is rejected", async () => { + await seedEnabled(["admin@example.com"]); + + const result = await patch({ oidcAllowedSubjects: [] }); + + assert.equal(result.status, 400); + assert.equal(result.body.error.code, "OIDC_ALLOWED_SUBJECTS_REQUIRED"); + assert.deepEqual((await settingsDb.getSettings()).oidcAllowedSubjects, ["admin@example.com"]); +}); + +test("an allowlist of blank entries is rejected while OIDC is enabled", async () => { + await seedEnabled(["admin@example.com"]); + + const result = await patch({ oidcAllowedSubjects: ["", " "] }); + + assert.equal(result.status, 400); + assert.equal(result.body.error.code, "OIDC_ALLOWED_SUBJECTS_REQUIRED"); +}); + +test("enabling OIDC without any allowed subject is rejected", async () => { + const result = await patch({ oidcEnabled: true }); + + assert.equal(result.status, 400); + assert.equal(result.body.error.code, "OIDC_ALLOWED_SUBJECTS_REQUIRED"); +}); + +test("replacing the allowlist with another non-empty one is accepted", async () => { + await seedEnabled(["admin@example.com"]); + + const result = await patch({ oidcAllowedSubjects: ["other@example.com"] }); + + assert.equal(result.status, 200); + assert.deepEqual((await settingsDb.getSettings()).oidcAllowedSubjects, ["other@example.com"]); +}); + +test("the allowlist can be emptied when OIDC is off", async () => { + await settingsDb.updateSettings({ oidcEnabled: false, oidcAllowedSubjects: ["a@example.com"] }); + + const result = await patch({ oidcAllowedSubjects: [] }); + + assert.equal(result.status, 200); +});