Skip to content

fix(hooks): bound force-push patterns to avoid chained-command false positives - #794

Merged
keito4 merged 1 commit into
mainfrom
fix/dangerous-cmd-force-push-fp
Jun 4, 2026
Merged

fix(hooks): bound force-push patterns to avoid chained-command false positives#794
keito4 merged 1 commit into
mainfrom
fix/dangerous-cmd-force-push-fp

Conversation

@keito4

@keito4 keito4 commented Jun 4, 2026

Copy link
Copy Markdown
Owner

Why

block_dangerous_commands.py は normalize 時に " ".join(cmd.split()) で改行を空白へ潰す。その状態で force-push 検出の git\s+push\s+.*-f\b 系が .* 貪欲マッチするため、push の引数を越えて後続コマンドや flatten された heredoc 本文中の -f にまでマッチし、安全なコマンドを誤ブロックしていた。

実例(このセッションで遭遇):

git push -u origin my-branch 2>&1 | tail -4   # -4 や後続テキストの -f で誤検知
git push -u origin b && cat <<EOF ... use -f ... EOF

What

force-push 3 パターンの .*[^|&;<>]* に変更し、**pipe / redirect / separator をまたがない(push 自身の引数内のみ)**よう限定。

- (r"git\s+push\s+.*--force", ...)
- (r"git\s+push\s+.*-f\b", ...)
- (r"git\s+push\s+.*--force-with-lease", ...)
+ (r"git\s+push\s+[^|&;<>]*--force", ...)
+ (r"git\s+push\s+[^|&;<>]*-f\b", ...)
+ (r"git\s+push\s+[^|&;<>]*--force-with-lease", ...)

検証

behavioral テスト 8 ケースで確認:

  • block 継続: --force / 短縮形 / --force-with-lease / origin +branch
  • 誤検知解消: pipe 連結 / heredoc 本文に -f / echo … -f && git status / 通常の push -u

hooks-integrity.test.js のパターン文字列アサーション更新 + 退行防止テスト追加。jest 111 / 全 jest green。

Risk

低。検出を緩めるのではなく「push 引数内に限定」する変更で、実 force push の検出は維持。

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • Improved dangerous command detection accuracy by refining pattern matching for git push force operations, reducing false positives while maintaining robust security protections.

…positives

block_dangerous_commands は normalize 時に改行を空白へ潰すため、`.*` が
push の引数を越えて後続コマンドや flatten された heredoc 本文中の `-f`
までマッチし、`git push -u … 2>&1 | tail -4` 等を誤ブロックしていた。

- force-push 3 パターンの `.*` を `[^|&;<>]*` に変更し、pipe/redirect/
  separator をまたがない(push 自身の引数内のみ)よう限定
- 実 force push(--force / 短縮形 / --force-with-lease / origin +branch)は
  引き続きブロック。誤検知は解消(.context の behavioral テストで8ケース確認)
- hooks-integrity: パターン文字列アサーション更新 + 退行防止テスト追加

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jun 4, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 40e15d05-633a-41dc-959e-7cd91e3d748b

📥 Commits

Reviewing files that changed from the base of the PR and between 64d0ba3 and 579a6c3.

📒 Files selected for processing (2)
  • .claude/hooks/block_dangerous_commands.py
  • test/hooks-integrity.test.js

📝 Walkthrough

Walkthrough

The PR tightens regex patterns in a Git hook that detects dangerous git push force operations by replacing unbounded wildcards with bounded ones ([^|&;<>]*), preventing false positives across command separators in chained shell commands. Tests are updated to verify the bounded matching and exclude spurious matches.

Changes

Git push force-detection regex bounding

