From 7f4918a1dec545a2dee549eb35cfec41d14f2571 Mon Sep 17 00:00:00 2001 From: Huarch Date: Fri, 25 Sep 2026 15:44:46 +0800 Subject: [PATCH] fix(agent): request approval for protected path access --- contracts/agent-v1.openapi.json | 2 +- contracts/manifest.json | 2 +- opencode.json | 59 ++++++++-------- src/contracts/openapi.ts | 2 +- src/routes/chat.ts | 2 +- src/routes/chatPermissionPolicy.ts | 44 ++++++++++-- tests/routes/chatPermissionPolicy.test.ts | 64 ++++++++++++++--- tests/routes/chatStream.test.ts | 83 +++++++++++++++++++---- tests/runtime/opencodeConfig.test.ts | 34 ++++++---- 9 files changed, 214 insertions(+), 78 deletions(-) diff --git a/contracts/agent-v1.openapi.json b/contracts/agent-v1.openapi.json index 04b46ba..cd18185 100644 --- a/contracts/agent-v1.openapi.json +++ b/contracts/agent-v1.openapi.json @@ -1014,7 +1014,7 @@ "auto", "always" ], - "description": "request forwards approval prompts; auto approves only the low-risk allowlist; always approves every prompt not explicitly denied by OpenCode." + "description": "request forwards approval prompts; auto approves only the low-risk allowlist and forwards other requests; always approves routine prompts but forwards sensitive paths and force removal for user approval." } }, "required": [ diff --git a/contracts/manifest.json b/contracts/manifest.json index f89affa..8c1f4f1 100644 --- a/contracts/manifest.json +++ b/contracts/manifest.json @@ -3,7 +3,7 @@ "contracts": { "agent": { "file": "agent-v1.openapi.json", - "sha256": "94bd8914597c56b6429160e8c556993ac0617ad079de2980a4b6cb9fdf89c039" + "sha256": "dbfaec4b72eb30592523f7c5a21097d4cfb2e3beb370540c6da24b3d4aa5b366" } } } diff --git a/opencode.json b/opencode.json index 5b8b025..107b7df 100644 --- a/opencode.json +++ b/opencode.json @@ -14,42 +14,45 @@ }, "permission": { "*": "ask", - "external_directory": "deny", + "external_directory": { + "*": "ask", + "/app/.opencode/skills/*": "allow" + }, "read": { "*": "allow", - ".env": "deny", - ".env.*": "deny", - "*.env": "deny", - "**/.env": "deny", - "**/.env.*": "deny", - "**/*.env": "deny", - "data/**": "deny", - "**/data/**": "deny", - "logs/**": "deny", - "**/logs/**": "deny" + ".env": "ask", + ".env.*": "ask", + "*.env": "ask", + "**/.env": "ask", + "**/.env.*": "ask", + "**/*.env": "ask", + "data/**": "ask", + "**/data/**": "ask", + "logs/**": "ask", + "**/logs/**": "ask" }, "edit": { "*": "ask", - ".env": "deny", - ".env.*": "deny", - "*.env": "deny", - "**/.env": "deny", - "**/.env.*": "deny", - "**/*.env": "deny", - "data/**": "deny", - "**/data/**": "deny", - "logs/**": "deny", - "**/logs/**": "deny" + ".env": "ask", + ".env.*": "ask", + "*.env": "ask", + "**/.env": "ask", + "**/.env.*": "ask", + "**/*.env": "ask", + "data/**": "ask", + "**/data/**": "ask", + "logs/**": "ask", + "**/logs/**": "ask" }, "bash": { "*": "ask", "rm *": "ask", - "rm -rf *": "deny", - "rm -fr *": "deny", - "rm -r -f *": "deny", - "rm -f -r *": "deny", - "rm --recursive --force *": "deny", - "rm --force --recursive *": "deny", + "rm -rf *": "ask", + "rm -fr *": "ask", + "rm -r -f *": "ask", + "rm -f -r *": "ask", + "rm --recursive --force *": "ask", + "rm --force --recursive *": "ask", "rmdir *": "ask", "mv *": "ask", "chmod *": "ask", @@ -57,7 +60,7 @@ "sudo *": "ask", "curl *": "ask", "wget *": "ask", - "*.env*": "deny" + "*.env*": "ask" }, "question": "allow", "activity_update": "allow", diff --git a/src/contracts/openapi.ts b/src/contracts/openapi.ts index 67587c4..754be43 100644 --- a/src/contracts/openapi.ts +++ b/src/contracts/openapi.ts @@ -218,7 +218,7 @@ register("/api/v1/agent/sessions/{session_id}/runs", "post", { .enum(["request", "auto", "always"]) .optional() .describe( - "request forwards approval prompts; auto approves only the low-risk allowlist; always approves every prompt not explicitly denied by OpenCode.", + "request forwards approval prompts; auto approves only the low-risk allowlist and forwards other requests; always approves routine prompts but forwards sensitive paths and force removal for user approval.", ), }), }, diff --git a/src/routes/chat.ts b/src/routes/chat.ts index 6610254..30e36e9 100644 --- a/src/routes/chat.ts +++ b/src/routes/chat.ts @@ -74,7 +74,7 @@ const payloadSchema = z.object({ .optional() .default("request") .describe( - "request forwards approval prompts; auto approves only low-risk allowlisted tools; always approves every prompt not explicitly denied by OpenCode", + "request forwards approval prompts; auto approves only low-risk allowlisted tools and forwards other requests; always approves routine prompts but forwards sensitive paths and force removal for user approval", ), }); diff --git a/src/routes/chatPermissionPolicy.ts b/src/routes/chatPermissionPolicy.ts index edcc344..f4b70b1 100644 --- a/src/routes/chatPermissionPolicy.ts +++ b/src/routes/chatPermissionPolicy.ts @@ -47,7 +47,7 @@ export const canAutoApprovePermission = ( } if (normalized === "bash") { - return isConversationWorkspace(context.workspaceRoot); + return isConversationWorkspace(context.workspaceRoot) && !isSensitiveBashCommand(context); } if (normalized === "read" || normalized === "edit" || normalized === "write") { @@ -70,12 +70,20 @@ export const resolvePermissionApproval = ( permission: string, context: PermissionApprovalContext = {}, ) => { - if (isDirectRecursiveForceRemove(permission, context)) { + const normalized = normalizePermission(permission); + if ( + isSensitiveBashCommand(context, normalized) || + (approvalMode === "always" && + (normalized === "external_directory" || + normalized === "read" || + ((normalized === "edit" || normalized === "write") && + !isSafeConversationFileAccess(context)))) + ) { return { autoApprove: false, - autoReject: true, - title: "已拒绝递归强制删除", - detail: "当前安全策略禁止直接执行带 recursive 和 force 参数的 rm 命令。", + autoReject: false, + title: "等待权限确认", + detail: undefined, } as const; } @@ -85,7 +93,7 @@ export const resolvePermissionApproval = ( autoReject: false, title: "已按始终允许模式放行", detail: - "当前会话处于始终允许模式，已放行本次权限请求；明确禁止的权限仍由 OpenCode 拒绝。", + "当前会话处于始终允许模式，已放行本次常规权限请求。", } as const; } @@ -106,6 +114,21 @@ export const resolvePermissionApproval = ( } as const; }; +const isSensitiveBashCommand = ( + context: PermissionApprovalContext, + normalizedPermission = "bash", +): boolean => { + if (normalizedPermission !== "bash") return false; + const command = + typeof context.metadata?.command === "string" + ? context.metadata.command + : context.patterns?.join("\n"); + return Boolean( + (command && containsProtectedPath(command, true)) || + isDirectRecursiveForceRemove("bash", context), + ); +}; + const isDirectRecursiveForceRemove = ( permission: string, context: PermissionApprovalContext, @@ -412,9 +435,16 @@ const isSafeConversationFileAccess = ( relativeTarget !== ".." && !relativeTarget.startsWith(`..${sep}`) && !isAbsolute(relativeTarget) && - !containsProtectedPath(relativeTarget, true) + !containsSensitiveFileSegment(relativeTarget) ); } catch { return false; } }; + +const containsSensitiveFileSegment = (value: string): boolean => + value + .replaceAll("\\", "/") + .toLowerCase() + .split("/") + .some((segment) => segment.includes(".env") || segment === "data" || segment === "logs"); diff --git a/tests/routes/chatPermissionPolicy.test.ts b/tests/routes/chatPermissionPolicy.test.ts index ff719d4..44c9fac 100644 --- a/tests/routes/chatPermissionPolicy.test.ts +++ b/tests/routes/chatPermissionPolicy.test.ts @@ -124,10 +124,22 @@ describe("permission approval policy", () => { expect(resolvePermissionApproval("request", "show_chart").autoApprove).toBe(false); expect(resolvePermissionApproval("auto", "show_chart").autoApprove).toBe(true); expect(resolvePermissionApproval("auto", "bash").autoApprove).toBe(false); + expect(resolvePermissionApproval("auto", "external_directory")).toMatchObject({ + autoApprove: false, + autoReject: false, + }); expect(resolvePermissionApproval("always", "bash")).toMatchObject({ autoApprove: true, title: "已按始终允许模式放行", }); + expect(resolvePermissionApproval("always", "external_directory")).toMatchObject({ + autoApprove: false, + autoReject: false, + }); + expect(resolvePermissionApproval("always", "read")).toMatchObject({ + autoApprove: false, + autoReject: false, + }); }); it("auto approves sandboxed shell and file access in a conversation workspace", async () => { @@ -142,12 +154,40 @@ describe("permission approval policy", () => { metadata: { command: "python3 analysis.py" }, }), ).toMatchObject({ autoApprove: true, autoReject: false }); + for (const approvalMode of ["auto", "always"] as const) { + expect( + resolvePermissionApproval(approvalMode, "bash", { + workspaceRoot, + metadata: { command: "cat .env" }, + }), + ).toMatchObject({ autoApprove: false, autoReject: false }); + } expect( canAutoApprovePermission("edit", { workspaceRoot, metadata: { filePath: join(workspaceRoot, "result.json") }, }), ).toBe(true); + expect( + resolvePermissionApproval("always", "edit", { + workspaceRoot, + metadata: { filePath: join(workspaceRoot, "result.json") }, + }), + ).toMatchObject({ autoApprove: true, autoReject: false }); + for (const filePath of [".env", "data/private.json", "logs/latest.log"]) { + expect( + resolvePermissionApproval("auto", "edit", { + workspaceRoot, + metadata: { filePath: join(workspaceRoot, filePath) }, + }), + ).toMatchObject({ autoApprove: false, autoReject: false }); + expect( + resolvePermissionApproval("always", "edit", { + workspaceRoot, + metadata: { filePath: join(workspaceRoot, filePath) }, + }), + ).toMatchObject({ autoApprove: false, autoReject: false }); + } expect( canAutoApprovePermission("glob", { workspaceRoot, @@ -173,17 +213,19 @@ describe("permission approval policy", () => { "(rm -rf ./target)", "! rm -rf ./target", "npm test && rm --recursive --force ./target", - ])("rejects direct recursive force removal in always mode: %s", (command) => { - expect( - resolvePermissionApproval("always", "bash", { - metadata: { command }, - patterns: [command], - }), - ).toMatchObject({ - autoApprove: false, - autoReject: true, - title: "已拒绝递归强制删除", - }); + ])("asks before recursive force removal in always mode: %s", (command) => { + for (const approvalMode of ["auto", "always"] as const) { + expect( + resolvePermissionApproval(approvalMode, "bash", { + metadata: { command }, + patterns: [command], + }), + ).toMatchObject({ + autoApprove: false, + autoReject: false, + title: "等待权限确认", + }); + } }); it.each(["rm tmp.txt", "rm -f tmp.txt", "rm -r tmp-dir", "echo 'rm -rf tmp'"])( diff --git a/tests/routes/chatStream.test.ts b/tests/routes/chatStream.test.ts index 40ad4d9..662b911 100644 --- a/tests/routes/chatStream.test.ts +++ b/tests/routes/chatStream.test.ts @@ -584,6 +584,28 @@ describe("streamPromptResponse", () => { always: ["npm test"], }, }, + { + type: "permission.asked", + properties: { + id: "perm-auto-external", + sessionID: "runtime-session-1", + permission: "external_directory", + patterns: ["/app/opencode.json/*"], + metadata: { filepath: "/app/opencode.json" }, + always: ["/app/opencode.json/*"], + }, + }, + { + type: "permission.asked", + properties: { + id: "perm-auto-env", + sessionID: "runtime-session-1", + permission: "read", + patterns: [".env"], + metadata: { filepath: ".env" }, + always: [".env"], + }, + }, { type: "session.idle", properties: { sessionID: "runtime-session-1" } }, ]), prompt: async () => undefined, @@ -606,9 +628,19 @@ describe("streamPromptResponse", () => { request_id: "perm-auto-bash", permission: "bash", }); + expect(events.filter((item) => item.event === "permission_request")[1]?.data).toMatchObject({ + request_id: "perm-auto-external", + permission: "external_directory", + target: "/app/opencode.json", + }); + expect(events.filter((item) => item.event === "permission_request")[2]?.data).toMatchObject({ + request_id: "perm-auto-env", + permission: "read", + target: ".env", + }); }); - it("approves every OpenCode ask once in always mode", async () => { + it("approves routine asks but forwards external paths in always mode", async () => { const replies: Array> = []; const runtime = { subscribeEvents: async () => @@ -624,6 +656,28 @@ describe("streamPromptResponse", () => { always: ["npm test"], }, }, + { + type: "permission.asked", + properties: { + id: "perm-always-external", + sessionID: "runtime-session-1", + permission: "external_directory", + patterns: ["/app/private/*"], + metadata: { filepath: "/app/private/config.json" }, + always: ["/app/private/*"], + }, + }, + { + type: "permission.asked", + properties: { + id: "perm-always-env", + sessionID: "runtime-session-1", + permission: "read", + patterns: [".env"], + metadata: { filepath: ".env" }, + always: [".env"], + }, + }, { type: "session.idle", properties: { sessionID: "runtime-session-1" } }, ]), prompt: async () => undefined, @@ -650,10 +704,17 @@ describe("streamPromptResponse", () => { reply: "once", }, ]); - expect(events.some((item) => item.event === "permission_request")).toBe(false); + expect(events.find((item) => item.event === "permission_request")?.data).toMatchObject({ + request_id: "perm-always-external", + permission: "external_directory", + }); + expect(events.filter((item) => item.event === "permission_request")[1]?.data).toMatchObject({ + request_id: "perm-always-env", + permission: "read", + }); }); - it("rejects recursive force removal even in always mode", async () => { + it("asks before recursive force removal even in always mode", async () => { const replies: Array> = []; const runtime = { subscribeEvents: async () => @@ -687,19 +748,10 @@ describe("streamPromptResponse", () => { write: (event, data) => events.push({ event, data }), }); - expect(replies).toEqual([ - { - requestId: "perm-always-rm-rf", - sessionId: "runtime-session-1", - directory: "/tmp/conversation-workspace-1", - reply: "reject", - }, - ]); - expect(events.some((item) => item.event === "permission_request")).toBe(false); - expect(events.find((item) => item.event === "permission_response")?.data).toEqual({ - session_id: "client-session-1", + expect(replies).toEqual([]); + expect(events.find((item) => item.event === "permission_request")?.data).toMatchObject({ request_id: "perm-always-rm-rf", - reply: "reject", + permission: "bash", }); }); @@ -735,6 +787,7 @@ describe("streamPromptResponse", () => { sessionId: "runtime-session-1", clientSessionId: "client-session-1", message: "read /tmp", + approvalMode: "auto", write: (event, data) => events.push({ event, data }), }); diff --git a/tests/runtime/opencodeConfig.test.ts b/tests/runtime/opencodeConfig.test.ts index bdf8fa1..98c911e 100644 --- a/tests/runtime/opencodeConfig.test.ts +++ b/tests/runtime/opencodeConfig.test.ts @@ -24,7 +24,7 @@ describe("internal OpenCode permissions", () => { expect(dockerfile.startsWith("FROM smanx/opencode:1.18.13@")).toBe(true); }); - it("keeps protected paths denied in every approval mode", async () => { + it("sends protected paths to approval while allowing mounted skills", async () => { const config = JSON.parse(await readFile("opencode.json", "utf8")) as { permission?: Record>; }; @@ -34,27 +34,35 @@ describe("internal OpenCode permissions", () => { const read = permission.read as Record | undefined; expect(permission["*"]).toBe("ask"); - expect(permission.external_directory).toBe("deny"); + expect(Object.entries(permission.external_directory ?? {})).toEqual([ + ["*", "ask"], + ["/app/.opencode/skills/*", "allow"], + ]); expect(permission.task).toBe("deny"); expect(permission.question).toBe("allow"); expect(permission.activity_update).toBe("allow"); expect(permission.final_answer).toBe("allow"); expect(permission.todowrite).toBe("allow"); expect(read?.["*"]).toBe("allow"); - expect(read?.["data/**"]).toBe("deny"); - expect(read?.["**/logs/**"]).toBe("deny"); + expect(read?.[".env"]).toBe("ask"); + expect(read?.["data/**"]).toBe("ask"); + expect(read?.["**/logs/**"]).toBe("ask"); + expect(Object.values(read ?? {})).not.toContain("deny"); expect(edit?.["*"]).toBe("ask"); - expect(edit?.["data/**"]).toBe("deny"); - expect(edit?.["**/logs/**"]).toBe("deny"); + expect(edit?.[".env"]).toBe("ask"); + expect(edit?.["data/**"]).toBe("ask"); + expect(edit?.["**/logs/**"]).toBe("ask"); + expect(Object.values(edit ?? {})).not.toContain("deny"); expect(bash?.["*"]).toBe("ask"); expect(bash?.["rm *"]).toBe("ask"); - expect(bash?.["rm -rf *"]).toBe("deny"); - expect(bash?.["rm -fr *"]).toBe("deny"); - expect(bash?.["rm -r -f *"]).toBe("deny"); - expect(bash?.["rm -f -r *"]).toBe("deny"); - expect(bash?.["rm --recursive --force *"]).toBe("deny"); - expect(bash?.["rm --force --recursive *"]).toBe("deny"); - expect(bash?.["*.env*"]).toBe("deny"); + expect(bash?.["rm -rf *"]).toBe("ask"); + expect(bash?.["rm -fr *"]).toBe("ask"); + expect(bash?.["rm -r -f *"]).toBe("ask"); + expect(bash?.["rm -f -r *"]).toBe("ask"); + expect(bash?.["rm --recursive --force *"]).toBe("ask"); + expect(bash?.["rm --force --recursive *"]).toBe("ask"); + expect(bash?.["*.env*"]).toBe("ask"); + expect(Object.values(bash ?? {})).not.toContain("deny"); expect(bash?.["*data/*"]).toBeUndefined(); expect(bash?.["*logs/*"]).toBeUndefined(); });