fix(cli-upload): replace image-size with probe-image-size (PER-10489)

[aryanku-dev]

Fixes #2380 / PER-10489 (customer escalation — Foresters Financial). Supersedes #2382, which took the same dependency but kept a hand-written bounded read; this one lets the package do that work.

Root cause

npm audit fails for anyone installing @percy/cli because packages/cli-upload depends on image-size, which has unpatched high-severity advisories:

CVE	Parser	Patched
CVE-2025-71330	ICNS	none
CVE-2025-71329	JXL, HEIF	none

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 upstream is archived, so there is nothing to upgrade to. Bumping was doubly blocked — #2301 pinned ~1.0.2 to keep Node 14 support, which image-size 2.x drops (it requires >=16.x).

This was reachable, not theoretical

image-size selects its parser from magic bytes, while upload.js filters candidates by extension (ALLOWED_FILE_TYPES). A file named screenshot.png whose contents begin with the ICNS magic bytes therefore reached the ICNS parser.

Verified against the exact pinned version, with a 64-byte ICNS buffer whose first entry declares a length of zero:

$ node poc.cjs
image-size 1.0.2 - parsing crafted ICNS...
   ...no further output; killed by SIGKILL after 10s (exit 137)

Because the loop blocks the event loop the process cannot handle a 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 for customers running GitHub Advanced Security gates.

Why probe-image-size

I surveyed the alternatives rather than assuming:

Package	Deps	Node floor	Immune to this bug class?
image-meta (unjs)	0	none	No — I read the source; its ICNS loop has the identical zero-length defect, merely unreported
image-dimensions (sindresorhus)	0	>=18	Yes
image-size-safe, image-size-next, @localnerve/image-size	0	>=16	Yes, but all published within the last 6 weeks by single maintainers with <2k weekly downloads
probe-image-size (nodeca)	3 direct	none	Yes

image-dimensions is the cleanest library, but it declares engines: node >=18. Yarn 1 treats that as fatal, so it would break both this repo's CI and any customer on Node 14:

error image-dimensions@2.5.1: The engine "node" is incompatible with this module. Expected version ">=18". Got "14.18.3"
error Found incompatible module.

Every modern zero-dependency option requires Node >=16. probe-image-size is the only maintained one that still installs on Node 14, so it is the only choice that fixes the advisory without a second breaking change on top.

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 rather than advancing by it. npm audit on its tree reports 0 vulnerabilities.

Pinned to ^7.3.0 rather than ^7.4.0: 7.4.0 is four days old and is currently quarantined by BrowserStack's package-manager guard, so yarn install fails for anyone inside the corp network. The caret still resolves forward to 7.4.0 for end users once it ages out.

The change

13 lines. The package does the work:

let { default: probeImageSize } = await import('probe-image-size/stream.js');
…
let size = await probeImageSize(fs.createReadStream(absolutePath)).catch(() => null);

if (!size) {
  log.info(`Skipping file with unreadable image data: ${relativePath}`);
  continue;
}

Two things this buys over calling the package's index:

• stream.js, not the package index. Verified by inspecting require.cache after import:

  stream.js loads: probe-image-size, stream-parser
  index.js  loads: has-flag, lodash.merge, ms, needle, probe-image-size, sax, stream-parser, supports-color
  

  The http/needle machinery is installed but never loaded.
• No bounded-read code of our own. The stream prober stops reading and tears the stream down as soon as it has the dimensions, so a multi-megabyte PNG is not pulled into memory just to read its header. image-size did this internally with a 512 KiB cap; that behaviour is preserved by the library rather than reimplemented here.

Lockfile: image-size and queue are removed; probe-image-size, needle, sax and stream-parser are added. debug, iconv-lite, ms, safer-buffer and lodash.merge were already in the tree, so the net change is +2 installed packages.

Behaviour change

A file with an accepted extension but unreadable contents previously threw and failed the entire upload run. It is now skipped, mirroring the existing Skipping unsupported file type path:

[percy] Skipping file with unreadable image data: crafted.png

Valid PNG/JPEG uploads are unaffected — img.type is still derived from the extension exactly as before, so no file that used to upload stops uploading.

Testing

