Skip to content

fix(exec): PowerShell /c and bash -l -c can be saved as allow-always rules - #1511

Open
SebTardif wants to merge 2 commits into
openclaw:mainfrom
SebTardif:fix/f108-inline-shell-allow-rules
Open

SebTardif wants to merge 2 commits into
openclaw:mainfrom
SebTardif:fix/f108-inline-shell-allow-rules

Conversation

@SebTardif

Copy link
Copy Markdown
Contributor

What Problem This Solves

Fixes: Allow always can save powershell.exe /c and bash -l -c as a reusable rule, so a later identical script runs with no prompt.

User Impact

User impact: those inline shell spellings stay one-shot. A direct program with arguments can still be saved.

Why This Change Was Made

The wrapper detector now treats PowerShell /c, /command, and colon-attached forms as inline commands. POSIX -c, --command, and -lc count in any argument after the shell name, so bash -l -c is included. bash script.sh and bash -l script.sh stay direct commands.

Evidence

Terminal output from commit bbc8dfe0.

Passed!  - Failed: 0, Passed: 9, Skipped: 0, Total: 9, Duration: 14 ms
powershell.exe /c Get-Date: ShellWrapper
pwsh /command Get-Date: ShellWrapper
powershell -c:Get-Date: ShellWrapper
bash -l -c echo hello: ShellWrapper
bash script.sh: not a wrapper
bash -l script.sh: not a wrapper

The full ExecApprovalV2NormalizationTests class earlier in this session: Passed 112, Failed 0.

Change Type

  • Bug fix
  • Feature
  • Refactor
  • Docs or instructions
  • Tests or validation
  • Security hardening
  • Chore or infrastructure

Scope

  • Tray or WinUI UX
  • Windows node capability
  • Local MCP or winnode
  • Gateway, connection, or pairing
  • Setup or onboarding
  • Permissions, privacy, or security
  • Tests, CI, or docs

Required proof pools

  • none: the decision is argv classification. No approved command was executed.

Validation

  • dotnet test ./tests/OpenClaw.Shared.Tests/OpenClaw.Shared.Tests.csproj --filter "FullyQualifiedName~TryBind_InlineShell_IsNotReusable|FullyQualifiedName~Normalizer_BashLoginInlineCommand|FullyQualifiedName~Normalizer_PowerShellExeSlashC" on bbc8dfe0: Passed 9, Failed 0.
  • ./build.ps1 was not run for this commit.

Real Behavior Proof

  • Behavior or issue addressed: powershell.exe /c and bash -l -c were not inline shells, so Allow always could pin them.
  • Real environment tested: Windows, this worktree, commit bbc8dfe0.
  • Exact steps or command run after this patch: dotnet test ./tests/OpenClaw.Shared.Tests/OpenClaw.Shared.Tests.csproj --filter "FullyQualifiedName~TryBind_InlineShell_IsNotReusable|FullyQualifiedName~Normalizer_BashLoginInlineCommand|FullyQualifiedName~Normalizer_PowerShellExeSlashC"
  • Evidence after fix: Those argv values fail TryBind with ShellWrapper. bash script.sh and bash -l script.sh are not wrappers.
  • Observed result after fix: The inline spellings cannot be stored as reusable allow rules.
  • Screenshot or artifact links verified? No
  • Not verified or blocked: The approval dialog was not opened, and no shell was executed.
  • What was not tested: A clicked Allow always in the tray.

Security Impact

  • New permissions or capabilities? No
  • Secrets or tokens handling changed? No
  • New or changed network calls? No
  • Command or tool execution surface changed? Yes
  • Data access scope changed? No
  • If any answer is Yes, explain the risk and mitigation: More inline shell spellings stay prompt-only. Direct executables are unchanged. cmd /c behavior is unchanged.

Compatibility and Migration

  • Backward compatible? Yes
  • Config or environment changes? No
  • Migration needed? No
  • If yes, list the exact upgrade steps: Existing saved rules are not rewritten. New Allow always decisions for these spellings are refused.

Review Conversations

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

Allow-always could store powershell.exe /c and bash -l -c as reusable
rules because those spellings were not treated as inline shells.

- Treat PowerShell /c, /command, and colon-attached forms as wrappers
- Treat POSIX -c, --command, and -lc in any argument after the shell name
- Keep bash script.sh and bash -l script.sh bindable

Signed-off-by: Sebastien Tardif <SebTardif@ncf.ca>
@clawsweeper

clawsweeper Bot commented Sep 24, 2026 •

Copy link
Copy Markdown

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@clawsweeper clawsweeper Bot added P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Sep 24, 2026
@clawsweeper

