fix: detect carried exec command forms [AI] by pgondhi987 · Pull Request #81000 · openclaw/openclaw

pgondhi987 · 2026-05-12T11:12:53Z

Summary

Problem: strict inline-eval detection did not inspect command forms carried through shell positional arguments.
Why it matters: approval-free allowlist execution needs the same command-risk checks for wrapped argv forms as direct argv forms.
What changed: added shared detection for POSIX shell positional carriers and covered command-executing carried tools in unit tests.
What did NOT change (scope boundary): no config defaults, permissions, network behavior, or approval storage format changed.

Change Type (select all)

Scope (select all touched areas)

Linked Issue/PR

N/A
This PR addresses a bug or regression

Real behavior proof (required for external PRs)

Behavior or issue addressed: strict inline-eval risk detection now follows shell positional carrier argv before approval-free allowlist execution.
Real environment tested: local OpenClaw checkout with Node and tsx.
Exact steps or command run after this patch: local Node/tsx probe importing src/infra/command-analysis/risks.ts and src/infra/command-analysis/policy.ts.
Evidence after fix: probe returned detected command-execution flags for shell positional carrier cases and null for a benign carried search case.
Observed result after fix: carried command-execution forms are classified before policy allow-through.
What was not tested: full pnpm test suite and live gateway/node runtime flow were not run in this metadata step.
Before evidence (optional but encouraged): issue context described the missed strict inline-eval classification.

Root Cause (if applicable)

Root cause: strict inline-eval risk detection inspected direct argv and known transparent command carriers, but not POSIX shell positional carrier argv.
Missing detection / guardrail: $0/$@ style shell carrier payloads were not recursively inspected before policy decisions.
Contributing context (if known): allowlist evaluation already had carrier-specific handling, while risk detection used a narrower traversal.

Regression Test Plan (if applicable)

Coverage level that should have caught this:
- Unit test
- Seam / integration test
- End-to-end test
- Existing coverage already sufficient
Target test or file: src/infra/command-analysis/risks.test.ts
Scenario the test should lock in: shell positional carriers preserve detection of command-executing carried tools while benign carried searches stay unflagged.
Why this is the smallest reliable guardrail: the shared risk detector feeds both gateway and node strict inline-eval policy paths.
Existing test that already covers this (if any): direct command-executing argv forms were covered; shell positional carrier forms were not.
If no new test is added, why not: N/A

User-visible / Behavior Changes

Approval-free allowlist execution with strict inline-eval enabled now requires explicit approval for command-executing forms hidden behind shell positional carriers.

Diagram (if applicable)

Before:
[wrapped argv] -> [outer shell inspected] -> [carried command risk missed]

After:
[wrapped argv] -> [positional carrier unwrapped] -> [carried command risk detected]

Security Impact (required)

New permissions/capabilities? (Yes/No) No
Secrets/tokens handling changed? (Yes/No) No
New/changed network calls? (Yes/No) No
Command/tool execution surface changed? (Yes/No) Yes
Data access scope changed? (Yes/No) No
If any Yes, explain risk + mitigation: command execution policy is more conservative for shell-carried argv forms when strict inline-eval is enabled.

Repro + Verification

Environment

OS: Linux
Runtime/container: local Node with tsx
Model/provider: N/A
Integration/channel (if any): N/A
Relevant config (redacted): strict inline-eval policy path

Steps

Import the shared command-risk detector.
Evaluate shell positional carrier argv cases that carry command-executing tools.
Evaluate a benign carried search argv case.

Expected

Command-executing carried forms return an inline-eval hit.
Benign carried search returns no hit.

Actual

Command-executing carried forms returned the expected flags.
Benign carried search returned null.

Evidence

Trace/log snippets

Human Verification (required)

What you personally verified (not just CI), and how:

Verified scenarios: direct Node/tsx probe of carried command-executing forms and benign carried search behavior.
Edge cases checked: sh -lc, bash -c, transparent env wrapper, and benign carried search.
What you did not verify: full repo test suite, CI, and live gateway/node runtime flow.

Review Conversations

I replied to or resolved every bot review conversation I addressed in this PR.
I left unresolved only the conversations that still need reviewer or maintainer judgment.

Compatibility / Migration

Backward compatible? (Yes/No) Yes
Config/env changes? (Yes/No) No
Migration needed? (Yes/No) No
If yes, exact upgrade steps: N/A

Risks and Mitigations

Risk: benign shell-carried command forms could be classified more conservatively if they use command-executing tool flags.
- Mitigation: tests include benign carried search behavior, and the detector only flags known inline command forms.

