From 161dd635fd3508315d33c19d9e8a4810c06f5f8b Mon Sep 17 00:00:00 2001 From: Kieran Klukas Date: Mon, 27 Jul 2026 23:37:29 -0400 Subject: [PATCH] security: fix critical auth flow vulnerabilities MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bind login challenges to the challenge value the client used (not "latest row wins") so cross-session challenge confusion and login DoS are no longer possible. Burn challenges on any verify attempt, not just success. Reject passkey counter regressions to catch cloned authenticators. Wrap invite consumption + user/credential creation in a transaction so a raced invite rolls back cleanly instead of leaving an orphaned account with a working session. Re-validate redirect_uri against the app's registered list in the consent POST (the GET path already did this) so attackers can't mint authorization codes bound to arbitrary redirect URIs. Escape appName in error-page hints (stored XSS via attacker-controlled client_name) and use URLSearchParams for success/deny redirects so the state parameter can't inject extra query params. 💘 Generated with Crush Assisted-by: Crush:kimi-k3 --- src/client/login.ts | 12 +- src/routes/auth.ts | 219 ++++++++++++++++++++-------------- src/routes/oauth/authorize.ts | 46 +++++-- 3 files changed, 177 insertions(+), 100 deletions(-) diff --git a/src/client/login.ts b/src/client/login.ts index 3595247..ae1fdb3 100644 --- a/src/client/login.ts +++ b/src/client/login.ts @@ -119,7 +119,11 @@ async function initConditionalUI() { const verifyRes = await fetch("/auth/login/verify", { method: "POST", headers: { "Content-Type": "application/json" }, - body: JSON.stringify({ response: authResponse, conditional: true }), + body: JSON.stringify({ + response: authResponse, + conditional: true, + challenge: options.challenge, + }), }); if (!verifyRes.ok) { @@ -199,7 +203,11 @@ loginForm.addEventListener("submit", async (e) => { const verifyRes = await fetch("/auth/login/verify", { method: "POST", headers: { "Content-Type": "application/json" }, - body: JSON.stringify({ username, response: authResponse }), + body: JSON.stringify({ + username, + response: authResponse, + challenge: options.challenge, + }), }); if (!verifyRes.ok) { diff --git a/src/routes/auth.ts b/src/routes/auth.ts index 6a11391..b234453 100644 --- a/src/routes/auth.ts +++ b/src/routes/auth.ts @@ -16,6 +16,15 @@ import { checkLdapGroupMembership, checkLdapUser } from "../ldap-cleanup"; const RP_NAME = "Indiko"; +// Thrown inside the registration transaction when the invite is exhausted so +// the whole registration rolls back. +class InviteExhaustedError extends Error { + constructor() { + super("Invite code fully used"); + this.name = "InviteExhaustedError"; + } +} + export function canRegister(_req: Request): Response { const userCount = db.query("SELECT COUNT(*) as count FROM users").get() as { count: number; @@ -208,6 +217,12 @@ export async function registerVerify(req: Request): Promise { return Response.json({ error: "Challenge expired" }, { status: 400 }); } + // Burn the challenge immediately — it must not be reusable regardless + // of whether verification succeeds or fails. + db.query("DELETE FROM challenges WHERE challenge = ?").run( + challenge.challenge, + ); + // Check if this is bootstrap (first user) const userCount = db.query("SELECT COUNT(*) as count FROM users").get() as { count: number; @@ -285,93 +300,101 @@ export async function registerVerify(req: Request): Promise { const { credential } = verification.registrationInfo; - // Check if this user is being provisioned via LDAP - let isLdapProvisioned = false; - if (inviteId) { - const invite = db - .query("SELECT ldap_username FROM invites WHERE id = ?") - .get(inviteId) as { ldap_username: string | null } | undefined; - isLdapProvisioned = - invite?.ldap_username !== null && invite?.ldap_username !== undefined; - } - - let userId: number; - let userIsAdmin: boolean; - - if (isPasskeyReset && existingUser) { - // Passkey reset: use existing user, just add credential - userId = existingUser.id; - userIsAdmin = existingUser.is_admin === 1; - } else { - // Create new user (bootstrap is always admin, invited users are regular users) - const insertUser = db.query( - "INSERT INTO users (username, name, is_admin, tier, role, provisioned_via_ldap) VALUES (?, ?, ?, ?, ?, ?) RETURNING id", - ); - const user = insertUser.get( - username, - username, - isBootstrap ? 1 : 0, - isBootstrap ? "admin" : "user", - isBootstrap ? "admin" : "user", - isLdapProvisioned ? 1 : 0, - ) as { id: number }; - userId = user.id; - userIsAdmin = isBootstrap; - } + // Consume the invite and create user/credential/session in one + // transaction. If the invite is exhausted the whole registration rolls + // back — no orphaned accounts or sessions. + const usedAt = Math.floor(Date.now() / 1000); + let userId = 0; + let userIsAdmin = false; - // Store credential - // credential.id is a Uint8Array, convert to Buffer for storage - db.query( - "INSERT INTO credentials (user_id, credential_id, public_key, counter, name) VALUES (?, ?, ?, ?, ?)", - ).run( - userId, - Buffer.from(credential.id), - Buffer.from(credential.publicKey), - credential.counter, - isPasskeyReset ? "Reset Passkey" : "Primary Passkey", - ); + try { + db.transaction(() => { + // Atomically consume the invite first — before any user rows exist. + if (inviteId) { + const result = db + .query( + "UPDATE invites SET current_uses = current_uses + 1 WHERE id = ? AND current_uses < max_uses", + ) + .run(inviteId); + + if (result.changes === 0) { + throw new InviteExhaustedError(); + } + } - // Mark invite as used if applicable - if (inviteId) { - const usedAt = Math.floor(Date.now() / 1000); + // Check if this user is being provisioned via LDAP + let isLdapProvisioned = false; + if (inviteId) { + const invite = db + .query("SELECT ldap_username FROM invites WHERE id = ?") + .get(inviteId) as { ldap_username: string | null } | undefined; + isLdapProvisioned = + invite?.ldap_username !== null && + invite?.ldap_username !== undefined; + } - // Atomically increment invite usage counter while checking max_uses limit - const result = db - .query( - "UPDATE invites SET current_uses = current_uses + 1 WHERE id = ? AND current_uses < max_uses", - ) - .run(inviteId); + if (isPasskeyReset && existingUser) { + // Passkey reset: use existing user, just add credential + userId = existingUser.id; + userIsAdmin = existingUser.is_admin === 1; + } else { + // Create new user (bootstrap is always admin, invited users are regular users) + const insertUser = db.query( + "INSERT INTO users (username, name, is_admin, tier, role, provisioned_via_ldap) VALUES (?, ?, ?, ?, ?, ?) RETURNING id", + ); + const user = insertUser.get( + username, + username, + isBootstrap ? 1 : 0, + isBootstrap ? "admin" : "user", + isBootstrap ? "admin" : "user", + isLdapProvisioned ? 1 : 0, + ) as { id: number }; + userId = user.id; + userIsAdmin = isBootstrap; + } + + // Store credential + // credential.id is a Uint8Array, convert to Buffer for storage + db.query( + "INSERT INTO credentials (user_id, credential_id, public_key, counter, name) VALUES (?, ?, ?, ?, ?)", + ).run( + userId, + Buffer.from(credential.id), + Buffer.from(credential.publicKey), + credential.counter, + isPasskeyReset ? "Reset Passkey" : "Primary Passkey", + ); - // Check if update was successful (0 rows affected means invite was already fully used) - if (result.changes === 0) { + if (inviteId) { + // Record this invite use + db.query( + "INSERT INTO invite_uses (invite_id, user_id, used_at) VALUES (?, ?, ?)", + ).run(inviteId, userId, usedAt); + + // Assign app roles to the new user (skip for passkey reset - they already have roles) + if (inviteRoles.length > 0 && !isPasskeyReset) { + const insertPermission = db.query( + "INSERT INTO permissions (user_id, app_id, role) VALUES (?, ?, ?)", + ); + for (const { app_id, role } of inviteRoles) { + insertPermission.run(userId, app_id, role); + } + } + } + })(); + } catch (err) { + if (err instanceof InviteExhaustedError) { return Response.json( { error: "Invite code fully used" }, { status: 403 }, ); } - - // Record this invite use - db.query( - "INSERT INTO invite_uses (invite_id, user_id, used_at) VALUES (?, ?, ?)", - ).run(inviteId, userId, usedAt); - - // Assign app roles to the new user (skip for passkey reset - they already have roles) - if (inviteRoles.length > 0 && !isPasskeyReset) { - const insertPermission = db.query( - "INSERT INTO permissions (user_id, app_id, role) VALUES (?, ?, ?)", - ); - for (const { app_id, role } of inviteRoles) { - insertPermission.run(userId, app_id, role); - } - } + throw err; } - // Delete challenge - db.query("DELETE FROM challenges WHERE challenge = ?").run( - challenge.challenge, - ); - - // Create session + // Create session (outside the transaction — if this fails the account + // still exists and the user can log in again) const token = crypto.randomUUID(); const expiresAt = Math.floor(Date.now() / 1000) + 86400; // 24 hours db.query( @@ -516,10 +539,16 @@ export async function loginOptions(req: Request): Promise { export async function loginVerify(req: Request): Promise { try { const body = await req.json(); - const { username, response, conditional } = body as { + const { + username, + response, + conditional, + challenge: clientChallenge, + } = body as { username?: string; response: AuthenticationResponseJSON; conditional?: boolean; + challenge?: string; }; if (!response) { @@ -530,6 +559,10 @@ export async function loginVerify(req: Request): Promise { return Response.json({ error: "Username required" }, { status: 400 }); } + if (!clientChallenge) { + return Response.json({ error: "Challenge required" }, { status: 400 }); + } + // Look up credential by ID to discover the username const credentialIdString = response.id; @@ -570,14 +603,15 @@ export async function loginVerify(req: Request): Promise { const user = { id: credentialWithUser.user_id }; const resolvedUsername = credentialWithUser.username; - // Verify challenge exists and is valid - // Conditional UI stores challenge with empty username; normal flow uses the actual username + // Verify challenge exists and is valid — look up by the challenge value + // the client used (returned in the verify body), not "latest row wins". + // This prevents cross-session challenge confusion and login DoS. const challengeUsername = conditional ? "" : resolvedUsername; const challenge = db .query( - "SELECT challenge, expires_at FROM challenges WHERE username = ? AND type = 'authentication' ORDER BY created_at DESC LIMIT 1", + "SELECT challenge, expires_at FROM challenges WHERE challenge = ? AND username = ? AND type = 'authentication'", ) - .get(challengeUsername) as + .get(clientChallenge, challengeUsername) as | { challenge: string; expires_at: number } | undefined; @@ -590,6 +624,12 @@ export async function loginVerify(req: Request): Promise { return Response.json({ error: "Challenge expired" }, { status: 400 }); } + // Burn the challenge immediately — it must not be reusable regardless + // of whether verification succeeds or fails. + db.query("DELETE FROM challenges WHERE challenge = ?").run( + challenge.challenge, + ); + // Verify authentication response let verification: VerifiedAuthenticationResponse; try { @@ -613,20 +653,25 @@ export async function loginVerify(req: Request): Promise { return Response.json({ error: "Verification failed" }, { status: 400 }); } + // Reject cloned authenticators: counter must advance. Authenticators + // that don't support counters always report 0; skip the check then. + const newCounter = verification.authenticationInfo.newCounter; + if (credential.counter > 0 && newCounter <= credential.counter) { + console.warn( + `[auth] counter regression for credential ${credential.credential_id.toString()} — possible cloned passkey (stored=${credential.counter}, got=${newCounter})`, + ); + return Response.json({ error: "Verification failed" }, { status: 400 }); + } + // Update credential counter db.query( "UPDATE credentials SET counter = ? WHERE user_id = ? AND credential_id = ?", ).run( - verification.authenticationInfo.newCounter, + newCounter, user.id, credential.credential_id, ); - // Delete challenge - db.query("DELETE FROM challenges WHERE challenge = ?").run( - challenge.challenge, - ); - // Create session const token = crypto.randomUUID(); const expiresAt = Math.floor(Date.now() / 1000) + 86400; // 24 hours diff --git a/src/routes/oauth/authorize.ts b/src/routes/oauth/authorize.ts index bc667af..1ff7040 100644 --- a/src/routes/oauth/authorize.ts +++ b/src/routes/oauth/authorize.ts @@ -1,7 +1,7 @@ import crypto from "node:crypto"; import { db } from "../../db"; import { ensureApp } from "../../lib/oauth/client-metadata"; -import { consentPage, errorPage } from "../../lib/oauth/pages"; +import { consentPage, errorPage, escapeHtml } from "../../lib/oauth/pages"; import { canonicalizeURL } from "../../lib/oauth/urls"; import { getUserFromCookie } from "../../lib/session"; import { token } from "./token"; @@ -113,7 +113,7 @@ export async function authorizeGet(req: Request): Promise { message: "The OAuth authorization request failed because the provided redirect_uri is not registered for this client application.", details, - hint: `The redirect_uri must exactly match a registered URI for ${appName}. If you are the application developer, please ensure your redirect_uri matches the one registered with this authorization server.`, + hint: `The redirect_uri must exactly match a registered URI for ${escapeHtml(appName)}. If you are the application developer, please ensure your redirect_uri matches the one registered with this authorization server.`, }); } @@ -205,9 +205,11 @@ export async function authorizeGet(req: Request): Promise { ).run(Math.floor(Date.now() / 1000), user.userId, clientId); const origin = process.env.ORIGIN || "http://localhost:3000"; - return Response.redirect( - `${redirectUri}?code=${code}&state=${state}&iss=${encodeURIComponent(origin)}`, - ); + const autoApproveUrl = new URL(redirectUri); + autoApproveUrl.searchParams.set("code", code); + autoApproveUrl.searchParams.set("state", state); + autoApproveUrl.searchParams.set("iss", origin); + return Response.redirect(autoApproveUrl.toString(), 302); } } @@ -338,11 +340,31 @@ export async function authorizePost(req: Request): Promise { }); } + // Re-validate redirect_uri against the app's registered set — the consent + // form POSTs attacker-controllable hidden fields, so we must not trust the + // submitted redirect_uri without checking it against the registered list. + const app = db + .query("SELECT redirect_uris FROM apps WHERE client_id = ?") + .get(clientId) as { redirect_uris: string } | undefined; + + if (!app) { + return new Response("Invalid client_id", { status: 400 }); + } + + const allowedRedirects = JSON.parse(app.redirect_uris) as string[]; + if (!allowedRedirects.includes(redirectUri)) { + return new Response("redirect_uri not registered for this client", { + status: 400, + }); + } + if (action === "deny") { const origin = process.env.ORIGIN || "http://localhost:3000"; - return Response.redirect( - `${redirectUri}?error=access_denied&state=${state}&iss=${encodeURIComponent(origin)}`, - ); + const denyUrl = new URL(redirectUri); + denyUrl.searchParams.set("error", "access_denied"); + denyUrl.searchParams.set("state", state); + denyUrl.searchParams.set("iss", origin); + return Response.redirect(denyUrl.toString(), 302); } // Get the scopes the user actually approved (from checkboxes) @@ -407,7 +429,9 @@ export async function authorizePost(req: Request): Promise { ); const origin = process.env.ORIGIN || "http://localhost:3000"; - return Response.redirect( - `${redirectUri}?code=${code}&state=${state}&iss=${encodeURIComponent(origin)}`, - ); + const successUrl = new URL(redirectUri); + successUrl.searchParams.set("code", code); + successUrl.searchParams.set("state", state); + successUrl.searchParams.set("iss", origin); + return Response.redirect(successUrl.toString(), 302); } -- 2.51.2