From ba84b12535b7d272cb4793109afc56dd332b32a0 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Mon, 16 Feb 2026 03:15:31 +0100 Subject: [PATCH] fix: harden pre-commit hook against option injection --- CHANGELOG.md | 1 + git-hooks/pre-commit | 38 +++++++++++++++++++++++++------ test/git-hooks-pre-commit.test.ts | 23 +++++++++++++++++++ vitest.config.ts | 7 +++++- 4 files changed, 61 insertions(+), 8 deletions(-) create mode 100644 test/git-hooks-pre-commit.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index f2f431d31d3..ead5000b56b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,7 @@ Docs: https://docs.openclaw.ai - Control UI: prevent stored XSS via assistant name/avatar by removing inline script injection, serving bootstrap config as JSON, and enforcing `script-src 'self'`. Thanks @Adam55A-code. - Web UI/Agents: hide `BOOTSTRAP.md` in the Agents Files list after onboarding is completed, avoiding confusing missing-file warnings for completed workspaces. (#17491) Thanks @gumadeiras. - Telegram: omit `message_thread_id` for DM sends/draft previews and keep forum-topic handling (`id=1` general omitted, non-general kept), preventing DM failures with `400 Bad Request: message thread not found`. (#10942) Thanks @garnetlyx. +- Dev tooling: harden git `pre-commit` hook against option injection from malicious filenames (for example `--force`), preventing accidental staging of ignored files. Thanks @mrthankyou. - Subagents/Models: preserve `agents.defaults.model.fallbacks` when subagent sessions carry a model override, so subagent runs fail over to configured fallback models instead of retrying only the overridden primary model. - Config/Gateway: make sensitive-key whitelist suffix matching case-insensitive while preserving `passwordFile` path exemptions, preventing accidental redaction of non-secret config values like `maxTokens` and IRC password-file paths. (#16042) Thanks @akramcodez. - Group chats: always inject group chat context (name, participants, reply guidance) into the system prompt on every turn, not just the first. Prevents the model from losing awareness of which group it's in and incorrectly using the message tool to send to the same group. (#14447) Thanks @tyler6204. diff --git a/git-hooks/pre-commit b/git-hooks/pre-commit index b58a53100d4..1b4475b6fb1 100755 --- a/git-hooks/pre-commit +++ b/git-hooks/pre-commit @@ -1,9 +1,33 @@ -#!/bin/sh -FILES=$(git diff --cached --name-only --diff-filter=ACMR | sed 's| |\\ |g') -[ -z "$FILES" ] && exit 0 +#!/usr/bin/env bash -echo "$FILES" | xargs pnpm lint --fix -echo "$FILES" | xargs pnpm format --no-error-on-unmatched-pattern -echo "$FILES" | xargs git add +set -euo pipefail -exit 0 +# Security: avoid option-injection from malicious file names (e.g. "--force"). +# Robustness: NUL-delimited file list handles spaces/newlines safely. +mapfile -d '' -t files < <(git diff --cached --name-only --diff-filter=ACMR -z) + +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 + +if [ "${#lint_files[@]}" -gt 0 ]; then + pnpm lint --fix -- "${lint_files[@]}" +fi + +if [ "${#format_files[@]}" -gt 0 ]; then + pnpm format -- "${format_files[@]}" +fi + +git add -- "${files[@]}" diff --git a/test/git-hooks-pre-commit.test.ts b/test/git-hooks-pre-commit.test.ts new file mode 100644 index 00000000000..485eeee8156 --- /dev/null +++ b/test/git-hooks-pre-commit.test.ts @@ -0,0 +1,23 @@ +import { readFileSync } from "node:fs"; +import path from "node:path"; +import { describe, expect, it } from "vitest"; + +describe("git-hooks/pre-commit", () => { + it("avoids option injection and unsafe whitespace parsing", () => { + const scriptPath = path.join(process.cwd(), "git-hooks", "pre-commit"); + const script = readFileSync(scriptPath, "utf8"); + + // NUL-delimited list: supports spaces/newlines in filenames. + expect(script).toMatch(/--name-only/); + expect(script).toMatch(/--diff-filter=ACMR/); + expect(script).toMatch(/\s-z\b/); + expect(script).toMatch(/mapfile -d '' -t files/); + + // Option-injection hardening: always pass paths after "--". + expect(script).toMatch(/\ngit add -- /); + + // The original bug used whitespace + xargs, and passed unsafe flags. + expect(script).not.toMatch(/xargs\s+git add/); + expect(script).not.toMatch(/--no-error-on-unmatched-pattern/); + }); +}); diff --git a/vitest.config.ts b/vitest.config.ts index 71a92990525..0b1ba53b06e 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -33,7 +33,12 @@ export default defineConfig({ unstubGlobals: true, pool: "forks", maxWorkers: isCI ? ciWorkers : localWorkers, - include: ["src/**/*.test.ts", "extensions/**/*.test.ts", "test/format-error.test.ts"], + include: [ + "src/**/*.test.ts", + "extensions/**/*.test.ts", + "test/format-error.test.ts", + "test/git-hooks-pre-commit.test.ts", + ], setupFiles: ["test/setup.ts"], exclude: [ "dist/**",