fix(exec): reject invalid host targets#74468
Conversation
openclaw-barnacle
Bot
added
docs
Greptile SummaryThis PR adds Confidence Score: 4/5Safe to merge; only P2 style and maintainability findings present. No P0 or P1 issues found. The validation logic is correct, error paths are well-tested, and the behavioral change (fail-closed on invalid host) is intentional and consistent with the PR description. Score is 4 (not 5) due to the duplicated host-value list in the schema file, which creates a future synchronization risk. src/agents/bash-tools.schemas.ts — the locally-defined EXEC_TOOL_HOST_VALUES is a duplicate of EXEC_TARGET_VALUES and will need manual updates if the allowed host set ever changes. Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/agents/bash-tools.schemas.ts
Line: 4
Comment:
**Duplicate host-value list diverges from `EXEC_TARGET_VALUES`**
`EXEC_TOOL_HOST_VALUES` is a separate hardcoded copy of the same four strings already exported as `EXEC_TARGET_VALUES` from `src/infra/exec-approvals.ts`. If a new target (e.g. `"worker"`) is added to the runtime constant but this schema-side array is not updated, the schema will silently reject the new value before `requireValidExecTarget` ever sees it. Since `exec-approvals.ts` is in a lower-level `infra` layer, importing from it in `bash-tools.schemas.ts` avoids the circular-dependency concern while keeping the two in sync.
```suggestion
import { EXEC_TARGET_VALUES } from "../infra/exec-approvals.js";
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/infra/exec-approvals.ts
Line: 62-64
Comment:
**Error message quotes raw (potentially whitespace-padded) value**
The throw uses the original `value` rather than `normalized`, so `requireValidExecTarget(" spark-ff13 ")` produces `Invalid exec host " spark-ff13 "` with embedded spaces inside the quotes. Using `normalized` instead makes the displayed value match what was actually looked up.
```suggestion
throw new Error(
`Invalid exec host "${normalized}". Allowed values: ${EXEC_TARGET_VALUES.join(", ")}.`,
);
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(exec): reject invalid host targets" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| import { Type } from "typebox"; | ||
| import { optionalStringEnum } from "./schema/typebox.js"; | ||
|
|
||
| const EXEC_TOOL_HOST_VALUES = ["auto", "sandbox", "gateway", "node"] as const; |
There was a problem hiding this comment.
EXEC_TARGET_VALUES
EXEC_TOOL_HOST_VALUES is a separate hardcoded copy of the same four strings already exported as EXEC_TARGET_VALUES from src/infra/exec-approvals.ts. If a new target (e.g. "worker") is added to the runtime constant but this schema-side array is not updated, the schema will silently reject the new value before requireValidExecTarget ever sees it. Since exec-approvals.ts is in a lower-level infra layer, importing from it in bash-tools.schemas.ts avoids the circular-dependency concern while keeping the two in sync.
| const EXEC_TOOL_HOST_VALUES = ["auto", "sandbox", "gateway", "node"] as const; | |
| import { EXEC_TARGET_VALUES } from "../infra/exec-approvals.js"; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/bash-tools.schemas.ts
Line: 4
Comment:
**Duplicate host-value list diverges from `EXEC_TARGET_VALUES`**
`EXEC_TOOL_HOST_VALUES` is a separate hardcoded copy of the same four strings already exported as `EXEC_TARGET_VALUES` from `src/infra/exec-approvals.ts`. If a new target (e.g. `"worker"`) is added to the runtime constant but this schema-side array is not updated, the schema will silently reject the new value before `requireValidExecTarget` ever sees it. Since `exec-approvals.ts` is in a lower-level `infra` layer, importing from it in `bash-tools.schemas.ts` avoids the circular-dependency concern while keeping the two in sync.
```suggestion
import { EXEC_TARGET_VALUES } from "../infra/exec-approvals.js";
```
How can I resolve this? If you propose a fix, please make it concise.| throw new Error( | ||
| `Invalid exec host "${value}". Allowed values: ${EXEC_TARGET_VALUES.join(", ")}.`, | ||
| ); |
There was a problem hiding this comment.
The throw uses the original value rather than normalized, so requireValidExecTarget(" spark-ff13 ") produces Invalid exec host " spark-ff13 " with embedded spaces inside the quotes. Using normalized instead makes the displayed value match what was actually looked up.
| throw new Error( | |
| `Invalid exec host "${value}". Allowed values: ${EXEC_TARGET_VALUES.join(", ")}.`, | |
| ); | |
| throw new Error( | |
| `Invalid exec host "${normalized}". Allowed values: ${EXEC_TARGET_VALUES.join(", ")}.`, | |
| ); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/infra/exec-approvals.ts
Line: 62-64
Comment:
**Error message quotes raw (potentially whitespace-padded) value**
The throw uses the original `value` rather than `normalized`, so `requireValidExecTarget(" spark-ff13 ")` produces `Invalid exec host " spark-ff13 "` with embedded spaces inside the quotes. Using `normalized` instead makes the displayed value match what was actually looked up.
```suggestion
throw new Error(
`Invalid exec host "${normalized}". Allowed values: ${EXEC_TARGET_VALUES.join(", ")}.`,
);
```
How can I resolve this? If you propose a fix, please make it concise.
|
Codex review: needs maintainer review before merge. What this changes: The PR adds runtime validation for per-call exec Maintainer follow-up before merge: This is an open implementation PR that appears valid; the remaining action is maintainer review and CI validation, not an automated replacement or repair branch. Review detailsBest possible solution: Land a narrow fail-closed exec host validation fix with schema, docs, changelog, and regression coverage, then let the linked bug close through the normal PR merge flow. Acceptance criteria:
What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against e46dccb35374. |
* fix(exec): reject invalid host targets * docs(changelog): credit exec host validation contributor --------- Co-authored-by: Peter Steinberger <[email protected]>
* fix(exec): reject invalid host targets * docs(changelog): credit exec host validation contributor --------- Co-authored-by: Peter Steinberger <[email protected]>
2 participants
Summary
hostvalues outsideauto,sandbox,gateway, andnodewere normalized away, so hostname-like input could silently fall back to the default target.host=autobehavior.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
host:values silently coerce to sandbox, causing agents to confabulate host-level facts #74426Root Cause (if applicable)
nullfor unknown host values and allowed target resolution to fall back to the configured/default target.hostinput fails before command launch.Regression Test Plan (if applicable)
src/infra/exec-approvals-policy.test.ts,src/agents/bash-tools.exec-foreground-failures.test.tshostvalues throw before the exec command can launch.User-visible / Behavior Changes
Direct exec calls with malformed
hostvalues now fail with an explicit validation error instead of falling back to the configured/default exec target. Validauto,sandbox,gateway, andnodebehavior is unchanged.Diagram (if applicable)
Security Impact (required)
Yes, explain risk + mitigation: command execution input validation is stricter; malformed per-call host values now fail closed before launch.Repro + Verification
Environment
Steps
host: "spark-ff13"and a command.hostvalue.Expected
hostbefore launching the command.Actual
Evidence
New regression tests cover invalid string and non-string host values, and the focused test lane passes.
Human Verification (required)
What I personally verified (not just CI), and how:
gatewaywith whitespace still normalizes correctly; non-string input fails explicitly.pnpm build && pnpm check && pnpm test; this PR was validated with focused tests pluscheck:changedfor the touched surface.Review Conversations
Compatibility / Migration
Risks and Mitigations
AI-assisted disclosure
This PR was AI-assisted. I reviewed the diff, understand the changed exec validation path, and ran the validation listed above, including local
codex review --base origin/main.