From 3cfba9848d76a53ff16b77782cf2901c0d00341b Mon Sep 17 00:00:00 2001 From: Kim T Date: Thu, 30 Jul 2026 22:23:20 -0700 Subject: [PATCH 1/6] [refactor] Split helpers/file.ts along its actual seams file.ts conflated four unrelated concerns in one 670+ line module: generic fs primitives, archive extraction/creation (incl. filesMove's package-format sorting), OS-default install-directory resolution, and privileged installer execution/admin elevation. Split into fs.ts, archive.ts, paths.ts, and installer.ts along those seams. file.ts itself becomes a pure re-export barrel (`export * from './fs.js'` etc.), so every existing import of '../helpers/file.js' - across ManagerLocal.ts's remaining callers, all test files, and the package's public index.ts/index-browser.ts exports - keeps working unchanged with zero behavior difference. Updated the primary internal consumers (ManagerLocal.ts, ConfigLocal.ts, RegistryLocal.ts, admin.ts, packageLocal.ts) to import from the specific new module they actually need instead of the barrel, since that's the actual point of the split - a reader can now tell what a file touches (generic fs vs. archive handling vs. privileged execution) from its imports alone. NOTE: local verification of this branch is currently blocked - see conversation for details on an unrelated live-registry-data-drift issue being fixed on other branches first. `npx tsc --noEmit` and `eslint .` both pass cleanly; full `npm test` has not yet been re-verified after rebasing onto the updated main. --- src/classes/ConfigLocal.ts | 2 +- src/classes/ManagerLocal.ts | 9 +- src/classes/RegistryLocal.ts | 2 +- src/helpers/admin.ts | 2 +- src/helpers/archive.ts | 196 ++++++++++ src/helpers/file.ts | 683 +---------------------------------- src/helpers/fs.ts | 240 ++++++++++++ src/helpers/installer.ts | 208 +++++++++++ src/helpers/packageLocal.ts | 2 +- src/helpers/paths.ts | 63 ++++ 10 files changed, 725 insertions(+), 682 deletions(-) create mode 100644 src/helpers/archive.ts create mode 100644 src/helpers/fs.ts create mode 100644 src/helpers/installer.ts create mode 100644 src/helpers/paths.ts diff --git a/src/classes/ConfigLocal.ts b/src/classes/ConfigLocal.ts index b4346ca..6d2be06 100644 --- a/src/classes/ConfigLocal.ts +++ b/src/classes/ConfigLocal.ts @@ -1,6 +1,6 @@ import path from 'path'; import { Config } from './Config.js'; -import { dirCreate, fileCreateJson, fileCreateYaml, fileDelete, fileExists, fileReadJson } from '../helpers/file.js'; +import { dirCreate, fileCreateJson, fileCreateYaml, fileDelete, fileExists, fileReadJson } from '../helpers/fs.js'; import { ConfigInterface } from '../types/Config.js'; import { configDefaultsLocal } from '../helpers/configLocal.js'; diff --git a/src/classes/ManagerLocal.ts b/src/classes/ManagerLocal.ts index b944b4e..b293579 100644 --- a/src/classes/ManagerLocal.ts +++ b/src/classes/ManagerLocal.ts @@ -3,8 +3,8 @@ import { Package } from './Package.js'; import { PackageVersion } from '../types/Package.js'; import { Manager } from './Manager.js'; import { Architecture } from '../types/Architecture.js'; +import { archiveExtract, filesMove } from '../helpers/archive.js'; import { - archiveExtract, dirCreate, dirDelete, dirEmpty, @@ -18,14 +18,11 @@ import { fileExec, fileExists, fileHash, - fileInstall, fileOpen, fileReadJson, fileReadYaml, - filesMove, - isAdmin, - runCliAsAdmin, -} from '../helpers/file.js'; +} from '../helpers/fs.js'; +import { fileInstall, isAdmin, runCliAsAdmin } from '../helpers/installer.js'; import { isValidGithubRepo, isValidSlug, isValidVersion, pathGetSlug, pathGetVersion } from '../helpers/utils.js'; import { commandExists, getArchitecture, getSystem, isTests } from '../helpers/utilsLocal.js'; import { apiBuffer } from '../helpers/api.js'; diff --git a/src/classes/RegistryLocal.ts b/src/classes/RegistryLocal.ts index 5f28f92..2cc01b1 100644 --- a/src/classes/RegistryLocal.ts +++ b/src/classes/RegistryLocal.ts @@ -1,7 +1,7 @@ import path from 'path'; import { ManagerLocal } from './ManagerLocal.js'; import { Registry } from './Registry.js'; -import { dirCreate, fileCreateJson, fileCreateYaml } from '../helpers/file.js'; +import { dirCreate, fileCreateJson, fileCreateYaml } from '../helpers/fs.js'; export class RegistryLocal extends Registry { protected override managers: Record; diff --git a/src/helpers/admin.ts b/src/helpers/admin.ts index b6516f8..278e476 100644 --- a/src/helpers/admin.ts +++ b/src/helpers/admin.ts @@ -9,7 +9,7 @@ import { RegistryType } from '../types/Registry.js'; import { ManagerLocal } from '../classes/ManagerLocal.js'; -import { dirApp } from './file.js'; +import { dirApp } from './paths.js'; export interface Arguments { appDir: string; diff --git a/src/helpers/archive.ts b/src/helpers/archive.ts new file mode 100644 index 0000000..83b20f0 --- /dev/null +++ b/src/helpers/archive.ts @@ -0,0 +1,196 @@ +import AdmZip from 'adm-zip'; +import { unlinkSync, writeFileSync } from 'fs'; +import { list, unpack } from '7zip-min'; +import * as tar from 'tar'; +import path from 'path'; +import mime from 'mime-types'; +import { SystemType } from '../types/SystemType.js'; +import { getSystem } from './utilsLocal.js'; +import { log } from './utils.js'; +import { dirContains, dirCreate, dirIs, dirRead, fileExec, fileExists, fileMove } from './fs.js'; + +// Archive extraction/creation, and filesMove()'s package-format sorting of an extracted +// archive's contents - the two are grouped together since filesMove() only ever runs +// immediately after archiveExtract() (see ManagerLocal.install()/helpers/installTargets.ts), on +// content this module itself just produced. + +// Rejects the "zip slip" pattern: an archive entry name like `../../../etc/passwd` or an +// absolute path that, once joined to the extraction directory, resolves outside of it. +export function isSafeArchiveEntryPath(entryName: string, targetRoot: string): boolean { + return dirContains(targetRoot, path.resolve(targetRoot, entryName)); +} + +export async function archiveExtract(filePath: string, dirPath: string) { + log('⎋', dirPath); + const fileName = path.basename(filePath).toLowerCase(); + const ext = path.extname(filePath).trim().toLowerCase(); + const targetRoot = path.resolve(dirPath); + + const tarExtensions = ['.tar', '.gz', '.tgz', '.xz', '.bz2', '.tbz2']; + const tarCompoundExtensions = ['.tar.gz', '.tar.xz', '.tar.bz2']; + const isTarFile = + tarExtensions.includes(ext) || tarCompoundExtensions.some(compoundExt => fileName.endsWith(compoundExt)); + + if (ext === '.zip') { + const zip: AdmZip = new AdmZip(filePath); + try { + // adm-zip's extractAllTo already guards against zip-slip internally (its sanitize()/ + // canonical() helpers fall back to the entry's basename if it would otherwise resolve + // outside the target directory) - this is the normal, non-fallback path. + return zip.extractAllTo(dirPath); + } catch (error: any) { + // Handle Windows special character issues by extracting files manually + if (getSystem() === SystemType.Win && error.message?.includes('ENOENT')) { + log('⚠️', 'Extracting files manually due to special characters in filenames'); + const entries = zip.getEntries(); + entries.forEach(entry => { + const sanitizedName: string = entry.entryName.replace(/[<>:"|?*]/g, '_').replace(/[\r\n]/g, ''); + // This manual path builds destinations by hand instead of going through adm-zip's own + // sanitize(), so it must enforce the same containment itself - stripping `<>:"|?*` and + // newlines does nothing to stop a `..`-based traversal. + if (!isSafeArchiveEntryPath(sanitizedName, targetRoot)) { + throw new Error(`Archive entry escapes extraction directory: ${entry.entryName}`); + } + const outputPath = path.join(dirPath, sanitizedName); + if (!entry.isDirectory) { + dirCreate(path.dirname(outputPath)); + writeFileSync(outputPath, entry.getData()); + } else { + dirCreate(outputPath); + } + }); + return; + } + } + } else if (isTarFile) { + // node-tar requires cwd to already exist, unlike AdmZip/7zip-min which create their + // own target directory. + dirCreate(dirPath); + // node-tar rejects '..' path segments and relativizes absolute paths by default + // (preservePaths is false unless explicitly opted into), so no extra check is needed here. + return await tar.extract({ + file: filePath, + cwd: dirPath, + }); + } else if (ext === '.7z') { + // Unlike adm-zip/node-tar, 7zip-min just shells out to the 7za binary with no per-entry + // containment logic of its own, and there's no way to sanitize an entry's destination + // mid-extraction. List the archive's contents first and refuse to extract at all if any + // entry would escape the target directory. + const entries: Array<{ name?: string }> = await new Promise((resolve, reject) => { + list(filePath, (err: any, result: any) => (err ? reject(err) : resolve(result || []))); + }); + const unsafeEntry = entries.find(entry => entry.name && !isSafeArchiveEntryPath(entry.name, targetRoot)); + if (unsafeEntry) { + throw new Error(`Archive entry escapes extraction directory: ${unsafeEntry.name}`); + } + return new Promise((resolve, reject) => { + unpack(filePath, dirPath, (err2: any) => { + if (err2) + return reject(new Error(`7z extraction failed: ${err2 && err2.message ? err2.message : String(err2)}`)); + return resolve(); + }); + }); + } +} + +export function filesMove(dirSource: string, dirTarget: string, dirSub: string, formatDir: Record) { + const filesAndFolders: string[] = dirRead(`${dirSource}/**/*`); + log('filesAndFolders', filesAndFolders); + + // First pass: identify bundle directories (app, clap, vst3, lv2, etc.) + const bundleDirs: Set = new Set(); + filesAndFolders.forEach(f => { + if (dirIs(f)) { + // Check if this is a macOS application bundle or plugin bundle + if (fileExists(path.join(f, 'Contents', 'Info.plist'))) { + bundleDirs.add(f); + } + // Check if this is an LV2 plugin folder + if (fileExists(path.join(f, 'manifest.ttl'))) { + bundleDirs.add(f); + } + // VST3 bundles on Linux (and some Windows builds) are directories without a macOS + // Info.plist, so they must be recognized by extension alone. + if (path.extname(f).slice(1).toLowerCase() === 'vst3') { + bundleDirs.add(f); + } + } + }); + + const files = filesAndFolders.filter(f => { + // Exclude files/folders that are inside bundle directories + for (const bundleDir of bundleDirs) { + if (f.startsWith(bundleDir + path.sep)) { + return false; // This path is inside a bundle, exclude it + } + } + + // Include regular files (not directories). + if (!dirIs(f)) return true; + + // Include bundle directories themselves (already identified above). + if (bundleDirs.has(f)) return true; + + // Otherwise ignore. + return false; + }); + const filesMoved: string[] = []; + log('files', files); + + // For each file, move to correct folder based on type + files.forEach((fileSource: string) => { + const fileExt: string = path.extname(fileSource).slice(1).toLowerCase(); + let fileExtTarget = formatDir[fileExt]; + + // Use mime-type detection as fallback for unmapped extensions + if (!fileExtTarget) { + const mimeType = mime.lookup(fileSource) || ''; + if (!mimeType || mimeType.startsWith('application/')) { + fileExtTarget = 'App'; + } + } + + // If this is not a supported file format, then ignore. + if (fileExtTarget === undefined) + return log(`${fileSource} - ${fileExt || 'no extension'} not mapped to a installation folder, skipping.`); + const fileTarget: string = path.join(dirTarget, fileExtTarget, dirSub, path.basename(fileSource)); + if (fileExists(fileTarget)) return log(`${fileSource} - ${fileTarget} already exists, skipping.`); + dirCreate(path.dirname(fileTarget)); + fileMove(fileSource, fileTarget); + // Set executable permissions for executable file types + if (fileExt === 'app') { + // For .app bundles, find and set permissions on the actual executable + const executablePath = path.join(fileTarget, 'Contents', 'MacOS', path.basename(fileTarget, '.app')); + if (fileExists(executablePath)) { + fileExec(executablePath); + } + } else if (['elf', 'exe', ''].includes(fileExt)) { + fileExec(fileTarget); + } + filesMoved.push(fileTarget); + }); + return filesMoved; +} + +export function zipCreate(filesPath: string, zipPath: string): void { + if (fileExists(zipPath)) { + unlinkSync(zipPath); + } + const zip: AdmZip = new AdmZip(); + const pathList: string[] = dirRead(filesPath); + pathList.forEach(pathItem => { + log('⎋', pathItem); + try { + if (dirIs(pathItem)) { + zip.addLocalFolder(pathItem, path.basename(pathItem)); + } else { + zip.addLocalFile(pathItem); + } + } catch (error) { + log(error); + } + }); + log('+', zipPath); + return zip.writeZip(zipPath); +} diff --git a/src/helpers/file.ts b/src/helpers/file.ts index 3dabd0a..09390b2 100644 --- a/src/helpers/file.ts +++ b/src/helpers/file.ts @@ -1,672 +1,11 @@ -import AdmZip from 'adm-zip'; -import { execFileSync, spawn } from 'child_process'; -import { - createReadStream, - chmodSync, - existsSync, - mkdirSync, - readdirSync, - readFileSync, - rmSync, - statSync, - unlinkSync, - writeFileSync, -} from 'fs'; -import { createHash } from 'crypto'; -import { list, unpack } from '7zip-min'; -import stream from 'stream/promises'; -import { GlobOptionsWithFileTypesFalse, globSync } from 'glob'; -import { moveSync } from 'fs-extra/esm'; -import os from 'os'; -import * as tar from 'tar'; -import path, { dirname } from 'path'; -import yaml from 'js-yaml'; -import { ZodIssueCode, ZodParsedType } from 'zod'; -import { PackageInterface } from '../types/Package.js'; -import { PluginFile } from '../types/Plugin.js'; -import { PresetFile } from '../types/Preset.js'; -import { ProjectFile } from '../types/Project.js'; -import { ZodIssue } from 'zod'; -import { SystemType } from '../types/SystemType.js'; -import { fileURLToPath } from 'url'; -import sudoPrompt from '@vscode/sudo-prompt'; -import { getSystem } from './utilsLocal.js'; -import { log } from './utils.js'; -import mime from 'mime-types'; - -// Rejects the "zip slip" pattern: an archive entry name like `../../../etc/passwd` or an -// absolute path that, once joined to the extraction directory, resolves outside of it. -export function isSafeArchiveEntryPath(entryName: string, targetRoot: string): boolean { - return dirContains(targetRoot, path.resolve(targetRoot, entryName)); -} - -export async function archiveExtract(filePath: string, dirPath: string) { - log('⎋', dirPath); - const fileName = path.basename(filePath).toLowerCase(); - const ext = path.extname(filePath).trim().toLowerCase(); - const targetRoot = path.resolve(dirPath); - - const tarExtensions = ['.tar', '.gz', '.tgz', '.xz', '.bz2', '.tbz2']; - const tarCompoundExtensions = ['.tar.gz', '.tar.xz', '.tar.bz2']; - const isTarFile = - tarExtensions.includes(ext) || tarCompoundExtensions.some(compoundExt => fileName.endsWith(compoundExt)); - - if (ext === '.zip') { - const zip: AdmZip = new AdmZip(filePath); - try { - // adm-zip's extractAllTo already guards against zip-slip internally (its sanitize()/ - // canonical() helpers fall back to the entry's basename if it would otherwise resolve - // outside the target directory) - this is the normal, non-fallback path. - return zip.extractAllTo(dirPath); - } catch (error: any) { - // Handle Windows special character issues by extracting files manually - if (getSystem() === SystemType.Win && error.message?.includes('ENOENT')) { - log('⚠️', 'Extracting files manually due to special characters in filenames'); - const entries = zip.getEntries(); - entries.forEach(entry => { - const sanitizedName: string = entry.entryName.replace(/[<>:"|?*]/g, '_').replace(/[\r\n]/g, ''); - // This manual path builds destinations by hand instead of going through adm-zip's own - // sanitize(), so it must enforce the same containment itself - stripping `<>:"|?*` and - // newlines does nothing to stop a `..`-based traversal. - if (!isSafeArchiveEntryPath(sanitizedName, targetRoot)) { - throw new Error(`Archive entry escapes extraction directory: ${entry.entryName}`); - } - const outputPath = path.join(dirPath, sanitizedName); - if (!entry.isDirectory) { - dirCreate(path.dirname(outputPath)); - writeFileSync(outputPath, entry.getData()); - } else { - dirCreate(outputPath); - } - }); - return; - } - } - } else if (isTarFile) { - // node-tar requires cwd to already exist, unlike AdmZip/7zip-min which create their - // own target directory. - dirCreate(dirPath); - // node-tar rejects '..' path segments and relativizes absolute paths by default - // (preservePaths is false unless explicitly opted into), so no extra check is needed here. - return await tar.extract({ - file: filePath, - cwd: dirPath, - }); - } else if (ext === '.7z') { - // Unlike adm-zip/node-tar, 7zip-min just shells out to the 7za binary with no per-entry - // containment logic of its own, and there's no way to sanitize an entry's destination - // mid-extraction. List the archive's contents first and refuse to extract at all if any - // entry would escape the target directory. - const entries: Array<{ name?: string }> = await new Promise((resolve, reject) => { - list(filePath, (err: any, result: any) => (err ? reject(err) : resolve(result || []))); - }); - const unsafeEntry = entries.find(entry => entry.name && !isSafeArchiveEntryPath(entry.name, targetRoot)); - if (unsafeEntry) { - throw new Error(`Archive entry escapes extraction directory: ${unsafeEntry.name}`); - } - return new Promise((resolve, reject) => { - unpack(filePath, dirPath, (err2: any) => { - if (err2) - return reject(new Error(`7z extraction failed: ${err2 && err2.message ? err2.message : String(err2)}`)); - return resolve(); - }); - }); - } -} - -export function dirApp(dirName = 'open-audio-stack') { - if (getSystem() === SystemType.Win) return process.env.APPDATA || path.join(os.homedir(), dirName); - else if (getSystem() === SystemType.Mac) return path.join(os.homedir(), 'Library', 'Preferences', dirName); - return path.join(os.homedir(), '.local', 'share', dirName); -} - -export function dirContains(parentDir: string, childDir: string): boolean { - const normalizedParent = path.normalize(parentDir); - const normalizedChild = path.normalize(childDir); - // A trailing separator is required before the prefix check, otherwise a sibling directory - // that merely shares a prefix (e.g. parent "/foo/bar" vs child "/foo/barbaz") would - // incorrectly count as contained. - return normalizedChild === normalizedParent || normalizedChild.startsWith(normalizedParent + path.sep); -} - -export function dirCreate(dir: string) { - if (!dirExists(dir)) { - log('+', dir); - mkdirSync(dir, { recursive: true }); - return dir; - } - return false; -} - -export function dirDelete(dir: string) { - if (dirExists(dir)) { - log('-', dir); - return rmSync(dir, { recursive: true }); - } - return false; -} - -export function dirEmpty(dir: string) { - const files: string[] = readdirSync(dir); - return files.length === 0 || (files.length === 1 && files[0] === '.DS_Store'); -} - -export function dirExists(dir: string) { - return existsSync(dir); -} - -export function dirIs(dir: string) { - return statSync(dir).isDirectory(); -} - -export function dirMove(dir: string, dirNew: string): void | boolean { - if (dirExists(dir)) { - log('-', dir); - log('+', dirNew); - return moveSync(dir, dirNew, { overwrite: true }); - } - return false; -} - -export function dirOpen(dir: string) { - if (process.env.CI) return Buffer.from(''); - // execFileSync never invokes a shell itself, but on Windows the target of that call would be - // cmd.exe - which *is* a command interpreter, and re-parses its `/c` command line using cmd's - // own grammar (where `&`, `|`, `^`, etc are metacharacters) regardless of how Node quoted the - // argv it was given. explorer.exe has no such reinterpretation - it treats its argument as a - // literal path - so it's used instead of cmd.exe /c start. Its exit code is unreliable - // (frequently non-zero even on success), so this uses spawn() and doesn't wait on the result, - // same as the CI short-circuit above already implies callers don't depend on one. - if (getSystem() === SystemType.Win) { - log('⎋', `explorer.exe "${dir}"`); - spawn('explorer.exe', [dir], { stdio: 'ignore' }); - return; - } else if (getSystem() === SystemType.Mac) { - log('⎋', `open "${dir}"`); - return execFileSync('open', [dir]); - } - log('⎋', `xdg-open "${dir}"`); - return execFileSync('xdg-open', [dir]); -} - -export function dirPackage(pkg: PackageInterface) { - const parts: string[] = pkg.slug.split('/'); - parts.push(pkg.version); - return path.join(...parts); -} - -export function dirPlugins() { - if (getSystem() === SystemType.Win) - return process.env['ProgramFiles(x86)'] || path.join('C:', 'Program Files (x86)', 'Common Files'); - else if (getSystem() === SystemType.Mac) return path.join(os.homedir(), 'Library', 'Audio', 'Plug-ins'); - // Under $HOME rather than the system-wide /usr/local/lib, matching the spec - this keeps the - // default writable without elevation, consistent with the unprivileged archive-install path - // (see ManagerLocal.install()). - return path.join(os.homedir(), 'usr', 'local', 'lib'); -} - -export function dirPresets() { - if (getSystem() === SystemType.Win) return path.join(os.homedir(), 'Documents', 'VST3 Presets'); - else if (getSystem() === SystemType.Mac) return path.join(os.homedir(), 'Library', 'Audio', 'Presets'); - return path.join(os.homedir(), '.vst3', 'presets'); -} - -export function dirProjects() { - // Windows throws permissions errors if you scan hidden folders - // Therefore set to a more specific path than Documents - if (getSystem() === SystemType.Win) return path.join(os.homedir(), 'Documents', 'Audio'); - else if (getSystem() === SystemType.Mac) return path.join(os.homedir(), 'Documents', 'Audio'); - return path.join(os.homedir(), 'Documents', 'Audio'); -} - -export function dirTemplates() { - return path.join(os.homedir(), 'Documents', 'Audio Templates'); -} - -export function dirApps() { - if (getSystem() === SystemType.Win) return path.join(os.homedir(), 'AppData', 'Local', 'Programs'); - else if (getSystem() === SystemType.Mac) return path.join('/Applications'); - return path.join('/usr', 'local', 'bin'); -} - -export function dirRead(dir: string, options?: GlobOptionsWithFileTypesFalse): string[] { - log('⌕', dir); - // Glob now expects forward slashes on Windows - // Convert backslashes from path.join() to forwardslashes - if (getSystem() === SystemType.Win) { - dir = dir.replace(/\\/g, '/'); - } - // Ignore Mac files in Contents folders - // Filter out any paths not starting with the base directory - // This is to prevent issues with symlinks. - const baseDir: string = dir.includes('*') ? dir.split('*')[0] : dir; - const allPaths = globSync(dir, { - ignore: [`${baseDir}/**/*.{app,component,lv2,vst,vst3}/**/*`], - realpath: true, - ...options, - }); - // Glob input paths use forward slashes. - // Glob output paths are system-specific. - const baseDirCrossPlatform: string = baseDir.split('/').join(path.sep); - return allPaths.filter(p => p.startsWith(baseDirCrossPlatform)); -} - -export function dirRename(dir: string, dirNew: string): void | boolean { - if (dirExists(dir)) { - return moveSync(dir, dirNew, { overwrite: true }); - } - return false; -} - -export function fileCreate(filePath: string, data: string | Buffer): void { - log('+', filePath); - return writeFileSync(filePath, data); -} - -export function fileCreateJson(filePath: string, data: object): void { - return fileCreate(filePath, JSON.stringify(data, null, 2)); -} - -export function fileCreateYaml(filePath: string, data: object): void { - return fileCreate(filePath, yaml.dump(data)); -} - -export function fileDate(filePath: string): Date { - return statSync(filePath).mtime; -} - -export function fileDelete(filePath: string): boolean | void { - if (fileExists(filePath)) { - log('-', filePath); - return unlinkSync(filePath); - } - return false; -} - -export function fileExec(filePath: string): void { - return chmodSync(filePath, '755'); -} - -export function fileExists(filePath: string): boolean { - return existsSync(filePath); -} - -export async function fileHash(filePath: string, algorithm = 'sha256'): Promise { - log('⎋', filePath); - const input = createReadStream(filePath); - const hash = createHash(algorithm); - await stream.pipeline(input, hash); - return hash.digest('hex'); -} - -// Mounts to an explicit, freshly created mountpoint (rather than scanning /Volumes for -// whatever showed up) so a concurrently mounted, unrelated disk image can't be picked up -// instead, and so we know exactly what to detach afterwards. -function installDmg(filePath: string) { - const mountPoint = path.join(os.tmpdir(), `oas-dmg-${Date.now()}-${Math.random().toString(36).slice(2)}`); - mkdirSync(mountPoint, { recursive: true }); - try { - log('⎋', `hdiutil attach -nobrowse -mountpoint "${mountPoint}" "${filePath}"`); - execFileSync('hdiutil', ['attach', '-nobrowse', '-mountpoint', mountPoint, filePath]); - const pkgs = dirRead(path.join(mountPoint, '**', '*.pkg')); - if (pkgs.length === 0) throw new Error(`No .pkg found inside ${filePath}`); - log('⎋', `sudo installer -pkg "${pkgs[0]}" -target /`); - return execFileSync('sudo', ['installer', '-pkg', pkgs[0], '-target', '/'], { stdio: 'inherit' }); - } finally { - try { - execFileSync('hdiutil', ['detach', mountPoint, '-force']); - } catch { - /* best-effort unmount */ - } - } -} - -// Every branch below uses execFileSync (no shell) rather than building a command string for -// execSync. This is the actual fix, not just a hardening pass: file paths here are derived -// from community-submitted registry metadata (file.url), so a shell string built via template -// literal is a command injection vector regardless of how strictly the url is validated -// upstream - execFileSync passes each argument as its own argv entry, so shell metacharacters -// in filePath (`$(...)`, backticks, `;`, `|`, `&&`, ...) can never be interpreted. -export function fileInstall(filePath: string) { - if (process.env.CI) return Buffer.from(''); - const ext = path.extname(filePath).toLowerCase(); - switch (ext) { - case '.dmg': - return installDmg(filePath); - case '.pkg': - log('⎋', `sudo installer -pkg "${filePath}" -target /`); - return execFileSync('sudo', ['installer', '-pkg', filePath, '-target', '/'], { stdio: 'inherit' }); - case '.deb': - log('⎋', `sudo dpkg -i "${filePath}" || sudo apt-get install -f -y`); - try { - return execFileSync('sudo', ['dpkg', '-i', filePath], { stdio: 'inherit' }); - } catch { - return execFileSync('sudo', ['apt-get', 'install', '-f', '-y'], { stdio: 'inherit' }); - } - case '.rpm': - log( - '⎋', - `sudo rpm -i --nodigest --nofiledigest --nosignature --force "${filePath}" || sudo dnf install -y "${filePath}" || sudo yum install -y "${filePath}"`, - ); - try { - return execFileSync( - 'sudo', - ['rpm', '-i', '--nodigest', '--nofiledigest', '--nosignature', '--force', filePath], - { stdio: 'inherit' }, - ); - } catch { - try { - return execFileSync('sudo', ['dnf', 'install', '-y', filePath], { stdio: 'inherit' }); - } catch { - return execFileSync('sudo', ['yum', 'install', '-y', filePath], { stdio: 'inherit' }); - } - } - case '.exe': - // Run the downloaded installer directly - no shell/`start` wrapper needed at all. - log('⎋', `"${filePath}" /quiet /norestart`); - return execFileSync(filePath, ['/quiet', '/norestart'], { stdio: 'inherit' }); - case '.msi': - log('⎋', `msiexec /i "${filePath}" /quiet /norestart`); - return execFileSync('msiexec', ['/i', filePath, '/quiet', '/norestart'], { stdio: 'inherit' }); - default: - throw new Error(`Unsupported file format: ${ext}`); - } -} - -export function fileMove(filePath: string, newPath: string): void | boolean { - if (fileExists(filePath)) { - log('-', filePath); - log('+', newPath); - return moveSync(filePath, newPath, { overwrite: true }); - } - return false; -} - -export function filesMove(dirSource: string, dirTarget: string, dirSub: string, formatDir: Record) { - const filesAndFolders: string[] = dirRead(`${dirSource}/**/*`); - log('filesAndFolders', filesAndFolders); - - // First pass: identify bundle directories (app, clap, vst3, lv2, etc.) - const bundleDirs: Set = new Set(); - filesAndFolders.forEach(f => { - if (dirIs(f)) { - // Check if this is a macOS application bundle or plugin bundle - if (fileExists(path.join(f, 'Contents', 'Info.plist'))) { - bundleDirs.add(f); - } - // Check if this is an LV2 plugin folder - if (fileExists(path.join(f, 'manifest.ttl'))) { - bundleDirs.add(f); - } - // VST3 bundles on Linux (and some Windows builds) are directories without a macOS - // Info.plist, so they must be recognized by extension alone. - if (path.extname(f).slice(1).toLowerCase() === 'vst3') { - bundleDirs.add(f); - } - } - }); - - const files = filesAndFolders.filter(f => { - // Exclude files/folders that are inside bundle directories - for (const bundleDir of bundleDirs) { - if (f.startsWith(bundleDir + path.sep)) { - return false; // This path is inside a bundle, exclude it - } - } - - // Include regular files (not directories). - if (!dirIs(f)) return true; - - // Include bundle directories themselves (already identified above). - if (bundleDirs.has(f)) return true; - - // Otherwise ignore. - return false; - }); - const filesMoved: string[] = []; - log('files', files); - - // For each file, move to correct folder based on type - files.forEach((fileSource: string) => { - const fileExt: string = path.extname(fileSource).slice(1).toLowerCase(); - let fileExtTarget = formatDir[fileExt]; - - // Use mime-type detection as fallback for unmapped extensions - if (!fileExtTarget) { - const mimeType = mime.lookup(fileSource) || ''; - if (!mimeType || mimeType.startsWith('application/')) { - fileExtTarget = 'App'; - } - } - - // If this is not a supported file format, then ignore. - if (fileExtTarget === undefined) - return log(`${fileSource} - ${fileExt || 'no extension'} not mapped to a installation folder, skipping.`); - const fileTarget: string = path.join(dirTarget, fileExtTarget, dirSub, path.basename(fileSource)); - if (fileExists(fileTarget)) return log(`${fileSource} - ${fileTarget} already exists, skipping.`); - dirCreate(path.dirname(fileTarget)); - fileMove(fileSource, fileTarget); - // Set executable permissions for executable file types - if (fileExt === 'app') { - // For .app bundles, find and set permissions on the actual executable - const executablePath = path.join(fileTarget, 'Contents', 'MacOS', path.basename(fileTarget, '.app')); - if (fileExists(executablePath)) { - fileExec(executablePath); - } - } else if (['elf', 'exe', ''].includes(fileExt)) { - fileExec(fileTarget); - } - filesMoved.push(fileTarget); - }); - return filesMoved; -} - -// filePath (and, for the Mac/Linux branches, the surrounding options) ultimately come from a -// package's `open` field in registry metadata, so this is the same command-injection surface as -// fileInstall - execFileSync (no shell) rather than execSync for those branches. The Windows -// branch below needs a different fix: see its own comment. -export function fileOpen(filePath: string, options: string[] = []) { - if (process.env.CI) return Buffer.from(''); - - if (getSystem() === SystemType.Mac) { - const isExecutable = !path.extname(filePath); - if (isExecutable) { - // Use spawn for executables with stdio inherit to show output - log('⎋', `spawn "${filePath}" ${options.join(' ')}`); - const child = spawn(filePath, options, { stdio: 'inherit' }); - return child; - } else { - log('⎋', `open "${filePath}"`); - return execFileSync('open', [filePath]); - } - } - - if (getSystem() === SystemType.Win) { - // execFileSync never invokes a shell itself, but on Windows the target of that call would - // be cmd.exe - which *is* a command interpreter, and re-parses its `/c` command line using - // cmd's own grammar (where `&`, `|`, `^`, etc are metacharacters) regardless of how Node - // quoted the argv it was given, so a filePath containing them (this is untrusted, coming - // from a package's `open` field) could still be reinterpreted. explorer.exe has no such - // reinterpretation - it treats its argument as a literal path - so it's used instead of - // cmd.exe /c start. Its exit code is unreliable (frequently non-zero even on success), so - // this uses spawn() and doesn't wait on/check the result. - log('⎋', `explorer.exe "${filePath}"`); - spawn('explorer.exe', [filePath], { stdio: 'ignore' }); - return; - } - log('⎋', `xdg-open "${filePath}"`); - return execFileSync('xdg-open', [filePath]); -} - -export function fileRead(filePath: string) { - log('⎋', filePath); - return readFileSync(filePath, 'utf8'); -} - -export function fileReadJson(filePath: string) { - if (fileExists(filePath)) { - log('⎋', filePath); - return JSON.parse(readFileSync(filePath, 'utf8').toString()); - } - return false; -} - -export function fileReadString(filePath: string) { - log('⎋', filePath); - return readFileSync(filePath, 'utf8').toString(); -} - -export function fileReadYaml(filePath: string) { - const file: string = fileReadString(filePath); - return yaml.load(file); -} - -export function fileSize(filePath: string) { - return statSync(filePath).size; -} - -export function isAdmin(): boolean { - if (process.platform === 'win32') { - try { - execFileSync('net', ['session'], { stdio: 'ignore' }); - return true; - } catch { - return false; - } - } else { - return process && process.getuid ? process.getuid() === 0 : false; - } -} - -export async function fileValidateMetadata(filePath: string, fileMetadata: PluginFile | PresetFile | ProjectFile) { - const errors: ZodIssue[] = []; - const hash = await fileHash(filePath); - if (fileMetadata.sha256 !== hash) { - errors.push({ - code: ZodIssueCode.invalid_type, - expected: fileMetadata.sha256 as ZodParsedType, - message: 'Required', - path: ['sha256'], - received: hash as ZodParsedType, - }); - } - if (fileMetadata.size !== fileSize(filePath)) { - errors.push({ - code: ZodIssueCode.invalid_type, - expected: String(fileMetadata.size) as ZodParsedType, - message: 'Required', - path: ['size'], - received: String(fileSize(filePath)) as ZodParsedType, - }); - } - return errors; -} - -export function getPlatform() { - if (getSystem() === SystemType.Win) return SystemType.Win; - else if (getSystem() === SystemType.Mac) return SystemType.Mac; - return SystemType.Linux; -} - -export interface AdminPayload { - appDir: string; - operation: string; - type: string; - id: string; - version?: string; - log?: boolean; -} - -// sudo-prompt's exec() only accepts a single command string run through a shell - there is no -// argv-array form to escape into. `appDir`/`id`/`version` ultimately come from registry -// metadata or local project files, so building `--flag "${value}"` text here would be the same -// command-injection surface as fileInstall. Instead, base64url-encode the dynamic payload: its -// alphabet is only [A-Za-z0-9_-], so whatever the payload contains, the shell only ever sees -// characters that can't be interpreted as shell syntax. -export function runCliAsAdmin(payload: AdminPayload): Promise { - return new Promise((resolve, reject) => { - const filename: string = fileURLToPath(import.meta.url).replace('src/', 'build/'); - const dirPathClean: string = dirname(filename).replace('app.asar', 'app.asar.unpacked'); - const script: string = path.join(dirPathClean, 'admin.js'); - const encodedPayload: string = Buffer.from(JSON.stringify(payload)).toString('base64url'); - - log(`Running as admin: node "${script}" --payload `); - - const cmd = `node ${JSON.stringify(script)} --payload ${encodedPayload}`; - - sudoPrompt.exec( - cmd, - { name: 'Open Audio Stack' }, - (error?: Error | undefined, stdout?: string | Buffer | undefined, stderr?: string | Buffer | undefined) => { - // Convert stdout/stderr buffers to strings for inspection - const stdoutStr = stdout ? (typeof stdout === 'string' ? stdout : stdout.toString()) : ''; - const stderrStr = stderr ? (typeof stderr === 'string' ? stderr : stderr.toString()) : ''; - - const out = stdoutStr + stderrStr; - log(out); - - // Try to parse structured JSON output from the admin script first. - // Admin script outputs JSON on its own line after a newline, so look for the last JSON object. - const lines = out.split('\n'); - let jsonPayload = null; - for (let i = lines.length - 1; i >= 0; i--) { - const line = lines[i].trim(); - if (!line) continue; // Skip empty lines - try { - jsonPayload = JSON.parse(line); - break; // Found valid JSON, stop searching backwards - } catch { - // This line is not JSON, continue searching - } - } - - // If we found JSON output from admin script, prioritize it over sudoPrompt error - if (jsonPayload) { - if (jsonPayload && (jsonPayload.status === 'ok' || jsonPayload.code === 0)) { - return resolve(); - } - const errMsg = jsonPayload && jsonPayload.message ? jsonPayload.message : JSON.stringify(jsonPayload); - return reject(new Error(`runCliAsAdmin: admin command reported error: ${errMsg}`)); - } - - // If no JSON found, check for sudoPrompt error - if (error) { - const msg = `runCliAsAdmin: admin command failed: ${error && error.message ? error.message : String(error)}${ - stderrStr ? `\nstderr: ${stderrStr}` : '' - }`; - const err: any = new Error(msg); - err.code = (error as any) && (error as any).code ? (error as any).code : undefined; - return reject(err); - } - - return reject( - new Error( - `runCliAsAdmin: admin command did not report completion. stdout: ${stdoutStr} stderr: ${stderrStr}`, - ), - ); - }, - ); - }); -} - -export function zipCreate(filesPath: string, zipPath: string): void { - if (fileExists(zipPath)) { - unlinkSync(zipPath); - } - const zip: AdmZip = new AdmZip(); - const pathList: string[] = dirRead(filesPath); - pathList.forEach(pathItem => { - log('⎋', pathItem); - try { - if (dirIs(pathItem)) { - zip.addLocalFolder(pathItem, path.basename(pathItem)); - } else { - zip.addLocalFile(pathItem); - } - } catch (error) { - log(error); - } - }); - log('+', zipPath); - return zip.writeZip(zipPath); -} +// This module used to hold every filesystem-adjacent helper in one 670+ line file, conflating +// generic fs primitives, archive extraction/creation, OS-default path resolution, and privileged +// installer execution - impossible to reason about (or import) one concern without the others. +// Split into fs.ts / archive.ts / paths.ts / installer.ts along those seams; this file now only +// re-exports all four, so every existing import of '../helpers/file.js' keeps working unchanged. +// New code should prefer importing directly from whichever of the four actually matches what it +// needs. +export * from './fs.js'; +export * from './archive.js'; +export * from './paths.js'; +export * from './installer.js'; diff --git a/src/helpers/fs.ts b/src/helpers/fs.ts new file mode 100644 index 0000000..18bd16a --- /dev/null +++ b/src/helpers/fs.ts @@ -0,0 +1,240 @@ +import { execFileSync, spawn } from 'child_process'; +import { + chmodSync, + createReadStream, + existsSync, + mkdirSync, + readdirSync, + readFileSync, + rmSync, + statSync, + unlinkSync, + writeFileSync, +} from 'fs'; +import { createHash } from 'crypto'; +import stream from 'stream/promises'; +import { GlobOptionsWithFileTypesFalse, globSync } from 'glob'; +import { moveSync } from 'fs-extra/esm'; +import path from 'path'; +import yaml from 'js-yaml'; +import { SystemType } from '../types/SystemType.js'; +import { getSystem } from './utilsLocal.js'; +import { log } from './utils.js'; + +// Generic, domain-agnostic filesystem primitives - directory/file create/read/move/delete, +// hashing, and "open with the OS". No audio-package-specific knowledge belongs here; see +// paths.ts (default install directories), archive.ts (archive extraction/creation, including +// filesMove()'s package-format sorting), and installer.ts (privileged installer execution) for +// that - this module is the shared base all three build on. + +export function dirContains(parentDir: string, childDir: string): boolean { + const normalizedParent = path.normalize(parentDir); + const normalizedChild = path.normalize(childDir); + // A trailing separator is required before the prefix check, otherwise a sibling directory + // that merely shares a prefix (e.g. parent "/foo/bar" vs child "/foo/barbaz") would + // incorrectly count as contained. + return normalizedChild === normalizedParent || normalizedChild.startsWith(normalizedParent + path.sep); +} + +export function dirCreate(dir: string) { + if (!dirExists(dir)) { + log('+', dir); + mkdirSync(dir, { recursive: true }); + return dir; + } + return false; +} + +export function dirDelete(dir: string) { + if (dirExists(dir)) { + log('-', dir); + return rmSync(dir, { recursive: true }); + } + return false; +} + +export function dirEmpty(dir: string) { + const files: string[] = readdirSync(dir); + return files.length === 0 || (files.length === 1 && files[0] === '.DS_Store'); +} + +export function dirExists(dir: string) { + return existsSync(dir); +} + +export function dirIs(dir: string) { + return statSync(dir).isDirectory(); +} + +export function dirMove(dir: string, dirNew: string): void | boolean { + if (dirExists(dir)) { + log('-', dir); + log('+', dirNew); + return moveSync(dir, dirNew, { overwrite: true }); + } + return false; +} + +export function dirOpen(dir: string) { + if (process.env.CI) return Buffer.from(''); + // execFileSync never invokes a shell itself, but on Windows the target of that call would be + // cmd.exe - which *is* a command interpreter, and re-parses its `/c` command line using cmd's + // own grammar (where `&`, `|`, `^`, etc are metacharacters) regardless of how Node quoted the + // argv it was given. explorer.exe has no such reinterpretation - it treats its argument as a + // literal path - so it's used instead of cmd.exe /c start. Its exit code is unreliable + // (frequently non-zero even on success), so this uses spawn() and doesn't wait on the result, + // same as the CI short-circuit above already implies callers don't depend on one. + if (getSystem() === SystemType.Win) { + log('⎋', `explorer.exe "${dir}"`); + spawn('explorer.exe', [dir], { stdio: 'ignore' }); + return; + } else if (getSystem() === SystemType.Mac) { + log('⎋', `open "${dir}"`); + return execFileSync('open', [dir]); + } + log('⎋', `xdg-open "${dir}"`); + return execFileSync('xdg-open', [dir]); +} + +export function dirRead(dir: string, options?: GlobOptionsWithFileTypesFalse): string[] { + log('⌕', dir); + // Glob now expects forward slashes on Windows + // Convert backslashes from path.join() to forwardslashes + if (getSystem() === SystemType.Win) { + dir = dir.replace(/\\/g, '/'); + } + // Ignore Mac files in Contents folders + // Filter out any paths not starting with the base directory + // This is to prevent issues with symlinks. + const baseDir: string = dir.includes('*') ? dir.split('*')[0] : dir; + const allPaths = globSync(dir, { + ignore: [`${baseDir}/**/*.{app,component,lv2,vst,vst3}/**/*`], + realpath: true, + ...options, + }); + // Glob input paths use forward slashes. + // Glob output paths are system-specific. + const baseDirCrossPlatform: string = baseDir.split('/').join(path.sep); + return allPaths.filter(p => p.startsWith(baseDirCrossPlatform)); +} + +export function dirRename(dir: string, dirNew: string): void | boolean { + if (dirExists(dir)) { + return moveSync(dir, dirNew, { overwrite: true }); + } + return false; +} + +export function fileCreate(filePath: string, data: string | Buffer): void { + log('+', filePath); + return writeFileSync(filePath, data); +} + +export function fileCreateJson(filePath: string, data: object): void { + return fileCreate(filePath, JSON.stringify(data, null, 2)); +} + +export function fileCreateYaml(filePath: string, data: object): void { + return fileCreate(filePath, yaml.dump(data)); +} + +export function fileDate(filePath: string): Date { + return statSync(filePath).mtime; +} + +export function fileDelete(filePath: string): boolean | void { + if (fileExists(filePath)) { + log('-', filePath); + return unlinkSync(filePath); + } + return false; +} + +export function fileExec(filePath: string): void { + return chmodSync(filePath, '755'); +} + +export function fileExists(filePath: string): boolean { + return existsSync(filePath); +} + +export async function fileHash(filePath: string, algorithm = 'sha256'): Promise { + log('⎋', filePath); + const input = createReadStream(filePath); + const hash = createHash(algorithm); + await stream.pipeline(input, hash); + return hash.digest('hex'); +} + +export function fileMove(filePath: string, newPath: string): void | boolean { + if (fileExists(filePath)) { + log('-', filePath); + log('+', newPath); + return moveSync(filePath, newPath, { overwrite: true }); + } + return false; +} + +// filePath (and, for the Mac/Linux branches, the surrounding options) ultimately come from a +// package's `open` field in registry metadata, so this is the same command-injection surface as +// installer.ts's fileInstall - execFileSync (no shell) rather than execSync for those branches. +// The Windows branch below needs a different fix: see its own comment. +export function fileOpen(filePath: string, options: string[] = []) { + if (process.env.CI) return Buffer.from(''); + + if (getSystem() === SystemType.Mac) { + const isExecutable = !path.extname(filePath); + if (isExecutable) { + // Use spawn for executables with stdio inherit to show output + log('⎋', `spawn "${filePath}" ${options.join(' ')}`); + const child = spawn(filePath, options, { stdio: 'inherit' }); + return child; + } else { + log('⎋', `open "${filePath}"`); + return execFileSync('open', [filePath]); + } + } + + if (getSystem() === SystemType.Win) { + // execFileSync never invokes a shell itself, but on Windows the target of that call would + // be cmd.exe - which *is* a command interpreter, and re-parses its `/c` command line using + // cmd's own grammar (where `&`, `|`, `^`, etc are metacharacters) regardless of how Node + // quoted the argv it was given, so a filePath containing them (this is untrusted, coming + // from a package's `open` field) could still be reinterpreted. explorer.exe has no such + // reinterpretation - it treats its argument as a literal path - so it's used instead of + // cmd.exe /c start. Its exit code is unreliable (frequently non-zero even on success), so + // this uses spawn() and doesn't wait on/check the result. + log('⎋', `explorer.exe "${filePath}"`); + spawn('explorer.exe', [filePath], { stdio: 'ignore' }); + return; + } + log('⎋', `xdg-open "${filePath}"`); + return execFileSync('xdg-open', [filePath]); +} + +export function fileRead(filePath: string) { + log('⎋', filePath); + return readFileSync(filePath, 'utf8'); +} + +export function fileReadJson(filePath: string) { + if (fileExists(filePath)) { + log('⎋', filePath); + return JSON.parse(readFileSync(filePath, 'utf8').toString()); + } + return false; +} + +export function fileReadString(filePath: string) { + log('⎋', filePath); + return readFileSync(filePath, 'utf8').toString(); +} + +export function fileReadYaml(filePath: string) { + const file: string = fileReadString(filePath); + return yaml.load(file); +} + +export function fileSize(filePath: string) { + return statSync(filePath).size; +} diff --git a/src/helpers/installer.ts b/src/helpers/installer.ts new file mode 100644 index 0000000..240edef --- /dev/null +++ b/src/helpers/installer.ts @@ -0,0 +1,208 @@ +import { execFileSync } from 'child_process'; +import { mkdirSync } from 'fs'; +import os from 'os'; +import path, { dirname } from 'path'; +import { fileURLToPath } from 'url'; +import sudoPrompt from '@vscode/sudo-prompt'; +import { ZodIssueCode, ZodParsedType, ZodIssue } from 'zod'; +import { PluginFile } from '../types/Plugin.js'; +import { PresetFile } from '../types/Preset.js'; +import { ProjectFile } from '../types/Project.js'; +import { log } from './utils.js'; +import { dirRead, fileHash, fileSize } from './fs.js'; + +// Privileged installer execution (fileInstall) and the admin-elevation bridge (runCliAsAdmin) - +// the two concerns that actually need to run something with elevated permissions, as opposed to +// fs.ts's unprivileged file operations. + +export function isAdmin(): boolean { + if (process.platform === 'win32') { + try { + execFileSync('net', ['session'], { stdio: 'ignore' }); + return true; + } catch { + return false; + } + } else { + return process && process.getuid ? process.getuid() === 0 : false; + } +} + +// Mounts to an explicit, freshly created mountpoint (rather than scanning /Volumes for +// whatever showed up) so a concurrently mounted, unrelated disk image can't be picked up +// instead, and so we know exactly what to detach afterwards. +function installDmg(filePath: string) { + const mountPoint = path.join(os.tmpdir(), `oas-dmg-${Date.now()}-${Math.random().toString(36).slice(2)}`); + mkdirSync(mountPoint, { recursive: true }); + try { + log('⎋', `hdiutil attach -nobrowse -mountpoint "${mountPoint}" "${filePath}"`); + execFileSync('hdiutil', ['attach', '-nobrowse', '-mountpoint', mountPoint, filePath]); + const pkgs = dirRead(path.join(mountPoint, '**', '*.pkg')); + if (pkgs.length === 0) throw new Error(`No .pkg found inside ${filePath}`); + log('⎋', `sudo installer -pkg "${pkgs[0]}" -target /`); + return execFileSync('sudo', ['installer', '-pkg', pkgs[0], '-target', '/'], { stdio: 'inherit' }); + } finally { + try { + execFileSync('hdiutil', ['detach', mountPoint, '-force']); + } catch { + /* best-effort unmount */ + } + } +} + +// Every branch below uses execFileSync (no shell) rather than building a command string for +// execSync. This is the actual fix, not just a hardening pass: file paths here are derived +// from community-submitted registry metadata (file.url), so a shell string built via template +// literal is a command injection vector regardless of how strictly the url is validated +// upstream - execFileSync passes each argument as its own argv entry, so shell metacharacters +// in filePath (`$(...)`, backticks, `;`, `|`, `&&`, ...) can never be interpreted. +export function fileInstall(filePath: string) { + if (process.env.CI) return Buffer.from(''); + const ext = path.extname(filePath).toLowerCase(); + switch (ext) { + case '.dmg': + return installDmg(filePath); + case '.pkg': + log('⎋', `sudo installer -pkg "${filePath}" -target /`); + return execFileSync('sudo', ['installer', '-pkg', filePath, '-target', '/'], { stdio: 'inherit' }); + case '.deb': + log('⎋', `sudo dpkg -i "${filePath}" || sudo apt-get install -f -y`); + try { + return execFileSync('sudo', ['dpkg', '-i', filePath], { stdio: 'inherit' }); + } catch { + return execFileSync('sudo', ['apt-get', 'install', '-f', '-y'], { stdio: 'inherit' }); + } + case '.rpm': + log( + '⎋', + `sudo rpm -i --nodigest --nofiledigest --nosignature --force "${filePath}" || sudo dnf install -y "${filePath}" || sudo yum install -y "${filePath}"`, + ); + try { + return execFileSync( + 'sudo', + ['rpm', '-i', '--nodigest', '--nofiledigest', '--nosignature', '--force', filePath], + { stdio: 'inherit' }, + ); + } catch { + try { + return execFileSync('sudo', ['dnf', 'install', '-y', filePath], { stdio: 'inherit' }); + } catch { + return execFileSync('sudo', ['yum', 'install', '-y', filePath], { stdio: 'inherit' }); + } + } + case '.exe': + // Run the downloaded installer directly - no shell/`start` wrapper needed at all. + log('⎋', `"${filePath}" /quiet /norestart`); + return execFileSync(filePath, ['/quiet', '/norestart'], { stdio: 'inherit' }); + case '.msi': + log('⎋', `msiexec /i "${filePath}" /quiet /norestart`); + return execFileSync('msiexec', ['/i', filePath, '/quiet', '/norestart'], { stdio: 'inherit' }); + default: + throw new Error(`Unsupported file format: ${ext}`); + } +} + +export async function fileValidateMetadata(filePath: string, fileMetadata: PluginFile | PresetFile | ProjectFile) { + const errors: ZodIssue[] = []; + const hash = await fileHash(filePath); + if (fileMetadata.sha256 !== hash) { + errors.push({ + code: ZodIssueCode.invalid_type, + expected: fileMetadata.sha256 as ZodParsedType, + message: 'Required', + path: ['sha256'], + received: hash as ZodParsedType, + }); + } + if (fileMetadata.size !== fileSize(filePath)) { + errors.push({ + code: ZodIssueCode.invalid_type, + expected: String(fileMetadata.size) as ZodParsedType, + message: 'Required', + path: ['size'], + received: String(fileSize(filePath)) as ZodParsedType, + }); + } + return errors; +} + +export interface AdminPayload { + appDir: string; + operation: string; + type: string; + id: string; + version?: string; + log?: boolean; +} + +// sudo-prompt's exec() only accepts a single command string run through a shell - there is no +// argv-array form to escape into. `appDir`/`id`/`version` ultimately come from registry +// metadata or local project files, so building `--flag "${value}"` text here would be the same +// command-injection surface as fileInstall. Instead, base64url-encode the dynamic payload: its +// alphabet is only [A-Za-z0-9_-], so whatever the payload contains, the shell only ever sees +// characters that can't be interpreted as shell syntax. +export function runCliAsAdmin(payload: AdminPayload): Promise { + return new Promise((resolve, reject) => { + const filename: string = fileURLToPath(import.meta.url).replace('src/', 'build/'); + const dirPathClean: string = dirname(filename).replace('app.asar', 'app.asar.unpacked'); + const script: string = path.join(dirPathClean, 'admin.js'); + const encodedPayload: string = Buffer.from(JSON.stringify(payload)).toString('base64url'); + + log(`Running as admin: node "${script}" --payload `); + + const cmd = `node ${JSON.stringify(script)} --payload ${encodedPayload}`; + + sudoPrompt.exec( + cmd, + { name: 'Open Audio Stack' }, + (error?: Error | undefined, stdout?: string | Buffer | undefined, stderr?: string | Buffer | undefined) => { + // Convert stdout/stderr buffers to strings for inspection + const stdoutStr = stdout ? (typeof stdout === 'string' ? stdout : stdout.toString()) : ''; + const stderrStr = stderr ? (typeof stderr === 'string' ? stderr : stderr.toString()) : ''; + + const out = stdoutStr + stderrStr; + log(out); + + // Try to parse structured JSON output from the admin script first. + // Admin script outputs JSON on its own line after a newline, so look for the last JSON object. + const lines = out.split('\n'); + let jsonPayload = null; + for (let i = lines.length - 1; i >= 0; i--) { + const line = lines[i].trim(); + if (!line) continue; // Skip empty lines + try { + jsonPayload = JSON.parse(line); + break; // Found valid JSON, stop searching backwards + } catch { + // This line is not JSON, continue searching + } + } + + // If we found JSON output from admin script, prioritize it over sudoPrompt error + if (jsonPayload) { + if (jsonPayload && (jsonPayload.status === 'ok' || jsonPayload.code === 0)) { + return resolve(); + } + const errMsg = jsonPayload && jsonPayload.message ? jsonPayload.message : JSON.stringify(jsonPayload); + return reject(new Error(`runCliAsAdmin: admin command reported error: ${errMsg}`)); + } + + // If no JSON found, check for sudoPrompt error + if (error) { + const msg = `runCliAsAdmin: admin command failed: ${error && error.message ? error.message : String(error)}${ + stderrStr ? `\nstderr: ${stderrStr}` : '' + }`; + const err: any = new Error(msg); + err.code = (error as any) && (error as any).code ? (error as any).code : undefined; + return reject(err); + } + + return reject( + new Error( + `runCliAsAdmin: admin command did not report completion. stdout: ${stdoutStr} stderr: ${stderrStr}`, + ), + ); + }, + ); + }); +} diff --git a/src/helpers/packageLocal.ts b/src/helpers/packageLocal.ts index d7228be..e40c027 100644 --- a/src/helpers/packageLocal.ts +++ b/src/helpers/packageLocal.ts @@ -1,5 +1,5 @@ import path from 'path'; -import { fileCreateJson, fileReadJson } from './file.js'; +import { fileCreateJson, fileReadJson } from './fs.js'; import { Package } from '../classes/Package.js'; import { log, pathGetSlug, pathGetVersion } from './utils.js'; import { PackageVersion } from '../index-browser.js'; diff --git a/src/helpers/paths.ts b/src/helpers/paths.ts new file mode 100644 index 0000000..5e6412b --- /dev/null +++ b/src/helpers/paths.ts @@ -0,0 +1,63 @@ +import os from 'os'; +import path from 'path'; +import { PackageInterface } from '../types/Package.js'; +import { SystemType } from '../types/SystemType.js'; +import { getSystem } from './utilsLocal.js'; + +// Default per-platform install directories (see specification.md's "App directory"/"Apps +// directory"/"Plugins directory"/"Presets directory"/"Projects directory"/"Templates directory" +// sections) plus package-relative path construction. Deliberately separate from fs.ts's generic +// primitives - this module is entirely about *which* path to use, never about touching the +// filesystem. + +export function dirApp(dirName = 'open-audio-stack') { + if (getSystem() === SystemType.Win) return process.env.APPDATA || path.join(os.homedir(), dirName); + else if (getSystem() === SystemType.Mac) return path.join(os.homedir(), 'Library', 'Preferences', dirName); + return path.join(os.homedir(), '.local', 'share', dirName); +} + +export function dirApps() { + if (getSystem() === SystemType.Win) return path.join(os.homedir(), 'AppData', 'Local', 'Programs'); + else if (getSystem() === SystemType.Mac) return path.join('/Applications'); + return path.join('/usr', 'local', 'bin'); +} + +export function dirPackage(pkg: PackageInterface) { + const parts: string[] = pkg.slug.split('/'); + parts.push(pkg.version); + return path.join(...parts); +} + +export function dirPlugins() { + if (getSystem() === SystemType.Win) + return process.env['ProgramFiles(x86)'] || path.join('C:', 'Program Files (x86)', 'Common Files'); + else if (getSystem() === SystemType.Mac) return path.join(os.homedir(), 'Library', 'Audio', 'Plug-ins'); + // Under $HOME rather than the system-wide /usr/local/lib, matching the spec - this keeps the + // default writable without elevation, consistent with the unprivileged archive-install path + // (see ManagerLocal.install()). + return path.join(os.homedir(), 'usr', 'local', 'lib'); +} + +export function dirPresets() { + if (getSystem() === SystemType.Win) return path.join(os.homedir(), 'Documents', 'VST3 Presets'); + else if (getSystem() === SystemType.Mac) return path.join(os.homedir(), 'Library', 'Audio', 'Presets'); + return path.join(os.homedir(), '.vst3', 'presets'); +} + +export function dirProjects() { + // Windows throws permissions errors if you scan hidden folders + // Therefore set to a more specific path than Documents + if (getSystem() === SystemType.Win) return path.join(os.homedir(), 'Documents', 'Audio'); + else if (getSystem() === SystemType.Mac) return path.join(os.homedir(), 'Documents', 'Audio'); + return path.join(os.homedir(), 'Documents', 'Audio'); +} + +export function dirTemplates() { + return path.join(os.homedir(), 'Documents', 'Audio Templates'); +} + +export function getPlatform() { + if (getSystem() === SystemType.Win) return SystemType.Win; + else if (getSystem() === SystemType.Mac) return SystemType.Mac; + return SystemType.Linux; +} From 73cf35ccc5b526ce181c3f800223929c1e2c47f6 Mon Sep 17 00:00:00 2001 From: Kim T Date: Thu, 30 Jul 2026 22:32:53 -0700 Subject: [PATCH 2/6] [fix] Retarget elevation/fileOpen mocks to the modules that actually export them After splitting helpers/file.ts, ManagerLocal.ts imports isAdmin/ runCliAsAdmin/fileInstall from the new installer.ts and fileOpen from fs.ts, but the "elevates when unprivileged" tests still spied on helpers/file.ts's re-export barrel (`vi.spyOn(fileHelpers, 'isAdmin')` etc). vi.spyOn patches a property on the exact namespace object it's given; a re-exporting barrel is a different namespace object even though `export *` makes the same underlying binding reachable through it. The mocks were silently not intercepting anything - the real, unmocked isAdmin()/runCliAsAdmin()/fileOpen() ran during test execution. This is what produced real macOS admin prompts and root-owned files under test/ during local runs of this branch (confirmed: this fix alone took a run from 3 failures citing "User did not grant permission" / real sudo-prompt errors to 218/218 passing with zero root-owned output). Retargeted the three affected spies to installerHelpers/ fsHelpers - the modules ManagerLocal.ts now actually imports these from - and added a comment on the first affected test explaining the barrel/spyOn interaction so it isn't reintroduced by a future test. --- tests/classes/ManagerLocal.test.ts | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/tests/classes/ManagerLocal.test.ts b/tests/classes/ManagerLocal.test.ts index 1443180..d3bbfd4 100644 --- a/tests/classes/ManagerLocal.test.ts +++ b/tests/classes/ManagerLocal.test.ts @@ -23,7 +23,8 @@ import { fileExists, fileReadJson, } from '../../src/helpers/file'; -import * as fileHelpers from '../../src/helpers/file'; +import * as fsHelpers from '../../src/helpers/fs'; +import * as installerHelpers from '../../src/helpers/installer'; import * as utilsLocalHelpers from '../../src/helpers/utilsLocal'; import { RegistryType } from '../../src/types/Registry'; import { ConfigInterface } from '../../src/types/Config'; @@ -213,9 +214,16 @@ test('Install archive package does not elevate when unprivileged', async () => { // Regression test for https://github.com/open-audio-stack/open-audio-stack-core/issues/83 - // a package whose only compatible file is an archive must install without admin elevation, // even in a headless environment with no polkit agent. - const isAdminSpy = vi.spyOn(fileHelpers, 'isAdmin').mockReturnValue(false); + // + // isAdmin/runCliAsAdmin/fileOpen below are spied on the specific module ManagerLocal.ts + // actually imports them from (installer.js/fs.js), not the helpers/file.js barrel - vi.spyOn + // patches a property on the exact namespace object you give it, and a re-exporting barrel is a + // *different* namespace object even though `export *` makes the same underlying binding + // reachable through it. Spying on the barrel here would silently fail to intercept the call, + // letting a real, unmocked elevation/admin-prompt flow run during tests. + const isAdminSpy = vi.spyOn(installerHelpers, 'isAdmin').mockReturnValue(false); const isTestsSpy = vi.spyOn(utilsLocalHelpers, 'isTests').mockReturnValue(false); - const runCliAsAdminSpy = vi.spyOn(fileHelpers, 'runCliAsAdmin').mockResolvedValue(undefined); + const runCliAsAdminSpy = vi.spyOn(installerHelpers, 'runCliAsAdmin').mockResolvedValue(undefined); mockRegistrySync(REGISTRY_PACKAGE_TYPES); const manager = new ManagerLocal(RegistryType.Projects, CONFIG); @@ -234,9 +242,9 @@ test('Install archive package does not elevate when unprivileged', async () => { test('Install installer-only package still elevates when unprivileged', async () => { // Regression guard alongside the above - a package with no compatible archive (only // installers) must still elevate, since there is no unprivileged install path available. - const isAdminSpy = vi.spyOn(fileHelpers, 'isAdmin').mockReturnValue(false); + const isAdminSpy = vi.spyOn(installerHelpers, 'isAdmin').mockReturnValue(false); const isTestsSpy = vi.spyOn(utilsLocalHelpers, 'isTests').mockReturnValue(false); - const runCliAsAdminSpy = vi.spyOn(fileHelpers, 'runCliAsAdmin').mockResolvedValue(undefined); + const runCliAsAdminSpy = vi.spyOn(installerHelpers, 'runCliAsAdmin').mockResolvedValue(undefined); mockRegistrySync(REGISTRY_PACKAGE_TYPES); const manager = new ManagerLocal(RegistryType.Plugins, CONFIG); @@ -266,9 +274,9 @@ test('Install all installs every listed package', async () => { }); test('Install all elevates when unprivileged', async () => { - const isAdminSpy = vi.spyOn(fileHelpers, 'isAdmin').mockReturnValue(false); + const isAdminSpy = vi.spyOn(installerHelpers, 'isAdmin').mockReturnValue(false); const isTestsSpy = vi.spyOn(utilsLocalHelpers, 'isTests').mockReturnValue(false); - const runCliAsAdminSpy = vi.spyOn(fileHelpers, 'runCliAsAdmin').mockResolvedValue(undefined); + const runCliAsAdminSpy = vi.spyOn(installerHelpers, 'runCliAsAdmin').mockResolvedValue(undefined); const manager = new ManagerLocal(RegistryType.Plugins, CONFIG); await manager.installAll(); @@ -460,7 +468,7 @@ test('Open runs the compatible file and propagates errors instead of swallowing await manager.sync(); await manager.install(PROJECT_PACKAGE.slug, PROJECT_PACKAGE.version); - const fileOpenSpy = vi.spyOn(fileHelpers, 'fileOpen').mockReturnValue(undefined as any); + const fileOpenSpy = vi.spyOn(fsHelpers, 'fileOpen').mockReturnValue(undefined as any); expect(manager.open(PROJECT_PACKAGE.slug, PROJECT_PACKAGE.version)).toEqual(true); expect(fileOpenSpy).toHaveBeenCalled(); From 8f97ed3dd7abd533b0b71d764ab189694904c4a1 Mon Sep 17 00:00:00 2001 From: Kim T Date: Thu, 30 Jul 2026 22:39:09 -0700 Subject: [PATCH 3/6] [fix] Address CodeQL zip-slip alert on the AdmZip Windows-fallback branch CodeQL flagged this PR with 1 new high-severity js/zipslip alert at archive.ts's AdmZip Windows-fallback branch. The code itself is byte-identical to what's on main (pre-existing, still-open alert #16 at the old file.ts:67) - not a vulnerability this refactor introduced, but CodeQL treats it as "new" since the code moved to a new file/path. The validation (isSafeArchiveEntryPath()) was already correct at runtime, but CodeQL's dataflow analysis wasn't recognizing it as a guard: each entry was validated and written in the same forEach iteration, one entry at a time. The adjacent .7z branch a few lines below - which isn't flagged - instead validates every entry up front and only writes afterward if all of them pass. Restructured the AdmZip branch to match that already-safe shape: map entries to their sanitized names, find any unsafe one and throw before writing anything, then write. Same validation logic, same error message, same result - verified via the full test suite (218/218 passing) - just reshaped so the guard-then-sink relationship is unambiguous to static analysis. --- src/helpers/archive.ts | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/src/helpers/archive.ts b/src/helpers/archive.ts index 83b20f0..50bf768 100644 --- a/src/helpers/archive.ts +++ b/src/helpers/archive.ts @@ -43,14 +43,22 @@ export async function archiveExtract(filePath: string, dirPath: string) { if (getSystem() === SystemType.Win && error.message?.includes('ENOENT')) { log('⚠️', 'Extracting files manually due to special characters in filenames'); const entries = zip.getEntries(); - entries.forEach(entry => { - const sanitizedName: string = entry.entryName.replace(/[<>:"|?*]/g, '_').replace(/[\r\n]/g, ''); - // This manual path builds destinations by hand instead of going through adm-zip's own - // sanitize(), so it must enforce the same containment itself - stripping `<>:"|?*` and - // newlines does nothing to stop a `..`-based traversal. - if (!isSafeArchiveEntryPath(sanitizedName, targetRoot)) { - throw new Error(`Archive entry escapes extraction directory: ${entry.entryName}`); - } + // This manual path builds destinations by hand instead of going through adm-zip's own + // sanitize(), so it must enforce the same containment itself - stripping `<>:"|?*` and + // newlines does nothing to stop a `..`-based traversal. Validate every entry up front + // (mirroring the .7z branch below) and refuse to extract anything at all if any entry is + // unsafe, rather than validating and writing one entry at a time in the same loop. + const sanitizedEntries = entries.map(entry => ({ + entry, + sanitizedName: entry.entryName.replace(/[<>:"|?*]/g, '_').replace(/[\r\n]/g, ''), + })); + const unsafeEntry = sanitizedEntries.find( + ({ sanitizedName }) => !isSafeArchiveEntryPath(sanitizedName, targetRoot), + ); + if (unsafeEntry) { + throw new Error(`Archive entry escapes extraction directory: ${unsafeEntry.entry.entryName}`); + } + sanitizedEntries.forEach(({ entry, sanitizedName }) => { const outputPath = path.join(dirPath, sanitizedName); if (!entry.isDirectory) { dirCreate(path.dirname(outputPath)); From 6f04d20d6f5fca1c8e23209ea311174a8c12b7b6 Mon Sep 17 00:00:00 2001 From: Kim T Date: Thu, 30 Jul 2026 22:43:49 -0700 Subject: [PATCH 4/6] [fix] Inline the zip-slip containment check at the point of use The previous restructuring (validate-all-then-write-all) didn't clear the CodeQL alert either - it was still tracing entry.entryName through to writeFileSync. Comparing against the adjacent .7z branch (which isn't flagged): that branch's untrusted entry.name only ever reaches a validation check - 7zip-min's unpack() gets the whole archive file and target directory (both trusted), never an individual entry name, so nothing untrusted reaches a filesystem-writing call in our own code. In the AdmZip branch, sanitizedName genuinely does flow into our own writeFileSync/dirCreate calls, so no restructuring of *when* the check runs changes that - what matters is *how* CodeQL can verify the guard. isSafeArchiveEntryPath()/dirContains() are correct at runtime, but the check reaches the sink through a call three functions deep, which static analysis can't verify as a guard. Replaced it with the containment check inlined directly ahead of the sink, computed on outputPath itself (the exact value passed to writeFileSync/dirCreate moments later) via path.resolve()/startsWith() - the same primitives dirContains() uses internally, just written at the point of use instead of indirected through a helper. Same behavior, verified via the full test suite (218/218 passing). --- src/helpers/archive.ts | 30 +++++++++++++----------------- 1 file changed, 13 insertions(+), 17 deletions(-) diff --git a/src/helpers/archive.ts b/src/helpers/archive.ts index 50bf768..50ee450 100644 --- a/src/helpers/archive.ts +++ b/src/helpers/archive.ts @@ -43,23 +43,19 @@ export async function archiveExtract(filePath: string, dirPath: string) { if (getSystem() === SystemType.Win && error.message?.includes('ENOENT')) { log('⚠️', 'Extracting files manually due to special characters in filenames'); const entries = zip.getEntries(); - // This manual path builds destinations by hand instead of going through adm-zip's own - // sanitize(), so it must enforce the same containment itself - stripping `<>:"|?*` and - // newlines does nothing to stop a `..`-based traversal. Validate every entry up front - // (mirroring the .7z branch below) and refuse to extract anything at all if any entry is - // unsafe, rather than validating and writing one entry at a time in the same loop. - const sanitizedEntries = entries.map(entry => ({ - entry, - sanitizedName: entry.entryName.replace(/[<>:"|?*]/g, '_').replace(/[\r\n]/g, ''), - })); - const unsafeEntry = sanitizedEntries.find( - ({ sanitizedName }) => !isSafeArchiveEntryPath(sanitizedName, targetRoot), - ); - if (unsafeEntry) { - throw new Error(`Archive entry escapes extraction directory: ${unsafeEntry.entry.entryName}`); - } - sanitizedEntries.forEach(({ entry, sanitizedName }) => { - const outputPath = path.join(dirPath, sanitizedName); + entries.forEach(entry => { + const sanitizedName: string = entry.entryName.replace(/[<>:"|?*]/g, '_').replace(/[\r\n]/g, ''); + // This manual path builds destinations by hand instead of going through adm-zip's own + // sanitize(), so it must enforce the same containment itself - stripping `<>:"|?*` and + // newlines does nothing to stop a `..`-based traversal. Checked inline against + // outputPath itself (the exact value passed to writeFileSync/dirCreate below), rather + // than through isSafeArchiveEntryPath()/dirContains() - static analysis (CodeQL) can't + // verify a path-traversal guard through a call indirection several functions deep, only + // a containment check written directly ahead of the sink that uses its result. + const outputPath = path.resolve(dirPath, sanitizedName); + if (outputPath !== targetRoot && !outputPath.startsWith(targetRoot + path.sep)) { + throw new Error(`Archive entry escapes extraction directory: ${entry.entryName}`); + } if (!entry.isDirectory) { dirCreate(path.dirname(outputPath)); writeFileSync(outputPath, entry.getData()); From bf3fe2f3f966e6c85250886f60457d7a420dcafb Mon Sep 17 00:00:00 2001 From: Kim T Date: Thu, 30 Jul 2026 22:48:43 -0700 Subject: [PATCH 5/6] [fix] Use a plain for-loop instead of .forEach() for the zip-slip guard Still not clearing the CodeQL alert with the inline check from the previous commit. Hypothesis: CodeQL's local guard-node recognition for this query traces direct control flow (if/for/while) within a function reliably, but not necessarily across a closure boundary - the containment check and the sink it guards were both inside a callback passed to Array.prototype.forEach(), an indirection through a library function between the parameter binding and the guard. Replaced entries.forEach(entry => {...}) with a plain for...of loop over the same entries array - identical logic and behavior (218/218 tests still passing), but the guard now sits in this function's own direct control flow instead of inside a passed-in callback. --- src/helpers/archive.ts | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/src/helpers/archive.ts b/src/helpers/archive.ts index 50ee450..f9711e9 100644 --- a/src/helpers/archive.ts +++ b/src/helpers/archive.ts @@ -43,15 +43,16 @@ export async function archiveExtract(filePath: string, dirPath: string) { if (getSystem() === SystemType.Win && error.message?.includes('ENOENT')) { log('⚠️', 'Extracting files manually due to special characters in filenames'); const entries = zip.getEntries(); - entries.forEach(entry => { + // This manual path builds destinations by hand instead of going through adm-zip's own + // sanitize(), so it must enforce the same containment itself - stripping `<>:"|?*` and + // newlines does nothing to stop a `..`-based traversal. Checked inline against outputPath + // itself (the exact value passed to writeFileSync/dirCreate below), in a plain for-loop + // rather than a .forEach() callback, so the guard sits in this function's own direct + // control flow - static analysis (CodeQL) can't verify a path-traversal guard through a + // call indirection several functions deep, or reliably across a callback boundary, only a + // containment check written directly ahead of the sink that uses its result. + for (const entry of entries) { const sanitizedName: string = entry.entryName.replace(/[<>:"|?*]/g, '_').replace(/[\r\n]/g, ''); - // This manual path builds destinations by hand instead of going through adm-zip's own - // sanitize(), so it must enforce the same containment itself - stripping `<>:"|?*` and - // newlines does nothing to stop a `..`-based traversal. Checked inline against - // outputPath itself (the exact value passed to writeFileSync/dirCreate below), rather - // than through isSafeArchiveEntryPath()/dirContains() - static analysis (CodeQL) can't - // verify a path-traversal guard through a call indirection several functions deep, only - // a containment check written directly ahead of the sink that uses its result. const outputPath = path.resolve(dirPath, sanitizedName); if (outputPath !== targetRoot && !outputPath.startsWith(targetRoot + path.sep)) { throw new Error(`Archive entry escapes extraction directory: ${entry.entryName}`); @@ -62,7 +63,7 @@ export async function archiveExtract(filePath: string, dirPath: string) { } else { dirCreate(outputPath); } - }); + } return; } } From c9284b5d1f77f4b87367f5fb1b885d4304217894 Mon Sep 17 00:00:00 2001 From: Kim T Date: Thu, 30 Jul 2026 22:53:17 -0700 Subject: [PATCH 6/6] [fix] Add a direct '..' substring check per CodeQL's own recommended pattern Fetched the full js/zipslip rule documentation via the code-scanning API - its own "GOOD" example checks the untrusted entry path directly for a ".." substring (fileName.indexOf('..') == -1) immediately before the write, not a resolved-path containment check. Every previous attempt here used the latter (path.resolve() + startsWith()), which is the more robust check (also catches absolute-path entries and path-separator edge cases) but apparently isn't what this query's guard-recognizer verifies on its own. Added the substring check as an explicit first guard, directly on sanitizedName, immediately before the existing containment check - defense in depth, not a replacement; the containment check remains the actually load-bearing defense. 218/218 tests still passing. --- src/helpers/archive.ts | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/src/helpers/archive.ts b/src/helpers/archive.ts index f9711e9..003040f 100644 --- a/src/helpers/archive.ts +++ b/src/helpers/archive.ts @@ -45,14 +45,18 @@ export async function archiveExtract(filePath: string, dirPath: string) { const entries = zip.getEntries(); // This manual path builds destinations by hand instead of going through adm-zip's own // sanitize(), so it must enforce the same containment itself - stripping `<>:"|?*` and - // newlines does nothing to stop a `..`-based traversal. Checked inline against outputPath - // itself (the exact value passed to writeFileSync/dirCreate below), in a plain for-loop - // rather than a .forEach() callback, so the guard sits in this function's own direct - // control flow - static analysis (CodeQL) can't verify a path-traversal guard through a - // call indirection several functions deep, or reliably across a callback boundary, only a - // containment check written directly ahead of the sink that uses its result. + // newlines does nothing to stop a `..`-based traversal. for (const entry of entries) { const sanitizedName: string = entry.entryName.replace(/[<>:"|?*]/g, '_').replace(/[\r\n]/g, ''); + // Reject any ".." path segment directly on the untrusted name - CodeQL's own + // documentation for this exact check (js/zipslip) recommends this pattern, and its + // guard-recognition doesn't reliably verify a resolved-path containment check on its + // own. The containment check right below is the actually load-bearing defense (it also + // catches absolute-path entries and platform path-separator edge cases this substring + // check alone would miss) - this is a belt-and-suspenders addition, not a replacement. + if (sanitizedName.includes('..')) { + throw new Error(`Archive entry escapes extraction directory: ${entry.entryName}`); + } const outputPath = path.resolve(dirPath, sanitizedName); if (outputPath !== targetRoot && !outputPath.startsWith(targetRoot + path.sep)) { throw new Error(`Archive entry escapes extraction directory: ${entry.entryName}`);