From 305d04b758e1d7e5c9dc27e424b54d0da12d09ff Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Mon, 20 Apr 2026 19:07:18 +0100 Subject: [PATCH] perf(test): move temp path guard to check --- package.json | 1 + .../check-temp-path-guardrails.ts | 226 +++++++++++------- scripts/check.mjs | 1 + .../runtime-source-guardrail-scan.ts | 124 ---------- 4 files changed, 147 insertions(+), 205 deletions(-) rename src/security/temp-path-guard.test.ts => scripts/check-temp-path-guardrails.ts (54%) delete mode 100644 src/test-utils/runtime-source-guardrail-scan.ts diff --git a/package.json b/package.json index 1e3faca8a1f..e45f33a090f 100644 --- a/package.json +++ b/package.json @@ -1261,6 +1261,7 @@ "check:madge-import-cycles": "node --import tsx scripts/check-madge-import-cycles.ts", "check:no-conflict-markers": "node scripts/check-no-conflict-markers.mjs", "check:static-import-sccs": "pnpm check:madge-import-cycles", + "check:temp-path-guardrails": "node --import tsx scripts/check-temp-path-guardrails.ts", "check:test-types": "pnpm tsgo:test", "check:timed": "node scripts/check-timed.mjs", "check:timed:all-types": "node scripts/check-timed.mjs --include-test-types", diff --git a/src/security/temp-path-guard.test.ts b/scripts/check-temp-path-guardrails.ts similarity index 54% rename from src/security/temp-path-guard.test.ts rename to scripts/check-temp-path-guardrails.ts index cd49bb5801e..648dbb53950 100644 --- a/src/security/temp-path-guard.test.ts +++ b/scripts/check-temp-path-guardrails.ts @@ -1,8 +1,6 @@ -import { describe, expect, it } from "vitest"; -import { - loadRuntimeSourceFilesForGuardrails, - shouldSkipGuardrailRuntimeSource, -} from "../test-utils/runtime-source-guardrail-scan.js"; +import { execFileSync } from "node:child_process"; +import fs from "node:fs/promises"; +import path from "node:path"; type QuoteChar = "'" | '"' | "`"; @@ -10,19 +8,67 @@ type QuoteScanState = { quote: QuoteChar | null; escaped: boolean; }; + +type RuntimeSourceGuardrailFile = { + relativePath: string; + source: string; +}; + const WEAK_RANDOM_SAME_LINE_PATTERN = /(?:Date\.now[^\r\n]*Math\.random|Math\.random[^\r\n]*Date\.now)/u; const PATH_JOIN_CALL_PATTERN = /path\s*\.\s*join\s*\(/u; const OS_TMPDIR_CALL_PATTERN = /os\s*\.\s*tmpdir\s*\(/u; +const FILE_READ_CONCURRENCY = 24; +const DEFAULT_GUARDRAIL_SKIP_PATTERNS = [ + /\.test\.tsx?$/, + /\.test-helpers\.tsx?$/, + /\.test-utils\.tsx?$/, + /\.test-harness\.tsx?$/, + /\.test-support\.tsx?$/, + /\.suite\.tsx?$/, + /\.e2e\.tsx?$/, + /\.d\.ts$/, + /[\\/](?:__tests__|tests|test-helpers|test-utils|test-support)[\\/]/, + /[\\/][^\\/]*test-helpers(?:\.[^\\/]+)?\.ts$/, + /[\\/][^\\/]*test-utils(?:\.[^\\/]+)?\.ts$/, + /[\\/][^\\/]*test-harness(?:\.[^\\/]+)?\.ts$/, + /[\\/][^\\/]*test-support(?:\.[^\\/]+)?\.ts$/, +]; -function shouldSkip(relativePath: string): boolean { - return shouldSkipGuardrailRuntimeSource(relativePath); +function shouldSkipGuardrailRuntimeSource(relativePath: string): boolean { + return DEFAULT_GUARDRAIL_SKIP_PATTERNS.some((pattern) => pattern.test(relativePath)); } function stripCommentsForScan(input: string): string { return input.replace(/\/\*[\s\S]*?\*\//g, "").replace(/(^|[^:])\/\/.*$/gm, "$1"); } +function beginQuotedSection(state: QuoteScanState, ch: string): boolean { + if (ch !== "'" && ch !== '"' && ch !== "`") { + return false; + } + state.quote = ch; + return true; +} + +function consumeQuotedChar(state: QuoteScanState, ch: string): boolean { + if (!state.quote) { + return false; + } + if (state.escaped) { + state.escaped = false; + return true; + } + if (ch === "\\") { + state.escaped = true; + return true; + } + if (ch === state.quote) { + state.quote = null; + } + return true; +} + function findMatchingParen(source: string, openIndex: number): number { let depth = 1; const quoteState: QuoteScanState = { quote: null, escaped: false }; @@ -115,32 +161,6 @@ function splitTopLevelArguments(source: string): string[] { return out; } -function beginQuotedSection(state: QuoteScanState, ch: string): boolean { - if (ch !== "'" && ch !== '"' && ch !== "`") { - return false; - } - state.quote = ch; - return true; -} - -function consumeQuotedChar(state: QuoteScanState, ch: string): boolean { - if (!state.quote) { - return false; - } - if (state.escaped) { - state.escaped = false; - return true; - } - if (ch === "\\") { - state.escaped = true; - return true; - } - if (ch === state.quote) { - state.quote = null; - } - return true; -} - function isOsTmpdirExpression(argument: string): boolean { return /^os\s*\.\s*tmpdir\s*\(\s*\)$/u.test(argument.trim()); } @@ -187,60 +207,104 @@ function hasDynamicTmpdirJoin(source: string): boolean { return false; } -describe("temp path guard", () => { - it("skips test helper filename variants", () => { - expect(shouldSkip("src/commands/test-helpers.ts")).toBe(true); - expect(shouldSkip("src/commands/sessions.test-helpers.ts")).toBe(true); - expect(shouldSkip("src\\commands\\sessions.test-helpers.ts")).toBe(true); - expect(shouldSkip("src/plugins/test-helpers/fs-fixtures.ts")).toBe(true); +function listTrackedRuntimeSourceFiles(repoRoot: string): string[] { + const stdout = execFileSync("git", ["-C", repoRoot, "ls-files", "--", "src", "extensions"], { + encoding: "utf8", + stdio: ["ignore", "pipe", "inherit"], }); + return stdout + .split(/\r?\n/u) + .filter(Boolean) + .filter((relativePath) => relativePath.endsWith(".ts") || relativePath.endsWith(".tsx")) + .filter((relativePath) => !shouldSkipGuardrailRuntimeSource(relativePath)) + .map((relativePath) => path.join(repoRoot, relativePath)); +} - it("detects dynamic and ignores static fixtures", () => { - const dynamicFixtures = [ - "const p = path.join(os.tmpdir(), `openclaw-${id}`);", - "const p = path.join(os.tmpdir(), 'safe', `${token}`);", - ]; - const staticFixtures = [ - "const p = path.join(os.tmpdir(), 'openclaw-fixed');", - "const p = path.join(os.tmpdir(), `openclaw-fixed`);", - "const p = path.join(os.tmpdir(), prefix + '-x');", - "const p = path.join(os.tmpdir(), segment);", - "const p = path.join('/tmp', `openclaw-${id}`);", - "// path.join(os.tmpdir(), `openclaw-${id}`)", - "const p = path.join(os.tmpdir());", - ]; - - expect(dynamicFixtures.every((fixture) => hasDynamicTmpdirJoin(fixture))).toBe(true); - expect(staticFixtures.every((fixture) => !hasDynamicTmpdirJoin(fixture))).toBe(true); +async function readRuntimeSourceFiles( + repoRoot: string, + absolutePaths: string[], +): Promise { + const output: Array = Array.from({ + length: absolutePaths.length, }); + let nextIndex = 0; - it("enforces runtime guardrails for tmpdir joins and weak randomness", async () => { - const files = await loadRuntimeSourceFilesForGuardrails(process.cwd()); - const offenders: string[] = []; - const weakRandomMatches: string[] = []; - - for (const file of files) { - const relativePath = file.relativePath; - const source = file.source; - const mightContainTmpdirJoin = - source.includes("tmpdir") && - source.includes("path") && - source.includes("join") && - source.includes("`"); - const mightContainWeakRandom = source.includes("Date.now") && source.includes("Math.random"); - - if (!mightContainTmpdirJoin && !mightContainWeakRandom) { + const worker = async () => { + for (;;) { + const index = nextIndex; + nextIndex += 1; + if (index >= absolutePaths.length) { + return; + } + const absolutePath = absolutePaths[index]; + if (!absolutePath) { continue; } - if (mightContainTmpdirJoin && hasDynamicTmpdirJoin(source)) { - offenders.push(relativePath); - } - if (mightContainWeakRandom && WEAK_RANDOM_SAME_LINE_PATTERN.test(source)) { - weakRandomMatches.push(relativePath); + let source: string; + try { + source = await fs.readFile(absolutePath, "utf8"); + } catch { + // File tracked by git but deleted on disk (e.g. pending deletion). + continue; } + output[index] = { + relativePath: path.relative(repoRoot, absolutePath), + source, + }; } + }; - expect(offenders).toEqual([]); - expect(weakRandomMatches).toEqual([]); - }); -}); + const workers = Array.from( + { length: Math.min(FILE_READ_CONCURRENCY, Math.max(1, absolutePaths.length)) }, + () => worker(), + ); + await Promise.all(workers); + return output.filter((entry): entry is RuntimeSourceGuardrailFile => entry !== undefined); +} + +async function main() { + const repoRoot = process.cwd(); + const files = await readRuntimeSourceFiles(repoRoot, listTrackedRuntimeSourceFiles(repoRoot)); + const offenders: string[] = []; + const weakRandomMatches: string[] = []; + + for (const file of files) { + const source = file.source; + const mightContainTmpdirJoin = + source.includes("tmpdir") && + source.includes("path") && + source.includes("join") && + source.includes("`"); + const mightContainWeakRandom = source.includes("Date.now") && source.includes("Math.random"); + + if (!mightContainTmpdirJoin && !mightContainWeakRandom) { + continue; + } + if (mightContainTmpdirJoin && hasDynamicTmpdirJoin(source)) { + offenders.push(file.relativePath); + } + if (mightContainWeakRandom && WEAK_RANDOM_SAME_LINE_PATTERN.test(source)) { + weakRandomMatches.push(file.relativePath); + } + } + + if (offenders.length === 0 && weakRandomMatches.length === 0) { + return; + } + + if (offenders.length > 0) { + console.error("Dynamic os.tmpdir()/path.join() template paths found:"); + for (const offender of offenders) { + console.error(`- ${offender}`); + } + } + if (weakRandomMatches.length > 0) { + console.error("Weak Date.now()+Math.random() same-line IDs found:"); + for (const offender of weakRandomMatches) { + console.error(`- ${offender}`); + } + } + process.exitCode = 1; +} + +await main(); diff --git a/scripts/check.mjs b/scripts/check.mjs index 3747d4490a9..e2f94c1b01b 100644 --- a/scripts/check.mjs +++ b/scripts/check.mjs @@ -9,6 +9,7 @@ export async function main(argv = process.argv.slice(2)) { const tailChecks = [ { name: "webhook body guard", args: ["lint:webhook:no-low-level-body-read"] }, + { name: "temp path guard", args: ["check:temp-path-guardrails"] }, { name: "pairing store guard", args: ["lint:auth:no-pairing-store-group"] }, { name: "pairing account guard", args: ["lint:auth:pairing-account-scope"] }, includeArchitecture diff --git a/src/test-utils/runtime-source-guardrail-scan.ts b/src/test-utils/runtime-source-guardrail-scan.ts deleted file mode 100644 index bcd4d550560..00000000000 --- a/src/test-utils/runtime-source-guardrail-scan.ts +++ /dev/null @@ -1,124 +0,0 @@ -import { execFileSync } from "node:child_process"; -import fs from "node:fs/promises"; -import path from "node:path"; -import { listRuntimeSourceFiles } from "./repo-scan.js"; - -export type RuntimeSourceGuardrailFile = { - relativePath: string; - source: string; -}; - -const DEFAULT_GUARDRAIL_SKIP_PATTERNS = [ - /\.test\.tsx?$/, - /\.test-helpers\.tsx?$/, - /\.test-utils\.tsx?$/, - /\.test-harness\.tsx?$/, - /\.test-support\.tsx?$/, - /\.suite\.tsx?$/, - /\.e2e\.tsx?$/, - /\.d\.ts$/, - /[\\/](?:__tests__|tests|test-helpers|test-utils|test-support)[\\/]/, - /[\\/][^\\/]*test-helpers(?:\.[^\\/]+)?\.ts$/, - /[\\/][^\\/]*test-utils(?:\.[^\\/]+)?\.ts$/, - /[\\/][^\\/]*test-harness(?:\.[^\\/]+)?\.ts$/, - /[\\/][^\\/]*test-support(?:\.[^\\/]+)?\.ts$/, -]; - -const runtimeSourceGuardrailCache = new Map>(); -const trackedRuntimeSourceListCache = new Map(); -const FILE_READ_CONCURRENCY = 24; - -export function shouldSkipGuardrailRuntimeSource(relativePath: string): boolean { - return DEFAULT_GUARDRAIL_SKIP_PATTERNS.some((pattern) => pattern.test(relativePath)); -} - -async function readRuntimeSourceFiles( - repoRoot: string, - absolutePaths: string[], -): Promise { - const output: Array = Array.from({ - length: absolutePaths.length, - }); - let nextIndex = 0; - - const worker = async () => { - for (;;) { - const index = nextIndex; - nextIndex += 1; - if (index >= absolutePaths.length) { - return; - } - const absolutePath = absolutePaths[index]; - if (!absolutePath) { - continue; - } - let source: string; - try { - source = await fs.readFile(absolutePath, "utf8"); - } catch { - // File tracked by git but deleted on disk (e.g. pending deletion). - continue; - } - output[index] = { - relativePath: path.relative(repoRoot, absolutePath), - source, - }; - } - }; - - const workers = Array.from( - { length: Math.min(FILE_READ_CONCURRENCY, Math.max(1, absolutePaths.length)) }, - () => worker(), - ); - await Promise.all(workers); - return output.filter((entry): entry is RuntimeSourceGuardrailFile => entry !== undefined); -} - -function tryListTrackedRuntimeSourceFiles(repoRoot: string): string[] | null { - const cached = trackedRuntimeSourceListCache.get(repoRoot); - if (cached) { - return cached.slice(); - } - - try { - const stdout = execFileSync("git", ["-C", repoRoot, "ls-files", "--", "src", "extensions"], { - encoding: "utf8", - stdio: ["ignore", "pipe", "ignore"], - }); - const files = stdout - .split(/\r?\n/u) - .filter(Boolean) - .filter((relativePath) => relativePath.endsWith(".ts") || relativePath.endsWith(".tsx")) - .filter((relativePath) => !shouldSkipGuardrailRuntimeSource(relativePath)) - .map((relativePath) => path.join(repoRoot, relativePath)); - trackedRuntimeSourceListCache.set(repoRoot, files); - return files.slice(); - } catch { - return null; - } -} - -export async function loadRuntimeSourceFilesForGuardrails( - repoRoot: string, -): Promise { - let pending = runtimeSourceGuardrailCache.get(repoRoot); - if (!pending) { - pending = (async () => { - const trackedFiles = tryListTrackedRuntimeSourceFiles(repoRoot); - const sourceFiles = - trackedFiles ?? - ( - await listRuntimeSourceFiles(repoRoot, { - roots: ["src", "extensions"], - extensions: [".ts", ".tsx"], - }) - ).filter((absolutePath) => { - const relativePath = path.relative(repoRoot, absolutePath); - return !shouldSkipGuardrailRuntimeSource(relativePath); - }); - return await readRuntimeSourceFiles(repoRoot, sourceFiles); - })(); - runtimeSourceGuardrailCache.set(repoRoot, pending); - } - return await pending; -}