feat(tools): shell tool security hardening by blib · Pull Request #1097 · sipeed/picoclaw

blib · 2026-03-04T14:27:28Z

📝 Description

Replace the regex-based deny-pattern shell executor with an AST-based risk classifier built on an in-process POSIX interpreter (mvdan.cc/sh/v3).

The old approach used os/exec with regex deny lists — brittle, bypassable via shell metacharacters, and platform-specific (separate _unix / _windows files). The new design parses every command into an AST before execution, classifying each resolved binary against a four-tier risk table (low → medium → high → critical). Commands above the configured threshold are blocked before any process is spawned.

Key changes:

Risk classifier (risk.go): static command table + argument-aware modifiers (e.g. rm is high, rm -rf is critical). User-configurable overrides and extra modifiers via config.
Environment sanitization (env.go): only allowlisted env vars propagated; explicit env_set support.
File-access sandbox (sandbox.go): OpenHandler restricts shell redirections to workspace directory, with symlink-escape prevention.
Cron integration (cron_exec_test.go): scheduled commands now go through the same security path (AC-8).
Config migration: old enable_deny_patterns / custom_deny_patterns fields deprecated with warnings; new risk_threshold, risk_overrides, arg_modifiers, env_allowlist, env_set.

Design documented in DDR (Design Decision Record): docs/design/DDR-shell-tool-hardening.md

🗣️ Type of Change

🐞 Bug fix (non-breaking change which fixes an issue)
✨ New feature (non-breaking change which adds functionality)
📖 Documentation update
⚡ Code refactoring (no functional changes, no api changes)

🤖 AI Code Generation

🤖 Fully AI-generated (100% AI, 0% Human)
🛠️ Mostly AI-generated (AI draft, Human verified/modified)
👨‍💻 Mostly Human-written (Human lead, AI assisted or none)

🔗 Related Issue

Supersedes: #820

Addresses shell tool security hardening — see docs/design/DDR-shell-tool-hardening.md for full threat model and acceptance criteria.

📚 Technical Context (Skip for Docs)

Reference URL: DDR-shell-tool-hardening.md
Reasoning: The previous regex deny-list approach was bypassable via variable indirection (a=sudo; $a rm -rf /), command substitution, and shell metacharacters. Moving to an in-process AST interpreter (mvdan.cc/sh/v3) lets us classify every resolved binary at execution time, eliminating regex evasion entirely. The four-tier risk model replaces the binary allow/deny with graduated control, and env sanitization prevents credential leakage.

🧪 Test Environment

Hardware: MacBook (Apple Silicon)
OS: macOS
Model/Provider: N/A (unit/integration tests only, no LLM calls)
Channels: N/A

☑️ Checklist

My code/docs follow the style of this project.
I have performed a self-review of my own changes.
I have updated the documentation accordingly.

Copilot

Pull request overview

This PR hardens the exec tool by replacing the regex/os/exec-based shell executor with an in-process mvdan.cc/sh/v3 interpreter plus AST/runtime interception to classify and block risky commands, sanitize environment variables, and restrict shell redirection file access to the workspace. It also updates configuration/docs and ensures cron-executed commands use the same security path.

Changes:

Introduces pkg/tools/shell/ runner with risk classification, env allowlisting, and redirection sandboxing; adapts ExecTool to use it (including async/background support).
Deprecates legacy deny/allow regex config fields and adds new config knobs (risk_threshold, risk_overrides, arg_modifiers, env_allowlist, env_set) with updated defaults and docs.
Adds/updates tests for runner, sandbox, env sanitization, async exec behavior, and cron integration.

Reviewed changes

