diff --git a/docs/architecture/AUTHZ_GUIDE.md b/docs/architecture/AUTHZ_GUIDE.md index 67f8040a9f7b..22e68241ffe4 100644 --- a/docs/architecture/AUTHZ_GUIDE.md +++ b/docs/architecture/AUTHZ_GUIDE.md @@ -75,6 +75,21 @@ supplemented: login issues (`src/app/api/auth/login/route.ts`), so the rest of the dashboard session pipeline (auto-refresh, cookie flags) is unchanged — OIDC only replaces how the cookie gets minted, not what it grants. +- Every `oidc*` setting (`oidcEnabled`, `oidcDisablePasswordLogin`, + `oidcIssuer`, `oidcClientId`, `oidcClientSecret`, `oidcAllowedSubjects`, + `oidcRedirectPath`, `oidcScopes`) is in `SECURITY_IMPACTING_SETTINGS_KEYS` + (`src/shared/constants/securityImpactingSettings.ts`): `PATCH /api/settings` + answers `400 PASSWORD_REQUIRED` unless the body carries the management + password as `currentPassword` (`401 PASSWORD_MISMATCH` when it is wrong). + `oidcDisablePasswordLogin` refuses password _login_ (`POST /api/auth/login`) + but keeps the stored password hash, and re-auth still checks + `currentPassword` against it. +- `POST /api/settings/import-json` skips that re-auth, so it drops every key in + `IMPORT_STRIPPED_SETTINGS_KEYS` (same file) before writing: the password and + `currentPassword`, every `SECURITY_IMPACTING_SETTINGS_KEYS` entry, and the + dashboard session state (`sessionsValidAfter`, `revokedDashboardSessions`). + Importing an older backup therefore cannot revive a session that a password + change or a sign-out already ended. ## Route Classes diff --git a/src/app/api/settings/import-json/route.ts b/src/app/api/settings/import-json/route.ts index 62336483e587..b43803d0c1f6 100644 --- a/src/app/api/settings/import-json/route.ts +++ b/src/app/api/settings/import-json/route.ts @@ -8,12 +8,15 @@ import { runJsonMigration, type LegacyJsonData } from "@/lib/db/jsonMigration"; import { getSettings } from "@/lib/db/settings"; import { setSystemPromptConfig } from "@omniroute/open-sse/services/systemPrompt.ts"; import { sanitizeErrorMessage } from "@omniroute/open-sse/utils/error"; -import { SECURITY_IMPACTING_SETTINGS_KEYS } from "@/shared/constants/securityImpactingSettings"; +import { IMPORT_STRIPPED_SETTINGS_KEYS } from "@/shared/constants/securityImpactingSettings"; -/** Drops the password and every key PATCH only accepts after password re-auth. */ +/** + * Drops the password (and any `currentPassword`), every key PATCH only accepts after password + * re-auth, and the dashboard session state (`sessionsValidAfter`, `revokedDashboardSessions`). + */ function stripSecurityImpactingSettings(settings: object): Record { const safeSettings: Record = { ...settings }; - for (const key of ["password", ...SECURITY_IMPACTING_SETTINGS_KEYS]) { + for (const key of IMPORT_STRIPPED_SETTINGS_KEYS) { delete safeSettings[key]; } return safeSettings; @@ -26,10 +29,12 @@ function stripSecurityImpactingSettings(settings: object): Record { core.resetDbInstance(); @@ -74,8 +84,12 @@ test("enabling OIDC without any allowed subject is rejected", async () => { test("replacing the allowlist with another non-empty one is accepted", async () => { await seedEnabled(["admin@example.com"]); + await seedPassword(); - const result = await patch({ oidcAllowedSubjects: ["other@example.com"] }); + const result = await patch({ + oidcAllowedSubjects: ["other@example.com"], + currentPassword: PASSWORD, + }); assert.equal(result.status, 200); assert.deepEqual((await settingsDb.getSettings()).oidcAllowedSubjects, ["other@example.com"]); @@ -83,8 +97,9 @@ test("replacing the allowlist with another non-empty one is accepted", async () test("the allowlist can be emptied when OIDC is off", async () => { await settingsDb.updateSettings({ oidcEnabled: false, oidcAllowedSubjects: ["a@example.com"] }); + await seedPassword(); - const result = await patch({ oidcAllowedSubjects: [] }); + const result = await patch({ oidcAllowedSubjects: [], currentPassword: PASSWORD }); assert.equal(result.status, 200); }); diff --git a/tests/unit/oidc-settings-reauth.test.ts b/tests/unit/oidc-settings-reauth.test.ts new file mode 100644 index 000000000000..f6bcb0865135 --- /dev/null +++ b/tests/unit/oidc-settings-reauth.test.ts @@ -0,0 +1,168 @@ +/** + * The OIDC identity configuration decides WHO can log into the dashboard: the issuer whose + * ID tokens are trusted, the client id they must be issued to (audience), the callback path + * the authorization code is sent to and the subject/email allowlist. Changing any of them is + * an authentication change, so `PATCH /api/settings` must demand the same password re-auth + * (`currentPassword`) it already demands for `oidcEnabled` / `oidcClientSecret`, and + * `POST /api/settings/import-json` (no re-auth) must not write them. Otherwise a stolen + * session or management credential can point the login at an attacker-controlled IdP, or + * allowlist the attacker's own subject, and keep dashboard access after the password rotates. + */ +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-reauth-")); +process.env.DATA_DIR = TEST_DATA_DIR; +process.env.OMNIROUTE_DISABLE_REDIS_AUTH_CACHE = "1"; + +const ORIGINAL_INITIAL_PASSWORD = process.env.INITIAL_PASSWORD; + +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"); +const importJsonRoute = await import("../../src/app/api/settings/import-json/route.ts"); +const managementPassword = await import("../../src/lib/auth/managementPassword.ts"); +const securitySettings = await import("../../src/shared/constants/securityImpactingSettings.ts"); +const { updateSettingsSchema } = await import("../../src/shared/validation/settingsSchemas.ts"); + +const PASSWORD = "operator-secret"; + +/** OIDC identity keys and an attacker-chosen value for each. */ +const OIDC_IDENTITY_CHANGES: Record = { + oidcIssuer: "https://attacker-idp.example", + oidcClientId: "attacker-client", + oidcAllowedSubjects: ["attacker@example.com"], + oidcRedirectPath: "@attacker.example/callback", + oidcScopes: ["openid", "offline_access"], +}; + +const SEEDED_OIDC = { + oidcEnabled: true, + oidcIssuer: "https://idp.example", + oidcClientId: "operator-client", + oidcClientSecret: "operator-client-secret", + oidcAllowedSubjects: ["admin@example.com"], + oidcRedirectPath: "/api/auth/oidc/callback", + oidcScopes: ["openid", "profile", "email"], +}; + +async function resetStorage() { + core.resetDbInstance(); + fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true, maxRetries: 5, retryDelay: 100 }); + fs.mkdirSync(TEST_DATA_DIR, { recursive: true }); + delete process.env.INITIAL_PASSWORD; +} + +async function seedConfiguredInstance() { + await settingsDb.updateSettings({ + requireLogin: true, + password: await managementPassword.hashManagementPassword(PASSWORD), + ...SEEDED_OIDC, + }); +} + +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 { error?: { code?: string; keys?: string[] } }, + }; +} + +test.beforeEach(async () => { + await resetStorage(); +}); + +test.after(() => { + core.resetDbInstance(); + fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true, maxRetries: 5, retryDelay: 100 }); + if (ORIGINAL_INITIAL_PASSWORD === undefined) { + delete process.env.INITIAL_PASSWORD; + } else { + process.env.INITIAL_PASSWORD = ORIGINAL_INITIAL_PASSWORD; + } +}); + +for (const [key, value] of Object.entries(OIDC_IDENTITY_CHANGES)) { + test(`PATCH ${key} without currentPassword is rejected and the stored value is kept`, async () => { + await seedConfiguredInstance(); + + const result = await patch({ [key]: value }); + + assert.equal(result.status, 400); + assert.equal(result.body.error?.code, "PASSWORD_REQUIRED"); + assert.deepEqual(result.body.error?.keys, [key]); + const after = (await settingsDb.getSettings()) as Record; + assert.deepEqual(after[key], (SEEDED_OIDC as Record)[key]); + }); + + test(`PATCH ${key} with a wrong currentPassword is rejected`, async () => { + await seedConfiguredInstance(); + + const result = await patch({ [key]: value, currentPassword: "not-the-password" }); + + assert.equal(result.status, 401); + assert.equal(result.body.error?.code, "PASSWORD_MISMATCH"); + const after = (await settingsDb.getSettings()) as Record; + assert.deepEqual(after[key], (SEEDED_OIDC as Record)[key]); + }); + + test(`PATCH ${key} with the correct currentPassword succeeds`, async () => { + await seedConfiguredInstance(); + + const result = await patch({ [key]: value, currentPassword: PASSWORD }); + + assert.equal(result.status, 200); + const after = (await settingsDb.getSettings()) as Record; + assert.deepEqual(after[key], value); + }); +} + +test("PATCH of a non-auth setting still needs no currentPassword", async () => { + await seedConfiguredInstance(); + + const result = await patch({ theme: "dark" }); + + assert.equal(result.status, 200); +}); + +test("every oidc* key PATCH /api/settings accepts is guarded by the re-auth gate", () => { + const guarded = new Set(securitySettings.SECURITY_IMPACTING_SETTINGS_KEYS); + const oidcKeys = Object.keys(updateSettingsSchema.shape).filter((k) => k.startsWith("oidc")); + // Fails loudly if the schema renames the OIDC keys and this filter silently matches nothing. + assert.ok(oidcKeys.length >= 8, `expected the OIDC keys in the schema, got ${oidcKeys}`); + for (const key of oidcKeys) { + assert.ok(guarded.has(key), key); + } +}); + +test("import-json does not write the OIDC identity keys", async () => { + await settingsDb.updateSettings({ requireLogin: false, ...SEEDED_OIDC }); + const before = (await settingsDb.getSettings()) as Record; + + const response = await importJsonRoute.POST( + new Request("http://localhost:20128/api/settings/import-json", { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ + settings: { ...OIDC_IDENTITY_CHANGES, oidcReauthBenignKey: "kept" }, + }), + }) + ); + assert.equal(response.status, 200, await response.clone().text()); + + const after = (await settingsDb.getSettings()) as Record; + assert.equal(after.oidcReauthBenignKey, "kept"); + for (const key of Object.keys(OIDC_IDENTITY_CHANGES)) { + assert.deepEqual(after[key], before[key], key); + } +}); diff --git a/tests/unit/settings-import-json-session-state.test.ts b/tests/unit/settings-import-json-session-state.test.ts new file mode 100644 index 000000000000..2e47e5c2c1e6 --- /dev/null +++ b/tests/unit/settings-import-json-session-state.test.ts @@ -0,0 +1,111 @@ +/** + * `POST /api/settings/import-json` must not write the dashboard session state a password change + * or a sign-out leaves behind (`sessionsValidAfter`, `revokedDashboardSessions`), nor a + * `currentPassword`. Those keys are not settings an operator configures: importing an older + * backup that carries them (or that carries an empty revocation list / a zero cutoff) would + * revive dashboard sessions the password change or the logout already ended. + */ +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"; + +const TEST_DATA_DIR = fs.mkdtempSync(path.join(os.tmpdir(), "omniroute-import-session-state-")); +process.env.DATA_DIR = TEST_DATA_DIR; +process.env.OMNIROUTE_DISABLE_REDIS_AUTH_CACHE = "1"; +process.env.JWT_SECRET = "import-session-state-jwt-secret"; + +const core = await import("../../src/lib/db/core.ts"); +const settingsDb = await import("../../src/lib/db/settings.ts"); +const { invalidateDbCache } = await import("../../src/lib/db/readCache.ts"); +const importJsonRoute = await import("../../src/app/api/settings/import-json/route.ts"); +const securitySettings = await import("../../src/shared/constants/securityImpactingSettings.ts"); +const sessionToken = await import("../../src/shared/utils/dashboardSessionToken.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 importSettings(settings: Record) { + const response = await importJsonRoute.POST( + new Request("http://localhost:20128/api/settings/import-json", { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ settings }), + }) + ); + assert.equal(response.status, 200, await response.clone().text()); +} + +test("the session-state keys stripped by import-json are the ones the session verifier reads", () => { + assert.deepEqual( + [...securitySettings.DASHBOARD_SESSION_STATE_SETTINGS_KEYS].sort(), + [sessionToken.REVOKED_SESSIONS_SETTING, sessionToken.SESSIONS_VALID_AFTER_SETTING].sort() + ); +}); + +test("import-json drops password, currentPassword, session state and every re-auth-gated key", () => { + const stripped = new Set(securitySettings.IMPORT_STRIPPED_SETTINGS_KEYS); + for (const key of [ + "password", + "currentPassword", + ...securitySettings.SECURITY_IMPACTING_SETTINGS_KEYS, + ...securitySettings.DASHBOARD_SESSION_STATE_SETTINGS_KEYS, + ]) { + assert.ok(stripped.has(key), key); + } +}); + +test("import-json keeps the stored session cutoff and revocation list", async () => { + await settingsDb.updateSettings({ + requireLogin: false, + sessionsValidAfter: 1_900_000_000, + revokedDashboardSessions: [{ jti: "signed-out-session", exp: 4_000_000_000 }], + }); + + await importSettings({ + sessionsValidAfter: 0, + revokedDashboardSessions: [], + currentPassword: "imported-current-password", + showQuickStartOnHome: false, + }); + + const after = (await settingsDb.getSettings()) as Record; + assert.equal(after.sessionsValidAfter, 1_900_000_000); + assert.deepEqual(after.revokedDashboardSessions, [ + { jti: "signed-out-session", exp: 4_000_000_000 }, + ]); + assert.equal(after.currentPassword, undefined); + // Non-security settings still import. + assert.equal(after.showQuickStartOnHome, false); +}); + +test("a signed-out dashboard session stays dead after importing an older backup", async () => { + const secret = sessionToken.getDashboardJwtSecret(); + assert.ok(secret); + const token = await sessionToken.mintDashboardSessionToken(secret); + const payload = await sessionToken.verifyDashboardSessionToken(token, secret); + assert.ok(payload?.jti, "freshly minted session verifies"); + + // Sign-out: the session id lands on the revocation list. + await settingsDb.updateSettings({ + requireLogin: false, + revokedDashboardSessions: [{ jti: payload.jti, exp: payload.exp }], + }); + invalidateDbCache(); + assert.equal(await sessionToken.verifyDashboardSessionToken(token, secret), null); + + // A backup taken before the sign-out carries an empty revocation list. + await importSettings({ revokedDashboardSessions: [], sessionsValidAfter: 0 }); + invalidateDbCache(); + + assert.equal(await sessionToken.verifyDashboardSessionToken(token, secret), null); +});