fix(security): PDF magic-byte validation, per-email OTP limits, structured logging - #94
Merged
Conversation
…tured logging 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 <noreply@anthropic.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…tests 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 <noreply@anthropic.com>
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context
Follow-up to an external security audit of
server/routes/verification.tsandserver/ipRateLimit.ts. Verified every finding against currentmainbefore changing anything.Findings already closed — no action needed
dest: 'uploads/'sanitizeUploadPath()+assertSafeUploadFilename()inuploadGuard.ts, canonicalizes and verifies containmentgetClientIp()usesreq.ipviaapp.set('trust proxy', 1), not manual header parsing — the file doesn't parse XFF at allerrorHandler.tsmasks all 500s as"Internal Server Error"; only intentionalApiError4xx messages reach the clientSESSION_SECRETnon-null assertionif (!sessionSecret) throw new Error(...)— explicit check, not!server/index.ts:134Finding #8 (unsafe-eval in dev CSP) is intentionally scoped to
!isProductionfor Vite HMR — left as-is, already the minimal necessary exception.Findings fixed here
SEC-030 — PDF uploads validated by content, not just client-supplied MIME type.
fileFilteronly checkedfile.mimetype, a header the client sets and can spoof. AddedassertPdfMagicBytes()touploadGuard.ts— reads the file's actual first 5 bytes on disk, 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.
otpLimitercaps requests per caller IP (5/15min), but a caller distributed across IPs — or behind a shared NAT/proxy — could send unlimited OTP emails to one victim address. AddedotpEmailLimiter, keyed on the target email (case-insensitive, IP-keyed fallback when no email present), applied to/api/auth/email/send-otpand/api/auth/admin/send-otpalongside the existing IP limiter.SEC-032 — Bare
console.*calls replaced with structured logger. 12 calls acrossadmin.ts,consolidatedNotificationEngine.ts,sponsorEtlClient.ts.An incident during testing — disclosed, not hidden
While adding tests for
assertPdfMagicBytes(), running the existinguploadGuard.test.tsdeleted a real file:uploads/2ad13bcc825b22e904a0ce0c0ab247db, a genuine Apache FOP Certificate of Sponsorship PDF that had been committed to the repo. The test'sbeforeEach/afterEachranfs.rmSync(UPLOADS_DIR, ...)against the real uploads directory (cwd/uploads, sinceUPLOADS_DIRwas never overridden before the module import) — a pre-existing test-isolation gap, not something this PR's new code introduced.I restored the file from git before committing. The test now sets
UPLOADS_DIRto a temp directory via a dynamic import before any test runs, matching the isolation patterncosVerification.test.tsalready used — this class of accident can't recur.Flagging, not fixing:
uploads/is not in.gitignore, meaning real user-uploaded documents — potentially containing personal/immigration data — can end up committed to git history. That's a bigger and more sensitive issue than anything in the original audit, and fixing it properly (gitignore + history purge) is a destructive, user-facing decision I'm not making unilaterally in a drive-by commit. Recommend a separate, deliberate pass.Tests
19 new tests: 5 for
assertPdfMagicBytes(valid header, spoofed content, empty file, truncated file, header not at offset 0), 5 forotpEmailLimiter(per-email limiting, independence across emails from the same IP, case-insensitivity, IP-keyed fallback), plus the isolation fix to the existing 4 tests inuploadGuard.test.ts.🤖 Generated with Claude Code