From 25026369094fb6efaafc4cba390eeb595e49398f Mon Sep 17 00:00:00 2001 From: awalsh128 Date: Fri, 6 Nov 2026 09:51:27 -0800 Subject: [PATCH] fix: archive and restore packages with system tar under sudo The in-process tar library ran as the runner user, so archiving failed on root-only files such as /etc/default/cacerts from ca-certificates-java (regression #72), and extraction could skip root-owned paths without failing. - Create and extract with sudo tar; the file list is piped NUL-separated on stdin so large packages don't exceed the argument size limit. - Keep directory symlinks such as /lib on extract. - Hand archives back to the runner user for the cache save. - Disable the command timeout for tar and maintainer scripts. - Record the installed (versioned) entries in manifest_main.json; it held unversioned names with no files. - Return every installed package on a cache hit so outputs include deps. - Split runInstallScripts into per-script runs. Co-Authored-By: Claude Opus 5.5 --- src/cache.ts | 156 +++++++++++++++++++++++++------------ test/cache-restore.test.ts | 97 ++++++++++++++++++----- 2 files changed, 187 insertions(+), 66 deletions(-) diff --git a/src/cache.ts b/src/cache.ts index 2ddd22d..b36285a 100644 --- a/src/cache.ts +++ b/src/cache.ts @@ -1,6 +1,5 @@ import * as ghcache from "@actions/cache"; import * as winston from "winston"; -import * as tar from "tar"; import * as crypto from "node:crypto"; import { promises as fs } from "node:fs"; import * as os from "node:os"; @@ -24,6 +23,29 @@ export function resolveCacheDir( return path.join(os.homedir(), cacheDir); } +/** + * Disables the command runner's default timeout for long-running commands. + * + * Archiving large packages (e.g. linux-headers) and some maintainer scripts + * (e.g. ca-certificates-java postinst) can take longer than the default. + */ +const NO_TIMEOUT_MS = -1; + +/** Maintainer scripts run on restore when install scripts are enabled. */ +type InstallScriptType = "preinst" | "postinst"; + +/** Order dpkg runs maintainer scripts in during install. */ +const INSTALL_SCRIPT_ORDER: readonly InstallScriptType[] = [ + "preinst", + "postinst", +]; + +/** Action argument dpkg passes to each maintainer script on a fresh install. */ +const INSTALL_SCRIPT_ACTIONS: Readonly> = { + preinst: "install", + postinst: "configure", +}; + export const MANIFEST_MAIN_FILENAME = "manifest_main.json"; export const MANIFEST_ALL_FILENAME = "manifest_all.json"; @@ -123,33 +145,43 @@ export class Cache { this.logger = logger; } + /** + * Runs a package's maintainer scripts in the order dpkg runs them on install. + * + * @param packageName Package whose scripts to run. + */ private async runInstallScripts(packageName: PackageName): Promise { - const runScript = async ( - packageName: PackageName, - scriptType: "preinst" | "postinst", - ) => { - const scriptPath = await findInstallScript(packageName, scriptType, "/"); - if (!scriptPath) { - this.logger.info( - `No ${scriptType} script found for package ${packageName.serialize()}, skipping`, - ); - return; - } - const scriptName = path.basename(scriptPath); - this.logger.info( - `Running ${scriptType} script '${scriptName}' for package ${packageName.serialize()}...`, - ); - await this.commandRunner.run("sudo", [ - "sh", - "-x", - scriptPath, - scriptType === "preinst" ? "install" : "configure", - ]); - this.logger.info(`Ran ${scriptType} script for ${packageName.name}`); - }; + for (const scriptType of INSTALL_SCRIPT_ORDER) { + await this.runInstallScript(packageName, scriptType); + } + } - await runScript(packageName, "preinst"); - await runScript(packageName, "postinst"); + /** + * Runs one maintainer script, if the package has it, with the action dpkg passes. + * + * @param packageName Package whose script to run. + * @param scriptType Maintainer script to run. + */ + private async runInstallScript( + packageName: PackageName, + scriptType: InstallScriptType, + ): Promise { + const scriptPath = await findInstallScript(packageName, scriptType, "/"); + if (!scriptPath) { + this.logger.info( + `No ${scriptType} script found for package ${packageName.serialize()}, skipping`, + ); + return; + } + this.logger.info( + `Running ${scriptType} script '${path.basename(scriptPath)}' for package ${packageName.serialize()}...`, + ); + await this.commandRunner.run( + "sudo", + ["sh", "-x", scriptPath, INSTALL_SCRIPT_ACTIONS[scriptType]], + { timeoutMs: NO_TIMEOUT_MS }, + ); + this.logger.info(`Ran ${scriptType} script for ${packageName.name}`); } async loadAndRestore( @@ -227,18 +259,37 @@ export class Cache { } for (const { packageName, archivePath } of archivePaths) { - await tar.extract({ - cwd: "/", - file: archivePath, - preservePaths: true, - }); + await this.extractArchive(archivePath); if (executeInstallScripts) { await this.runInstallScripts(packageName); } } - return await Manifest.readFromFile(manifestPath); + // Return every installed package so outputs can report dependencies too. + return manifestAll; + } + + /** + * Extracts a package archive onto the root filesystem. + * + * Runs system tar under sudo because package files live in root-owned + * directories. Fails on any entry error instead of skipping it. + * + * @param archivePath Absolute path to the package archive. + */ + private async extractArchive(archivePath: string): Promise { + await this.commandRunner.run( + "sudo", + [ + "tar", + "--extract", + `--file=${archivePath}`, + "--directory=/", + "--keep-directory-symlink", + ], + { timeoutMs: NO_TIMEOUT_MS }, + ); } private async createArchive(manifestEntry: ManifestEntry): Promise { @@ -267,17 +318,29 @@ export class Cache { this.logger.info( `Archiving package ${manifestEntry.packageName.serialize()} with ${manifestEntry.filepaths.length} files...`, ); - await tar.create( - { - cwd: "/", - file: archivePath, - portable: false, - preservePaths: false, - follow: false, - noDirRecurse: false, - }, - manifestEntry.filepaths, + // Packages can install root-only files (e.g. /etc/default/cacerts from + // ca-certificates-java), so system tar runs under sudo. The file list is + // piped on stdin because large packages exceed the argument size limit, + // and is NUL-separated so no path is parsed as a tar option. + await this.commandRunner.run( + "sudo", + [ + "tar", + "--create", + `--file=${archivePath}`, + "--directory=/", + "--null", + "--verbatim-files-from", + "--files-from=-", + ], + { input: manifestEntry.filepaths.join("\0"), timeoutMs: NO_TIMEOUT_MS }, ); + // Hand the archive back to the runner user so the cache save can manage it. + await this.commandRunner.run("sudo", [ + "chown", + `${os.userInfo().uid}:${os.userInfo().gid}`, + archivePath, + ]); this.logger.info( `Archive created for package ${manifestEntry.packageName.serialize()} at ${archivePath}`, ); @@ -286,13 +349,10 @@ export class Cache { private async writeManifests(manifest: Manifest): Promise { const allEntries = manifest.entries; const packages = manifest.cacheKey.packageNames; - const entriesByName = new Map( - allEntries.map((entry) => [entry.packageName.serialize(), entry]), + // Keep the installed name, which carries the resolved version and arch. + const mainEntries = allEntries.filter((entry) => + packages.includes(entry.packageName), ); - const mainEntries = packages.toArray().map((pkg) => { - const installed = entriesByName.get(pkg.serialize()); - return new ManifestEntry(pkg, installed?.filepaths ?? []); - }); const write = async (manifest: Manifest, filename: string) => { this.logger.info( diff --git a/test/cache-restore.test.ts b/test/cache-restore.test.ts index 5b9e43f..991f524 100644 --- a/test/cache-restore.test.ts +++ b/test/cache-restore.test.ts @@ -1,8 +1,11 @@ import fs from "node:fs"; import path from "node:path"; import * as ghcache from "@actions/cache"; -import * as tar from "tar"; -import { createPackageName, type PackageName } from "ts-apt"; +import { + createPackageName, + type CommandOptions, + type PackageName, +} from "ts-apt"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import winston from "winston"; @@ -37,7 +40,6 @@ vi.mock("@actions/cache", () => ({ saveCache: vi.fn(), isFeatureAvailable: vi.fn(), })); -vi.mock("tar", () => ({ extract: vi.fn(), create: vi.fn() })); const CACHE_DIR = path.join(HOME_DIR, CACHE_DEFAULT_DIRNAME); const CACHE_ID = 42; @@ -81,9 +83,34 @@ async function writeCache(archivedPackages = INSTALL_ORDER): Promise { archivedPackages.forEach((pkg) => fs.writeFileSync(archivePath(pkg), "")); } -function createCache(): Cache { +/** Archive operations observed through the command runner. */ +interface TarCalls { + created: [archive: string, files: string[]][]; + extracted: string[]; +} + +const argValue = (args: string[], flag: string) => + args.find((arg) => arg.startsWith(`${flag}=`))?.slice(flag.length + 1); + +/** Records sudo tar calls, including the file list piped to tar create. */ +function recordTar(calls: TarCalls) { + return vi.fn( + async (_command: string, args: string[] = [], options: CommandOptions = {}) => { + const archive = argValue(args, "--file") ?? ""; + if (args.includes("--create")) { + calls.created.push([archive, String(options.input ?? "").split("\0")]); + } + if (args.includes("--extract")) { + calls.extracted.push(archive); + } + return { cmdLine: "", stdout: "", stderr: "", exitCode: 0 }; + }, + ); +} + +function createCache(calls: TarCalls = { created: [], extracted: [] }): Cache { const logger = winston.createLogger({ silent: true }); - return new Cache({ run: vi.fn() } as never, logger); + return new Cache({ run: recordTar(calls) } as never, logger); } describe("Cache", () => { @@ -101,33 +128,34 @@ describe("Cache", () => { it("loadAndRestore cache hit extracts archives in install order", async () => { await writeCache(); - const manifest = await createCache().loadAndRestore(KEY, false); + const calls: TarCalls = { created: [], extracted: [] }; + + const manifest = await createCache(calls).loadAndRestore(KEY, false); expect(manifest).toBeDefined(); - expect( - vi.mocked(tar.extract).mock.calls.map(([options]) => options?.file), - ).toEqual(INSTALL_ORDER.map(archivePath)); + expect(calls.extracted).toEqual(INSTALL_ORDER.map(archivePath)); }); it("loadAndRestore archive missing returns undefined without extracting", async () => { await writeCache(INSTALL_ORDER.slice(1)); - const manifest = await createCache().loadAndRestore(KEY, false); + const calls: TarCalls = { created: [], extracted: [] }; + + const manifest = await createCache(calls).loadAndRestore(KEY, false); expect(manifest).toBeUndefined(); - expect(tar.extract).not.toHaveBeenCalled(); + expect(calls.extracted).toEqual([]); }); it("archiveAndSave packages with files creates archives of their files", async () => { - const cacheId = await createCache().archiveAndSave(MANIFEST); + const calls: TarCalls = { created: [], extracted: [] }; + + const cacheId = await createCache(calls).archiveAndSave(MANIFEST); expect(cacheId).toBe(CACHE_ID); - // tar.create is overloaded, so type the (options, files) call shape used. - const createCalls = vi.mocked(tar.create).mock.calls as unknown as [ - { file: string }, - string[], - ][]; - expect(createCalls.map(([options, files]) => [options.file, files])).toEqual(INSTALL_ORDER.map((pkg) => [archivePath(pkg), filesOf(pkg)])); + expect(calls.created).toEqual( + INSTALL_ORDER.map((pkg) => [archivePath(pkg), filesOf(pkg)]), + ); }); it("archiveAndSave manifest written records package files", async () => { @@ -140,4 +168,37 @@ describe("Cache", () => { MANIFEST.entries.map((entry) => entry.filepaths), ); }); + + it("loadAndRestore cache hit returns all installed packages", async () => { + await writeCache(); + + const manifest = await createCache().loadAndRestore(KEY, false); + + expect( + manifest?.entries.map((entry) => entry.packageName.serialize()), + ).toEqual(MANIFEST.entries.map((entry) => entry.packageName.serialize())); + }); + + it("archiveAndSave requested package without version records installed entry in main manifest", async () => { + const unversionedKey = new CacheKey( + "v1", + "1", + "x86_64", + ActionPackageNames.fromInput(REQUESTED_PACKAGE.name), + ); + + await createCache().archiveAndSave( + new Manifest(MANIFEST.created, MANIFEST.entries, unversionedKey), + ); + + const written = await Manifest.readFromFile( + path.join(CACHE_DIR, MANIFEST_MAIN_FILENAME), + ); + expect( + written.entries.map((entry) => [ + entry.packageName.serialize(), + entry.filepaths, + ]), + ).toEqual([[REQUESTED_PACKAGE.serialize(), filesOf(REQUESTED_PACKAGE)]]); + }); });