clawsweeper Bot commented Sep 24, 2026 •

Copy link
Copy Markdown

Codex review: needs real behavior proof before merge. Reviewed September 24, 2026, 2:30 PM ET / 18:30 UTC (Revision 2).

ClawSweeper review

What this changes

The branch expands PowerShell and POSIX shell detection so more inline command forms are treated as one-time approvals, and adds focused normalizer and binder tests.

Merge readiness

⛔ Blocked before merge - 6 items remain

This PR addresses a real gap in reusable command approvals, and current main does not contain the fix. Two parser boundary defects remain from the prior review: script arguments can be mistaken for inline shell commands. The upgrade effect on saved rules and real approval-path behavior also need resolution before merge.

Priority: P2
Reviewed head: 031e8512a2ec8ac330cd55790038f99dc3f87ddf
Owner decision: Required. See Decision needed.

Review scores

Measure Result What it means
Overall readiness 🦪 silver shellfish (2/6) The patch is focused, but two parser defects and unit-only proof leave it short of merge readiness.
Proof confidence 🦪 silver shellfish (2/6) Needs real behavior proof before merge: The PR reports focused Windows tests of the changed normalizer and binder at the earlier head, but no real system.run approval, saved-rule upgrade, or later execution result. Current-head production-path proof is still needed; redact private details in any transcript or screenshot. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Patch quality 🦐 gold shrimp (3/6) 2 actionable review findings remain.

Verification

Check Result Evidence
Real behavior Needs proof Needs real behavior proof before merge: The PR reports focused Windows tests of the changed normalizer and binder at the earlier head, but no real system.run approval, saved-rule upgrade, or later execution result. Current-head production-path proof is still needed; redact private details in any transcript or screenshot. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed 8 items Introduced POSIX scan: The PR scans every argument for an inline flag, including arguments after a script operand.
Introduced PowerShell forms: New slash and colon command forms are recognized anywhere in the argument list, including after a -File script operand.
Approval consequence: A detected wrapper makes reusable binding fail; normalization then supplies no saved-rule match or Allow always pattern.
Findings 2 actionable findings [P1] Stop POSIX option scanning at the script operand
[P1] Keep PowerShell script arguments out of command detection
Security None None.

How this fits together

The Windows node receives command arguments from system.run and normalizes them before checking approval policy. That classification determines whether an approval can become a saved rule and whether a later command can use it.

flowchart LR
  A[system.run arguments] --> B[Shell form detection]
  B --> C{Inline command?}
  C -->|Yes| D[One-time approval]
  C -->|No| E[Reusable command binding]
  E --> F[Saved-rule match]
  D --> G[Approved execution]
  F --> G
Loading

Decision needed

Question Recommendation
Should existing saved rules for these newly recognized inline shell forms become inactive immediately after upgrade? Fail closed with upgrade guidance: Keep inline shell commands one-time and tell users why an older saved rule now prompts again.

Why: That fail-closed change affects existing approvals and requires an explicit compatibility and security choice.

Before merge

  • Add real behavior proof - Needs real behavior proof before merge: The PR reports focused Windows tests of the changed normalizer and binder at the earlier head, but no real system.run approval, saved-rule upgrade, or later execution result. Current-head production-path proof is still needed; redact private details in any transcript or screenshot. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
  • Stop POSIX option scanning at the script operand (P1) - For bash script.sh -c value, the new loop treats a script argument as an inline shell flag. Reusable binding then fails and an existing saved rule cannot match. Stop interpreting shell options once the script operand is reached, and cover arguments after that operand.
  • Keep PowerShell script arguments out of command detection (P1) - For pwsh -File script.ps1 /c value, the newly recognized /c occurs after the script path but is classified as a shell command switch. That suppresses reusable approval for a direct script invocation. End switch detection at the -File operand and cover slash and colon forms used as script arguments.
  • Resolve merge risk (P1) - Existing saved rules for the newly classified inline commands can stop matching after upgrade. A maintainer must confirm the intended fail-closed transition and its user-facing guidance.
  • Complete next step (P2) - Repair both script-operand parsing defects, obtain the saved-rule upgrade decision, add redacted current-head system.run proof, and report the required validation. Updating the PR body should trigger re-review; otherwise ask a maintainer to comment @clawsweeper re-review.
  • Resolve maintainer decision - Resolve the maintainer decision shown above before merge.

