From 908532be73e91c945ef53746da59fd40af5da311 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Mon, 27 Apr 2026 00:13:42 +0200 Subject: [PATCH] feat(security): SSRF strict IPv4, PAT logging redaction, threshold rate-limit, N+1 fix, pg-boss SIGTERM, blocking CI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Security hardening from 2026-04-26 audit: - SEC-002: isPublicUrl() now uses a strict IPv4 parser that rejects leading-zero forms like 010.0.0.1, which historically slipped past the h.startsWith('10.') check. Also covers CGNAT (100.64/10), IPv6 unique-local (fc00::/7), and additional metadata-service edge cases. Documented as input-time only (DNS rebinding still possible at fetch time). - SEC-004: GitHub PAT is now redacted from logged error bodies in publish-github.ts before flowing to Wide Events / Loki. Adds an escapeRegex helper for safe RegExp construction. - SEC-006: PUT /api/user/thresholds now goes through checkRateLimit (30 writes / 5 min, per-user) — audit-log was already there but unbounded. Makes audit-log enumeration unattractive. - PERF-001: insights/comprehensive route now batches medication-intake events into a single 'medicationId IN (...)' query and groups in memory instead of N+1 per medication. Latency was scaling poorly for users with many active meds. - REL-003: pg-boss now installs SIGTERM/SIGINT handlers that call boss.stop({ graceful: true, timeout: 30s }) so in-flight handlers drain on docker stop / coolify redeploy instead of being killed. - LOG-001: Replaced console.log/error in reminder-worker with a workerLog helper. Lifecycle errors still go to stderr (only audience outside any handler context); routine init messages are silent because pg-boss own events + Wide Events from handlers carry the state. - CI: ESLint and TypeScript checks are now blocking — continue-on-error removed. Tests already were. Required pre-existing 'any' types in api-handler.ts to be properly justified with eslint-disable comments (Next.js variadic handler signature; the bound type still constrains return to Promise). --- .github/workflows/security.yml | 2 - src/app/api/insights/comprehensive/route.ts | 31 +++++-- src/app/api/user/thresholds/route.ts | 12 +++ src/lib/api-handler.ts | 7 ++ src/lib/feedback/publish-github.ts | 18 ++++- src/lib/jobs/reminder-worker.ts | 53 ++++++++++-- src/lib/validations/notifications.ts | 89 ++++++++++++++++----- 7 files changed, 173 insertions(+), 39 deletions(-) diff --git a/.github/workflows/security.yml b/.github/workflows/security.yml index b6a2247d6..15cb6bdf9 100644 --- a/.github/workflows/security.yml +++ b/.github/workflows/security.yml @@ -30,11 +30,9 @@ jobs: - name: ESLint run: pnpm lint - continue-on-error: true - name: TypeScript strict check run: pnpm typecheck - continue-on-error: true - name: Unit tests run: pnpm test diff --git a/src/app/api/insights/comprehensive/route.ts b/src/app/api/insights/comprehensive/route.ts index 0fa7477b3..b999de3de 100644 --- a/src/app/api/insights/comprehensive/route.ts +++ b/src/app/api/insights/comprehensive/route.ts @@ -216,15 +216,30 @@ export const GET = apiHandler(async () => { const bpMedications = medications.filter( (med) => (categoryMap[med.id] ?? "OTHER") === "BLOOD_PRESSURE", ); + // Single round-trip for all medications instead of N+1: one query keyed + // on `medicationId IN (...)`, then group in memory. The previous loop + // hit Postgres once per medication, which scales poorly for users with + // many active meds. + const allEvents = medications.length + ? await prisma.medicationIntakeEvent.findMany({ + where: { + medicationId: { in: medications.map((m) => m.id) }, + userId, + scheduledFor: { gte: ninetyDaysAgo }, + }, + orderBy: { scheduledFor: "desc" }, + }) + : []; + + const eventsByMed = new Map(); + for (const ev of allEvents) { + const list = eventsByMed.get(ev.medicationId); + if (list) list.push(ev); + else eventsByMed.set(ev.medicationId, [ev]); + } + for (const med of medications) { - const events = await prisma.medicationIntakeEvent.findMany({ - where: { - medicationId: med.id, - userId, - scheduledFor: { gte: ninetyDaysAgo }, - }, - orderBy: { scheduledFor: "desc" }, - }); + const events = eventsByMed.get(med.id) ?? []; const mapped = events.map((e) => ({ takenAt: e.takenAt, skipped: e.skipped, diff --git a/src/app/api/user/thresholds/route.ts b/src/app/api/user/thresholds/route.ts index 86512637e..2d422ef8a 100644 --- a/src/app/api/user/thresholds/route.ts +++ b/src/app/api/user/thresholds/route.ts @@ -12,6 +12,7 @@ import { apiSuccess, apiError, safeJson, getClientIp } from "@/lib/api-response" import { annotate } from "@/lib/logging/context"; import { prisma } from "@/lib/db"; import { auditLog } from "@/lib/auth/audit"; +import { checkRateLimit } from "@/lib/rate-limit"; import { getAllEffectiveRanges, type ThresholdOverridesJson, @@ -59,6 +60,17 @@ export const PUT = apiHandler(async (request: NextRequest) => { const { user } = await requireAuth(); annotate({ action: { name: "user.thresholds.update" } }); + // 30 writes / 5 min — generous for legitimate UI edits, tight enough to + // make audit-log enumeration unattractive. Per-user, not per-IP. + const rl = await checkRateLimit( + `thresholds:update:${user.id}`, + 30, + 5 * 60 * 1000, + ); + if (!rl.allowed) { + return apiError("Too many requests, please slow down", 429); + } + const { data: body, error: jsonError } = await safeJson(request); if (jsonError) return jsonError; diff --git a/src/lib/api-handler.ts b/src/lib/api-handler.ts index a452b9123..900604e1b 100644 --- a/src/lib/api-handler.ts +++ b/src/lib/api-handler.ts @@ -26,9 +26,16 @@ export class HttpError extends Error { * No CSRF check — HealthLog does not use CSRF tokens. * Auth annotation happens in routes via requireAuth(). */ +// Next.js route handlers come in two shapes — `(request)` for static routes +// and `(request, { params })` for dynamic ones. The variadic generic is the +// only signature TS accepts that covers both at the call site. The `any[]` +// here is constrained by the bound (T must return Promise) so it +// does not loosen handler bodies — only their parameter list. +// eslint-disable-next-line @typescript-eslint/no-explicit-any export function apiHandler Promise>( handler: T, ): T { + // eslint-disable-next-line @typescript-eslint/no-explicit-any const wrapped = async (...args: any[]): Promise => { const request = args[0] as NextRequest; const url = new URL(request.url); diff --git a/src/lib/feedback/publish-github.ts b/src/lib/feedback/publish-github.ts index 1c740ade1..865f523c0 100644 --- a/src/lib/feedback/publish-github.ts +++ b/src/lib/feedback/publish-github.ts @@ -74,6 +74,15 @@ function sanitize(text: string, max = 5000): string { return text.replace(/```/g, "\\`\\`\\`").slice(0, max); } +/** + * Escape a string for safe inclusion in a `RegExp(..)` constructor — the + * standard "don't let an attacker turn input into regex metacharacters" + * helper. Used to redact secrets from logged error bodies. + */ +function escapeRegex(s: string): string { + return s.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); +} + /** * Publishes a feedback item as a GitHub Issue. Returns issue number + URL. * Throws GithubPublishError on API failure. @@ -146,7 +155,14 @@ export async function publishFeedbackToGithub( if (!res.ok) { const errBody = await res.text(); - getEvent()?.addWarning("GitHub issue creation failed: " + errBody.slice(0, 500)); + // Strip the GitHub PAT before logging — GitHub error responses can echo + // request headers in some failure modes, and Wide Events flow to Loki + // unredacted. Keep the response shape useful for debugging without + // leaking the credential. + const sanitisedErr = errBody + .replace(new RegExp(escapeRegex(config.token), "g"), "[REDACTED]") + .slice(0, 500); + getEvent()?.addWarning("GitHub issue creation failed: " + sanitisedErr); throw new GithubPublishError( `Failed to create GitHub issue (HTTP ${res.status})`, res.status === 401 || res.status === 403 ? res.status : 502, diff --git a/src/lib/jobs/reminder-worker.ts b/src/lib/jobs/reminder-worker.ts index 37adeba64..9036bc0af 100644 --- a/src/lib/jobs/reminder-worker.ts +++ b/src/lib/jobs/reminder-worker.ts @@ -925,25 +925,64 @@ async function handleDataBackup(jobs: Job[]) { }); } +/** + * Internal logger that prefers structured Wide-Event annotations when a + * worker context is active, and falls back to stderr only for true + * lifecycle events that fire outside any handler (init, fatal startup + * errors, shutdown). Avoids the historical pattern of `console.log` + * everywhere in this file, which polluted production stdout and was + * never queryable in Loki. + */ +function workerLog(level: "info" | "error", msg: string, err?: unknown): void { + if (level === "error") { + // Errors during worker init or shutdown happen outside any request + // context, so stderr is the only audience the operator has. + if (err !== undefined) console.error(`[pg-boss] ${msg}`, err); + else console.error(`[pg-boss] ${msg}`); + } + // info-level lifecycle messages are intentionally silent — pg-boss own + // events surface state, and Wide Events from handlers carry the work. +} + export async function startReminderWorker() { - console.log("[pg-boss] Initializing pg-boss with DATABASE_URL..."); if (!DATABASE_URL) { - console.error("[pg-boss] CRITICAL: DATABASE_URL is not set!"); + workerLog("error", "CRITICAL: DATABASE_URL is not set, refusing to start"); return; } const boss = new PgBoss(DATABASE_URL); boss.on("error", (error: unknown) => { - console.error("[pg-boss] Error:", error); + workerLog("error", "boss emitted error", error); recordError(); }); - console.log("[pg-boss] Connecting to database..."); await boss.start(); setGlobalBoss(boss); markWorkerStarted(); - console.log("[pg-boss] Connected and started"); + + // Graceful shutdown: drain in-flight jobs on SIGTERM/SIGINT (sent by + // Docker Compose `docker stop`, Kubernetes pod termination, Coolify + // redeploys). Without this, pending handlers were force-killed and could + // either be lost or replayed on next start. We only register the listeners + // once — re-entering startReminderWorker (e.g. on hot-reload in dev) is + // a no-op for the handlers because they capture `boss` by closure and the + // first signal stops everything. + let shutdownInProgress = false; + const onSignal = async (signal: "SIGTERM" | "SIGINT") => { + if (shutdownInProgress) return; + shutdownInProgress = true; + workerLog("error", `received ${signal}, draining boss`); + try { + // graceful=true waits for active handlers to finish, then closes the + // pg connection pool. timeout cap so a stuck handler can't block deploys. + await boss.stop({ graceful: true, timeout: 30_000 }); + } catch (err) { + workerLog("error", "boss.stop failed during shutdown", err); + } + }; + process.once("SIGTERM", () => void onSignal("SIGTERM")); + process.once("SIGINT", () => void onSignal("SIGINT")); // pg-boss v12 requires explicit queue creation before scheduling const allQueues = [ @@ -964,7 +1003,6 @@ export async function startReminderWorker() { for (const q of allQueues) { await boss.createQueue(q); } - console.log(`[pg-boss] Created ${allQueues.length} queues`); // Schedule recurring cron jobs const schedules: [string, string][] = [ @@ -984,7 +1022,6 @@ export async function startReminderWorker() { for (const [name, cron] of schedules) { await boss.schedule(name, cron, {}, { tz: "Europe/Berlin" }); } - console.log(`[pg-boss] Scheduled ${schedules.length} cron jobs`); // Register the handler await boss.work( @@ -1057,7 +1094,7 @@ if ( process.argv[1]?.endsWith("reminder-worker.js") ) { startReminderWorker().catch((err) => { - console.error("Failed to start reminder worker:", err); + workerLog("error", "Failed to start reminder worker", err); process.exit(1); }); } diff --git a/src/lib/validations/notifications.ts b/src/lib/validations/notifications.ts index 2471e61a3..b9cca2797 100644 --- a/src/lib/validations/notifications.ts +++ b/src/lib/validations/notifications.ts @@ -7,31 +7,80 @@ export const notificationPreferenceSchema = z.object({ enabled: z.boolean(), }); -// Block private/internal network URLs to prevent SSRF. Exported so other -// server-initiated egress (AI providers, webhooks) can reuse it. +/** + * Parse a dotted-quad IPv4 string into normalized octets. + * + * Returns null if the input is not exactly four octets, contains non-digit + * characters, has any octet outside 0–255, or contains leading zeros (which + * some `parseInt` paths historically allowed — bypassing prefix checks like + * `h.startsWith("10.")` because "010.x.x.x" still parses to 10). + */ +function parseIpv4Strict(host: string): [number, number, number, number] | null { + const parts = host.split("."); + if (parts.length !== 4) return null; + const octets: number[] = []; + for (const part of parts) { + if (!/^(0|[1-9][0-9]?|1[0-9]{2}|2[0-4][0-9]|25[0-5])$/.test(part)) { + return null; + } + octets.push(Number(part)); + } + return [octets[0], octets[1], octets[2], octets[3]]; +} + +/** + * Block private/internal network URLs to prevent SSRF. Exported so other + * server-initiated egress (AI providers, webhooks) can reuse it. + * + * Defense layers: + * 1. Protocol allowlist: only http(s). + * 2. Hostname denylist for unicast loopback / link-local / metadata services. + * 3. RFC1918 + CGNAT + 169.254/16 block via strict IPv4 parser + * (rejects leading zeros so "010.0.0.1" cannot bypass the "10." check). + * 4. IPv6 loopback / link-local block. + * + * NOTE: This is an *input-time* check on user-supplied URLs. It does not + * defeat DNS rebinding (where a public hostname later resolves to a private + * IP). For that, pin the resolved IP at fetch time or use an HTTP client + * that blocks redirects to private ranges. + */ export function isPublicUrl(url: string): boolean { try { const parsed = new URL(url); - if (parsed.protocol !== "https:" && parsed.protocol !== "http:") + if (parsed.protocol !== "https:" && parsed.protocol !== "http:") { return false; - const h = parsed.hostname; - if ( - h === "169.254.169.254" || - h === "127.0.0.1" || - h === "[::1]" || - h === "0.0.0.0" || - h === "localhost" || - h.startsWith("10.") || - h.startsWith("192.168.") || - h.endsWith(".internal") || - h.endsWith(".local") - ) - return false; - // 172.16.0.0/12 - if (h.startsWith("172.")) { - const second = parseInt(h.split(".")[1] ?? "0", 10); - if (second >= 16 && second <= 31) return false; } + const h = parsed.hostname.toLowerCase(); + + // Hostname denylist (literal strings — no IP parsing involved). + if ( + h === "localhost" || + h.endsWith(".internal") || + h.endsWith(".local") || + h.endsWith(".localhost") + ) { + return false; + } + + // IPv6 loopback / link-local / unique-local. URL.hostname strips brackets. + if (h === "::1" || h === "[::1]" || h.startsWith("fe80:") || h.startsWith("fc") || h.startsWith("fd")) { + return false; + } + + // Strict IPv4 parsing — rejects leading-zero forms like "010.0.0.1". + const ip = parseIpv4Strict(h); + if (ip) { + const [a, b] = ip; + if (a === 127) return false; // 127.0.0.0/8 loopback + if (a === 0) return false; // 0.0.0.0/8 reserved + if (a === 10) return false; // 10.0.0.0/8 RFC1918 + if (a === 169 && b === 254) return false; // 169.254.0.0/16 link-local + AWS metadata + if (a === 172 && b >= 16 && b <= 31) return false; // 172.16.0.0/12 RFC1918 + if (a === 192 && b === 168) return false; // 192.168.0.0/16 RFC1918 + if (a === 100 && b >= 64 && b <= 127) return false; // 100.64.0.0/10 CGNAT + // Any other valid IPv4 is treated as public. + } + return true; } catch { return false;