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.
This commit is contained in:
KeplerAeroIT
parent
d7d9aae148
commit
7aa647c1bd
|
|
@ -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
|
||||
|
|
|
|||
14
src/db/db.ts
14
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;");
|
||||
|
|
|
|||
|
|
@ -21,4 +21,5 @@ export class User {
|
|||
email!: string;
|
||||
password!: string;
|
||||
oidc_sub!: string | null;
|
||||
oidc_issuer!: string | null;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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<ReturnType<typeof client.discovery>> | 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;
|
||||
}
|
||||
|
|
@ -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<ReturnType<typeof client.discovery>> | 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<ReturnType<typeof client.authorizationCodeGrant>>;
|
||||
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();
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -110,7 +110,7 @@ export const root = new Elysia().use(userService).get(
|
|||
<>
|
||||
<Header
|
||||
webroot={WEBROOT}
|
||||
accountRegistration={ACCOUNT_REGISTRATION}
|
||||
accountRegistration={ACCOUNT_REGISTRATION && !OIDC_ONLY}
|
||||
allowUnauthenticated={ALLOW_UNAUTHENTICATED}
|
||||
hideHistory={HIDE_HISTORY}
|
||||
loggedIn
|
||||
|
|
|
|||
|
|
@ -10,11 +10,11 @@ import {
|
|||
ALLOW_UNAUTHENTICATED,
|
||||
HIDE_HISTORY,
|
||||
HTTP_ALLOWED,
|
||||
OIDC_ENABLED,
|
||||
OIDC_NAME,
|
||||
OIDC_ONLY,
|
||||
WEBROOT,
|
||||
} from "../helpers/env";
|
||||
import { isOidcReady } from "../helpers/oidcClient";
|
||||
|
||||
export let FIRST_RUN = db.query("SELECT * FROM users").get() === null || false;
|
||||
|
||||
|
|
@ -145,7 +145,7 @@ export const user = new Elysia()
|
|||
<>
|
||||
<Header
|
||||
webroot={WEBROOT}
|
||||
accountRegistration={ACCOUNT_REGISTRATION}
|
||||
accountRegistration={ACCOUNT_REGISTRATION && !OIDC_ONLY}
|
||||
allowUnauthenticated={ALLOW_UNAUTHENTICATED}
|
||||
hideHistory={HIDE_HISTORY}
|
||||
/>
|
||||
|
|
@ -267,7 +267,7 @@ export const user = new Elysia()
|
|||
<>
|
||||
<Header
|
||||
webroot={WEBROOT}
|
||||
accountRegistration={ACCOUNT_REGISTRATION}
|
||||
accountRegistration={ACCOUNT_REGISTRATION && !OIDC_ONLY}
|
||||
allowUnauthenticated={ALLOW_UNAUTHENTICATED}
|
||||
hideHistory={HIDE_HISTORY}
|
||||
/>
|
||||
|
|
@ -318,14 +318,14 @@ export const user = new Elysia()
|
|||
</div>
|
||||
</form>
|
||||
) : null}
|
||||
{OIDC_ENABLED && !OIDC_ONLY ? (
|
||||
{isOidcReady() && !OIDC_ONLY ? (
|
||||
<div class="my-4 flex items-center gap-4 text-sm text-neutral-500">
|
||||
<hr class="flex-1 border-neutral-700" />
|
||||
or
|
||||
<hr class="flex-1 border-neutral-700" />
|
||||
</div>
|
||||
) : null}
|
||||
{OIDC_ENABLED ? (
|
||||
{isOidcReady() ? (
|
||||
<a
|
||||
href={`${WEBROOT}/login/oidc`}
|
||||
role="button"
|
||||
|
|
@ -423,7 +423,7 @@ export const user = new Elysia()
|
|||
<>
|
||||
<Header
|
||||
webroot={WEBROOT}
|
||||
accountRegistration={ACCOUNT_REGISTRATION}
|
||||
accountRegistration={ACCOUNT_REGISTRATION && !OIDC_ONLY}
|
||||
allowUnauthenticated={ALLOW_UNAUTHENTICATED}
|
||||
hideHistory={HIDE_HISTORY}
|
||||
loggedIn
|
||||
|
|
|
|||
Loading…
Reference in New Issue