Findings

  • [P1] Stop POSIX option scanning at the script operand — src/OpenClaw.Shared/ExecApprovals/ExecShellWrapperNormalizer.cs:81-89
  • [P1] Keep PowerShell script arguments out of command detection — src/OpenClaw.Shared/ExecApprovals/ExecShellWrapperNormalizer.cs:114-116
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production and test delta production +26/-8 lines, tests +63/-0 lines The production change is focused on shell classification, with added binder coverage but no approval-path proof.

Merge-risk options

Maintainer options:

  1. Confirm fail-closed upgrade (recommended)
    Approve the changed treatment of saved rules and require a real upgrade-path demonstration and user-facing guidance.
  2. Pause for a compatibility design
    Hold this PR if existing saved-rule behavior must be preserved under a separately reviewed authorization contract.

Technical review

Best possible solution:

Parse shell options only until the script operand, retain one-time treatment for actual inline commands, and document and prove the approved upgrade behavior for saved rules.

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

Yes, the source gives a high-confidence binder path: the new scans classify a script argument as a wrapper and suppress reusable binding. This review did not execute it.

Is this the best way to solve the issue?

No. Recognizing the missing inline forms is appropriate, but scanning past a script operand breaks direct-script classification.

Full review comments:

  • [P1] Stop POSIX option scanning at the script operand — src/OpenClaw.Shared/ExecApprovals/ExecShellWrapperNormalizer.cs:81-89
    For bash script.sh -c value, the new loop treats a script argument as an inline shell flag. Reusable binding then fails and an existing saved rule cannot match. Stop interpreting shell options once the script operand is reached, and cover arguments after that operand.
    Confidence: 0.97
  • [P1] Keep PowerShell script arguments out of command detection — src/OpenClaw.Shared/ExecApprovals/ExecShellWrapperNormalizer.cs:114-116
    For pwsh -File script.ps1 /c value, the newly recognized /c occurs after the script path but is classified as a shell command switch. That suppresses reusable approval for a direct script invocation. End switch detection at the -File operand and cover slash and colon forms used as script arguments.
    Confidence: 0.9

Overall correctness: patch is incorrect
Overall confidence: 0.93

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against 273b0182745a.

Labels

Label changes:

No label changes.

Label justifications:

  • P2: This is a focused approval-policy repair with limited blast radius, but it needs parser and upgrade review.
  • merge-risk: 🚨 compatibility: New wrapper classifications can disable previously saved approvals and can misclassify script arguments.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🦐 gold shrimp.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR reports focused Windows tests of the changed normalizer and binder at the earlier head, but no real system.run approval, saved-rule upgrade, or later execution result. Current-head production-path proof is still needed; redact private details in any transcript or screenshot. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Evidence

What I checked:

Likely related people:

  • AlexAlves87: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
  • Barbara Kudiess: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
  • Caleb Eden: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Fix and cover option parsing around POSIX and PowerShell script operands.
  • Show redacted current-head system.run approval and rerun behavior, including the agreed saved-rule upgrade case.
  • Report the required build, shared and tray tests, and MXC E2E result.

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (1 earlier review cycle)
  • reviewed 2026-09-24T18:12:04.149Z sha bbc8dfe :: needs real behavior proof before merge. :: [P2] Stop scanning POSIX options at the script operand | [P2] Keep PowerShell script arguments out of command detection

Signed-off-by: Sebastien Tardif <SebTardif@ncf.ca>
@karkarl

karkarl commented Sep 25, 2026

Copy link
Copy Markdown
Collaborator

Global triage: HOLD_FOR_AUTHOR. Take confidence 5%; recommendation confidence 99%; effort medium; risk high.

Three verified parsing gaps block this security-boundary change:

  1. Valid combined Bash option clusters such as -ec still permit durable inline-command approval. Together with existing slash/backslash argument normalization, an approved escaped separator can be replayed as an executable separator.
  2. POSIX scanning continues after the script operand, so bash script.sh -c value incorrectly treats the script argument as a shell switch and disables reusable approval.
  3. PowerShell scanning likewise continues after -File script.ps1, misclassifying script arguments such as /c or -c:value.

Please implement operand-aware, shell-specific option parsing and add saved-rule replay coverage for combined flags, post-script arguments, PowerShell -File, and existing-rule upgrades.

The PR must declare and run windows-wsl-mxc with validate-mxc-e2e.ps1 without -AllowSkip, plus real system.run approval and MCP discovery/invocation proof. The current none declaration is not valid for exec-approval behavior.

The red hosted lanes are not linked to this patch: Core failed an unrelated bounded-cancellation test, the E2E shards failed during shared setup initialization, and CI Gate is derivative. They do not replace the missing MXC proof.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P2 Normal priority bug or improvement with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants