Skip to content

fix(chat-bash): inherit tools.exec security settings#41598

Closed
updatedb wants to merge 2 commits into
openclaw:mainfrom
updatedb:fix/chat-bash-inherit-exec-security
Closed

fix(chat-bash): inherit tools.exec security settings#41598
updatedb wants to merge 2 commits into
openclaw:mainfrom
updatedb:fix/chat-bash-inherit-exec-security

Conversation

@updatedb

Copy link
Copy Markdown

No description provided.

@greptile-apps

greptile-apps Bot commented Mar 10, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes handleBashChatCommand to properly inherit host, security, and ask settings from tools.exec config when constructing the exec tool, bringing it in line with how timeoutSec, notifyOnExit, and notifyOnExitEmptySuccess were already forwarded.

The three new fields are correctly typed — ExecToolConfig's string literal unions are directly compatible with ExecToolDefaults types.

In the chat-bash flow, elevated: true is always passed to execTool.execute(), which unconditionally sets host to "gateway" in the execution logic. Therefore the host inheritance has no practical runtime effect in this code path today, but it is a harmless and forward-compatible addition.

The security and ask inheritance is the meaningful fix: previously these would fall back to defaults ("allowlist" and "on-miss"/loaded approvals respectively) regardless of user configuration; now they correctly respect tools.exec.security and tools.exec.ask settings.

Confidence Score: 5/5

  • This PR is safe to merge — it is a straightforward, additive fix with no regressions or behavioral changes in normal code paths.
  • The change is minimal (3 lines) and follows an already-established pattern in the same function. Types are verified compatible, and the security semantics are an improvement: security and ask are now correctly inherited from config rather than silently falling back to permissive defaults. The host addition is harmless since elevation always overrides it to "gateway" in this code path.
  • No files require special attention.

Last reviewed commit: 0ae5876

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

Copy link
Copy Markdown
Contributor

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: 6246b6e75b

ℹ️ 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".

const bashSlashRequested =
command.commandBodyNormalized === "/bash" || command.commandBodyNormalized.startsWith("/bash ");
const bashBangRequested = command.commandBodyNormalized.startsWith("!");
const bashBangRequested = /^!(?:\s|:|[a-z0-9]|$)/i.test(command.commandBodyNormalized);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 Badge Preserve bang alias for punctuation-prefixed commands

The new bashBangRequested regex only treats ! as a bash alias when the next character is whitespace, :, or [a-z0-9], so valid commands like !/bin/ls, !./script.sh, or !$SHELL -lc ... are no longer routed to handleBashChatCommand. This is a regression from the previous startsWith("!") behavior and conflicts with parseBashRequest, which still supports arbitrary command text after !; users now silently lose the ! alias for a class of real shell commands unless they switch to /bash ....

Useful? React with 👍 / 👎.

@openclaw-barnacle

Copy link
Copy Markdown

This pull request has been automatically marked as stale due to inactivity.
Please add updates or it will be closed.

@openclaw-barnacle openclaw-barnacle Bot added stale Marked as stale due to inactivity triage: blank-template Candidate: PR template appears mostly untouched. labels Apr 26, 2026
@clawsweeper

clawsweeper Bot commented Apr 27, 2026

Copy link
Copy Markdown
Contributor

Thanks for the context here. I swept through the related work, and this is now duplicate or superseded.

Close as superseded: the useful config-inheritance part is now carried forward by #73982, which is the active broader implementation candidate and explicitly preserves this contributor’s attribution. This PR should not be merged as-is because its bang-command router change still regresses valid punctuation-prefixed shell commands.

So I’m closing this here and keeping the remaining discussion on the canonical linked item.

Review details

Best possible solution:

Use #73982 as the canonical follow-up: land a focused fix that makes chat-bash inherit the same effective exec defaults as the normal exec tool, preserves broad bang-command routing, and resolves the remaining maintainer/security review questions there.

Security review:

Security review cleared: The PR touches host-exec policy but only forwards existing config defaults and tests command routing; it does not add dependencies, workflows, package scripts, secret handling, or new code-download/execution paths.

What I checked:

  • Current main still has the original gap: handleBashChatCommand forwards timeoutSec, notifyOnExit, and notifyOnExitEmptySuccess into createExecTool, but not host, security, or ask. (src/auto-reply/reply/bash-command.ts:347, 7c51cd2baf1c)
  • Normal exec path already forwards policy defaults: The regular agent exec path resolves agent/global exec defaults and passes host, security, ask, node, PATH, safe-bin, strict inline-eval, timeout, approval notice, and notification defaults to the lazy exec tool. (src/agents/pi-tools.ts:195, 7c51cd2baf1c)
  • Exec runtime consumes those defaults: createExecTool derives the effective target from defaults.host, then merges configured/default security and ask before gateway/node approval handling. (src/agents/bash-tools.exec.ts:1546, 7c51cd2baf1c)
  • Docs define tools.exec as policy input: Exec approval docs say the effective policy is the stricter of tools.exec.* and approval defaults, and exec docs list tools.exec.host, tools.exec.security, and tools.exec.ask as config defaults. Public docs: docs/tools/exec-approvals.md. (docs/tools/exec-approvals.md:18, 7c51cd2baf1c)
  • PR diff introduces a bang-route regression: The PR changes bashBangRequested from startsWith("!") to a regex that only accepts whitespace, colon, alnum, or end-of-string after !, excluding existing commands such as !/bin/ls, !./script.sh, and !$SHELL -lc .... (src/auto-reply/reply/commands-bash.ts:13, 6246b6e75b0d)
  • Existing review already identified the regression: The PR has a P2 review comment on commands-bash.ts asking to preserve punctuation-prefixed bang aliases because parseBashRequest still supports arbitrary command text after !. (src/auto-reply/reply/commands-bash.ts:13, 6246b6e75b0d)

Likely related people:

  • steipete: Recent commits touch chat runtime policy isolation, command gating/security, exec YOLO semantics, and exec timeout behavior around the same policy surface. (role: recent maintainer and exec policy owner; confidence: high; commits: 6b41ef311f31, 08e020881d6e, 4a846dd129b1; files: src/auto-reply/reply/bash-command.ts, src/auto-reply/reply/directive-handling.persist.ts, src/agents/bash-tools.exec.ts)
  • Takhoffman: Recent focused commits changed bash wrapper routing and target-session behavior in the files this PR modifies. (role: recent bash wrapper maintainer; confidence: medium; commits: 9b95d65ea26f, 55f35708e181; files: src/auto-reply/reply/commands-bash.ts, src/auto-reply/reply/bash-command.ts, src/auto-reply/reply/commands-bash-alias.test.ts)
  • vincentkoc: Recent command flag and cycle refactors touched adjacent auto-reply and agent tool configuration surfaces that affect how this command path is wired. (role: adjacent command/config maintainer; confidence: medium; commits: 3059b36306ac, 74e7b8d47b18; files: src/auto-reply/reply/bash-command.ts, src/agents/pi-tools.ts)

Codex review notes: model gpt-5.5, reasoning high; reviewed against 7c51cd2baf1c.

@openclaw-barnacle openclaw-barnacle Bot removed the stale Marked as stale due to inactivity label Apr 28, 2026
@clawsweeper clawsweeper Bot closed this Apr 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size: S triage: blank-template Candidate: PR template appears mostly untouched.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant