Uh oh!
There was an error while loading. Please reload this page.
fix(cli-upload): replace image-size with probe-image-size (PER-10427) - #2382
fix(cli-upload): replace image-size with probe-image-size (PER-10427)#2382aryanku-dev wants to merge 4 commits into
Conversation
…10427) `image-size` is archived upstream and carries three unfixable high-severity advisories (CVE-2025-71329, CVE-2025-71330), so `npm audit` fails for anyone who installs @percy/cli. There is no patched version to move to — every published release through 2.0.2 is affected — and 2.x would also undo the Node 14 support that #2301 pinned ~1.0.2 to keep. The command only ever accepts png, jpg and jpeg (ALLOWED_FILE_TYPES), so a general purpose image parser was always more surface than this needed. Reading the two formats we actually support is about eighty lines and removes the dependency outright. The advisories were reachable here, not just theoretical: `image-size` picks its parser from magic bytes while `percy upload` filters on extension, so an ICNS buffer named `.png` reached the ICNS parser and wedged the event loop — `percy upload` hung indefinitely and did not respond to SIGTERM. Such a file is now skipped with a log line. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CI enforces 100% coverage; image-size.js left branches 51, 56-59 and 70 unhit. Adds fixtures for the four segment-walk exits that had no test: walking off the chain into non-marker data, a standalone marker preceding the frame header, SOS/EOI reached before any frame, and a file that ends before the frame payload it announced. Also drops the optional chaining on the marker read. The loop bound `offset + 4 <= fileSize` already proves those four bytes exist, so the null arm was unreachable and could never be covered. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
aryanku-dev
left a comment
There was a problem hiding this comment.
Claude Code Review (automated) — 3 inline finding(s). Full report in the PR comment below. Verdict: Passed.
| // signature (8) + chunk length (4) + chunk type (4) + width (4) + height (4) | ||
| function pngSize(fd) { | ||
| let header = readAt(fd, 24, 0); | ||
| if (!header?.subarray(12, 16).equals(IHDR)) return null; |
There was a problem hiding this comment.
[Medium] Apple "fried" (CgBI) PNGs are now silently skipped
image-size@1.0.2 special-cased the CgBI chunk that Apple's PNG optimiser (Xcode / iOS asset-catalog exports) inserts before IHDR, which shifts IHDR from offset 12 to 28 and the dimensions from 16/20 to 32/36. This check requires IHDR at offset 12, so a fried PNG returns null — and with the new skip-don't-throw path in upload.js it is dropped from the upload with only an info-level log line. Previously it was sized and uploaded correctly.
No other format-coverage gap was found: progressive JPEGs, large-EXIF JPEGs and multi-chunk PNGs are all handled.
Suggestion: detect CgBI at offset 12 and re-read at the shifted offsets, e.g.
constCGBI=Buffer.from('CgBI','ascii');functionpngSize(fd){letheader=readAt(fd,40,0)??readAt(fd,24,0);if(!header)returnnull;if(header.length>=40&&header.subarray(12,16).equals(CGBI)){returnheader.subarray(28,32).equals(IHDR)
? {width: header.readUInt32BE(32),height: header.readUInt32BE(36)}
: null;}returnheader.subarray(12,16).equals(IHDR)
? {width: header.readUInt32BE(16),height: header.readUInt32BE(20)}
: null;}The ?? readAt(fd, 24, 0) fallback keeps the existing truncated-PNG specs returning null. Alternatively, accept the limitation and record it in the PR description / release notes.
Reviewer: stack-code-reviewer
| } else { | ||
| let absolutePath = path.resolve(args.dirname, relativePath); | ||
| let img = { relativePath, absolutePath, ...imageSize(absolutePath) }; | ||
| let size = imageSize(absolutePath); |
There was a problem hiding this comment.
[Low] Filesystem-level throws from imageSize() still abort the whole run
The continue below is correctly placed — it skips only this file, not the loop — and the new regression specs confirm the run completes and other images still upload.
But this call sits outside any try, so fs.openSync failures (permission denied, broken symlink, file deleted between the glob and the read) still throw and kill the entire upload. That is not a regression — image-size behaved the same — but it runs against this PR's stated intent that one bad file shouldn't take down the run.
Suggestion: decide explicitly — either wrap the call and treat FS errors as another skip, or note that FS-level failures remain intentionally fatal.
Reviewer: stack-code-reviewer
| .toEqual({ width: 320, height: 240 }); | ||
| }); | ||
| // CVE-2025-71330 / CVE-2025-71329 — the advisories that made `image-size` |
There was a problem hiding this comment.
[Low] This comment implies coverage the fixture does not exercise
The comment presents the ICNS fixture as pinning the zero-length-entry loop-termination behaviour, but imageSize() dispatches purely on the first 8 bytes — an ICNS buffer is rejected at the signature check before any segment-walking code runs.
The test is still valuable: it is a genuine dispatcher-safety regression, proving a .png-named ICNS no longer reaches a vulnerable parser. The comment just describes something else.
Suggestion: reword to describe what it actually proves.
Reviewer: stack-code-reviewer
aryanku-dev
commented
Aug 15, 2026
Claude Code PR ReviewPR:#2382 • Head:cb6ad32 • Reviewers: stack-code-reviewer SummaryRemoves the archived Review Table
Findings1. Apple "fried" (CgBI) PNGs are now silently skipped
2. Filesystem-level throws from
3. ICNS regression test's comment implies coverage it does not exercise
Notes on areas explicitly checked and found clean
Verdict: PASS |
…uilt-in parser Replaces the hand-rolled PNG/JPEG reader with `probe-image-size`, so the package depends on a maintained parser rather than one we own. `probe-image-size` has no ICNS, JXL or HEIF parser at all, and its ISOBMFF reader rejects a box smaller than its own header rather than advancing by it — the shape of every advisory that made `image-size` unfixable. Its tree is advisory-free, it is pure ES5 so Node 14 is unaffected, and 7.4.0 is current. Only the `sync.js` entrypoint is imported, which pulls in the buffer parsers and none of the http or stream machinery. The extension is required because the package publishes no `exports` map and this package is ESM. Two details worth noting: - Reads a bounded 512 KiB prefix, matching the `MaxBufferSize` that `image-size` applied, so no file that used to be readable becomes unreadable. - Gates on the reported type, because the parser reads about ten formats while `upload` accepts only png and jpeg. A GIF named `.png` is still skipped. `jpegStandaloneMarkerBeforeFrame` declared an 11-byte SOF0 segment inside a 16-byte buffer, one byte past the end. The previous reader returned dimensions anyway because it never checked the declared length against the bytes present; this one does, so the fixture is now a valid single-component frame header rather than a truncated one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Drops the `image-size.js` adapter, the `fixtures.js` module and the `image-size` unit specs. Those existed to hold and prove a hand-rolled PNG/JPEG parser; with `probe-image-size` doing the parsing, they covered upstream's segment walking rather than anything this package owns. What remains of the adapter is a bounded header read and a format gate, both short enough to live beside their only caller. The four image fixtures the end-to-end specs use move inline, and a spec covers the format gate directly: a GIF named `.png` clears the extension filter and parses fine, so only the gate keeps it out. The change to `percy upload` is now the dependency swap plus the skip path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
aryanku-dev
commented
Aug 21, 2026
Closing as superseded by #2390, which merged on 19 Aug. Both PRs replace
|
Fixes#2380 / PER-10427.
Root cause
npm auditfails for anyone installing@percy/clibecausepackages/cli-uploaddepends onimage-size, which has unpatched high-severity advisories:Both are CWE-835 (infinite loop) and share one shape: a zero-valued length field leaves the read offset unchanged, so the parser loops forever and blocks the event loop. Every version through 2.0.2 is affected and the upstream repo was archived on 2026-06-03, so there is nothing to upgrade to. Bumping was doubly blocked — #2301 pinned
~1.0.2to keep Node 14 support, whichimage-size2.x drops.This was reachable, not theoretical
image-sizeselects its parser from magic bytes, whileupload.jsfilters candidates by extension (ALLOWED_FILE_TYPES). A file namedscreenshot.pngwhose contents begin with the ICNS magic bytes therefore reached the ICNS parser.Verified against the shipped code — a 64-byte crafted
.pngin an upload directory:Because the loop blocks the event loop the process cannot handle the signal and needs
SIGKILL. This is a local CLI reading the user's own directory, so practical severity is well below the CVSS score — but it is a real hang, and it is what makes the audit finding non-dismissable.Fix
Replaces
image-sizewithprobe-image-size.It is immune to this bug class by construction, not just unreported: it has no ICNS, JXL or HEIF parser at all, and its ISOBMFF reader rejects a box smaller than its own header (
size < 8) rather than advancing by it. Its whole tree is advisory-free, it is pure ES5 so Node 14 is unaffected, and 7.4.0 is current.Only the
sync.jsentrypoint is imported — the buffer parsers, none of the http or stream machinery. The extension is required because the package publishes noexportsmap andcli-uploadis ESM.Two details in
upload.js:MaxBufferSizeimage-sizeapplied, so no file that used to be readable becomes unreadable and large images are not loaded whole just to read dimensions..pngclears the extension filter and parses fine — only the gate keeps it out.image-sizeand its transitivequeueare gone.probe-image-sizebrings 8 transitive packages (needle,iconv-lite,sax,safer-buffer,debug,ms,lodash.merge,stream-parser); sincecli-uploadships unbundled these install for end users even thoughsync.jsnever touches them.npm auditon that tree reports 0 vulnerabilities.Behaviour change
A file with an accepted extension but unreadable contents previously threw and failed the entire
uploadrun. It is now skipped, mirroring the existingSkipping unsupported file typepath:Valid PNG/JPEG uploads are unaffected.
Testing
upload.test.jsgains three specs — a file with an accepted extension but unreadable contents, a crafted ICNS file named.png(the CVE-2025-71330 regression), and a GIF named.pngfor the format gate. Each asserts the file is skipped and the build still finalizes. Its fixtures now use real PNG/JPEG bytes; the previous fixture was a GIF written through.toString(), which only survived because GIF headers happen to be UTF-8 safe.End-to-end against a directory holding a 1280x720 PNG, a 640x480 JPEG, a GIF renamed
.pngand the crafted ICNS payload:Reads both real images, skips the other two, no hang.
🤖 Generated with Claude Code