All 13 existing specs pass unchanged, on Node 14 to match CI. One spec is added for the new skip branch, using the CVE-2025-71330 proof of concept as its fixture:

Executed 14 of 14 specs SUCCESS

-----------|---------|----------|---------|---------|
File       | % Stmts | % Branch | % Funcs | % Lines |
-----------|---------|----------|---------|---------|
 upload.js |     100 |      100 |     100 |     100 |

End-to-end against a directory holding a real 1280x720 PNG, a real 640x480 JPEG and the crafted ICNS payload named .png:

[percy] Percy has started!
[percy] Skipping file with unreadable image data: crafted.png
[percy] Snapshot found: shot-1280x720.png
[percy] Snapshot found: shot-640x480.jpg
[percy] Found 2 snapshots

Reads both real images at the correct dimensions, skips the crafted one, no hang, exit 0.

🤖 Generated with Claude Code

[aryanku-dev]

Claude Code Review (automated) — 2 inline finding(s). Full report in the PR comment below. Verdict: Passed.

[aryanku-dev]

[Low] .catch(() => null) collapses every failure mode into one message

probe-image-size's stream.js wires src.on('error', reject), so errors from fs.createReadStream itself — ENOENT if a file is deleted mid-run, EACCES on a permissions problem, EMFILE under fd pressure — reach the same rejection path as "unrecognized image format". All of them now log the identical Skipping file with unreadable image data: … line, so a permissions or fd-exhaustion problem in the field is indistinguishable from a corrupt image in a support ticket.

Suggestion: keep the skip, preserve the cause at debug level:

let size = await probeImageSize(fs.createReadStream(absolutePath)).catch(err => {
  log.debug(`Probe failed for ${relativePath}: ${err.message}`);
  return null;
});

Reviewer: stack-code-reviewer

[aryanku-dev]

[Low] Probe runs unconditionally, including on the BYOS path that discards the result

When tokenType === 'generic' (line 117) the probed dimensions are thrown away — BYOS_TAG is a fixed {width: 1, height: 1} and only img.absolutePath is used. This was already true of the old synchronous imageSize() call, but the per-file cost is now a stream open plus a pipe-to-N-parsers setup rather than one buffered sync read, so the wasted work is more expensive. Relatedly, the loop awaits this per file where it previously did a sync read, so a directory with thousands of images pays that setup cost sequentially.

Suggestion: move the probe inside the non-generic branch (or short-circuit before it) so BYOS uploads skip it. If large directories become a real pain point, bound the probes with config.concurrency, already plumbed through for the discovery queue.

Reviewer: stack-code-reviewer

[aryanku-dev]

Claude Code PR Review

PR: #2390 • Head: 9dcbe39 • Reviewers: stack-code-reviewer

Summary