clawsweeper · 2026-05-12T11:16:19Z

Codex review: needs real behavior proof before merge.

Summary
The branch adds shell positional-carrier traversal to command inline-eval detection and expands command-analysis unit tests for carried find and xargs forms.

Reproducibility: yes. source inspection gives a concrete path: sh -lc '$0 $*' find . -exec id {} ';' is accepted as a positional carrier by current allowlist matching while the PR detector returns null for star. I did not execute a runtime probe because this review is read-only.

Real behavior proof
Needs real behavior proof before merge: The body describes a Node/tsx probe but includes no copied output, logs, terminal screenshot, or artifact; the contributor should add redacted proof and update the PR body for re-review.

Next step before merge
Missing contributor proof plus a P1 approval-boundary finding make this a human-review blocker, not a repair-lane candidate while the external proof gate is unmet.

Security
Needs attention: The diff changes command execution approval gating and leaves an unquoted $* carrier bypass in strictInlineEval coverage.

Review findings

[P1] Fail closed for unquoted $* carriers — src/infra/command-analysis/risks.ts:141-142

Review details

Best possible solution:

Align risk detection with the allowlist’s positional-carrier contract by covering or failing closed on unquoted $*, then add a strictInlineEval policy-path regression and posted real runtime proof before merge.

Do we have a high-confidence way to reproduce the issue?

Yes, source inspection gives a concrete path: sh -lc '$0 $*' find . -exec id {} ';' is accepted as a positional carrier by current allowlist matching while the PR detector returns null for star. I did not execute a runtime probe because this review is read-only.

Is this the best way to solve the issue?

No. The direction is right, but the current patch should handle or fail closed on unquoted $* and prove the strictInlineEval allowlist path, not only direct detector-level cases.

Full review comments:

[P1] Fail closed for unquoted $* carriers — src/infra/command-analysis/risks.ts:141-142
The new parser accepts $* in the carrier regex but returns null when the normalized token is star. Current allowlist matching treats sh -lc '$0 $*' find . -exec id {} ';' as a positional carrier to the allowlisted executable, so strictInlineEval can still miss the carried find -exec command and allow approval-free execution. Either unwrap unquoted $* conservatively or stop the allowlist path from treating it as a direct carrier.
Confidence: 0.86

Overall correctness: patch is incorrect
Overall confidence: 0.86

Security concerns:

[high] Unquoted star carriers remain approval-risky — src/infra/command-analysis/risks.ts:141
Because the allowlist path treats direct positional carrier commands as carried executable invocations, but the new detector returns null for $*, strictInlineEval can still miss a command-executing carried argv form.
Confidence: 0.84

Acceptance criteria:

pnpm test src/infra/command-analysis/risks.test.ts
pnpm test src/infra/exec-approvals-allow-always.test.ts
pnpm test src/agents/bash-tools.exec-host-gateway.test.ts
pnpm test src/agents/bash-tools.exec-host-node.test.ts

What I checked:

Protected live PR state: The live PR API shows this external PR is open at head d4a71a6 and carries the protected maintainer label, so conservative cleanup should not close it. (d4a71a662bed)
PR diff drops star carriers: The PR diff admits $* in the positional-carrier regex, then returns null when the normalized token is star, so unquoted $* is not inspected for carried command execution. (src/infra/command-analysis/risks.ts:141, d4a71a662bed)
Current allowlist treats star as a positional carrier: Current main recognizes direct shell positional carrier invocations with [@*] and then matches the carried executable path, so risk detection needs to align with that allowlist path. (src/infra/exec-approvals-allowlist.ts:859, f506ced2b3a6)
Strict inline-eval gates explicit approval: Gateway execution derives inlineEvalHit from detectPolicyInlineEval; a missed carried inline-eval hit can avoid the explicit approval required by strictInlineEval. (src/agents/bash-tools.exec-host-gateway.ts:295, f506ced2b3a6)
Current main lacks positional-carrier traversal in risks: Current main only checks direct interpreter inline-eval and known command carriers in detectInlineEvalArgvInternal, so the central gap is not already implemented on main. (src/infra/command-analysis/risks.ts:122, f506ced2b3a6)
Existing review context: The existing ClawSweeper comment already identified the same P1 $* approval-boundary finding and noted missing real behavior proof; no later review comments were present. (d4a71a662bed)

Likely related people:

