From c1655982d4f98c09c497b196999ee336328fa6f3 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Mon, 16 Feb 2026 03:35:56 +0100 Subject: [PATCH] refactor: centralize pre-commit file filtering --- git-hooks/pre-commit | 33 ++++++----- scripts/pre-commit/filter-staged-files.mjs | 39 +++++++++++++ test/git-hooks-pre-commit.integration.test.ts | 55 +++++++++++++++++++ test/git-hooks-pre-commit.test.ts | 7 ++- vitest.config.ts | 7 +-- 5 files changed, 119 insertions(+), 22 deletions(-) create mode 100644 scripts/pre-commit/filter-staged-files.mjs create mode 100644 test/git-hooks-pre-commit.integration.test.ts diff --git a/git-hooks/pre-commit b/git-hooks/pre-commit index 1b4475b6fb..919e8507bb 100755 --- a/git-hooks/pre-commit +++ b/git-hooks/pre-commit @@ -2,7 +2,21 @@ set -euo pipefail -# Security: avoid option-injection from malicious file names (e.g. "--force"). +ROOT_DIR="$(git rev-parse --show-toplevel 2>/dev/null || pwd)" +RUN_NODE_TOOL="$ROOT_DIR/scripts/pre-commit/run-node-tool.sh" +FILTER_FILES="$ROOT_DIR/scripts/pre-commit/filter-staged-files.mjs" + +if [[ ! -x "$RUN_NODE_TOOL" ]]; then + echo "Missing helper: $RUN_NODE_TOOL" >&2 + exit 1 +fi + +if [[ ! -f "$FILTER_FILES" ]]; then + echo "Missing helper: $FILTER_FILES" >&2 + exit 1 +fi + +# Security: avoid option-injection from malicious file names (e.g. "--all", "--force"). # Robustness: NUL-delimited file list handles spaces/newlines safely. mapfile -d '' -t files < <(git diff --cached --name-only --diff-filter=ACMR -z) @@ -10,24 +24,15 @@ if [ "${#files[@]}" -eq 0 ]; then exit 0 fi -lint_files=() -format_files=() -for file in "${files[@]}"; do - case "$file" in - *.ts | *.tsx | *.js | *.jsx | *.mjs | *.cjs) lint_files+=("$file") ;; - esac - - case "$file" in - *.ts | *.tsx | *.js | *.jsx | *.mjs | *.cjs | *.json | *.md | *.mdx) format_files+=("$file") ;; - esac -done +mapfile -d '' -t lint_files < <(node "$FILTER_FILES" lint -- "${files[@]}") +mapfile -d '' -t format_files < <(node "$FILTER_FILES" format -- "${files[@]}") if [ "${#lint_files[@]}" -gt 0 ]; then - pnpm lint --fix -- "${lint_files[@]}" + "$RUN_NODE_TOOL" oxlint --type-aware --fix -- "${lint_files[@]}" fi if [ "${#format_files[@]}" -gt 0 ]; then - pnpm format -- "${format_files[@]}" + "$RUN_NODE_TOOL" oxfmt --write -- "${format_files[@]}" fi git add -- "${files[@]}" diff --git a/scripts/pre-commit/filter-staged-files.mjs b/scripts/pre-commit/filter-staged-files.mjs new file mode 100644 index 0000000000..7e3dcfd7ab --- /dev/null +++ b/scripts/pre-commit/filter-staged-files.mjs @@ -0,0 +1,39 @@ +#!/usr/bin/env node +import path from "node:path"; + +/** + * Prints selected files as NUL-delimited tokens to stdout. + * + * Usage: + * node scripts/pre-commit/filter-staged-files.mjs lint -- + * node scripts/pre-commit/filter-staged-files.mjs format -- + * + * Keep this dependency-free: the pre-commit hook runs in many environments. + */ + +const mode = process.argv[2]; +const rawArgs = process.argv.slice(3); +const files = rawArgs[0] === "--" ? rawArgs.slice(1) : rawArgs; + +if (mode !== "lint" && mode !== "format") { + process.stderr.write("usage: filter-staged-files.mjs -- \n"); + process.exit(2); +} + +const lintExts = new Set([".ts", ".tsx", ".js", ".jsx", ".mjs", ".cjs"]); +const formatExts = new Set([".ts", ".tsx", ".js", ".jsx", ".mjs", ".cjs", ".json", ".md", ".mdx"]); + +const shouldSelect = (filePath) => { + const ext = path.extname(filePath).toLowerCase(); + if (mode === "lint") { + return lintExts.has(ext); + } + return formatExts.has(ext); +}; + +for (const file of files) { + if (shouldSelect(file)) { + process.stdout.write(file); + process.stdout.write("\0"); + } +} diff --git a/test/git-hooks-pre-commit.integration.test.ts b/test/git-hooks-pre-commit.integration.test.ts new file mode 100644 index 0000000000..bed536f153 --- /dev/null +++ b/test/git-hooks-pre-commit.integration.test.ts @@ -0,0 +1,55 @@ +import { execFileSync } from "node:child_process"; +import { chmodSync, copyFileSync } from "node:fs"; +import { mkdir, mkdtemp, writeFile } from "node:fs/promises"; +import os from "node:os"; +import path from "node:path"; +import { describe, expect, it } from "vitest"; + +const run = (cwd: string, cmd: string, args: string[] = []) => { + return execFileSync(cmd, args, { cwd, encoding: "utf8" }).trim(); +}; + +describe("git-hooks/pre-commit (integration)", () => { + it("does not treat staged filenames as git-add flags (e.g. --all)", async () => { + const dir = await mkdtemp(path.join(os.tmpdir(), "openclaw-pre-commit-")); + run(dir, "git", ["init", "-q"]); + run(dir, "git", ["config", "user.email", "test@example.com"]); + run(dir, "git", ["config", "user.name", "Test"]); + + // Copy the hook + helpers so the test exercises real on-disk wiring. + await mkdir(path.join(dir, "git-hooks"), { recursive: true }); + await mkdir(path.join(dir, "scripts", "pre-commit"), { recursive: true }); + copyFileSync( + path.join(process.cwd(), "git-hooks", "pre-commit"), + path.join(dir, "git-hooks", "pre-commit"), + ); + copyFileSync( + path.join(process.cwd(), "scripts", "pre-commit", "run-node-tool.sh"), + path.join(dir, "scripts", "pre-commit", "run-node-tool.sh"), + ); + copyFileSync( + path.join(process.cwd(), "scripts", "pre-commit", "filter-staged-files.mjs"), + path.join(dir, "scripts", "pre-commit", "filter-staged-files.mjs"), + ); + chmodSync(path.join(dir, "git-hooks", "pre-commit"), 0o755); + chmodSync(path.join(dir, "scripts", "pre-commit", "run-node-tool.sh"), 0o755); + + await writeFile(path.join(dir, "tracked.txt"), "initial\n"); + run(dir, "git", ["add", "--", "tracked.txt"]); + run(dir, "git", ["commit", "-qm", "init"]); + + // Create changes that should NOT be staged by the hook. + await writeFile(path.join(dir, "secret.txt"), "do-not-stage\n"); // untracked, not ignored + await writeFile(path.join(dir, "tracked.txt"), "changed\n"); // tracked, but not staged + + // Stage a maliciously-named file. Older hooks using `xargs git add` could run `git add --all`. + await writeFile(path.join(dir, "--all"), "flag\n"); + run(dir, "git", ["add", "--", "--all"]); + + // Run the hook directly (same logic as when installed via core.hooksPath). + run(dir, "bash", ["git-hooks/pre-commit"]); + + const staged = run(dir, "git", ["diff", "--cached", "--name-only"]).split("\n").filter(Boolean); + expect(staged).toEqual(["--all"]); + }); +}); diff --git a/test/git-hooks-pre-commit.test.ts b/test/git-hooks-pre-commit.test.ts index 485eeee815..f3cb0f2b6b 100644 --- a/test/git-hooks-pre-commit.test.ts +++ b/test/git-hooks-pre-commit.test.ts @@ -16,8 +16,11 @@ describe("git-hooks/pre-commit", () => { // Option-injection hardening: always pass paths after "--". expect(script).toMatch(/\ngit add -- /); - // The original bug used whitespace + xargs, and passed unsafe flags. + // The original bug used whitespace + xargs. expect(script).not.toMatch(/xargs\s+git add/); - expect(script).not.toMatch(/--no-error-on-unmatched-pattern/); + + // Expected helper wiring for consistent tool invocation. + expect(script).toMatch(/scripts\/pre-commit\/run-node-tool\.sh/); + expect(script).toMatch(/scripts\/pre-commit\/filter-staged-files\.mjs/); }); }); diff --git a/vitest.config.ts b/vitest.config.ts index 0b1ba53b06..c3a042499e 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -33,12 +33,7 @@ export default defineConfig({ unstubGlobals: true, pool: "forks", maxWorkers: isCI ? ciWorkers : localWorkers, - include: [ - "src/**/*.test.ts", - "extensions/**/*.test.ts", - "test/format-error.test.ts", - "test/git-hooks-pre-commit.test.ts", - ], + include: ["src/**/*.test.ts", "extensions/**/*.test.ts", "test/**/*.test.ts"], setupFiles: ["test/setup.ts"], exclude: [ "dist/**",