fix(coding-agent): allow quoted metacharacters in bash patterns - #7553
Conversation
- Replaced the raw character guard with quote-aware scanning while retaining command substitution and unquoted shell-control protections. - Added regression coverage for the reported Cargo benchmark filter. Fixes #7552
![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: 54b2e38564
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (quote === "'") { | ||
| if (ch === "'") quote = undefined; | ||
| continue; |
There was a problem hiding this comment.
When a user has a common allow rule such as bash.patterns: [{ match: "git *", approval: "allow" }], this branch treats all metacharacters inside single quotes as harmless, so git -c alias.x='!touch /tmp/pwn; printf ok' x now matches the allow rule and runs without approval. I confirmed the Git behavior with git -c alias.omp-review-test='!printf first; printf second' ...; ! aliases run through a shell, so the semicolon becomes executable compound syntax rather than literal argument text. The raw guard in the parent blocked this case, and it breaks the repo-documented allow contract that patterns cannot approve compound commands (docs/bash-tool-runtime.md:36).
Useful? React with 👍 / 👎.
- Kept quoted shell controls prompt-gated when code-evaluation options can reinterpret the argument. - Covered the reported git inline shell-alias bypass while retaining Cargo regex approval. Fixes #7552
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9bcd31fcd2
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| } | ||
| // Expansion remains active inside double quotes; other control-looking | ||
| // characters are literal argument text. | ||
| if (ch === "`" || ch === "$") return true; |
There was a problem hiding this comment.
When the reinterpreted payload is double-quoted rather than single-quoted, this branch never sets hasQuotedShellControl, so the -c/-e guard at the end is skipped. With a common allow rule like bash.patterns: [{ match: "git *", approval: "allow" }], git -c alias.x="!touch /tmp/pwn; printf ok" x now matches as a simple allowed command even though Git runs ! aliases through a shell; I verified this exact double-quoted form executes the semicolon-separated payload. This is the remaining gap after the single-quoted alias case was covered.
Useful? React with 👍 / 👎.
- Treated double-quoted shell-control chars like single-quoted ones so a -c/-e reinterpretation option still gates them. - Covered the double-quoted git inline shell-alias bypass. Fixes #7552
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b621a9a637
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (ch === "\\") { | ||
| i++; | ||
| continue; |
There was a problem hiding this comment.
When a user has a common allow rule like bash.patterns: [{ match: "git *", approval: "allow" }], this backslash skip treats escaped metacharacters as literal and leaves hasQuotedShellControl false. The outer shell removes those escapes before Git sees the value; I checked git --help, which documents -c <name>=<value>, and verified git -c alias.x=!touch\ /tmp/pwn\;\ printf\ ok x runs the semicolon-separated ! alias through a shell. Fresh evidence relative to the quoted-alias threads: the payload uses backslash escapes rather than quotes, so this new scanner now approves shell control that the parent raw guard rejected.
Useful? React with 👍 / 👎.
- Preserved escaped control characters for the downstream reinterpretation safety decision. - Covered the backslash-escaped git inline shell-alias bypass. Fixes #7552
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 98a65ffbdf
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| "(": true, | ||
| ")": true, | ||
| }; | ||
| const BASH_APPROVAL_REINTERPRETED_ARGUMENT_RE = /(?:^|[ \t])(?:-[^-]*[ce]|--(?:command|eval))(?:[= \t]|$)/u; |
There was a problem hiding this comment.
When users configure an allow rule such as bash.patterns: [{ match: "python3 *", approval: "allow" }], this regex only treats -c/-e as reinterpreting code when the option is followed by whitespace, =, or end-of-line. Several interpreters accept the payload attached to the same argv word (I checked python3 -c'...', ruby -e'...', and perl -e'...' execute), so a command like python3 -c'import os; os.system("rm -rf build")' records the quoted semicolon as reinterpretable but then returns false here and is auto-allowed, whereas the previous raw shell-control guard blocked it. Please treat attached short-option payloads as reinterpreted too, or parse argv before applying the allow rule.
Useful? React with 👍 / 👎.
… allowlist matching command_allowlist glob rules (e.g. 'cargo *') rejected any command whose quoted arguments contained shell metacharacters — a cargo benchmark regex filter like '^layer3/write/(a|b)$' disqualified the whole command even though those characters are literal to the shell. _has_allowlist_shell_operator is now quote-aware: - metacharacters inside single/double quotes or behind a backslash are treated as literal arguments; - $ and backtick inside DOUBLE quotes still disqualify (expansion is active there); - quoted/escaped control characters still disqualify when the command carries a -c/-e/--command/--eval-style option that hands the payload to another interpreter (sh -c '...', git -c alias.x='!...' x); - unterminated quotes disqualify (shape can't be reasoned about). Compound commands (unquoted ; & | < > backtick $( newline) are rejected exactly as before. hermes_cli/approvals_suggest.derive_glob picks up the same semantics via its existing import.
… allowlist matching command_allowlist glob rules (e.g. 'cargo *') rejected any command whose quoted arguments contained shell metacharacters — a cargo benchmark regex filter like '^layer3/write/(a|b)$' disqualified the whole command even though those characters are literal to the shell. _has_allowlist_shell_operator is now quote-aware: - metacharacters inside single/double quotes or behind a backslash are treated as literal arguments; - $ and backtick inside DOUBLE quotes still disqualify (expansion is active there); - quoted/escaped control characters still disqualify when the command carries a -c/-e/--command/--eval-style option that hands the payload to another interpreter (sh -c '...', git -c alias.x='!...' x); - unterminated quotes disqualify (shape can't be reasoned about). Compound commands (unquoted ; & | < > backtick $( newline) are rejected exactly as before. hermes_cli/approvals_suggest.derive_glob picks up the same semantics via its existing import.
…ite (#8) * fix(config): parse list/mapping literals in hermes config set Fold the list/mapping parser INSIDE the existing string-typed-value coercion guard (the `not isinstance(_default_value_for_key(key), str)` block from e4ea0a0) instead of running it unconditionally, so a genuinely string-typed setting whose value merely starts with '[' or '{' is left untouched while non-string keys get JSON/YAML flow literals parsed to real lists/dicts. Update website/docs/user-guide/configuring-models.md: the `config set only writes scalar values` note is no longer accurate; document the list/mapping support with a quoted example. Fixes NousResearch#40545 NousResearch#50168 * fix(config): extend structured-value parsing to multi-line YAML blocks with a conservative trigger Consolidation follow-up on top of NousResearch#59182's cherry-picked base: - Add _looks_structured_value(): triggers a yaml.safe_load structured parse only when the value starts with '[' / '{' or spans multiple lines with YAML list-item ('- x') or mapping-entry ('key: v') shaped lines. Deliberately avoids the over-broad leading '-' trigger from NousResearch#88066 so '-5' and '--flag' stay strings. - Stays folded INSIDE the string-typed-key guard: keys whose DEFAULT_CONFIG type is str (e.g. approvals.mode) are never coerced. - Tests: multi-line YAML list/dict, string-typed key given '[x]' and '-5' stays string, dash-prefixed scalars stay strings, plain multi-line prose stays a string, load_config round-trip. Sabotage-verified: 7 of the suite's tests fail on main without the fix. * Port from can1357/oh-my-pi#7553: allow quoted shell metacharacters in allowlist matching command_allowlist glob rules (e.g. 'cargo *') rejected any command whose quoted arguments contained shell metacharacters — a cargo benchmark regex filter like '^layer3/write/(a|b)$' disqualified the whole command even though those characters are literal to the shell. _has_allowlist_shell_operator is now quote-aware: - metacharacters inside single/double quotes or behind a backslash are treated as literal arguments; - $ and backtick inside DOUBLE quotes still disqualify (expansion is active there); - quoted/escaped control characters still disqualify when the command carries a -c/-e/--command/--eval-style option that hands the payload to another interpreter (sh -c '...', git -c alias.x='!...' x); - unterminated quotes disqualify (shape can't be reasoned about). Compound commands (unquoted ; & | < > backtick $( newline) are rejected exactly as before. hermes_cli/approvals_suggest.derive_glob picks up the same semantics via its existing import. * feat(terminal): interpret signal-termination exit codes for the model Port from Kilo-Org/kilocode#12698: report signal-terminated commands with a human-readable note instead of a bare numeric exit code. Kilo's fix settles a signal-killed process as the conventional 128+signum exit code so its bash tool stops hanging. Hermes already produces numeric codes for signal deaths (subprocess -signum, or the shell's 128+signum), but the model saw a bare exit_code=-9 or 137 and burned turns mis-diagnosing (137 = OOM kill being the most common). This adapts the idea to Hermes' existing exit-code semantics tier: - _interpret_signal_exit(): maps negative codes (definite signal death) and the 128+signum band (hedged with 'usually') to a note naming the signal and its likely cause, wired into _interpret_exit_code() ahead of the per-command semantics table. - Curated signal table (SIGKILL/SIGSEGV/SIGTERM/SIGABRT/...) so ambiguous application exit codes are never mislabeled; uncurated 128+N codes stay silent, SIGINT is excluded (executor's interrupt-marker path owns rc=130). - Notes surface via the existing exit_code_meaning result field. E2E verified against real SIGSEGV/SIGKILL processes. * Port from MoonshotAI/kimi-code#2596/NousResearch#2600: surface MCP tool-result _meta to the model, minus protocol-reserved keys MCP tool results carry a server _meta mapping (exposed as .meta by the Python SDK) alongside structuredContent. Servers return namespaced machine-readable contracts there (validated payloads, browser-handoff URLs); Hermes previously dropped the field entirely, so that data was invisible to the agent. Now _meta is included in the JSON tool output, after filtering protocol-reserved keys per the MCP spec's key-name rules: a prefix is reserved when a modelcontextprotocol or mcp label is followed by at least one more label (modelcontextprotocol.io/..., tools.mcp.com/...). Vendor namespaces with a trailing reserved word (com.example.mcp/...) and unprefixed keys pass through. Non-serializable metadata drops the extras rather than failing the call. * fix(telegram): rebind TypeHandler in the deferred SDK import `check_telegram_requirements()` re-imports python-telegram-bot after a lazy install and rebinds the module-level aliases that the top-level `except ImportError` block set to `typing.Any`. TypeHandler was left out of all three places: the `global` declaration, the `from telegram.ext import (...)` list, and the assignments. So whenever the top-level import fails and the deferred path runs, every other alias is restored and TELEGRAM_AVAILABLE flips to True, while TypeHandler stays `Any`. Handler registration then raises `TypeError: Any cannot be instantiated` and the gateway reports: [Telegram] Failed to connect to Telegram: Any cannot be instantiated Gateway started with no connected platforms The 22.6 -> 22.8 pin bump named in NousResearch#85272 is the trigger rather than the defect: it makes the top-level import fail, which is what routes the module through the deferred path where the omission has always been. * Port from aaif-goose/goose#10746: strip invisible Unicode TAG chars from MCP content Unicode TAG characters (U+E0000-U+E007F) render as nothing in terminals and chat UIs but are fully visible to LLM tokenizers, making them an ASCII-smuggling prompt-injection channel for untrusted MCP servers. - tools/ansi_strip.py: new strip_unicode_tags() with fast path; unlike goose we preserve valid emoji tag sequences (U+1F3F4 base + tag spec + U+E007F cancel), so regional flags survive. - tools/mcp_tool.py: applied at every MCP text ingestion point — tool result text blocks, embedded resource text, read_resource contents, get_prompt message content, and tool descriptions entering the schema. - tests/tools/test_unicode_tag_strip.py: smuggled-instruction vectors, goose's test vector, emoji-tag-sequence preservation, ZWJ untouched. * feat(business-os): implement Waves 1-33 strategic enhancements and full E2E QA suite - Add Waves 1-33 capabilities across finance, governance, GTM, SEO, and top-tier web/graphic design. - Implement Design System Tokens, WebGL Shaders, Motion Architecture, and Skeleton Shimmer Loaders. - Add WCAG 2.1 AAA Accessible Keyboard Focus Rings, Adaptive Breakpoints, and Fluid Typography Scalers. - Fix pre-existing unit test timing issues and model catalog mocking in test_models, test_objective_worker, and authority_integrity. - Create comprehensive E2E functional test suite (test_full_qa_e2e_workflow.py) verified across 381 unit & integration tests. * chore(contributors): add contributor mapping for paul.lesyuk@gmail.com --------- Co-authored-by: Sam Liu <sam7894604@gmail.com> Co-authored-by: Teknium <127238744+teknium1@users.noreply.github.com> Co-authored-by: Pavel Lesyuk <paul.lesyuk@gmail.com>
2 participants
Repro
With
bash.patterns: [{ match: "cargo *", approval: "allow" }], evaluatecargo bench --manifest-path layers/layer3/Cargo.toml --bench standardized_criterion -- --full '^layer3/write/file-wal/batch-(10|1000|10000)$'; currentmainreturns the Bash approval decision"exec"instead of{ tier: "write", policy: "allow" }.Cause
BashTool.approval()inpackages/coding-agent/src/tools/bash.tscalledBASH_APPROVAL_SHELL_CONTROL_REagainst the raw command, so shell metacharacters inside single-quoted literal arguments were indistinguishable from executable compound-command syntax.Fix
BashTool.approval()decision.Verification
bun test packages/coding-agent/test/tools/approval.test.tspasses 33 tests; the exact reported command now returns{"tier":"write","policy":"allow"}. Skipped the pre-publish gate after verifying on a cleanorigin/maincheckout thatbun run fixrewrites unrelatedcrates/pi-shell/src/shell.rsandbun checkfails identically in unrelated omptype migration paths (openai-responses-server-schema.tsandtask/types.ts). Fixes #7552