fix: harden safe-bin argument validation [AI] by pgondhi987 · Pull Request #80999 · openclaw/openclaw

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

Summary

Problem: safe-bin allowlist checks accepted shell-expanded argument forms as literal value tokens.
Why it matters: argument validation should fail closed when shell transport can rewrite token boundaries before execution.
What changed: safe-bin validation now rejects POSIX-style parameter expansion tokens, with regression coverage in policy and node-host system.run paths.
What did NOT change (scope boundary): no default safe-bin list, config schema, permissions, or network behavior changed.

Change Type (select all)

Scope (select all touched areas)

Linked Issue/PR

Related: N/A
This PR fixes a bug or regression

Real behavior proof (required for external PRs)

Behavior or issue addressed: safe-bin allowlist argument validation rejects shell-expanded token carriers before dispatch.
Real environment tested: Not run in this metadata drafting step.
Exact steps or command run after this patch: N/A
Evidence after fix: N/A
Observed result after fix: N/A
What was not tested: full OpenClaw runtime pairing flow and repository test wrapper.
Before evidence: N/A

Root Cause (if applicable)

Root cause: safe-bin literal-token validation blocked paths and globbing but did not reject shell parameter-expansion forms that can alter argv boundaries at runtime.
Missing detection / guardrail: regression coverage did not include shell-expanded safe-bin option values or positional tokens.
Contributing context (if known): safe-bin checks operate before shell transport executes the command.

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-safe-bins.test.ts; src/node-host/invoke-system-run.test.ts
Scenario the test should lock in: shell-expanded safe-bin value and positional tokens are denied before command execution.
Why this is the smallest reliable guardrail: it covers the shared validator plus the node-host system.run policy path without requiring a full gateway run.
Existing test that already covers this (if any): none specific to this token-carrier form.
If no new test is added, why not: N/A

User-visible / Behavior Changes

Safe-bin allowlist mode is stricter for shell-expanded argument tokens. Commands using those forms now require approval or fail allowlist matching instead of being auto-allowed.

Diagram (if applicable)

Before:
[system.run safe-bin command] -> [literal-token check passes] -> [shell may rewrite argv]

After:
[system.run safe-bin command] -> [expansion-token check fails] -> [allowlist miss]

Security Impact (required)

New permissions/capabilities? No
Secrets/tokens handling changed? No
New/changed network calls? No
Command/tool execution surface changed? Yes
Data access scope changed? No
If any Yes, explain risk + mitigation: command execution allowlist behavior is narrowed for safe-bin arguments that can be rewritten by shell expansion; mitigation is fail-closed validation with regression coverage.

Repro + Verification

Environment

OS: Linux-compatible POSIX path covered by tests
Runtime/container: Node test runtime
Model/provider: N/A
Integration/channel (if any): node-host system.run
Relevant config (redacted): tools.exec security allowlist, ask off, default safe bins

Steps

Submit a safe-bin shell-wrapper command containing parameter expansion in an option value.
Evaluate allowlist policy.
Confirm the command is denied before runCommand dispatch.

Expected

Allowlist matching fails and the command is not executed.

Actual

Covered by added regression tests; full wrapper command not run during metadata drafting.

Evidence

Failing test/log before + passing after
Trace/log snippets
Screenshot/recording
Perf numbers (if relevant)

Human Verification (required)

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

Verified scenarios: reviewed validator and regression-test changes for safe-bin value and positional token handling.
Edge cases checked: short option value, long option value, positional token, node-host system.run allowlist miss.
What you did not verify: full OpenClaw runtime pairing flow; repository test wrapper in this PR drafting step.

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
If yes, exact upgrade steps: N/A

Risks and Mitigations

Risk: some previously auto-allowed safe-bin shell commands with literal dollar-sign arguments may now require approval.
- Mitigation: the change is scoped to safe-bin literal validation, and explicit approval remains available for intentional commands.

AI-assisted: yes

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

Codex review: needs real behavior proof before merge.

Summary
The branch rejects POSIX shell-expansion carriers in safe-bin argument validation, rewrites POSIX safe-bin shell payloads before system.run dispatch, and adds policy/node-host regression tests.

Reproducibility: yes. By source inspection, current main accepts these safe-bin value and positional tokens through isSafeLiteralToken while the docs promise no $VARS expansion for safe bins; I did not run a live repro because this review is read-only.

Real behavior proof
Needs real behavior proof before merge: Missing: the PR body says no real environment command was run and after-fix evidence is N/A; the contributor should add redacted terminal output, logs, screenshot, recording, live output, or a linked artifact showing a real after-fix system.run denial, then update the PR body to trigger re-review or ask a maintainer to comment @clawsweeper re-review.

