Fix: Percy CLI hangs on Node.js 26 during Chromium extraction (PER-10062) #2343
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,109 @@ | ||
| import fs from 'fs'; | ||
| import path from 'path'; | ||
| import stream from 'stream'; | ||
| import { promisify } from 'util'; | ||
| import yauzl from 'yauzl'; | ||
|
|
||
| const pipeline = promisify(stream.pipeline); | ||
| const openZip = promisify(yauzl.open); | ||
|
|
||
| // stat mode constants for entry external file attributes | ||
| const IFMT = 0o170000; | ||
| const IFDIR = 0o040000; | ||
| const IFLNK = 0o120000; | ||
|
|
||
| // 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; | ||
| } | ||
|
|
||
| // Collects a readable stream into a utf8 string (symlink targets) | ||
| async function readAll(readable) { | ||
| let chunks = []; | ||
| for await (let chunk of readable) chunks.push(chunk); | ||
| return Buffer.concat(chunks).toString('utf8'); | ||
| } | ||
|
|
||
| // Extracts a zip archive into a directory, preserving file modes, directory | ||
| // entries, and symlinks. Replaces the unmaintained extract-zip package, whose | ||
| // yauzl@2 dependency deadlocks on deflate entries under Node 26 | ||
| // (https://github.com/thejoshwolfe/yauzl/issues/176). | ||
| export default async function unzip(archive, { dir }) { | ||
| if (!path.isAbsolute(dir)) { | ||
| throw new Error('Target directory is expected to be absolute'); | ||
| } | ||
|
|
||
| await fs.promises.mkdir(dir, { recursive: true }); | ||
| dir = await fs.promises.realpath(dir); | ||
|
|
||
| let zipfile = await openZip(archive, { lazyEntries: true, autoClose: false }); | ||
| let openReadStream = promisify(zipfile.openReadStream.bind(zipfile)); | ||
|
Contributor
Author
There was a problem hiding this comment. [Low] yauzl@3 ships a native promise/async-iterator API
Suggestion: non-blocking follow-up — adopt Reviewer: stack:code-reviewer |
||
|
|
||
| try { | ||
| await new Promise((resolve, reject) => { | ||
| let done = false; | ||
| let finish = err => { | ||
| /* istanbul ignore if: defensive guard for stray zipfile events | ||
| after the promise has already settled */ | ||
| if (done) return; | ||
| done = true; | ||
| if (err) reject(err); else resolve(); | ||
| }; | ||
|
|
||
| zipfile.on('error', finish); | ||
| zipfile.on('end', () => finish()); | ||
|
|
||
| zipfile.on('entry', async entry => { | ||
| try { | ||
| // skip macOS resource fork entries | ||
| if (entry.fileName.startsWith('__MACOSX/')) { | ||
| return zipfile.readEntry(); | ||
| } | ||
|
|
||
| // strip any leading traversal segments before joining (zip-slip) | ||
| let fileName = entry.fileName.replace(/^(\.\.(\/|\\|$))+/, ''); | ||
| // `dir` is the trusted install target from install.js, not user input, | ||
| // and `fileName` is sanitized above with the traversal guard below | ||
| // nosemgrep: javascript.lang.security.audit.path-traversal.path-join-resolve-traversal.path-join-resolve-traversal | ||
| let dest = path.join(dir, fileName); | ||
Check warningCode scanning / Semgrep OSS Semgrep Finding: javascript.lang.security.audit.path-traversal.path-join-resolve-traversal.path-join-resolve-traversal Warning |
||
|
github-advanced-security[bot] marked this conversation as resolved.
Fixed
prklm10 marked this conversation as resolved.
Dismissed
|
||
|
|
||
| // guard against zip-slip for any remaining traversal | ||
| /* istanbul ignore if: yauzl rejects traversal entry names at parse | ||
| time (validateFileName); kept as defense-in-depth */ | ||
| if (path.relative(dir, dest).split(path.sep).includes('..')) { | ||
| throw new Error(`Out of bound path "${dest}" found while processing file ${entry.fileName}`); | ||
| } | ||
|
|
||
| let mode = (entry.externalFileAttributes >> 16) & 0xFFFF; | ||
| let isSymlink = (mode & IFMT) === IFLNK; | ||
| let isDir = (mode & IFMT) === IFDIR || | ||
| entry.fileName.endsWith('/') || | ||
| // directories from some windows archivers lack mode bits | ||
| ((entry.versionMadeBy >> 8) === 0 && entry.externalFileAttributes === 16); | ||
|
|
||
| if (isDir) { | ||
| await fs.promises.mkdir(dest, { recursive: true, mode: extractedMode(mode, true) }); | ||
| } else { | ||
| await fs.promises.mkdir(path.dirname(dest), { recursive: true }); | ||
| let contents = await openReadStream(entry); | ||
|
|
||
| if (isSymlink) { | ||
|
Contributor
Author
There was a problem hiding this comment. [Medium] Retry over a partial extraction throws
Suggestion: Reviewer: stack:code-reviewer |
||
| await fs.promises.symlink(await readAll(contents), dest); | ||
|
Contributor
Author
There was a problem hiding this comment. [Medium] Symlink target is not validated for containment The zip-slip guard only checks where the symlink is created, not where it points — an archive can plant a link inside Suggestion: resolve the target against let linkTarget = await readAll(contents);
let resolved = path.resolve(path.dirname(dest), linkTarget);
if (path.relative(dir, resolved).split(path.sep).includes('..')) {
throw new Error(`Out of bound symlink target "${linkTarget}" in ${entry.fileName}`);
}
await fs.promises.symlink(linkTarget, dest);Reviewer: stack:code-reviewer
Contributor
Author
There was a problem hiding this comment. [Low] Windows symlink Omitting the Suggestion: pass an explicit Reviewer: stack:code-reviewer |
||
| } else { | ||
| await pipeline(contents, fs.createWriteStream(dest, { mode: extractedMode(mode, false) })); | ||
| } | ||
| } | ||
|
|
||
| zipfile.readEntry(); | ||
| } catch (error) { | ||
| finish(error); | ||
| } | ||
| }); | ||
|
|
||
| zipfile.readEntry(); | ||
| }); | ||
| } finally { | ||
| zipfile.close(); | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -83,6 +83,9 @@ const METADATA_HOSTNAMES = new Set([ | |
| // ::ffff:169.254.169.254 compares equal to the literal 169.254.169.254 — the | ||
| // OS routes it to the IPv4 service, so it must not slip past the IP set). | ||
| function canonicalHost(host) { | ||
| /* istanbul ignore next: every caller guards against falsy hosts | ||
|
Contributor
Author
There was a problem hiding this comment. [Low] Unrelated coverage fix bundled into the Node 26 hang PR The Suggestion: split into a separate commit/PR titled around the coverage fix, unless CI required them to land together on this branch. Reviewer: stack:code-reviewer |
||
| (matchMetadataHost early-returns; METADATA_IPS maps literals) — | ||
| kept as a defensive no-op for direct use */ | ||
| if (!host) return host; | ||
| let h = String(host).toLowerCase().replace(/\.$/, ''); | ||
| let bare = h.replace(/^\[/, '').replace(/\]$/, ''); | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,210 @@ | ||
| import fs from 'fs'; | ||
| import os from 'os'; | ||
| import path from 'path'; | ||
| import zlib from 'zlib'; | ||
| import unzip from '../../src/unzip.js'; | ||
|
|
||
| // minimal crc32 used to craft valid zip fixtures (zlib.crc32 requires node 20+) | ||
| function crc32(buf) { | ||
| let crc = ~0; | ||
|
|
||
| for (let byte of buf) { | ||
| crc ^= byte; | ||
| for (let i = 0; i < 8; i++) { | ||
| crc = (crc >>> 1) ^ (0xEDB88320 & -(crc & 1)); | ||
| } | ||
| } | ||
|
|
||
| return ~crc >>> 0; | ||
| } | ||
|
|
||
| // builds a zip buffer from entry descriptors: | ||
| // { name, data, mode, deflate, attrs, madeBy, corrupt } | ||
| function makeZip(entries) { | ||
| let locals = []; | ||
| let centrals = []; | ||
| let offset = 0; | ||
|
|
||
| for (let { | ||
| name, data = '', mode = 0o644, deflate = false, | ||
| attrs = null, madeBy = (3 << 8) | 20, corrupt = false | ||
| } of entries) { | ||
| let nameBuf = Buffer.from(name); | ||
| let contents = Buffer.from(data); | ||
| let crc = crc32(contents); | ||
| let stored = deflate ? zlib.deflateRawSync(contents) : contents; | ||
| let method = deflate ? 8 : 0; | ||
|
|
||
| // corrupt the stored bytes after sizes and crc were computed | ||
| if (corrupt) stored = Buffer.from(stored.map(byte => byte ^ 0xFF)); | ||
|
|
||
| let local = Buffer.alloc(30); | ||
| local.writeUInt32LE(0x04034b50, 0); | ||
| local.writeUInt16LE(20, 4); // version needed | ||
| local.writeUInt16LE(method, 8); | ||
| local.writeUInt32LE(crc, 14); | ||
| local.writeUInt32LE(stored.length, 18); | ||
| local.writeUInt32LE(contents.length, 22); | ||
| local.writeUInt16LE(nameBuf.length, 26); | ||
| locals.push(local, nameBuf, stored); | ||
|
|
||
| let central = Buffer.alloc(46); | ||
| central.writeUInt32LE(0x02014b50, 0); | ||
| central.writeUInt16LE(madeBy, 4); // version made by | ||
| central.writeUInt16LE(20, 6); // version needed | ||
| central.writeUInt16LE(method, 10); | ||
| central.writeUInt32LE(crc, 16); | ||
| central.writeUInt32LE(stored.length, 20); | ||
| central.writeUInt32LE(contents.length, 24); | ||
| central.writeUInt16LE(nameBuf.length, 28); | ||
| central.writeUInt32LE(attrs !== null ? attrs : (mode << 16) >>> 0, 38); // external attributes | ||
| central.writeUInt32LE(offset, 42); | ||
| centrals.push(central, nameBuf); | ||
|
|
||
| offset += local.length + nameBuf.length + stored.length; | ||
| } | ||
|
|
||
| let cd = Buffer.concat(centrals); | ||
| let eocd = Buffer.alloc(22); | ||
| eocd.writeUInt32LE(0x06054b50, 0); | ||
| eocd.writeUInt16LE(entries.length, 8); | ||
| eocd.writeUInt16LE(entries.length, 10); | ||
| eocd.writeUInt32LE(cd.length, 12); | ||
| eocd.writeUInt32LE(offset, 16); | ||
|
|
||
| return Buffer.concat([...locals, cd, eocd]); | ||
| } | ||
|
|
||
| describe('Unit / Unzip', () => { | ||
| let tmp, archive; | ||
|
|
||
| beforeEach(() => { | ||
| tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'percy-unzip-')); | ||
| archive = path.join(tmp, 'fixture.zip'); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| fs.rmSync(tmp, { recursive: true, force: true }); | ||
| }); | ||
|
|
||
| it('extracts stored and deflated entries', async () => { | ||
| let big = 'percy'.repeat(100_000); | ||
|
|
||
| fs.writeFileSync(archive, makeZip([ | ||
| { name: 'dir/', mode: 0o40755 }, | ||
| { name: 'dir/stored.txt', data: 'stored contents' }, | ||
| { name: 'dir/deflated.txt', data: big, deflate: true } | ||
| ])); | ||
|
|
||
| await unzip(archive, { dir: path.join(tmp, 'out') }); | ||
|
|
||
| expect(fs.readFileSync(path.join(tmp, 'out/dir/stored.txt'), 'utf8')).toBe('stored contents'); | ||
| expect(fs.readFileSync(path.join(tmp, 'out/dir/deflated.txt'), 'utf8')).toBe(big); | ||
| }); | ||
|
|
||
| it('creates intermediate directories for nested entries', async () => { | ||
| fs.writeFileSync(archive, makeZip([ | ||
| { name: 'a/b/c/file.txt', data: 'nested' } | ||
| ])); | ||
|
|
||
| await unzip(archive, { dir: path.join(tmp, 'out') }); | ||
|
|
||
| expect(fs.readFileSync(path.join(tmp, 'out/a/b/c/file.txt'), 'utf8')).toBe('nested'); | ||
| }); | ||
|
|
||
| it('skips macOS resource fork entries', async () => { | ||
| fs.writeFileSync(archive, makeZip([ | ||
| { name: 'file.txt', data: 'real' }, | ||
| { name: '__MACOSX/file.txt', data: 'fork' } | ||
| ])); | ||
|
|
||
| await unzip(archive, { dir: path.join(tmp, 'out') }); | ||
|
|
||
| expect(fs.existsSync(path.join(tmp, 'out/file.txt'))).toBe(true); | ||
| expect(fs.existsSync(path.join(tmp, 'out/__MACOSX'))).toBe(false); | ||
| }); | ||
|
|
||
| it('rejects entries that would escape the target directory', async () => { | ||
| fs.writeFileSync(archive, makeZip([ | ||
| { name: '../evil.txt', data: 'oops' } | ||
| ])); | ||
|
|
||
| // 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/); | ||
|
|
||
| expect(fs.existsSync(path.join(tmp, 'evil.txt'))).toBe(false); | ||
| }); | ||
|
|
||
| it('applies default modes when the archive provides none', async () => { | ||
| fs.writeFileSync(archive, makeZip([ | ||
| { name: 'defaults/', attrs: 0 }, | ||
| { name: 'defaults/file.txt', data: 'contents', attrs: 0 } | ||
| ])); | ||
|
|
||
| await unzip(archive, { dir: path.join(tmp, 'out') }); | ||
|
|
||
| expect(fs.readFileSync(path.join(tmp, 'out/defaults/file.txt'), 'utf8')).toBe('contents'); | ||
|
|
||
| if (process.platform !== 'win32') { | ||
| expect(fs.statSync(path.join(tmp, 'out/defaults')).mode & 0o777).toBe(0o755); | ||
| expect(fs.statSync(path.join(tmp, 'out/defaults/file.txt')).mode & 0o777).toBe(0o644); | ||
| } | ||
| }); | ||
|
|
||
| it('treats windows-style directory attributes as directories', async () => { | ||
| fs.writeFileSync(archive, makeZip([ | ||
| { name: 'windir', attrs: 16, madeBy: 20 } | ||
| ])); | ||
|
|
||
| await unzip(archive, { dir: path.join(tmp, 'out') }); | ||
|
|
||
| expect(fs.statSync(path.join(tmp, 'out/windir')).isDirectory()).toBe(true); | ||
| }); | ||
|
|
||
| it('rejects entries with corrupted compressed data', async () => { | ||
| fs.writeFileSync(archive, makeZip([ | ||
| { name: 'corrupt.txt', data: 'percy'.repeat(1000), deflate: true, corrupt: true } | ||
| ])); | ||
|
|
||
| await expectAsync(unzip(archive, { dir: path.join(tmp, 'out') })) | ||
| .toBeRejectedWithError(/invalid|corrupt|crc/i); | ||
| }); | ||
|
|
||
| it('rejects relative target directories', async () => { | ||
| fs.writeFileSync(archive, makeZip([ | ||
| { name: 'file.txt', data: 'contents' } | ||
| ])); | ||
|
|
||
| await expectAsync(unzip(archive, { dir: 'relative/out' })) | ||
| .toBeRejectedWithError('Target directory is expected to be absolute'); | ||
| }); | ||
|
|
||
| if (process.platform !== 'win32') { | ||
| it('preserves executable file modes', async () => { | ||
| fs.writeFileSync(archive, makeZip([ | ||
| { name: 'bin/exec.sh', data: '#!/bin/sh\n', mode: 0o100755 }, | ||
| { name: 'bin/plain.txt', data: 'plain', mode: 0o100644 } | ||
| ])); | ||
|
|
||
| await unzip(archive, { dir: path.join(tmp, 'out') }); | ||
|
|
||
| expect(fs.statSync(path.join(tmp, 'out/bin/exec.sh')).mode & 0o777).toBe(0o755); | ||
| expect(fs.statSync(path.join(tmp, 'out/bin/plain.txt')).mode & 0o777).toBe(0o644); | ||
| }); | ||
|
|
||
| it('restores symlink entries', async () => { | ||
| fs.writeFileSync(archive, makeZip([ | ||
| { name: 'target.txt', data: 'linked contents' }, | ||
| { name: 'link.txt', data: 'target.txt', mode: 0o120755 } | ||
| ])); | ||
|
|
||
| await unzip(archive, { dir: path.join(tmp, 'out') }); | ||
|
|
||
| expect(fs.lstatSync(path.join(tmp, 'out/link.txt')).isSymbolicLink()).toBe(true); | ||
| expect(fs.readlinkSync(path.join(tmp, 'out/link.txt'))).toBe('target.txt'); | ||
| expect(fs.readFileSync(path.join(tmp, 'out/link.txt'), 'utf8')).toBe('linked contents'); | ||
| }); | ||
| } | ||
| }); |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Low] Explicit mode
0silently becomes the default644/755The falsy check means an entry that intentionally sets permission bits to
0gets the defaults instead. This mirrors extract-zip's behavior, so it's intentional parity — but a future reader may "fix" it and change behavior.Suggestion: add a one-line comment noting the falsy check is intentional parity with extract-zip.
Reviewer: stack:code-reviewer