PR #57584: fix: evaluate shell wrapper inline compound commands against allowlist (#57377)

• Repository: openclaw/openclaw
• Author: WZBbiao
• State: closed | merged: True
• Link: https://github.com/openclaw/openclaw/pull/57584

Description (problem / solution / changelog)

Summary

• Problem: When a skill invokes a CLI tool via exec, OpenClaw constructs a compound shell command (/bin/sh -c "cat SKILL.md && printf '---CMD---' && gog-wrapper calendar events"). With security: "allowlist" + ask: "deny", the allowlist check resolves the binary as /bin/sh and silently drops the execution — no error, no log, no approval prompt.
• Why it matters: Silent drops cause model-dependent failure modes: Claude enters infinite </s> token loops (cost risk), Haiku fabricates output (hallucination risk), only GPT handles it gracefully. This affects any skill wrapping a CLI tool.
• What changed: Added recursive evaluation of shell wrapper inline commands — when a /bin/sh -c "..." compound command contains chain operators (&&, ||, ;), each sub-command is parsed and individually checked against the allowlist.
• What did NOT change: Single-command shell wrappers (e.g. bash -lc 'script.sh') keep original behavior to preserve allow-always persisted-pattern security constraints. No changes to the approval prompt flow or error reporting (the silent-drop-to-error improvement is a separate concern).

Change Type (select all)

• Bug fix
• Feature
• Refactor required for the fix
• Docs
• Security hardening
• Chore/infra

Scope (select all touched areas)

• Gateway / orchestration
• Skills / tool execution
• Auth / tokens
• Memory / storage
• Integrations
• API / contracts
• UI / DX
• CI/CD / infra

Linked Issue/PR

• Closes #57377
• This PR fixes a bug or regression

Root Cause / Regression History (if applicable)

• Root cause: evaluateSegments() in exec-approvals-allowlist.ts only checked the outermost binary (/bin/sh) against the allowlist when processing shell wrapper segments. The extracted inline command was only used for script path and $0 "$@" carrier checks, not for recursive sub-command validation.
• Missing detection / guardrail: No test coverage for compound commands wrapped in shell invocations.
• Prior context: The shell wrapper resolution code (shell-wrapper-resolution.ts) already extracted inline commands via extractShellWrapperInlineCommand(), but the allowlist evaluator didn't use this for recursive checking.
• Why this regressed now: This behavior likely existed since the allowlist feature was introduced — it became visible as more skills started wrapping CLI tools.

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/exec-approvals-analysis.test.ts
• Scenario the test should lock in: Compound commands inside shell wrappers are recursively evaluated; all sub-commands must be in allowlist; single-command wrappers are NOT recursively evaluated.
• Why this is the smallest reliable guardrail: Tests directly exercise the evaluateShellAllowlist function with compound command inputs.
• If no new test is added, why not: 5 new tests added.

User-visible / Behavior Changes

• Skill-invoked compound shell commands (e.g. cat SKILL.md && gog-wrapper calendar events) now correctly evaluate each sub-command against the allowlist instead of checking only /bin/sh.
• Single-command shell wrappers retain original behavior (no recursive evaluation).

Diagram (if applicable)

Before:
[skill exec] -> sh -c "cat SKILL.md && gog" -> resolves to /bin/sh -> NOT in allowlist -> SILENT DROP

After:
[skill exec] -> sh -c "cat SKILL.md && gog" -> detects chain operators -> recurse:
                                                  ├─ cat SKILL.md   -> safe bin ✓
                                                  └─ gog            -> in allowlist ✓ -> ALLOWED

Security Impact (required)

• New permissions/capabilities? No
• Secrets/tokens handling changed? No
• New/changed network calls? No
• Command/tool execution surface changed? Yes — compound shell commands are now recursively evaluated instead of being silently rejected.
  • Risk: Recursive evaluation could theoretically allow a carefully crafted compound command to pass allowlist checks.
  • Mitigation: Recursion depth limited to MAX_SHELL_WRAPPER_INLINE_EVAL_DEPTH = 3; only triggers on chain operators (&&, ||, ;); ALL sub-commands must individually satisfy the allowlist; single-command wrappers are excluded to preserve allow-always security constraints.
• Data access scope changed? No

Repro + Verification

Environment

• OS: macOS 14.x
• Runtime/container: Node.js v25.4.0
• Model/provider: Any (affects all models differently)
• Relevant config: security: "allowlist", ask: "deny" in exec config

Steps

1. Install a skill that wraps a CLI tool (e.g. gog skill)
2. Configure security: "allowlist" with the CLI tool path
3. Trigger the skill — it constructs sh -c "cat SKILL.md && gog-wrapper calendar events"
4. Observe the exec result

Expected

• Command executes successfully, returning CLI output.

Actual (before fix)

• Command silently dropped. Agent receives empty result. Model-dependent failures ensue.

Evidence

• Failing test/log before + passing after
• 199/199 exec-approvals tests pass (5 new + 194 existing across 8 test files)

Human Verification (required)

• Verified scenarios: Compound command with all binaries in allowlist passes; compound with unlisted binary fails; mixed allowlist + safe bins; single-command wrapper not recursed.
• Edge cases checked: Recursion depth limit; Windows platform excluded; empty chain parts; single-command wrappers preserve allow-always constraints.
• What you did not verify: End-to-end skill invocation with real CLI tool.

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
• Config/env changes? No
• Migration needed? No

Risks and Mitigations

• Risk: Recursive evaluation adds complexity to the security-critical allowlist path.
  • Mitigation: Depth limit of 3; only triggered by chain operators; comprehensive test coverage; single-command exclusion preserves existing security invariants.

Changed files

• src/agents/pi-embedded-subscribe.handlers.messages.ts (modified, +22/-2)
• src/infra/exec-approvals-allowlist.ts (modified, +97/-0)
• src/infra/exec-approvals-analysis.test.ts (modified, +134/-1)