Next step before merge
Remaining blockers are contributor-supplied real behavior proof and explicit protected-label maintainer handling, not a narrow repair-lane code defect.

Security
Cleared: The diff narrows command-execution allowlist behavior and adds targeted tests; I found no new dependency, workflow, secret, permission, or supply-chain exposure.

Review details

Best possible solution:

Land the fail-closed validator and POSIX shell-payload rewrite after the contributor supplies redacted real system.run denial proof and a maintainer accepts the protected security-policy change.

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

Yes. By source inspection, current main accepts these safe-bin value and positional tokens through isSafeLiteralToken while the docs promise no $VARS expansion for safe bins; I did not run a live repro because this review is read-only.

Is this the best way to solve the issue?

Yes. The proposed direction is the narrow maintainable fix: reject expansion carriers in the shared safe-bin validator and rebuild POSIX safe-bin shell payloads before dispatch, with merge held for proof and protected-label handling.

Acceptance criteria:

pnpm test src/infra/exec-approvals-safe-bins.test.ts src/node-host/invoke-system-run.test.ts

What I checked:

Current validator gap: Current main's isSafeLiteralToken rejects empty, glob, and path-like tokens, but it does not reject shell parameter-expansion carriers before option values or positional tokens are accepted. (src/infra/exec-safe-bin-policy-validator.ts:34, 3b2735f3eea3)
Documented safe-bin contract: The advanced exec approvals docs say safe-bin argv tokens are forced to literal text at execution time with no globbing and no $VARS expansion for stdin-only segments. Public docs: docs/tools/exec-approvals-advanced.md. (docs/tools/exec-approvals-advanced.md:62, 3b2735f3eea3)
Current node-host allowlist path: Current main evaluates shell payloads through evaluateShellAllowlist and returns segment metadata, but resolveSystemRunExecArgv only rewrites Windows shell wrappers and does not rewrite POSIX safe-bin shell payloads. (src/node-host/invoke-system-run-allowlist.ts:36, 3b2735f3eea3)
PR validator change: The PR adds hasShellExpansionToken to the safe-bin literal validator and applies it alongside glob/path-like rejection. (src/infra/exec-safe-bin-policy-validator.ts:29, 4b1b2286aa26)
PR system.run handling: The PR threads segmentSatisfiedBy through node-host allowlist evaluation and rebuilds POSIX shell payloads with buildSafeBinsShellCommand, failing closed on execution-plan mismatch. (src/node-host/invoke-system-run-allowlist.ts:131, 4b1b2286aa26)
PR regression coverage: The PR adds safe-bin validator cases for short-option values, long-option values, positionals, plus node-host system.run denial and rewrite coverage. (src/node-host/invoke-system-run.test.ts:750, 4b1b2286aa26)

Likely related people:

steipete: Peter Steinberger authored the safe-bin policy centralization and module split that created the current validator path touched by this PR. (role: introduced behavior / feature owner; confidence: high; commits: 0d0f4c699237, 9530c0108589; files: src/infra/exec-safe-bin-policy.ts, src/infra/exec-safe-bin-policy-validator.ts, src/infra/exec-approvals-allowlist.ts)
pgondhi987: Current-main history attributes recent shell-wrapper and inline-command hardening near this system.run/exec analysis path to Pavan Kumar Gondhi, beyond this PR branch itself. (role: recent adjacent contributor; confidence: medium; commits: de47989731d3, 50f4440c9663; files: src/infra/shell-inline-command.ts, src/infra/shell-wrapper-resolution.ts, src/infra/system-run-command.ts)

Remaining risk / open question:

No after-fix real behavior proof is present for a real system.run denial path; tests are supplemental only for this external PR gate.
The protected maintainer label requires explicit maintainer handling before merge.

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

pgondhi987 · 2026-05-12T11:33:38Z

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 PR rejects shell-expansion tokens in safe-bin literal argument validation and adds regression coverage for policy and node-host system.run allowlist paths.

Reproducibility: yes. by source inspection: current main accepts safe-bin values and positionals through isSafeLiteralToken, which only checks glob and path-like tokens. I did not execute the repro because this sweep is read-only.

Real behavior proof
Needs real behavior proof before merge: The PR body explicitly says no real environment command was run and evidence is N/A, so the contributor needs after-fix terminal output, logs, screenshot, recording, or linked artifact from a real system.run denial with private details redacted; updating the PR body should trigger a fresh ClawSweeper review, or a maintainer can comment @clawsweeper re-review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, ask a maintainer to comment @clawsweeper re-review.

