From 0f45e3bfada0b8e3087bccf7cef82658f3430527 Mon Sep 17 00:00:00 2001 From: Alexandre Pereira Date: Fri, 2 Oct 2026 12:02:38 +0100 Subject: [PATCH 1/4] fix(host-kit): keep extracted directories owner-accessible An archive could declare a directory without owner read access (e.g. 0o300). Recursive mkdir applied that mode to implied parents too, so cleanup's recursive rm failed with EACCES: a failed extraction reported the cleanup error instead of its own and leaked the temp tree, and a successful one leaked it after install. Entry modes now come from one rule shared by zip and tar: declared permission bits only, at least owner rwx for directories and rw for files. Implied parents are created with the default mode. Adds adversarial extraction tests pinning link, special-entry, escaping-path, PAX override, duplicate, case-collision, and set-id handling, none of which were covered. --- .../src/internal/archive-extraction-tar.ts | 11 +- .../src/internal/archive-extraction-zip.ts | 13 +- .../internal/archive-extraction.fixtures.ts | 77 +++++- .../src/internal/archive-extraction.test.ts | 259 ++++++++++++++++++ .../src/internal/archive-safety.test.ts | 15 + .../host-kit/src/internal/archive-safety.ts | 30 ++ 6 files changed, 391 insertions(+), 14 deletions(-) create mode 100644 packages/host-kit/src/internal/archive-extraction.test.ts diff --git a/packages/host-kit/src/internal/archive-extraction-tar.ts b/packages/host-kit/src/internal/archive-extraction-tar.ts index 0ab3f10bd5..87127db10f 100644 --- a/packages/host-kit/src/internal/archive-extraction-tar.ts +++ b/packages/host-kit/src/internal/archive-extraction-tar.ts @@ -6,6 +6,8 @@ import * as tar from 'tar-stream'; import { ArchiveBudget, archiveError, + createExtractedDirectory, + extractedEntryMode, normalizeArchiveEntryName, reserveArchiveManifest, resolveArchiveOutputPath, @@ -120,11 +122,6 @@ function readTarKind(type: tar.Headers['type']): 'directory' | 'file' { throw archiveError('ARCHIVE_UNSAFE_ENTRY', 'Archive contains a link or special entry'); } -function safeMode(mode: number | undefined, kind: 'directory' | 'file'): number { - const fallback = kind === 'directory' ? 0o755 : 0o644; - return (mode ?? fallback) & 0o777; -} - function manifestEntryFromTarHeader(header: tar.Headers): ArchiveManifestEntry | undefined { if (isRootDirectoryMarker(header.name, header.type)) return undefined; const name = normalizeArchiveEntryName(header.name); @@ -133,7 +130,7 @@ function manifestEntryFromTarHeader(header: tar.Headers): ArchiveManifestEntry | if (!Number.isSafeInteger(size) || size < 0) { throw archiveError('ARCHIVE_INVALID_ENTRY', 'Archive entry has an invalid size'); } - return { name, kind, size, mode: safeMode(header.mode, kind) }; + return { name, kind, size, mode: extractedEntryMode(kind, header.mode) }; } async function writeTarEntry( @@ -144,7 +141,7 @@ async function writeTarEntry( ): Promise { const outputPath = resolveArchiveOutputPath(outputRoot, manifestEntry.name); if (manifestEntry.kind === 'directory') { - await fs.mkdir(outputPath, { recursive: true, mode: manifestEntry.mode }); + await createExtractedDirectory(outputPath, manifestEntry.mode); await drainTarEntry(entry); return; } diff --git a/packages/host-kit/src/internal/archive-extraction-zip.ts b/packages/host-kit/src/internal/archive-extraction-zip.ts index c3fd285644..01475739bf 100644 --- a/packages/host-kit/src/internal/archive-extraction-zip.ts +++ b/packages/host-kit/src/internal/archive-extraction-zip.ts @@ -5,6 +5,8 @@ import * as yauzl from 'yauzl'; import { ArchiveBudget, archiveError, + createExtractedDirectory, + extractedEntryMode, normalizeArchiveEntryName, reserveArchiveManifest, resolveArchiveOutputPath, @@ -59,7 +61,7 @@ export async function extractZipArchive(options: ZipOptions): Promise { } const outputPath = resolveArchiveOutputPath(options.outputRoot, expected.name); if (expected.kind === 'directory') { - await fs.mkdir(outputPath, { recursive: true, mode: expected.mode }); + await createExtractedDirectory(outputPath, expected.mode); } else { await fs.mkdir(path.dirname(outputPath), { recursive: true }); const source = await openEntryStream(zipFile, entry); @@ -152,13 +154,12 @@ function toManifestEntry(entry: yauzl.Entry): ArchiveManifestEntry { assertZipEntryIsReadable(entry); const name = normalizeArchiveEntryName(entry.fileName); const unixMode = (entry.externalFileAttributes >>> 16) & 0xffff; - const directory = isZipDirectory(entry.fileName, unixMode); - const fallback = directory ? 0o755 : 0o644; + const kind = isZipDirectory(entry.fileName, unixMode) ? 'directory' : 'file'; return { name, - kind: directory ? 'directory' : 'file', - size: directory ? 0 : entry.uncompressedSize, - mode: unixMode & 0o777 || fallback, + kind, + size: kind === 'directory' ? 0 : entry.uncompressedSize, + mode: extractedEntryMode(kind, unixMode), }; } diff --git a/packages/host-kit/src/internal/archive-extraction.fixtures.ts b/packages/host-kit/src/internal/archive-extraction.fixtures.ts index dbcfa63e13..92a92febbc 100644 --- a/packages/host-kit/src/internal/archive-extraction.fixtures.ts +++ b/packages/host-kit/src/internal/archive-extraction.fixtures.ts @@ -1,7 +1,7 @@ import { promises as fs } from 'node:fs'; import path from 'node:path'; -import { gzipSync } from 'node:zlib'; +import { crc32, gzipSync } from 'node:zlib'; import * as tar from 'tar-stream'; import { runCmdSync } from './exec.ts'; import { mkdtempForTest } from './tmp-dir.fixtures.ts'; @@ -35,3 +35,78 @@ export async function createZipWithEncryptedSecondEntry(archivePath: string): Pr runCmdSync('zip', ['-q', archivePath, 'first.txt'], { cwd: staging }); runCmdSync('zip', ['-q', '-P', 'secret', archivePath, 'second.txt'], { cwd: staging }); } + +/** One stored zip entry; `mode` is the full Unix `st_mode`, file-type bits included. */ +export type ZipFixtureEntry = { name: string; mode: number; data?: string }; + +/** + * Writes a stored (uncompressed) zip whose names and Unix attributes are exactly as given, which + * `zip` itself refuses to produce for links, special files, and escaping names. + */ +export async function writeZipFixture( + archivePath: string, + entries: readonly ZipFixtureEntry[], +): Promise { + const localRecords: Buffer[] = []; + const centralRecords: Buffer[] = []; + let offset = 0; + for (const entry of entries) { + const name = Buffer.from(entry.name, 'utf8'); + const data = Buffer.from(entry.data ?? '', 'utf8'); + const checksum = crc32(data); + const local = Buffer.alloc(30); + local.writeUInt32LE(0x04034b50, 0); + local.writeUInt16LE(20, 4); + local.writeUInt16LE(0x0800, 6); + local.writeUInt32LE(checksum, 14); + local.writeUInt32LE(data.length, 18); + local.writeUInt32LE(data.length, 22); + local.writeUInt16LE(name.length, 26); + const central = Buffer.alloc(46); + central.writeUInt32LE(0x02014b50, 0); + central.writeUInt16LE((3 << 8) | 20, 4); + central.writeUInt16LE(20, 6); + central.writeUInt16LE(0x0800, 8); + central.writeUInt32LE(checksum, 16); + central.writeUInt32LE(data.length, 20); + central.writeUInt32LE(data.length, 24); + central.writeUInt16LE(name.length, 28); + central.writeUInt32LE((entry.mode << 16) >>> 0, 38); + central.writeUInt32LE(offset, 42); + localRecords.push(local, name, data); + centralRecords.push(central, name); + offset += local.length + name.length + data.length; + } + const centralDirectory = Buffer.concat(centralRecords); + const end = Buffer.alloc(22); + end.writeUInt32LE(0x06054b50, 0); + end.writeUInt16LE(entries.length, 8); + end.writeUInt16LE(entries.length, 10); + end.writeUInt32LE(centralDirectory.length, 12); + end.writeUInt32LE(offset, 16); + await fs.writeFile(archivePath, Buffer.concat([...localRecords, centralDirectory, end])); +} + +/** `pax` records are written as a PAX extended header; tar-stream's typings omit the field. */ +export type TarFixtureEntry = { + header: tar.Headers; + pax?: Record; + data?: string; +}; + +export async function writeTarFixture( + archivePath: string, + entries: readonly TarFixtureEntry[], +): Promise { + const pack = tar.pack(); + for (const entry of entries) { + const header: tar.Headers & { pax?: Record } = entry.pax + ? { ...entry.header, pax: entry.pax } + : entry.header; + pack.entry(header, Buffer.from(entry.data ?? '')); + } + pack.finalize(); + const chunks: Buffer[] = []; + for await (const chunk of pack) chunks.push(Buffer.from(chunk)); + await fs.writeFile(archivePath, Buffer.concat(chunks)); +} diff --git a/packages/host-kit/src/internal/archive-extraction.test.ts b/packages/host-kit/src/internal/archive-extraction.test.ts new file mode 100644 index 0000000000..21149dedea --- /dev/null +++ b/packages/host-kit/src/internal/archive-extraction.test.ts @@ -0,0 +1,259 @@ +import assert from 'node:assert/strict'; +import { promises as fs } from 'node:fs'; +import path from 'node:path'; +import { test } from 'vitest'; +import { AppError } from '@agent-device/kernel/errors'; +import { extractArchiveSafely, type SupportedArchiveType } from './archive-extraction.ts'; +import { + writeTarFixture, + writeZipFixture, + type TarFixtureEntry, + type ZipFixtureEntry, +} from './archive-extraction.fixtures.ts'; +import { mkdtempForTest } from './tmp-dir.fixtures.ts'; + +const S_IFREG = 0o100000; +const S_IFDIR = 0o040000; +const S_IFLNK = 0o120000; +const S_IFIFO = 0o010000; + +type ArchiveFixture = + | { type: 'zip'; entries: readonly ZipFixtureEntry[] } + | { type: 'tar'; entries: readonly TarFixtureEntry[] }; + +async function extractFixture(fixture: ArchiveFixture): Promise<{ + root: string; + outputRoot: string; + error: unknown; +}> { + const root = await mkdtempForTest('agent-device-archive-extraction-'); + const archivePath = path.join(root, `fixture.${fixture.type}`); + if (fixture.type === 'zip') await writeZipFixture(archivePath, fixture.entries); + else await writeTarFixture(archivePath, fixture.entries); + const outputRoot = path.join(root, 'extracted'); + const type: SupportedArchiveType = fixture.type; + const error = await extractArchiveSafely({ archivePath, outputRoot, type }).then( + () => undefined, + (error: unknown) => error, + ); + return { root, outputRoot, error }; +} + +function reason(error: unknown): unknown { + return error instanceof AppError ? error.details?.reason : undefined; +} + +async function pathExists(target: string): Promise { + return await fs.lstat(target).then( + () => true, + () => false, + ); +} + +const REFUSED_ENTRIES: ReadonlyArray<{ + label: string; + fixture: ArchiveFixture; + reason?: string; +}> = [ + { + label: 'zip symlink', + fixture: { type: 'zip', entries: [{ name: 'App.app/link', mode: S_IFLNK | 0o777, data: '/' }] }, + reason: 'ARCHIVE_UNSAFE_ENTRY', + }, + { + label: 'zip FIFO', + fixture: { type: 'zip', entries: [{ name: 'App.app/fifo', mode: S_IFIFO | 0o644 }] }, + reason: 'ARCHIVE_UNSAFE_ENTRY', + }, + { + label: 'zip parent-escaping path', + fixture: { + type: 'zip', + entries: [{ name: 'App.app/../../escaped', mode: S_IFREG | 0o644, data: 'x' }], + }, + }, + { + label: 'zip absolute path', + fixture: { type: 'zip', entries: [{ name: '/escaped', mode: S_IFREG | 0o644, data: 'x' }] }, + }, + { + label: 'tar symlink', + fixture: { + type: 'tar', + entries: [{ header: { name: 'App.app/link', type: 'symlink', linkname: '/' } }], + }, + reason: 'ARCHIVE_UNSAFE_ENTRY', + }, + { + label: 'tar hard link', + fixture: { + type: 'tar', + entries: [{ header: { name: 'App.app/link', type: 'link', linkname: '/etc/hosts' } }], + }, + reason: 'ARCHIVE_UNSAFE_ENTRY', + }, + { + label: 'tar character device', + fixture: { + type: 'tar', + entries: [{ header: { name: 'App.app/dev', type: 'character-device' } }], + }, + reason: 'ARCHIVE_UNSAFE_ENTRY', + }, + { + label: 'tar FIFO', + fixture: { type: 'tar', entries: [{ header: { name: 'App.app/fifo', type: 'fifo' } }] }, + reason: 'ARCHIVE_UNSAFE_ENTRY', + }, + { + label: 'tar parent-escaping path', + fixture: { type: 'tar', entries: [{ header: { name: '../escaped' }, data: 'x' }] }, + reason: 'ARCHIVE_UNSAFE_PATH', + }, + { + label: 'tar PAX path override', + fixture: { + type: 'tar', + entries: [{ header: { name: 'App.app/safe' }, pax: { path: '../../escaped' }, data: 'x' }], + }, + reason: 'ARCHIVE_UNSAFE_PATH', + }, + { + label: 'tar PAX linkpath symlink', + fixture: { + type: 'tar', + entries: [ + { + header: { name: 'App.app/link', type: 'symlink', linkname: 'x' }, + pax: { linkpath: '/' }, + }, + ], + }, + reason: 'ARCHIVE_UNSAFE_ENTRY', + }, +]; + +test.each(REFUSED_ENTRIES)( + '$label is refused and nothing is written', + async ({ fixture, reason: expectedReason }) => { + const { root, outputRoot, error } = await extractFixture(fixture); + + assert.ok(error instanceof Error, 'extraction must be refused'); + if (expectedReason) assert.equal(reason(error), expectedReason); + assert.equal(await pathExists(outputRoot), false); + assert.deepEqual((await fs.readdir(root)).sort(), [`fixture.${fixture.type}`]); + assert.equal(await pathExists(path.join(path.dirname(root), 'escaped')), false); + assert.equal(await pathExists('/escaped'), false); + }, +); + +test.each([ + { + type: 'zip', + entries: [ + { name: 'App.app/a', mode: S_IFREG | 0o644, data: 'first' }, + { name: 'App.app/a', mode: S_IFREG | 0o644, data: 'second' }, + ], + }, + { + type: 'tar', + entries: [ + { header: { name: 'App.app/a' }, data: 'first' }, + { header: { name: 'App.app/a' }, data: 'second' }, + ], + }, +])('a duplicate $type entry is refused instead of overwriting the first', async (fixture) => { + const { outputRoot, error } = await extractFixture(fixture); + + assert.equal((error as NodeJS.ErrnoException).code, 'EEXIST'); + assert.equal(await pathExists(outputRoot), false); +}); + +test('case-colliding zip entries never overwrite each other', async () => { + const { outputRoot, error } = await extractFixture({ + type: 'zip', + entries: [ + { name: 'App.app/Name', mode: S_IFREG | 0o644, data: 'upper' }, + { name: 'App.app/name', mode: S_IFREG | 0o644, data: 'lower' }, + ], + }); + + if (error === undefined) { + assert.equal(await fs.readFile(path.join(outputRoot, 'App.app/Name'), 'utf8'), 'upper'); + assert.equal(await fs.readFile(path.join(outputRoot, 'App.app/name'), 'utf8'), 'lower'); + } else { + assert.equal((error as NodeJS.ErrnoException).code, 'EEXIST'); + assert.equal(await pathExists(outputRoot), false); + } +}); + +test.each([ + { type: 'zip', entries: [{ name: 'App.app/App', mode: S_IFREG | 0o7755, data: 'bin' }] }, + { type: 'tar', entries: [{ header: { name: 'App.app/App', mode: 0o7755 }, data: 'bin' }] }, +])('$type extraction strips set-id and sticky bits but keeps execute bits', async (fixture) => { + const { outputRoot, error } = await extractFixture(fixture); + + assert.equal(error, undefined); + const mode = (await fs.stat(path.join(outputRoot, 'App.app/App'))).mode; + assert.equal(mode & 0o7000, 0); + assert.equal(mode & 0o100, 0o100); +}); + +const UNREADABLE_DIRECTORY = { + zip: { name: 'App.app/locked/', mode: S_IFDIR | 0o300 }, + tar: { header: { name: 'App.app/locked', type: 'directory', mode: 0o300 } }, +} as const; + +test.each([ + { + type: 'zip', + entries: [ + UNREADABLE_DIRECTORY.zip, + { name: 'App.app/locked/payload', mode: S_IFREG | 0o644, data: 'payload' }, + ], + }, + { + type: 'tar', + entries: [ + UNREADABLE_DIRECTORY.tar, + { header: { name: 'App.app/locked/payload' }, data: 'payload' }, + ], + }, +])('a $type directory declared without owner access stays removable', async (fixture) => { + const { outputRoot, error } = await extractFixture(fixture); + + assert.equal(error, undefined); + for (const directory of ['App.app', 'App.app/locked']) { + const mode = (await fs.stat(path.join(outputRoot, directory))).mode; + assert.equal(mode & 0o700, 0o700, `${directory} must stay owner-accessible`); + } + await fs.rm(outputRoot, { recursive: true }); + assert.equal(await pathExists(outputRoot), false); +}); + +test.each([ + { + type: 'zip', + entries: [ + UNREADABLE_DIRECTORY.zip, + { name: 'App.app/locked/payload', mode: S_IFREG | 0o644, data: 'payload' }, + { name: 'App.app/locked/payload', mode: S_IFREG | 0o644, data: 'payload' }, + ], + }, + { + type: 'tar', + entries: [ + UNREADABLE_DIRECTORY.tar, + { header: { name: 'App.app/locked/payload' }, data: 'payload' }, + { header: { name: 'App.app/locked/payload' }, data: 'payload' }, + ], + }, +])( + 'a failed $type extraction under an unreadable directory reports its own error and leaves nothing', + async (fixture) => { + const { outputRoot, error } = await extractFixture(fixture); + + assert.equal((error as NodeJS.ErrnoException).code, 'EEXIST'); + assert.equal(await pathExists(outputRoot), false); + }, +); diff --git a/packages/host-kit/src/internal/archive-safety.test.ts b/packages/host-kit/src/internal/archive-safety.test.ts index 22697566e9..96956a3cb4 100644 --- a/packages/host-kit/src/internal/archive-safety.test.ts +++ b/packages/host-kit/src/internal/archive-safety.test.ts @@ -3,6 +3,7 @@ import { test } from 'vitest'; import { AppError } from '@agent-device/kernel/errors'; import { ArchiveBudget, + extractedEntryMode, normalizeArchiveEntryName, resolveArchiveOutputPath, } from './archive-safety.ts'; @@ -79,3 +80,17 @@ test('archive reservation rejects bytes or entries beyond its inspected manifest (error) => reason(error) === 'ARCHIVE_MANIFEST_MISMATCH', ); }); + +test('every declared mode extracts owner-accessible with set-id and sticky bits stripped', () => { + const ownerAccess = { directory: 0o700, file: 0o600 } as const; + for (const kind of ['directory', 'file'] as const) { + for (let declared = 0; declared <= 0o177777; declared += 1) { + const mode = extractedEntryMode(kind, declared); + assert.equal(mode & ~0o777, 0, `${kind} ${declared.toString(8)} kept non-permission bits`); + assert.equal(mode & ownerAccess[kind], ownerAccess[kind]); + if (declared & 0o777) assert.equal(mode & 0o077, declared & 0o077); + } + } + assert.equal(extractedEntryMode('directory', undefined), 0o755); + assert.equal(extractedEntryMode('file', undefined), 0o644); +}); diff --git a/packages/host-kit/src/internal/archive-safety.ts b/packages/host-kit/src/internal/archive-safety.ts index 892fc51961..c536c97be6 100644 --- a/packages/host-kit/src/internal/archive-safety.ts +++ b/packages/host-kit/src/internal/archive-safety.ts @@ -1,3 +1,4 @@ +import { promises as fs } from 'node:fs'; import path from 'node:path'; import { AppError } from '@agent-device/kernel/errors'; import { @@ -179,6 +180,35 @@ export function resolveArchiveOutputPath(outputRoot: string, entryName: string): return resolvedEntry; } +const DEFAULT_ENTRY_PERMISSIONS = { directory: 0o755, file: 0o644 } as const; +const OWNER_ENTRY_ACCESS = { directory: 0o700, file: 0o600 } as const; + +/** + * The mode an extracted entry is written with: declared permission bits only, never set-id or + * sticky bits, and always enough owner access that the extraction root can be removed recursively. + */ +export function extractedEntryMode( + kind: ArchiveManifestEntry['kind'], + declaredMode: number | undefined, +): number { + const permissions = (declaredMode ?? 0) & 0o777; + return (permissions || DEFAULT_ENTRY_PERMISSIONS[kind]) | OWNER_ENTRY_ACCESS[kind]; +} + +/** + * Creates one directory entry with its own mode. Parents an archive never declared are created + * with the default mode, and a directory an earlier entry already implied is accepted as is. + */ +export async function createExtractedDirectory(outputPath: string, mode: number): Promise { + await fs.mkdir(path.dirname(outputPath), { recursive: true }); + try { + await fs.mkdir(outputPath, { mode }); + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== 'EEXIST') throw error; + if (!(await fs.lstat(outputPath)).isDirectory()) throw error; + } +} + export function archiveError(reason: string, message: string, cause?: unknown): AppError { return new AppError('INVALID_ARGS', message, { reason }, cause); } From 4b1b5948a3fe474532db49c3e161d8b7471a9de4 Mon Sep 17 00:00:00 2001 From: Alexandre Pereira Date: Fri, 2 Oct 2026 13:00:19 +0100 Subject: [PATCH 2/4] fix(host-kit): apply declared archive modes regardless of entry order A directory entry that follows its children now takes its declared mode instead of keeping the implied default, so entry order no longer changes extracted permissions. An explicit mode of zero is honored as owner access only; only an absent mode falls back to the default, and zip entries without Unix attributes still get it. The tar fixture can now encode a zero mode, which tar-stream otherwise replaces with its default. Tests cover both entry orders, zero modes, attribute-less zip entries, and a directory colliding with an earlier file. --- .../src/internal/archive-extraction-zip.ts | 2 +- .../internal/archive-extraction.fixtures.ts | 44 ++++++++- .../src/internal/archive-extraction.test.ts | 89 +++++++++++++++++++ .../src/internal/archive-safety.test.ts | 8 +- .../host-kit/src/internal/archive-safety.ts | 10 ++- 5 files changed, 146 insertions(+), 7 deletions(-) diff --git a/packages/host-kit/src/internal/archive-extraction-zip.ts b/packages/host-kit/src/internal/archive-extraction-zip.ts index 01475739bf..2d279cd16f 100644 --- a/packages/host-kit/src/internal/archive-extraction-zip.ts +++ b/packages/host-kit/src/internal/archive-extraction-zip.ts @@ -159,7 +159,7 @@ function toManifestEntry(entry: yauzl.Entry): ArchiveManifestEntry { name, kind, size: kind === 'directory' ? 0 : entry.uncompressedSize, - mode: extractedEntryMode(kind, unixMode), + mode: extractedEntryMode(kind, unixMode === 0 ? undefined : unixMode), }; } diff --git a/packages/host-kit/src/internal/archive-extraction.fixtures.ts b/packages/host-kit/src/internal/archive-extraction.fixtures.ts index 92a92febbc..bf7f593f7a 100644 --- a/packages/host-kit/src/internal/archive-extraction.fixtures.ts +++ b/packages/host-kit/src/internal/archive-extraction.fixtures.ts @@ -99,14 +99,54 @@ export async function writeTarFixture( entries: readonly TarFixtureEntry[], ): Promise { const pack = tar.pack(); + const zeroModeNames = new Set(); for (const entry of entries) { + if (entry.header.mode === 0) zeroModeNames.add(entry.header.name); const header: tar.Headers & { pax?: Record } = entry.pax ? { ...entry.header, pax: entry.pax } - : entry.header; + : { ...entry.header }; pack.entry(header, Buffer.from(entry.data ?? '')); } pack.finalize(); const chunks: Buffer[] = []; for await (const chunk of pack) chunks.push(Buffer.from(chunk)); - await fs.writeFile(archivePath, Buffer.concat(chunks)); + const archive = Buffer.concat(chunks); + restoreZeroTarModes(archive, zeroModeNames); + await fs.writeFile(archivePath, archive); +} + +const TAR_BLOCK = 512; + +/** + * tar-stream's pack replaces a zero mode with its default, so a declared zero is written back into + * the emitted ustar header, checksum included. + */ +function restoreZeroTarModes(archive: Buffer, names: ReadonlySet): void { + if (names.size === 0) return; + for (let offset = 0; offset + TAR_BLOCK <= archive.length;) { + const block = archive.subarray(offset, offset + TAR_BLOCK); + const name = readTarField(block, 0, 100); + if (!name) return; + if (names.has(name.replace(/\/$/, ''))) { + block.write('000000 ', 100, 'ascii'); + block.write(`${tarChecksum(block).toString(8).padStart(6, '0')} `, 148, 'ascii'); + } + const size = Number.parseInt(readTarField(block, 124, 12), 8) || 0; + offset += TAR_BLOCK + Math.ceil(size / TAR_BLOCK) * TAR_BLOCK; + } +} + +function readTarField(block: Buffer, start: number, length: number): string { + return block + .toString('ascii', start, start + length) + .split('\0')[0]! + .trim(); +} + +function tarChecksum(block: Buffer): number { + let sum = 8 * 0x20; + for (let index = 0; index < TAR_BLOCK; index += 1) { + if (index < 148 || index >= 156) sum += block[index]!; + } + return sum; } diff --git a/packages/host-kit/src/internal/archive-extraction.test.ts b/packages/host-kit/src/internal/archive-extraction.test.ts index 21149dedea..83fdaf42ac 100644 --- a/packages/host-kit/src/internal/archive-extraction.test.ts +++ b/packages/host-kit/src/internal/archive-extraction.test.ts @@ -199,6 +199,95 @@ test.each([ assert.equal(mode & 0o100, 0o100); }); +test.each([ + { + type: 'zip', + entries: [ + { name: 'App.app/d/child', mode: S_IFREG | 0o644, data: 'x' }, + { name: 'App.app/d/', mode: S_IFDIR | 0o700 }, + ], + }, + { + type: 'tar', + entries: [ + { header: { name: 'App.app/d/child' }, data: 'x' }, + { header: { name: 'App.app/d', type: 'directory', mode: 0o700 } }, + ], + }, +])( + 'a $type directory declared after its children still gets its declared mode', + async (fixture) => { + const { outputRoot, error } = await extractFixture(fixture); + + assert.equal(error, undefined); + assert.equal((await fs.stat(path.join(outputRoot, 'App.app/d'))).mode & 0o777, 0o700); + }, +); + +test.each([ + { + type: 'zip', + entries: [ + { name: 'App.app/private/', mode: S_IFDIR }, + { name: 'App.app/private/secret', mode: S_IFREG, data: 'x' }, + ], + }, + { + type: 'tar', + entries: [ + { header: { name: 'App.app/private', type: 'directory', mode: 0 } }, + { header: { name: 'App.app/private/secret', mode: 0 }, data: 'x' }, + ], + }, +])('an explicit $type mode of zero extracts with owner access only', async (fixture) => { + const { outputRoot, error } = await extractFixture(fixture); + + assert.equal(error, undefined); + assert.equal((await fs.stat(path.join(outputRoot, 'App.app/private'))).mode & 0o777, 0o700); + assert.equal( + (await fs.stat(path.join(outputRoot, 'App.app/private/secret'))).mode & 0o777, + 0o600, + ); +}); + +test('a zip entry without Unix attributes gets the default mode', async () => { + const { outputRoot, error } = await extractFixture({ + type: 'zip', + entries: [ + { name: 'App.app/dir/', mode: 0 }, + { name: 'App.app/dir/file', mode: 0, data: 'x' }, + ], + }); + + assert.equal(error, undefined); + const directory = (await fs.stat(path.join(outputRoot, 'App.app/dir'))).mode & 0o777; + const file = (await fs.stat(path.join(outputRoot, 'App.app/dir/file'))).mode & 0o777; + assert.equal(directory, 0o755 & ~process.umask()); + assert.equal(file, 0o644 & ~process.umask()); +}); + +test.each([ + { + type: 'zip', + entries: [ + { name: 'App.app/x', mode: S_IFREG | 0o644, data: 'file' }, + { name: 'App.app/x/', mode: S_IFDIR | 0o755 }, + ], + }, + { + type: 'tar', + entries: [ + { header: { name: 'App.app/x' }, data: 'file' }, + { header: { name: 'App.app/x', type: 'directory' } }, + ], + }, +])('a $type directory colliding with an earlier file is refused', async (fixture) => { + const { outputRoot, error } = await extractFixture(fixture); + + assert.equal((error as NodeJS.ErrnoException).code, 'EEXIST'); + assert.equal(await pathExists(outputRoot), false); +}); + const UNREADABLE_DIRECTORY = { zip: { name: 'App.app/locked/', mode: S_IFDIR | 0o300 }, tar: { header: { name: 'App.app/locked', type: 'directory', mode: 0o300 } }, diff --git a/packages/host-kit/src/internal/archive-safety.test.ts b/packages/host-kit/src/internal/archive-safety.test.ts index 96956a3cb4..aae6ff0e83 100644 --- a/packages/host-kit/src/internal/archive-safety.test.ts +++ b/packages/host-kit/src/internal/archive-safety.test.ts @@ -88,9 +88,15 @@ test('every declared mode extracts owner-accessible with set-id and sticky bits const mode = extractedEntryMode(kind, declared); assert.equal(mode & ~0o777, 0, `${kind} ${declared.toString(8)} kept non-permission bits`); assert.equal(mode & ownerAccess[kind], ownerAccess[kind]); - if (declared & 0o777) assert.equal(mode & 0o077, declared & 0o077); + assert.equal( + mode & 0o077, + declared & 0o077, + `${kind} ${declared.toString(8)} changed group/other bits`, + ); } } assert.equal(extractedEntryMode('directory', undefined), 0o755); assert.equal(extractedEntryMode('file', undefined), 0o644); + assert.equal(extractedEntryMode('directory', 0), 0o700); + assert.equal(extractedEntryMode('file', 0), 0o600); }); diff --git a/packages/host-kit/src/internal/archive-safety.ts b/packages/host-kit/src/internal/archive-safety.ts index c536c97be6..8c64ec7893 100644 --- a/packages/host-kit/src/internal/archive-safety.ts +++ b/packages/host-kit/src/internal/archive-safety.ts @@ -186,18 +186,21 @@ const OWNER_ENTRY_ACCESS = { directory: 0o700, file: 0o600 } as const; /** * The mode an extracted entry is written with: declared permission bits only, never set-id or * sticky bits, and always enough owner access that the extraction root can be removed recursively. + * An entry that declares no mode at all gets the default; an explicit mode of zero is honored. */ export function extractedEntryMode( kind: ArchiveManifestEntry['kind'], declaredMode: number | undefined, ): number { - const permissions = (declaredMode ?? 0) & 0o777; - return (permissions || DEFAULT_ENTRY_PERMISSIONS[kind]) | OWNER_ENTRY_ACCESS[kind]; + const permissions = + declaredMode === undefined ? DEFAULT_ENTRY_PERMISSIONS[kind] : declaredMode & 0o777; + return permissions | OWNER_ENTRY_ACCESS[kind]; } /** * Creates one directory entry with its own mode. Parents an archive never declared are created - * with the default mode, and a directory an earlier entry already implied is accepted as is. + * with the default mode; a directory an earlier entry already implied takes the declared mode, so + * entry order does not change the extracted permissions. */ export async function createExtractedDirectory(outputPath: string, mode: number): Promise { await fs.mkdir(path.dirname(outputPath), { recursive: true }); @@ -206,6 +209,7 @@ export async function createExtractedDirectory(outputPath: string, mode: number) } catch (error) { if ((error as NodeJS.ErrnoException).code !== 'EEXIST') throw error; if (!(await fs.lstat(outputPath)).isDirectory()) throw error; + await fs.chmod(outputPath, mode); } } From 85fec9324a4d6c71bb4cce5cef8be8dfd3b4681a Mon Sep 17 00:00:00 2001 From: Alexandre Pereira Date: Fri, 2 Oct 2026 13:28:59 +0100 Subject: [PATCH 3/4] test(host-kit): restore zero tar modes by entry order Matching headers by name would patch every duplicate when one declares a zero mode and depended on the 100-byte name field. The fixture now patches by emitted entry order, skipping the PAX extended header that precedes an entry, and the zero-mode tar test carries a PAX record to prove the skip. --- .../internal/archive-extraction.fixtures.ts | 40 +++++++++---------- .../src/internal/archive-extraction.test.ts | 6 ++- 2 files changed, 24 insertions(+), 22 deletions(-) diff --git a/packages/host-kit/src/internal/archive-extraction.fixtures.ts b/packages/host-kit/src/internal/archive-extraction.fixtures.ts index bf7f593f7a..fe56667598 100644 --- a/packages/host-kit/src/internal/archive-extraction.fixtures.ts +++ b/packages/host-kit/src/internal/archive-extraction.fixtures.ts @@ -99,50 +99,48 @@ export async function writeTarFixture( entries: readonly TarFixtureEntry[], ): Promise { const pack = tar.pack(); - const zeroModeNames = new Set(); - for (const entry of entries) { - if (entry.header.mode === 0) zeroModeNames.add(entry.header.name); + const zeroModeEntries = new Set(); + entries.forEach((entry, index) => { + if (entry.header.mode === 0) zeroModeEntries.add(index); const header: tar.Headers & { pax?: Record } = entry.pax ? { ...entry.header, pax: entry.pax } : { ...entry.header }; pack.entry(header, Buffer.from(entry.data ?? '')); - } + }); pack.finalize(); const chunks: Buffer[] = []; for await (const chunk of pack) chunks.push(Buffer.from(chunk)); const archive = Buffer.concat(chunks); - restoreZeroTarModes(archive, zeroModeNames); + restoreZeroTarModes(archive, zeroModeEntries); await fs.writeFile(archivePath, archive); } const TAR_BLOCK = 512; +const PAX_TYPEFLAGS = new Set(['x', 'g']); /** * tar-stream's pack replaces a zero mode with its default, so a declared zero is written back into - * the emitted ustar header, checksum included. + * the emitted ustar header, checksum included. Entries are matched by emitted order; the PAX + * extended header tar-stream emits before an entry is not counted. */ -function restoreZeroTarModes(archive: Buffer, names: ReadonlySet): void { - if (names.size === 0) return; +function restoreZeroTarModes(archive: Buffer, zeroModeEntries: ReadonlySet): void { + if (zeroModeEntries.size === 0) return; + let entryIndex = 0; for (let offset = 0; offset + TAR_BLOCK <= archive.length;) { const block = archive.subarray(offset, offset + TAR_BLOCK); - const name = readTarField(block, 0, 100); - if (!name) return; - if (names.has(name.replace(/\/$/, ''))) { - block.write('000000 ', 100, 'ascii'); - block.write(`${tarChecksum(block).toString(8).padStart(6, '0')} `, 148, 'ascii'); + if (block.every((byte) => byte === 0)) return; + if (!PAX_TYPEFLAGS.has(String.fromCharCode(block[156]!))) { + if (zeroModeEntries.has(entryIndex)) { + block.write('000000 ', 100, 'ascii'); + block.write(`${tarChecksum(block).toString(8).padStart(6, '0')} `, 148, 'ascii'); + } + entryIndex += 1; } - const size = Number.parseInt(readTarField(block, 124, 12), 8) || 0; + const size = Number.parseInt(block.toString('ascii', 124, 136).split('\0')[0]!, 8) || 0; offset += TAR_BLOCK + Math.ceil(size / TAR_BLOCK) * TAR_BLOCK; } } -function readTarField(block: Buffer, start: number, length: number): string { - return block - .toString('ascii', start, start + length) - .split('\0')[0]! - .trim(); -} - function tarChecksum(block: Buffer): number { let sum = 8 * 0x20; for (let index = 0; index < TAR_BLOCK; index += 1) { diff --git a/packages/host-kit/src/internal/archive-extraction.test.ts b/packages/host-kit/src/internal/archive-extraction.test.ts index 83fdaf42ac..dfb074d30e 100644 --- a/packages/host-kit/src/internal/archive-extraction.test.ts +++ b/packages/host-kit/src/internal/archive-extraction.test.ts @@ -236,7 +236,11 @@ test.each([ type: 'tar', entries: [ { header: { name: 'App.app/private', type: 'directory', mode: 0 } }, - { header: { name: 'App.app/private/secret', mode: 0 }, data: 'x' }, + { + header: { name: 'App.app/private/secret', mode: 0 }, + pax: { path: 'App.app/private/secret' }, + data: 'x', + }, ], }, ])('an explicit $type mode of zero extracts with owner access only', async (fixture) => { From 5d9f400aaecf2d7813626fce96b4045fd2c4800b Mon Sep 17 00:00:00 2001 From: Alexandre Pereira Date: Fri, 2 Oct 2026 13:36:58 +0100 Subject: [PATCH 4/4] fix(host-kit): apply the umask to extracted directories in both branches mkdir applies the process umask to its mode and chmod does not, so a directory declared after its children extracted with more permissions than the same directory declared before them. Both branches now chmod to the declared mode under the umask, with owner access kept, matching what the kernel does for files. The entry-order test declares 0o770 and expects it under the umask in both orders. --- .../src/internal/archive-extraction.test.ts | 36 ++++++++++--------- .../host-kit/src/internal/archive-safety.ts | 9 ++--- 2 files changed, 24 insertions(+), 21 deletions(-) diff --git a/packages/host-kit/src/internal/archive-extraction.test.ts b/packages/host-kit/src/internal/archive-extraction.test.ts index dfb074d30e..e50336efae 100644 --- a/packages/host-kit/src/internal/archive-extraction.test.ts +++ b/packages/host-kit/src/internal/archive-extraction.test.ts @@ -199,28 +199,30 @@ test.each([ assert.equal(mode & 0o100, 0o100); }); -test.each([ - { - type: 'zip', - entries: [ - { name: 'App.app/d/child', mode: S_IFREG | 0o644, data: 'x' }, - { name: 'App.app/d/', mode: S_IFDIR | 0o700 }, - ], - }, - { - type: 'tar', - entries: [ - { header: { name: 'App.app/d/child' }, data: 'x' }, - { header: { name: 'App.app/d', type: 'directory', mode: 0o700 } }, - ], - }, +const GROUP_WRITABLE_DIRECTORY = { + zip: { name: 'App.app/d/', mode: S_IFDIR | 0o770 }, + tar: { header: { name: 'App.app/d', type: 'directory', mode: 0o770 } }, +} as const; +const DIRECTORY_CHILD = { + zip: { name: 'App.app/d/child', mode: S_IFREG | 0o644, data: 'x' }, + tar: { header: { name: 'App.app/d/child' }, data: 'x' }, +} as const; + +test.each([ + { type: 'zip', order: 'before', entries: [GROUP_WRITABLE_DIRECTORY.zip, DIRECTORY_CHILD.zip] }, + { type: 'zip', order: 'after', entries: [DIRECTORY_CHILD.zip, GROUP_WRITABLE_DIRECTORY.zip] }, + { type: 'tar', order: 'before', entries: [GROUP_WRITABLE_DIRECTORY.tar, DIRECTORY_CHILD.tar] }, + { type: 'tar', order: 'after', entries: [DIRECTORY_CHILD.tar, GROUP_WRITABLE_DIRECTORY.tar] }, ])( - 'a $type directory declared after its children still gets its declared mode', + 'a $type directory declared $order its children gets its declared mode under the umask', async (fixture) => { const { outputRoot, error } = await extractFixture(fixture); assert.equal(error, undefined); - assert.equal((await fs.stat(path.join(outputRoot, 'App.app/d'))).mode & 0o777, 0o700); + assert.equal( + (await fs.stat(path.join(outputRoot, 'App.app/d'))).mode & 0o777, + 0o770 & ~process.umask(), + ); }, ); diff --git a/packages/host-kit/src/internal/archive-safety.ts b/packages/host-kit/src/internal/archive-safety.ts index 8c64ec7893..01324e34bd 100644 --- a/packages/host-kit/src/internal/archive-safety.ts +++ b/packages/host-kit/src/internal/archive-safety.ts @@ -199,18 +199,19 @@ export function extractedEntryMode( /** * Creates one directory entry with its own mode. Parents an archive never declared are created - * with the default mode; a directory an earlier entry already implied takes the declared mode, so - * entry order does not change the extracted permissions. + * with the default mode. Whether the directory is new or an earlier entry already implied it, it + * ends up with the declared mode under the process umask, as the kernel applies to files, so entry + * order does not change the extracted permissions. */ export async function createExtractedDirectory(outputPath: string, mode: number): Promise { await fs.mkdir(path.dirname(outputPath), { recursive: true }); try { - await fs.mkdir(outputPath, { mode }); + await fs.mkdir(outputPath); } catch (error) { if ((error as NodeJS.ErrnoException).code !== 'EEXIST') throw error; if (!(await fs.lstat(outputPath)).isDirectory()) throw error; - await fs.chmod(outputPath, mode); } + await fs.chmod(outputPath, (mode & ~process.umask()) | OWNER_ENTRY_ACCESS.directory); } export function archiveError(reason: string, message: string, cause?: unknown): AppError {