fix(cli): critical security fixes for subagent permissions, writeFile boundary, and policy bypass

[johnpippett]

Summary

Addresses three critical/high security findings from the 2026-04-28 AI Tooling Security Audit:

1. Subagent bypasses permissions (Critical) — Removed the hard-coded policies: [{ tool: "*", permission: "allow" }] override in extensions/cli/src/subagent/executor.ts so subagents now inherit the main agent's tool permission policies instead of unconditionally allowing all tools.

2. writeFile no boundary (High) — Added throwIfFileIsSecurityConcern checks to both the preprocess and run functions in extensions/cli/src/tools/writeFile.ts, aligning it with the security posture of readFile and edit tools.

3. Policy bypass (High) — Fixed extensions/cli/src/permissions/permissionChecker.ts so that dynamic tool policy evaluation (e.g., terminal command security analysis) is respected rather than being overridden by static user allow policies. The fix takes the most restrictive of the static policy and the dynamically evaluated policy, and gracefully handles invalid evaluator responses.

Test Results

• All existing CLI tests pass (1,848 passed, 29 skipped).
• permissionChecker.test.ts and permissionChecker.integration.test.ts updated and passing.

---

Summary by cubic

Fixes three critical/high security issues in the CLI: subagents now inherit tool permissions, writeFile enforces path safety checks, and dynamic tool policy evaluation can’t be bypassed.

• Bug Fixes
  • Subagent permissions: removed the allow-all override so subagents inherit the main agent’s tool policies.
  • writeFile boundary: added throwIfFileIsSecurityConcern in both preprocess and run to match readFile/edit posture.
  • Policy evaluation: permissionChecker now takes the most restrictive result between static policy and dynamic evaluation, and safely handles invalid evaluator outputs.
  • Tests: updated permissionChecker unit/integration tests; all CLI tests pass.

^{Written for commit 5b77e11. Summary will update on new commits. Review in cubic}

[github-actions]

Thank you for your submission, we really appreciate it. Like many open-source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution. You can sign the CLA by just posting a Pull Request Comment same as the below format.

---

I have read the CLA Document and I hereby sign the CLA

---

_{You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot.}

[cubic-dev-ai]

1 issue found across 4 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="extensions/cli/src/permissions/permissionChecker.ts">

<violation number="1" location="extensions/cli/src/permissions/permissionChecker.ts:200">
P1: Invalid dynamic policy fallback is fail-open (`basePermission`), which can bypass dynamic restrictions when evaluator output is unexpected.</violation>
</file>

_{Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.}

[cubic-dev-ai]

P1: Invalid dynamic policy fallback is fail-open (basePermission), which can bypass dynamic restrictions when evaluator output is unexpected.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At extensions/cli/src/permissions/permissionChecker.ts, line 200:

<comment>Invalid dynamic policy fallback is fail-open (`basePermission`), which can bypass dynamic restrictions when evaluator output is unexpected.</comment>

<file context>
@@ -158,17 +189,26 @@ export function checkToolPermission(
+    if (!validPolicies.includes(evaluatedPolicy)) {
       return {
-        permission: "exclude",
+        permission: basePermission,
         matchedPolicy,
       };
</file context>

Suggested change

	permission: basePermission,
	permission: getMostRestrictivePermissionPolicy(basePermission, "ask"),

[rpelevin]

This PR is directionally important because it fixes three places where permission policy can be bypassed below the user-facing intent.

The regression boundary I would want around it:

• subagent inherits parent tool policy unless an explicit narrower policy is supplied;
• dynamic evaluator can only tighten a static allow, never loosen a deny;
• writeFile, read, and edit share the same security-boundary check before side effects;
• invalid evaluator output fails closed;
• audit/log state distinguishes static_allow, dynamic_deny, boundary_deny, and inherited_policy_deny.

The subtle failure mode is "allow" becoming a universal default. For agent tools, allow should be scoped to exact tool/action/resource, and dynamic checks should run as a final pre-execution gate. If any layer cannot produce a valid decision, the tool should not run.

That turns the fix from a one-off patch into a permission invariant maintainers can preserve across future CLI/subagent changes.

[johnpippett]

Fixed in 84856a5. The permission checker now denies the tool call when the policy evaluator throws or returns an invalid value. Before, it kept the static permission. New tests cover undefined, null, empty, wrong-case and non-string output, and an evaluator that throws.

[johnpippett]

I am closing this pull request because I will not sign the CLA. The findings in the description stay available here if you want to fix them yourselves. Thank you for the review.