From eaef85d500633eb0166ffc8a1615decc0d55f979 Mon Sep 17 00:00:00 2001 From: Sag Date: Thu, 25 Jun 2026 11:42:05 +0200 Subject: [PATCH 1/4] Added opt-in owner permission normalization to @tryghost/zip extract - Adds an `ensureOwnerPermissions` option (default `false`) to `extract()` - When enabled, normalizes extracted entry permissions so the owner can always read/move/remove the result: directories gain at least owner rwx and files gain at least owner rw, while existing execute/group/world bits are preserved - Fixes archives containing read-only directories (e.g. `dr-xr-xr-x`/`0555`) that otherwise fail to extract because nested files cannot be written into them - Normalization mutates only the in-memory entry handed to extract-zip; the source zip is never rewritten or chmodded, and it runs after the existing size-limit, symlink and filename checks - Defaults are unchanged for existing callers Co-Authored-By: Claude Opus 4.8 --- packages/zip/README.md | 12 +++ packages/zip/lib/extract.js | 87 +++++++++++++-- packages/zip/test/zip.test.js | 196 ++++++++++++++++++++++++++++++++++ 3 files changed, 289 insertions(+), 6 deletions(-) diff --git a/packages/zip/README.md b/packages/zip/README.md index 4a1acc563..d4c6f3383 100644 --- a/packages/zip/README.md +++ b/packages/zip/README.md @@ -26,6 +26,18 @@ let res = await zip.compress('path/to/a/folder', 'path/to/archive.zip', [options let res = await zip.extract('path/to/archive.zip', 'path/to/files', [options]) ``` +### `extract` options + +- `limits.perEntryUncompressedBytes` / `limits.totalUncompressedBytes` — reject archives whose entries exceed the given uncompressed sizes. +- `onEntry(entry, zipfile)` — called for every entry before it is written. +- `ensureOwnerPermissions` (default `false`) — when `true`, normalizes extracted entry permissions so the owner can always read, move and remove the result. Directories gain at least owner `rwx` and files gain at least owner `rw`, while existing execute/group/world bits are preserved. The source zip is never modified. + + This fixes archives that contain read-only directories (for example a `dr-xr-xr-x` / `0555` folder), which otherwise fail to extract because nested files cannot be written into them. It is intended for **trusted temporary extraction** of user-supplied archives (such as theme zips) where the caller must be able to read, move and remove the extracted tree. + + ``` + let res = await zip.extract('path/to/upload.zip', 'path/to/tmp', {ensureOwnerPermissions: true}) + ``` + ## Develop This is a mono repository, managed with [Nx](https://nx.dev). diff --git a/packages/zip/lib/extract.js b/packages/zip/lib/extract.js index 4cc6d8b80..a13e08afe 100644 --- a/packages/zip/lib/extract.js +++ b/packages/zip/lib/extract.js @@ -2,6 +2,73 @@ const errors = require('@tryghost/errors'); const defaultOptions = {}; +// POSIX `stat` mode constants, matching the ones extract-zip uses internally +// when it derives an entry's type and permissions from its external attributes. +const S_IFMT = 0o170000; // bit mask for the file type bit fields +const S_IFDIR = 0o040000; // directory +const S_IFLNK = 0o120000; // symbolic link + +// Owner permission bits we guarantee when `ensureOwnerPermissions` is enabled. +const OWNER_DIR_BITS = 0o700; // rwx, so the tree can be traversed, written and removed +const OWNER_FILE_BITS = 0o600; // rw, so files can be read and rewritten/removed + +// Reads the POSIX mode an entry carries in the high 16 bits of its external +// file attributes, exactly as extract-zip does when extracting. +function getEntryMode(entry) { + return (entry.externalFileAttributes >> 16) & 0xffff; +} + +// Mirrors extract-zip's directory detection: POSIX directory type bit, a +// trailing slash, or the Windows directory attribute. +function isDirectoryEntry(entry, mode) { + if ((mode & S_IFMT) === S_IFDIR) { + return true; + } + + if (entry.fileName.endsWith('/')) { + return true; + } + + // Windows' way of specifying a directory + // https://github.com/maxogden/extract-zip/issues/13#issuecomment-154494566 + const madeBy = entry.versionMadeBy >> 8; + return madeBy === 0 && entry.externalFileAttributes === 16; +} + +// Normalises an entry's permissions in place so the extracting user can always +// read, move and remove the result. Only the in-memory entry handed to +// extract-zip is changed; the original zip file is never rewritten. +function ensureOwnerPermissions(entry) { + const mode = getEntryMode(entry); + + // No POSIX mode bits are present. extract-zip falls back to its own default + // dir/file modes (0o755/0o644), which already grant the owner full access, + // so leave the entry untouched to preserve default behaviour. + if (mode === 0) { + return; + } + + const isDir = isDirectoryEntry(entry, mode); + const ownerBits = isDir ? OWNER_DIR_BITS : OWNER_FILE_BITS; + let nextMode = mode | ownerBits; + + // A directory that was only inferred (trailing slash / Windows attribute) + // may lack the POSIX directory type bit. Set it so extract-zip still + // classifies the rewritten mode as a directory. + if (isDir && (nextMode & S_IFMT) !== S_IFDIR) { + nextMode = (nextMode & ~S_IFMT) | S_IFDIR; + } + + if (nextMode === mode) { + return; + } + + // Write the new POSIX mode back into the high 16 bits while preserving the + // low 16 bits (DOS attributes and other zip-specific flags). + const lowBits = entry.externalFileAttributes & 0xffff; + entry.externalFileAttributes = (((nextMode & 0xffff) << 16) | lowBits) >>> 0; +} + function normalizeLimit(value, fieldName) { if (value === undefined || value === null || value === Infinity) { return Infinity; @@ -106,12 +173,9 @@ function throwOnTotalTooLarge(entry, observedBytes, limitBytes, entriesProcessed } function throwOnSymlinks(entry) { - // Check if symlink - const mode = (entry.externalFileAttributes >> 16) & 0xffff; - // check if it's a symlink or dir (using stat mode constants) - const IFMT = 61440; - const IFLNK = 40960; - const symlink = (mode & IFMT) === IFLNK; + // Check if symlink (using stat mode constants) + const mode = getEntryMode(entry); + const symlink = (mode & S_IFMT) === S_IFLNK; if (symlink) { throw new errors.UnsupportedMediaTypeError({ @@ -156,6 +220,10 @@ function throwOnLargeFilenames(entry) { * @param {Object} [options.limits] - if present, sets maximum uncompressed sizes * @param {Integer} options.limits.perEntryUncompressedBytes - maximum uncompressed size of each entry * @param {Integer} options.limits.totalUncompressedBytes - maximum total uncompressed size across all entries + * @param {Boolean} [options.ensureOwnerPermissions] - when true, normalizes extracted entry permissions so the + * owner can always read/move/remove the result: directories gain at least owner rwx and files at least owner rw, + * while existing execute/group/world bits are preserved. The source zip is never modified. Defaults to false. + * Intended for trusted temporary extraction of user-supplied archives (e.g. theme zips with read-only directories). */ module.exports = async (zipToExtract, destination, options) => { const opts = Object.assign({}, defaultOptions, options); @@ -167,6 +235,7 @@ module.exports = async (zipToExtract, destination, options) => { opts.dir = destination; + const shouldEnsureOwnerPermissions = opts.ensureOwnerPermissions === true; const originalOnEntry = opts.onEntry; opts.onEntry = (entry, zipfile) => { const entryUncompressedBytes = getEntryUncompressedSize(entry, limits); @@ -188,6 +257,12 @@ module.exports = async (zipToExtract, destination, options) => { if (originalOnEntry) { originalOnEntry(entry, zipfile); } + + // Normalize permissions last, immediately before extract-zip writes the + // entry. Do not add async work here: extract-zip does not await onEntry. + if (shouldEnsureOwnerPermissions) { + ensureOwnerPermissions(entry); + } }; await extract(zipToExtract, opts); diff --git a/packages/zip/test/zip.test.js b/packages/zip/test/zip.test.js index 4ab0aff61..2de0f0032 100644 --- a/packages/zip/test/zip.test.js +++ b/packages/zip/test/zip.test.js @@ -62,6 +62,33 @@ function createZipWithEntries(zipPath, entries) { }); } +// Builds a zip where each entry can carry an explicit `mode` and `type` +// (e.g. a read-only `0o555` directory). Only defined keys are forwarded so that +// archiver applies its own defaults for the rest. A `null` source produces an +// empty entry, which is what directory entries need. +function createZipWithModedEntries(zipPath, entries) { + return createZip(zipPath, (archive) => { + for (const entry of entries) { + const data = { name: entry.name }; + + if (entry.type) { + data.type = entry.type; + } + + if (typeof entry.mode === 'number') { + data.mode = entry.mode; + } + + archive.append(entry.content === undefined ? null : entry.content, data); + } + }); +} + +// Reads the permission bits (lowest 9 bits) of a path's mode. +function permissionBits(targetPath) { + return fs.statSync(targetPath).mode & 0o777; +} + describe('Compress and Extract should be opposite functions', function () { let symlinkPath, themeFolder, zipDestination, unzipDestination; @@ -469,3 +496,172 @@ describe('Extract zip', function () { } }); }); + +describe('Extract zip with ensureOwnerPermissions', function () { + let zipDestination, unzipDestination; + + beforeAll(function () { + zipDestination = path.join(__dirname, 'fixtures', 'perms-theme.zip'); + unzipDestination = path.join(__dirname, 'fixtures', 'perms-theme-unzipped'); + }); + + afterEach(function () { + if (fs.existsSync(zipDestination)) { + fs.rmSync(zipDestination, { recursive: true, force: true }); + } + + if (fs.existsSync(unzipDestination)) { + // chmod the tree back to writable first so a read-only directory left + // behind by a default-mode extraction can still be removed. + for (const entry of fs.readdirSync(unzipDestination, { recursive: true })) { + fs.chmodSync(path.join(unzipDestination, entry), 0o755); + } + + fs.rmSync(unzipDestination, { recursive: true, force: true }); + } + }); + + it('preserves default read-only directory permissions when the option is omitted', async function () { + await createZipWithModedEntries(zipDestination, [ + { name: 'readonly-dir/', type: 'directory', mode: 0o555 }, + ]); + + await extract(zipDestination, unzipDestination); + + const dirPath = path.join(unzipDestination, 'readonly-dir'); + assert.equal(fs.existsSync(dirPath), true); + // Default behaviour is unchanged: the owner write bit is not added. + assert.equal(permissionBits(dirPath) & 0o200, 0); + }); + + it('extracts an archive with a read-only directory and read-only files', async function () { + await createZipWithModedEntries(zipDestination, [ + { name: 'readonly-dir/', type: 'directory', mode: 0o555 }, + { name: 'readonly-dir/nested.txt', content: 'nested content', mode: 0o444 }, + { name: 'readonly-file.txt', content: 'top-level content', mode: 0o444 }, + { name: 'writable-file.txt', content: 'already writable', mode: 0o644 }, + ]); + + await extract(zipDestination, unzipDestination, { ensureOwnerPermissions: true }); + + const dirPath = path.join(unzipDestination, 'readonly-dir'); + const nestedPath = path.join(dirPath, 'nested.txt'); + const filePath = path.join(unzipDestination, 'readonly-file.txt'); + const writablePath = path.join(unzipDestination, 'writable-file.txt'); + + // The directory gains owner rwx so the nested file can be written into it. + assert.equal((permissionBits(dirPath) & 0o700) === 0o700, true); + // Existing group/world read+execute bits are preserved. + assert.equal(permissionBits(dirPath) & 0o055, 0o055); + + // The nested file is extracted and readable. + assert.equal(fs.readFileSync(nestedPath, 'utf8'), 'nested content'); + + // Read-only files gain owner write while keeping group/world read. + assert.equal(permissionBits(filePath) & 0o600, 0o600); + assert.equal(permissionBits(filePath) & 0o044, 0o044); + assert.equal(fs.readFileSync(filePath, 'utf8'), 'top-level content'); + + // Files that already grant owner write are left untouched. + assert.equal(permissionBits(writablePath), 0o644); + assert.equal(fs.readFileSync(writablePath, 'utf8'), 'already writable'); + }); + + it('keeps execute bits on executable files while adding owner write', async function () { + await createZipWithModedEntries(zipDestination, [ + { name: 'script.sh', content: '#!/bin/sh\necho hi\n', mode: 0o555 }, + ]); + + await extract(zipDestination, unzipDestination, { ensureOwnerPermissions: true }); + + const scriptPath = path.join(unzipDestination, 'script.sh'); + const bits = permissionBits(scriptPath); + + // Owner gains write... + assert.equal(bits & 0o200, 0o200); + // ...without losing any of the original r-xr-xr-x execute/read bits. + assert.equal(bits & 0o555, 0o555); + }); + + it('sets the directory type bit on inferred directories that lack POSIX type bits', async function () { + // A directory recognised only by its trailing slash, carrying 0o555 + // permission bits but no POSIX directory type bit, plus a low DOS bit. + const entry = { + fileName: 'inferred-dir/', + externalFileAttributes: (((0o555 << 16) >>> 0) | 0x10) >>> 0, + versionMadeBy: 0x0300, // unix host, so the Windows directory rule does not apply + }; + + const originalLoad = Module._load; + Module._load = function (request, parent, isMain) { + if (request === 'extract-zip') { + return async (zipPath, opts) => { + opts.onEntry(entry, {}); + }; + } + + return originalLoad(request, parent, isMain); + }; + + try { + await extract(zipDestination, unzipDestination, { ensureOwnerPermissions: true }); + } finally { + Module._load = originalLoad; + } + + const normalizedMode = (entry.externalFileAttributes >> 16) & 0xffff; + // The directory type bit is now present... + assert.equal(normalizedMode & 0o170000, 0o040000); + // ...owner gains rwx while the original r-x bits are preserved. + assert.equal(normalizedMode & 0o777, 0o755); + // Low DOS attribute bits are preserved. + assert.equal(entry.externalFileAttributes & 0xffff, 0x10); + }); + + it('leaves entries without POSIX mode bits untouched', async function () { + // No POSIX mode bits at all: extract-zip applies its own defaults, so the + // entry must be left exactly as-is. + const entry = { fileName: 'no-mode.txt', externalFileAttributes: 0 }; + + const originalLoad = Module._load; + Module._load = function (request, parent, isMain) { + if (request === 'extract-zip') { + return async (zipPath, opts) => { + opts.onEntry(entry, {}); + }; + } + + return originalLoad(request, parent, isMain); + }; + + try { + await extract(zipDestination, unzipDestination, { ensureOwnerPermissions: true }); + } finally { + Module._load = originalLoad; + } + + assert.equal(entry.externalFileAttributes, 0); + }); + + it('still rejects symlink entries when ensureOwnerPermissions is enabled', async function () { + await createZip(zipDestination, (archive) => { + archive.symlink('themes/test-target', 'symlink-entry'); + }); + + await assert.rejects( + () => + extract(zipDestination, unzipDestination, { + ensureOwnerPermissions: true, + }), + (err) => { + assert.equal(err.errorType, 'UnsupportedMediaTypeError'); + assert.equal(err.message, 'Symlinks are not allowed in the zip folder.'); + assert.equal(err.code, 'SYMLINK_NOT_ALLOWED'); + assert.deepEqual(err.errorDetails, { + entryName: 'themes/test-target', + }); + return true; + }, + ); + }); +}); From ca53c17aa2c9b0f01222c2080bbb7e917a3e4d05 Mon Sep 17 00:00:00 2001 From: Sag Date: Thu, 25 Jun 2026 11:52:54 +0200 Subject: [PATCH 2/4] Honor owner-less defaults for zero-mode entries in ensureOwnerPermissions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously, entries with no POSIX mode bits early-returned from ensureOwnerPermissions and were handed to extract-zip as mode 0. When the caller combined `ensureOwnerPermissions: true` with a `defaultDirMode`/ `defaultFileMode` that omits owner bits (e.g. 0o555/0o444), extract-zip synthesized that owner-less default, so a no-POSIX-mode directory/file could still be created without owner write — violating the option's guarantee. Now zero-mode entries resolve the same default extract-zip would apply (mirroring its getExtractedMode fallback) and then have the owner bits OR-ed in, so the owner can always read/move/traverse the extracted tree regardless of the supplied defaults. The default case (no custom defaults) is unchanged. Co-Authored-By: Claude Opus 4.8 --- packages/zip/lib/extract.js | 46 +++++++++++++++++---- packages/zip/test/zip.test.js | 76 +++++++++++++++++++++++++++++++++-- 2 files changed, 111 insertions(+), 11 deletions(-) diff --git a/packages/zip/lib/extract.js b/packages/zip/lib/extract.js index a13e08afe..af7981fa9 100644 --- a/packages/zip/lib/extract.js +++ b/packages/zip/lib/extract.js @@ -35,20 +35,48 @@ function isDirectoryEntry(entry, mode) { return madeBy === 0 && entry.externalFileAttributes === 16; } +// Resolves the mode extract-zip would synthesise for an entry that carries no +// POSIX mode bits, mirroring its getExtractedMode() fallback: the caller's +// defaultDirMode/defaultFileMode, or 0o755/0o644 when those are not set. +function resolveDefaultMode(isDir, opts) { + let mode = 0; + + if (isDir) { + if (opts.defaultDirMode) { + mode = parseInt(opts.defaultDirMode, 10); + } + + if (!mode) { + mode = 0o755; + } + } else { + if (opts.defaultFileMode) { + mode = parseInt(opts.defaultFileMode, 10); + } + + if (!mode) { + mode = 0o644; + } + } + + return mode; +} + // Normalises an entry's permissions in place so the extracting user can always // read, move and remove the result. Only the in-memory entry handed to // extract-zip is changed; the original zip file is never rewritten. -function ensureOwnerPermissions(entry) { - const mode = getEntryMode(entry); +function ensureOwnerPermissions(entry, opts) { + let mode = getEntryMode(entry); + const isDir = isDirectoryEntry(entry, mode); - // No POSIX mode bits are present. extract-zip falls back to its own default - // dir/file modes (0o755/0o644), which already grant the owner full access, - // so leave the entry untouched to preserve default behaviour. + // No POSIX mode bits are present, so extract-zip would synthesise the mode + // from defaultDirMode/defaultFileMode (falling back to 0o755/0o644). Resolve + // that same default here so the owner bits below are guaranteed even when the + // caller-supplied defaults omit them. if (mode === 0) { - return; + mode = resolveDefaultMode(isDir, opts); } - const isDir = isDirectoryEntry(entry, mode); const ownerBits = isDir ? OWNER_DIR_BITS : OWNER_FILE_BITS; let nextMode = mode | ownerBits; @@ -59,6 +87,8 @@ function ensureOwnerPermissions(entry) { nextMode = (nextMode & ~S_IFMT) | S_IFDIR; } + // The mode extract-zip would already produce grants the owner the required + // access and needs no type-bit fix, so leave the entry untouched. if (nextMode === mode) { return; } @@ -261,7 +291,7 @@ module.exports = async (zipToExtract, destination, options) => { // Normalize permissions last, immediately before extract-zip writes the // entry. Do not add async work here: extract-zip does not await onEntry. if (shouldEnsureOwnerPermissions) { - ensureOwnerPermissions(entry); + ensureOwnerPermissions(entry, opts); } }; diff --git a/packages/zip/test/zip.test.js b/packages/zip/test/zip.test.js index 2de0f0032..cdc09ff35 100644 --- a/packages/zip/test/zip.test.js +++ b/packages/zip/test/zip.test.js @@ -618,9 +618,9 @@ describe('Extract zip with ensureOwnerPermissions', function () { assert.equal(entry.externalFileAttributes & 0xffff, 0x10); }); - it('leaves entries without POSIX mode bits untouched', async function () { - // No POSIX mode bits at all: extract-zip applies its own defaults, so the - // entry must be left exactly as-is. + it('leaves entries without POSIX mode bits untouched when defaults already grant owner access', async function () { + // No POSIX mode bits and no custom defaults: extract-zip applies its own + // owner-accessible defaults (0o755/0o644), so the entry is left as-is. const entry = { fileName: 'no-mode.txt', externalFileAttributes: 0 }; const originalLoad = Module._load; @@ -643,6 +643,76 @@ describe('Extract zip with ensureOwnerPermissions', function () { assert.equal(entry.externalFileAttributes, 0); }); + it('guarantees owner permissions for zero-mode entries even when defaultDirMode/defaultFileMode omit them', async function () { + // Zero-mode entries (e.g. Windows / no-POSIX archives) whose mode would + // otherwise be synthesised from owner-less caller defaults. + const dirEntry = { fileName: 'win-dir/', externalFileAttributes: 0, versionMadeBy: 0 }; + const fileEntry = { fileName: 'win-file.txt', externalFileAttributes: 0, versionMadeBy: 0 }; + + const originalLoad = Module._load; + Module._load = function (request, parent, isMain) { + if (request === 'extract-zip') { + return async (zipPath, opts) => { + opts.onEntry(dirEntry, {}); + opts.onEntry(fileEntry, {}); + }; + } + + return originalLoad(request, parent, isMain); + }; + + try { + await extract(zipDestination, unzipDestination, { + ensureOwnerPermissions: true, + defaultDirMode: 0o555, + defaultFileMode: 0o444, + }); + } finally { + Module._load = originalLoad; + } + + const dirMode = (dirEntry.externalFileAttributes >> 16) & 0xffff; + const fileMode = (fileEntry.externalFileAttributes >> 16) & 0xffff; + + // Directory: owner rwx is guaranteed, the default r-x bits are preserved, + // and the POSIX directory type bit is set. + assert.equal(dirMode & 0o700, 0o700); + assert.equal(dirMode & 0o055, 0o055); + assert.equal(dirMode & 0o170000, 0o040000); + + // File: owner rw is guaranteed while the default read bits are preserved. + assert.equal(fileMode & 0o600, 0o600); + assert.equal(fileMode & 0o044, 0o044); + }); + + it('normalizes a zero-mode directory to the owner-accessible default when no defaults are supplied', async function () { + // A Windows-style directory entry with no POSIX mode bits and no caller + // defaults: extract-zip would fall back to 0o755, which already grants + // owner rwx, but we still stamp the directory type bit so it round-trips. + const dirEntry = { fileName: 'win-dir/', externalFileAttributes: 0, versionMadeBy: 0 }; + + const originalLoad = Module._load; + Module._load = function (request, parent, isMain) { + if (request === 'extract-zip') { + return async (zipPath, opts) => { + opts.onEntry(dirEntry, {}); + }; + } + + return originalLoad(request, parent, isMain); + }; + + try { + await extract(zipDestination, unzipDestination, { ensureOwnerPermissions: true }); + } finally { + Module._load = originalLoad; + } + + const dirMode = (dirEntry.externalFileAttributes >> 16) & 0xffff; + assert.equal(dirMode & 0o170000, 0o040000); + assert.equal(dirMode & 0o777, 0o755); + }); + it('still rejects symlink entries when ensureOwnerPermissions is enabled', async function () { await createZip(zipDestination, (archive) => { archive.symlink('themes/test-target', 'symlink-entry'); From 95bac4e8037567681c8af97d3b2d876357c8ba79 Mon Sep 17 00:00:00 2001 From: Sag Date: Thu, 25 Jun 2026 12:15:21 +0200 Subject: [PATCH 3/4] Mark optional extract() options as optional in JSDoc The JSDoc for `defaultDirMode`, `defaultFileMode`, `onEntry`, and the `limits` sub-fields omitted the `[ ]` optional markers, so the types generated from the JSDoc treated them as required. TypeScript consumers (e.g. gscan) could not call `extract(zip, dest, {ensureOwnerPermissions: true})` without also supplying those params. They all have defaults or are "if present", so they are now correctly marked optional. Co-Authored-By: Claude Opus 4.8 --- packages/zip/lib/extract.js | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/zip/lib/extract.js b/packages/zip/lib/extract.js index af7981fa9..6784628dd 100644 --- a/packages/zip/lib/extract.js +++ b/packages/zip/lib/extract.js @@ -244,12 +244,12 @@ function throwOnLargeFilenames(entry) { * @param {String} zipToExtract - full path to zip file that should be extracted * @param {String} destination - full path of the extraction target * @param {Object} [options] - * @param {Integer} options.defaultDirMode - Directory Mode (permissions), defaults to 0o755 - * @param {Integer} options.defaultFileMode - File Mode (permissions), defaults to 0o644 - * @param {Function} options.onEntry - if present, will be called with (entry, zipfile) for every entry in the zip + * @param {Integer} [options.defaultDirMode] - Directory Mode (permissions), defaults to 0o755 + * @param {Integer} [options.defaultFileMode] - File Mode (permissions), defaults to 0o644 + * @param {Function} [options.onEntry] - if present, will be called with (entry, zipfile) for every entry in the zip * @param {Object} [options.limits] - if present, sets maximum uncompressed sizes - * @param {Integer} options.limits.perEntryUncompressedBytes - maximum uncompressed size of each entry - * @param {Integer} options.limits.totalUncompressedBytes - maximum total uncompressed size across all entries + * @param {Integer} [options.limits.perEntryUncompressedBytes] - maximum uncompressed size of each entry + * @param {Integer} [options.limits.totalUncompressedBytes] - maximum total uncompressed size across all entries * @param {Boolean} [options.ensureOwnerPermissions] - when true, normalizes extracted entry permissions so the * owner can always read/move/remove the result: directories gain at least owner rwx and files at least owner rw, * while existing execute/group/world bits are preserved. The source zip is never modified. Defaults to false. From e83aaff77163596a8a10a6bf9c71330dd5bbdb31 Mon Sep 17 00:00:00 2001 From: Sag Date: Thu, 25 Jun 2026 14:13:55 +0200 Subject: [PATCH 4/4] Extract owner-permission logic into its own module for direct unit testing Moves the permission-normalization logic out of extract.js into: - lib/ensure-owner-permissions.js - the pure `(entry, opts)` normalizer - lib/entry-mode.js - shared getEntryMode() + POSIX stat constants This lets the permission tests assert on the function directly with synthetic entries instead of patching Module._load to mock extract-zip just to drive the onEntry callback. The four affected tests are now plain, synchronous unit tests with no module mocking, and the Windows directory-attribute path (previously impractical to reach through the extract() pipeline) is now covered too. Behavior is unchanged; the real-archiver extraction tests still cover the end-to-end path where extract-zip honors the rewritten attributes. Co-Authored-By: Claude Opus 4.8 --- packages/zip/lib/ensure-owner-permissions.js | 95 ++++++++++++ packages/zip/lib/entry-mode.js | 18 +++ packages/zip/lib/extract.js | 99 +------------ packages/zip/test/zip.test.js | 144 +++++++------------ 4 files changed, 164 insertions(+), 192 deletions(-) create mode 100644 packages/zip/lib/ensure-owner-permissions.js create mode 100644 packages/zip/lib/entry-mode.js diff --git a/packages/zip/lib/ensure-owner-permissions.js b/packages/zip/lib/ensure-owner-permissions.js new file mode 100644 index 000000000..64990871b --- /dev/null +++ b/packages/zip/lib/ensure-owner-permissions.js @@ -0,0 +1,95 @@ +const { S_IFMT, S_IFDIR, getEntryMode } = require('./entry-mode'); + +// Owner permission bits we guarantee when `ensureOwnerPermissions` is enabled. +const OWNER_DIR_BITS = 0o700; // rwx, so the tree can be traversed, written and removed +const OWNER_FILE_BITS = 0o600; // rw, so files can be read and rewritten/removed + +// Mirrors extract-zip's directory detection: POSIX directory type bit, a +// trailing slash, or the Windows directory attribute. +function isDirectoryEntry(entry, mode) { + if ((mode & S_IFMT) === S_IFDIR) { + return true; + } + + if (entry.fileName.endsWith('/')) { + return true; + } + + // Windows' way of specifying a directory + // https://github.com/maxogden/extract-zip/issues/13#issuecomment-154494566 + const madeBy = entry.versionMadeBy >> 8; + return madeBy === 0 && entry.externalFileAttributes === 16; +} + +// Resolves the mode extract-zip would synthesise for an entry that carries no +// POSIX mode bits, mirroring its getExtractedMode() fallback: the caller's +// defaultDirMode/defaultFileMode, or 0o755/0o644 when those are not set. +function resolveDefaultMode(isDir, opts) { + let mode = 0; + + if (isDir) { + if (opts.defaultDirMode) { + mode = parseInt(opts.defaultDirMode, 10); + } + + if (!mode) { + mode = 0o755; + } + } else { + if (opts.defaultFileMode) { + mode = parseInt(opts.defaultFileMode, 10); + } + + if (!mode) { + mode = 0o644; + } + } + + return mode; +} + +/** + * Normalises a zip entry's permissions in place so the extracting user can + * always read, move and remove the result: directories gain at least owner rwx + * and files at least owner rw, while existing execute/group/world bits are + * preserved. Only the in-memory entry handed to extract-zip is changed; the + * original zip file is never rewritten. + * + * @param {Object} entry - the yauzl entry extract-zip is about to extract + * @param {Object} [opts] - the extract options (read for defaultDirMode/defaultFileMode) + */ +function ensureOwnerPermissions(entry, opts = {}) { + let mode = getEntryMode(entry); + const isDir = isDirectoryEntry(entry, mode); + + // No POSIX mode bits are present, so extract-zip would synthesise the mode + // from defaultDirMode/defaultFileMode (falling back to 0o755/0o644). Resolve + // that same default here so the owner bits below are guaranteed even when the + // caller-supplied defaults omit them. + if (mode === 0) { + mode = resolveDefaultMode(isDir, opts); + } + + const ownerBits = isDir ? OWNER_DIR_BITS : OWNER_FILE_BITS; + let nextMode = mode | ownerBits; + + // A directory that was only inferred (trailing slash / Windows attribute) + // may lack the POSIX directory type bit. Set it so extract-zip still + // classifies the rewritten mode as a directory. + if (isDir && (nextMode & S_IFMT) !== S_IFDIR) { + nextMode = (nextMode & ~S_IFMT) | S_IFDIR; + } + + // The mode extract-zip would already produce grants the owner the required + // access and needs no type-bit fix, so leave the entry untouched. + if (nextMode === mode) { + return; + } + + // Write the new POSIX mode back into the high 16 bits while preserving the + // low 16 bits (DOS attributes and other zip-specific flags). + const lowBits = entry.externalFileAttributes & 0xffff; + entry.externalFileAttributes = (((nextMode & 0xffff) << 16) | lowBits) >>> 0; +} + +module.exports = ensureOwnerPermissions; diff --git a/packages/zip/lib/entry-mode.js b/packages/zip/lib/entry-mode.js new file mode 100644 index 000000000..c100d709b --- /dev/null +++ b/packages/zip/lib/entry-mode.js @@ -0,0 +1,18 @@ +// POSIX `stat` mode constants, matching the ones extract-zip uses internally +// when it derives an entry's type and permissions from its external attributes. +const S_IFMT = 0o170000; // bit mask for the file type bit fields +const S_IFDIR = 0o040000; // directory +const S_IFLNK = 0o120000; // symbolic link + +// Reads the POSIX mode an entry carries in the high 16 bits of its external +// file attributes, exactly as extract-zip does when extracting. +function getEntryMode(entry) { + return (entry.externalFileAttributes >> 16) & 0xffff; +} + +module.exports = { + S_IFMT, + S_IFDIR, + S_IFLNK, + getEntryMode, +}; diff --git a/packages/zip/lib/extract.js b/packages/zip/lib/extract.js index 6784628dd..1a4d995fc 100644 --- a/packages/zip/lib/extract.js +++ b/packages/zip/lib/extract.js @@ -1,104 +1,9 @@ const errors = require('@tryghost/errors'); +const { S_IFMT, S_IFLNK, getEntryMode } = require('./entry-mode'); +const ensureOwnerPermissions = require('./ensure-owner-permissions'); const defaultOptions = {}; -// POSIX `stat` mode constants, matching the ones extract-zip uses internally -// when it derives an entry's type and permissions from its external attributes. -const S_IFMT = 0o170000; // bit mask for the file type bit fields -const S_IFDIR = 0o040000; // directory -const S_IFLNK = 0o120000; // symbolic link - -// Owner permission bits we guarantee when `ensureOwnerPermissions` is enabled. -const OWNER_DIR_BITS = 0o700; // rwx, so the tree can be traversed, written and removed -const OWNER_FILE_BITS = 0o600; // rw, so files can be read and rewritten/removed - -// Reads the POSIX mode an entry carries in the high 16 bits of its external -// file attributes, exactly as extract-zip does when extracting. -function getEntryMode(entry) { - return (entry.externalFileAttributes >> 16) & 0xffff; -} - -// Mirrors extract-zip's directory detection: POSIX directory type bit, a -// trailing slash, or the Windows directory attribute. -function isDirectoryEntry(entry, mode) { - if ((mode & S_IFMT) === S_IFDIR) { - return true; - } - - if (entry.fileName.endsWith('/')) { - return true; - } - - // Windows' way of specifying a directory - // https://github.com/maxogden/extract-zip/issues/13#issuecomment-154494566 - const madeBy = entry.versionMadeBy >> 8; - return madeBy === 0 && entry.externalFileAttributes === 16; -} - -// Resolves the mode extract-zip would synthesise for an entry that carries no -// POSIX mode bits, mirroring its getExtractedMode() fallback: the caller's -// defaultDirMode/defaultFileMode, or 0o755/0o644 when those are not set. -function resolveDefaultMode(isDir, opts) { - let mode = 0; - - if (isDir) { - if (opts.defaultDirMode) { - mode = parseInt(opts.defaultDirMode, 10); - } - - if (!mode) { - mode = 0o755; - } - } else { - if (opts.defaultFileMode) { - mode = parseInt(opts.defaultFileMode, 10); - } - - if (!mode) { - mode = 0o644; - } - } - - return mode; -} - -// Normalises an entry's permissions in place so the extracting user can always -// read, move and remove the result. Only the in-memory entry handed to -// extract-zip is changed; the original zip file is never rewritten. -function ensureOwnerPermissions(entry, opts) { - let mode = getEntryMode(entry); - const isDir = isDirectoryEntry(entry, mode); - - // No POSIX mode bits are present, so extract-zip would synthesise the mode - // from defaultDirMode/defaultFileMode (falling back to 0o755/0o644). Resolve - // that same default here so the owner bits below are guaranteed even when the - // caller-supplied defaults omit them. - if (mode === 0) { - mode = resolveDefaultMode(isDir, opts); - } - - const ownerBits = isDir ? OWNER_DIR_BITS : OWNER_FILE_BITS; - let nextMode = mode | ownerBits; - - // A directory that was only inferred (trailing slash / Windows attribute) - // may lack the POSIX directory type bit. Set it so extract-zip still - // classifies the rewritten mode as a directory. - if (isDir && (nextMode & S_IFMT) !== S_IFDIR) { - nextMode = (nextMode & ~S_IFMT) | S_IFDIR; - } - - // The mode extract-zip would already produce grants the owner the required - // access and needs no type-bit fix, so leave the entry untouched. - if (nextMode === mode) { - return; - } - - // Write the new POSIX mode back into the high 16 bits while preserving the - // low 16 bits (DOS attributes and other zip-specific flags). - const lowBits = entry.externalFileAttributes & 0xffff; - entry.externalFileAttributes = (((nextMode & 0xffff) << 16) | lowBits) >>> 0; -} - function normalizeLimit(value, fieldName) { if (value === undefined || value === null || value === Infinity) { return Infinity; diff --git a/packages/zip/test/zip.test.js b/packages/zip/test/zip.test.js index cdc09ff35..e35246075 100644 --- a/packages/zip/test/zip.test.js +++ b/packages/zip/test/zip.test.js @@ -5,6 +5,7 @@ const { hashElement } = require('folder-hash'); const EventEmitter = require('events'); const Module = require('module'); const createArchive = require('../lib/create-archive'); +const ensureOwnerPermissions = require('../lib/ensure-owner-permissions'); // Mimic how we expect this to be required const { compress, extract } = require('../'); @@ -583,7 +584,31 @@ describe('Extract zip with ensureOwnerPermissions', function () { assert.equal(bits & 0o555, 0o555); }); - it('sets the directory type bit on inferred directories that lack POSIX type bits', async function () { + it('still rejects symlink entries when ensureOwnerPermissions is enabled', async function () { + await createZip(zipDestination, (archive) => { + archive.symlink('themes/test-target', 'symlink-entry'); + }); + + await assert.rejects( + () => + extract(zipDestination, unzipDestination, { + ensureOwnerPermissions: true, + }), + (err) => { + assert.equal(err.errorType, 'UnsupportedMediaTypeError'); + assert.equal(err.message, 'Symlinks are not allowed in the zip folder.'); + assert.equal(err.code, 'SYMLINK_NOT_ALLOWED'); + assert.deepEqual(err.errorDetails, { + entryName: 'themes/test-target', + }); + return true; + }, + ); + }); +}); + +describe('ensureOwnerPermissions', function () { + it('sets the directory type bit on inferred directories that lack POSIX type bits', function () { // A directory recognised only by its trailing slash, carrying 0o555 // permission bits but no POSIX directory type bit, plus a low DOS bit. const entry = { @@ -592,84 +617,36 @@ describe('Extract zip with ensureOwnerPermissions', function () { versionMadeBy: 0x0300, // unix host, so the Windows directory rule does not apply }; - const originalLoad = Module._load; - Module._load = function (request, parent, isMain) { - if (request === 'extract-zip') { - return async (zipPath, opts) => { - opts.onEntry(entry, {}); - }; - } + ensureOwnerPermissions(entry); - return originalLoad(request, parent, isMain); - }; - - try { - await extract(zipDestination, unzipDestination, { ensureOwnerPermissions: true }); - } finally { - Module._load = originalLoad; - } - - const normalizedMode = (entry.externalFileAttributes >> 16) & 0xffff; + const mode = (entry.externalFileAttributes >> 16) & 0xffff; // The directory type bit is now present... - assert.equal(normalizedMode & 0o170000, 0o040000); + assert.equal(mode & 0o170000, 0o040000); // ...owner gains rwx while the original r-x bits are preserved. - assert.equal(normalizedMode & 0o777, 0o755); + assert.equal(mode & 0o777, 0o755); // Low DOS attribute bits are preserved. assert.equal(entry.externalFileAttributes & 0xffff, 0x10); }); - it('leaves entries without POSIX mode bits untouched when defaults already grant owner access', async function () { + it('leaves entries without POSIX mode bits untouched when defaults already grant owner access', function () { // No POSIX mode bits and no custom defaults: extract-zip applies its own // owner-accessible defaults (0o755/0o644), so the entry is left as-is. const entry = { fileName: 'no-mode.txt', externalFileAttributes: 0 }; - const originalLoad = Module._load; - Module._load = function (request, parent, isMain) { - if (request === 'extract-zip') { - return async (zipPath, opts) => { - opts.onEntry(entry, {}); - }; - } - - return originalLoad(request, parent, isMain); - }; - - try { - await extract(zipDestination, unzipDestination, { ensureOwnerPermissions: true }); - } finally { - Module._load = originalLoad; - } + ensureOwnerPermissions(entry); assert.equal(entry.externalFileAttributes, 0); }); - it('guarantees owner permissions for zero-mode entries even when defaultDirMode/defaultFileMode omit them', async function () { + it('guarantees owner permissions for zero-mode entries even when defaultDirMode/defaultFileMode omit them', function () { // Zero-mode entries (e.g. Windows / no-POSIX archives) whose mode would // otherwise be synthesised from owner-less caller defaults. const dirEntry = { fileName: 'win-dir/', externalFileAttributes: 0, versionMadeBy: 0 }; const fileEntry = { fileName: 'win-file.txt', externalFileAttributes: 0, versionMadeBy: 0 }; + const opts = { defaultDirMode: 0o555, defaultFileMode: 0o444 }; - const originalLoad = Module._load; - Module._load = function (request, parent, isMain) { - if (request === 'extract-zip') { - return async (zipPath, opts) => { - opts.onEntry(dirEntry, {}); - opts.onEntry(fileEntry, {}); - }; - } - - return originalLoad(request, parent, isMain); - }; - - try { - await extract(zipDestination, unzipDestination, { - ensureOwnerPermissions: true, - defaultDirMode: 0o555, - defaultFileMode: 0o444, - }); - } finally { - Module._load = originalLoad; - } + ensureOwnerPermissions(dirEntry, opts); + ensureOwnerPermissions(fileEntry, opts); const dirMode = (dirEntry.externalFileAttributes >> 16) & 0xffff; const fileMode = (fileEntry.externalFileAttributes >> 16) & 0xffff; @@ -685,53 +662,30 @@ describe('Extract zip with ensureOwnerPermissions', function () { assert.equal(fileMode & 0o044, 0o044); }); - it('normalizes a zero-mode directory to the owner-accessible default when no defaults are supplied', async function () { + it('normalizes a zero-mode directory to the owner-accessible default when no defaults are supplied', function () { // A Windows-style directory entry with no POSIX mode bits and no caller // defaults: extract-zip would fall back to 0o755, which already grants // owner rwx, but we still stamp the directory type bit so it round-trips. const dirEntry = { fileName: 'win-dir/', externalFileAttributes: 0, versionMadeBy: 0 }; - const originalLoad = Module._load; - Module._load = function (request, parent, isMain) { - if (request === 'extract-zip') { - return async (zipPath, opts) => { - opts.onEntry(dirEntry, {}); - }; - } - - return originalLoad(request, parent, isMain); - }; - - try { - await extract(zipDestination, unzipDestination, { ensureOwnerPermissions: true }); - } finally { - Module._load = originalLoad; - } + ensureOwnerPermissions(dirEntry); const dirMode = (dirEntry.externalFileAttributes >> 16) & 0xffff; assert.equal(dirMode & 0o170000, 0o040000); assert.equal(dirMode & 0o777, 0o755); }); - it('still rejects symlink entries when ensureOwnerPermissions is enabled', async function () { - await createZip(zipDestination, (archive) => { - archive.symlink('themes/test-target', 'symlink-entry'); - }); + it('recognizes a directory via the Windows directory attribute (no trailing slash)', function () { + // No POSIX type bit and no trailing slash: detected as a directory only + // by the Windows directory attribute (externalFileAttributes === 16, + // versionMadeBy host byte === 0). + const dirEntry = { fileName: 'win-dir', externalFileAttributes: 16, versionMadeBy: 0 }; - await assert.rejects( - () => - extract(zipDestination, unzipDestination, { - ensureOwnerPermissions: true, - }), - (err) => { - assert.equal(err.errorType, 'UnsupportedMediaTypeError'); - assert.equal(err.message, 'Symlinks are not allowed in the zip folder.'); - assert.equal(err.code, 'SYMLINK_NOT_ALLOWED'); - assert.deepEqual(err.errorDetails, { - entryName: 'themes/test-target', - }); - return true; - }, - ); + ensureOwnerPermissions(dirEntry); + + const dirMode = (dirEntry.externalFileAttributes >> 16) & 0xffff; + // Treated as a directory: type bit set and owner rwx guaranteed. + assert.equal(dirMode & 0o170000, 0o040000); + assert.equal(dirMode & 0o700, 0o700); }); });