Skip to content

feat(security): enforceable execution permissions for shell and filesystem access (#1450) - #1506

Open
B-A-M-N wants to merge 5 commits into
CodebuffAI:mainfrom
B-A-M-N:security/1450-execution-permissions
Open

B-A-M-N wants to merge 5 commits into
CodebuffAI:mainfrom
B-A-M-N:security/1450-execution-permissions

Conversation

@B-A-M-N

@B-A-M-N B-A-M-N commented Oct 12, 2026 •

Copy link
Copy Markdown

SDK execution policy foundation

Adds per-run execution permissions to the SDK. Each run captures its own policy, checks tool calls before dispatch, and guards filesystem reads and writes through the run's filesystem adapter.

The policy classifies the registered built-in tool set. Unknown tools remain fail-closed while restrictions are active. Dispatch authorization follows the resolved destination, so MCP calls that reuse built-in names and host overrides require external-tool trust. Trusted built-in overrides still retain built-in permission checks, and both direct read_files override paths check trust before calling the host. Spawned and nested agents inherit the parent's policy callback.

--read-only and --project-only deny unrestricted shell execution until an OS-enforced sandbox can enforce those limits. A separate follow-up adds validation for the nested target path in apply_patch.

PR branch and scope

  • PR: #1506
  • Base: main
  • PR head branch: security/1450-execution-permissions
  • Head: 3cf2370efebb951527139963ec3501d208f9003c
  • Equivalent stack branch: security/1450-sdk-core-review-r2
  • SDK commits: f823c975e policy model, 7c9b22fe7 SDK enforcement, f5516ba36 policy types, 55b23d6fe dispatch-origin authorization, and 3cf2370ef typed regression fixtures.
  • Diff against main: 7 files, +1,437 / -13.

Verification

The complete stacked suite passed 98 tests across 9 files. It includes 38 SDK policy, inheritance, and read-filter tests in this PR. The SDK typecheck reports existing provider metadata incompatibilities in untouched upstream files; no changed file reports a type error.

Inference for this contribution was provided by FreeInference.org. FreeInference did not review, direct, approve, or otherwise participate in this work; all implementation, testing, and publication decisions are the contributor's own. If you find this useful, consider donating to FreeInference.org — they provide a useful and vital service.

@codebuff-team

Copy link
Copy Markdown
Contributor

Good instincts here: moving enforcement out of the model and into the runtime is the right call, the no-global-policy-state design is correct for an embeddable SDK, the symlink-aware canonicalizePath/isPathWithinRoot and Windows-case-insensitive containment logic are well thought out, and you shipped real adversarial tests (execution-policy.test.ts, shell-approval-bridge.test.ts) instead of hand-waving. The apply_patch fix in run.ts (using operation.path) is a believable, narrow bug fix on its own.

Three things that need work before this is portable as-is:

  1. classifyTool() is a closed allowlist (execution-policy.ts). Any built-in tool not in that switch silently becomes external and gets denied the moment any restriction flag is set. That's a correctness landmine every time a new tool is added to the SDK — a maintainer has to remember to update this file or a legitimate tool quietly breaks under --read-only/--project-only/--ask-shell. This needs to be derived from (or validated against) the actual tool registry, not a hand-maintained list.

  2. --read-only/--project-only disable shell entirely, not just unrestricted writes/escapes. That's defensible as a stopgap, but it's a large behavior surprise for users who expect --read-only to still allow git diff/ls/cat via shell. Worth calling out explicitly as a known limitation or scoping shell commands instead of blanket-denying.

  3. Sub-agent spawning (spawn_agents/spawn_agent_inline) classifies as agent and is always allowed, and nothing in the diff shows the run's ExecutionPolicy being threaded into whatever spawns a sub-agent's own runOnce. If a spawned sub-agent doesn't inherit noShell/readOnly/projectOnly, this is a straightforward bypass of the entire feature. This needs to be confirmed and, if true, fixed before this can be called "enforced."

Given the size (+1337/-10) and that it rewrites a security-critical dispatch path in run.ts, I'd want this split and the sub-agent propagation question answered before porting.

@codebuff-team codebuff-team added bot:triaged Classified by the community triage bot pr:needs-work Right idea, not mergeable as written labels Oct 12, 2026

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

bot:triaged Classified by the community triage bot pr:needs-work Right idea, not mergeable as written

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants