fix(security): PDF magic-byte validation, per-email OTP limits, structured logging

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`.

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;
 

server/middleware/__tests__/otpEmailLimiter.test.ts

@@ -0,0 +1,74 @@
+/**
+ * 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.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: seedEmail });
+    }
+    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 () => {
+    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);
+  });
+});

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,

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); // 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);
@@ -167,6 +168,7 @@ export function registerAdminRoutes(app: Express): void {
       }
 
       safeFilePath = sanitizeUploadPath(req.file.path);
+      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);
@@ -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)),
       });

server/routes/verification.ts

@@ -13,7 +13,7 @@
 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,16 @@
     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); // codeql[js/path-injection] - safeFilePath is validated by sanitizeUploadPath
+    } catch (err) {
+      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.");
+    }
+
     let userId: string | undefined = betaUserId;
 
     if (!betaUser.ipExempt && !isAdminUser) {

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;
 }

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 <cwd>/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,32 @@ 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.each([
+    { name: "a spoofed mimetype but non-PDF content", filename: "fake.pdf", content: "<html><body>not a pdf</body></html>" },
+    { 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/);
+  });
+});

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 };
 }