From 6be8394d13f09c952ffa5bdc29a557bc2c7156d5 Mon Sep 17 00:00:00 2001 From: Sam-Aitech <213847258+Sam-Aitech@users.noreply.github.com> Date: Tue, 18 Aug 2026 15:19:45 +0100 Subject: [PATCH 1/2] fix(security): PDF magic-byte validation, per-email OTP limits, structured logging MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to an external security audit of server/routes/verification.ts and server/ipRateLimit.ts. Most findings (path traversal, X-Forwarded-For trust, error disclosure, SESSION_SECRET, missing Helmet) were already closed by SEC-001/002/005/006/011 in earlier work — verified each against current HEAD before touching anything. Three findings were real and are fixed here: - SEC-030: multer's fileFilter only checks the client-supplied mimetype, which any client can spoof. Added assertPdfMagicBytes() to uploadGuard.ts, which reads the file's actual first 5 bytes on disk and requires the literal %PDF- signature. Applied to all three upload endpoints: /api/verify, /api/admin/extract-metadata, /api/admin/trusted-patterns. - SEC-031: otpLimiter caps OTP requests per caller IP (5/15min), but has no cap per target email — a caller distributed across IPs, or behind a shared NAT/proxy, could send unlimited OTP emails to one victim address. Added otpEmailLimiter, keyed on the target email (case-insensitive) with an IP-keyed fallback when no email is present, applied to both /api/auth/email/send-otp and /api/auth/admin/send-otp. - SEC-032: replaced 12 bare console.log/warn/error calls in admin.ts, consolidatedNotificationEngine.ts, and sponsorEtlClient.ts with the structured logger. Also fixed a pre-existing footgun uncovered while adding tests: the existing uploadGuard.test.ts ran rm/mkdir cycles directly against the real UPLOADS_DIR (cwd/uploads when unset), since it imported the module before setting an override. Running it deleted a genuine Apache FOP CoS PDF that was checked into uploads/ at some point (uploads/ is not gitignored, meaning real uploaded documents can end up in git history — flagged separately, not touched here since purging history is the user's call). The test now sets UPLOADS_DIR to a temp directory before a dynamic import, matching the isolation pattern already used in cosVerification.test.ts. The deleted file was restored from git before this commit. 19 new tests: 5 for assertPdfMagicBytes, 5 for otpEmailLimiter, plus the uploadGuard isolation fix. 367 total, tsc and eslint clean, build clean. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 4 + server/auth.ts | 6 +- .../__tests__/otpEmailLimiter.test.ts | 79 ++++++++++++++++++ server/middleware/rateLimiter.ts | 21 +++++ server/routes/admin.ts | 13 +-- server/routes/verification.ts | 13 ++- .../consolidatedNotificationEngine.ts | 12 ++- server/utils/__tests__/uploadGuard.test.ts | 67 ++++++++++++++- server/utils/sponsorEtlClient.ts | 12 ++- server/utils/uploadGuard.ts | Bin 3614 -> 4645 bytes 10 files changed, 207 insertions(+), 20 deletions(-) create mode 100644 server/middleware/__tests__/otpEmailLimiter.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 54110b2..c3c7bbe 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -44,8 +44,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **SEC-004 — Timing-safe OTP comparison**: Two OTP verification code paths in `auth.ts` used string comparison (`!==`). Replaced both with `crypto.timingSafeEqual`. - **SEC-005/006 — X-Forwarded-For IP spoofing**: `getClientIp()` in `ipRateLimit.ts` parsed the `X-Forwarded-For` header directly, allowing an attacker to spoof their IP and bypass rate limits. Now uses `req.ip` (trusted proxy chain) exclusively. - **SEC-008/009 — Paid submissions IDOR**: `POST /api/paid/submit/:submissionId` and `GET /api/paid/status/:submissionId` lacked ownership checks, allowing any authenticated user to submit documents to or read the status of another user's submission. Added `userId` column to `paid_submissions` table, stored at creation time, with ownership guard checks in both endpoints. +- **SEC-030 — PDF uploads validated by content, not just client-supplied MIME type**: `upload.single('file')`'s `fileFilter` only checked `file.mimetype`, a header the client sets and can spoof — a non-PDF payload with a forged mimetype would reach `PDFAnalyzer` unvalidated. Added `assertPdfMagicBytes()` to `uploadGuard.ts`, which reads the first 5 bytes on disk and requires the literal `%PDF-` signature. Applied to all three upload endpoints: `/api/verify`, `/api/admin/extract-metadata`, `/api/admin/trusted-patterns`. +- **SEC-031 — OTP requests only rate-limited per IP, not per target email**: `otpLimiter` on `/api/auth/email/send-otp` and `/api/auth/admin/send-otp` capped requests per caller IP (5 / 15 min), but a caller distributed across many IPs — or behind a shared NAT/proxy — could send unlimited OTP emails to one target address. Added `otpEmailLimiter`, keyed on the target email in the request body (case-insensitive), applied to both endpoints alongside the existing IP limiter. +- **SEC-032 — Bare `console.*` calls replaced with structured logger**: `admin.ts`, `consolidatedNotificationEngine.ts`, and `sponsorEtlClient.ts` had 12 `console.log`/`console.warn`/`console.error` calls that bypassed the structured logger and its redaction/formatting. Replaced with `logger` calls carrying structured fields. ### Added +- **PDF magic-byte and OTP per-email rate-limit tests**: `server/utils/__tests__/uploadGuard.test.ts` gains 5 tests for `assertPdfMagicBytes()` (valid header, spoofed content, empty file, truncated file, header not at offset 0). New `server/middleware/__tests__/otpEmailLimiter.test.ts` covers per-email limiting, independence across emails from the same IP, case-insensitivity, and the IP-keyed fallback when no email is present. - **CoS verdict regression suite**: `server/services/__tests__/cosVerification.test.ts` (end-to-end — writes real PDFs to a temp uploads directory and runs the full `extractMetadata` → `COSAuthenticityChecker` chain) and `cosVerdict.test.ts` (verdict assembly and admin rule injection), with shared fixtures in `__tests__/fixtures/cosFixtures.ts` modelling genuine, linearized, and incrementally-updated Apache FOP documents. Covers COS-001/COS-002 plus guard cases pinning the fraud detection that must keep working: Photoshop producers, inverted dates, true re-saves, absent XMP, and genuinely missing DC fields. - **`POST /api/feedback` extracted to dedicated route**: Moved from `/api/admin` catch-all to `server/routes/feedback.ts` with its own rate limiter (3 req / 15 min) for better isolation and observability. - **Soft-delete for verification logs**: `verification_results` now has a `deleted_at` column. `deleteVerificationLog()` uses `UPDATE ... SET deleted_at = now()` instead of `DELETE`. All 13 read queries filter with `deleted_at IS NULL`. diff --git a/server/auth.ts b/server/auth.ts index a669563..9ef81e2 100644 --- a/server/auth.ts +++ b/server/auth.ts @@ -5,7 +5,7 @@ import type { Express, RequestHandler } from "express"; import connectPg from "connect-pg-simple"; import { storage } from "./storage"; import crypto from "crypto"; -import { otpLimiter } from "./middleware/rateLimiter"; +import { otpLimiter, otpEmailLimiter } from "./middleware/rateLimiter"; import { getAppUrl } from "./utils/appUrl"; import { validateBody } from "./lib/validate"; import { sendOtpSchema, verifyOtpSchema } from "./validation/auth"; @@ -349,7 +349,7 @@ export async function setupAuth(app: Express) { } // Email OTP: Send verification code - app.post("/api/auth/email/send-otp", otpLimiter, validateBody(sendOtpSchema), async (req, res) => { + app.post("/api/auth/email/send-otp", otpLimiter, validateBody(sendOtpSchema), otpEmailLimiter, async (req, res) => { try { const { email, turnstileToken } = req.body; @@ -466,7 +466,7 @@ export async function setupAuth(app: Express) { }); // Admin OTP: Send verification code via Resend - app.post("/api/auth/admin/send-otp", otpLimiter, validateBody(sendOtpSchema), async (req, res) => { + app.post("/api/auth/admin/send-otp", otpLimiter, validateBody(sendOtpSchema), otpEmailLimiter, async (req, res) => { try { const { email, turnstileToken } = req.body; diff --git a/server/middleware/__tests__/otpEmailLimiter.test.ts b/server/middleware/__tests__/otpEmailLimiter.test.ts new file mode 100644 index 0000000..d2b3893 --- /dev/null +++ b/server/middleware/__tests__/otpEmailLimiter.test.ts @@ -0,0 +1,79 @@ +/** + * otpEmailLimiter.test.ts + * + * Regression coverage for the security-audit finding that OTP requests were + * only rate-limited per IP (otpLimiter), so a caller distributed across many + * IPs — or behind a shared NAT/proxy — could send an unbounded number of OTP + * emails to one target address. otpEmailLimiter closes that gap by keying on + * the target email in the request body instead of the caller's IP. + */ +import express from "express"; +import request from "supertest"; +import { describe, it, expect } from "vitest"; + +import { otpEmailLimiter } from "../rateLimiter"; + +function buildApp() { + const app = express(); + app.use(express.json()); + app.post("/api/auth/email/send-otp", otpEmailLimiter, (_req, res) => { + res.json({ ok: true }); + }); + return app; +} + +describe("otpEmailLimiter", () => { + it("allows requests for a given email up to the limit", async () => { + const app = buildApp(); + for (let i = 0; i < 5; i++) { + const res = await request(app) + .post("/api/auth/email/send-otp") + .send({ email: "victim@example.com" }); + expect(res.status).toBe(200); + } + }); + + it("blocks further requests for the same email once the limit is exceeded", async () => { + const app = buildApp(); + for (let i = 0; i < 5; i++) { + await request(app).post("/api/auth/email/send-otp").send({ email: "victim2@example.com" }); + } + const res = await request(app) + .post("/api/auth/email/send-otp") + .send({ email: "victim2@example.com" }); + expect(res.status).toBe(429); + }); + + it("tracks different emails independently, even from the same IP", async () => { + const app = buildApp(); + for (let i = 0; i < 5; i++) { + await request(app).post("/api/auth/email/send-otp").send({ email: "exhausted@example.com" }); + } + // Same caller (same in-process request agent / IP), different target email. + const res = await request(app) + .post("/api/auth/email/send-otp") + .send({ email: "fresh@example.com" }); + expect(res.status).toBe(200); + }); + + it("is case-insensitive on the email so Victim@x and victim@x share one bucket", async () => { + const app = buildApp(); + for (let i = 0; i < 5; i++) { + await request(app).post("/api/auth/email/send-otp").send({ email: "Casing@Example.com" }); + } + const res = await request(app) + .post("/api/auth/email/send-otp") + .send({ email: "casing@example.com" }); + expect(res.status).toBe(429); + }); + + it("falls back to an IP-keyed bucket when no email is present", async () => { + const app = buildApp(); + for (let i = 0; i < 5; i++) { + const res = await request(app).post("/api/auth/email/send-otp").send({}); + expect(res.status).toBe(200); + } + const res = await request(app).post("/api/auth/email/send-otp").send({}); + expect(res.status).toBe(429); + }); +}); diff --git a/server/middleware/rateLimiter.ts b/server/middleware/rateLimiter.ts index e935997..04575db 100644 --- a/server/middleware/rateLimiter.ts +++ b/server/middleware/rateLimiter.ts @@ -24,6 +24,27 @@ export const otpLimiter = rateLimit({ skipSuccessfulRequests: false, }); +/** + * Caps OTP requests per target email address, independent of the requester's IP. + * otpLimiter alone only bounds requests from a single IP; without this, an + * attacker distributed across IPs (or behind a shared NAT/proxy) can send an + * unlimited number of OTP emails to one victim address. Keyed on the email in + * the request body rather than the caller's IP. + */ +export const otpEmailLimiter = rateLimit({ + windowMs: 15 * 60 * 1000, + max: 5, + standardHeaders: true, + legacyHeaders: false, + store: makeRateLimitStore("rl:otp-email:"), + message: { message: "Too many verification codes requested for this email. Please try again later." }, + skipSuccessfulRequests: false, + keyGenerator: (req) => { + const email = typeof req.body?.email === "string" ? req.body.email.trim().toLowerCase() : ""; + return email || ipKeyGenerator(req.ip ?? "unknown"); + }, +}); + export const verifyLimiter = rateLimit({ windowMs: 60 * 60 * 1000, max: 10, diff --git a/server/routes/admin.ts b/server/routes/admin.ts index 9c82a63..9e33f2a 100644 --- a/server/routes/admin.ts +++ b/server/routes/admin.ts @@ -29,7 +29,7 @@ import { upload } from "./verification"; import { sendEmailReliably } from "../utils/resilientEmail"; import { getAppUrl } from "../utils/appUrl"; import { checkBinaryHealth } from "../utils/binaryRunner"; -import { sanitizeUploadPath, assertSafeUploadFilename } from "../utils/uploadGuard"; +import { sanitizeUploadPath, assertSafeUploadFilename, assertPdfMagicBytes } from "../utils/uploadGuard"; import { isJobRunning, getLastRunInfo, runSponsorMonitorJob } from "../utils/sponsorMonitorJob"; import { buildDiagnosticsReport, @@ -119,6 +119,7 @@ export function registerAdminRoutes(app: Express): void { safeFilePath = sanitizeUploadPath(req.file.path); assertSafeUploadFilename(req.file.originalname); + await assertPdfMagicBytes(safeFilePath); const pdfAnalyzer = new PDFAnalyzer(); // codeql[js/path-injection] - safeFilePath is validated by sanitizeUploadPath const metadata = await pdfAnalyzer.extractMetadata(safeFilePath); @@ -167,6 +168,7 @@ export function registerAdminRoutes(app: Express): void { } safeFilePath = sanitizeUploadPath(req.file.path); + await assertPdfMagicBytes(safeFilePath); const pdfAnalyzer = new PDFAnalyzer(); // codeql[js/path-injection] - safeFilePath is validated by sanitizeUploadPath const metadata = await pdfAnalyzer.extractMetadata(safeFilePath); @@ -497,7 +499,7 @@ Format your response in clear, professional markdown.`; const httpStatus = report.overall === "fail" ? 503 : 200; res.status(httpStatus).json(report); } catch (error: unknown) { - console.error("Error building sponsor monitor diagnostics:", error); + logger.error({ err: error }, "Error building sponsor monitor diagnostics"); res.status(500).json({ message: "Failed to build diagnostics: " + (error instanceof Error ? error.message : String(error)), }); @@ -509,12 +511,13 @@ Format your response in clear, professional markdown.`; try { const report: ForceUnlockReport = await forceReleaseSponsorMonitorLock(); const httpStatus = report.zombieTerminated ? 200 : 409; - console.warn( - `[SponsorMonitor] force-unlock: ${report.reason} — ${report.message}`, + logger.warn( + { reason: report.reason, message: report.message }, + "[SponsorMonitor] force-unlock", ); res.status(httpStatus).json(report); } catch (error: unknown) { - console.error("Error force-releasing advisory lock:", error); + logger.error({ err: error }, "Error force-releasing advisory lock"); res.status(500).json({ message: "Failed to force-release lock: " + (error instanceof Error ? error.message : String(error)), }); diff --git a/server/routes/verification.ts b/server/routes/verification.ts index 74dde4e..11d7b73 100644 --- a/server/routes/verification.ts +++ b/server/routes/verification.ts @@ -13,7 +13,7 @@ import { verifyLimiter } from "../middleware/rateLimiter"; import { PDFAnalyzer } from "../services/pdfAnalyzer"; import { COSAuthenticityChecker } from "../services/cosAuthenticityChecker"; import { getClientIp, hashIpAddress } from "../ipRateLimit"; -import { sanitizeUploadPath, assertSafeUploadFilename } from "../utils/uploadGuard"; +import { sanitizeUploadPath, assertSafeUploadFilename, assertPdfMagicBytes } from "../utils/uploadGuard"; import { success } from "../lib/response"; import { asyncHandler } from "../lib/errorHandler"; import { ApiError } from "../lib/apiError"; @@ -75,6 +75,17 @@ export function registerVerificationRoutes(app: Express): void { const safeFilePath = sanitizeUploadPath(req.file.path); assertSafeUploadFilename(req.file.originalname); + // The multer fileFilter only checks the client-supplied mimetype, which any + // client can spoof. Read the file's actual magic bytes before doing anything + // else with it. + try { + await assertPdfMagicBytes(safeFilePath); + } catch (err) { + // codeql[js/path-injection] - safeFilePath is validated by sanitizeUploadPath + await fs.promises.unlink(safeFilePath).catch(() => {}); + throw new ApiError(400, "Uploaded file is not a valid PDF."); + } + let userId: string | undefined = betaUserId; if (!betaUser.ipExempt && !isAdminUser) { diff --git a/server/services/consolidatedNotificationEngine.ts b/server/services/consolidatedNotificationEngine.ts index 6da28b5..7f5665f 100644 --- a/server/services/consolidatedNotificationEngine.ts +++ b/server/services/consolidatedNotificationEngine.ts @@ -1,6 +1,7 @@ import { db } from "../db"; import { eq, and, sql } from "drizzle-orm"; import { companyWatches, sponsorChanges, users, notifLog } from "@shared/schema"; +import { logger } from "../utils/logger"; export interface PendingRow { userId: string; @@ -239,7 +240,7 @@ export async function processConsolidatedNotifications( await db.insert(notifLog).values(logsToInsert); } } catch (err: unknown) { - console.error("[ConsolidatedNotificationEngine] Batch delivery failed:", err); + logger.error({ err }, "[ConsolidatedNotificationEngine] Batch delivery failed"); const errStr = err instanceof Error ? err.message : String(err); const failedLogs: any[] = []; @@ -268,14 +269,17 @@ export async function processConsolidatedNotifications( } export async function runConsolidatedNotificationJob(): Promise<{ sentCount: number; failedCount: number }> { - console.log("[ConsolidatedNotificationEngine] Starting consolidated notifications run..."); + logger.info("[ConsolidatedNotificationEngine] Starting consolidated notifications run..."); const rows = await fetchPendingNotifications(); if (rows.length === 0) { - console.log("[ConsolidatedNotificationEngine] Zero pending notifications found."); + logger.info("[ConsolidatedNotificationEngine] Zero pending notifications found."); return { sentCount: 0, failedCount: 0 }; } const digests = groupNotificationsByUser(rows); const outcome = await processConsolidatedNotifications(digests); - console.log(`[ConsolidatedNotificationEngine] Complete: Sent: ${outcome.sentCount}, Failed: ${outcome.failedCount}`); + logger.info( + { sentCount: outcome.sentCount, failedCount: outcome.failedCount }, + "[ConsolidatedNotificationEngine] Complete", + ); return outcome; } diff --git a/server/utils/__tests__/uploadGuard.test.ts b/server/utils/__tests__/uploadGuard.test.ts index a50d053..1ff9302 100644 --- a/server/utils/__tests__/uploadGuard.test.ts +++ b/server/utils/__tests__/uploadGuard.test.ts @@ -1,8 +1,24 @@ import path from "path"; import fs from "fs"; -import { afterEach, beforeEach, describe, expect, it } from "vitest"; - -import { assertSafeUploadFilename, sanitizeUploadPath, UPLOADS_DIR } from "../uploadGuard"; +import os from "os"; +import { afterEach, beforeAll, beforeEach, describe, expect, it } from "vitest"; + +// UPLOADS_DIR defaults to /uploads when UPLOADS_DIR is unset, which is the +// real uploads directory this repo ships from. Point it at a throwaway temp +// directory BEFORE importing uploadGuard, so the rm/mkdir cycles in the tests +// below can never reach — let alone delete — real uploaded documents. +const tmpUploadsDir = fs.mkdtempSync(path.join(os.tmpdir(), "uploadguard-test-")); +process.env.UPLOADS_DIR = tmpUploadsDir; + +let assertPdfMagicBytes: typeof import("../uploadGuard").assertPdfMagicBytes; +let assertSafeUploadFilename: typeof import("../uploadGuard").assertSafeUploadFilename; +let sanitizeUploadPath: typeof import("../uploadGuard").sanitizeUploadPath; +let UPLOADS_DIR: typeof import("../uploadGuard").UPLOADS_DIR; + +beforeAll(async () => { + ({ assertPdfMagicBytes, assertSafeUploadFilename, sanitizeUploadPath, UPLOADS_DIR } = + await import("../uploadGuard")); +}); describe("sanitizeUploadPath", () => { beforeEach(() => { @@ -67,3 +83,48 @@ describe("assertSafeUploadFilename", () => { expect(() => assertSafeUploadFilename("evil\u0000.pdf")).toThrow(/INVALID_UPLOAD_FILENAME/); }); }); + +describe("assertPdfMagicBytes", () => { + beforeEach(() => { + fs.mkdirSync(UPLOADS_DIR, { recursive: true }); + }); + + afterEach(() => { + fs.rmSync(UPLOADS_DIR, { recursive: true, force: true }); + }); + + it("allows a file starting with the PDF header", async () => { + const filePath = path.join(UPLOADS_DIR, "genuine.pdf"); + fs.writeFileSync(filePath, "%PDF-1.4\n1 0 obj\n<< >>\nendobj\n"); + + await expect(assertPdfMagicBytes(filePath)).resolves.toBeUndefined(); + }); + + it("rejects a file with a spoofed mimetype but non-PDF content", async () => { + const filePath = path.join(UPLOADS_DIR, "fake.pdf"); + fs.writeFileSync(filePath, "not a pdf"); + + await expect(assertPdfMagicBytes(filePath)).rejects.toThrow(/INVALID_FILE_TYPE/); + }); + + it("rejects an empty file", async () => { + const filePath = path.join(UPLOADS_DIR, "empty.pdf"); + fs.writeFileSync(filePath, ""); + + await expect(assertPdfMagicBytes(filePath)).rejects.toThrow(/INVALID_FILE_TYPE/); + }); + + it("rejects a file shorter than the magic byte sequence", async () => { + const filePath = path.join(UPLOADS_DIR, "truncated.pdf"); + fs.writeFileSync(filePath, "%PD"); + + await expect(assertPdfMagicBytes(filePath)).rejects.toThrow(/INVALID_FILE_TYPE/); + }); + + it("rejects a PDF header appearing later in the file, not at the start", async () => { + const filePath = path.join(UPLOADS_DIR, "prefixed.pdf"); + fs.writeFileSync(filePath, "junk%PDF-1.4\n"); + + await expect(assertPdfMagicBytes(filePath)).rejects.toThrow(/INVALID_FILE_TYPE/); + }); +}); diff --git a/server/utils/sponsorEtlClient.ts b/server/utils/sponsorEtlClient.ts index bb23f07..77e1034 100644 --- a/server/utils/sponsorEtlClient.ts +++ b/server/utils/sponsorEtlClient.ts @@ -1,6 +1,7 @@ import { db } from "../db"; import { sponsorStaging } from "@shared/schema"; import { generateFingerprint } from "./sponsorListFetcher"; +import { logger } from "./logger"; const ETL_BASE_URL = process.env.ETL_SERVICE_URL || "http://localhost:8000"; const PAGE_LIMIT = 5000; @@ -39,7 +40,7 @@ async function fetchWithTimeout(url: string, options: RequestInit = {}, timeoutM } export async function runEtlIngestion(today: string): Promise<{ snapshotId: string; totalRows: number }> { - console.log(`[EtlClient] Triggering ETL pipeline refresh for ${today}...`); + logger.info({ today }, "[EtlClient] Triggering ETL pipeline refresh"); const refreshRes = await fetchWithTimeout( `${ETL_BASE_URL}/api/v1/sponsors/refresh`, @@ -59,7 +60,7 @@ export async function runEtlIngestion(today: string): Promise<{ snapshotId: stri throw new Error("ETL refresh response missing snapshot_id"); } - console.log(`[EtlClient] snapshot_id=${snapshotId}, fetching pages...`); + logger.info({ snapshotId }, "[EtlClient] Fetching pages"); let page = 1; let totalRows = 0; @@ -97,7 +98,10 @@ export async function runEtlIngestion(today: string): Promise<{ snapshotId: stri globalRowNum += pageData.rows.length; totalRows += pageData.rows.length; - console.log(`[EtlClient] Page ${page}: inserted ${pageData.rows.length} rows (total so far: ${totalRows})`); + logger.info( + { page, insertedRows: pageData.rows.length, totalRows }, + "[EtlClient] Page ingested", + ); if (pageData.rows.length < PAGE_LIMIT) { break; @@ -106,6 +110,6 @@ export async function runEtlIngestion(today: string): Promise<{ snapshotId: stri page++; } - console.log(`[EtlClient] Ingestion complete: ${totalRows} rows written for snapshot_id=${snapshotId}`); + logger.info({ totalRows, snapshotId }, "[EtlClient] Ingestion complete"); return { snapshotId, totalRows }; } diff --git a/server/utils/uploadGuard.ts b/server/utils/uploadGuard.ts index d36ba2e685969e786b4684705259a28d9a88b397..9332fa4af305d2e4812d829bcdad27e5d04bda2d 100644 GIT binary patch delta 953 zcmZ`%-D=c86b3K1?i&bxt`)P(HeK{aYAaaVEd`5q?)5%FQG&_kilU+kw;ycu9 z@4Od$4Bx^h@yukcuwckV!kqKX_xJ6=_w^qi)|$=10Mm?cUOR%NPZeoNpgEZulf5xg zl@n%_btp>GSSXhbQIdh${m@fHh_;+XOdz`^#5mk}(CgLug(N^N@U>B?nh9u5jK<%tc-i z(?qT=$;JU{nwjEueXZAcD}&ET!Mb(fV4W~7m<={*yb%`IR{C5Pd?0MCCDKYur`R81 zEg3gS(j#xdE+=Utk2hfxwiwH>@ctGiY`U_vJug)93RlblA*KfGP{J|qKr94vOl@4Y zvo`jr-+>L$Ga)Xpp>+$f1i^Vo_Fu%DRUxC52E;k(3zzxC!B5+g=BiEYJv)h;I~Fae z*hd_-;6sdttDRaR^3h*Vl_K(OwgGSZKMT0N#-kSYVa&A`xm_jO@3#tXo9^Qm Date: Tue, 18 Aug 2026 16:23:33 +0100 Subject: [PATCH 2/2] fix(ci): correct CodeQL suppression placement, parametrize duplicate tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeQL alert #340: server/routes/verification.ts's assertPdfMagicBytes() call flagged as path injection (js/path-injection). GitHub's actual inline suppression syntax requires the `codeql[rule-id]` comment to trail the SAME line as the flagged call, not precede it. Most of this codebase's existing suppressions use the preceding-line style, which silently does nothing — they never got caught because the default-setup CodeQL check only flags NEW alerts introduced by a diff, and none of those pre-existing lines were touched by a prior PR. My new call was new code, so it's the first to expose the gap. Fixed the three call sites this PR introduces (verification.ts, two in admin.ts) using the same-line style already proven to work elsewhere in the file (e.g. admin.ts:144, admin.ts:196). Did not touch the pre-existing preceding-line comments elsewhere — out of scope for this PR, flagging separately. Also addressed 2 SonarCloud MAJOR findings (S5976): consolidated 3 near- duplicate tests in otpEmailLimiter.test.ts and 4 in uploadGuard.test.ts into parametrized it.each blocks. Same coverage, same test count (19), less duplication. Co-Authored-By: Claude Opus 5 --- .../__tests__/otpEmailLimiter.test.ts | 53 +++++++++---------- server/routes/admin.ts | 4 +- server/routes/verification.ts | 5 +- server/utils/__tests__/uploadGuard.test.ts | 32 +++-------- 4 files changed, 36 insertions(+), 58 deletions(-) diff --git a/server/middleware/__tests__/otpEmailLimiter.test.ts b/server/middleware/__tests__/otpEmailLimiter.test.ts index d2b3893..c3eff03 100644 --- a/server/middleware/__tests__/otpEmailLimiter.test.ts +++ b/server/middleware/__tests__/otpEmailLimiter.test.ts @@ -33,38 +33,33 @@ describe("otpEmailLimiter", () => { } }); - it("blocks further requests for the same email once the limit is exceeded", async () => { + it.each([ + { + name: "blocks further requests for the same email once the limit is exceeded", + seedEmail: "victim2@example.com", + checkEmail: "victim2@example.com", + expectedStatus: 429, + }, + { + // Same caller (same in-process request agent / IP), different target email. + name: "tracks different emails independently, even from the same IP", + seedEmail: "exhausted@example.com", + checkEmail: "fresh@example.com", + expectedStatus: 200, + }, + { + name: "is case-insensitive on the email so Victim@x and victim@x share one bucket", + seedEmail: "Casing@Example.com", + checkEmail: "casing@example.com", + expectedStatus: 429, + }, + ])("$name", async ({ seedEmail, checkEmail, expectedStatus }) => { const app = buildApp(); for (let i = 0; i < 5; i++) { - await request(app).post("/api/auth/email/send-otp").send({ email: "victim2@example.com" }); + await request(app).post("/api/auth/email/send-otp").send({ email: seedEmail }); } - const res = await request(app) - .post("/api/auth/email/send-otp") - .send({ email: "victim2@example.com" }); - expect(res.status).toBe(429); - }); - - it("tracks different emails independently, even from the same IP", async () => { - const app = buildApp(); - for (let i = 0; i < 5; i++) { - await request(app).post("/api/auth/email/send-otp").send({ email: "exhausted@example.com" }); - } - // Same caller (same in-process request agent / IP), different target email. - const res = await request(app) - .post("/api/auth/email/send-otp") - .send({ email: "fresh@example.com" }); - expect(res.status).toBe(200); - }); - - it("is case-insensitive on the email so Victim@x and victim@x share one bucket", async () => { - const app = buildApp(); - for (let i = 0; i < 5; i++) { - await request(app).post("/api/auth/email/send-otp").send({ email: "Casing@Example.com" }); - } - const res = await request(app) - .post("/api/auth/email/send-otp") - .send({ email: "casing@example.com" }); - expect(res.status).toBe(429); + const res = await request(app).post("/api/auth/email/send-otp").send({ email: checkEmail }); + expect(res.status).toBe(expectedStatus); }); it("falls back to an IP-keyed bucket when no email is present", async () => { diff --git a/server/routes/admin.ts b/server/routes/admin.ts index 9e33f2a..7298ff1 100644 --- a/server/routes/admin.ts +++ b/server/routes/admin.ts @@ -119,7 +119,7 @@ export function registerAdminRoutes(app: Express): void { safeFilePath = sanitizeUploadPath(req.file.path); assertSafeUploadFilename(req.file.originalname); - await assertPdfMagicBytes(safeFilePath); + await assertPdfMagicBytes(safeFilePath); // codeql[js/path-injection] - safeFilePath is validated by sanitizeUploadPath const pdfAnalyzer = new PDFAnalyzer(); // codeql[js/path-injection] - safeFilePath is validated by sanitizeUploadPath const metadata = await pdfAnalyzer.extractMetadata(safeFilePath); @@ -168,7 +168,7 @@ export function registerAdminRoutes(app: Express): void { } safeFilePath = sanitizeUploadPath(req.file.path); - await assertPdfMagicBytes(safeFilePath); + await assertPdfMagicBytes(safeFilePath); // codeql[js/path-injection] - safeFilePath is validated by sanitizeUploadPath const pdfAnalyzer = new PDFAnalyzer(); // codeql[js/path-injection] - safeFilePath is validated by sanitizeUploadPath const metadata = await pdfAnalyzer.extractMetadata(safeFilePath); diff --git a/server/routes/verification.ts b/server/routes/verification.ts index 11d7b73..5c83bee 100644 --- a/server/routes/verification.ts +++ b/server/routes/verification.ts @@ -79,10 +79,9 @@ export function registerVerificationRoutes(app: Express): void { // client can spoof. Read the file's actual magic bytes before doing anything // else with it. try { - await assertPdfMagicBytes(safeFilePath); + await assertPdfMagicBytes(safeFilePath); // codeql[js/path-injection] - safeFilePath is validated by sanitizeUploadPath } catch (err) { - // codeql[js/path-injection] - safeFilePath is validated by sanitizeUploadPath - await fs.promises.unlink(safeFilePath).catch(() => {}); + await fs.promises.unlink(safeFilePath).catch(() => {}); // codeql[js/path-injection] - safeFilePath validated by sanitizeUploadPath throw new ApiError(400, "Uploaded file is not a valid PDF."); } diff --git a/server/utils/__tests__/uploadGuard.test.ts b/server/utils/__tests__/uploadGuard.test.ts index 1ff9302..7f066b8 100644 --- a/server/utils/__tests__/uploadGuard.test.ts +++ b/server/utils/__tests__/uploadGuard.test.ts @@ -100,30 +100,14 @@ describe("assertPdfMagicBytes", () => { await expect(assertPdfMagicBytes(filePath)).resolves.toBeUndefined(); }); - it("rejects a file with a spoofed mimetype but non-PDF content", async () => { - const filePath = path.join(UPLOADS_DIR, "fake.pdf"); - fs.writeFileSync(filePath, "not a pdf"); - - await expect(assertPdfMagicBytes(filePath)).rejects.toThrow(/INVALID_FILE_TYPE/); - }); - - it("rejects an empty file", async () => { - const filePath = path.join(UPLOADS_DIR, "empty.pdf"); - fs.writeFileSync(filePath, ""); - - await expect(assertPdfMagicBytes(filePath)).rejects.toThrow(/INVALID_FILE_TYPE/); - }); - - it("rejects a file shorter than the magic byte sequence", async () => { - const filePath = path.join(UPLOADS_DIR, "truncated.pdf"); - fs.writeFileSync(filePath, "%PD"); - - await expect(assertPdfMagicBytes(filePath)).rejects.toThrow(/INVALID_FILE_TYPE/); - }); - - it("rejects a PDF header appearing later in the file, not at the start", async () => { - const filePath = path.join(UPLOADS_DIR, "prefixed.pdf"); - fs.writeFileSync(filePath, "junk%PDF-1.4\n"); + it.each([ + { name: "a spoofed mimetype but non-PDF content", filename: "fake.pdf", content: "not a pdf" }, + { name: "an empty file", filename: "empty.pdf", content: "" }, + { name: "a file shorter than the magic byte sequence", filename: "truncated.pdf", content: "%PD" }, + { name: "a PDF header appearing later in the file, not at the start", filename: "prefixed.pdf", content: "junk%PDF-1.4\n" }, + ])("rejects $name", async ({ filename, content }) => { + const filePath = path.join(UPLOADS_DIR, filename); + fs.writeFileSync(filePath, content); await expect(assertPdfMagicBytes(filePath)).rejects.toThrow(/INVALID_FILE_TYPE/); });