fix(hooks): bound remaining dangerous-command flag wildcards (FP class) - #795
Conversation
#794 では force-push 系のみ修正したが、同じ greedy `.*` が他7パターンにも 残っていた。normalize で改行が空白化されるため、連結コマンド/パイプ/ リダイレクト/heredoc 本文中の flag 風トークンに到達して誤検知しうる。 全フラグパターンの `.*` を `[^|&;<>]*`（区切りをまたがない）に統一: - git clean -f - rm -r…-f / -f…-r（直接形 rm -rf は不変） - docker run --privileged - kubectl delete pod --all / kubectl scale --replicas=0 - terraform apply -auto-approve - aws s3 rm --recursive 実コマンドの検出は維持（behavioral 15 ケースで block 8 / allow 7 を確認）。 test: hooks-integrity.test.js が file-length ハード上限(500行)に達したため、 command-safety 系2フック(block_dangerous_commands / block_inline_secrets)の describe を test/hooks-command-safety.test.js へ分離。退行防止アサーション （flag 系に greedy `\s+.*` が残らないこと）も追加。 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe PR hardens the dangerous-command hook by stripping quoted strings and tightening regex boundaries to prevent separator bypasses and false positives. A new comprehensive Jest test suite validates both dangerous-command and inline-secrets hooks, replacing inline tests previously scattered in the integrity test file. ChangesHook Command-Safety Hardening
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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
test/hooks-command-safety.test.jsOops! Something went wrong! :( ESLint: 10.4.1 ReferenceError: describe is not defined test/hooks-integrity.test.jsOops! Something went wrong! :( ESLint: 10.4.1 ReferenceError: describe is not defined 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. Comment |
|
Claude finished @keito4's task in 3m 34s —— View job PR レビュー: fix(hooks): bound remaining dangerous-command flag wildcards (FP class)
総評: ✅ 承認推奨正確かつリスク最小の修正。 Security ✅FP を修正する変更であり、実破壊コマンドの検出を緩める変更ではない。 Quality ✅
変更された7パターンはすべて一貫して
退行防止テストが適切に追加されている。特に以下のテスト設計が優れている： // hooks-command-safety.test.js:91-95
test('should bound all flag wildcards to a single command (no greedy \\s+.*)', () => {
expect(content).not.toContain('\\s+.*');
});これにより、将来
ファイル長 500 行制限対策として command-safety の describe ブロックを別ファイルへ分離。参照コメントも添えられており、テストの所在が明確。 Minor 観察（このPRが起因ではない既存挙動）
Performance ✅正規表現の変更は実行時コストに実質的な影響なし。 AI Residuals ✅モック残骸・ハードコード・スキップテスト・仮実装なし。 Verdict: APPROVE — 全7パターンの FP バグを根絶しており、検出精度向上・退行防止テスト追加・ファイル分割の3点が整合した良質な修正。 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ab2ea48ca1
ℹ️ 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".
| (r"docker\s+system\s+prune", "docker system prune"), | ||
| (r"docker\s+volume\s+prune", "docker volume prune (data loss)"), | ||
| (r"docker\s+run\s+.*--privileged", "docker run --privileged (host access)"), | ||
| (r"docker\s+run\s+[^|&;<>]*--privileged", "docker run --privileged (host access)"), |
There was a problem hiding this comment.
When an argument before the dangerous flag legitimately contains a quoted or escaped separator, this pattern stops scanning too early and allows the same command the hook is meant to block; for example, I checked docker run --name 'a;b' --privileged ubuntu and the hook exits 0, while the equivalent command without the semicolon exits 2. The same bypass applies to the other newly bounded [^|&;<>]* flag patterns, because the hook operates on the raw command string rather than parsed shell tokens.
Useful? React with 👍 / 👎.
|
(aside) |
|
Claude finished @keito4's task in 8m 5s —— View job CI失敗修正作業
調査結果と対応内容根本原因: ローカル作業ツリーの
対応: 確認結果:
|
|
(aside) |
|
Claude finished @keito4's task in 7m 2s —— View job CI失敗調査・修正作業
調査結果CIチェックは全て通過しています（失敗なし）。
CI ジョブログ（run 26953648009）でも確認: マージがブロックされている実際の原因{
"mergeStateStatus": "BLOCKED",
"mergeable": "MERGEABLE",
"reviewDecision": "REVIEW_REQUIRED"
}CIが落ちているのではなく、ブランチ保護ルールによりコードレビューの承認（Approve）が必要な状態です。
これはコード変更で解決できる問題ではなく、レビュアーが Pull Request を Approve する必要があります。 補足ローカルでも全テストが通過していることを確認済みです:
|
|
(aside) |
|
Claude finished @keito4's task in 13m 4s —— View job CI失敗調査・修正作業
対応内容根本原因（今回）
修正内容 Codex P1 指摘（quoted separator bypass）を解消するため、パターンマッチング前に引用符内コンテンツを除去する前処理を追加: # Strip single- and double-quoted string content so that shell separators
# inside argument values (e.g. --name 'a;b') are not treated as real command
# boundaries by [^|&;<>]* patterns.
normalized = re.sub(r"'[^']*'", " ", normalized)
normalized = re.sub(r'"[^"]*"', " ", normalized)バイパス修正の例
テスト結果 コミット: マージについての補足全CIチェックは通過しています。PR は |
…ex P1 bypass Quoted shell separators in argument values (e.g. --name 'a;b') were being treated as real command boundaries by [^|&;<>]* patterns, allowing commands like `docker run --name 'a;b' --privileged ubuntu` to bypass detection. Add re.sub pre-processing to strip single- and double-quoted string content from the normalised command before applying DANGEROUS_PATTERNS. Regression test added to hooks-command-safety.test.js to assert the guard is present. Refs: Codex P1 review on PR #795 Co-authored-by: keito4 <keito4@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude/hooks/block_dangerous_commands.py:
- Around line 21-26: The current quoted-content stripping (the two re.sub lines
that assign to normalized) removes entire quoted payloads and can hide dangerous
commands passed via flags like -c; instead, only neutralize shell metacharacters
inside quoted strings so the surrounding command and flags remain visible to
pattern matching. Update the logic in block_dangerous_commands.py that builds
normalized to find single- and double-quoted spans (the existing "'[^']*'" and
'"[^"]*"') and replace only dangerous separators (e.g., | & ; < > ` $ ( ) and
backticks/newlines as needed) within those quotes with spaces or another safe
placeholder while preserving the rest of the quoted text, so functions/patterns
that detect commands like bash -c still see the argument content for dangerous
tokens.
In `@test/hooks-command-safety.test.js`:
- Around line 98-103: The current test 'should strip quoted string content to
prevent quoted-separator bypass (Codex P1)' only inspects source text via the
content variable; change it to invoke the hook/command at runtime with crafted
payloads (e.g., "--name 'a;b'" and "--name \"$(bash -c 'echo vulnerable')\""/a
direct bash -c payload) using the existing test runner/helper that executes the
hook (reuse whatever spawn/runHook helper the suite uses) and assert process
exit codes and outputs: assert non-zero exit (or specific failure code) and that
stdout/stderr contains the expected detection/blocked message for both single-
and double-quoted cases, so regressions that bypass detection are caught. Ensure
the test still checks for the source-string replacements (content) but adds
these execution-level assertions for both quoted-separator and quoted-command
payloads.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f71d2e79-2bf3-463b-91ef-cbd7896014fa
📒 Files selected for processing (3)
.claude/hooks/block_dangerous_commands.pytest/hooks-command-safety.test.jstest/hooks-integrity.test.js
| # Strip single- and double-quoted string content so that shell separators | ||
| # inside argument values (e.g. --name 'a;b') are not treated as real command | ||
| # boundaries by [^|&;<>]* patterns. This prevents the quoted-separator bypass | ||
| # identified in Codex P1 review (PR #795). | ||
| normalized = re.sub(r"'[^']*'", " ", normalized) | ||
| normalized = re.sub(r'"[^"]*"', " ", normalized) |
There was a problem hiding this comment.
Quoted-content stripping introduces a dangerous-command bypass.
At Line 25-26, full quoted payloads are removed before matching. bash -c "rm -rf /" is normalized to essentially bash -c, so destructive commands inside -c are no longer detectable.
Suggested fix
-# Strip single- and double-quoted string content so that shell separators
-# inside argument values (e.g. --name 'a;b') are not treated as real command
-# boundaries by [^|&;<>]* patterns. This prevents the quoted-separator bypass
-# identified in Codex P1 review (PR `#795`).
-normalized = re.sub(r"'[^']*'", " ", normalized)
-normalized = re.sub(r'"[^"]*"', " ", normalized)
+# Neutralize separators inside quoted strings without removing quoted command text.
+def _neutralize_quoted_separators(text: str) -> str:
+ text = re.sub(
+ r"'([^']*)'",
+ lambda m: "'" + re.sub(r"[|&;<>]", " ", m.group(1)) + "'",
+ text,
+ )
+ return re.sub(
+ r'"([^"]*)"',
+ lambda m: '"' + re.sub(r"[|&;<>]", " ", m.group(1)) + '"',
+ text,
+ )
+
+normalized = _neutralize_quoted_separators(normalized)As per coding guidelines, **/*.{js,ts,jsx,tsx,py,java,go,rb,php,json,lock,txt,yaml,yml} must fail on critical security vulnerabilities during Security Code Analysis.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.claude/hooks/block_dangerous_commands.py around lines 21 - 26, The current
quoted-content stripping (the two re.sub lines that assign to normalized)
removes entire quoted payloads and can hide dangerous commands passed via flags
like -c; instead, only neutralize shell metacharacters inside quoted strings so
the surrounding command and flags remain visible to pattern matching. Update the
logic in block_dangerous_commands.py that builds normalized to find single- and
double-quoted spans (the existing "'[^']*'" and '"[^"]*"') and replace only
dangerous separators (e.g., | & ; < > ` $ ( ) and backticks/newlines as needed)
within those quotes with spaces or another safe placeholder while preserving the
rest of the quoted text, so functions/patterns that detect commands like bash -c
still see the argument content for dangerous tokens.
| test('should strip quoted string content to prevent quoted-separator bypass (Codex P1)', () => { | ||
| // Without this, --name 'a;b' would have ';' treated as a real boundary, | ||
| // allowing docker run --name 'a;b' --privileged ubuntu to bypass the check. | ||
| expect(content).toContain("re.sub(r\"'[^']*'\""); | ||
| expect(content).toContain('re.sub(r\'"[^"]*"\''); | ||
| }); |
There was a problem hiding this comment.
This test only validates source text, not hook behavior.
Line 98-103 passes even when bash -c "rm -rf /" bypasses detection. Add execution-level assertions for exit codes so quoted-payload regressions are caught.
Suggested behavioral regression test pattern
+const { spawnSync } = require('child_process');
+
+function runDangerousHook(command) {
+ const hook = path.join(hooksDir, 'block_dangerous_commands.py');
+ const input = JSON.stringify({ tool_input: { command } });
+ return spawnSync('python3', [hook], { input, encoding: 'utf8' });
+}
+
+test('blocks dangerous command inside quoted -c payload', () => {
+ const res = runDangerousHook('bash -c "rm -rf /tmp/demo"');
+ expect(res.status).toBe(2);
+});
+
+test('does not treat quoted separators as command boundaries', () => {
+ const res = runDangerousHook("docker run --name 'a;b' ubuntu:latest");
+ expect(res.status).toBe(0);
+});As per coding guidelines, **/*.{js,ts,jsx,tsx,py,java,go,rb,php} should follow TDD methodology with meaningful coverage of behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/hooks-command-safety.test.js` around lines 98 - 103, The current test
'should strip quoted string content to prevent quoted-separator bypass (Codex
P1)' only inspects source text via the content variable; change it to invoke the
hook/command at runtime with crafted payloads (e.g., "--name 'a;b'" and "--name
\"$(bash -c 'echo vulnerable')\""/a direct bash -c payload) using the existing
test runner/helper that executes the hook (reuse whatever spawn/runHook helper
the suite uses) and assert process exit codes and outputs: assert non-zero exit
(or specific failure code) and that stdout/stderr contains the expected
detection/blocked message for both single- and double-quoted cases, so
regressions that bypass detection are caught. Ensure the test still checks for
the source-string replacements (content) but adds these execution-level
assertions for both quoted-separator and quoted-command payloads.
|
🎉 This PR is included in version 1.115.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
1 participant
Why
#794 で force-push 系のみ
.*→[^|&;<>]*に限定したが、同じ greedy.*の誤検知バグが他の危険コマンドパターンにも残存していた。normalize()が改行を空白へ潰すため、.*は連結コマンド（a && b）・パイプ（| x）・リダイレクト（2>&1）・heredoc 本文中の flag 風トークンにまで到達して誤ブロックし得る。What
残り7パターンの
.*を[^|&;<>]*（区切りをまたがない）に統一し、FP クラスを根絶:git clean -frm -r…-f/-f…-rrm -rfは不変docker run --privilegedkubectl delete pod --all/kubectl scale --replicas=0terraform apply -auto-approveaws s3 rm --recursive検証
behavioral 15 ケース: 実コマンド8件は引き続き block / 連結・パイプ7件は allow。
\s+.*が残らないことをアサートtest/hooks-command-safety.test.jsに分離Risk
低。検出を緩めるのではなく「コマンド引数内に限定」する変更で、実破壊コマンドの検出は維持。
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests