Add file size validation to OCR extractors and attachment upload - #1724
Add file size validation to OCR extractors and attachment upload#1724selta-goodparty wants to merge 11 commits into
Conversation
Enforces actual S3 object size against declared size in completeUpload and adds ZIP central directory inspection in the DOCX extractor to reject archives whose total uncompressed size exceeds 100 MB before mammoth/jszip decompresses them. Also adds buffer size guards to both DOCX and PDF extractors as defense in depth. https://claude.ai/code/session_01TSQLE4BhPu4sBT1zUupAnr
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
![delegate-reviewer[bot]](https://avatars.githubusercontent.com/in/3615711?s=60&v=4){kind=small}
Adds 4 tests for S3Service.getObjectSize (success, NoSuchKey, HTTP 404, rethrow) and 5 tests for DocxOcrExtractor covering buffer size limit, invalid archive detection, decompression size limit, missing object, and valid passthrough. https://claude.ai/code/session_01TSQLE4BhPu4sBT1zUupAnr
The integration test mocked the old objectExists call; now mocks getObjectSize to match the updated completeUpload flow that verifies actual S3 object size. https://claude.ai/code/session_01TSQLE4BhPu4sBT1zUupAnr
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||
📊 Overall Test Coverage59.90% (average of lines, statements, functions, and branches) |
|
@goodparty_org/contracts RC published: |
Preview EnvironmentYour preview environment is ready!
|
|
delegate review |
| if (bytes.length > MAX_BUFFER_BYTES) { | ||
| throw new BadRequestException('attachment_too_large') | ||
| } |
There was a problem hiding this comment.
The MAX_BUFFER_BYTES guard and the missing-object path in PdfOcrExtractor.extract have no unit test coverage. docx.extractor.test.ts establishes the repo pattern: each extractor gets a direct-instantiation test file covering the not-found, oversized-buffer, and happy-path cases. ocr.service.test.ts mocks this extractor entirely, so these branches are never exercised and could regress silently.\n\nAdd src/ocr/tests/pdf.extractor.test.ts with at minimum: (1) s3.getFileBytes returns undefined → NotFoundException; (2) buffer length exceeds MAX_BUFFER_BYTES → BadRequestException('attachment_too_large'); (3) valid small buffer → calls pdf-parse and returns the text. Mock pdf-parse with vi.mock as mammoth is mocked in the docx test.\n\n
| .spyOn(s3, 'getSignedUrlForViewing') | ||
| .mockResolvedValue('https://s3.example/download-url'), | ||
| exists: vi.spyOn(s3, 'objectExists').mockResolvedValue(true), | ||
| exists: vi.spyOn(s3, 'getObjectSize').mockResolvedValue(1_500_000), |
There was a problem hiding this comment.
The completeUpload tests have no case for the second branch of the size guard (actualSize > attachment.sizeBytes when actualSize is still under ATTACHMENT_MAX_BYTES). This mock returns 1_500_000 while size_bytes is also 1_500_000 — equal values, so the declared-vs-actual comparison is never proven to fire.\n\nAdd a test that mocks getObjectSize to return a value larger than the presigned size_bytes but below 20 MB, and asserts a 400 with upload_size_exceeded.\n\n
Adds pdf.extractor.test.ts covering missing object, oversized buffer, and valid extraction paths. Adds a completeUpload test asserting 400 when the actual S3 object size exceeds the declared size_bytes. https://claude.ai/code/session_01TSQLE4BhPu4sBT1zUupAnr
📊 Overall Test Coverage61.25% (average of lines, statements, functions, and branches) |
|
@goodparty_org/contracts RC published: |
|
delegate review |
ZIP64 archives use 0xFFFFFFFF sentinels in the 32-bit EOCD fields. The decompression size check now returns 0 (skips) when ZIP64 sentinels are detected in the CD offset/size, and skips individual entries with sentinel uncompressed sizes. The 20 MB buffer limit still protects against oversized payloads. https://claude.ai/code/session_01TSQLE4BhPu4sBT1zUupAnr
📊 Overall Test Coverage61.22% (average of lines, statements, functions, and branches) |
|
@goodparty_org/contracts RC published: |
|
delegate review |
ZIP64 archives bypass the decompression size check since their 32-bit EOCD fields are sentinels. DOCX files never legitimately need ZIP64, so reject them outright instead of skipping the guard. Also adds throwOnError: true to the sendMessage call in completeUpload so a queue failure surfaces as a 502 instead of silently dropping the OCR job. https://claude.ai/code/session_01TSQLE4BhPu4sBT1zUupAnr
Asserts completeUpload passes throwOnError:true to the OCR producer and adds a case proving a producer failure surfaces (no silent ack). Drops an unnecessary `as never` cast and restores the DocxOcrExtractor doc comment removed earlier. https://claude.ai/code/session_01TSQLE4BhPu4sBT1zUupAnr
develop added a unique constraint on elected_office.user_id. Both the annotation download ownership test and the polls cross-office test created a second elected office for the same user within one test, which now violates the constraint on the PR merge ref. Assign the foreign office to a separate user, mirroring the passing presign access test.
📊 Overall Test Coverage62.18% (average of lines, statements, functions, and branches) |
|
@goodparty_org/contracts RC published: |
|
delegate review |
| const nameLen = buf.readUInt16LE(pos + 28) | ||
| const extraLen = buf.readUInt16LE(pos + 30) | ||
| const commentLen = buf.readUInt16LE(pos + 32) | ||
| pos += 46 + nameLen + extraLen + commentLen |
There was a problem hiding this comment.
A crafted CD entry with an inflated commentLen (or nameLen/extraLen) can advance pos past cdEnd, causing the while-loop to exit before reading subsequent entries. If the large-uncompressed-size entry is placed last, its size is never added to total, and the ZIP-bomb guard silently passes. The attacker writes a multi-entry ZIP where entries 0..N-2 have small uncompressed sizes but entry N-1 has commentLen=65535, which jumps pos past cdEnd before entry N (with uncompressed=~4GB) is ever read. totalUncompressedSize returns a small value and mammoth decompresses the bomb.
No downstream guard catches this — the only decompression-size check is the comparison against MAX_DECOMPRESSED_BYTES after this function returns.
| const nameLen = buf.readUInt16LE(pos + 28) | |
| const extraLen = buf.readUInt16LE(pos + 30) | |
| const commentLen = buf.readUInt16LE(pos + 32) | |
| pos += 46 + nameLen + extraLen + commentLen | |
| const nameLen = buf.readUInt16LE(pos + 28) | |
| const extraLen = buf.readUInt16LE(pos + 30) | |
| const commentLen = buf.readUInt16LE(pos + 32) | |
| const next = pos + 46 + nameLen + extraLen + commentLen | |
| if (next > cdEnd) { | |
| throw new BadRequestException('attachment_invalid_archive') | |
| } | |
| pos = next |
A malformed CD entry whose name/extra/comment lengths sum past the CD boundary could cause the iterator to skip the final entries, letting a trailing zip-bomb entry evade the decompression-size check. Validate that the next entry offset stays within cdEnd before advancing.
…can-9aef26fc-vuln-1306931-lhwal # Conflicts: # src/annotations/tests/annotationAttachments.controller.test.ts # src/vendors/aws/services/s3.service.ts
📊 Overall Test Coverage55.07% (average of lines, statements, functions, and branches) |
|
delegate review |
There was a problem hiding this comment.
Advisory mode — this PR has had 5+ prior non-approval bot review rounds. Further blocking comments would be churn rather than signal. 4 concern(s) remain below for human reviewers; the bot will not block this PR again. Push more commits to retrigger the bot on a fresh head if needed.
src/ocr/extractors/docx.extractor.ts:27 — EOCD scan accepts false-positive signature from ZIP comment
The backward scan returns the first (highest-offset) EOCD signature match, but a ZIP comment that contains the four bytes 0x06054b50 will appear after the true EOCD in the file and be found first. When that false EOCD's cdOffset/cdSize point at a region with no valid CD entries, totalUncompressedSize returns 0. A 0-byte total passes the zip-bomb guard unconditionally, so a crafted DOCX can carry an arbitrarily large decompressed payload and bypass the check entirely before being handed to mammoth.
The ZIP spec disambiguates by requiring that the EOCD comment-length field (buf.readUInt16LE(i + 20)) equals buf.length - i - 22. Add that validation to the scan so only the structurally correct EOCD is accepted:
const findEocdOffset = (buf: Buffer): number => {
for (
let i = buf.length - EOCD_MIN_SIZE;
i >= Math.max(0, buf.length - 65557);
i--
) {
if (
buf.readUInt32LE(i) === EOCD_SIGNATURE &&
buf.readUInt16LE(i + 20) === buf.length - i - 22
)
return i
}
return -1
}
src/ocr/extractors/docx.extractor.ts:48 — Silent break on unexpected CD signature allows zip-bomb bypass
When buf.readUInt32LE(pos) !== CD_ENTRY_SIGNATURE the loop breaks silently, returning only the partial accumulated total. An attacker can craft a DOCX where the first few central-directory entries sum to just under 100 MB (passing the check), then embed a non-signature byte mid-CD so the loop exits early — while the actual local file entries that mammoth decompresses can be arbitrarily large. The MAX_DECOMPRESSED_BYTES guard is entirely defeated.
A non-matching signature mid-CD is unambiguous corruption; it should be rejected:
if (buf.readUInt32LE(pos) !== CD_ENTRY_SIGNATURE)
throw new BadRequestException('attachment_invalid_archive')
src/annotations/services/annotationAttachment.service.ts:203 — Zero-byte upload bypasses upload_not_received guard
S3 returns ContentLength: 0 for an empty object; head.contentLength === null is false for 0, so the check passes. The subsequent size comparisons (0 > ATTACHMENT_MAX_BYTES and 0 > attachment.sizeBytes) are also both false (since sizeBytes is at least 1 per the contract's .positive() constraint), so the OCR job is enqueued for a zero-byte file.
Fix: if (!head || !head.contentLength) {
src/ocr/tests/docx.extractor.test.ts:190-197 — Tautological ZIP64 test
buildZip64EocdBuffer sets 0xffffffff in both eocd+12 (cdSize) and eocd+16 (cdOffset), so totalUncompressedSize throws at the EOCD-level sentinel check and never enters the CD-entry loop. The uncompressed === ZIP64_SENTINEL branch has zero test coverage.
buildZipBuffer(0xffffffff) already builds a ZIP with valid EOCD fields and a CD entry whose uncompressed size is 0xffffffff, which exercises the intended code path. Replace the test fixture:
it('rejects ZIP64 archives as unsupported', async () => {
const zip64 = buildZipBuffer(0xffffffff)
const { extractor } = buildExtractor(zip64)
await expect(extractor.extract(input())).rejects.toThrow(
'attachment_unsupported_format',
)
})
- findEocdOffset now validates the comment-length field so a signature inside an archive comment cannot be mistaken for the real EOCD (which would zero out the size walk and bypass the bomb guard). - A bad central-directory entry signature mid-walk now throws instead of silently stopping, closing an early-termination bypass. - completeUpload rejects zero-byte objects (ContentLength 0 previously slipped past the null check). - Add tests for the comment false-positive, corrupt CD entry, and the per-entry ZIP64 size sentinel branch.
📊 Overall Test Coverage55.10% (average of lines, statements, functions, and branches) |
2 participants
Summary
Adds proactive file size validation to prevent decompression bombs and oversized uploads. The DOCX extractor now parses ZIP central directory headers to detect decompressed size before extraction, while both DOCX and PDF extractors enforce a 20 MB buffer limit. The attachment service validates actual S3 object size against declared size during upload completion.
Key Changes
DOCX extractor (
src/ocr/extractors/docx.extractor.ts):findEocdOffset()andtotalUncompressedSize()helpers to parse ZIP central directory and calculate total uncompressed size without decompressingMAX_BUFFER_BYTES(20 MB) andMAX_DECOMPRESSED_BYTES(100 MB) limits before calling mammothBadRequestExceptionwith specific error codes for invalid archives or size violationsPDF extractor (
src/ocr/extractors/pdf.extractor.ts):MAX_BUFFER_BYTES(20 MB) validation before PDF parsingBadRequestException('attachment_too_large')if buffer exceeds limitAttachment service (
src/annotations/services/annotationAttachmentService.ts):ATTACHMENT_MAX_BYTESconstant (20 MB)completeUpload()to call news3.getObjectSize()instead ofobjectExists()sizeBytesS3 service (
src/vendors/aws/services/s3.service.ts):getObjectSize()method that returnsContentLengthfromHeadObjectCommandundefinedif object doesn't exist (404), allowing caller to distinguish missing vs. oversizedImplementation Details
The DOCX validation uses ZIP format knowledge to avoid decompressing the entire file—it reads the End of Central Directory record and iterates central directory entries to sum uncompressed sizes. This allows early rejection of zip bombs before mammoth processes them.
All three validation points (declared size, buffer size, decompressed size) work together: the service gates upload completion, then extractors provide defense-in-depth before calling third-party libraries.
https://claude.ai/code/session_01TSQLE4BhPu4sBT1zUupAnr
Note
Medium Risk
Changes attachment upload gating and OCR input handling (resource exhaustion / zip bombs); logic is localized but mistakes could block legitimate uploads or leave presign without declared-size caps.
Overview
Tightens annotation attachment handling so uploads and OCR cannot pull unbounded bytes into the API or third-party parsers.
Upload completion now uses S3
HeadObjectContentLength(via newgetObjectSize) instead of a mere existence check. It rejects missing objects, objects over 20 MB, and objects larger than the declaredsizeBytes(upload_size_exceeded).PDF and DOCX OCR paths enforce a 20 MB in-memory cap before parsing. DOCX additionally walks the ZIP central directory (without decompressing) and blocks archives whose summed uncompressed size exceeds 100 MB, mitigating zip-bomb style abuse before mammoth runs.
Reviewed by Cursor Bugbot for commit 05c829f. Configure here.