Layer / File(s) Summary
Bounded regex matching for git push force operations
.claude/hooks/block_dangerous_commands.py, test/hooks-integrity.test.js
Regex patterns for detecting --force, -f, and --force-with-lease are constrained with `[^

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Possibly related PRs

  • keito4/config#584: Introduces the force-push deny behavior that this PR's regex improvements refine for more accurate detection.

Suggested labels

bug

Poem

🐰 A hook catches force-pushes with tighter regex reins,
No more greedy wildcards spanning chained commands' lanes,
Bounded patterns dance 'tween pipes with careful grace,
Tests verify each bound—false positives erase!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: bounding force-push regex patterns to eliminate false positives in chained commands.
Description check ✅ Passed The description is well-structured and complete, providing clear context on the problem, the solution, verification approach, and risk assessment, though it uses Japanese in some sections.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/dangerous-cmd-force-push-fp

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

test/hooks-integrity.test.js

Oops! Something went wrong! :(

ESLint: 10.4.1

ReferenceError: describe is not defined
at Object. (/test/eslint-config.test.js:4:1)
at Module._compile (node:internal/modules/cjs/loader:1830:14)
at Object..js (node:internal/modules/cjs/loader:1961:10)
at Module.load (node:internal/modules/cjs/loader:1553:32)
at Module._load (node:internal/modules/cjs/loader:1355:12)
at wrapModuleLoad (node:internal/modules/cjs/loader:255:19)
at loadCJSModuleWithModuleLoad (node:internal/modules/esm/translators:326:3)
at ModuleWrap. (node:internal/modules/esm/translators:231:7)
at ModuleJob.run (node:internal/modules/esm/module_job:437:25)
at async node:internal/modules/esm/loader:639:26


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@claude

claude Bot commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

Claude finished @keito4's task in 1m 50s —— View job


コードレビュー

  • コンテキスト収集
  • 変更ファイルの確認
  • レビュー実施
  • フィードバック投稿

概要

.claude/hooks/block_dangerous_commands.py の force-push 検出パターンを .* から [^|&;<>]* へ変更し、パイプ・リダイレクト・コマンド区切りをまたいだ誤検知を防ぐ修正。方向性は正しく、実装も適切です。

Verdict: ✅ APPROVE(minor / recommendation のみ)


Security(セキュリティ)

問題なし。検出の穴ではなく「スコープの限定」であり、実際の force push は引き続きブロックされます。


品質(Quality)

✅ 正しい挙動の確認

[^|&;<>]* が意図通りに機能する主なケースを確認しました:

入力(正規化後) 旧パターン 新パターン 期待値
git push origin main -f ブロック ブロック
git push -u origin branch 2>&1 | tail -4 ❌誤検知 スルー
git push -u origin b && cat <<eof use -f eof ❌誤検知 スルー
git push --force-with-lease(引数なし) ブロック ブロック
git push "origin" main -f ブロック ブロック

Recommendation: git clean -f パターンも同様の懸念あり

# block_dangerous_commands.py:28(本 PR 対象外)
(r"git\s+clean\s+.*-f", "git clean -f (delete untracked files)"),

git cleangit push と同様に .* を使っています。例えば:

git clean -n && some-cmd --verbose -f

正規化後: git clean -n && some-cmd --verbose -f
上記は .*&& を越えてマッチするため、誤検知の可能性があります。本 PR のスコープ外ですが、後続 PR でまとめて対応すると一貫性が上がります。

Fix this →


テストカバレッジ

✅ パターン文字列アサーション(lines 108–120)

新旧パターンの文字列チェックと退行防止テスト(not.toContain で旧パターンが残らないことを保証)が追加されており、適切です。

Minor: テストはファイル内容の文字列検索のみ

hooks-integrity.test.js の各テストは content.toContain(...) でソースファイル内のパターン文字列の存在を確認するものです。実際に hook にコマンド文字列を渡して「誤検知しないこと」「正しくブロックすること」を動的に検証するテストがあると、より堅牢になります。

例(参考):

// ブロックされること
expect(runHook('git push origin main --force')).toEqual({ exitCode: 2 });
// 誤検知しないこと  
expect(runHook('git push -u origin b && cat -f file')).toEqual({ exitCode: 0 });

PR 説明にある「8 ケースのbehavioralテスト」が別途 BATS 等で実施済みであれば問題ありません。


AI Residuals

コメント(.claude/hooks/block_dangerous_commands.py 22–24行)は変更の WHY を的確に説明しており、適切です。


まとめ

観点 評価
Security ✅ 問題なし
Performance ✅ 影響なし
Quality ⚠️ git clean -f.* も将来的に修正推奨
Accessibility N/A
AI Residuals ✅ 問題なし

修正の方向性・実装ともに正しく、リスクは低いです。LGTM 🚀

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 579a6c3751

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +27 to +29
(r"git\s+push\s+[^|&;<>]*--force", "git push --force (force push)"),
(r"git\s+push\s+[^|&;<>]*-f\b", "git push -f (force push)"),
(r"git\s+push\s+[^|&;<>]*--force-with-lease", "git push --force-with-lease"),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Block force pushes with inline redirections

When a redirection appears before the force option in the same simple command, this negated class stops scanning at >/< and misses the later force flag. Bash still treats later words as arguments after a redirection (e.g. f 2>/dev/null --force origin main passes --force), and the hook now exits 0 for git push 2>/dev/null --force origin main, allowing the force-push policy to be bypassed while still executing a real force push.

Useful? React with 👍 / 👎.

@keito4 keito4 self-assigned this Jun 4, 2026
@keito4
keito4 merged commit 8c9d426 into main Jun 4, 2026
20 checks passed
@keito4
keito4 deleted the fix/dangerous-cmd-force-push-fp branch June 4, 2026 12:43
@github-actions

github-actions Bot commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

🎉 This PR is included in version 1.114.2 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

released リリース済み

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant