From f0fa213cd3037ee9105388b67ee7331cb6ccf598 Mon Sep 17 00:00:00 2001 From: Aiden Cline Date: Tue, 18 Nov 2025 16:31:16 -0600 Subject: [PATCH 1/3] fix: permission checks not enforcing deny settings --- .opencode/opencode.jsonc | 3 + packages/opencode/src/agent/agent.ts | 4 +- packages/opencode/src/session/processor.ts | 10 +- packages/opencode/src/tool/bash.ts | 15 +- packages/opencode/src/tool/edit.ts | 24 +- packages/opencode/src/tool/patch.ts | 16 +- packages/opencode/src/tool/read.ts | 8 +- packages/opencode/src/tool/write.ts | 17 +- packages/opencode/test/tool/bash.test.ts | 196 +++++++++++++++- packages/opencode/test/tool/edit.test.ts | 253 +++++++++++++++++++++ packages/opencode/test/tool/patch.test.ts | 191 +++++++++++++++- 11 files changed, 708 insertions(+), 29 deletions(-) create mode 100644 packages/opencode/test/tool/edit.test.ts diff --git a/.opencode/opencode.jsonc b/.opencode/opencode.jsonc index 2a4558e4288..6e3a5ea744f 100644 --- a/.opencode/opencode.jsonc +++ b/.opencode/opencode.jsonc @@ -8,4 +8,7 @@ }, }, }, + "permission": { + "external_directory": "ask" + } } diff --git a/packages/opencode/src/agent/agent.ts b/packages/opencode/src/agent/agent.ts index 740f67b7e04..dfbd0393b13 100644 --- a/packages/opencode/src/agent/agent.ts +++ b/packages/opencode/src/agent/agent.ts @@ -250,8 +250,8 @@ function mergeAgentPermissions(basePermission: any, overridePermission: any): Ag edit: merged.edit ?? "allow", webfetch: merged.webfetch ?? "allow", bash: mergedBash ?? { "*": "allow" }, - doom_loop: merged.doom_loop, - external_directory: merged.external_directory, + doom_loop: merged.doom_loop ?? "ask", + external_directory: merged.external_directory ?? "ask", } return result diff --git a/packages/opencode/src/session/processor.ts b/packages/opencode/src/session/processor.ts index 5c2005587b2..97ce98cb538 100644 --- a/packages/opencode/src/session/processor.ts +++ b/packages/opencode/src/session/processor.ts @@ -147,7 +147,10 @@ export namespace SessionProcessor { ) ) { const permission = await Agent.get(input.assistantMessage.mode).then((x) => x.permission) - if (permission.doom_loop === "ask") { + // Secure default: only allow if explicitly set to "allow" or "ask" + if (permission.doom_loop === "allow") { + // Explicitly allowed, proceed + } else if (permission.doom_loop === "ask") { await Permission.ask({ type: "doom_loop", pattern: value.toolName, @@ -160,6 +163,11 @@ export namespace SessionProcessor { input: value.input, }, }) + } else { + // Default deny for "deny", undefined, null, or any other value + throw new Error( + `Permission denied: Possible doom loop detected - "${value.toolName}" called ${DOOM_LOOP_THRESHOLD} times with identical arguments`, + ) } } } diff --git a/packages/opencode/src/tool/bash.ts b/packages/opencode/src/tool/bash.ts index f184d5efe59..7ac5c55cd12 100644 --- a/packages/opencode/src/tool/bash.ts +++ b/packages/opencode/src/tool/bash.ts @@ -108,12 +108,10 @@ export const BashTool = Tool.define("bash", { // always allow cd if it passes above check if (command[0] !== "cd") { const action = Wildcard.allStructured({ head: command[0], tail: command.slice(1) }, permissions) - if (action === "deny") { - throw new Error( - `The user has specifically restricted access to this command, you are not allowed to execute it. Here is the configuration: ${JSON.stringify(permissions)}`, - ) - } - if (action === "ask") { + // Secure default: only allow if explicitly set to "allow" or "ask" + if (action === "allow") { + // Explicitly allowed, proceed + } else if (action === "ask") { const pattern = (() => { if (command.length === 0) return const head = command[0] @@ -124,6 +122,11 @@ export const BashTool = Tool.define("bash", { if (pattern) { askPatterns.add(pattern) } + } else { + // Default deny for "deny", undefined, null, or any other value + throw new Error( + `Permission denied: Command not allowed by bash permissions. Command: ${command[0]}. Configuration: ${JSON.stringify(permissions)}`, + ) } } } diff --git a/packages/opencode/src/tool/edit.ts b/packages/opencode/src/tool/edit.ts index ca9859370bc..9bc7a286a5f 100644 --- a/packages/opencode/src/tool/edit.ts +++ b/packages/opencode/src/tool/edit.ts @@ -44,7 +44,10 @@ export const EditTool = Tool.define("edit", { const filePath = path.isAbsolute(params.filePath) ? params.filePath : path.join(Instance.directory, params.filePath) if (!Filesystem.contains(Instance.directory, filePath)) { const parentDir = path.dirname(filePath) - if (agent.permission.external_directory === "ask") { + // Secure default: only allow if explicitly set to "allow" or "ask" + if (agent.permission.external_directory === "allow") { + // Explicitly allowed, proceed + } else if (agent.permission.external_directory === "ask") { await Permission.ask({ type: "external_directory", pattern: parentDir, @@ -57,6 +60,9 @@ export const EditTool = Tool.define("edit", { parentDir, }, }) + } else { + // Default deny for "deny", undefined, null, or any other value + throw new Error(`Permission denied: Cannot edit file outside working directory: ${filePath}`) } } @@ -67,7 +73,10 @@ export const EditTool = Tool.define("edit", { if (params.oldString === "") { contentNew = params.newString diff = trimDiff(createTwoFilesPatch(filePath, filePath, contentOld, contentNew)) - if (agent.permission.edit === "ask") { + // Secure default: only allow if explicitly set to "allow" or "ask" + if (agent.permission.edit === "allow") { + // Explicitly allowed, proceed + } else if (agent.permission.edit === "ask") { await Permission.ask({ type: "edit", sessionID: ctx.sessionID, @@ -79,6 +88,9 @@ export const EditTool = Tool.define("edit", { diff, }, }) + } else { + // Default deny for "deny", undefined, null, or any other value + throw new Error(`Permission denied: Cannot edit file: ${filePath}`) } await Bun.write(filePath, params.newString) await Bus.publish(File.Event.Edited, { @@ -98,7 +110,10 @@ export const EditTool = Tool.define("edit", { diff = trimDiff( createTwoFilesPatch(filePath, filePath, normalizeLineEndings(contentOld), normalizeLineEndings(contentNew)), ) - if (agent.permission.edit === "ask") { + // Secure default: only allow if explicitly set to "allow" or "ask" + if (agent.permission.edit === "allow") { + // Explicitly allowed, proceed + } else if (agent.permission.edit === "ask") { await Permission.ask({ type: "edit", sessionID: ctx.sessionID, @@ -110,6 +125,9 @@ export const EditTool = Tool.define("edit", { diff, }, }) + } else { + // Default deny for "deny", undefined, null, or any other value + throw new Error(`Permission denied: Cannot edit file: ${filePath}`) } await file.write(contentNew) diff --git a/packages/opencode/src/tool/patch.ts b/packages/opencode/src/tool/patch.ts index 0571cd35318..9e827d90b17 100644 --- a/packages/opencode/src/tool/patch.ts +++ b/packages/opencode/src/tool/patch.ts @@ -55,7 +55,10 @@ export const PatchTool = Tool.define("patch", { if (!Filesystem.contains(Instance.directory, filePath)) { const parentDir = path.dirname(filePath) - if (agent.permission.external_directory === "ask") { + // Secure default: only allow if explicitly set to "allow" or "ask" + if (agent.permission.external_directory === "allow") { + // Explicitly allowed, proceed + } else if (agent.permission.external_directory === "ask") { await Permission.ask({ type: "external_directory", pattern: parentDir, @@ -68,6 +71,9 @@ export const PatchTool = Tool.define("patch", { parentDir, }, }) + } else { + // Default deny for "deny", undefined, null, or any other value + throw new Error(`Permission denied: Cannot patch file outside working directory: ${filePath}`) } } @@ -141,7 +147,10 @@ export const PatchTool = Tool.define("patch", { } // Check permissions if needed - if (agent.permission.edit === "ask") { + // Secure default: only allow if explicitly set to "allow" or "ask" + if (agent.permission.edit === "allow") { + // Explicitly allowed, proceed + } else if (agent.permission.edit === "ask") { await Permission.ask({ type: "edit", sessionID: ctx.sessionID, @@ -152,6 +161,9 @@ export const PatchTool = Tool.define("patch", { diff: totalDiff, }, }) + } else { + // Default deny for "deny", undefined, null, or any other value + throw new Error(`Permission denied: Cannot apply patch to ${fileChanges.length} files`) } // Apply the changes diff --git a/packages/opencode/src/tool/read.ts b/packages/opencode/src/tool/read.ts index ec97c8a6c23..7025cbe9453 100644 --- a/packages/opencode/src/tool/read.ts +++ b/packages/opencode/src/tool/read.ts @@ -33,7 +33,10 @@ export const ReadTool = Tool.define("read", { if (!ctx.extra?.["bypassCwdCheck"] && !Filesystem.contains(Instance.directory, filepath)) { const parentDir = path.dirname(filepath) - if (agent.permission.external_directory === "ask") { + // Secure default: only allow if explicitly set to "allow" or "ask" + if (agent.permission.external_directory === "allow") { + // Explicitly allowed, proceed + } else if (agent.permission.external_directory === "ask") { await Permission.ask({ type: "external_directory", pattern: parentDir, @@ -46,6 +49,9 @@ export const ReadTool = Tool.define("read", { parentDir, }, }) + } else { + // Default deny for "deny", undefined, null, or any other value + throw new Error(`Permission denied: Cannot access file outside working directory: ${filepath}`) } } diff --git a/packages/opencode/src/tool/write.ts b/packages/opencode/src/tool/write.ts index 58a0c177eb9..1318ad3733b 100644 --- a/packages/opencode/src/tool/write.ts +++ b/packages/opencode/src/tool/write.ts @@ -23,7 +23,10 @@ export const WriteTool = Tool.define("write", { const filepath = path.isAbsolute(params.filePath) ? params.filePath : path.join(Instance.directory, params.filePath) if (!Filesystem.contains(Instance.directory, filepath)) { const parentDir = path.dirname(filepath) - if (agent.permission.external_directory === "ask") { + // Secure default: only allow if explicitly set to "allow" or "ask" + if (agent.permission.external_directory === "allow") { + // Explicitly allowed, proceed + } else if (agent.permission.external_directory === "ask") { await Permission.ask({ type: "external_directory", pattern: parentDir, @@ -36,6 +39,9 @@ export const WriteTool = Tool.define("write", { parentDir, }, }) + } else { + // Default deny for "deny", undefined, null, or any other value + throw new Error(`Permission denied: Cannot write file outside working directory: ${filepath}`) } } @@ -43,7 +49,10 @@ export const WriteTool = Tool.define("write", { const exists = await file.exists() if (exists) await FileTime.assert(ctx.sessionID, filepath) - if (agent.permission.edit === "ask") + // Secure default: only allow if explicitly set to "allow" or "ask" + if (agent.permission.edit === "allow") { + // Explicitly allowed, proceed + } else if (agent.permission.edit === "ask") { await Permission.ask({ type: "write", sessionID: ctx.sessionID, @@ -56,6 +65,10 @@ export const WriteTool = Tool.define("write", { exists, }, }) + } else { + // Default deny for "deny", undefined, null, or any other value + throw new Error(`Permission denied: Cannot write file: ${filepath}`) + } await Bun.write(filepath, params.content) await Bus.publish(File.Event.Edited, { diff --git a/packages/opencode/test/tool/bash.test.ts b/packages/opencode/test/tool/bash.test.ts index 2919ccb0245..f10ffcce5f8 100644 --- a/packages/opencode/test/tool/bash.test.ts +++ b/packages/opencode/test/tool/bash.test.ts @@ -1,7 +1,16 @@ -import { describe, expect, test } from "bun:test" +import { describe, expect, test, mock } from "bun:test" import path from "path" import { BashTool } from "../../src/tool/bash" import { Instance } from "../../src/project/instance" +import { Permission } from "../../src/permission" +import { Log } from "../../src/util/log" + +Log.init({ print: false }) + +// Mock Permission.ask to auto-allow in tests +Permission.ask = mock(async () => { + return +}) const ctx = { sessionID: "test", @@ -12,7 +21,6 @@ const ctx = { metadata: () => {}, } -const bash = await BashTool.init() const projectRoot = path.join(__dirname, "../..") describe("tool.bash", () => { @@ -20,6 +28,7 @@ describe("tool.bash", () => { await Instance.provide({ directory: projectRoot, fn: async () => { + const bash = await BashTool.init() const result = await bash.execute( { command: "echo 'test'", @@ -37,6 +46,7 @@ describe("tool.bash", () => { await Instance.provide({ directory: projectRoot, fn: async () => { + const bash = await BashTool.init() expect( bash.execute( { @@ -49,4 +59,186 @@ describe("tool.bash", () => { }, }) }) + + test("should allow commands when bash permission='allow' via wildcard", async () => { + const { Agent } = await import("../../src/agent/agent") + const originalGet = Agent.get + + // Mock Agent.get to return agent with bash='allow' via wildcard + Agent.get = mock(async () => ({ + permission: { + edit: "allow" as const, + bash: { "*": "allow" as const }, + webfetch: "allow" as const, + external_directory: "ask" as const, + doom_loop: "ask" as const, + }, + })) as any + + const originalAsk = Permission.ask + const permissionAskMock = mock(async () => {}) + Permission.ask = permissionAskMock + + try { + await Instance.provide({ + directory: projectRoot, + fn: async () => { + const bash = await BashTool.init() + const result = await bash.execute( + { + command: "echo 'permission test'", + description: "Test bash permission", + }, + ctx, + ) + + expect(result.metadata.exit).toBe(0) + expect(result.metadata.output).toContain("permission test") + + // Verify Permission.ask was NOT called + expect(permissionAskMock).not.toHaveBeenCalled() + }, + }) + } finally { + Agent.get = originalGet + Permission.ask = originalAsk + } + }) + + test("should deny commands when bash permission='deny' via wildcard", async () => { + const { Agent } = await import("../../src/agent/agent") + const originalGet = Agent.get + + // Mock Agent.get to return agent with bash='deny' via wildcard + Agent.get = mock(async () => ({ + permission: { + edit: "allow" as const, + bash: { "*": "deny" as const }, + webfetch: "allow" as const, + external_directory: "ask" as const, + doom_loop: "ask" as const, + }, + })) as any + + const originalAsk = Permission.ask + const permissionAskMock = mock(async () => {}) + Permission.ask = permissionAskMock + + try { + await Instance.provide({ + directory: projectRoot, + fn: async () => { + const bash = await BashTool.init() + await expect( + bash.execute( + { + command: "echo 'should be denied'", + description: "Test bash permission deny", + }, + ctx, + ), + ).rejects.toThrow("Permission denied: Command not allowed by bash permissions") + + // Verify Permission.ask was NOT called + expect(permissionAskMock).not.toHaveBeenCalled() + }, + }) + } finally { + Agent.get = originalGet + Permission.ask = originalAsk + } + }) + + test("should ask for permission when bash permission='ask' via wildcard", async () => { + const { Agent } = await import("../../src/agent/agent") + const originalGet = Agent.get + + // Mock Agent.get to return agent with bash='ask' via wildcard + Agent.get = mock(async () => ({ + permission: { + edit: "allow" as const, + bash: { "*": "ask" as const }, + webfetch: "allow" as const, + external_directory: "ask" as const, + doom_loop: "ask" as const, + }, + })) as any + + const originalAsk = Permission.ask + const permissionAskMock = mock(async (input: any) => { + expect(input.type).toBe("bash") + // Resolve without throwing to simulate user approval + }) + Permission.ask = permissionAskMock + + try { + await Instance.provide({ + directory: projectRoot, + fn: async () => { + const bash = await BashTool.init() + const result = await bash.execute( + { + command: "echo 'ask permission'", + description: "Test bash permission ask", + }, + ctx, + ) + + expect(result.metadata.exit).toBe(0) + expect(result.metadata.output).toContain("ask permission") + + // Verify Permission.ask WAS called + expect(permissionAskMock).toHaveBeenCalled() + }, + }) + } finally { + Agent.get = originalGet + Permission.ask = originalAsk + } + }) + + test("should deny when no wildcard match (undefined permission)", async () => { + const { Agent } = await import("../../src/agent/agent") + const originalGet = Agent.get + + // Mock Agent.get to return agent with specific command allowed, but not echo + Agent.get = mock(async () => ({ + permission: { + edit: "allow" as const, + bash: { "ls *": "allow" as const }, // Only ls is allowed + webfetch: "allow" as const, + external_directory: "ask" as const, + doom_loop: "ask" as const, + }, + })) as any + + const originalAsk = Permission.ask + const permissionAskMock = mock(async () => {}) + Permission.ask = permissionAskMock + + try { + await Instance.provide({ + directory: projectRoot, + fn: async () => { + const bash = await BashTool.init() + // echo doesn't match any pattern, should default to deny + await expect( + bash.execute( + { + command: "echo 'no match'", + description: "Test undefined permission", + }, + ctx, + ), + ).rejects.toThrow("Permission denied: Command not allowed by bash permissions") + + // Verify Permission.ask was NOT called + expect(permissionAskMock).not.toHaveBeenCalled() + }, + }) + } finally { + Agent.get = originalGet + Permission.ask = originalAsk + } + }) }) diff --git a/packages/opencode/test/tool/edit.test.ts b/packages/opencode/test/tool/edit.test.ts new file mode 100644 index 00000000000..8ebcb50262e --- /dev/null +++ b/packages/opencode/test/tool/edit.test.ts @@ -0,0 +1,253 @@ +import { describe, expect, test, mock } from "bun:test" +import path from "path" +import { EditTool } from "../../src/tool/edit" +import { Instance } from "../../src/project/instance" +import { tmpdir } from "../fixture/fixture" +import { Permission } from "../../src/permission" +import { FileTime } from "../../src/file/time" +import * as fs from "fs/promises" +import { Log } from "../../src/util/log" + +Log.init({ print: false }) + +const ctx = { + sessionID: "test", + messageID: "", + toolCallID: "", + agent: "build", + abort: AbortSignal.any([]), + metadata: () => {}, +} + +describe("tool.edit", () => { + test("should allow edits when permission='allow'", async () => { + const { Agent } = await import("../../src/agent/agent") + const originalGet = Agent.get + + // Mock Agent.get to return agent with edit='allow' + Agent.get = mock(async () => ({ + permission: { + edit: "allow" as const, + bash: { "*": "allow" as const }, + webfetch: "allow" as const, + external_directory: "ask" as const, + doom_loop: "ask" as const, + }, + })) as any + + const originalAsk = Permission.ask + const permissionAskMock = mock(async () => {}) + Permission.ask = permissionAskMock + + try { + await using fixture = await tmpdir() + await Instance.provide({ + directory: fixture.path, + fn: async () => { + const editTool = await EditTool.init() + const testFile = path.join(fixture.path, "test.txt") + await fs.writeFile(testFile, "original content") + + // Mark file as read (required by FileTime.assert) + FileTime.read(ctx.sessionID, testFile) + + // Should succeed without asking + await editTool.execute( + { + filePath: testFile, + oldString: "original", + newString: "modified", + }, + ctx, + ) + + // Verify Permission.ask was NOT called + expect(permissionAskMock).not.toHaveBeenCalled() + + // Verify file was edited + const content = await fs.readFile(testFile, "utf-8") + expect(content).toBe("modified content") + }, + }) + } finally { + Agent.get = originalGet + Permission.ask = originalAsk + } + }) + + test("should deny edits when permission='deny'", async () => { + const { Agent } = await import("../../src/agent/agent") + const originalGet = Agent.get + + // Mock Agent.get to return agent with edit='deny' + Agent.get = mock(async () => ({ + permission: { + edit: "deny" as const, + bash: { "*": "allow" as const }, + webfetch: "allow" as const, + external_directory: "ask" as const, + doom_loop: "ask" as const, + }, + })) as any + + const originalAsk = Permission.ask + const permissionAskMock = mock(async () => {}) + Permission.ask = permissionAskMock + + try { + await using fixture = await tmpdir() + await Instance.provide({ + directory: fixture.path, + fn: async () => { + const editTool = await EditTool.init() + const testFile = path.join(fixture.path, "test.txt") + await fs.writeFile(testFile, "original content") + + // Mark file as read (required by FileTime.assert) + FileTime.read(ctx.sessionID, testFile) + + // Should throw without asking + await expect( + editTool.execute( + { + filePath: testFile, + oldString: "original", + newString: "modified", + }, + ctx, + ), + ).rejects.toThrow("Permission denied: Cannot edit file") + + // Verify Permission.ask was NOT called + expect(permissionAskMock).not.toHaveBeenCalled() + + // Verify file was NOT edited + const content = await fs.readFile(testFile, "utf-8") + expect(content).toBe("original content") + }, + }) + } finally { + Agent.get = originalGet + Permission.ask = originalAsk + } + }) + + test("should ask and allow edits when permission='ask' and user approves", async () => { + const { Agent } = await import("../../src/agent/agent") + const originalGet = Agent.get + + // Mock Agent.get to return agent with edit='ask' + Agent.get = mock(async () => ({ + permission: { + edit: "ask" as const, + bash: { "*": "allow" as const }, + webfetch: "allow" as const, + external_directory: "ask" as const, + doom_loop: "ask" as const, + }, + })) as any + + const originalAsk = Permission.ask + const permissionAskMock = mock(async (input: any) => { + expect(input.type).toBe("edit") + // Resolve without throwing to simulate user approval + }) + Permission.ask = permissionAskMock + + try { + await using fixture = await tmpdir() + await Instance.provide({ + directory: fixture.path, + fn: async () => { + const editTool = await EditTool.init() + const testFile = path.join(fixture.path, "test.txt") + await fs.writeFile(testFile, "original content") + + // Mark file as read (required by FileTime.assert) + FileTime.read(ctx.sessionID, testFile) + + // Should succeed after asking + await editTool.execute( + { + filePath: testFile, + oldString: "original", + newString: "modified", + }, + ctx, + ) + + // Verify Permission.ask WAS called + expect(permissionAskMock).toHaveBeenCalled() + + // Verify file was edited + const content = await fs.readFile(testFile, "utf-8") + expect(content).toBe("modified content") + }, + }) + } finally { + Agent.get = originalGet + Permission.ask = originalAsk + } + }) + + test("should ask and deny edits when permission='ask' and user denies", async () => { + const { Agent } = await import("../../src/agent/agent") + const originalGet = Agent.get + + // Mock Agent.get to return agent with edit='ask' + Agent.get = mock(async () => ({ + permission: { + edit: "ask" as const, + bash: { "*": "allow" as const }, + webfetch: "allow" as const, + external_directory: "ask" as const, + doom_loop: "ask" as const, + }, + })) as any + + const originalAsk = Permission.ask + const permissionAskMock = mock(async (input: any) => { + expect(input.type).toBe("edit") + // Throw to simulate user denial + throw new Error("Permission denied by user") + }) + Permission.ask = permissionAskMock + + try { + await using fixture = await tmpdir() + await Instance.provide({ + directory: fixture.path, + fn: async () => { + const editTool = await EditTool.init() + const testFile = path.join(fixture.path, "test.txt") + await fs.writeFile(testFile, "original content") + + // Mark file as read (required by FileTime.assert) + FileTime.read(ctx.sessionID, testFile) + + // Should throw because permission was denied + await expect( + editTool.execute( + { + filePath: testFile, + oldString: "original", + newString: "modified", + }, + ctx, + ), + ).rejects.toThrow("Permission denied by user") + + // Verify Permission.ask WAS called + expect(permissionAskMock).toHaveBeenCalled() + + // Verify file was NOT edited + const content = await fs.readFile(testFile, "utf-8") + expect(content).toBe("original content") + }, + }) + } finally { + Agent.get = originalGet + Permission.ask = originalAsk + } + }) +}) diff --git a/packages/opencode/test/tool/patch.test.ts b/packages/opencode/test/tool/patch.test.ts index 6d7d6db87f5..18e89cecb21 100644 --- a/packages/opencode/test/tool/patch.test.ts +++ b/packages/opencode/test/tool/patch.test.ts @@ -1,10 +1,13 @@ -import { describe, expect, test } from "bun:test" +import { describe, expect, test, mock } from "bun:test" import path from "path" import { PatchTool } from "../../src/tool/patch" import { Instance } from "../../src/project/instance" import { tmpdir } from "../fixture/fixture" import { Permission } from "../../src/permission" import * as fs from "fs/promises" +import { Log } from "../../src/util/log" + +Log.init({ print: false }) const ctx = { sessionID: "test", @@ -48,20 +51,188 @@ describe("tool.patch", () => { }) }) - test.skip("should ask permission for files outside working directory", async () => { - await Instance.provide({ - directory: "/tmp", - fn: async () => { - const maliciousPatch = `*** Begin Patch + test("should ask permission for files outside working directory (permission='ask', user denies)", async () => { + // Mock Permission.ask to verify it gets called with correct parameters + const permissionAskMock = mock(async (input: any) => { + // Verify the permission request has the right properties + expect(input.type).toBe("external_directory") + expect(input.title).toContain("/etc/passwd") + expect(input.sessionID).toBe(ctx.sessionID) + // Throw to simulate user denying permission + throw new Error("Permission denied by user") + }) + + const originalAsk = Permission.ask + Permission.ask = permissionAskMock + + try { + await Instance.provide({ + directory: "/tmp/opencode-test-" + Date.now(), + fn: async () => { + const patchTool = await PatchTool.init() + const maliciousPatch = `*** Begin Patch *** Add File: /etc/passwd +malicious content *** End Patch` - patchTool.execute({ patchText: maliciousPatch }, ctx) - // TODO: this sucks - await new Promise((resolve) => setTimeout(resolve, 1000)) - expect(Permission.pending()[ctx.sessionID]).toBeDefined() + + // Should throw because permission was denied + await expect(patchTool.execute({ patchText: maliciousPatch }, ctx)).rejects.toThrow( + "Permission denied by user", + ) + + // Verify Permission.ask was called + expect(permissionAskMock).toHaveBeenCalled() + }, + }) + } finally { + // Restore original Permission.ask + Permission.ask = originalAsk + } + }) + + test("should allow files outside working directory when permission='allow'", async () => { + const { Agent } = await import("../../src/agent/agent") + const originalGet = Agent.get + + // Mock Agent.get to return agent with external_directory='allow' + Agent.get = mock(async () => ({ + permission: { + edit: "allow" as const, + bash: { "*": "allow" as const }, + webfetch: "allow" as const, + external_directory: "allow" as const, + doom_loop: "ask" as const, }, + })) as any + + const originalAsk = Permission.ask + const permissionAskMock = mock(async () => {}) + Permission.ask = permissionAskMock + + try { + await using fixture = await tmpdir() + await Instance.provide({ + directory: fixture.path, + fn: async () => { + const patchTool = await PatchTool.init() + const externalPath = "/tmp/opencode-external-" + Date.now() + ".txt" + const patch = `*** Begin Patch +*** Add File: ${externalPath} ++external content +*** End Patch` + + // Should succeed without asking + const result = await patchTool.execute({ patchText: patch }, ctx) + expect(result.output).toContain("Patch applied successfully") + + // Verify Permission.ask was NOT called + expect(permissionAskMock).not.toHaveBeenCalled() + + // Clean up + await fs.unlink(externalPath).catch(() => {}) + }, + }) + } finally { + Agent.get = originalGet + Permission.ask = originalAsk + } + }) + + test("should deny files outside working directory when permission='deny'", async () => { + const { Agent } = await import("../../src/agent/agent") + const originalGet = Agent.get + + // Mock Agent.get to return agent with external_directory='deny' + Agent.get = mock(async () => ({ + permission: { + edit: "allow" as const, + bash: { "*": "allow" as const }, + webfetch: "allow" as const, + external_directory: "deny" as const, + doom_loop: "ask" as const, + }, + })) as any + + const originalAsk = Permission.ask + const permissionAskMock = mock(async () => {}) + Permission.ask = permissionAskMock + + try { + await using fixture = await tmpdir() + await Instance.provide({ + directory: fixture.path, + fn: async () => { + const patchTool = await PatchTool.init() + const externalPath = "/tmp/opencode-external-deny-test.txt" + const patch = `*** Begin Patch +*** Add File: ${externalPath} ++external content +*** End Patch` + + // Should throw without asking + await expect(patchTool.execute({ patchText: patch }, ctx)).rejects.toThrow( + "Permission denied: Cannot patch file outside working directory", + ) + + // Verify Permission.ask was NOT called + expect(permissionAskMock).not.toHaveBeenCalled() + }, + }) + } finally { + Agent.get = originalGet + Permission.ask = originalAsk + } + }) + + test("should ask and allow files outside working directory when permission='ask' and user approves", async () => { + const { Agent } = await import("../../src/agent/agent") + const originalGet = Agent.get + + // Mock Agent.get to return agent with external_directory='ask' + Agent.get = mock(async () => ({ + permission: { + edit: "allow" as const, + bash: { "*": "allow" as const }, + webfetch: "allow" as const, + external_directory: "ask" as const, + doom_loop: "ask" as const, + }, + })) as any + + const originalAsk = Permission.ask + const permissionAskMock = mock(async (input: any) => { + expect(input.type).toBe("external_directory") + // Resolve without throwing to simulate user approval }) + Permission.ask = permissionAskMock + + try { + await using fixture = await tmpdir() + await Instance.provide({ + directory: fixture.path, + fn: async () => { + const patchTool = await PatchTool.init() + const externalPath = "/tmp/opencode-external-ask-" + Date.now() + ".txt" + const patch = `*** Begin Patch +*** Add File: ${externalPath} ++external content +*** End Patch` + + // Should succeed after asking + const result = await patchTool.execute({ patchText: patch }, ctx) + expect(result.output).toContain("Patch applied successfully") + + // Verify Permission.ask WAS called + expect(permissionAskMock).toHaveBeenCalled() + + // Clean up + await fs.unlink(externalPath).catch(() => {}) + }, + }) + } finally { + Agent.get = originalGet + Permission.ask = originalAsk + } }) test("should handle simple add file operation", async () => { From a12caa8e60ab9d8b47ef79b1f3fb1f264d2cd02e Mon Sep 17 00:00:00 2001 From: Aiden Cline Date: Tue, 18 Nov 2025 16:32:44 -0600 Subject: [PATCH 2/3] Revert "fix: permission checks not enforcing deny settings" This reverts commit f0fa213cd3037ee9105388b67ee7331cb6ccf598. --- .opencode/opencode.jsonc | 3 - packages/opencode/src/agent/agent.ts | 4 +- packages/opencode/src/session/processor.ts | 10 +- packages/opencode/src/tool/bash.ts | 15 +- packages/opencode/src/tool/edit.ts | 24 +- packages/opencode/src/tool/patch.ts | 16 +- packages/opencode/src/tool/read.ts | 8 +- packages/opencode/src/tool/write.ts | 17 +- packages/opencode/test/tool/bash.test.ts | 196 +--------------- packages/opencode/test/tool/edit.test.ts | 253 --------------------- packages/opencode/test/tool/patch.test.ts | 191 +--------------- 11 files changed, 29 insertions(+), 708 deletions(-) delete mode 100644 packages/opencode/test/tool/edit.test.ts diff --git a/.opencode/opencode.jsonc b/.opencode/opencode.jsonc index 6e3a5ea744f..2a4558e4288 100644 --- a/.opencode/opencode.jsonc +++ b/.opencode/opencode.jsonc @@ -8,7 +8,4 @@ }, }, }, - "permission": { - "external_directory": "ask" - } } diff --git a/packages/opencode/src/agent/agent.ts b/packages/opencode/src/agent/agent.ts index dfbd0393b13..740f67b7e04 100644 --- a/packages/opencode/src/agent/agent.ts +++ b/packages/opencode/src/agent/agent.ts @@ -250,8 +250,8 @@ function mergeAgentPermissions(basePermission: any, overridePermission: any): Ag edit: merged.edit ?? "allow", webfetch: merged.webfetch ?? "allow", bash: mergedBash ?? { "*": "allow" }, - doom_loop: merged.doom_loop ?? "ask", - external_directory: merged.external_directory ?? "ask", + doom_loop: merged.doom_loop, + external_directory: merged.external_directory, } return result diff --git a/packages/opencode/src/session/processor.ts b/packages/opencode/src/session/processor.ts index 97ce98cb538..5c2005587b2 100644 --- a/packages/opencode/src/session/processor.ts +++ b/packages/opencode/src/session/processor.ts @@ -147,10 +147,7 @@ export namespace SessionProcessor { ) ) { const permission = await Agent.get(input.assistantMessage.mode).then((x) => x.permission) - // Secure default: only allow if explicitly set to "allow" or "ask" - if (permission.doom_loop === "allow") { - // Explicitly allowed, proceed - } else if (permission.doom_loop === "ask") { + if (permission.doom_loop === "ask") { await Permission.ask({ type: "doom_loop", pattern: value.toolName, @@ -163,11 +160,6 @@ export namespace SessionProcessor { input: value.input, }, }) - } else { - // Default deny for "deny", undefined, null, or any other value - throw new Error( - `Permission denied: Possible doom loop detected - "${value.toolName}" called ${DOOM_LOOP_THRESHOLD} times with identical arguments`, - ) } } } diff --git a/packages/opencode/src/tool/bash.ts b/packages/opencode/src/tool/bash.ts index 7ac5c55cd12..f184d5efe59 100644 --- a/packages/opencode/src/tool/bash.ts +++ b/packages/opencode/src/tool/bash.ts @@ -108,10 +108,12 @@ export const BashTool = Tool.define("bash", { // always allow cd if it passes above check if (command[0] !== "cd") { const action = Wildcard.allStructured({ head: command[0], tail: command.slice(1) }, permissions) - // Secure default: only allow if explicitly set to "allow" or "ask" - if (action === "allow") { - // Explicitly allowed, proceed - } else if (action === "ask") { + if (action === "deny") { + throw new Error( + `The user has specifically restricted access to this command, you are not allowed to execute it. Here is the configuration: ${JSON.stringify(permissions)}`, + ) + } + if (action === "ask") { const pattern = (() => { if (command.length === 0) return const head = command[0] @@ -122,11 +124,6 @@ export const BashTool = Tool.define("bash", { if (pattern) { askPatterns.add(pattern) } - } else { - // Default deny for "deny", undefined, null, or any other value - throw new Error( - `Permission denied: Command not allowed by bash permissions. Command: ${command[0]}. Configuration: ${JSON.stringify(permissions)}`, - ) } } } diff --git a/packages/opencode/src/tool/edit.ts b/packages/opencode/src/tool/edit.ts index 9bc7a286a5f..ca9859370bc 100644 --- a/packages/opencode/src/tool/edit.ts +++ b/packages/opencode/src/tool/edit.ts @@ -44,10 +44,7 @@ export const EditTool = Tool.define("edit", { const filePath = path.isAbsolute(params.filePath) ? params.filePath : path.join(Instance.directory, params.filePath) if (!Filesystem.contains(Instance.directory, filePath)) { const parentDir = path.dirname(filePath) - // Secure default: only allow if explicitly set to "allow" or "ask" - if (agent.permission.external_directory === "allow") { - // Explicitly allowed, proceed - } else if (agent.permission.external_directory === "ask") { + if (agent.permission.external_directory === "ask") { await Permission.ask({ type: "external_directory", pattern: parentDir, @@ -60,9 +57,6 @@ export const EditTool = Tool.define("edit", { parentDir, }, }) - } else { - // Default deny for "deny", undefined, null, or any other value - throw new Error(`Permission denied: Cannot edit file outside working directory: ${filePath}`) } } @@ -73,10 +67,7 @@ export const EditTool = Tool.define("edit", { if (params.oldString === "") { contentNew = params.newString diff = trimDiff(createTwoFilesPatch(filePath, filePath, contentOld, contentNew)) - // Secure default: only allow if explicitly set to "allow" or "ask" - if (agent.permission.edit === "allow") { - // Explicitly allowed, proceed - } else if (agent.permission.edit === "ask") { + if (agent.permission.edit === "ask") { await Permission.ask({ type: "edit", sessionID: ctx.sessionID, @@ -88,9 +79,6 @@ export const EditTool = Tool.define("edit", { diff, }, }) - } else { - // Default deny for "deny", undefined, null, or any other value - throw new Error(`Permission denied: Cannot edit file: ${filePath}`) } await Bun.write(filePath, params.newString) await Bus.publish(File.Event.Edited, { @@ -110,10 +98,7 @@ export const EditTool = Tool.define("edit", { diff = trimDiff( createTwoFilesPatch(filePath, filePath, normalizeLineEndings(contentOld), normalizeLineEndings(contentNew)), ) - // Secure default: only allow if explicitly set to "allow" or "ask" - if (agent.permission.edit === "allow") { - // Explicitly allowed, proceed - } else if (agent.permission.edit === "ask") { + if (agent.permission.edit === "ask") { await Permission.ask({ type: "edit", sessionID: ctx.sessionID, @@ -125,9 +110,6 @@ export const EditTool = Tool.define("edit", { diff, }, }) - } else { - // Default deny for "deny", undefined, null, or any other value - throw new Error(`Permission denied: Cannot edit file: ${filePath}`) } await file.write(contentNew) diff --git a/packages/opencode/src/tool/patch.ts b/packages/opencode/src/tool/patch.ts index 9e827d90b17..0571cd35318 100644 --- a/packages/opencode/src/tool/patch.ts +++ b/packages/opencode/src/tool/patch.ts @@ -55,10 +55,7 @@ export const PatchTool = Tool.define("patch", { if (!Filesystem.contains(Instance.directory, filePath)) { const parentDir = path.dirname(filePath) - // Secure default: only allow if explicitly set to "allow" or "ask" - if (agent.permission.external_directory === "allow") { - // Explicitly allowed, proceed - } else if (agent.permission.external_directory === "ask") { + if (agent.permission.external_directory === "ask") { await Permission.ask({ type: "external_directory", pattern: parentDir, @@ -71,9 +68,6 @@ export const PatchTool = Tool.define("patch", { parentDir, }, }) - } else { - // Default deny for "deny", undefined, null, or any other value - throw new Error(`Permission denied: Cannot patch file outside working directory: ${filePath}`) } } @@ -147,10 +141,7 @@ export const PatchTool = Tool.define("patch", { } // Check permissions if needed - // Secure default: only allow if explicitly set to "allow" or "ask" - if (agent.permission.edit === "allow") { - // Explicitly allowed, proceed - } else if (agent.permission.edit === "ask") { + if (agent.permission.edit === "ask") { await Permission.ask({ type: "edit", sessionID: ctx.sessionID, @@ -161,9 +152,6 @@ export const PatchTool = Tool.define("patch", { diff: totalDiff, }, }) - } else { - // Default deny for "deny", undefined, null, or any other value - throw new Error(`Permission denied: Cannot apply patch to ${fileChanges.length} files`) } // Apply the changes diff --git a/packages/opencode/src/tool/read.ts b/packages/opencode/src/tool/read.ts index 7025cbe9453..ec97c8a6c23 100644 --- a/packages/opencode/src/tool/read.ts +++ b/packages/opencode/src/tool/read.ts @@ -33,10 +33,7 @@ export const ReadTool = Tool.define("read", { if (!ctx.extra?.["bypassCwdCheck"] && !Filesystem.contains(Instance.directory, filepath)) { const parentDir = path.dirname(filepath) - // Secure default: only allow if explicitly set to "allow" or "ask" - if (agent.permission.external_directory === "allow") { - // Explicitly allowed, proceed - } else if (agent.permission.external_directory === "ask") { + if (agent.permission.external_directory === "ask") { await Permission.ask({ type: "external_directory", pattern: parentDir, @@ -49,9 +46,6 @@ export const ReadTool = Tool.define("read", { parentDir, }, }) - } else { - // Default deny for "deny", undefined, null, or any other value - throw new Error(`Permission denied: Cannot access file outside working directory: ${filepath}`) } } diff --git a/packages/opencode/src/tool/write.ts b/packages/opencode/src/tool/write.ts index 1318ad3733b..58a0c177eb9 100644 --- a/packages/opencode/src/tool/write.ts +++ b/packages/opencode/src/tool/write.ts @@ -23,10 +23,7 @@ export const WriteTool = Tool.define("write", { const filepath = path.isAbsolute(params.filePath) ? params.filePath : path.join(Instance.directory, params.filePath) if (!Filesystem.contains(Instance.directory, filepath)) { const parentDir = path.dirname(filepath) - // Secure default: only allow if explicitly set to "allow" or "ask" - if (agent.permission.external_directory === "allow") { - // Explicitly allowed, proceed - } else if (agent.permission.external_directory === "ask") { + if (agent.permission.external_directory === "ask") { await Permission.ask({ type: "external_directory", pattern: parentDir, @@ -39,9 +36,6 @@ export const WriteTool = Tool.define("write", { parentDir, }, }) - } else { - // Default deny for "deny", undefined, null, or any other value - throw new Error(`Permission denied: Cannot write file outside working directory: ${filepath}`) } } @@ -49,10 +43,7 @@ export const WriteTool = Tool.define("write", { const exists = await file.exists() if (exists) await FileTime.assert(ctx.sessionID, filepath) - // Secure default: only allow if explicitly set to "allow" or "ask" - if (agent.permission.edit === "allow") { - // Explicitly allowed, proceed - } else if (agent.permission.edit === "ask") { + if (agent.permission.edit === "ask") await Permission.ask({ type: "write", sessionID: ctx.sessionID, @@ -65,10 +56,6 @@ export const WriteTool = Tool.define("write", { exists, }, }) - } else { - // Default deny for "deny", undefined, null, or any other value - throw new Error(`Permission denied: Cannot write file: ${filepath}`) - } await Bun.write(filepath, params.content) await Bus.publish(File.Event.Edited, { diff --git a/packages/opencode/test/tool/bash.test.ts b/packages/opencode/test/tool/bash.test.ts index f10ffcce5f8..2919ccb0245 100644 --- a/packages/opencode/test/tool/bash.test.ts +++ b/packages/opencode/test/tool/bash.test.ts @@ -1,16 +1,7 @@ -import { describe, expect, test, mock } from "bun:test" +import { describe, expect, test } from "bun:test" import path from "path" import { BashTool } from "../../src/tool/bash" import { Instance } from "../../src/project/instance" -import { Permission } from "../../src/permission" -import { Log } from "../../src/util/log" - -Log.init({ print: false }) - -// Mock Permission.ask to auto-allow in tests -Permission.ask = mock(async () => { - return -}) const ctx = { sessionID: "test", @@ -21,6 +12,7 @@ const ctx = { metadata: () => {}, } +const bash = await BashTool.init() const projectRoot = path.join(__dirname, "../..") describe("tool.bash", () => { @@ -28,7 +20,6 @@ describe("tool.bash", () => { await Instance.provide({ directory: projectRoot, fn: async () => { - const bash = await BashTool.init() const result = await bash.execute( { command: "echo 'test'", @@ -46,7 +37,6 @@ describe("tool.bash", () => { await Instance.provide({ directory: projectRoot, fn: async () => { - const bash = await BashTool.init() expect( bash.execute( { @@ -59,186 +49,4 @@ describe("tool.bash", () => { }, }) }) - - test("should allow commands when bash permission='allow' via wildcard", async () => { - const { Agent } = await import("../../src/agent/agent") - const originalGet = Agent.get - - // Mock Agent.get to return agent with bash='allow' via wildcard - Agent.get = mock(async () => ({ - permission: { - edit: "allow" as const, - bash: { "*": "allow" as const }, - webfetch: "allow" as const, - external_directory: "ask" as const, - doom_loop: "ask" as const, - }, - })) as any - - const originalAsk = Permission.ask - const permissionAskMock = mock(async () => {}) - Permission.ask = permissionAskMock - - try { - await Instance.provide({ - directory: projectRoot, - fn: async () => { - const bash = await BashTool.init() - const result = await bash.execute( - { - command: "echo 'permission test'", - description: "Test bash permission", - }, - ctx, - ) - - expect(result.metadata.exit).toBe(0) - expect(result.metadata.output).toContain("permission test") - - // Verify Permission.ask was NOT called - expect(permissionAskMock).not.toHaveBeenCalled() - }, - }) - } finally { - Agent.get = originalGet - Permission.ask = originalAsk - } - }) - - test("should deny commands when bash permission='deny' via wildcard", async () => { - const { Agent } = await import("../../src/agent/agent") - const originalGet = Agent.get - - // Mock Agent.get to return agent with bash='deny' via wildcard - Agent.get = mock(async () => ({ - permission: { - edit: "allow" as const, - bash: { "*": "deny" as const }, - webfetch: "allow" as const, - external_directory: "ask" as const, - doom_loop: "ask" as const, - }, - })) as any - - const originalAsk = Permission.ask - const permissionAskMock = mock(async () => {}) - Permission.ask = permissionAskMock - - try { - await Instance.provide({ - directory: projectRoot, - fn: async () => { - const bash = await BashTool.init() - await expect( - bash.execute( - { - command: "echo 'should be denied'", - description: "Test bash permission deny", - }, - ctx, - ), - ).rejects.toThrow("Permission denied: Command not allowed by bash permissions") - - // Verify Permission.ask was NOT called - expect(permissionAskMock).not.toHaveBeenCalled() - }, - }) - } finally { - Agent.get = originalGet - Permission.ask = originalAsk - } - }) - - test("should ask for permission when bash permission='ask' via wildcard", async () => { - const { Agent } = await import("../../src/agent/agent") - const originalGet = Agent.get - - // Mock Agent.get to return agent with bash='ask' via wildcard - Agent.get = mock(async () => ({ - permission: { - edit: "allow" as const, - bash: { "*": "ask" as const }, - webfetch: "allow" as const, - external_directory: "ask" as const, - doom_loop: "ask" as const, - }, - })) as any - - const originalAsk = Permission.ask - const permissionAskMock = mock(async (input: any) => { - expect(input.type).toBe("bash") - // Resolve without throwing to simulate user approval - }) - Permission.ask = permissionAskMock - - try { - await Instance.provide({ - directory: projectRoot, - fn: async () => { - const bash = await BashTool.init() - const result = await bash.execute( - { - command: "echo 'ask permission'", - description: "Test bash permission ask", - }, - ctx, - ) - - expect(result.metadata.exit).toBe(0) - expect(result.metadata.output).toContain("ask permission") - - // Verify Permission.ask WAS called - expect(permissionAskMock).toHaveBeenCalled() - }, - }) - } finally { - Agent.get = originalGet - Permission.ask = originalAsk - } - }) - - test("should deny when no wildcard match (undefined permission)", async () => { - const { Agent } = await import("../../src/agent/agent") - const originalGet = Agent.get - - // Mock Agent.get to return agent with specific command allowed, but not echo - Agent.get = mock(async () => ({ - permission: { - edit: "allow" as const, - bash: { "ls *": "allow" as const }, // Only ls is allowed - webfetch: "allow" as const, - external_directory: "ask" as const, - doom_loop: "ask" as const, - }, - })) as any - - const originalAsk = Permission.ask - const permissionAskMock = mock(async () => {}) - Permission.ask = permissionAskMock - - try { - await Instance.provide({ - directory: projectRoot, - fn: async () => { - const bash = await BashTool.init() - // echo doesn't match any pattern, should default to deny - await expect( - bash.execute( - { - command: "echo 'no match'", - description: "Test undefined permission", - }, - ctx, - ), - ).rejects.toThrow("Permission denied: Command not allowed by bash permissions") - - // Verify Permission.ask was NOT called - expect(permissionAskMock).not.toHaveBeenCalled() - }, - }) - } finally { - Agent.get = originalGet - Permission.ask = originalAsk - } - }) }) diff --git a/packages/opencode/test/tool/edit.test.ts b/packages/opencode/test/tool/edit.test.ts deleted file mode 100644 index 8ebcb50262e..00000000000 --- a/packages/opencode/test/tool/edit.test.ts +++ /dev/null @@ -1,253 +0,0 @@ -import { describe, expect, test, mock } from "bun:test" -import path from "path" -import { EditTool } from "../../src/tool/edit" -import { Instance } from "../../src/project/instance" -import { tmpdir } from "../fixture/fixture" -import { Permission } from "../../src/permission" -import { FileTime } from "../../src/file/time" -import * as fs from "fs/promises" -import { Log } from "../../src/util/log" - -Log.init({ print: false }) - -const ctx = { - sessionID: "test", - messageID: "", - toolCallID: "", - agent: "build", - abort: AbortSignal.any([]), - metadata: () => {}, -} - -describe("tool.edit", () => { - test("should allow edits when permission='allow'", async () => { - const { Agent } = await import("../../src/agent/agent") - const originalGet = Agent.get - - // Mock Agent.get to return agent with edit='allow' - Agent.get = mock(async () => ({ - permission: { - edit: "allow" as const, - bash: { "*": "allow" as const }, - webfetch: "allow" as const, - external_directory: "ask" as const, - doom_loop: "ask" as const, - }, - })) as any - - const originalAsk = Permission.ask - const permissionAskMock = mock(async () => {}) - Permission.ask = permissionAskMock - - try { - await using fixture = await tmpdir() - await Instance.provide({ - directory: fixture.path, - fn: async () => { - const editTool = await EditTool.init() - const testFile = path.join(fixture.path, "test.txt") - await fs.writeFile(testFile, "original content") - - // Mark file as read (required by FileTime.assert) - FileTime.read(ctx.sessionID, testFile) - - // Should succeed without asking - await editTool.execute( - { - filePath: testFile, - oldString: "original", - newString: "modified", - }, - ctx, - ) - - // Verify Permission.ask was NOT called - expect(permissionAskMock).not.toHaveBeenCalled() - - // Verify file was edited - const content = await fs.readFile(testFile, "utf-8") - expect(content).toBe("modified content") - }, - }) - } finally { - Agent.get = originalGet - Permission.ask = originalAsk - } - }) - - test("should deny edits when permission='deny'", async () => { - const { Agent } = await import("../../src/agent/agent") - const originalGet = Agent.get - - // Mock Agent.get to return agent with edit='deny' - Agent.get = mock(async () => ({ - permission: { - edit: "deny" as const, - bash: { "*": "allow" as const }, - webfetch: "allow" as const, - external_directory: "ask" as const, - doom_loop: "ask" as const, - }, - })) as any - - const originalAsk = Permission.ask - const permissionAskMock = mock(async () => {}) - Permission.ask = permissionAskMock - - try { - await using fixture = await tmpdir() - await Instance.provide({ - directory: fixture.path, - fn: async () => { - const editTool = await EditTool.init() - const testFile = path.join(fixture.path, "test.txt") - await fs.writeFile(testFile, "original content") - - // Mark file as read (required by FileTime.assert) - FileTime.read(ctx.sessionID, testFile) - - // Should throw without asking - await expect( - editTool.execute( - { - filePath: testFile, - oldString: "original", - newString: "modified", - }, - ctx, - ), - ).rejects.toThrow("Permission denied: Cannot edit file") - - // Verify Permission.ask was NOT called - expect(permissionAskMock).not.toHaveBeenCalled() - - // Verify file was NOT edited - const content = await fs.readFile(testFile, "utf-8") - expect(content).toBe("original content") - }, - }) - } finally { - Agent.get = originalGet - Permission.ask = originalAsk - } - }) - - test("should ask and allow edits when permission='ask' and user approves", async () => { - const { Agent } = await import("../../src/agent/agent") - const originalGet = Agent.get - - // Mock Agent.get to return agent with edit='ask' - Agent.get = mock(async () => ({ - permission: { - edit: "ask" as const, - bash: { "*": "allow" as const }, - webfetch: "allow" as const, - external_directory: "ask" as const, - doom_loop: "ask" as const, - }, - })) as any - - const originalAsk = Permission.ask - const permissionAskMock = mock(async (input: any) => { - expect(input.type).toBe("edit") - // Resolve without throwing to simulate user approval - }) - Permission.ask = permissionAskMock - - try { - await using fixture = await tmpdir() - await Instance.provide({ - directory: fixture.path, - fn: async () => { - const editTool = await EditTool.init() - const testFile = path.join(fixture.path, "test.txt") - await fs.writeFile(testFile, "original content") - - // Mark file as read (required by FileTime.assert) - FileTime.read(ctx.sessionID, testFile) - - // Should succeed after asking - await editTool.execute( - { - filePath: testFile, - oldString: "original", - newString: "modified", - }, - ctx, - ) - - // Verify Permission.ask WAS called - expect(permissionAskMock).toHaveBeenCalled() - - // Verify file was edited - const content = await fs.readFile(testFile, "utf-8") - expect(content).toBe("modified content") - }, - }) - } finally { - Agent.get = originalGet - Permission.ask = originalAsk - } - }) - - test("should ask and deny edits when permission='ask' and user denies", async () => { - const { Agent } = await import("../../src/agent/agent") - const originalGet = Agent.get - - // Mock Agent.get to return agent with edit='ask' - Agent.get = mock(async () => ({ - permission: { - edit: "ask" as const, - bash: { "*": "allow" as const }, - webfetch: "allow" as const, - external_directory: "ask" as const, - doom_loop: "ask" as const, - }, - })) as any - - const originalAsk = Permission.ask - const permissionAskMock = mock(async (input: any) => { - expect(input.type).toBe("edit") - // Throw to simulate user denial - throw new Error("Permission denied by user") - }) - Permission.ask = permissionAskMock - - try { - await using fixture = await tmpdir() - await Instance.provide({ - directory: fixture.path, - fn: async () => { - const editTool = await EditTool.init() - const testFile = path.join(fixture.path, "test.txt") - await fs.writeFile(testFile, "original content") - - // Mark file as read (required by FileTime.assert) - FileTime.read(ctx.sessionID, testFile) - - // Should throw because permission was denied - await expect( - editTool.execute( - { - filePath: testFile, - oldString: "original", - newString: "modified", - }, - ctx, - ), - ).rejects.toThrow("Permission denied by user") - - // Verify Permission.ask WAS called - expect(permissionAskMock).toHaveBeenCalled() - - // Verify file was NOT edited - const content = await fs.readFile(testFile, "utf-8") - expect(content).toBe("original content") - }, - }) - } finally { - Agent.get = originalGet - Permission.ask = originalAsk - } - }) -}) diff --git a/packages/opencode/test/tool/patch.test.ts b/packages/opencode/test/tool/patch.test.ts index 18e89cecb21..6d7d6db87f5 100644 --- a/packages/opencode/test/tool/patch.test.ts +++ b/packages/opencode/test/tool/patch.test.ts @@ -1,13 +1,10 @@ -import { describe, expect, test, mock } from "bun:test" +import { describe, expect, test } from "bun:test" import path from "path" import { PatchTool } from "../../src/tool/patch" import { Instance } from "../../src/project/instance" import { tmpdir } from "../fixture/fixture" import { Permission } from "../../src/permission" import * as fs from "fs/promises" -import { Log } from "../../src/util/log" - -Log.init({ print: false }) const ctx = { sessionID: "test", @@ -51,188 +48,20 @@ describe("tool.patch", () => { }) }) - test("should ask permission for files outside working directory (permission='ask', user denies)", async () => { - // Mock Permission.ask to verify it gets called with correct parameters - const permissionAskMock = mock(async (input: any) => { - // Verify the permission request has the right properties - expect(input.type).toBe("external_directory") - expect(input.title).toContain("/etc/passwd") - expect(input.sessionID).toBe(ctx.sessionID) - // Throw to simulate user denying permission - throw new Error("Permission denied by user") - }) - - const originalAsk = Permission.ask - Permission.ask = permissionAskMock - - try { - await Instance.provide({ - directory: "/tmp/opencode-test-" + Date.now(), - fn: async () => { - const patchTool = await PatchTool.init() - const maliciousPatch = `*** Begin Patch + test.skip("should ask permission for files outside working directory", async () => { + await Instance.provide({ + directory: "/tmp", + fn: async () => { + const maliciousPatch = `*** Begin Patch *** Add File: /etc/passwd +malicious content *** End Patch` - - // Should throw because permission was denied - await expect(patchTool.execute({ patchText: maliciousPatch }, ctx)).rejects.toThrow( - "Permission denied by user", - ) - - // Verify Permission.ask was called - expect(permissionAskMock).toHaveBeenCalled() - }, - }) - } finally { - // Restore original Permission.ask - Permission.ask = originalAsk - } - }) - - test("should allow files outside working directory when permission='allow'", async () => { - const { Agent } = await import("../../src/agent/agent") - const originalGet = Agent.get - - // Mock Agent.get to return agent with external_directory='allow' - Agent.get = mock(async () => ({ - permission: { - edit: "allow" as const, - bash: { "*": "allow" as const }, - webfetch: "allow" as const, - external_directory: "allow" as const, - doom_loop: "ask" as const, + patchTool.execute({ patchText: maliciousPatch }, ctx) + // TODO: this sucks + await new Promise((resolve) => setTimeout(resolve, 1000)) + expect(Permission.pending()[ctx.sessionID]).toBeDefined() }, - })) as any - - const originalAsk = Permission.ask - const permissionAskMock = mock(async () => {}) - Permission.ask = permissionAskMock - - try { - await using fixture = await tmpdir() - await Instance.provide({ - directory: fixture.path, - fn: async () => { - const patchTool = await PatchTool.init() - const externalPath = "/tmp/opencode-external-" + Date.now() + ".txt" - const patch = `*** Begin Patch -*** Add File: ${externalPath} -+external content -*** End Patch` - - // Should succeed without asking - const result = await patchTool.execute({ patchText: patch }, ctx) - expect(result.output).toContain("Patch applied successfully") - - // Verify Permission.ask was NOT called - expect(permissionAskMock).not.toHaveBeenCalled() - - // Clean up - await fs.unlink(externalPath).catch(() => {}) - }, - }) - } finally { - Agent.get = originalGet - Permission.ask = originalAsk - } - }) - - test("should deny files outside working directory when permission='deny'", async () => { - const { Agent } = await import("../../src/agent/agent") - const originalGet = Agent.get - - // Mock Agent.get to return agent with external_directory='deny' - Agent.get = mock(async () => ({ - permission: { - edit: "allow" as const, - bash: { "*": "allow" as const }, - webfetch: "allow" as const, - external_directory: "deny" as const, - doom_loop: "ask" as const, - }, - })) as any - - const originalAsk = Permission.ask - const permissionAskMock = mock(async () => {}) - Permission.ask = permissionAskMock - - try { - await using fixture = await tmpdir() - await Instance.provide({ - directory: fixture.path, - fn: async () => { - const patchTool = await PatchTool.init() - const externalPath = "/tmp/opencode-external-deny-test.txt" - const patch = `*** Begin Patch -*** Add File: ${externalPath} -+external content -*** End Patch` - - // Should throw without asking - await expect(patchTool.execute({ patchText: patch }, ctx)).rejects.toThrow( - "Permission denied: Cannot patch file outside working directory", - ) - - // Verify Permission.ask was NOT called - expect(permissionAskMock).not.toHaveBeenCalled() - }, - }) - } finally { - Agent.get = originalGet - Permission.ask = originalAsk - } - }) - - test("should ask and allow files outside working directory when permission='ask' and user approves", async () => { - const { Agent } = await import("../../src/agent/agent") - const originalGet = Agent.get - - // Mock Agent.get to return agent with external_directory='ask' - Agent.get = mock(async () => ({ - permission: { - edit: "allow" as const, - bash: { "*": "allow" as const }, - webfetch: "allow" as const, - external_directory: "ask" as const, - doom_loop: "ask" as const, - }, - })) as any - - const originalAsk = Permission.ask - const permissionAskMock = mock(async (input: any) => { - expect(input.type).toBe("external_directory") - // Resolve without throwing to simulate user approval }) - Permission.ask = permissionAskMock - - try { - await using fixture = await tmpdir() - await Instance.provide({ - directory: fixture.path, - fn: async () => { - const patchTool = await PatchTool.init() - const externalPath = "/tmp/opencode-external-ask-" + Date.now() + ".txt" - const patch = `*** Begin Patch -*** Add File: ${externalPath} -+external content -*** End Patch` - - // Should succeed after asking - const result = await patchTool.execute({ patchText: patch }, ctx) - expect(result.output).toContain("Patch applied successfully") - - // Verify Permission.ask WAS called - expect(permissionAskMock).toHaveBeenCalled() - - // Clean up - await fs.unlink(externalPath).catch(() => {}) - }, - }) - } finally { - Agent.get = originalGet - Permission.ask = originalAsk - } }) test("should handle simple add file operation", async () => { From acad66e2149fc13816a15058f5d7d7cd2be3fbd3 Mon Sep 17 00:00:00 2001 From: Aiden Cline Date: Tue, 18 Nov 2025 17:16:10 -0600 Subject: [PATCH 3/3] fixes --- packages/opencode/src/permission/index.ts | 7 ++++++- packages/opencode/src/session/processor.ts | 12 ++++++++++++ packages/opencode/src/tool/edit.ts | 11 +++++++++++ packages/opencode/src/tool/patch.ts | 11 +++++++++++ packages/opencode/src/tool/read.ts | 11 +++++++++++ packages/opencode/src/tool/write.ts | 11 +++++++++++ 6 files changed, 62 insertions(+), 1 deletion(-) diff --git a/packages/opencode/src/permission/index.ts b/packages/opencode/src/permission/index.ts index 3a4a9901b71..32dbd5a0370 100644 --- a/packages/opencode/src/permission/index.ts +++ b/packages/opencode/src/permission/index.ts @@ -186,8 +186,13 @@ export namespace Permission { public readonly permissionID: string, public readonly toolCallID?: string, public readonly metadata?: Record, + public readonly reason?: string, ) { - super(`The user rejected permission to use this specific tool call. You may try again with different parameters.`) + super( + reason !== undefined + ? reason + : `The user rejected permission to use this specific tool call. You may try again with different parameters.`, + ) } } } diff --git a/packages/opencode/src/session/processor.ts b/packages/opencode/src/session/processor.ts index 5c2005587b2..2af8e888eb6 100644 --- a/packages/opencode/src/session/processor.ts +++ b/packages/opencode/src/session/processor.ts @@ -136,6 +136,7 @@ export namespace SessionProcessor { const parts = await MessageV2.parts(input.assistantMessage.id) const lastThree = parts.slice(-DOOM_LOOP_THRESHOLD) + if ( lastThree.length === DOOM_LOOP_THRESHOLD && lastThree.every( @@ -160,6 +161,17 @@ export namespace SessionProcessor { input: value.input, }, }) + } else if (permission.doom_loop === "deny") { + throw new Permission.RejectedError( + input.assistantMessage.sessionID, + "doom_loop", + value.toolCallId, + { + tool: value.toolName, + input: value.input, + }, + `You seem to be stuck in a doom loop, please stop repeating the same action`, + ) } } } diff --git a/packages/opencode/src/tool/edit.ts b/packages/opencode/src/tool/edit.ts index ca9859370bc..46627a3edf5 100644 --- a/packages/opencode/src/tool/edit.ts +++ b/packages/opencode/src/tool/edit.ts @@ -57,6 +57,17 @@ export const EditTool = Tool.define("edit", { parentDir, }, }) + } else if (agent.permission.external_directory === "deny") { + throw new Permission.RejectedError( + ctx.sessionID, + "external_directory", + ctx.callID, + { + filepath: filePath, + parentDir, + }, + `File ${filePath} is not in the current working directory`, + ) } } diff --git a/packages/opencode/src/tool/patch.ts b/packages/opencode/src/tool/patch.ts index 0571cd35318..b8934f7c939 100644 --- a/packages/opencode/src/tool/patch.ts +++ b/packages/opencode/src/tool/patch.ts @@ -68,6 +68,17 @@ export const PatchTool = Tool.define("patch", { parentDir, }, }) + } else if (agent.permission.external_directory === "deny") { + throw new Permission.RejectedError( + ctx.sessionID, + "external_directory", + ctx.callID, + { + filepath: filePath, + parentDir, + }, + `File ${filePath} is not in the current working directory`, + ) } } diff --git a/packages/opencode/src/tool/read.ts b/packages/opencode/src/tool/read.ts index ec97c8a6c23..7e523496c37 100644 --- a/packages/opencode/src/tool/read.ts +++ b/packages/opencode/src/tool/read.ts @@ -46,6 +46,17 @@ export const ReadTool = Tool.define("read", { parentDir, }, }) + } else if (agent.permission.external_directory === "deny") { + throw new Permission.RejectedError( + ctx.sessionID, + "external_directory", + ctx.callID, + { + filepath: filepath, + parentDir, + }, + `File ${filepath} is not in the current working directory`, + ) } } diff --git a/packages/opencode/src/tool/write.ts b/packages/opencode/src/tool/write.ts index 58a0c177eb9..42bbe4690a6 100644 --- a/packages/opencode/src/tool/write.ts +++ b/packages/opencode/src/tool/write.ts @@ -36,6 +36,17 @@ export const WriteTool = Tool.define("write", { parentDir, }, }) + } else if (agent.permission.external_directory === "deny") { + throw new Permission.RejectedError( + ctx.sessionID, + "external_directory", + ctx.callID, + { + filepath: filepath, + parentDir, + }, + `File ${filepath} is not in the current working directory`, + ) } }