Next step before merge
Not a repair-lane candidate because the remaining blockers are external real behavior proof and protected-label maintainer handling, not a narrow automation repair.

Security
Cleared: The diff narrows the command execution allowlist surface and adds targeted tests; I found no new secret, permission, dependency, or supply-chain exposure.

Review details

Best possible solution:

Land a shared-validator fail-closed check with the added regression coverage after the contributor supplies real system.run denial proof and a maintainer accepts the stricter safe-bin policy.

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

Yes, by source inspection: current main accepts safe-bin values and positionals through isSafeLiteralToken, which only checks glob and path-like tokens. I did not execute the repro because this sweep is read-only.

Is this the best way to solve the issue?

Yes, the proposed direction is the narrowest maintainable fix: reject expansion carriers in the shared safe-bin literal validator and cover both direct policy and node-host system.run paths. Merge should wait for real behavior proof and maintainer review.

What I checked:

Current validator gap: Current main's safe-bin literal check rejects glob and path-like tokens, but it has no shell-expansion-token check before accepting option values or positionals. (src/infra/exec-safe-bin-policy-validator.ts:34, 3a4c97c291da)

Safe-bin runtime path: isSafeBinUsage resolves a trusted safe-bin executable and then delegates argv validation to validateSafeBinArgv, so the shared validator is the right place to fail closed. (src/infra/exec-approvals-allowlist.ts:65, 3a4c97c291da)

Node-host shell path: system.run evaluates shell payloads through evaluateShellAllowlist while threading safe-bin profiles and trusted dirs, so the PR's node-host regression target is on the relevant dispatch path. (src/node-host/invoke-system-run-allowlist.ts:36, 3a4c97c291da)

PR diff coverage: The PR adds hasShellExpansionToken to safe literal validation and tests short-option values, long-option values, positionals, and a node-host system.run allowlist miss. (src/infra/exec-safe-bin-policy-validator.ts:29, 24fb806d7390)

Real behavior proof gap: The PR body says the real environment was not run and after-fix evidence is N/A, so the external proof gate is not satisfied. (24fb806d7390)

Protected label: The provided GitHub context shows the maintainer label on this PR, which blocks conservative cleanup closure and requires explicit maintainer handling.

Likely related people:

Peter Steinberger: Authored the commits that centralized safe-bin policy checks and split the safe-bin validator/profile modules now touched by this PR. (role: introduced behavior / feature owner; confidence: high; commits: 0d0f4c699237, 9530c0108589; files: src/infra/exec-safe-bin-policy.ts, src/infra/exec-safe-bin-policy-validator.ts, src/infra/exec-approvals-allowlist.ts)

pgondhi987: Current-main history in this checkout attributes recent exec and node-host inline-command hardening to Pavan Kumar Gondhi, including adjacent tests in invoke-system-run.test.ts. (role: recent area contributor; confidence: medium; commits: 47eb2d48d434, de47989731d3; files: src/infra/exec-safe-bin-policy-validator.ts, src/infra/exec-approvals-allowlist.ts, src/node-host/invoke-system-run.test.ts)

Remaining risk / open question:

No after-fix real behavior proof is attached for the external PR; tests and source review are supplemental only.

The change intentionally narrows safe-bin allowlisting, so maintainers should accept the fail-closed behavior for literal-dollar safe-bin arguments before merge.

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

# Conflicts: # src/infra/exec-approvals-allowlist.ts

* fix: reject shell expansion in safe-bin tokens * fix: complete safe-bin shell payload handling * addressing codex review * addressing ci * addressing ci * addressing codex review * docs: add changelog entry for PR merge

fix: reject shell expansion in safe-bin tokens

24fb806

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

fix: complete safe-bin shell payload handling

36597f4

openclaw-barnacle Bot added size: M and removed size: XS labels May 12, 2026

pgondhi987 added 7 commits May 12, 2026 12:02

addressing codex review

cb7aa06

addressing ci

b493a91

addressing ci

633301a

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

4718f44

# Conflicts: # src/infra/exec-approvals-allowlist.ts

addressing codex review

4b1b228

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

86eb6ae

docs: add changelog entry for PR merge

c6d2470

pgondhi987 merged commit 9ac4272 into openclaw:main May 12, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

fix: harden safe-bin argument validation [AI]#80999

fix: harden safe-bin argument validation [AI]#80999
pgondhi987 merged 9 commits into
openclaw:mainfrom
pgondhi987:fix/fix-624

pgondhi987 commented May 12, 2026

Uh oh!

clawsweeper Bot commented May 12, 2026 •

edited

Loading

Uh oh!

pgondhi987 commented May 12, 2026

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

clawsweeper Bot commented May 12, 2026 •

edited

Loading