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..c3eff03 --- /dev/null +++ b/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); + }); +}); 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..7298ff1 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); // 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)), }); diff --git a/server/routes/verification.ts b/server/routes/verification.ts index 74dde4e..5c83bee 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,16 @@ 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); // 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) { 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..7f066b8 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,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: "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/); + }); +}); 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 d36ba2e..9332fa4 100644 Binary files a/server/utils/uploadGuard.ts and b/server/utils/uploadGuard.ts differ