From 7aa647c1bdbb0b8f5fa362ce83eea59d5eb85082 Mon Sep 17 00:00:00 2001 From: KeplerAeroIT Date: Fri, 31 Jul 2026 15:13:26 +0530 Subject: [PATCH] fix(auth): address OIDC review findings (cookie validation, security, races) - Fix the actual production bug: Elysia's cookie parser auto-JSON.parses any cookie value shaped like {...}/[...], so the JSON-encoded oidcFlow cookie was silently turned into an object and rejected by its own t.String() schema before the handler ever ran. Switch to a dot-delimited string (PKCE verifier/state/nonce are base64url, so "." is a safe delimiter) and validate its shape explicitly instead of a bare JSON.parse. - Store the OIDC issuer alongside sub and match on the (issuer, sub) pair, since sub is only guaranteed unique within a single issuer. - Only auto-link an OIDC login to an existing local account when the provider has verified the email; an unverified/attacker-set email claim could otherwise hijack an existing account. - Add a unique index on (oidc_issuer, oidc_sub) and use INSERT ... ON CONFLICT DO NOTHING so two concurrent first-logins for the same new identity can't provision duplicate user rows. - Move OIDC discovery into a shared helper exposing isOidcReady(), and use it (rather than just checking that the env vars are set) to decide whether the login page shows the SSO button - so a failed discovery at startup doesn't advertise a provider that can never complete a login. - Fix compose.yaml's TZ typo (Asia/Koltaka -> Asia/Kolkata), which otherwise throws at runtime and breaks the history page's date formatting. - Gate the Register link behind OIDC_ONLY everywhere Header is rendered, so OIDC-only deployments don't show a dead local-registration link. --- compose.yaml | 2 +- src/db/db.ts | 14 +++++- src/db/types.ts | 1 + src/helpers/oidcClient.ts | 23 ++++++++++ src/pages/oidc.tsx | 96 +++++++++++++++++++++++---------------- src/pages/root.tsx | 2 +- src/pages/user.tsx | 12 ++--- 7 files changed, 102 insertions(+), 48 deletions(-) create mode 100644 src/helpers/oidcClient.ts diff --git a/compose.yaml b/compose.yaml index 8ecec6d..c4789ab 100644 --- a/compose.yaml +++ b/compose.yaml @@ -16,7 +16,7 @@ services: # - FFMPEG_ARGS=-hwaccel vulkan # additional arguments to pass to ffmpeg # - WEBROOT=/convertx # the root path of the web interface, leave empty to disable # - HIDE_HISTORY=true # hides the history tab in the web interface, defaults to false - - TZ=Asia/Koltaka # set your timezone, defaults to UTC + - TZ=Asia/Kolkata # set your timezone, defaults to UTC # - UNAUTHENTICATED_USER_SHARING=true # for use with ALLOW_UNAUTHENTICATED=true to share history with all unauthenticated users / devices # OIDC / SSO login (e.g. Authentik) - set all four of these to enable the "Login with ..." button # never commit real client IDs/secrets here - put them in a local, gitignored .env file instead diff --git a/src/db/db.ts b/src/db/db.ts index 3b7ca5c..212d8b8 100644 --- a/src/db/db.ts +++ b/src/db/db.ts @@ -10,8 +10,10 @@ CREATE TABLE IF NOT EXISTS users ( id INTEGER PRIMARY KEY AUTOINCREMENT, email TEXT NOT NULL, password TEXT NOT NULL, - oidc_sub TEXT + oidc_sub TEXT, + oidc_issuer TEXT ); +CREATE UNIQUE INDEX IF NOT EXISTS idx_users_oidc_identity ON users(oidc_issuer, oidc_sub); CREATE TABLE IF NOT EXISTS file_names ( id INTEGER PRIMARY KEY AUTOINCREMENT, job_id INTEGER NOT NULL, @@ -28,7 +30,7 @@ CREATE TABLE IF NOT EXISTS jobs ( num_files INTEGER DEFAULT 0, FOREIGN KEY (user_id) REFERENCES users(id) ); -PRAGMA user_version = 2;`); +PRAGMA user_version = 3;`); } const dbVersion = (db.query("PRAGMA user_version").get() as { user_version?: number }).user_version; @@ -42,6 +44,14 @@ if ((dbVersion ?? 0) < 2) { db.exec("PRAGMA user_version = 2;"); console.log("Updated database to version 2."); } +if ((dbVersion ?? 0) < 3) { + db.exec("ALTER TABLE users ADD COLUMN oidc_issuer TEXT;"); + // sub is only guaranteed unique within its issuer, so identity is the (issuer, sub) pair. + // SQLite treats every NULL as distinct, so local-only users (both columns NULL) never collide. + db.exec("CREATE UNIQUE INDEX IF NOT EXISTS idx_users_oidc_identity ON users(oidc_issuer, oidc_sub);"); + db.exec("PRAGMA user_version = 3;"); + console.log("Updated database to version 3."); +} // enable WAL mode db.exec("PRAGMA journal_mode = WAL;"); diff --git a/src/db/types.ts b/src/db/types.ts index d6719c1..5237b7b 100644 --- a/src/db/types.ts +++ b/src/db/types.ts @@ -21,4 +21,5 @@ export class User { email!: string; password!: string; oidc_sub!: string | null; + oidc_issuer!: string | null; } diff --git a/src/helpers/oidcClient.ts b/src/helpers/oidcClient.ts new file mode 100644 index 0000000..49bade1 --- /dev/null +++ b/src/helpers/oidcClient.ts @@ -0,0 +1,23 @@ +import * as client from "openid-client"; +import { OIDC_CLIENT_ID, OIDC_CLIENT_SECRET, OIDC_ENABLED, OIDC_ISSUER } from "./env"; + +export let oidcConfig: Awaited> | undefined; + +if (OIDC_ENABLED) { + try { + oidcConfig = await client.discovery( + new URL(OIDC_ISSUER), + OIDC_CLIENT_ID, + OIDC_CLIENT_SECRET, + ); + console.log("OIDC: discovered issuer", OIDC_ISSUER); + } catch (error) { + console.error("OIDC: failed to discover issuer, SSO login will be unavailable:", error); + } +} + +// OIDC_ENABLED only reflects that the env vars are set; this reflects whether +// discovery actually succeeded, i.e. whether the login button should be usable. +export function isOidcReady() { + return oidcConfig !== undefined; +} diff --git a/src/pages/oidc.tsx b/src/pages/oidc.tsx index a87c323..44d0fcd 100644 --- a/src/pages/oidc.tsx +++ b/src/pages/oidc.tsx @@ -5,33 +5,31 @@ import db from "../db/db"; import { User } from "../db/types"; import { HTTP_ALLOWED, - OIDC_CLIENT_ID, - OIDC_CLIENT_SECRET, - OIDC_ENABLED, OIDC_ISSUER, OIDC_REDIRECT_URI, OIDC_SCOPES, WEBROOT, } from "../helpers/env"; +import { oidcConfig } from "../helpers/oidcClient"; import { markFirstRunComplete, userService } from "./user"; -let oidcConfig: Awaited> | undefined; - -if (OIDC_ENABLED) { - try { - oidcConfig = await client.discovery( - new URL(OIDC_ISSUER), - OIDC_CLIENT_ID, - OIDC_CLIENT_SECRET, - ); - console.log("OIDC: discovered issuer", OIDC_ISSUER); - } catch (error) { - console.error("OIDC: failed to discover issuer, SSO login will be unavailable:", error); - } -} - const flowCookiePath = `${WEBROOT}/login/oidc`; +// PKCE verifier/state/nonce are all base64url (oauth4webapi's randomBytes()), +// so "." can never appear in a value and is safe as a delimiter here. This +// also avoids Elysia's cookie parser, which auto-JSON.parses any cookie value +// that looks like `{...}`/`[...]` - a JSON-encoded cookie here would get +// silently turned into an object before the route's `t.String()` schema ever +// saw it, failing validation before the handler even runs. +function parseFlowCookie(value: string): { code_verifier: string; state: string; nonce: string } | null { + const parts = value.split("."); + if (parts.length !== 3 || parts.some((part) => part.length === 0)) { + return null; + } + const [code_verifier, state, nonce] = parts as [string, string, string]; + return { code_verifier, state, nonce }; +} + export const oidc = new Elysia().use(userService).get( "/login/oidc", async ({ redirect, cookie: { oidcFlow } }) => { @@ -45,7 +43,7 @@ export const oidc = new Elysia().use(userService).get( const nonce = client.randomNonce(); oidcFlow.set({ - value: JSON.stringify({ code_verifier, state, nonce }), + value: `${code_verifier}.${state}.${nonce}`, httpOnly: true, secure: !HTTP_ALLOWED, sameSite: "lax", @@ -76,14 +74,16 @@ export const oidc = new Elysia().use(userService).get( return redirect(`${WEBROOT}/login`, 302); } - const { code_verifier, state, nonce } = JSON.parse(oidcFlow.value) as { - code_verifier: string; - state: string; - nonce: string; - }; + const flow = parseFlowCookie(oidcFlow.value); oidcFlow.path = flowCookiePath; oidcFlow.remove(); + if (!flow) { + console.error("OIDC: malformed oidcFlow cookie"); + return redirect(`${WEBROOT}/login`, 302); + } + const { code_verifier, state, nonce } = flow; + let tokens: Awaited>; try { tokens = await client.authorizationCodeGrant(oidcConfig, new URL(request.url), { @@ -103,10 +103,12 @@ export const oidc = new Elysia().use(userService).get( } let email = typeof claims.email === "string" ? claims.email : undefined; + let emailVerified = claims.email_verified === true; if (!email) { try { const userinfo = await client.fetchUserInfo(oidcConfig, tokens.access_token, claims.sub); email = typeof userinfo.email === "string" ? userinfo.email : undefined; + emailVerified = userinfo.email_verified === true; } catch (error) { console.error("OIDC: failed to fetch userinfo:", error); } @@ -117,28 +119,46 @@ export const oidc = new Elysia().use(userService).get( return redirect(`${WEBROOT}/login`, 302); } - let user = db.query("SELECT * FROM users WHERE oidc_sub = ?").as(User).get(claims.sub); + // sub is only guaranteed unique within its issuer, so identity is the (issuer, sub) pair. + let user = db + .query("SELECT * FROM users WHERE oidc_issuer = ? AND oidc_sub = ?") + .as(User) + .get(OIDC_ISSUER, claims.sub); if (!user) { - const existingByEmail = db.query("SELECT * FROM users WHERE email = ?").as(User).get(email); + // Only auto-link to an existing local account when the provider has + // positively verified the email - otherwise an unverified/attacker-set + // email claim could hijack an existing account. + if (emailVerified) { + const existingByEmail = db.query("SELECT * FROM users WHERE email = ?").as(User).get(email); + if (existingByEmail) { + db.query("UPDATE users SET oidc_sub = ?, oidc_issuer = ? WHERE id = ?").run( + claims.sub, + OIDC_ISSUER, + existingByEmail.id, + ); + user = existingByEmail; + } + } - if (existingByEmail) { - // Link the existing local account to this OIDC identity. - db.query("UPDATE users SET oidc_sub = ? WHERE id = ?").run(claims.sub, existingByEmail.id); - user = existingByEmail; - } else { + if (!user) { const isFirstUser = db.query("SELECT * FROM users").get() === null; // Local password login stays disabled for SSO-provisioned accounts; // this hash is never revealed and the field is only NOT NULL for schema reasons. const unusablePassword = await Bun.password.hash(randomUUID()); - db.query("INSERT INTO users (email, password, oidc_sub) VALUES (?, ?, ?)").run( - email, - unusablePassword, - claims.sub, - ); - user = db.query("SELECT * FROM users WHERE oidc_sub = ?").as(User).get(claims.sub); + // ON CONFLICT guards against two concurrent first-logins for the same + // new identity both passing the SELECT above and racing to insert. + db.query( + `INSERT INTO users (email, password, oidc_sub, oidc_issuer) + VALUES (?, ?, ?, ?) + ON CONFLICT(oidc_issuer, oidc_sub) DO NOTHING`, + ).run(email, unusablePassword, claims.sub, OIDC_ISSUER); + user = db + .query("SELECT * FROM users WHERE oidc_issuer = ? AND oidc_sub = ?") + .as(User) + .get(OIDC_ISSUER, claims.sub); - if (isFirstUser) { + if (isFirstUser && user) { markFirstRunComplete(); } } diff --git a/src/pages/root.tsx b/src/pages/root.tsx index 29ec14f..4afdc16 100644 --- a/src/pages/root.tsx +++ b/src/pages/root.tsx @@ -110,7 +110,7 @@ export const root = new Elysia().use(userService).get( <>
@@ -267,7 +267,7 @@ export const user = new Elysia() <>
@@ -318,14 +318,14 @@ export const user = new Elysia() ) : null} - {OIDC_ENABLED && !OIDC_ONLY ? ( + {isOidcReady() && !OIDC_ONLY ? (
or
) : null} - {OIDC_ENABLED ? ( + {isOidcReady() ? (