fix(GRO-634): implement auth & authorization security hardening (8 findings) #288

Merged
scrubs-mcbarkley-ceo[bot] merged 4 commits from feature/gro-622-security-hardening into main 2026-04-15 07:00:24 +00:00
8 changed files with 160 additions and 60 deletions
@@ -68,6 +68,7 @@ vi.mock("@groombook/db", () => {
}),
appointments,
eq: () => ({}),
and: (..._clauses: unknown[]) => ({}),
};
});
+3 -2
View File
@@ -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 () => {
+16 -1
View File
@@ -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,
})
);
+16 -4
View File
@@ -89,7 +89,7 @@ export async function initAuth(): Promise<void> {
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<void> {
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<string, string> = {};
try {
@@ -203,6 +203,18 @@ export async function initAuth(): Promise<void> {
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
+3 -3
View File
@@ -166,9 +166,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
);
+24 -11
View File
@@ -268,29 +268,36 @@ 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`);
}
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`);
});
@@ -312,19 +319,15 @@ 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
const updated = await db
.update(appointments)
.set({
confirmationStatus: "cancelled",
@@ -332,7 +335,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`);
});
+13 -2
View File
@@ -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);
}
+84 -37
View File
@@ -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<string, { count: number; resetAt: number }>();
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<AppEnv>();
// 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)