From 42622a6522d0c400951227d5dff3868a2e48d5ec Mon Sep 17 00:00:00 2001 From: alanprado1 <100140427+alanprado1@users.noreply.github.com> Date: Sat, 24 Oct 2026 17:17:29 +1100 Subject: [PATCH 1/2] vfs: validate directory renames in ZipProvider ZipProvider.rename() and renameSync() never inspected the destination when renaming a directory, so a directory could be merged into a non-empty directory (silently overwriting its files) or placed beneath an existing regular file. Reject both cases with ENOTEMPTY and ENOTDIR, matching rename(2) and the default provider. Directory renames also interleaved reading and writing members, so a member that failed to read (e.g. a CRC mismatch) left the members before it already written to the archive, which a writable ZipFile persists to disk. Read every member before writing any. Fixes: https://github.com/nodejs/node/issues/66972 --- lib/internal/vfs/providers/ziparchive.js | 38 +++++++++++++++++++++--- 1 file changed, 34 insertions(+), 4 deletions(-) diff --git a/lib/internal/vfs/providers/ziparchive.js b/lib/internal/vfs/providers/ziparchive.js index 297d960a1f22..eb48426f3857 100644 --- a/lib/internal/vfs/providers/ziparchive.js +++ b/lib/internal/vfs/providers/ziparchive.js @@ -360,6 +360,23 @@ class ZipProvider extends VirtualProvider { return false; } + /** + * Throws when the directory being renamed cannot replace `name`: renaming a + * directory onto a non-directory fails with ENOTDIR, and onto a non-empty + * directory with ENOTEMPTY, matching rename(2) and the default provider. + * @param {string} name + * @param {string} path + */ + #validateDirectoryRenameTarget(name, path) { + if (this.#source.has(name)) throw createENOTDIR('rename', path); + const prefix = `${name}/`; + for (const key of this.#source.keys()) { + if (key !== prefix && StringPrototypeStartsWith(key, prefix)) { + throw createENOTEMPTY('rename', path); + } + } + } + /** * Throws when an ancestor of `name` is a file. Missing ancestors are valid: * ZIP archives represent directories implicitly when they contain entries @@ -593,13 +610,20 @@ class ZipProvider extends VirtualProvider { entries = [{ oldName, newName, entry }]; } if (oldName === newName) return; + if (entry === null) this.#validateDirectoryRenameTarget(newName, newPath); this.#validateParentDirectories(newName, 'rename', newPath); + // Read every member before writing any, so that a member that fails to + // read (e.g. a CRC mismatch) aborts the rename with the archive untouched. + const contents = []; for (let i = 0; i < entries.length; i++) { const item = entries[i]; item.entry ??= await this.#getEntry(item.oldName); - await this.#source.add( - item.newName, await item.entry.content(), renameOptions(item.entry)); + ArrayPrototypePush(contents, await item.entry.content()); + } + for (let i = 0; i < entries.length; i++) { + const item = entries[i]; + await this.#source.add(item.newName, contents[i], renameOptions(item.entry)); } for (let i = 0; i < entries.length; i++) { await this.#source.delete(entries[i].oldName); @@ -622,13 +646,19 @@ class ZipProvider extends VirtualProvider { entries = [{ oldName, newName, entry }]; } if (oldName === newName) return; + if (entry === null) this.#validateDirectoryRenameTarget(newName, newPath); this.#validateParentDirectories(newName, 'rename', newPath); + // See rename(): read every member before writing any. + const contents = []; for (let i = 0; i < entries.length; i++) { const item = entries[i]; item.entry ??= this.#getEntrySync(item.oldName); - this.#source.addSync( - item.newName, item.entry.contentSync(), renameOptions(item.entry)); + ArrayPrototypePush(contents, item.entry.contentSync()); + } + for (let i = 0; i < entries.length; i++) { + const item = entries[i]; + this.#source.addSync(item.newName, contents[i], renameOptions(item.entry)); } for (let i = 0; i < entries.length; i++) { this.#deleteEntrySync(entries[i].oldName); From ef4daeee0de807be41885a00072a0b7f4b0ebad5 Mon Sep 17 00:00:00 2001 From: alanprado1 <100140427+alanprado1@users.noreply.github.com> Date: Sat, 24 Oct 2026 17:18:39 +1100 Subject: [PATCH 2/2] test: cover ZipProvider directory renames Refs: https://github.com/nodejs/node/issues/66972 --- test/parallel/test-vfs-zip-provider-rename.js | 159 ++++++++++++++++++ 1 file changed, 159 insertions(+) create mode 100644 test/parallel/test-vfs-zip-provider-rename.js diff --git a/test/parallel/test-vfs-zip-provider-rename.js b/test/parallel/test-vfs-zip-provider-rename.js new file mode 100644 index 000000000000..77da8d8ed6d2 --- /dev/null +++ b/test/parallel/test-vfs-zip-provider-rename.js @@ -0,0 +1,159 @@ +// Flags: --disable-warning=ExperimentalWarning +'use strict'; + +// ZipProvider directory renames must follow the same rules as the default +// provider (see test-vfs-rename.js): a directory cannot replace a non-empty +// directory (ENOTEMPTY) or a non-directory (ENOTDIR), and a rename that fails +// part-way must not leave the archive modified. +// Refs: https://github.com/nodejs/node/issues/66972 + +const common = require('../common'); +const tmpdir = require('../common/tmpdir'); +const assert = require('assert'); +const fs = require('fs'); +const path = require('path'); +const zlib = require('zlib'); +const vfs = require('node:vfs'); + +tmpdir.refresh(); + +const CORRUPT_PAYLOAD = Buffer.from('payload whose CRC-32 will no longer match'); + +async function buildArchive(entries) { + const members = []; + for (const [name, content, options] of entries) { + members.push(await zlib.ZipEntry.create(name, Buffer.from(content), options)); + } + const chunks = []; + for await (const chunk of zlib.createZipArchive(members)) chunks.push(chunk); + return Buffer.concat(chunks); +} + +// Flips one byte of a stored (uncompressed) member so that reading it fails +// its CRC-32 check. +function corrupt(archive) { + const offset = archive.indexOf(CORRUPT_PAYLOAD); + assert.ok(offset > 0, 'corrupt member must be stored uncompressed'); + archive[offset] ^= 0xff; + return archive; +} + +function sortedKeys(source) { + return [...source.keys()].sort(); +} + +// Run every case through both the synchronous and promise-based API. +const renamers = [ + ['renameSync', async (v, from, to) => v.renameSync(from, to)], + ['promises.rename', (v, from, to) => v.promises.rename(from, to)], +]; + +(async () => { + for (const [label, rename] of renamers) { + // Directory onto a non-empty directory throws ENOTEMPTY and changes nothing. + { + const zip = new zlib.ZipBuffer(await buildArchive([ + ['src/file.txt', 'Original A'], + ['dst/file.txt', 'Original B'], + ['dst/keep.txt', 'Keep this file'], + ])); + const before = sortedKeys(zip); + const myVfs = vfs.create(new vfs.ZipProvider(zip)); + await assert.rejects(rename(myVfs, '/src', '/dst'), { code: 'ENOTEMPTY' }, label); + assert.deepStrictEqual(sortedKeys(zip), before); + assert.strictEqual(myVfs.readFileSync('/dst/file.txt', 'utf8'), 'Original B'); + assert.strictEqual(myVfs.readFileSync('/src/file.txt', 'utf8'), 'Original A'); + } + + // Same, when the destination is an explicit directory entry. + { + const zip = new zlib.ZipBuffer(await buildArchive([ + ['src/file.txt', 'Original A'], + ['dst/', ''], + ['dst/keep.txt', 'Keep this file'], + ])); + const before = sortedKeys(zip); + const myVfs = vfs.create(new vfs.ZipProvider(zip)); + await assert.rejects(rename(myVfs, '/src', '/dst'), { code: 'ENOTEMPTY' }, label); + assert.deepStrictEqual(sortedKeys(zip), before); + } + + // Directory onto a regular file throws ENOTDIR and changes nothing. + { + const zip = new zlib.ZipBuffer(await buildArchive([ + ['src/child.txt', 'CHILD CONTENT'], + ['target', 'ORIGINAL FILE'], + ])); + const before = sortedKeys(zip); + const myVfs = vfs.create(new vfs.ZipProvider(zip)); + await assert.rejects(rename(myVfs, '/src', '/target'), { code: 'ENOTDIR' }, label); + assert.deepStrictEqual(sortedKeys(zip), before); + assert.strictEqual(myVfs.readFileSync('/target', 'utf8'), 'ORIGINAL FILE'); + } + + // A source member that cannot be read fails the rename before anything + // is written, whether or not the destination already exists. + { + const zip = new zlib.ZipBuffer(corrupt(await buildArchive([ + ['src/01-good.txt', 'FIRST'], + ['src/02-bad.bin', CORRUPT_PAYLOAD, { method: 'store' }], + ]))); + const before = sortedKeys(zip); + const myVfs = vfs.create(new vfs.ZipProvider(zip)); + await assert.rejects(rename(myVfs, '/src', '/moved'), + { code: 'ERR_ZIP_ENTRY_CORRUPT' }, label); + assert.deepStrictEqual(sortedKeys(zip), before); + } + + // Renames that must keep working. + { + const zip = new zlib.ZipBuffer(await buildArchive([ + ['src/file.txt', 'A'], + ['empty/', ''], + ['f.txt', 'x'], + ])); + const myVfs = vfs.create(new vfs.ZipProvider(zip)); + + // Directory onto an empty directory. + await rename(myVfs, '/src', '/empty'); + assert.strictEqual(myVfs.existsSync('/src'), false); + assert.strictEqual(myVfs.readFileSync('/empty/file.txt', 'utf8'), 'A'); + + // Directory to a new name. + await rename(myVfs, '/empty', '/moved'); + assert.strictEqual(myVfs.existsSync('/empty'), false); + assert.strictEqual(myVfs.readFileSync('/moved/file.txt', 'utf8'), 'A'); + + // File onto a directory still throws EISDIR; directory into its own + // descendant still throws EINVAL. + await assert.rejects(rename(myVfs, '/f.txt', '/moved'), { code: 'EISDIR' }, label); + await assert.rejects(rename(myVfs, '/moved', '/moved/sub'), { code: 'EINVAL' }, label); + } + + // Disk-backed archive: a failed rename leaves the file on disk unchanged. + { + const file = path.join(tmpdir.path, `rename-${label}.zip`); + fs.writeFileSync(file, corrupt(await buildArchive([ + ['dst/01-good.txt', 'ORIGINAL DESTINATION'], + ['src/01-good.txt', 'REPLACEMENT DATA'], + ['src/02-bad.bin', CORRUPT_PAYLOAD, { method: 'store' }], + ]))); + + const zip = zlib.ZipFile.openSync(file, { writable: true }); + const myVfs = vfs.create(new vfs.ZipProvider(zip)); + await assert.rejects(rename(myVfs, '/src', '/dst'), { code: 'ENOTEMPTY' }, label); + await assert.rejects(rename(myVfs, '/src', '/moved'), + { code: 'ERR_ZIP_ENTRY_CORRUPT' }, label); + zip.closeSync(); + + const reopened = zlib.ZipFile.openSync(file); + assert.deepStrictEqual(sortedKeys(reopened), [ + 'dst/01-good.txt', 'src/01-good.txt', 'src/02-bad.bin', + ]); + assert.strictEqual( + vfs.create(new vfs.ZipProvider(reopened)).readFileSync('/dst/01-good.txt', 'utf8'), + 'ORIGINAL DESTINATION'); + reopened.closeSync(); + } + } +})().then(common.mustCall());