mirror of
https://github.com/openclaw/openclaw.git
synced 2026-02-19 18:39:20 -05:00
refactor: centralize pre-commit file filtering
This commit is contained in:
@@ -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[@]}"
|
||||
|
||||
39
scripts/pre-commit/filter-staged-files.mjs
Normal file
39
scripts/pre-commit/filter-staged-files.mjs
Normal file
@@ -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 -- <files...>
|
||||
* node scripts/pre-commit/filter-staged-files.mjs format -- <files...>
|
||||
*
|
||||
* 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 <lint|format> -- <files...>\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");
|
||||
}
|
||||
}
|
||||
55
test/git-hooks-pre-commit.integration.test.ts
Normal file
55
test/git-hooks-pre-commit.integration.test.ts
Normal file
@@ -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"]);
|
||||
});
|
||||
});
|
||||
@@ -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/);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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/**",
|
||||
|
||||
Reference in New Issue
Block a user