Copilot reviewed 22 out of 23 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
pkg/tools/shell_tool.go	New `ExecTool` adapter using the in-process shell runner + async/background support + config wiring.
pkg/tools/shell_tool_test.go	Tests sync/async execution and callback behavior.
pkg/tools/cron_exec_test.go	Ensures scheduled (cron) commands go through the same blocking path.
pkg/tools/shell/runner.go	New interpreter-backed execution pipeline with `ExecHandlers`, timeout handling, and output capture/truncation.
pkg/tools/shell/runner_test.go	End-to-end runner tests (blocking, timeout, env, pipelines, etc.).
pkg/tools/shell/risk.go	Risk levels, command table, arg modifiers, overrides, and blocked-command error formatting.
pkg/tools/shell/risk_test.go	Unit tests for base table, modifiers, overrides, parsing, and matching helpers.
pkg/tools/shell/env.go	Builds sanitized environment (default allowlist + prefixes + env_set override).
pkg/tools/shell/env_test.go	Verifies secret filtering, allowlist extension, and env_set behavior.
pkg/tools/shell/sandbox.go	`OpenHandler` implementation restricting redirections to workspace (with symlink-escape prevention + safe paths).
pkg/tools/shell/sandbox_test.go	Tests workspace allow/deny, safe paths, symlink escape, and dotted-name regression.
pkg/config/config.go	Adds new exec config fields and keeps old ones as deprecated/ignored.
pkg/config/defaults.go	Updates default exec config to use `risk_threshold: "medium"`.
docs/tools_configuration.md	Documents new exec tool configuration + behavior model.
docs/design/DDR-shell-tool-hardening.md	Design Decision Record documenting threat model/approach/migration.
config/config.example.json	Updates example config to new exec tool fields.
pkg/tools/shell.go	Removes legacy regex-based executor implementation.
pkg/tools/shell_test.go	Removes legacy exec tool tests (superseded by new runner/tool tests).
pkg/tools/shell_timeout_unix_test.go	Removes legacy process-group timeout test (needs replacement coverage under new runner).
pkg/tools/shell_process_unix.go	Removes legacy Unix process-group termination helpers.
pkg/tools/shell_process_windows.go	Removes legacy Windows process termination helpers.
go.mod	Adds `mvdan.cc/sh/v3` dependency (and updates module requirements).
go.sum	Updates dependency checksums for new modules/transitives.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 22 out of 23 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 24 out of 25 changed files in this pull request and generated 6 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 27 out of 28 changed files in this pull request and generated 4 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

keithy · 2026-03-05T02:42:44Z

Does this fix the fact that skills scripts get the entire environment including all the secrets API keys etc.

blib · 2026-03-05T07:56:35Z

Does this fix the fact that skills scripts get the entire environment including all the secrets API keys etc.

if it is executed with same shell tool yes.
but! let me see what is going on with MCP servers! Good catch!

Copilot

Pull request overview

Copilot reviewed 27 out of 28 changed files in this pull request and generated 5 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

CLAassistant · 2026-03-05T09:46:40Z

All committers have signed the CLA.

Copilot

Pull request overview

Copilot reviewed 30 out of 31 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 29 out of 30 changed files in this pull request and generated 5 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 29 out of 30 changed files in this pull request and generated 4 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@blib

The async callback created in the agent loop was only logging and doing nothing, forcing SubagentManager to import bus and deliver results directly. This moves result delivery responsibility to the loop side where it belongs, and removes the bus dependency from SubagentManager. Ref: sipeed#1070 (comment by @blib), sipeed#1097 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

@blib

The async callback created in the agent loop was only logging and doing nothing, forcing SubagentManager to import bus and deliver results directly. This moves result delivery responsibility to the loop side where it belongs, and removes the bus dependency from SubagentManager. Ref: sipeed#1070 (comment by @blib), sipeed#1097 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Replace regex deny-patterns with in-process POSIX interpreter (mvdan.cc/sh/v3) and four-tier risk classification. Add env sanitization, file-access sandboxing, and argument-aware risk modifiers. Cron commands now share the same security path. Signed-off-by: Boris Bliznioukov <blib@mail.com>

… sandboxing for shell commands Signed-off-by: Boris Bliznioukov <blib@mail.com>

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Signed-off-by: Boris Bliznioukov <blib@mail.com>

…ests for path handling Signed-off-by: Boris Bliznioukov <blib@mail.com>

…commands; add Windows-specific tests and deprecate old config fields Signed-off-by: Boris Bliznioukov <blib@mail.com>

Signed-off-by: Boris Bliznioukov <blib@mail.com>

