fix: enhance prometheus-md-only hook to intercept bash file modifications (#2273)#2403
Closed
guazi04 wants to merge 1 commit intocode-yeongyu:devfrom
Closed
fix: enhance prometheus-md-only hook to intercept bash file modifications (#2273)#2403guazi04 wants to merge 1 commit intocode-yeongyu:devfrom
guazi04 wants to merge 1 commit intocode-yeongyu:devfrom
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 6 files
Confidence score: 2/5
- High-risk security issue in
src/hooks/prometheus-md-only/bash-command-policy.ts:hasCompoundOperatorsmisses single&, which can bypass command-policy checks and enable arbitrary background command execution. - Two additional security-related gaps increase regression risk: missing
-oblocking tests forsort/treeinsrc/hooks/prometheus-md-only/bash-command-policy.test.ts, and case-sensitive tool matching insrc/hooks/prometheus-md-only/constants.tsthat may missBash. - Because these findings are high severity (7–9/10) with strong confidence, this is not a low-risk merge until the policy logic and coverage are tightened.
- Pay close attention to
src/hooks/prometheus-md-only/bash-command-policy.ts,src/hooks/prometheus-md-only/bash-command-policy.test.ts,src/hooks/prometheus-md-only/constants.ts- close security bypass paths and add coverage for blocked flag/tool variants.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/hooks/prometheus-md-only/constants.ts">
<violation number="1" location="src/hooks/prometheus-md-only/constants.ts:14">
P1: Missing case variation for Bash tool name - only lowercase "bash" in BASH_TOOLS array, but matching is case-sensitive. Add "Bash" to match BLOCKED_TOOLS pattern.</violation>
</file>
<file name="src/hooks/prometheus-md-only/bash-command-policy.ts">
<violation number="1" location="src/hooks/prometheus-md-only/bash-command-policy.ts:123">
P0: Security bypass: Missing detection for single ampersand (`&`) background operator allows arbitrary command execution. The `hasCompoundOperators` function detects `;`, `|`, `||`, and `&&` but fails to detect `&`, enabling attacks like `cat file & rm -rf /` where the second command executes unconstrained.</violation>
</file>
<file name="src/hooks/prometheus-md-only/bash-command-policy.test.ts">
<violation number="1" location="src/hooks/prometheus-md-only/bash-command-policy.test.ts:60">
P1: Security Vulnerability: Missing tests for `-o` (output) flag blocking on `sort` and `tree` commands. Both commands support `-o FILE` to write output to arbitrary files, but the tests don't verify these flags are blocked. The `sort` and `tree` commands are in READ_ONLY_COMMANDS without flag validation, allowing commands like `sort data.txt -o src/index.ts` to bypass write restrictions. Add tests verifying `sort -o` and `tree -o` are properly blocked, similar to existing tests for find -exec and git diff --output.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| if (char === ";") return true | ||
| if (char === "|" && command[i + 1] === "|") return true | ||
| if (char === "|") return true | ||
| if (char === "&" && command[i + 1] === "&") return true |
There was a problem hiding this comment.
P0: Security bypass: Missing detection for single ampersand (&) background operator allows arbitrary command execution. The hasCompoundOperators function detects ;, |, ||, and && but fails to detect &, enabling attacks like cat file & rm -rf / where the second command executes unconstrained.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/hooks/prometheus-md-only/bash-command-policy.ts, line 123:
<comment>Security bypass: Missing detection for single ampersand (`&`) background operator allows arbitrary command execution. The `hasCompoundOperators` function detects `;`, `|`, `||`, and `&&` but fails to detect `&`, enabling attacks like `cat file & rm -rf /` where the second command executes unconstrained.</comment>
<file context>
@@ -0,0 +1,405 @@
+ if (char === ";") return true
+ if (char === "|" && command[i + 1] === "|") return true
+ if (char === "|") return true
+ if (char === "&" && command[i + 1] === "&") return true
+ }
+
</file context>
Author
There was a problem hiding this comment.
Good catch. Fixed — single & (background operator) is now detected in hasCompoundOperators(). The check correctly handles && first, then detects lone & (excluding &> redirects). Added tests for cat file & rm -rf / (blocked) and quoted & (allowed).
guazi04
force-pushed
the
fix/prometheus-bash-file-guard
branch
from
91d43e2 to
17dbc2a
Compare
Collaborator
|
@acamq I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
8 issues found across 6 files
Confidence score: 1/5
- Multiple high-confidence security bypasses in
src/hooks/prometheus-md-only/bash-command-policy.ts(quoted-flag matching gaps, subshell quote-state bug, redirection parsing gaps, and unchecked output-file argument forms) indicate likely policy evasion and arbitrary file overwrite risk. src/hooks/prometheus-md-only/bash-command-policy.test.tsreports a concrete command-execution vector (rg --pre) plus missing tokenizer bypass coverage, so current tests do not reliably guard against these regressions.src/hooks/prometheus-md-only/hook.tsmay skip validation whenoutput.args.commandis absent, creating an additional path for unvalidated command input depending on parameter shape.- Pay close attention to
src/hooks/prometheus-md-only/bash-command-policy.ts,src/hooks/prometheus-md-only/bash-command-policy.test.ts, andsrc/hooks/prometheus-md-only/hook.ts- security-critical validation bypasses and missing/insufficient protections are concentrated here.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/hooks/prometheus-md-only/hook.ts">
<violation number="1" location="src/hooks/prometheus-md-only/hook.ts:53">
P1: Potential bash command policy bypass via alternative parameter names. When `output.args.command` is falsy/undefined, the hook returns early without validation. If the bash tool supports alternative parameter names (script, cmd, code, commands array), malicious commands could bypass analyzeBashCommand completely. Should reject bash calls lacking valid command argument with an error instead of silent return.</violation>
</file>
<file name="src/hooks/prometheus-md-only/bash-command-policy.ts">
<violation number="1" location="src/hooks/prometheus-md-only/bash-command-policy.ts:13">
P0: Arbitrary file overwrites via xxd/uniq positional output arguments and sort --output flag bypass. Commands xxd, uniq accept output files as second positional arguments, and sort accepts --output=FILE (not checked, only -o is validated). These allowlisted commands can overwrite any file on the system, bypassing the .sisyphus/**/*.md restriction.</violation>
<violation number="2" location="src/hooks/prometheus-md-only/bash-command-policy.ts:131">
P0: Missing double quote tracking in `hasSubshells()` function creates a security bypass vulnerability. When a single quote appears inside double quotes, it should be treated as a literal character (not a quote delimiter). The function should track `inDoubleQuote` state like `tokenize()` and `hasCompoundOperators()` already do.</violation>
<violation number="3" location="src/hooks/prometheus-md-only/bash-command-policy.ts:186">
P0: Security bypass in redirection validation: The tokenization logic fails to detect redirects without preceding spaces (e.g., `echo "data">/etc/passwd`) and arbitrary file descriptors (e.g., `cat 3>/etc/shadow`). The tokenizer only splits on whitespace, so `data>/etc/passwd` becomes a single token that doesn't start with known operators. The redirectOps set also doesn't include file descriptors beyond 1 and 2.</violation>
<violation number="4" location="src/hooks/prometheus-md-only/bash-command-policy.ts:292">
P0: Dangerous flags can bypass blocklists via quoted arguments due to quote preservation in tokenizer but exact string matching in validators. The tokenizer preserves quotes (e.g., `"-exec"` stays as `"-exec"`), but validators check against unquoted flag names (e.g., `-exec`). Since bash removes outer quotes before passing arguments to commands, an attacker can use `find . "-exec" touch pwned.txt ";"` to bypass the `-exec` blocklist check.</violation>
</file>
<file name="src/hooks/prometheus-md-only/bash-command-policy.test.ts">
<violation number="1" location="src/hooks/prometheus-md-only/bash-command-policy.test.ts:45">
P0: `rg` (ripgrep) is allowed but permits arbitrary command execution via the `--pre` flag. The implementation lacks validation to block this dangerous flag, which can execute arbitrary commands on each file searched (CVE-2021-3013).</violation>
<violation number="2" location="src/hooks/prometheus-md-only/bash-command-policy.test.ts:145">
P2: `git remote` and `git config` validations may permit mutating or interactive flags if combined with safe ones. The tests should verify that dangerous combinations like `git remote update -v` (mutating fetch) and `git config --get --edit` (interactive editor) are properly blocked. Add test cases to ensure these edge cases are rejected by the validation.</violation>
<violation number="3" location="src/hooks/prometheus-md-only/bash-command-policy.test.ts:454">
P1: Test file lacks coverage for intra-token quoting/escaping bypass attempts in bash tokenizer security validation</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| const READ_ONLY_COMMANDS = new Set([ | ||
| "cat", "grep", "rg", "find", "ls", "tree", "wc", "head", "tail", "less", | ||
| "file", "stat", "du", "df", "pwd", "which", "realpath", "dirname", "basename", | ||
| "diff", "sort", "uniq", "tr", "cut", "jq", "xxd", |
There was a problem hiding this comment.
P0: Arbitrary file overwrites via xxd/uniq positional output arguments and sort --output flag bypass. Commands xxd, uniq accept output files as second positional arguments, and sort accepts --output=FILE (not checked, only -o is validated). These allowlisted commands can overwrite any file on the system, bypassing the .sisyphus/**/*.md restriction.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/hooks/prometheus-md-only/bash-command-policy.ts, line 13:
<comment>Arbitrary file overwrites via xxd/uniq positional output arguments and sort --output flag bypass. Commands xxd, uniq accept output files as second positional arguments, and sort accepts --output=FILE (not checked, only -o is validated). These allowlisted commands can overwrite any file on the system, bypassing the .sisyphus/**/*.md restriction.</comment>
<file context>
@@ -0,0 +1,424 @@
+const READ_ONLY_COMMANDS = new Set([
+ "cat", "grep", "rg", "find", "ls", "tree", "wc", "head", "tail", "less",
+ "file", "stat", "du", "df", "pwd", "which", "realpath", "dirname", "basename",
+ "diff", "sort", "uniq", "tr", "cut", "jq", "xxd",
+ "hexdump", "strings",
+])
</file context>
| return { allowed: true, reason: "allowed git read-only subcommand" } | ||
| } | ||
|
|
||
| function validateFindCommand(tokens: string[]): BashCommandResult { |
There was a problem hiding this comment.
P0: Dangerous flags can bypass blocklists via quoted arguments due to quote preservation in tokenizer but exact string matching in validators. The tokenizer preserves quotes (e.g., "-exec" stays as "-exec"), but validators check against unquoted flag names (e.g., -exec). Since bash removes outer quotes before passing arguments to commands, an attacker can use find . "-exec" touch pwned.txt ";" to bypass the -exec blocklist check.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/hooks/prometheus-md-only/bash-command-policy.ts, line 292:
<comment>Dangerous flags can bypass blocklists via quoted arguments due to quote preservation in tokenizer but exact string matching in validators. The tokenizer preserves quotes (e.g., `"-exec"` stays as `"-exec"`), but validators check against unquoted flag names (e.g., `-exec`). Since bash removes outer quotes before passing arguments to commands, an attacker can use `find . "-exec" touch pwned.txt ";"` to bypass the `-exec` blocklist check.</comment>
<file context>
@@ -0,0 +1,424 @@
+ return { allowed: true, reason: "allowed git read-only subcommand" }
+}
+
+function validateFindCommand(tokens: string[]): BashCommandResult {
+ for (const token of tokens) {
+ if (DANGEROUS_FIND_FLAGS.has(token)) {
</file context>
| return false | ||
| } | ||
|
|
||
| function hasSubshells(command: string): boolean { |
There was a problem hiding this comment.
P0: Missing double quote tracking in hasSubshells() function creates a security bypass vulnerability. When a single quote appears inside double quotes, it should be treated as a literal character (not a quote delimiter). The function should track inDoubleQuote state like tokenize() and hasCompoundOperators() already do.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/hooks/prometheus-md-only/bash-command-policy.ts, line 131:
<comment>Missing double quote tracking in `hasSubshells()` function creates a security bypass vulnerability. When a single quote appears inside double quotes, it should be treated as a literal character (not a quote delimiter). The function should track `inDoubleQuote` state like `tokenize()` and `hasCompoundOperators()` already do.</comment>
<file context>
@@ -0,0 +1,424 @@
+ return false
+}
+
+function hasSubshells(command: string): boolean {
+ let inSingleQuote = false
+ let escaped = false
</file context>
| */ | ||
| function extractRedirectTargets(tokens: string[]): string[] { | ||
| const targets: string[] = [] | ||
| const redirectOps = new Set([">", ">>", "1>", "2>", "&>", ">|"]) |
There was a problem hiding this comment.
P0: Security bypass in redirection validation: The tokenization logic fails to detect redirects without preceding spaces (e.g., echo "data">/etc/passwd) and arbitrary file descriptors (e.g., cat 3>/etc/shadow). The tokenizer only splits on whitespace, so data>/etc/passwd becomes a single token that doesn't start with known operators. The redirectOps set also doesn't include file descriptors beyond 1 and 2.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/hooks/prometheus-md-only/bash-command-policy.ts, line 186:
<comment>Security bypass in redirection validation: The tokenization logic fails to detect redirects without preceding spaces (e.g., `echo "data">/etc/passwd`) and arbitrary file descriptors (e.g., `cat 3>/etc/shadow`). The tokenizer only splits on whitespace, so `data>/etc/passwd` becomes a single token that doesn't start with known operators. The redirectOps set also doesn't include file descriptors beyond 1 and 2.</comment>
<file context>
@@ -0,0 +1,424 @@
+ */
+function extractRedirectTargets(tokens: string[]): string[] {
+ const targets: string[] = []
+ const redirectOps = new Set([">", ">>", "1>", "2>", "&>", ">|"])
+
+ for (let i = 0; i < tokens.length; i++) {
</file context>
| expect(result.allowed).toBe(true) | ||
| }) | ||
|
|
||
| test("#then rg should be allowed", () => { |
There was a problem hiding this comment.
P0: rg (ripgrep) is allowed but permits arbitrary command execution via the --pre flag. The implementation lacks validation to block this dangerous flag, which can execute arbitrary commands on each file searched (CVE-2021-3013).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/hooks/prometheus-md-only/bash-command-policy.test.ts, line 45:
<comment>`rg` (ripgrep) is allowed but permits arbitrary command execution via the `--pre` flag. The implementation lacks validation to block this dangerous flag, which can execute arbitrary commands on each file searched (CVE-2021-3013).</comment>
<file context>
@@ -0,0 +1,738 @@
+ expect(result.allowed).toBe(true)
+ })
+
+ test("#then rg should be allowed", () => {
+ const result = analyzeBashCommand("rg 'pattern' src/", WORKSPACE)
+ expect(result.allowed).toBe(true)
</file context>
| } | ||
|
|
||
| if (BASH_TOOLS.includes(toolName)) { | ||
| const command = output.args.command as string | undefined |
There was a problem hiding this comment.
P1: Potential bash command policy bypass via alternative parameter names. When output.args.command is falsy/undefined, the hook returns early without validation. If the bash tool supports alternative parameter names (script, cmd, code, commands array), malicious commands could bypass analyzeBashCommand completely. Should reject bash calls lacking valid command argument with an error instead of silent return.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/hooks/prometheus-md-only/hook.ts, line 53:
<comment>Potential bash command policy bypass via alternative parameter names. When `output.args.command` is falsy/undefined, the hook returns early without validation. If the bash tool supports alternative parameter names (script, cmd, code, commands array), malicious commands could bypass analyzeBashCommand completely. Should reject bash calls lacking valid command argument with an error instead of silent return.</comment>
<file context>
@@ -37,6 +38,36 @@ export function createPrometheusMdOnlyHook(ctx: PluginInput) {
+ }
+
+ if (BASH_TOOLS.includes(toolName)) {
+ const command = output.args.command as string | undefined
+ if (!command) {
+ return
</file context>
|
|
||
| describe("#given dangerous flags on safe commands", () => { | ||
| describe("#when find has exec flags", () => { | ||
| test("#then find -exec should be blocked", () => { |
There was a problem hiding this comment.
P1: Test file lacks coverage for intra-token quoting/escaping bypass attempts in bash tokenizer security validation
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/hooks/prometheus-md-only/bash-command-policy.test.ts, line 454:
<comment>Test file lacks coverage for intra-token quoting/escaping bypass attempts in bash tokenizer security validation</comment>
<file context>
@@ -0,0 +1,738 @@
+
+ describe("#given dangerous flags on safe commands", () => {
+ describe("#when find has exec flags", () => {
+ test("#then find -exec should be blocked", () => {
+ const result = analyzeBashCommand("find . -exec rm {} \\;", WORKSPACE)
+ expect(result.allowed).toBe(false)
</file context>
| expect(result.allowed).toBe(true) | ||
| }) | ||
|
|
||
| test("#then git config --get should be allowed", () => { |
There was a problem hiding this comment.
P2: git remote and git config validations may permit mutating or interactive flags if combined with safe ones. The tests should verify that dangerous combinations like git remote update -v (mutating fetch) and git config --get --edit (interactive editor) are properly blocked. Add test cases to ensure these edge cases are rejected by the validation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/hooks/prometheus-md-only/bash-command-policy.test.ts, line 145:
<comment>`git remote` and `git config` validations may permit mutating or interactive flags if combined with safe ones. The tests should verify that dangerous combinations like `git remote update -v` (mutating fetch) and `git config --get --edit` (interactive editor) are properly blocked. Add test cases to ensure these edge cases are rejected by the validation.</comment>
<file context>
@@ -0,0 +1,738 @@
+ expect(result.allowed).toBe(true)
+ })
+
+ test("#then git config --get should be allowed", () => {
+ const result = analyzeBashCommand("git config --get user.name", WORKSPACE)
+ expect(result.allowed).toBe(true)
</file context>
This was referenced Mar 10, 2026
Author
|
Closing in favor of #2414 (deny bash entirely for Prometheus). The tokenizer-based approach has too many security bypass vectors to be practical. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #2273 — Prometheus agent can bypass file write restrictions via bash/interactive_bash tools.
Instead of denying bash access entirely (as attempted in #2298), this PR enhances the existing
prometheus-md-onlyhook to intercept bash commands that modify non-.mdfiles, following a default-deny strategy.Approach
The hook now inspects
Bashandinteractive_bashtool inputs using a self-contained command analyzer:cat,ls,grep,findwithout exec flags,gitwith read-only subcommands, etc.)>,>>) are only allowed to.sisyphus/**/*.mdpaths (reuses existingisAllowedFile())&&,||,;,|are blocked (each could contain mutations)$(...), backticks, and process substitution<(,>(are blocked (even inside double quotes — only single-quoted literals are safe)Key design decisions
awk,sed,yqare excluded from the allowlist because they can write files internally without shell redirections (e.g.,awk 'BEGIN{print 1 > "file"}',sed 'w file')log,diff,show,blame,status,ls-files, etc.). Mutating subcommands likebranch,tag,remote(without-v) are blocked.--outputflag detected in both--output=fileand--output fileformsFiles changed
bash-command-policy.tsbash-command-policy.test.tshook.tsconstants.tsBASH_TOOLSandBLOCKED_BASH_TOOLSconstantsindex.test.tsindex.tsTest coverage
156 tests total, 0 failures. Tests include:
find -exec,find -fprintf, etc.)echo "$(rm file)", backticks in double quotes)<(...),>(...))Summary by cubic
Intercepts
bash/Bashin theprometheus-md-onlyhook to block file mutations with a strict default-deny policy; only safe reads are allowed, and writes are limited to.sisyphus/**/*.md. Also blocksinteractive_bash. Fixes #2273.bashcommands.cat,ls,grep,findwithout exec; read-onlygit).>/>>only to.sisyphus/**/*.md(handles quoted paths).;,&&,||,|,$(), backticks,<( ),>( )).git: read-only subcommands only; rejects--output;remoterequires-v/--verbose;configlimited to--get/--list; blocks mutatingtagflags.env,sudo,bash -c,xargs,sh/zsh).interactive_bash.Written for commit 17dbc2a. Summary will update on new commits.