steipete: Recent merged history introduced and routed shared command-analysis and inline-eval behavior through the exec approval path. (role: command-analysis feature introducer; confidence: high; commits: 3f7e6eebc2f5, bd0e10a2f68f, cf1991d27deb; files: src/infra/command-analysis/risks.ts, src/infra/exec-approvals-allowlist.ts, src/agents/bash-tools.exec-host-gateway.ts)
vincentkoc: Recent merged commits hardened carrier-risk detection in the same command-analysis file, including exec carrier and env split carrier handling. (role: recent command-risk contributor; confidence: high; commits: 1bf824f586b3, e2f4aa461700; files: src/infra/command-analysis/risks.ts, src/infra/command-analysis/risks.test.ts)
pgondhi987: Prior merged work touched shell-wrapper allowlist parsing and this PR builds on that same exec-approval shell-wrapper boundary, so the author is relevant beyond only opening this PR. (role: recent adjacent contributor; confidence: medium; commits: fc065b2693bf; files: src/infra/exec-approvals-allowlist.ts, src/infra/command-analysis/risks.ts)

Remaining risk / open question:

The PR touches command execution approval gating and still leaves an unquoted $* positional-carrier path uncovered.
The PR body describes a local probe but does not include copied output, logs, a terminal screenshot, or a linked artifact showing after-fix behavior.

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

pgondhi987 · 2026-05-12T11:46:34Z

Not applicable to this automation stage; changelog/release-note and external real behavior proof requirements are handled outside auto-pr stages.

Quoted comment from @clawsweeper:

Codex review: needs real behavior proof before merge.

Summary
The branch adds shell positional-carrier traversal to command inline-eval detection and expands command-analysis unit tests for carried find and xargs forms.

Reproducibility: yes. source inspection gives a concrete path: sh -lc '$0 $*' find . -exec id {} ';' is accepted as a positional carrier by current allowlist matching while the PR detector returns null for star. I did not execute a runtime probe because this review is read-only.

Real behavior proof
Needs real behavior proof before merge: The body describes a Node/tsx probe but includes no copied output, logs, terminal screenshot, or artifact; the contributor should add redacted proof and update the PR body for re-review.

Next step before merge
Missing contributor proof plus a P1 approval-boundary finding make this a human-review blocker, not a repair-lane candidate while the external proof gate is unmet.

Security
Needs attention: The diff changes command execution approval gating and leaves an unquoted $* carrier bypass in strictInlineEval coverage.

Review findings

[P1] Fail closed for unquoted $* carriers — src/infra/command-analysis/risks.ts:141-142

Review details

Best possible solution:

Align risk detection with the allowlist’s positional-carrier contract by covering or failing closed on unquoted $*, then add a strictInlineEval policy-path regression and posted real runtime proof before merge.

Do we have a high-confidence way to reproduce the issue?

Yes, source inspection gives a concrete path: sh -lc '$0 $*' find . -exec id {} ';' is accepted as a positional carrier by current allowlist matching while the PR detector returns null for star. I did not execute a runtime probe because this review is read-only.

Is this the best way to solve the issue?

No. The direction is right, but the current patch should handle or fail closed on unquoted $* and prove the strictInlineEval allowlist path, not only direct detector-level cases.

Full review comments:

[P1] Fail closed for unquoted $* carriers — src/infra/command-analysis/risks.ts:141-142
The new parser accepts $* in the carrier regex but returns null when the normalized token is star. Current allowlist matching treats sh -lc '$0 $*' find . -exec id {} ';' as a positional carrier to the allowlisted executable, so strictInlineEval can still miss the carried find -exec command and allow approval-free execution. Either unwrap unquoted $* conservatively or stop the allowlist path from treating it as a direct carrier.
Confidence: 0.86

Overall correctness: patch is incorrect
Overall confidence: 0.86

Security concerns:

[high] Unquoted star carriers remain approval-risky — src/infra/command-analysis/risks.ts:141
Because the allowlist path treats direct positional carrier commands as carried executable invocations, but the new detector returns null for $*, strictInlineEval can still miss a command-executing carried argv form.
Confidence: 0.84

Acceptance criteria:

pnpm test src/infra/command-analysis/risks.test.ts

pnpm test src/infra/exec-approvals-allow-always.test.ts

pnpm test src/agents/bash-tools.exec-host-gateway.test.ts

pnpm test src/agents/bash-tools.exec-host-node.test.ts

What I checked:

Protected live PR state: The live PR API shows this external PR is open at head d4a71a6 and carries the protected maintainer label, so conservative cleanup should not close it. (d4a71a662bed)

