Fix: Percy CLI hangs on Node.js 26 during Chromium extraction — adm-zip variant (PER-10062)#2344
Conversation
…rage break) Master's Test @percy/core job has been red since a4b6b8a — global 100% coverage fails on src/utils.js:103, the canonicalHost catch for colon-containing hosts that are not valid IPv6. No spec fed such a host through the metadata gates. Adds one isMetadataIP spec ('abc:def') that deterministically exercises the fallback on Node 14/20/26. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…a callers) Second half of the master coverage break: the falsy-host guard in canonicalHost is unreachable — matchMetadataHost early-returns on falsy hosts before calling it and METADATA_IPS maps hardcoded literals. Kept as a defensive no-op with an ignore pragma per repo convention. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… (PER-10062) Chromium archive extraction hangs forever on Node.js 26 because extract-zip@2.0.1 depends on yauzl@2, which relies on undefined stream destroy() behavior that Node 26 tightened — openReadStream callbacks never fire on deflate entries (thejoshwolfe/yauzl#176). adm-zip inflates synchronously and is unaffected. adm-zip's own extractAllTo is not used: verified on Node 26 it writes symlink entries as regular files containing the target path (breaking the Chromium.app bundle) and mishandles windows-style directory attributes. The local unzip module iterates getEntries() and restores modes, directories, and symlinks itself, with a zip-slip guard. Verified: 10 unzip specs (deflate, symlinks, exec modes, windows dirs, corrupted data, zip-slip) pass on Node 14 and Node 26; 100% coverage. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
CodeQL js/zipslip does not model split(sep).includes('..') as a barrier;
path.relative(dir, dest).startsWith('..') is its recognized
RelativePathStartsWithSanitizer shape. Same rejection semantics.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| // `dir` is the trusted install target from install.js, not user input, | ||
| // and entry names are validated by the traversal guard below | ||
| // nosemgrep: javascript.lang.security.audit.path-traversal.path-join-resolve-traversal.path-join-resolve-traversal | ||
| let dest = path.join(dir, entry.entryName); |
| // `dir` is the trusted install target from install.js, not user input, | ||
| // and entry names are validated by the traversal guard below | ||
| // nosemgrep: javascript.lang.security.audit.path-traversal.path-join-resolve-traversal.path-join-resolve-traversal | ||
| let dest = path.join(dir, entry.entryName); |
… specs The exec:start specs waited a fixed 1s after start() before pinging, but request() only retries ECONNREFUSED 5 times at 50ms intervals, so on slow Windows runners where the server takes longer to bind the specs fail with EEXIT: 1 (flaking on master too, not specific to this branch). Replace the sleep with a healthcheck poll bounded at 15s (within the 25s Windows jasmine timeout). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…6-admzip # Conflicts: # packages/core/src/utils.js # packages/core/test/utils.test.js
prklm10
left a comment
prklm10
left a comment
There was a problem hiding this comment.
Claude Code Review (automated) — 5 inline finding(s). Full report in the PR comment below. Verdict: Passed.
| // yauzl 3 rejects relative paths at parse time; the unzip module also | ||
| // guards against zip-slip for entries yauzl allows through | ||
| await expectAsync(unzip(archive, { dir: path.join(tmp, 'out') })) | ||
| .toBeRejectedWithError(/invalid relative path|Out of bound path/); |
There was a problem hiding this comment.
[Medium] Stale comment + dead regex branch
The comment says "yauzl 3 rejects relative paths at parse time," but this module no longer uses yauzl at all — it's leftover from the extract-zip era. The invalid relative path alternative in the regex is dead (adm-zip never throws that string).
Suggestion: describe the zip-slip guard instead and tighten the regex to /Out of bound path/.
Reviewer: stack:code-reviewer
| // healthcheck endpoint until the server is reachable. A fixed sleep proved | ||
| // flaky on slow Windows CI runners where startup can exceed a second and | ||
| // `request` only retries ECONNREFUSED 5 times at 50ms intervals. | ||
| async function waitForHealthcheck(port = 5338, deadline = Date.now() + 15000) { |
There was a problem hiding this comment.
[Medium] Poll deadline exceeds Jasmine's non-Windows timeout
The 15s default deadline is longer than jasmine.DEFAULT_TIMEOUT_INTERVAL = 10000 set for non-Windows platforms in scripts/test-helpers.js (25s is Windows-only). A slow Linux/macOS runner needing >10s would fail with Jasmine's generic "Async function did not complete within 10000ms" instead of the informative healthcheck error this helper is meant to provide.
Suggestion: cap the default deadline below 10s on non-Windows, or raise the affected specs' own timeout to at least the poll deadline.
Reviewer: stack:code-reviewer
| // Returns the file mode of an extracted entry, with defaults matching the | ||
| // previous extract-zip behavior when the archive does not provide one | ||
| function extractedMode(mode, isDir) { | ||
| return (mode || (isDir ? 0o755 : 0o644)) & 0o777; |
There was a problem hiding this comment.
[Low] mode || default conflates "no mode" with "mode 0"
An entry whose archive mode is genuinely 0 silently gets the 0755/0644 default instead of 0. Harmless for Chromium archives, but this module isn't Chromium-specific.
Suggestion: add a one-line comment noting the tradeoff.
Reviewer: stack:code-reviewer
| let contents = entry.getData(); | ||
|
|
||
| if (isSymlink) { | ||
| await fs.promises.symlink(contents.toString('utf8'), dest); |
There was a problem hiding this comment.
[Low] Symlink creation isn't idempotent on retried extractions
fs.promises.symlink throws EEXIST if dest already exists. download() in install.js only skips extraction when the final executable exists, so a retry after a mid-extraction failure into the same revision outdir could fail permanently until .local-chromium/<revision> is deleted manually.
Suggestion: await fs.promises.rm(dest, { force: true }) before symlink, or remove the outdir on extraction failure.
Reviewer: stack:code-reviewer
| while (true) { | ||
| try { | ||
| return await request(`http://localhost:${port}/percy/healthcheck`, { retries: 0, noProxy: true }); | ||
| } catch (err) { |
There was a problem hiding this comment.
[Low] Retries on any error, not just connection-refused
If the server binds but the healthcheck route itself errors, the loop masks the real error for up to 15s before surfacing it, delaying failure feedback.
Suggestion: optionally retry only connection errors (e.g. err.code === 'ECONNREFUSED') and rethrow others immediately.
Reviewer: stack:code-reviewer
Claude Code PR ReviewPR: #2344 • Head: 31d304b • Reviewers: stack:code-reviewer SummaryReplaces the unmaintained Review Table
Findings
Verdict: PASS — no Critical/High findings; the extraction logic was verified against adm-zip's source, coverage is thorough, and all 46 CI checks pass. The two Medium test-hygiene findings are worth a quick follow-up but are non-blocking. |
3 participants
Fixes PER-10062. Alternative implementation to #2343 — same root cause and test contract, different extraction library. Pick one.
Root cause
Identical to #2343:
extract-zip@2.0.1→yauzl@2relies on undefined streamdestroy()behavior that Node.js 26 tightened, soopenReadStream()callbacks never fire on deflate entries and Chromium extraction hangs forever (thejoshwolfe/yauzl#176).percy execblocks before spawning the wrapped command → no tests run, builds finalizeno_snapshots.Fix
Replaces
extract-zipwith a localpackages/core/src/unzip.jsbuilt onadm-zip@^0.6.0, which inflates synchronously and is unaffected by the Node 26 stream change.Why not adm-zip's own
extractAllTo: verified on Node 26 that it writes symlink entries as regular files containing the target path — this breaks theChromium.appbundle on macOS (framework symlinks) — and mishandles windows-style directory attributes. The wrapper iteratesgetEntries()and restores file modes, directories, and symlinks itself, with a zip-slip guard (reachable here, unlike with yauzl 3 which pre-validates entry names).Trade-offs vs #2343 (yauzl@3)
pend.getData()CRC-validates every entry (corrupted data throws instead of writing bad files).Testing
unzip.test.jscontract as Fix: Percy CLI hangs on Node.js 26 during Chromium extraction (PER-10062) #2343 (hand-crafted zip fixtures: stored + deflated entries, nested dirs,__MACOSX/skip, zip-slip rejection, default + executable modes, symlinks, windows dir attrs, corrupted data): pass on Node 14.21.3 and Node 26.5.0, 100% coverage on the new module.🤖 Generated with Claude Code