From b4eda2d5539fe8d59e202e29e227fd17daea9e58 Mon Sep 17 00:00:00 2001 From: grabowski Date: Fri, 17 Apr 2026 14:18:28 +0700 Subject: [PATCH] Fix security audit findings: auth scoping, OIDC hardening, CSP, file download C3: Budget allocation now verifies project belongs to company M4: Expense approve/reject scoped by company via project join H2: OIDC cookies get secure flag on HTTPS H3: OIDC auto-link only when email_verified by provider H4: Content-Security-Policy + X-Content-Type-Options in hooks M7: SSRF favicon redirect depth capped at 3 M2: File downloads use attachment disposition (not inline) Co-Authored-By: Claude Opus 4.6 (1M context) --- src/hooks.server.ts | 10 +++++++++- src/lib/server/auth/oidc.ts | 2 +- src/lib/server/favicons/index.ts | 5 +++-- .../(app)/companies/[companyId]/budget/+page.server.ts | 9 +++++---- .../[docId]/versions/[versionId]/file/+server.ts | 5 +++-- .../companies/[companyId]/expenses/+page.server.ts | 8 ++++++-- src/routes/(auth)/oidc/+server.ts | 5 ++++- src/routes/(auth)/oidc/callback/+server.ts | 6 +++--- 8 files changed, 34 insertions(+), 16 deletions(-) diff --git a/src/hooks.server.ts b/src/hooks.server.ts index fd7472c..185235e 100644 --- a/src/hooks.server.ts +++ b/src/hooks.server.ts @@ -30,5 +30,13 @@ export const handle: Handle = async ({ event, resolve }) => { event.locals.session = null; } - return resolve(event); + const response = await resolve(event); + + response.headers.set( + 'Content-Security-Policy', + "default-src 'self'; script-src 'self'; style-src 'self' 'unsafe-inline'; img-src 'self' data:; font-src 'self'; connect-src 'self'; frame-ancestors 'none'" + ); + response.headers.set('X-Content-Type-Options', 'nosniff'); + + return response; }; diff --git a/src/lib/server/auth/oidc.ts b/src/lib/server/auth/oidc.ts index e8f0b87..ca09178 100644 --- a/src/lib/server/auth/oidc.ts +++ b/src/lib/server/auth/oidc.ts @@ -117,7 +117,7 @@ export async function exchangeCode( export async function getUserInfo( accessToken: string -): Promise<{ sub: string; email: string; name?: string }> { +): Promise<{ sub: string; email: string; name?: string; email_verified?: boolean }> { const config = await getOIDCConfig(); const res = await fetch(config.userinfoEndpoint, { diff --git a/src/lib/server/favicons/index.ts b/src/lib/server/favicons/index.ts index ce3896b..03bfa27 100644 --- a/src/lib/server/favicons/index.ts +++ b/src/lib/server/favicons/index.ts @@ -74,7 +74,8 @@ async function resolvePublicIp(hostname: string): Promise { return ips[0]; } -async function safeFetch(targetUrl: URL): Promise { +async function safeFetch(targetUrl: URL, depth = 0): Promise { + if (depth > 3) return null; if (targetUrl.protocol !== 'http:' && targetUrl.protocol !== 'https:') return null; try { await resolvePublicIp(targetUrl.hostname); @@ -109,7 +110,7 @@ async function safeFetch(targetUrl: URL): Promise { } catch { return null; } - return safeFetch(next); + return safeFetch(next, depth + 1); } return res; } catch { diff --git a/src/routes/(app)/companies/[companyId]/budget/+page.server.ts b/src/routes/(app)/companies/[companyId]/budget/+page.server.ts index 0f1cd8d..c9a2592 100644 --- a/src/routes/(app)/companies/[companyId]/budget/+page.server.ts +++ b/src/routes/(app)/companies/[companyId]/budget/+page.server.ts @@ -9,7 +9,7 @@ import { expenses, companyLog } from '$lib/server/db/schema.js'; -import { eq, sql } from 'drizzle-orm'; +import { and, eq, sql } from 'drizzle-orm'; import { requireCompanyRole } from '$lib/server/authorization.js'; import { logCompanyEvent } from '$lib/server/audit.js'; import { formatCurrency } from '$lib/utils/currency.js'; @@ -118,12 +118,13 @@ export const actions: Actions = { return fail(400, { error: 'Project and non-zero amount are required' }); } - // Get project name and company currency for the log + // Verify project belongs to this company const [project] = await db .select({ name: projects.name }) .from(projects) - .where(eq(projects.id, projectId)) + .where(and(eq(projects.id, projectId), eq(projects.companyId, params.companyId))) .limit(1); + if (!project) return fail(400, { error: 'Project not found in this company' }); const [company] = await db .select({ currency: companies.currency }) @@ -137,7 +138,7 @@ export const actions: Actions = { allocatedBudget: sql`${projects.allocatedBudget}::numeric + ${amount.toFixed(2)}::numeric`, updatedAt: new Date() }) - .where(eq(projects.id, projectId)); + .where(and(eq(projects.id, projectId), eq(projects.companyId, params.companyId))); await db.insert(budgetAllocations).values({ companyId: params.companyId, diff --git a/src/routes/(app)/companies/[companyId]/documents/[docId]/versions/[versionId]/file/+server.ts b/src/routes/(app)/companies/[companyId]/documents/[docId]/versions/[versionId]/file/+server.ts index 6cc4bfc..8b7cd17 100644 --- a/src/routes/(app)/companies/[companyId]/documents/[docId]/versions/[versionId]/file/+server.ts +++ b/src/routes/(app)/companies/[companyId]/documents/[docId]/versions/[versionId]/file/+server.ts @@ -41,8 +41,9 @@ export const GET: RequestHandler = async ({ locals, params }) => { return new Response(new Blob([buf as BlobPart], { type: row.mimeType }), { headers: { - 'Content-Disposition': `inline; filename="${safeName}"`, - 'Cache-Control': 'private, no-store' + 'Content-Disposition': `attachment; filename="${safeName}"`, + 'Cache-Control': 'private, no-store', + 'X-Content-Type-Options': 'nosniff' } }); }; diff --git a/src/routes/(app)/companies/[companyId]/expenses/+page.server.ts b/src/routes/(app)/companies/[companyId]/expenses/+page.server.ts index 98bb4fb..69f27a2 100644 --- a/src/routes/(app)/companies/[companyId]/expenses/+page.server.ts +++ b/src/routes/(app)/companies/[companyId]/expenses/+page.server.ts @@ -94,8 +94,10 @@ export const actions: Actions = { accountId: expenses.accountId }) .from(expenses) - .where(eq(expenses.id, expenseId)) + .innerJoin(projects, eq(expenses.projectId, projects.id)) + .where(and(eq(expenses.id, expenseId), eq(projects.companyId, params.companyId))) .limit(1); + if (!expense) return fail(404, { error: 'Expense not found' }); await db.transaction(async (tx) => { await tx @@ -137,8 +139,10 @@ export const actions: Actions = { const [expense] = await db .select({ title: expenses.title, amount: expenses.amount, currency: expenses.currency }) .from(expenses) - .where(eq(expenses.id, expenseId)) + .innerJoin(projects, eq(expenses.projectId, projects.id)) + .where(and(eq(expenses.id, expenseId), eq(projects.companyId, params.companyId))) .limit(1); + if (!expense) return fail(404, { error: 'Expense not found' }); await db.transaction(async (tx) => { await tx diff --git a/src/routes/(auth)/oidc/+server.ts b/src/routes/(auth)/oidc/+server.ts index 202bb7f..94737d6 100644 --- a/src/routes/(auth)/oidc/+server.ts +++ b/src/routes/(auth)/oidc/+server.ts @@ -7,7 +7,8 @@ import { isOIDCEnabled } from '$lib/server/auth/oidc.js'; -export const GET: RequestHandler = async ({ cookies }) => { +export const GET: RequestHandler = async ({ cookies, url: reqUrl }) => { + const isSecure = reqUrl.protocol === 'https:'; if (!isOIDCEnabled()) { redirect(302, '/login'); } @@ -17,6 +18,7 @@ export const GET: RequestHandler = async ({ cookies }) => { cookies.set('oidc_state', state, { httpOnly: true, + secure: isSecure, sameSite: 'lax', path: '/', maxAge: 600 // 10 minutes @@ -24,6 +26,7 @@ export const GET: RequestHandler = async ({ cookies }) => { cookies.set('oidc_code_verifier', codeVerifier, { httpOnly: true, + secure: isSecure, sameSite: 'lax', path: '/', maxAge: 600 diff --git a/src/routes/(auth)/oidc/callback/+server.ts b/src/routes/(auth)/oidc/callback/+server.ts index 4bf6267..843448d 100644 --- a/src/routes/(auth)/oidc/callback/+server.ts +++ b/src/routes/(auth)/oidc/callback/+server.ts @@ -45,8 +45,8 @@ export const GET: RequestHandler = async (event) => { .then((r) => r[0] ?? null); if (!user) { - // Check if a user with this email exists (link accounts) - if (userInfo.email) { + // Check if a user with this email exists — only auto-link if provider verified the email + if (userInfo.email && userInfo.email_verified) { user = await db .select() .from(users) @@ -55,7 +55,7 @@ export const GET: RequestHandler = async (event) => { .then((r) => r[0] ?? null); if (user) { - // Link OIDC identity to existing user + // Link OIDC identity to existing user (email verified by provider) await db .update(users) .set({