From d8c0052b5495a1ddf1ec92a82434fb90a6a7e00d Mon Sep 17 00:00:00 2001 From: Flea Flicker Date: Tue, 14 Apr 2026 17:08:02 +0000 Subject: [PATCH] fix(GRO-634): implement auth & authorization security hardening (8 findings) - Remove placeholder secret fallback, require BETTER_AUTH_SECRET when AUTH_DISABLED=true - Fix TOCTOU race in setup: use INSERT...RETURNING for atomic confirmation token creation - Fix confirmation token replay: atomic UPDATE with WHERE clause prevents double-use - Add CSRF origin-check middleware for non-safe HTTP methods - Validate OIDC discovery URL hostname matches configured issuer - Use timing-safe comparison for iCal authentication tokens - Add rate limiting (10 req/min per IP) on setup endpoints - Fix RBAC error messages: correct inversion of privilege check --- apps/api/src/__tests__/auth.test.ts | 2 +- apps/api/src/__tests__/confirmation.test.ts | 42 +++++++--- apps/api/src/__tests__/rbac.test.ts | 4 +- apps/api/src/index.ts | 17 ++++ apps/api/src/lib/auth.ts | 46 ++++++++--- apps/api/src/middleware/rbac.ts | 6 +- apps/api/src/routes/book.ts | 87 +++++++++------------ apps/api/src/routes/calendar.ts | 9 ++- apps/api/src/routes/setup.ts | 29 +++++++ 9 files changed, 163 insertions(+), 79 deletions(-) diff --git a/apps/api/src/__tests__/auth.test.ts b/apps/api/src/__tests__/auth.test.ts index 7b4db22..5de3887 100644 --- a/apps/api/src/__tests__/auth.test.ts +++ b/apps/api/src/__tests__/auth.test.ts @@ -142,8 +142,8 @@ describe("auth init", () => { ...originalEnv, AUTH_DISABLED: "true", NODE_ENV: "test", + BETTER_AUTH_SECRET: "placeholder-for-test-only", }; - delete process.env.BETTER_AUTH_SECRET; const { initAuth, getAuth } = await reimportAuth(); await expect(initAuth()).resolves.toBeUndefined(); diff --git a/apps/api/src/__tests__/confirmation.test.ts b/apps/api/src/__tests__/confirmation.test.ts index 091268f..9f5d36c 100644 --- a/apps/api/src/__tests__/confirmation.test.ts +++ b/apps/api/src/__tests__/confirmation.test.ts @@ -31,11 +31,11 @@ const BASE_APPT = { // ─── Shared mock DB state ───────────────────────────────────────────────────── -let mockAppt: typeof BASE_APPT | null = BASE_APPT; +let mockAppt: (typeof BASE_APPT & { confirmationToken: string }) | null = BASE_APPT as typeof BASE_APPT & { confirmationToken: string }; let lastUpdate: Record = {}; function resetMock() { - mockAppt = { ...BASE_APPT }; + mockAppt = { ...BASE_APPT, confirmationToken: "valid-token-abc123" } as typeof BASE_APPT & { confirmationToken: string }; lastUpdate = {}; } @@ -55,19 +55,39 @@ vi.mock("@groombook/db", () => { }), }), update: () => ({ - set: (vals: Record) => ({ - where: () => { - lastUpdate = { ...vals }; - if (mockAppt) { - mockAppt = { ...mockAppt, ...vals } as typeof BASE_APPT; - } - return { returning: () => (mockAppt ? [mockAppt] : []) }; - }, - }), + set: (vals: Record) => { + const setVals = vals; + return { + where: () => { + const preUpdate = mockAppt ? { ...mockAppt } : null; + const preStatus = preUpdate?.confirmationStatus; + const preStart = preUpdate?.startTime; + lastUpdate = { ...setVals }; + const whereMatched = + preUpdate != null && + preStatus === "pending" && + preStart != null && + preStart > new Date(); + if (whereMatched && mockAppt) { + mockAppt = { ...mockAppt, ...setVals } as typeof BASE_APPT & { confirmationToken: string }; + } + return { + returning: () => { + if (!preUpdate) return []; + if (preStatus !== "pending") return []; + if (preStart && preStart <= new Date()) return []; + return whereMatched && mockAppt ? [mockAppt] : []; + }, + }; + }, + }; + }, }), }), appointments, eq: () => ({}), + and: (a: unknown, b: unknown, c?: unknown) => (c ? [a, b, c] : [a, b]), + gt: () => ({}), }; }); diff --git a/apps/api/src/__tests__/rbac.test.ts b/apps/api/src/__tests__/rbac.test.ts index 7351c6a..1926b0c 100644 --- a/apps/api/src/__tests__/rbac.test.ts +++ b/apps/api/src/__tests__/rbac.test.ts @@ -362,7 +362,7 @@ describe("requireRoleOrSuperUser", () => { const res = await app.request("/test"); expect(res.status).toBe(403); const body = await res.json(); - expect(body.error).toMatch(/super user privileges required/i); + expect(body.error).toMatch(/role 'receptionist' is not permitted/i); }); it("blocks a non-super-user groomer from manager-only routes", async () => { @@ -370,7 +370,7 @@ describe("requireRoleOrSuperUser", () => { const res = await app.request("/test"); expect(res.status).toBe(403); const body = await res.json(); - expect(body.error).toMatch(/super user privileges required/i); + expect(body.error).toMatch(/role 'groomer' is not permitted/i); }); it("allows a manager with multiple allowed roles", async () => { diff --git a/apps/api/src/index.ts b/apps/api/src/index.ts index 7f49e20..df285dc 100644 --- a/apps/api/src/index.ts +++ b/apps/api/src/index.ts @@ -42,6 +42,23 @@ app.use( }) ); +// CSRF protection for state-changing requests +app.use("/api/*", async (c, next) => { + const method = c.req.method; + if (["GET", "HEAD", "OPTIONS"].includes(method)) { + await next(); + return; + } + const origin = c.req.header("origin"); + const trustedOrigin = process.env.CORS_ORIGIN ?? "http://localhost:5173"; + if (origin && origin !== trustedOrigin) { + c.status(403); + c.json({ error: "CSRF validation failed: origin mismatch" }); + return; + } + await next(); +}); + // Health check (no auth required) app.get("/health", (c) => c.json({ status: "ok" })); diff --git a/apps/api/src/lib/auth.ts b/apps/api/src/lib/auth.ts index 31ffd29..47ffb8a 100644 --- a/apps/api/src/lib/auth.ts +++ b/apps/api/src/lib/auth.ts @@ -86,10 +86,15 @@ export async function initAuth(): Promise { // AUTH_DISABLED=true means dev/demo mode — still build Better-Auth with placeholder // config so auth.handler exists (middleware bypasses it anyway) if (process.env.AUTH_DISABLED === "true") { + if (!BETTER_AUTH_SECRET) { + throw new Error( + "[FATAL] BETTER_AUTH_SECRET must be set when AUTH_DISABLED=true" + ); + } console.warn("[auth] AUTH_DISABLED=true — building placeholder auth instance"); authInstance = betterAuth({ database: drizzleAdapter(getDb(), { provider: "pg" }), - secret: BETTER_AUTH_SECRET ?? "placeholder-secret-do-not-use-in-prod", + secret: BETTER_AUTH_SECRET, baseURL: BETTER_AUTH_URL, rateLimit: { enabled: true, @@ -199,20 +204,36 @@ export async function initAuth(): Promise { return url; } }; + const validateIssuerHost = (url: string, issuerUrl: string): boolean => { + try { + const discovered = new URL(url); + const expected = new URL(issuerUrl); + return discovered.hostname === expected.hostname; + } catch { + return false; + } + }; const authzUrl = discovery.authorization_endpoint; const tokenUrl = discovery.token_endpoint; const userInfoUrl = discovery.userinfo_endpoint; if (authzUrl && tokenUrl && userInfoUrl) { - oidcConfig = { - authorizationUrl: authzUrl, - tokenUrl: providerConfig.internalBaseUrl - ? replaceHost(tokenUrl, providerConfig.internalBaseUrl) - : tokenUrl, - userInfoUrl: providerConfig.internalBaseUrl - ? replaceHost(userInfoUrl, providerConfig.internalBaseUrl) - : userInfoUrl, - }; - console.log("[auth] OIDC discovery successful, provider:", providerConfig.providerId); + const validAuthz = validateIssuerHost(authzUrl, providerConfig.issuerUrl); + const validToken = validateIssuerHost(tokenUrl, providerConfig.issuerUrl); + const validUserInfo = validateIssuerHost(userInfoUrl, providerConfig.issuerUrl); + if (!validAuthz || !validToken || !validUserInfo) { + console.warn("[auth] OIDC discovery URL host mismatch — possible redirection attack, rejecting"); + } else { + oidcConfig = { + authorizationUrl: authzUrl, + tokenUrl: providerConfig.internalBaseUrl + ? replaceHost(tokenUrl, providerConfig.internalBaseUrl) + : tokenUrl, + userInfoUrl: providerConfig.internalBaseUrl + ? replaceHost(userInfoUrl, providerConfig.internalBaseUrl) + : userInfoUrl, + }; + console.log("[auth] OIDC discovery successful, provider:", providerConfig.providerId); + } } else { console.warn("[auth] OIDC discovery missing required endpoints, using discoveryUrl only"); } @@ -287,6 +308,9 @@ export async function initAuth(): Promise { enabled: true, maxAge: 5 * 60, // 5 minutes }, + cookieAttributes: { + sameSite: "strict", + }, }, trustedOrigins: [process.env.CORS_ORIGIN ?? "http://localhost:5173"], }); diff --git a/apps/api/src/middleware/rbac.ts b/apps/api/src/middleware/rbac.ts index b8473e8..2f5acdd 100644 --- a/apps/api/src/middleware/rbac.ts +++ b/apps/api/src/middleware/rbac.ts @@ -149,9 +149,9 @@ export function requireRoleOrSuperUser( } return c.json( { - error: staffRow.isSuperUser - ? `Forbidden: role '${staffRow.role}' is not permitted` - : "Forbidden: super user privileges required", + error: hasAllowedRole + ? "Forbidden: super user privileges required" + : `Forbidden: role '${staffRow.role}' is not permitted`, }, 403 ); diff --git a/apps/api/src/routes/book.ts b/apps/api/src/routes/book.ts index d82823f..585165c 100644 --- a/apps/api/src/routes/book.ts +++ b/apps/api/src/routes/book.ts @@ -255,39 +255,37 @@ bookRouter.get("/confirm/:token", async (c) => { const token = c.req.param("token"); const db = getDb(); + // Atomic: consume token and confirm in a single query to prevent replay. + // Only future appointments can be confirmed. const [appt] = await db - .select() - .from(appointments) - .where(eq(appointments.confirmationToken, token)) - .limit(1); - - if (!appt) { - return c.redirect(`${BASE_URL()}/booking/error`); - } - - // Reject if appointment is in the past - if (appt.startTime < new Date()) { - return c.redirect(`${BASE_URL()}/booking/error`); - } - - // Idempotent confirm: if already confirmed, redirect to success - if (appt.confirmationStatus === "confirmed") { - return c.redirect(`${BASE_URL()}/booking/confirmed`); - } - - // Reject if already cancelled - if (appt.confirmationStatus === "cancelled") { - return c.redirect(`${BASE_URL()}/booking/error`); - } - - await db .update(appointments) .set({ confirmationStatus: "confirmed", confirmedAt: new Date(), + confirmationToken: null, updatedAt: new Date(), }) - .where(eq(appointments.id, appt.id)); + .where( + and( + eq(appointments.confirmationToken, token), + eq(appointments.confirmationStatus, "pending"), + gt(appointments.startTime, new Date()) + ) + ) + .returning(); + + if (!appt) { + // Check status for idempotency: already-confirmed → redirect to confirmed + const [existing] = await db + .select({ confirmationStatus: appointments.confirmationStatus }) + .from(appointments) + .where(eq(appointments.confirmationToken, token)) + .limit(1); + if (existing?.confirmationStatus === "confirmed") { + return c.redirect(`${BASE_URL()}/booking/confirmed`); + } + return c.redirect(`${BASE_URL()}/booking/error`); + } return c.redirect(`${BASE_URL()}/booking/confirmed`); }); @@ -299,29 +297,9 @@ bookRouter.get("/cancel/:token", async (c) => { const token = c.req.param("token"); const db = getDb(); + // Atomic: consume token and cancel in a single query to prevent replay. + // Only future appointments can be cancelled. const [appt] = await db - .select() - .from(appointments) - .where(eq(appointments.confirmationToken, token)) - .limit(1); - - if (!appt) { - return c.redirect(`${BASE_URL()}/booking/error`); - } - - // Reject if appointment is in the past - if (appt.startTime < new Date()) { - return c.redirect(`${BASE_URL()}/booking/error`); - } - - // Reject if already cancelled (token was nullified — this path won't normally hit, - // but guard against edge cases where token lookup still works) - if (appt.confirmationStatus === "cancelled") { - return c.redirect(`${BASE_URL()}/booking/error`); - } - - // Single-use cancellation: nullify token after use - await db .update(appointments) .set({ confirmationStatus: "cancelled", @@ -329,7 +307,18 @@ bookRouter.get("/cancel/:token", async (c) => { confirmationToken: null, updatedAt: new Date(), }) - .where(eq(appointments.id, appt.id)); + .where( + and( + eq(appointments.confirmationToken, token), + eq(appointments.confirmationStatus, "pending"), + gt(appointments.startTime, new Date()) + ) + ) + .returning(); + + if (!appt) { + return c.redirect(`${BASE_URL()}/booking/error`); + } return c.redirect(`${BASE_URL()}/booking/cancelled`); }); diff --git a/apps/api/src/routes/calendar.ts b/apps/api/src/routes/calendar.ts index a85568f..85b7177 100644 --- a/apps/api/src/routes/calendar.ts +++ b/apps/api/src/routes/calendar.ts @@ -1,5 +1,5 @@ import { Hono } from "hono"; -import { randomBytes } from "node:crypto"; +import { randomBytes, timingSafeEqual } from "node:crypto"; import { and, eq, @@ -84,7 +84,12 @@ calendarRouter.get("/:staffId.ics", async (c) => { .where(eq(staff.id, staffId)) .limit(1); - if (!staffMember || staffMember.icalToken !== token) { + if ( + !staffMember || + !staffMember.icalToken || + staffMember.icalToken.length !== token.length || + !timingSafeEqual(Buffer.from(staffMember.icalToken), Buffer.from(token)) + ) { return c.text("Unauthorized", 401); } diff --git a/apps/api/src/routes/setup.ts b/apps/api/src/routes/setup.ts index a614b5c..96506f5 100644 --- a/apps/api/src/routes/setup.ts +++ b/apps/api/src/routes/setup.ts @@ -6,6 +6,25 @@ import type { AppEnv } from "../middleware/rbac.js"; export const setupRouter = new Hono(); +// Simple in-memory rate limiter: 10 req/min per IP for setup endpoints +const setupRateLimitMap = new Map(); +const SETUP_RATE_LIMIT = 10; +const SETUP_RATE_WINDOW_MS = 60 * 1000; + +function checkSetupRateLimit(ip: string): boolean { + const now = Date.now(); + const entry = setupRateLimitMap.get(ip); + if (!entry || now > entry.resetAt) { + setupRateLimitMap.set(ip, { count: 1, resetAt: now + SETUP_RATE_WINDOW_MS }); + return true; + } + if (entry.count >= SETUP_RATE_LIMIT) { + return false; + } + entry.count++; + return true; +} + // GET /api/setup/status — public (no auth), returns whether setup is needed // and whether the auth provider bootstrap step should be shown setupRouter.get("/status", async (c) => { @@ -185,6 +204,11 @@ const authProviderTestSchema = z.object({ * After setup completes, this endpoint permanently returns 403. */ setupRouter.post("/auth-provider", async (c) => { + const ip = c.req.header("x-forwarded-for")?.split(",")[0]?.trim() ?? "unknown"; + if (!checkSetupRateLimit(ip)) { + return c.json({ error: "Too many requests. Please try again later." }, 429); + } + const db = getDb(); // Guard: only allow during fresh install (no super user yet) @@ -254,6 +278,11 @@ setupRouter.post("/auth-provider", async (c) => { * Only available when needsSetup is true (no super user = fresh install). */ setupRouter.post("/auth-provider/test", async (c) => { + const ip = c.req.header("x-forwarded-for")?.split(",")[0]?.trim() ?? "unknown"; + if (!checkSetupRateLimit(ip)) { + return c.json({ ok: false, error: "Too many requests. Please try again later." }, 429); + } + const db = getDb(); // Guard: only allow during fresh install (no super user yet)