Security: add tirith pre-exec command scanning#41320
Conversation
Greptile SummaryThis PR adds an optional pre-exec security layer using tirith that intercepts shell commands before execution and returns Key strengths:
Recommended improvements:
Confidence Score: 4/5
Last reviewed commit: 597ef0c |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| let securityWarning = false; | ||
| if (params.commandSecurityConfig?.enabled !== false) { | ||
| const shell = String(nodeInfo?.platform ?? "") | ||
| .toLowerCase() | ||
| .startsWith("win") | ||
| ? "cmd" | ||
| : "posix"; | ||
| const tirithExecEnv: Record<string, string> = { ...coerceEnv(process.env) }; | ||
| const hostShellPath = getShellPathFromLoginShell({ | ||
| env: process.env, | ||
| timeoutMs: resolveShellEnvFallbackTimeoutMs(process.env), | ||
| }); | ||
| if (hostShellPath) { | ||
| tirithExecEnv.PATH = hostShellPath; | ||
| } | ||
| const securityCheck = await checkCommandSecurity(params.command, params.commandSecurityConfig, { | ||
| shell, | ||
| env: tirithExecEnv, | ||
| }); | ||
| if (securityCheck.action === "block") { | ||
| throw new Error(`exec denied: command blocked by security scan.\n${securityCheck.summary}`); | ||
| } | ||
| if (securityCheck.action === "warn" && securityCheck.findings.length > 0) { | ||
| securityWarning = true; | ||
| params.warnings.push(`Security: ${securityCheck.summary}`); | ||
| } | ||
| } |
There was a problem hiding this comment.
The security check (checkCommandSecurity) at lines 186–212 runs after system.run.prepare at line 107. If tirith returns block, an error is thrown, but the prepared execution context may be abandoned without cleanup.
Compare this to the gateway path in bash-tools.exec.ts (line 422), where the security check runs before any downstream API calls. Moving the security check to line ~150 (after nodeInfo is resolved but before system.run.prepare) would:
- Avoid an unnecessary prepare API call if the command is blocked
- Match the cleaner pattern in the gateway path
- Eliminate potential stale state
The check needs nodeInfo.platform for shell detection, so it cannot move earlier than line 97, but it can move to after line 105 but before line 107.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/bash-tools.exec-host-node.ts
Line: 186-212
Comment:
The security check (`checkCommandSecurity`) at lines 186–212 runs *after* `system.run.prepare` at line 107. If tirith returns `block`, an error is thrown, but the prepared execution context may be abandoned without cleanup.
Compare this to the gateway path in `bash-tools.exec.ts` (line 422), where the security check runs before any downstream API calls. Moving the security check to line ~150 (after `nodeInfo` is resolved but before `system.run.prepare`) would:
- Avoid an unnecessary prepare API call if the command is blocked
- Match the cleaner pattern in the gateway path
- Eliminate potential stale state
The check needs `nodeInfo.platform` for shell detection, so it cannot move earlier than line 97, but it can move to after line 105 but before line 107.
How can I resolve this? If you propose a fix, please make it concise.| findings = (verdict.findings || []).map((f: Record<string, unknown>) => ({ | ||
| rule_id: (f.rule_id as string) || "", | ||
| severity: (f.severity as string) || "", | ||
| title: (f.title as string) || "", | ||
| description: (f.description as string) || "", | ||
| })); | ||
| if (findings.length > 0) { | ||
| summary = findings | ||
| .map((f) => { | ||
| return f.severity ? `[${f.severity}] ${f.title}` : f.title; | ||
| }) | ||
| .join("; "); | ||
| } |
There was a problem hiding this comment.
The findings array from tirith output is mapped without any length cap at line 84:
findings = (verdict.findings || []).map((f: Record<string, unknown>) => ({ ... }));A misbehaving or compromised tirith binary could return tens of thousands of findings. The downstream summary string (lines 91–95) would then be an unbounded concatenation, which is surfaced in error messages and stored in warnings.
For consistency with the stdout fallback (which is capped at 500 chars at line 100), consider adding a cap to findings and truncating the summary—e.g. slice(0, 50) on the findings array.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/security/command-security.ts
Line: 84-96
Comment:
The `findings` array from tirith output is mapped without any length cap at line 84:
```ts
findings = (verdict.findings || []).map((f: Record<string, unknown>) => ({ ... }));
```
A misbehaving or compromised tirith binary could return tens of thousands of findings. The downstream `summary` string (lines 91–95) would then be an unbounded concatenation, which is surfaced in error messages and stored in `warnings`.
For consistency with the stdout fallback (which is capped at 500 chars at line 100), consider adding a cap to findings and truncating the summary—e.g. `slice(0, 50)` on the findings array.
How can I resolve this? If you propose a fix, please make it concise.
Integrate tirith as an optional pre-exec security scanner for shell commands. tirith analyzes commands before execution and returns allow, warn, or block based on 66 detection rules covering pipe-to-shell patterns, homograph URLs, environment injection, and more. - Add `checkCommandSecurity()` wrapper in `src/security/command-security.ts` - Hook into gateway/sandbox path (bash-tools.exec.ts) and node path (bash-tools.exec-host-node.ts) with correct shell detection per host - Escalate warn→block on approval-less paths (bypassApprovals, sandbox) - Deny on approval timeout when security warning is active - Use login-shell PATH for tirith resolution (not env.PATH with pathPrepend) - Add `tools.exec.commandSecurity` config with enabled, failOpen, timeoutMs, and tirithPath options - Forward config through pi-tools, auto-reply, and zod schema - Add 20 unit tests and 8 integration tests (28 total) Closes discussion #10317
- Move tirith check before system.run.prepare to avoid wasted API call when command is blocked - Cap findings at 50 entries and summary at 500 chars to bound output from misbehaving tirith binary
8f91424 to
b0a4d0c
Compare
|
This pull request has been automatically marked as stale due to inactivity. |
|
Thanks for the context here. I swept through the related work, and this is now duplicate or superseded. Keep open: the hardening direction is useful, but the PR is not merge-ready because it changes exec behavior by default, runs a third-party scanner before approval with raw environment, does not constrain tirith to side-effect-free preflight, lacks real behavior proof, and is currently conflicting with main. Canonical path: Close this stale PR. The latest review rated it F, the branch still lacks merge-ready proof, and there has been no human follow-up after the durable review. So I’m closing this here because the remaining work is already tracked in the canonical issue. Review detailsBest possible solution: Close this stale PR. The latest review rated it F, the branch still lacks merge-ready proof, and there has been no human follow-up after the durable review. Do we have a high-confidence way to reproduce the issue? Yes for the review blockers: PR-head source shows the default-on condition and raw inherited scanner environment, and upstream tirith source shows daemon, audit/session, background update, and webhook side-effect paths. The feature itself is new, so there is no current-main bug reproduction to establish. Is this the best way to solve the issue? No. A tirith-backed scanner may be useful, but this implementation should not merge until the default policy, sanitized environment, cwd-aware scanning, side-effect-free preflight contract, and real runtime proof are settled. Security review: Security review needs attention: The diff introduces concrete security-boundary concerns around pre-approval third-party process execution, inherited environment exposure, and unconstrained scanner side effects.
What I checked:
Likely related people:
Codex review notes: model gpt-5.5, reasoning high; reviewed against 0d7d99befab4. |
clawsweeper
Bot
added
the
P1
openclaw-barnacle
Bot
added
the
triage: needs-real-behavior-proof
clawsweeper
Bot
added
impact:security
|
ClawSweeper PR egg 🎁 Pass real behavior proof to wake the egg and unlock a hatchable treat. Where did the egg go?
|
|
ClawSweeper applied the proposed close for this PR.
|
1 participant
Summary
allow,warn, orblock. Two hook points: gateway/sandbox (pre-dispatch) and node (post-dispatch, after platform resolution). Configurable viatools.exec.commandSecurity.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
User-visible / Behavior Changes
tools.exec.commandSecuritywith fieldsenabled(default: true),failOpen(default: true),timeoutMs(default: 5000),tirithPath(default: "tirith").What is tirith?
tirith is a terminal security tool that intercepts shell commands and analyzes them before execution. It runs entirely locally — no network calls, no telemetry, no cloud dependency. Sub-millisecond overhead on clean commands.
66 detection rules across 11 categories:
curl | bash,wget | sh, every source-to-sink variant includingpython <(curl ...),eval $(wget ...)curl -k), shortened URLsWhy this matters for OpenClaw: The agent can execute arbitrary shell commands. A prompt injection or hallucinated command could pipe a remote script to bash, or curl a homograph URL that visually mimics a legitimate domain but resolves to an attacker's server. tirith catches these before execution.
Beyond command scanning, tirith also offers (not part of this PR, but available to users):
tirith_check_command,tirith_check_url,tirith_scan_file,tirith_verify_mcp_config, and more.cursorrules,CLAUDE.md,.mcp.json, etc.)tirith setup openclawinstalls the integration automaticallyInstall:
brew install sheeki03/tap/tirith·cargo install tirith·npm i -g @anthropic/tirith· more optionsHow it works
Two hook points:
bash-tools.exec.ts(pre-dispatch)process.platform→ posix/powershellbash-tools.exec-host-node.ts(post-dispatch)nodeInfo.platform→ posix/cmdlistNodes()Warn escalation: On paths without an approval mechanism (elevated-bypass gateway, sandbox), warn is escalated to block. Rationale: if tirith detects something suspicious and there's no way to show the user, the safe default is to block.
Security Impact (required)
No— tirith runs as a local child process, same as existing toolsNoNo— tirith analysis is entirely localYes— adds a pre-exec security check that can block or warn on commandsNoexecFile(notexec) — the command string is passed as an argv element, not shell-interpolated. The check runs with a configurable timeout (default 5s) and fail-open default, so a misbehaving tirith binary cannot hang the exec pipeline. If tirith is not installed (ENOENT), the check silently allows.Repro + Verification
Environment
tools.exec.commandSecurity.enabled: trueSteps
brew install sheeki03/tap/tirithls -laExpected
Actual
Evidence
28 new tests passing:
src/security/command-security.test.ts— 20 unit tests covering all exit code paths, JSON parsing, failOpen behavior, shell flag forwarding, env passthrough, tilde expansionsrc/agents/bash-tools.exec.approval-id.test.ts— 8 integration tests covering gateway block, gateway warn-to-approval, elevated bypass escalation, enabled=false skip, node block, node warn-to-approval, node shell detectionHuman Verification (required)
shell-utils.tsandnode-shell.tsconventions.Review Conversations
Compatibility / Migration
Yes— fail-open by default, no behavior change when tirith is not installedYes— new optionaltools.exec.commandSecurityconfig keyNo— zero-config activation, works out of the boxbrew install sheeki03/tap/tirithorcargo install tirith), optionally configuretools.exec.commandSecurityin configFailure Recovery (if this breaks)
tools.exec.commandSecurity.enabled: falsein config, or uninstall tirith (ENOENT = allow)timeoutMs(default 5s), then allowed (fail-open). If tirith returns unexpected exit codes, behavior depends onfailOpensetting.Risks and Mitigations
tirithPathto an absolute path.