From 0a66dd4b902fa926ba311600befb2d004ae26016 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 16 Oct 2026 03:33:27 +0000 Subject: [PATCH 1/2] feat(scripts): validate-urls takes a file list The import lanes run `--changed-only --base main` before anything is committed, so the diff is empty and the URL check ran on zero files. `--files` reads listed paths through `checkContainedRegularFile`; the git call is an argv array. Part of AUREO-1474 Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_013X2d8vEWjRnkZydpXo94JF --- CLAUDE.md | 2 + scripts/validate-urls.ts | 148 +++++++++++++++++++++++++++------------ 2 files changed, 107 insertions(+), 43 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 7d6807f30..838c71b89 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -44,6 +44,8 @@ Community-driven database of audio software, plugins, DAWs, and hardware for mus of text (`--tsv` for the review list) - `pnpm prose-layout:apply` - Apply the lossless layout fixes (`--write` to apply) +- `pnpm validate-urls --files ` - Check the URLs of the data files + listed one per line (`-` reads stdin), before an import has committed - `pnpm format:check` - Check formatting Five of the audits above take `--findings `: `power-input-audit`, diff --git a/scripts/validate-urls.ts b/scripts/validate-urls.ts index 938da1a1b..33ccaf976 100644 --- a/scripts/validate-urls.ts +++ b/scripts/validate-urls.ts @@ -7,14 +7,17 @@ * Usage: * pnpm validate-urls # Validate all files * pnpm validate-urls --changed-only --base # Validate only changed files + * pnpm validate-urls --files # Validate the data files listed + * # one per line (`-` reads stdin) * pnpm validate-urls --use-cache # Use cached results * pnpm validate-urls --update-cache # Update cache with results * pnpm validate-urls --ignore-cache # Force recheck all URLs */ -import { execSync } from "node:child_process"; +import { execFileSync } from "node:child_process"; import fs from "node:fs"; import path from "node:path"; +import { pathToFileURL } from "node:url"; import { parse as parseYaml } from "yaml"; import { getCachedUrl, @@ -178,64 +181,109 @@ interface ValidationResult { interface ParsedArgs { changedOnly: boolean; baseSha?: string; + // Path of a file listing the data files to check, `-` for stdin. + filesList?: string; useCache: boolean; updateCache: boolean; ignoreCache: boolean; } -function parseArgs(): ParsedArgs { - const args = process.argv.slice(2); +export function parseArgs(args: string[]): ParsedArgs { const changedOnly = args.includes("--changed-only"); const baseIndex = args.indexOf("--base"); const baseSha = baseIndex !== -1 ? args[baseIndex + 1] : undefined; + const filesIndex = args.indexOf("--files"); + const filesList = filesIndex !== -1 ? args[filesIndex + 1] : undefined; const useCache = args.includes("--use-cache"); const updateCache = args.includes("--update-cache"); const ignoreCache = args.includes("--ignore-cache"); - return { changedOnly, baseSha, useCache, updateCache, ignoreCache }; + if (changedOnly && !baseSha) { + throw new Error("--changed-only requires --base "); + } + if (filesIndex !== -1 && !filesList) { + throw new Error("--files requires a path (or - for stdin)"); + } + // Each flag names the file set on its own; silently taking the union + // would hide which lane the caller meant. + if (changedOnly && filesList !== undefined) { + throw new Error("--changed-only and --files are mutually exclusive"); + } + + return { changedOnly, baseSha, filesList, useCache, updateCache, ignoreCache }; +} + +/** + * Resolve a list of data paths, one per line, to the files it names. The + * pathname match is lexical, so it says nothing about what is at the path. + * A pull request can commit a symlink at data/software/x.yaml, and + * existsSync/readFileSync would both follow it: pointed at /dev/urandom + * that is an unbounded read on the runner. checkContainedRegularFile + * refuses a symlink, a device, and anything whose canonical path leaves + * `root`, and a refused path is reported and never read. + */ +export function resolveListedFiles( + list: string, + root: string = DATA_DIR, + options: { ignoreMissing?: boolean } = {} +): string[] { + const files: string[] = []; + for (const line of list.split("\n")) { + const relative = line.trim(); + if (relative === "") continue; + const checked = checkContainedRegularFile(path.resolve(relative), root); + if (checked.path) { + files.push(checked.path); + } else if (!(options.ignoreMissing && checked.reason === "does not exist")) { + console.error(`Skipping ${relative}: ${checked.reason}`); + } + } + return files; } // Get list of changed YAML files in data/ function getChangedFiles(baseSha: string): string[] { - // Validate SHA format to prevent command injection + // The sha reaches git as one argv entry, so a shell never sees it. The + // format check stays because a ref that is not a sha is a caller mistake + // worth naming rather than something to hand to git. if (!/^[a-f0-9]{7,40}$/i.test(baseSha)) { console.error(`Invalid git SHA format: ${baseSha}`); return []; } try { - const output = execSync(`git diff --name-only ${baseSha} HEAD`, { + const output = execFileSync("git", ["diff", "--name-only", baseSha, "HEAD"], { encoding: "utf-8", }); - // The pathname match is lexical, so it says nothing about what is at - // the path. A pull request can commit a symlink at - // data/software/x.yaml, and existsSync/readFileSync would both follow - // it: pointed at /dev/urandom that is an unbounded read on the runner. - // checkContainedRegularFile refuses a symlink, a device, and anything - // whose canonical path leaves data/. - const changed: string[] = []; - for (const relative of output.split("\n")) { - if ( - !relative.match(/^data\/(software|content|hardware|accessories|manufacturers)\/.*\.yaml$/) - ) { - continue; - } - const checked = checkContainedRegularFile(path.join(process.cwd(), relative), DATA_DIR); - if (checked.path) { - changed.push(checked.path); - } else if (checked.reason !== "does not exist") { - // A deleted file is nothing to check; anything else is worth saying. - console.error(`Skipping ${relative}: ${checked.reason}`); - } - } - return changed; + const changed = output + .split("\n") + .filter((relative) => + /^data\/(software|content|hardware|accessories|manufacturers)\/.*\.yaml$/.test(relative) + ) + .join("\n"); + // A deleted file is nothing to check; anything else is worth saying. + return resolveListedFiles(changed, DATA_DIR, { ignoreMissing: true }); } catch (error) { console.error("Failed to get changed files:", error); return []; } } +// The listed files for `--files`: a file on disk, or stdin for `-`. The +// list itself is a path the caller chose, so it goes through the guard +// before it is read, like every path this script did not pick. +function readFilesList(filesList: string): string { + if (filesList === "-") { + return fs.readFileSync(0, "utf-8"); + } + const checked = checkContainedRegularFile(filesList, process.cwd()); + if (!checked.path) { + throw new Error(`Cannot read --files ${filesList}: ${checked.reason}`); + } + return fs.readFileSync(checked.path, "utf-8"); +} + // Extract all URLs from a parsed YAML object into a shared set, stopping once // `limit` unique URLs have been collected. The caller passes maxUrlsPerFile + 1 // so a file over budget is provably over it without walking the rest of a @@ -524,6 +572,7 @@ async function processFile( async function validate( changedOnly: boolean, baseSha?: string, + filesList?: string, cacheOptions?: { useCache: boolean; updateCache: boolean; @@ -541,6 +590,9 @@ async function validate( ); } console.log(`Checking ${files.length} changed file(s)...`); + } else if (filesList !== undefined) { + files = resolveListedFiles(readFilesList(filesList)); + console.log(`Checking ${files.length} listed file(s)...`); } else { // Get all YAML files files = [ @@ -569,8 +621,12 @@ async function validate( } // The changed-file path runs on every PR against attacker-supplied YAML, so - // it is capped and deadlined. A full-catalog run is scheduled and trusted. - const budget: RunBudget | undefined = changedOnly + // it is capped and deadlined, and a listed run gets the same budget: it is + // the import lanes' pre-commit form of the same check, and a file that + // would fail the PR runner should fail here too. A full-catalog run is + // scheduled and trusted. + const scoped = changedOnly || filesList !== undefined; + const budget: RunBudget | undefined = scoped ? { signal: AbortSignal.timeout(RUN_DEADLINE_MS), maxUrlsPerFile: MAX_URLS_PER_FILE, @@ -753,20 +809,26 @@ function writeConsoleOutput(result: ValidationResult): void { } // Main -const { changedOnly, baseSha, useCache, updateCache, ignoreCache } = parseArgs(); +async function main(): Promise { + const { changedOnly, baseSha, filesList, useCache, updateCache, ignoreCache } = parseArgs( + process.argv.slice(2) + ); -if (changedOnly && !baseSha) { - console.error("Error: --changed-only requires --base "); - process.exit(1); -} + const result = await validate(changedOnly, baseSha, filesList, { + useCache, + updateCache, + ignoreCache, + }); -const result = await validate(changedOnly, baseSha, { - useCache, - updateCache, - ignoreCache, -}); + writeConsoleOutput(result); + writeGitHubSummary(result); -writeConsoleOutput(result); -writeGitHubSummary(result); + process.exit(result.valid ? 0 : 1); +} -process.exit(result.valid ? 0 : 1); +if (process.argv[1] && import.meta.url === pathToFileURL(process.argv[1]).href) { + main().catch((error) => { + console.error(`Error: ${error instanceof Error ? error.message : String(error)}`); + process.exit(1); + }); +} From 2fc9caeb3f885f29338105fb545084d12718a9db Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 16 Oct 2026 03:34:39 +0000 Subject: [PATCH 2/2] test(scripts): cover validate-urls --files Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_013X2d8vEWjRnkZydpXo94JF --- scripts/__tests__/validate-urls.test.ts | 92 +++++++++++++++++++++++++ 1 file changed, 92 insertions(+) create mode 100644 scripts/__tests__/validate-urls.test.ts diff --git a/scripts/__tests__/validate-urls.test.ts b/scripts/__tests__/validate-urls.test.ts new file mode 100644 index 000000000..0cd5c1b3e --- /dev/null +++ b/scripts/__tests__/validate-urls.test.ts @@ -0,0 +1,92 @@ +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { afterEach, describe, expect, it, vi } from "vitest"; + +// The file-selection half of the script is what is under test, and nothing +// here names a URL. The guard is stubbed anyway, so that a regression that +// reached the network would fail here instead of making a request. +vi.mock("../lib/url-guard.js", () => ({ + fetchPublic: vi.fn(() => Promise.reject(new Error("network disabled in tests"))), + isPrivateDestinationError: () => false, +})); + +import { parseArgs, resolveListedFiles } from "../validate-urls.js"; + +describe("parseArgs", () => { + it("reads --files and leaves the git lane off", () => { + expect(parseArgs(["--files", "changed.txt"])).toMatchObject({ + changedOnly: false, + filesList: "changed.txt", + }); + expect(parseArgs(["--files", "-"]).filesList).toBe("-"); + }); + + it("refuses --files together with --changed-only", () => { + expect(() => parseArgs(["--changed-only", "--base", "abc1234", "--files", "x"])).toThrow( + /mutually exclusive/ + ); + }); + + it("still requires --base with --changed-only", () => { + expect(() => parseArgs(["--changed-only"])).toThrow(/--base/); + }); +}); + +describe("resolveListedFiles", () => { + const dirs: string[] = []; + afterEach(() => { + for (const dir of dirs.splice(0)) fs.rmSync(dir, { recursive: true, force: true }); + vi.restoreAllMocks(); + }); + const tmpRoot = (): string => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), "catalog-validate-urls-")); + dirs.push(dir); + const root = path.join(dir, "data"); + fs.mkdirSync(path.join(root, "software"), { recursive: true }); + return root; + }; + + it("resolves only the listed files, skipping blank lines", () => { + const root = tmpRoot(); + const listed = path.join(root, "software", "a.yaml"); + const unlisted = path.join(root, "software", "b.yaml"); + fs.writeFileSync(listed, "name: A\n"); + fs.writeFileSync(unlisted, "name: B\n"); + + expect(resolveListedFiles(`${listed}\n\n`, root)).toEqual([fs.realpathSync(listed)]); + }); + + it("refuses a path that leaves the root and never reads it", () => { + const root = tmpRoot(); + const outside = path.join(root, "..", "outside.yaml"); + fs.writeFileSync(outside, "url: https://example.com\n"); + const stderr = vi.spyOn(console, "error").mockImplementation(() => {}); + + expect( + resolveListedFiles(`${path.join(root, "software", "..", "..", "outside.yaml")}\n`, root) + ).toEqual([]); + expect(stderr).toHaveBeenCalledWith(expect.stringMatching(/outside\.yaml.*is outside/)); + }); + + it("refuses a symlink inside the root", () => { + const root = tmpRoot(); + const link = path.join(root, "software", "x.yaml"); + fs.symlinkSync("/dev/null", link); + const stderr = vi.spyOn(console, "error").mockImplementation(() => {}); + + expect(resolveListedFiles(link, root)).toEqual([]); + expect(stderr).toHaveBeenCalledWith(expect.stringMatching(/is a symlink/)); + }); + + it("reports a listed file that does not exist unless told not to", () => { + const root = tmpRoot(); + const missing = path.join(root, "software", "gone.yaml"); + const stderr = vi.spyOn(console, "error").mockImplementation(() => {}); + + expect(resolveListedFiles(missing, root)).toEqual([]); + expect(stderr).toHaveBeenCalledTimes(1); + expect(resolveListedFiles(missing, root, { ignoreMissing: true })).toEqual([]); + expect(stderr).toHaveBeenCalledTimes(1); + }); +});