…t import Signed-off-by: Boris Bliznioukov <blib@mail.com>

…round delivery Signed-off-by: Boris Bliznioukov <blib@mail.com>

…llback handling Signed-off-by: Boris Bliznioukov <blib@mail.com>

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

…ling Signed-off-by: Boris Bliznioukov <blib@mail.com>

Signed-off-by: Boris Bliznioukov <blib@mail.com>

…mand_DeepPath for clarity Signed-off-by: Boris Bliznioukov <blib@mail.com>

Copilot

Pull request overview

Copilot reviewed 29 out of 30 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

…ce risk classification Signed-off-by: Boris Bliznioukov <blib@mail.com>

keithy · 2026-03-09T00:14:27Z

I am extracting the environment sanitisation into a separate PR, breaking things down into smaller more digestable chunks.

sipeed-bot · 2026-03-25T11:52:50Z

@blib Hi! This PR has had no activity for over 2 weeks, so I'm closing it for now to keep things organized. Feel free to reopen anytime if you'd like to continue.

Copilot AI review requested due to automatic review settings March 4, 2026 14:27

Copilot started reviewing on behalf of blib March 4, 2026 14:27 View session

sipeed-bot bot added type: enhancement New feature or request domain: tool domain: config labels Mar 4, 2026

Copilot AI reviewed Mar 4, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings March 4, 2026 14:45

Copilot started reviewing on behalf of blib March 4, 2026 14:46 View session

Copilot AI reviewed Mar 4, 2026

View reviewed changes

Comment thread pkg/tools/shell/risk.go

Comment thread pkg/tools/shell/runner.go

Comment thread pkg/tools/shell/runner.go

Copilot AI review requested due to automatic review settings March 4, 2026 16:40

Copilot started reviewing on behalf of blib March 4, 2026 16:41 View session

Copilot AI reviewed Mar 4, 2026

View reviewed changes

Comment thread pkg/tools/shell/env.go

Comment thread pkg/tools/shell/runner.go Outdated

Comment thread pkg/tools/shell/sandbox_test.go

Comment thread pkg/tools/shell/runner_test.go

Comment thread pkg/tools/shell_tool.go

Comment thread docs/tools_configuration.md Outdated

Copilot AI review requested due to automatic review settings March 4, 2026 20:36

Copilot started reviewing on behalf of blib March 4, 2026 20:36 View session

Copilot AI reviewed Mar 4, 2026

View reviewed changes

Comment thread pkg/tools/shell_tool_test.go Outdated

Comment thread pkg/tools/shell_tool_test.go Outdated

Comment thread pkg/migrate/sources/openclaw/openclaw_config.go

Comment thread pkg/tools/shell/runner_test.go

github-actions bot mentioned this pull request Mar 5, 2026

🦞 OpenClaw 生态日报 2026-03-05 duanyytop/agents-radar#77

Open

Copilot AI review requested due to automatic review settings March 5, 2026 08:03

Copilot started reviewing on behalf of blib March 5, 2026 08:03 View session

Copilot AI reviewed Mar 5, 2026

View reviewed changes

Comment thread pkg/config/config.go

Comment thread pkg/config/config.go Outdated

Comment thread pkg/config/defaults.go

Comment thread config/config.example.json

Comment thread pkg/tools/shell/sandbox.go Outdated

Copilot AI review requested due to automatic review settings March 5, 2026 09:47

Copilot started reviewing on behalf of blib March 5, 2026 09:48 View session

Copilot AI reviewed Mar 5, 2026

View reviewed changes

Comment thread pkg/tools/shell_tool.go Outdated

Comment thread pkg/tools/shell/runner.go Outdated

Comment thread pkg/tools/shell/runner_windows_test.go

blib mentioned this pull request Mar 5, 2026

feat: execute LLM tool calls in parallel for faster response #1070

Merged

10 tasks

Copilot AI review requested due to automatic review settings March 5, 2026 21:19

Copilot started reviewing on behalf of blib March 5, 2026 21:19 View session

Copilot AI reviewed Mar 5, 2026

View reviewed changes

Comment thread docs/tools_configuration.md

