fix(task): subagent inherits parent agent's deny rules (Plan Mode security bypass) (#26597)
Co-authored-by: Developer <temp@example.com>
This commit is contained in:
@@ -0,0 +1,33 @@
|
|||||||
|
import type { Permission } from "../permission"
|
||||||
|
import type { Agent } from "./agent"
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Build the `permission` ruleset for a subagent's session when it's spawned
|
||||||
|
* via the task tool. Combines:
|
||||||
|
*
|
||||||
|
* 1. The parent **agent's** deny rules — Plan Mode and other agent-level
|
||||||
|
* restrictions live on the agent ruleset, not on the session, so a
|
||||||
|
* subagent that only inherited the parent SESSION's permission would
|
||||||
|
* silently bypass them. (#26514)
|
||||||
|
* 2. The parent **session's** deny rules and external_directory rules —
|
||||||
|
* same forwarding the original code already did.
|
||||||
|
* 3. Default `todowrite` and `task` denies if the subagent's own ruleset
|
||||||
|
* doesn't already permit them.
|
||||||
|
*/
|
||||||
|
export function deriveSubagentSessionPermission(input: {
|
||||||
|
parentSessionPermission: Permission.Ruleset
|
||||||
|
parentAgent: Agent.Info | undefined
|
||||||
|
subagent: Agent.Info
|
||||||
|
}): Permission.Ruleset {
|
||||||
|
const canTask = input.subagent.permission.some((rule) => rule.permission === "task")
|
||||||
|
const canTodo = input.subagent.permission.some((rule) => rule.permission === "todowrite")
|
||||||
|
const parentAgentDenies = input.parentAgent?.permission.filter((rule) => rule.action === "deny") ?? []
|
||||||
|
return [
|
||||||
|
...parentAgentDenies,
|
||||||
|
...input.parentSessionPermission.filter(
|
||||||
|
(rule) => rule.permission === "external_directory" || rule.action === "deny",
|
||||||
|
),
|
||||||
|
...(canTodo ? [] : [{ permission: "todowrite" as const, pattern: "*" as const, action: "deny" as const }]),
|
||||||
|
...(canTask ? [] : [{ permission: "task" as const, pattern: "*" as const, action: "deny" as const }]),
|
||||||
|
]
|
||||||
|
}
|
||||||
@@ -4,6 +4,7 @@ import { Session } from "@/session/session"
|
|||||||
import { SessionID, MessageID } from "../session/schema"
|
import { SessionID, MessageID } from "../session/schema"
|
||||||
import { MessageV2 } from "../session/message-v2"
|
import { MessageV2 } from "../session/message-v2"
|
||||||
import { Agent } from "../agent/agent"
|
import { Agent } from "../agent/agent"
|
||||||
|
import { deriveSubagentSessionPermission } from "../agent/subagent-permissions"
|
||||||
import type { SessionPrompt } from "../session/prompt"
|
import type { SessionPrompt } from "../session/prompt"
|
||||||
import { Config } from "@/config/config"
|
import { Config } from "@/config/config"
|
||||||
import { Effect, Exit, Schema } from "effect"
|
import { Effect, Exit, Schema } from "effect"
|
||||||
@@ -58,41 +59,25 @@ export const TaskTool = Tool.define(
|
|||||||
return yield* Effect.fail(new Error(`Unknown agent type: ${params.subagent_type} is not a valid agent type`))
|
return yield* Effect.fail(new Error(`Unknown agent type: ${params.subagent_type} is not a valid agent type`))
|
||||||
}
|
}
|
||||||
|
|
||||||
const canTask = next.permission.some((rule) => rule.permission === id)
|
|
||||||
const canTodo = next.permission.some((rule) => rule.permission === "todowrite")
|
|
||||||
|
|
||||||
const taskID = params.task_id
|
const taskID = params.task_id
|
||||||
const session = taskID
|
const session = taskID
|
||||||
? yield* sessions.get(SessionID.make(taskID)).pipe(Effect.catchCause(() => Effect.succeed(undefined)))
|
? yield* sessions.get(SessionID.make(taskID)).pipe(Effect.catchCause(() => Effect.succeed(undefined)))
|
||||||
: undefined
|
: undefined
|
||||||
const parent = yield* sessions.get(ctx.sessionID)
|
const parent = yield* sessions.get(ctx.sessionID)
|
||||||
|
const parentAgent = parent.agent
|
||||||
|
? yield* agent.get(parent.agent).pipe(Effect.catchCause(() => Effect.succeed(undefined)))
|
||||||
|
: undefined
|
||||||
const nextSession =
|
const nextSession =
|
||||||
session ??
|
session ??
|
||||||
(yield* sessions.create({
|
(yield* sessions.create({
|
||||||
parentID: ctx.sessionID,
|
parentID: ctx.sessionID,
|
||||||
title: params.description + ` (@${next.name} subagent)`,
|
title: params.description + ` (@${next.name} subagent)`,
|
||||||
permission: [
|
permission: [
|
||||||
...(parent.permission ?? []).filter(
|
...deriveSubagentSessionPermission({
|
||||||
(rule) => rule.permission === "external_directory" || rule.action === "deny",
|
parentSessionPermission: parent.permission ?? [],
|
||||||
),
|
parentAgent,
|
||||||
...(canTodo
|
subagent: next,
|
||||||
? []
|
}),
|
||||||
: [
|
|
||||||
{
|
|
||||||
permission: "todowrite" as const,
|
|
||||||
pattern: "*" as const,
|
|
||||||
action: "deny" as const,
|
|
||||||
},
|
|
||||||
]),
|
|
||||||
...(canTask
|
|
||||||
? []
|
|
||||||
: [
|
|
||||||
{
|
|
||||||
permission: id,
|
|
||||||
pattern: "*" as const,
|
|
||||||
action: "deny" as const,
|
|
||||||
},
|
|
||||||
]),
|
|
||||||
...(cfg.experimental?.primary_tools?.map((item) => ({
|
...(cfg.experimental?.primary_tools?.map((item) => ({
|
||||||
pattern: "*",
|
pattern: "*",
|
||||||
action: "allow" as const,
|
action: "allow" as const,
|
||||||
@@ -144,8 +129,8 @@ export const TaskTool = Tool.define(
|
|||||||
},
|
},
|
||||||
agent: next.name,
|
agent: next.name,
|
||||||
tools: {
|
tools: {
|
||||||
...(canTodo ? {} : { todowrite: false }),
|
...(next.permission.some((rule) => rule.permission === "todowrite") ? {} : { todowrite: false }),
|
||||||
...(canTask ? {} : { task: false }),
|
...(next.permission.some((rule) => rule.permission === id) ? {} : { task: false }),
|
||||||
...Object.fromEntries((cfg.experimental?.primary_tools ?? []).map((item) => [item, false])),
|
...Object.fromEntries((cfg.experimental?.primary_tools ?? []).map((item) => [item, false])),
|
||||||
},
|
},
|
||||||
parts,
|
parts,
|
||||||
|
|||||||
@@ -0,0 +1,141 @@
|
|||||||
|
/**
|
||||||
|
* Reproducer for opencode issue #26514:
|
||||||
|
*
|
||||||
|
* In Plan Mode (the `plan` agent), the main agent's edit/write tools are
|
||||||
|
* blocked by the plan agent's permission ruleset (`edit: { "*": "deny" }`).
|
||||||
|
* However, when the plan agent spawns a subagent via the `task` tool, the
|
||||||
|
* subagent retains full file modification capabilities — a security bypass.
|
||||||
|
*
|
||||||
|
* This test replicates the permission ruleset that would govern a
|
||||||
|
* `general` subagent when launched from a `plan` parent session, mirroring
|
||||||
|
* the logic in `src/tool/task.ts` (filtered parent permissions ++ runtime
|
||||||
|
* subagent agent permissions, evaluated as in `session/prompt.ts`).
|
||||||
|
*
|
||||||
|
* The expected (secure) behavior is that the subagent inherits the plan
|
||||||
|
* mode read-only restriction and `edit`/`write` resolve to `deny`. On
|
||||||
|
* origin/dev this assertion fails because the parent **agent** permissions
|
||||||
|
* are not propagated to the subagent — only the parent **session**
|
||||||
|
* permissions are passed through, and Plan Mode's restrictions live on the
|
||||||
|
* agent, not the session.
|
||||||
|
*/
|
||||||
|
import { test, expect, afterEach } from "bun:test"
|
||||||
|
import { Effect } from "effect"
|
||||||
|
import { disposeAllInstances, provideInstance, tmpdir } from "../fixture/fixture"
|
||||||
|
import { WithInstance } from "../../src/project/with-instance"
|
||||||
|
import { Agent } from "../../src/agent/agent"
|
||||||
|
import { deriveSubagentSessionPermission } from "../../src/agent/subagent-permissions"
|
||||||
|
import { Permission } from "../../src/permission"
|
||||||
|
|
||||||
|
afterEach(async () => {
|
||||||
|
await disposeAllInstances()
|
||||||
|
})
|
||||||
|
|
||||||
|
function load<A>(dir: string, fn: (svc: Agent.Interface) => Effect.Effect<A>) {
|
||||||
|
return Effect.runPromise(provideInstance(dir)(Agent.Service.use(fn)).pipe(Effect.provide(Agent.defaultLayer)))
|
||||||
|
}
|
||||||
|
|
||||||
|
// `deriveSubagentSessionPermission` is imported from production. The test
|
||||||
|
// exercises the actual helper that task.ts uses to build the subagent's
|
||||||
|
// session permission, so any regression in that helper trips this test.
|
||||||
|
|
||||||
|
test("[#26514] subagent spawned from plan mode inherits read-only restriction (edit denied)", async () => {
|
||||||
|
await using tmp = await tmpdir()
|
||||||
|
await WithInstance.provide({
|
||||||
|
directory: tmp.path,
|
||||||
|
fn: async () => {
|
||||||
|
const planAgent = await load(tmp.path, (svc) => svc.get("plan"))
|
||||||
|
const generalAgent = await load(tmp.path, (svc) => svc.get("general"))
|
||||||
|
|
||||||
|
expect(planAgent).toBeDefined()
|
||||||
|
expect(generalAgent).toBeDefined()
|
||||||
|
// Sanity: the plan agent itself blocks edit. (Note: `write` and
|
||||||
|
// `apply_patch` route through the `edit` permission at the runtime
|
||||||
|
// tool layer — see Permission.disabled / EDIT_TOOLS.)
|
||||||
|
expect(Permission.evaluate("edit", "/some/file.ts", planAgent!.permission).action).toBe("deny")
|
||||||
|
|
||||||
|
// Simulate the plan-mode parent session: in real flow the plan
|
||||||
|
// session's `permission` field is empty (Plan Mode lives on the agent
|
||||||
|
// ruleset, not the session). So we pass [] through as the parent
|
||||||
|
// session permission, exactly like the actual code path.
|
||||||
|
const parentSessionPermission: Permission.Ruleset = []
|
||||||
|
|
||||||
|
const subagentSessionPermission = deriveSubagentSessionPermission({
|
||||||
|
parentSessionPermission,
|
||||||
|
parentAgent: planAgent,
|
||||||
|
subagent: generalAgent!,
|
||||||
|
})
|
||||||
|
|
||||||
|
// Mirror the runtime evaluation in session/prompt.ts (~line 410, 639):
|
||||||
|
// ruleset: Permission.merge(agent.permission, session.permission ?? [])
|
||||||
|
const effective = Permission.merge(generalAgent!.permission, subagentSessionPermission)
|
||||||
|
|
||||||
|
expect(Permission.evaluate("edit", "/some/file.ts", effective).action).toBe("deny")
|
||||||
|
expect(Permission.evaluate("edit", "/another/path/index.tsx", effective).action).toBe("deny")
|
||||||
|
},
|
||||||
|
})
|
||||||
|
})
|
||||||
|
|
||||||
|
test("[#26514] explore subagent launched from plan mode also stays read-only", async () => {
|
||||||
|
// Sibling check: even though `explore` is intrinsically read-only, the
|
||||||
|
// bug surface is the same. Including this case to document that the fix
|
||||||
|
// should propagate the parent **agent** permissions, not just deny edit
|
||||||
|
// when the subagent happens to already deny it.
|
||||||
|
await using tmp = await tmpdir()
|
||||||
|
await WithInstance.provide({
|
||||||
|
directory: tmp.path,
|
||||||
|
fn: async () => {
|
||||||
|
const planAgent = await load(tmp.path, (svc) => svc.get("plan"))
|
||||||
|
const explore = await load(tmp.path, (svc) => svc.get("explore"))
|
||||||
|
expect(planAgent).toBeDefined()
|
||||||
|
expect(explore).toBeDefined()
|
||||||
|
|
||||||
|
const parentSessionPermission: Permission.Ruleset = []
|
||||||
|
const subagentSessionPermission = deriveSubagentSessionPermission({
|
||||||
|
parentSessionPermission,
|
||||||
|
parentAgent: planAgent,
|
||||||
|
subagent: explore!,
|
||||||
|
})
|
||||||
|
const effective = Permission.merge(explore!.permission, subagentSessionPermission)
|
||||||
|
|
||||||
|
// Already deny — sanity check.
|
||||||
|
expect(Permission.evaluate("edit", "/x.ts", effective).action).toBe("deny")
|
||||||
|
},
|
||||||
|
})
|
||||||
|
})
|
||||||
|
|
||||||
|
test("[#26514] custom user subagent launched from plan mode bypasses Plan Mode read-only", async () => {
|
||||||
|
// The most damaging case: a user-defined subagent with default
|
||||||
|
// permissions (allow-by-default, like `general`). The subagent must NOT
|
||||||
|
// be able to edit when the parent agent is `plan`.
|
||||||
|
await using tmp = await tmpdir({
|
||||||
|
config: {
|
||||||
|
agent: {
|
||||||
|
my_subagent: {
|
||||||
|
description: "A user-defined subagent",
|
||||||
|
mode: "subagent",
|
||||||
|
},
|
||||||
|
},
|
||||||
|
},
|
||||||
|
})
|
||||||
|
await WithInstance.provide({
|
||||||
|
directory: tmp.path,
|
||||||
|
fn: async () => {
|
||||||
|
const planAgent = await load(tmp.path, (svc) => svc.get("plan"))
|
||||||
|
const my = await load(tmp.path, (svc) => svc.get("my_subagent"))
|
||||||
|
expect(planAgent).toBeDefined()
|
||||||
|
expect(my).toBeDefined()
|
||||||
|
|
||||||
|
const parentSessionPermission: Permission.Ruleset = []
|
||||||
|
const subagentSessionPermission = deriveSubagentSessionPermission({
|
||||||
|
parentSessionPermission,
|
||||||
|
parentAgent: planAgent,
|
||||||
|
subagent: my!,
|
||||||
|
})
|
||||||
|
const effective = Permission.merge(my!.permission, subagentSessionPermission)
|
||||||
|
|
||||||
|
// BUG: on origin/dev edit resolves to "allow" because the plan
|
||||||
|
// agent's `edit: deny *` rule never reaches the subagent.
|
||||||
|
expect(Permission.evaluate("edit", "/some/file.ts", effective).action).toBe("deny")
|
||||||
|
},
|
||||||
|
})
|
||||||
|
})
|
||||||
Reference in New Issue
Block a user