PR diff drops star carriers: The PR diff admits $* in the positional-carrier regex, then returns null when the normalized token is star, so unquoted $* is not inspected for carried command execution. (src/infra/command-analysis/risks.ts:141, d4a71a662bed)

Current allowlist treats star as a positional carrier: Current main recognizes direct shell positional carrier invocations with [@*] and then matches the carried executable path, so risk detection needs to align with that allowlist path. (src/infra/exec-approvals-allowlist.ts:859, f506ced2b3a6)

Strict inline-eval gates explicit approval: Gateway execution derives inlineEvalHit from detectPolicyInlineEval; a missed carried inline-eval hit can avoid the explicit approval required by strictInlineEval. (src/agents/bash-tools.exec-host-gateway.ts:295, f506ced2b3a6)

Current main lacks positional-carrier traversal in risks: Current main only checks direct interpreter inline-eval and known command carriers in detectInlineEvalArgvInternal, so the central gap is not already implemented on main. (src/infra/command-analysis/risks.ts:122, f506ced2b3a6)

Existing review context: The existing ClawSweeper comment already identified the same P1 $* approval-boundary finding and noted missing real behavior proof; no later review comments were present. (d4a71a662bed)

Likely related people:

steipete: Recent merged history introduced and routed shared command-analysis and inline-eval behavior through the exec approval path. (role: command-analysis feature introducer; confidence: high; commits: 3f7e6eebc2f5, bd0e10a2f68f, cf1991d27deb; files: src/infra/command-analysis/risks.ts, src/infra/exec-approvals-allowlist.ts, src/agents/bash-tools.exec-host-gateway.ts)

vincentkoc: Recent merged commits hardened carrier-risk detection in the same command-analysis file, including exec carrier and env split carrier handling. (role: recent command-risk contributor; confidence: high; commits: 1bf824f586b3, e2f4aa461700; files: src/infra/command-analysis/risks.ts, src/infra/command-analysis/risks.test.ts)

pgondhi987: Prior merged work touched shell-wrapper allowlist parsing and this PR builds on that same exec-approval shell-wrapper boundary, so the author is relevant beyond only opening this PR. (role: recent adjacent contributor; confidence: medium; commits: fc065b2693bf; files: src/infra/exec-approvals-allowlist.ts, src/infra/command-analysis/risks.ts)

Remaining risk / open question:

The PR touches command execution approval gating and still leaves an unquoted $* positional-carrier path uncovered.

The PR body describes a local probe but does not include copied output, logs, a terminal screenshot, or a linked artifact showing after-fix behavior.

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

* fix: detect carried inline exec forms * addressing review-skill * addressing codex review * addressing ci * docs: add changelog entry for PR merge

fix: detect carried inline exec forms

bf53580

openclaw-barnacle Bot added size: S maintainer Maintainer-authored PR labels May 12, 2026

addressing review-skill

9093d58

pgondhi987 added 2 commits May 12, 2026 11:23

addressing codex review

83c5392

addressing ci

d4a71a6

pgondhi987 added 2 commits May 12, 2026 14:05

Merge remote-tracking branch 'origin/main' into fix/fix-623

ee8be1c

docs: add changelog entry for PR merge

9bae180

pgondhi987 merged commit 65d7232 into openclaw:main May 12, 2026
21 of 22 checks passed

clawsweeper Bot mentioned this pull request Jun 29, 2026

test: cover versioned inline eval approval [AI] #97833

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

fix: detect carried exec command forms [AI]#81000

fix: detect carried exec command forms [AI]#81000
pgondhi987 merged 6 commits into
openclaw:mainfrom
pgondhi987:fix/fix-623

pgondhi987 commented May 12, 2026

Uh oh!

clawsweeper Bot commented May 12, 2026 •

edited

Loading

Uh oh!

pgondhi987 commented May 12, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Uh oh!

Conversation

pgondhi987 commented May 12, 2026

Summary

Change Type (select all)

Scope (select all touched areas)

Linked Issue/PR

Real behavior proof (required for external PRs)

Root Cause (if applicable)

Regression Test Plan (if applicable)

User-visible / Behavior Changes

Diagram (if applicable)

Security Impact (required)

Repro + Verification

Environment

Steps

Expected

Actual

Evidence

Human Verification (required)

Review Conversations

Compatibility / Migration

Risks and Mitigations

Uh oh!

clawsweeper Bot commented May 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pgondhi987 commented May 12, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

clawsweeper Bot commented May 12, 2026 •

edited

Loading