Comment thread docs/design/DDR-shell-tool-hardening.md Outdated

Comment thread pkg/tools/shell_tool_test.go

Comment thread pkg/tools/shell_tool.go Outdated

Comment thread docs/tools_configuration.md

Copilot AI review requested due to automatic review settings March 5, 2026 22:22

Copilot started reviewing on behalf of blib March 5, 2026 22:22 View session

Copilot AI reviewed Mar 5, 2026

View reviewed changes

Comment thread pkg/tools/shell_tool.go

Comment thread docs/tools_configuration.md

Comment thread pkg/tools/shell/risk_test.go Outdated

Comment thread pkg/tools/shell/sandbox.go

github-actions bot mentioned this pull request Mar 6, 2026

🦞 OpenClaw 生态日报 2026-03-06 duanyytop/agents-radar#83

Open

blib and others added 13 commits March 6, 2026 16:42

feat(security): fix reviewer comments enhance risk classification and…

54eb65a

… sandboxing for shell commands Signed-off-by: Boris Bliznioukov <blib@mail.com>

Update docs/tools_configuration.md

36f6221

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

feat(tests): format shell array for improved readability in risk tests

cdb691b

Signed-off-by: Boris Bliznioukov <blib@mail.com>

feat(security): enhance Windows command risk classification and add t…

08def76

…ests for path handling Signed-off-by: Boris Bliznioukov <blib@mail.com>

feat(security): enhance sandboxing and risk classification for shell …

de42aa5

…commands; add Windows-specific tests and deprecate old config fields Signed-off-by: Boris Bliznioukov <blib@mail.com>

fix(tests): linter error

328e489

Signed-off-by: Boris Bliznioukov <blib@mail.com>

refactor(tests): clean up captureStdout function and remove unused fm…

4bb4200

…t import Signed-off-by: Boris Bliznioukov <blib@mail.com>

feat(exec): implement AsyncExecutor for ExecTool with bus-based backg…

bf1b07d

…round delivery Signed-off-by: Boris Bliznioukov <blib@mail.com>

feat(exec): refactor ExecTool to remove bus dependency and improve ca…

970a568

…llback handling Signed-off-by: Boris Bliznioukov <blib@mail.com>

Update pkg/config/defaults.go

c1e33f6

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

feat(exec): enable execution in config and update async callback hand…

6c28061

…ling Signed-off-by: Boris Bliznioukov <blib@mail.com>

feat(exec): include user content in ExecuteAsync callback response

88da084

Signed-off-by: Boris Bliznioukov <blib@mail.com>

blib force-pushed the feat/shell-security branch from f8e0da6 to 88da084 Compare March 6, 2026 15:43

fix: add missing log import in agent/instance.go

f0c6727

Copilot AI review requested due to automatic review settings March 6, 2026 15:45

Copilot started reviewing on behalf of blib March 6, 2026 15:46 View session

refactor: rename TestClassifyCommand_BackslashPath to TestClassifyCom…

1a28887

…mand_DeepPath for clarity Signed-off-by: Boris Bliznioukov <blib@mail.com>

Copilot AI reviewed Mar 6, 2026

View reviewed changes

Comment thread pkg/tools/shell/risk.go Outdated

feat(risk): add argument profiles for command normalization and enhan…

cea543c

…ce risk classification Signed-off-by: Boris Bliznioukov <blib@mail.com>

github-actions bot mentioned this pull request Mar 9, 2026

🦞 OpenClaw 生态日报 2026-03-09 rollysys/agents-radar#58

Open

keithy mentioned this pull request Mar 9, 2026

feat(security): add environment sanitization for exec tool #1261

Open

sipeed-bot bot closed this Mar 25, 2026

Conversation

blib commented Mar 4, 2026

📝 Description

🗣️ Type of Change

🤖 AI Code Generation

🔗 Related Issue

📚 Technical Context (Skip for Docs)

🧪 Test Environment

☑️ Checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

keithy commented Mar 5, 2026

Uh oh!

blib commented Mar 5, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

CLAassistant commented Mar 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

CLAassistant commented Mar 5, 2026 •

edited

Loading