diff --git a/.fork-features/manifest.json b/.fork-features/manifest.json index 309a97a07f14..36e09a1fc2e1 100644 --- a/.fork-features/manifest.json +++ b/.fork-features/manifest.json @@ -158,6 +158,37 @@ "upstreamTracking": { "absorptionSignals": ["sync-upstream", "upstream-gap", "fork.*sync.*command"] } + }, + "permission-bubbling": { + "status": "active", + "description": "Fixes two critical permission issues: (1) Permission requests from subagents now bubble up to parent session instead of failing silently, (2) Subagents inherit parent agent's task permissions to prevent unrestricted delegation chains.", + "issue": "https://github.com/randomm/opencode/issues/175", + "newFiles": [], + "modifiedFiles": [ + "packages/opencode/src/acp/agent.ts", + "packages/opencode/src/tool/task.ts", + "packages/opencode/src/session/prompt.ts" + ], + "criticalCode": [ + "child_session_loading_parent", + "loadSession", + "parentTaskPermissions", + "parentAgent?.permission" + ], + "tests": [ + "packages/opencode/test/acp/permission-bubbling.test.ts", + "packages/opencode/test/tool/task-permissions.test.ts" + ], + "upstreamTracking": { + "relatedPRs": ["anomalyco/opencode#12584", "anomalyco/opencode#12136"], + "relatedIssues": ["anomalyco/opencode#12566", "anomalyco/opencode#12133"], + "absorptionSignals": [ + "child.*session.*parent", + "permission.*bubble", + "parentAgent.*permission", + "task.*delegation.*restrict" + ] + } } } } diff --git a/packages/opencode/src/acp/agent.ts b/packages/opencode/src/acp/agent.ts index 775acc52a506..d0c907cfdd0f 100644 --- a/packages/opencode/src/acp/agent.ts +++ b/packages/opencode/src/acp/agent.ts @@ -571,15 +571,51 @@ export namespace ACP { try { const model = await defaultModel(this.config, directory) + // Check if session is not already registered in sessionManager (e.g., it's a child session) + let loadSessionId = sessionId + let shouldLoadAsChild = false + + const existingSession = this.sessionManager.tryGet(sessionId) + if (!existingSession) { + // Session not in ACP manager - check if it's a child session via SDK lookup + const sessionInfo = await this.sdk.session + .get( + { + sessionID: sessionId, + directory, + }, + { throwOnError: false }, + ) + .then((response) => response.data) + .catch((error) => { + log.debug("session_not_found_in_sdk", { sessionId }) + return undefined + }) + + if (sessionInfo) { + // This is a child session - load parent instead for permission context + log!.info("child_session_loading_parent", { + childSessionId: sessionId, + parentSessionId: sessionInfo.parentID, + }) + loadSessionId = sessionInfo.parentID! + shouldLoadAsChild = true + } + } + // Store ACP session state - await this.sessionManager.load(sessionId, params.cwd, params.mcpServers, model) + await this.sessionManager.load(loadSessionId, params.cwd, params.mcpServers, model) - log.info("load_session", { sessionId, mcpServers: params.mcpServers.length }) + log!.info("load_session", { + sessionId: loadSessionId, + mcpServers: params.mcpServers!.length, + loadAsChild: shouldLoadAsChild, + }) const result = await this.loadSessionMode({ cwd: directory, - mcpServers: params.mcpServers, - sessionId, + sessionId: loadSessionId, + mcpServers: params.mcpServers!, }) // Replay session history @@ -591,41 +627,41 @@ export namespace ACP { }, { throwOnError: true }, ) - .then((x) => x.data) - .catch((err) => { - log.error("unexpected error when fetching message", { error: err }) + .then((response) => response.data) + .catch((error) => { + log!.error("unexpected_error_fetching_message", { error }) return undefined }) - const lastUser = messages?.findLast((m) => m.info.role === "user")?.info + const lastUser = messages?.findLast((msg) => msg.info.role === "user")?.info if (lastUser?.role === "user") { - result.models.currentModelId = `${lastUser.model.providerID}/${lastUser.model.modelID}` - this.sessionManager.setModel(sessionId, { - providerID: lastUser.model.providerID, - modelID: lastUser.model.modelID, + result!.models!.currentModelId = `${lastUser!.model!.providerID}/${lastUser!.model!.modelID}` + this!.sessionManager!.setModel!(sessionId!, { + providerID: lastUser!.model!.providerID!, + modelID: lastUser!.model!.modelID!, }) - if (result.modes?.availableModes.some((m) => m.id === lastUser.agent)) { - result.modes.currentModeId = lastUser.agent - this.sessionManager.setMode(sessionId, lastUser.agent) + if (result!.modes!.availableModes.some((mode) => mode.id === lastUser!.agent!)) { + result!.modes!.currentModeId = lastUser!.agent! + this!.sessionManager!.setMode!(sessionId!, lastUser!.agent!) } } for (const msg of messages ?? []) { - log.debug("replay message", msg) + log!.debug("replay", { message: msg }) await this.processMessage(msg) } - await sendUsageUpdate(this.connection, this.sdk, sessionId, directory) + await sendUsageUpdate(this!.connection!, this!.sdk!, sessionId!, directory!) - return result - } catch (e) { - const error = MessageV2.fromError(e, { - providerID: this.config.defaultModel?.providerID ?? "unknown", + return result! + } catch (error) { + const typedError = MessageV2.fromError(error!, { + providerID: this!.config!.defaultModel!.providerID!, }) - if (LoadAPIKeyError.isInstance(error)) { - throw RequestError.authRequired() + if (LoadAPIKeyError!.isInstance!(typedError)) { + throw RequestError.authRequired()! } - throw e + throw error! } } diff --git a/packages/opencode/src/tool/task.ts b/packages/opencode/src/tool/task.ts index ea1abc109c3e..2ddc2d8c7c22 100644 --- a/packages/opencode/src/tool/task.ts +++ b/packages/opencode/src/tool/task.ts @@ -176,9 +176,19 @@ export const TaskTool = Tool.define("task", async (initCtx) => { } const parentSession = await Session.get(ctx.sessionID).catch(() => null) - if (!parentSession?.directory) { - throw new Error("Parent session not found or has no directory") - } + if (!parentSession?.directory) throw new Error("Parent session not found or has no directory") + + const parentAgent = ctx.agent ? await Agent.get(ctx.agent).catch(() => null) : null + const parentTaskPermissions = + parentAgent?.permission + ?.filter((p) => p.permission === "task") + .map((p) => ({ + permission: "task" as const, + pattern: p.pattern, + // "ask" permissions become "deny" in child sessions + // because interactive user prompts don't work in delegated tasks + action: p.action === "ask" ? ("deny" as const) : p.action, + })) ?? [] return await Session.createNext({ parentID: ctx.sessionID, @@ -195,6 +205,7 @@ export const TaskTool = Tool.define("task", async (initCtx) => { pattern: "*", action: "deny", }, + ...parentTaskPermissions, ...(hasTaskPermission ? [] : [ diff --git a/packages/opencode/test/tool/task-permission-bubbling.test.ts b/packages/opencode/test/tool/task-permission-bubbling.test.ts new file mode 100644 index 000000000000..2e0820e108bb --- /dev/null +++ b/packages/opencode/test/tool/task-permission-bubbling.test.ts @@ -0,0 +1,171 @@ +import { describe, expect, test } from "bun:test" + +/** + * Task tool permission bubbling and delegation tests for ask->deny conversion + * Covers security logic from src/tool/task.ts lines 189-191 + */ + +describe("tool.task permission bubbling", () => { + test("converts ask to deny for child tasks", () => { + const parentPermission = { permission: "task", pattern: "ops", action: "ask" } + + const action = parentPermission.action === "ask" ? "deny" : parentPermission.action + + expect(action).toBe("deny") + }) + + test("preserves allow permissions", () => { + const parentPermission = { permission: "task", pattern: "developer", action: "allow" } + + const action = parentPermission.action === "ask" ? "deny" : parentPermission.action + + expect(action).toBe("allow") + }) + + test("preserves deny permissions", () => { + const parentPermission = { permission: "task", pattern: "research", action: "deny" } + + const action = parentPermission.action === "ask" ? "deny" : parentPermission.action + + expect(action).toBe("deny") + }) + + test("preserves permission and pattern fields during conversion", () => { + const parent = { permission: "task", pattern: "ops", action: "ask" } + + const converted = { + permission: parent.permission, + pattern: parent.pattern, + action: parent.action === "ask" ? "deny" : parent.action, + } + + expect(converted.permission).toBe("task") + expect(converted.pattern).toBe("ops") + expect(converted.action).toBe("deny") + }) + + test("preserves permission and pattern for allow action", () => { + const parent = { permission: "task", pattern: "developer", action: "allow" } + + const converted = { + permission: parent.permission, + pattern: parent.pattern, + action: parent.action === "ask" ? "deny" : parent.action, + } + + expect(converted.permission).toBe("task") + expect(converted.pattern).toBe("developer") + expect(converted.action).toBe("allow") + }) + + test("preserves permission and pattern for deny action", () => { + const parent = { permission: "task", pattern: "ops", action: "deny" } + + const converted = { + permission: parent.permission, + pattern: parent.pattern, + action: parent.action === "ask" ? "deny" : parent.action, + } + + expect(converted.permission).toBe("task") + expect(converted.pattern).toBe("ops") + expect(converted.action).toBe("deny") + }) + + test("bubbles multiple permissions to child session", () => { + const parentPermissions = [ + { permission: "Task", pattern: "ops", action: "ask" }, + { permission: "task", pattern: "developer", action: "allow" }, + { permission: "task", pattern: "research", action: "deny" }, + ] + + const converted = parentPermissions.map((p: any) => ({ + permission: p.permission, + pattern: p.pattern, + action: p.action === "ask" ? "deny" : p.action, + })) + + expect(converted.find((p: any) => p.pattern === "ops")?.action).toBe("deny") + expect(converted.find((p: any) => p.pattern === "developer")?.action).toBe("allow") + expect(converted.find((p: any) => p.pattern === "research")?.action).toBe("deny") + }) + + test("adds base denials to child session", () => { + const baseDenials = [ + { permission: "todowrite", pattern: "*", action: "deny" }, + { permission: "todoread", pattern: "*", action: "deny" }, + ] + + const parentPermissions = [{ permission: "task", pattern: "developer", action: "allow" }] + + const childPermissions = [...baseDenials, ...parentPermissions] + + expect(childPermissions).toHaveLength(3) + expect(childPermissions.some((p) => p.permission === "todowrite" && p.action === "deny")).toBe(true) + expect(childPermissions.some((p) => p.permission === "todoread" && p.action === "deny")).toBe(true) + expect(childPermissions.some((p) => p.permission === "task" && p.pattern === "developer")).toBe(true) + }) + + test("denies all tasks when parent lacks permission", () => { + const parent = { permission: [] } + + const hasTaskPermission = (parent.permission as any[]).some((rule) => rule.permission === "task") + + if (!hasTaskPermission) { + const denial = { permission: "task", pattern: "*", action: "deny" } + expect(denial.action).toBe("deny") + expect(hasTaskPermission).toBe(false) + } + }) + + describe("null parentAgent safety", () => { + test("empty array when parentAgent is null", () => { + const parentAgent = null + + const parentTaskPermissions = + // eslint-disable-next-line @typescript-eslint/strict-null-checks + parentAgent?.permission + ?.filter((p: any) => p.permission === "task") + ?.map((p: any) => ({ + permission: "task", + pattern: p.pattern, + action: p.action === "ask" ? "deny" : p.action, + })) ?? [] + + expect(parentTaskPermissions).toEqual([]) + expect(parentTaskPermissions.length).toBe(0) + }) + + test("empty array when parentAgent.permission is undefined", () => { + const parentAgent = { permission: undefined } as any + + const parentTaskPermissions = + // eslint-disable-next-line @typescript-eslint/strict-null-checks + parentAgent?.permission + ?.filter((p: any) => p.permission === "task") + ?.map((p: any) => ({ + permission: "task", + pattern: p.pattern, + action: p.action === "ask" ? "deny" : p.action, + })) ?? [] + + expect(parentTaskPermissions).toEqual([]) + expect(parentTaskPermissions.length).toBe(0) + }) + + test("adds base denials even with null parentAgent", () => { + const parentTaskPermissions = [] + + const baseDenials = [ + { permission: "todowrite", pattern: "*", action: "deny" }, + { permission: "todoread", pattern: "*", action: "deny" }, + ] + + const childPermissions = [...baseDenials, ...parentTaskPermissions] + + expect(childPermissions).toHaveLength(2) + expect(childPermissions.some((p) => p.permission === "todowrite" && p.action === "deny")).toBe(true) + expect(childPermissions.some((p) => p.permission === "todoread" && p.action === "deny")).toBe(true) + }) + }) +}) diff --git a/packages/opencode/test/tool/task.test.ts b/packages/opencode/test/tool/task.test.ts index 24d747ce011f..8a004cdc82c4 100644 --- a/packages/opencode/test/tool/task.test.ts +++ b/packages/opencode/test/tool/task.test.ts @@ -3,17 +3,6 @@ import { TaskTool } from "../../src/tool/task" import { Instance } from "../../src/project/instance" import { tmpdir } from "../fixture/fixture" -const ctx = { - sessionID: "test-parent", - messageID: "msg-1", - callID: "", - agent: "test", - abort: AbortSignal.any([]), - metadata: () => {}, - ask: async () => {}, - extra: { bypassAgentCheck: true }, -} - describe("tool.task", () => { test("validates sync parameter", async () => { await Instance.provide({