Replaces the archived, CVE-affected image-size dependency in @percy/cli-upload with probe-image-size's stream.js entrypoint, closing CVE-2025-71330 / CVE-2025-71329 (CWE-835 infinite loop reachable because image-size picked its parser from magic bytes while upload.js filtered by extension). A file with an accepted extension but unreadable contents is now skipped with a log line instead of failing the whole run.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	Pass	No credentials in the diff.
High	Security	Authentication/authorization checks present	Pass	Existing ALLOWED_TOKEN_TYPES gate at upload.js:85 is untouched.
High	Security	Input validation and sanitization	Pass	This is the point of the PR — the crafted-magic-bytes DoS path is removed. probe-image-size ships no ICNS/HEIF/JXL parser at all, and its ISOBMFF reader rejects a box smaller than its own header rather than advancing by it. Verified by the reviewer against the extracted 7.3.0 tarball: the exact craftedIcns fixture rejects in ~5 ms instead of hanging.
High	Security	No IDOR — resource ownership validated	N/A	No multi-tenant resource access in this diff.
High	Security	No SQL injection (parameterized queries)	N/A	No SQL.
High	Correctness	Logic is correct, handles edge cases	Pass	Independently verified at head: fs is imported (upload.js:1), so fs.createReadStream is in scope; continue sits directly inside for (let relativePath of pathnames) (upload.js:97) and correctly advances to the next file; img.type is reassigned from the extension at upload.js:112, so the extra fields probe-image-size returns (mime, wUnits, hUnits) cannot change upload behaviour. getImageResources destructures only the six fields it needs, so nothing leaks into the payload.
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	The .catch(() => null) is deliberate and surfaces a user-visible skip line, mirroring the existing Skipping unsupported file type branch. It does collapse the underlying error detail — tracked below as a Low finding, not a gate failure.
High	Correctness	No race conditions or concurrency issues	Pass	The probe is awaited inside a sequential for…of; no shared mutable state introduced. No FD leak: stream.js drives the read through stream.pipeline, which destroys the fs.createReadStream on both the resolve and reject paths.
Medium	Testing	New code has corresponding tests	Pass	One spec added for the new skip branch, using the CVE-2025-71330 PoC as its fixture; author reports 14/14 specs passing with 100% statement/branch/function/line coverage on upload.js.
Medium	Testing	Error paths and edge cases tested	Pass	The unreadable-image path is covered and asserts both the skip line and that the run still completes (Uploading 3 snapshots…, Finalized build #1…). The reviewer confirmed the crafted bytes genuinely trigger the CVE against the real dependency rather than being a synthetic no-op.
Medium	Testing	Existing tests still pass (no regressions)	Pass	All 13 pre-existing specs unchanged and passing per the PR description.
Medium	Performance	No N+1 queries or unbounded data fetching	Pass	The stream prober tears the stream down as soon as it has the dimensions, preserving the bounded-read behaviour image-size did internally with a 512 KiB cap — without reimplementing it locally.
Medium	Performance	Long-running tasks use background jobs	N/A	Not applicable to a local CLI directory scan.
Medium	Quality	Follows existing codebase patterns	Pass	The skip branch mirrors the adjacent Skipping unsupported file type handling; the dynamic await import matches the existing lazy-import style.
Medium	Quality	Changes are focused (single concern)	Pass	Three source files plus the lockfile, all serving the one dependency swap.
Low	Quality	Meaningful names, no dead code	Pass	image-size and its only-consumer transitive dep queue@6.0.2 are fully removed from yarn.lock; no other package in the monorepo referenced either.
Low	Quality	Comments explain why, not what	Pass	Both added comments explain rationale (why stream.js over the package index; why skip rather than fail).
Low	Quality	No unnecessary dependencies added	Pass	needle, sax, stream-parser come in as probe-image-size deps; debug, iconv-lite, ms, safer-buffer and lodash.merge were already in the tree, so the net change is +2 installed packages. stream.js never requires needle, so the HTTP machinery is installed but never loaded. Justified: every zero-dependency alternative requires Node ≥16, which would break the repo's Node 14 support.

Findings

• File: packages/cli-upload/src/upload.js:104
• Severity: Low
• Reviewer: stack-code-reviewer
• Issue: probe-image-size's stream.js wires src.on('error', reject), so errors from fs.createReadStream itself — ENOENT if a file is deleted mid-run, EACCES on a permissions problem, EMFILE under file-descriptor pressure — reach the same rejection path as "unrecognized image format". .catch(() => null) discards all of them and every case logs the identical Skipping file with unreadable image data: … line, so a permissions or fd-exhaustion problem in the field is indistinguishable from a corrupt image in a support ticket.
• Suggestion: Keep the skip behaviour but preserve the cause at debug level: .catch(err => { log.debug(\Probe failed for ${relativePath}: ${err.message}`); return null; })`.

---

• File: packages/cli-upload/src/upload.js:104
• Severity: Low
• Reviewer: stack-code-reviewer
• Issue: On the BYOS path (tokenType === 'generic', upload.js:117) the probed dimensions are discarded entirely — BYOS_TAG is a fixed {width: 1, height: 1} and only img.absolutePath is used. The probe still runs for every file. This was already true of the old synchronous imageSize() call, but the per-file cost is now a stream open plus a pipe-to-N-parsers setup rather than one buffered sync read, so the wasted work is more expensive.
• Suggestion: Move the probe inside the non-generic branch so BYOS uploads skip it, or short-circuit with if (tokenType === 'generic') before probing.

---

• File: packages/cli-upload/src/upload.js:104
• Severity: Low
• Reviewer: stack-code-reviewer
• Issue: The loop now awaits a stream-based probe per file where it previously did a synchronous buffered read, so a directory with hundreds or thousands of images pays the per-file stream-setup and event-loop overhead sequentially.
• Suggestion: Not blocking for a security fix. If large upload directories become a reported pain point, bound this with config.concurrency (already plumbed through for the discovery/snapshot queue) via a limited Promise.all.

---

Non-blocking nit (test intent): the new spec relies on Jasmine's default timeout to catch a reintroduced hang rather than asserting a bound explicitly. That works — a real hang would fail the suite — but an explicit timeout guard would make the regression's intent self-documenting.

Verified, no action needed: no file-descriptor leak (stream.pipeline destroys the read stream on both paths); no property collision or field leak into the uploaded payload; continue is valid in the enclosing for…of; probe-image-size declares no exports map so the probe-image-size/stream.js subpath resolves on Node 14, and its CJS module.exports = function interops correctly with { default: probeImageSize }; ico.js — the closest analog to the vulnerable ICNS loop — bounds its per-entry loop by a uint16 count rather than attacker-controlled lengths, so the bug class is not reintroduced.

---

Verdict: PASS — the dependency swap is correct, the CVE fix was verified empirically against the real package rather than taken on trust, and the three open findings are all Low-severity follow-ups.

[aryanku-dev]

Claude Code PR Review

PR: #2390 • Head: 5c8dce0 • Reviewers: stack-code-reviewer

Continues the previous review — changes since 9dcbe39 (delta).

Disclosure: the delta commit reviewed here (5c8dce0) was authored by Claude earlier in the same session that is running this review. It was reviewed by a subagent given an explicit instruction to treat it with added skepticism rather than deference, and its central claims were re-verified against packages/core/src/page.js by the orchestrator. It has still had no independent human review — weigh this section accordingly.

Summary

Two commits: 9dcbe39 swaps the CVE-affected image-size dependency in @percy/cli-upload for probe-image-size's stream.js entrypoint; 5c8dce0 replaces a setInterval-driven counter in the visual-regression fixture with a single deterministic mutation, removing an intermittent snapshot diff.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	Pass	None in either commit.
High	Security	Authentication/authorization checks present	Pass	ALLOWED_TOKEN_TYPES gate untouched.
High	Security	Input validation and sanitization	Pass	The point of 9dcbe39: CVE-2025-71330/71329 (CWE-835) closed. probe-image-size ships no ICNS/HEIF/JXL parser; verified empirically last run — the crafted fixture rejects in ~5 ms rather than hanging.
High	Security	No IDOR — resource ownership validated	N/A	No multi-tenant resource access.
High	Security	No SQL injection (parameterized queries)	N/A	No SQL.
High	Correctness	Logic is correct, handles edge cases	Pass	Delta verified against the real capture pipeline: <closed-dynamic-card> is in static markup, so the constructor runs at upgrade and connectedCallback fires synchronously after it — this._shadow is always assigned before it is read. ShadowRoot.getElementById is valid (DocumentOrShadowRoot mixin) and already used by AsyncDataCard in the same file. Re-entry is safe: textContent = '42' is idempotent, where the old count++ would have been actively wrong on reconnection.
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	.catch(() => null) is deliberate and logs a visible skip line; it does discard the cause — carried forward as a Low finding, not a gate failure.
High	Correctness	No race conditions or concurrency issues	Pass	The delta removes a race rather than adding one. No FD leak in 9dcbe39 (stream.pipeline destroys the read stream on both paths). See the Medium finding below for a pre-existing latent race on the same page.
Medium	Testing	New code has corresponding tests	Pass	9dcbe39 adds a spec for the skip branch using the CVE PoC as its fixture (14/14, 100% coverage on upload.js). 5c8dce0 is itself test-fixture code.
Medium	Testing	Error paths and edge cases tested	Pass	Unreadable-image path covered, asserting both the skip line and that the run completes.
Medium	Testing	Existing tests still pass (no regressions)	Pass	13 pre-existing specs unchanged. No other fixture or spec asserts on the Count: N digit, so the delta is correctly scoped to this one page.
Medium	Performance	No N+1 queries or unbounded data fetching	Pass	Stream prober tears down as soon as dimensions are known.
Medium	Performance	Long-running tasks use background jobs	N/A	Local CLI.
Medium	Quality	Follows existing codebase patterns	Pass	Skip branch mirrors the adjacent Skipping unsupported file type; the fixture's connectedCallback mutation matches AsyncDataCard's existing shape.
Medium	Quality	Changes are focused (single concern)	Pass	Two concerns now — the dependency swap and an unrelated regression-fixture fix. Landed together deliberately: the flake was turning this PR's own Percy build red, so it is the PR's blocker rather than unrelated drive-by work.
Low	Quality	Meaningful names, no dead code	Pass	image-size and its only consumer queue@6.0.2 fully removed from yarn.lock.
Low	Quality	Comments explain why, not what	Pass	The delta's comment accurately describes the mechanism — confirmed against closed-shadow.js, which resolves a live CDP object reference rather than a snapshot taken at attachShadow time, so the constructor-vs-connectedCallback distinction is real and not decorative.
Low	Quality	No unnecessary dependencies added	Pass	Net +2 installed packages; every zero-dependency alternative requires Node ≥16, which would break Node 14 support.

Findings

New this run

• File: test/regression/pages/interactive-states.html:582
• Severity: Medium
• Reviewer: stack-code-reviewer
• Issue: AsyncDataCard.connectedCallback schedules a setTimeout(..., 1000) that swaps #content from "Loading data…" to the loaded list — the same flake shape the delta just fixed, on the same page. It is not covered by Percy's custom-elements wait: WAIT_FOR_CUSTOM_ELEMENTS_BODY only polls :not(:defined), and async-data-card is defined synchronously at line 596, so the wait never applies to it. Tracing the real capture budget in packages/core/src/page.js:273-297: network.idle() (~150 ms at the configured networkIdleTimeout) then the custom-elements wait, which is capped at DEFAULT_WAIT_FOR_CUSTOM_ELEMENTS_TIMEOUT = 500 (page.js:19) — and on this page lazy-defined-widget stays undefined for its full 2000 ms, so that wait will burn its entire 500 ms ceiling every run rather than resolving early. That is ~650 ms before any CDP overhead for exposeClosedShadowRoots, insertPercyDom and serialization, against a 1000 ms window: roughly 350 ms of margin. This is pre-existing and not introduced by this delta, but it is structurally the same race, and the root cause of the bug just fixed was exactly CDP/CI overhead pushing past a nominal 1 s boundary.
• Suggestion: either push the delay well clear of the ~650 ms+ budget (mirroring the 2000 ms margin lazy-defined-widget already uses), or convert it to a deterministic single mutation the way ClosedDynamicCard just was — set the loaded state directly in connectedCallback instead of racing a timer against capture. Worth a follow-up ticket; not a reason to hold this PR.

Carried forward from 9dcbe39 (unresolved — this delta does not touch upload.js)

• packages/cli-upload/src/upload.js:104 — Low — .catch(() => null) collapses ENOENT/EACCES/EMFILE into the same "unreadable image data" line, losing the cause for support triage. Suggestion: log the error at debug level before returning null.
• packages/cli-upload/src/upload.js:104 — Low — the probe runs even on the BYOS (generic token) path, where dimensions are discarded for a fixed {1,1} tag. Suggestion: move the probe inside the non-generic branch.
• packages/cli-upload/src/upload.js:104 — Low — the per-file probe is awaited sequentially where it was previously a synchronous read. Suggestion: bound with config.concurrency if large directories become a pain point.

Verified, no action needed

percy-delayed-card (500 ms, dom-structures.html) looked like the closest call but does not actually race: the custom-elements wait's 500 ms deadline is set after network.idle() has already elapsed, while the widget's 500 ms counts from navigation — so the deadline is always strictly later and the wait observes the definition and resolves early. lazy-defined-widget (2000 ms) is comfortably outside the budget and deterministically captures its undefined state. Both are fine as-is.

---

Verdict: PASS — the delta removes a real flake without hollowing out the case it covers, and the one new finding is a pre-existing Medium worth a follow-up rather than a blocker.