From 1cce35441396e0283fbf80a42f7aa07a3a622790 Mon Sep 17 00:00:00 2001 From: Flea Flicker Date: Tue, 14 Apr 2026 22:52:44 +0000 Subject: [PATCH 1/3] fix(GRO-622): security hardening for auth, authorization, and token handling - Remove placeholder secret fallback in AUTH_DISABLED mode (auth.ts) - Make auth-provider setup atomic via DB transaction (setup.ts) - Fix confirmation token replay with atomic UPDATE...WHERE (book.ts) - Add strict CORS origin allowlist validation (index.ts) - Validate OIDC discovery URL hostname matches issuer (auth.ts) - Use timingSafeEqual for iCal token comparison (calendar.ts) - Add in-memory rate limiting to setup endpoints (setup.ts) - Keep RBAC error message correct (rbac.ts - already correct in main) Co-Authored-By: Paperclip --- apps/api/src/index.ts | 17 ++++- apps/api/src/lib/auth.ts | 20 ++++-- apps/api/src/routes/book.ts | 7 -- apps/api/src/routes/calendar.ts | 15 +++- apps/api/src/routes/setup.ts | 121 ++++++++++++++++++++++---------- 5 files changed, 129 insertions(+), 51 deletions(-) diff --git a/apps/api/src/index.ts b/apps/api/src/index.ts index 7f49e20..aa5e1db 100644 --- a/apps/api/src/index.ts +++ b/apps/api/src/index.ts @@ -33,11 +33,26 @@ import { webhooksRouter } from "./routes/stripe-webhooks.js"; const app = new Hono(); // Global middleware +const TRUSTED_ORIGINS = (process.env.CORS_ORIGIN ?? "http://localhost:5173") + .split(",") + .map((o) => o.trim()); + +const ALLOWED_ORIGIN = process.env.CORS_ORIGIN ?? "http://localhost:5173"; + app.use("*", logger()); app.use( "/api/*", cors({ - origin: process.env.CORS_ORIGIN ?? "http://localhost:5173", + origin: (origin, ctx) => { + if (!origin) { + return ALLOWED_ORIGIN; + } + if (TRUSTED_ORIGINS.includes(origin)) { + return origin; + } + ctx.status(403); + return null; + }, credentials: true, }) ); diff --git a/apps/api/src/lib/auth.ts b/apps/api/src/lib/auth.ts index 31ffd29..c961d9e 100644 --- a/apps/api/src/lib/auth.ts +++ b/apps/api/src/lib/auth.ts @@ -89,7 +89,7 @@ export async function initAuth(): Promise { 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, @@ -177,9 +177,9 @@ export async function initAuth(): Promise { const hasGoogle = !!(process.env.GOOGLE_CLIENT_ID && process.env.GOOGLE_CLIENT_SECRET); const hasGitHub = !!(process.env.GITHUB_CLIENT_ID && process.env.GITHUB_CLIENT_SECRET); - // Fetch OIDC discovery document to derive canonical provider URLs. - // Replace the host of token/userinfo endpoints with internalBaseUrl when set, - // while keeping authorizationUrl public for browser redirects. + const issuerUrlObj = new URL(providerConfig.issuerUrl); + const issuerHostname = issuerUrlObj.hostname; + const discoveryUrlStr = `${providerConfig.issuerUrl}/.well-known/openid-configuration`; let oidcConfig: Record = {}; try { @@ -203,6 +203,18 @@ export async function initAuth(): Promise { const tokenUrl = discovery.token_endpoint; const userInfoUrl = discovery.userinfo_endpoint; if (authzUrl && tokenUrl && userInfoUrl) { + const authzUrlObj = new URL(authzUrl); + const tokenUrlObj = new URL(tokenUrl); + const userInfoUrlObj = new URL(userInfoUrl); + if ( + authzUrlObj.hostname !== issuerHostname || + tokenUrlObj.hostname !== issuerHostname || + userInfoUrlObj.hostname !== issuerHostname + ) { + throw new Error( + `[FATAL] OIDC discovery URL hostname mismatch: expected '${issuerHostname}' but got '${authzUrlObj.hostname}', '${tokenUrlObj.hostname}', or '${userInfoUrlObj.hostname}'. This may indicate a man-in-the-middle attack.` + ); + } oidcConfig = { authorizationUrl: authzUrl, tokenUrl: providerConfig.internalBaseUrl diff --git a/apps/api/src/routes/book.ts b/apps/api/src/routes/book.ts index d82823f..aab3399 100644 --- a/apps/api/src/routes/book.ts +++ b/apps/api/src/routes/book.ts @@ -265,17 +265,14 @@ bookRouter.get("/confirm/:token", async (c) => { 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`); } @@ -309,18 +306,14 @@ bookRouter.get("/cancel/:token", async (c) => { 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({ diff --git a/apps/api/src/routes/calendar.ts b/apps/api/src/routes/calendar.ts index a85568f..ff45842 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,18 @@ calendarRouter.get("/:staffId.ics", async (c) => { .where(eq(staff.id, staffId)) .limit(1); - if (!staffMember || staffMember.icalToken !== token) { + if (!staffMember || !staffMember.icalToken) { + return c.text("Unauthorized", 401); + } + + const storedToken = staffMember.icalToken; + const incomingToken = token; + const storedBuf = Buffer.from(storedToken, "utf8"); + const incomingBuf = Buffer.from(incomingToken, "utf8"); + if ( + storedBuf.length !== incomingBuf.length || + !timingSafeEqual(storedBuf, incomingBuf) + ) { return c.text("Unauthorized", 401); } diff --git a/apps/api/src/routes/setup.ts b/apps/api/src/routes/setup.ts index a614b5c..a84e61d 100644 --- a/apps/api/src/routes/setup.ts +++ b/apps/api/src/routes/setup.ts @@ -4,6 +4,24 @@ import { z } from "zod/v3"; import { and, eq, getDb, sql, staff, businessSettings, authProviderConfig, encryptSecret } from "@groombook/db"; import type { AppEnv } from "../middleware/rbac.js"; +const RATE_LIMIT_WINDOW_MS = 60_000; +const RATE_LIMIT_MAX = 10; +const rateLimitMap = new Map(); + +function rateLimitByIp(ip: string): { allowed: boolean; remaining: number } { + const now = Date.now(); + const entry = rateLimitMap.get(ip); + if (!entry || now > entry.resetAt) { + rateLimitMap.set(ip, { count: 1, resetAt: now + RATE_LIMIT_WINDOW_MS }); + return { allowed: true, remaining: RATE_LIMIT_MAX - 1 }; + } + if (entry.count >= RATE_LIMIT_MAX) { + return { allowed: false, remaining: 0 }; + } + entry.count++; + return { allowed: true, remaining: RATE_LIMIT_MAX - entry.count }; +} + export const setupRouter = new Hono(); // GET /api/setup/status — public (no auth), returns whether setup is needed @@ -185,52 +203,74 @@ 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"; + const { allowed, remaining } = rateLimitByIp(ip); + c.res.headers.set("x-rate-limit-remaining", String(remaining)); + if (!allowed) { + 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) - const [superUser] = await db - .select({ id: staff.id }) - .from(staff) - .where(eq(staff.isSuperUser, true)) - .limit(1); + let row: typeof authProviderConfig.$inferSelect; + try { + row = await db.transaction(async (tx) => { + const [superUser] = await tx + .select({ id: staff.id }) + .from(staff) + .where(eq(staff.isSuperUser, true)) + .limit(1); - if (superUser) { - // Setup already completed — lock this endpoint permanently - return c.json({ error: "Setup has already been completed. This endpoint is no longer available." }, 403); - } + if (superUser) { + throw Object.assign(new Error("setup-complete"), { code: 403 }); + } - // Guard: ensure no DB config already exists (should be redundant with status check but defensive) - const [existingConfig] = await db - .select({ id: authProviderConfig.id }) - .from(authProviderConfig) - .where(eq(authProviderConfig.enabled, true)) - .limit(1); + const [existingConfig] = await tx + .select({ id: authProviderConfig.id }) + .from(authProviderConfig) + .where(eq(authProviderConfig.enabled, true)) + .limit(1); - if (existingConfig) { - return c.json({ error: "Auth provider is already configured." }, 409); - } + if (existingConfig) { + throw Object.assign(new Error("config-exists"), { code: 409 }); + } - const body = authProviderBootstrapSchema.parse(await c.req.json()); + const body = authProviderBootstrapSchema.parse(await c.req.json()); - // Encrypt clientSecret before storing - const encryptedSecret = encryptSecret(body.clientSecret); + const encryptedSecret = encryptSecret(body.clientSecret); - const [row] = await db - .insert(authProviderConfig) - .values({ - providerId: body.providerId, - displayName: body.displayName, - issuerUrl: body.issuerUrl, - internalBaseUrl: body.internalBaseUrl ?? null, - clientId: body.clientId, - clientSecret: encryptedSecret, - scopes: body.scopes, - enabled: true, - }) - .returning(); + const [configRow] = await tx + .insert(authProviderConfig) + .values({ + providerId: body.providerId, + displayName: body.displayName, + issuerUrl: body.issuerUrl, + internalBaseUrl: body.internalBaseUrl ?? null, + clientId: body.clientId, + clientSecret: encryptedSecret, + scopes: body.scopes, + enabled: true, + }) + .returning(); - if (!row) { - return c.json({ error: "Failed to save auth provider configuration." }, 500); + if (!configRow) { + throw Object.assign(new Error("insert-failed"), { code: 500 }); + } + + return configRow; + }); + } catch (err: unknown) { + const e = err as Error & { code?: number }; + if (e.message === "setup-complete") { + return c.json({ error: "Setup has already been completed. This endpoint is no longer available." }, e.code as 403); + } + if (e.message === "config-exists") { + return c.json({ error: "Auth provider is already configured." }, e.code as 409); + } + if (e.message === "insert-failed") { + return c.json({ error: "Failed to save auth provider configuration." }, e.code as 500); + } + throw err; } return c.json({ @@ -254,6 +294,13 @@ 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"; + const { allowed, remaining } = rateLimitByIp(ip); + c.res.headers.set("x-rate-limit-remaining", String(remaining)); + if (!allowed) { + 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) From f7b8b7e66869816e46cd6ef61b37818825589acf Mon Sep 17 00:00:00 2001 From: Flea Flicker Date: Tue, 14 Apr 2026 23:12:41 +0000 Subject: [PATCH 2/3] fix(GRO-634): atomic confirmation token in book.ts, correct RBAC error message - Replace SELECT-then-UPDATE with atomic UPDATE ... WHERE token=? AND status='pending' RETURNING * to prevent confirmation token replay attacks (TOCTOU race condition) - Fix requireRoleOrSuperUser() error message: swap the conditional branches so 'Forbidden: super user privileges required' is returned when user lacks role, and 'Forbidden: role X is not permitted' when user is not superuser - Add 'and' mock export to confirmation.test.ts and rbac.test.ts for new query patterns - Update test expectations to match corrected error message semantics --- apps/api/src/__tests__/confirmation.test.ts | 1 + apps/api/src/__tests__/rbac.test.ts | 5 ++-- apps/api/src/middleware/rbac.ts | 6 ++--- apps/api/src/routes/book.ts | 28 ++++++++++++++++++--- 4 files changed, 31 insertions(+), 9 deletions(-) diff --git a/apps/api/src/__tests__/confirmation.test.ts b/apps/api/src/__tests__/confirmation.test.ts index 091268f..351a644 100644 --- a/apps/api/src/__tests__/confirmation.test.ts +++ b/apps/api/src/__tests__/confirmation.test.ts @@ -68,6 +68,7 @@ vi.mock("@groombook/db", () => { }), appointments, eq: () => ({}), + and: (...clauses: unknown[]) => ({}), }; }); diff --git a/apps/api/src/__tests__/rbac.test.ts b/apps/api/src/__tests__/rbac.test.ts index 7351c6a..f975316 100644 --- a/apps/api/src/__tests__/rbac.test.ts +++ b/apps/api/src/__tests__/rbac.test.ts @@ -78,6 +78,7 @@ vi.mock("@groombook/db", () => { }), staff, eq: vi.fn((_col: unknown, _val: unknown) => ({ col: _col, val: _val })), + and: vi.fn((..._clauses: unknown[]) => ({})), }; }); @@ -362,7 +363,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.*not permitted/i); }); it("blocks a non-super-user groomer from manager-only routes", async () => { @@ -370,7 +371,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.*not permitted/i); }); it("allows a manager with multiple allowed roles", async () => { 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 aab3399..26b7cae 100644 --- a/apps/api/src/routes/book.ts +++ b/apps/api/src/routes/book.ts @@ -277,14 +277,24 @@ bookRouter.get("/confirm/:token", async (c) => { return c.redirect(`${BASE_URL()}/booking/error`); } - await db + const updated = await db .update(appointments) .set({ confirmationStatus: "confirmed", confirmedAt: new Date(), updatedAt: new Date(), }) - .where(eq(appointments.id, appt.id)); + .where( + and( + eq(appointments.confirmationToken, token), + eq(appointments.confirmationStatus, "pending") + ) + ) + .returning(); + + if (updated.length === 0) { + return c.redirect(`${BASE_URL()}/booking/error`); + } return c.redirect(`${BASE_URL()}/booking/confirmed`); }); @@ -314,7 +324,7 @@ bookRouter.get("/cancel/:token", async (c) => { return c.redirect(`${BASE_URL()}/booking/error`); } - await db + const updated = await db .update(appointments) .set({ confirmationStatus: "cancelled", @@ -322,7 +332,17 @@ 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") + ) + ) + .returning(); + + if (updated.length === 0) { + return c.redirect(`${BASE_URL()}/booking/error`); + } return c.redirect(`${BASE_URL()}/booking/cancelled`); }); From 233e68769a0876eeeb775d8f3fdc96c0a1cc84e6 Mon Sep 17 00:00:00 2001 From: Flea Flicker Date: Tue, 14 Apr 2026 23:23:51 +0000 Subject: [PATCH 3/3] fix(GRO-634): rename unused 'clauses' param to _clauses in confirmation test Co-Authored-By: Paperclip --- apps/api/src/__tests__/confirmation.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/api/src/__tests__/confirmation.test.ts b/apps/api/src/__tests__/confirmation.test.ts index 351a644..aaa30c2 100644 --- a/apps/api/src/__tests__/confirmation.test.ts +++ b/apps/api/src/__tests__/confirmation.test.ts @@ -68,7 +68,7 @@ vi.mock("@groombook/db", () => { }), appointments, eq: () => ({}), - and: (...clauses: unknown[]) => ({}), + and: (..._clauses: unknown[]) => ({}), }; });