Skip to content

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

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

B-A-M-N wants to merge 7 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
canonicalizePath now walks up only for genuinely-missing paths (ENOENT/ENOTDIR) and rethrows permission errors and symlink loops. Replace the root-unsafe slice with path.basename. guardFileSystem refuses strict confinement when a custom adapter lacks a compatible canonicalizer. handleToolCall resolves file-path targets again and refuses an unverifiable mutating target under --project-only. Exempt the audited CLI ask_user UI override from the external-tool gate while keeping MCP/custom name collisions external. Document that projectOnly is not an OS boundary.

Tests cover root-boundary canonicalization, fail-closed realpath errors, the adapter refusal, and ask_user under all four permission modes.
Add a top-level regression that goes through CodebuffClient.run with a restricted policy, spawns a child through the real spawn_agents handler, and proves the child's inherited requestToolCall denies shell and writes. This is the end-to-end inheritance check requested in review, beyond the handler-level unit tests.
@B-A-M-N

B-A-M-N commented Oct 18, 2026

Copy link
Copy Markdown
Author

Re-review requested for dc9ee0bb3.

This revision keeps #1506 to the SDK execution-policy foundation and addresses the three portability concerns from the Oct 12 review.

1. classifyTool() closed allowlist — fixed.
BUILT_IN_TOOL_CAPABILITIES is now an exhaustive Record<ToolName, ToolCapability>, so adding a registered built-in without a policy classification is a compile error instead of silently becoming external. execution-policy.test.ts cross-checks it against the runtime tool registry (toolParams) and asserts both key sets equal the registered toolNames.

2. --read-only / --project-only disable shell — documented as a known limitation.
sdk/README.md now states explicitly that the SDK applies no OS sandbox and that an unrestricted shell could write outside the filesystem adapter or reach outside the project, so readOnly and projectOnly disable shell commands (including git status, git diff, ls, cat), with the rationale and the escape hatch (ask_user still works). Scoped shell commands instead of blanket denial remain a follow-up.

3. Sub-agent policy inheritance — confirmed and fixed.
The run's ExecutionPolicy (and its canonicalize hook) is threaded into the closure a spawned sub-agent receives for its own runOnce, so a child inherits noShell / readOnly / projectOnly. New regression in sdk/src/__tests__/subagent-execution-policy.test.ts: a top-level client.run({ noShell, projectOnly, readOnly }) spawns a child through the real spawn_agents handler and asserts the child's inherited requestToolCall returns --no-shell for run_terminal_command and --read-only for write_file.

Additional hardening in this head:

  • canonicalizePath() fails closed on any non-ENOENT realpath error and handles the / boundary with path.basename.
  • guardFileSystem() refuses projectOnly with a custom filesystem adapter unless a matching canonicalize hook is supplied, rather than silently weakening confinement.
  • handleToolCall() refuses mutating tools whose target cannot be resolved under --project-only (no fallback to the project root) and resolves filePaths targets.
  • ask_user is allowed under all four modes via a narrow TRUSTED_UI_OVERRIDE_TOOLS exemption; MCP/custom tools reusing the name stay external. No global allowExternalTools.

Split. The apply_patch target validation and the CLI integration are now dependent drafts (#1530, #1531), keeping this PR to the SDK foundation.

Local evidence: 56 pass / 0 fail across the affected SDK suites; changed-file typecheck clean.

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