Skip to content

fix(cli): read caption images through checked file descriptors - #3724

Merged
jrusso1020 merged 1 commit into
mainfrom
fix/security-caption-image-read
Sep 6, 2026
Merged

fix(cli): read caption images through checked file descriptors#3724
jrusso1020 merged 1 commit into
mainfrom
fix/security-caption-image-read

Conversation

@jrusso1020

Copy link
Copy Markdown
Collaborator

Capture captioning checked an image's size by pathname, then reopened that pathname to read it. A replacement between those operations could send different bytes to the vision provider. Open once in read-only mode, check size and read through the same descriptor, and close it before the provider request (including oversized files and failures).

Preserves the 4,000,000-byte inclusive limit, early oversized-image skip, provider payloads, batching, and failed-asset warnings. Addresses CodeQL alert #265.

Validation: all 214 capture tests, CLI typecheck, parser/core/lint builds, lint/format and signed commit hooks passed. New real-filesystem tests cover replacement after the size check (reproduces on main), both size-limit boundaries, and descriptor cleanup on stat/read failures. GitHub Actions and Windows verification pending.

@miguel-heygen miguel-heygen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pathname race is closed cleanly. contentExtractor.ts:457-465 opens once in read-only mode, checks and reads the same descriptor, and the finally closes it on the oversize return and on stat/read failures before captionOne can issue a provider request. The surrounding Promise.allSettled path preserves batching, sibling success, and the existing sanitized warning count.

The regression coverage is strong: contentExtractor.file-race.test.ts:72-103 distinguishes path replacement from descriptor identity, pins both sides of the inclusive 4,000,000-byte boundary, and proves closure after both failure sites. I ran all 214 capture tests plus changed-file lint/format locally; all pass. CI Build and Typecheck are green at this head; Windows, the full Test job, CLI smoke, and JavaScript CodeQL are still running and remain landing gates.

Non-blocking residual: contentExtractor.ts:460-462 pins the inode, not an immutable byte snapshot. A writer that grows the same inode after fstatSync can still make readFileSync(fd) return more than 4,000,000 bytes (I reproduced 4 checked bytes → 4,000,001 read bytes). That does not reopen the reported pathname-substitution bug or invalidate the CodeQL-recommended descriptor fix. If the provider limit must remain hard even against concurrent in-place writers, cap the descriptor read or check buffer.length before encoding in a follow-up.

Verdict: APPROVE
Reasoning: The exact head fixes #265 with one descriptor and correct cleanup while preserving stable-file behavior; the remaining same-inode mutation caveat is pre-existing and outside the reported replacement race.

— Magi

@jrusso1020
jrusso1020 merged commit 41551c7 into main Sep 6, 2026
49 checks passed
@jrusso1020
jrusso1020 deleted the fix/security-caption-image-read branch September 6, 2026 00:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants