diff --git a/.changeset/strict-ask-plan-guards.md b/.changeset/strict-ask-plan-guards.md index d46736109b6..9f886644a00 100644 --- a/.changeset/strict-ask-plan-guards.md +++ b/.changeset/strict-ask-plan-guards.md @@ -2,4 +2,4 @@ "@kilocode/cli": patch --- -Prevent Ask and Plan modes from editing files before an explicit implementation step. +Prevent Ask and Plan modes, including saved or allow-all approvals, from editing files before an explicit implementation step. diff --git a/packages/opencode/src/kilocode/permission/drain.ts b/packages/opencode/src/kilocode/permission/drain.ts index f91df26b5a1..df35c90b4fc 100644 --- a/packages/opencode/src/kilocode/permission/drain.ts +++ b/packages/opencode/src/kilocode/permission/drain.ts @@ -6,6 +6,7 @@ import { ConfigProtection } from "@/kilocode/permission/config-paths" interface PendingEntry { info: Permission.Request ruleset: Permission.Ruleset + hardRuleset?: Permission.Ruleset deferred: Deferred.Deferred } @@ -25,9 +26,12 @@ export function drainCovered( if (id === exclude) continue // Never auto-resolve config file edit permissions if (ConfigProtection.isRequest(entry.info)) continue - const actions = entry.info.patterns.map((pattern: string) => - Permission.evaluate(entry.info.permission, pattern, entry.ruleset, approved), - ) + const actions = entry.info.patterns.map((pattern: string) => { + const rule = Permission.evaluate(entry.info.permission, pattern, entry.ruleset, approved) + const hard = entry.hardRuleset ? Permission.evaluate(entry.info.permission, pattern, entry.hardRuleset) : undefined + if (hard?.action === "deny") return hard + return rule + }) const denied = actions.some((r: Permission.Rule) => r.action === "deny") const allowed = !denied && actions.every((r: Permission.Rule) => r.action === "allow") if (!denied && !allowed) continue diff --git a/packages/opencode/src/kilocode/session/prompt.ts b/packages/opencode/src/kilocode/session/prompt.ts index 8dcb90e08d9..d8b1b2b7d97 100644 --- a/packages/opencode/src/kilocode/session/prompt.ts +++ b/packages/opencode/src/kilocode/session/prompt.ts @@ -64,6 +64,11 @@ export namespace KiloSessionPrompt { return Permission.merge(rules, input.agent.permission, rules.filter((rule) => rule.action === "deny")) } + export function hardPermissions(input: { agent: { name: string; permission: Permission.Ruleset } }) { + if (!["ask", "plan"].includes(input.agent.name)) return + return input.agent.permission + } + /** * Mutable cache for environment details, keyed by user message ID * so it recomputes when a new user message arrives. diff --git a/packages/opencode/src/permission/index.ts b/packages/opencode/src/permission/index.ts index f8bde8a8485..46a3190fcf2 100644 --- a/packages/opencode/src/permission/index.ts +++ b/packages/opencode/src/permission/index.ts @@ -120,6 +120,7 @@ export const AskInput = Schema.Struct({ ...Request.fields, id: Schema.optional(PermissionID), ruleset: Ruleset, + hardRuleset: Schema.optional(Ruleset), // kilocode_change }) .annotate({ identifier: "PermissionAskInput" }) .pipe(withStatics((s) => ({ zod: zod(s) }))) @@ -159,6 +160,7 @@ export interface Interface { interface PendingEntry { info: Request ruleset: Ruleset // kilocode_change + hardRuleset?: Ruleset // kilocode_change deferred: Deferred.Deferred } @@ -173,6 +175,17 @@ export function evaluate(permission: string, pattern: string, ...rulesets: Rules return evalRule(permission, pattern, ...rulesets) } +// kilocode_change start +function veto(permission: string, pattern: string, ruleset?: Ruleset) { + if (!ruleset) return false + return evaluate(permission, pattern, ruleset).action === "deny" +} + +function subset(permission: string, ruleset: Ruleset) { + return ruleset.filter((rule) => Wildcard.match(permission, rule.permission)) +} +// kilocode_change end + export class Service extends Context.Service()("@opencode/Permission") {} export const layer = Layer.effect( @@ -205,7 +218,7 @@ export const layer = Layer.effect( const ask = Effect.fn("Permission.ask")(function* (input: AskInput) { const { approved, pending } = yield* InstanceState.get(state) - const { ruleset, ...request } = input + const { ruleset, hardRuleset, ...request } = input // kilocode_change const s = yield* InstanceState.get(state) // kilocode_change const local = s.session[request.sessionID] ?? [] // kilocode_change let needsAsk = false @@ -217,9 +230,14 @@ export const layer = Layer.effect( for (const pattern of request.patterns) { const rule = evaluate(request.permission, pattern, ruleset, approved, local) // kilocode_change — include session-scoped rules log.info("evaluated", { permission: request.permission, pattern, action: rule }) + // kilocode_change start — saved/session approvals cannot override hard Ask/Plan denials + if (veto(request.permission, pattern, hardRuleset)) { + return yield* new DeniedError({ ruleset: subset(request.permission, hardRuleset ?? []) }) + } + // kilocode_change end if (rule.action === "deny") { return yield* new DeniedError({ - ruleset: ruleset.filter((rule) => Wildcard.match(request.permission, rule.permission)), + ruleset: subset(request.permission, ruleset), // kilocode_change }) } // kilocode_change start — override "allow" to "ask" for config paths @@ -244,7 +262,7 @@ export const layer = Layer.effect( log.info("asking", { id, permission: info.permission, patterns: info.patterns }) const deferred = yield* Deferred.make() - pending.set(id, { info, ruleset, deferred }) // kilocode_change + pending.set(id, { info, ruleset, hardRuleset, deferred }) // kilocode_change yield* bus.publish(Event.Asked, info) return yield* Effect.ensuring( Deferred.await(deferred), @@ -302,9 +320,11 @@ export const layer = Layer.effect( for (const [id, item] of pending.entries()) { if (item.info.sessionID !== existing.info.sessionID) continue - const ok = item.info.patterns.every( - (pattern) => evaluate(item.info.permission, pattern, item.ruleset, approved).action === "allow", // kilocode_change — include original ruleset - ) + if (ConfigProtection.isRequest(item.info)) continue // kilocode_change + const ok = item.info.patterns.every((pattern) => { + if (veto(item.info.permission, pattern, item.hardRuleset)) return false // kilocode_change + return evaluate(item.info.permission, pattern, item.ruleset, approved).action === "allow" // kilocode_change — include original ruleset + }) if (!ok) continue pending.delete(id) yield* bus.publish(Event.Replied, { @@ -390,7 +410,10 @@ export const layer = Layer.effect( if (input.requestID) { const entry = s.pending.get(PermissionID.make(input.requestID)) - if (entry && (!input.sessionID || entry.info.sessionID === input.sessionID)) { + const ok = entry + ? entry.info.patterns.every((pattern) => !veto(entry.info.permission, pattern, entry.hardRuleset)) + : false // kilocode_change + if (entry && ok && (!input.sessionID || entry.info.sessionID === input.sessionID)) { s.pending.delete(PermissionID.make(input.requestID)) yield* bus.publish(Event.Replied, { sessionID: entry.info.sessionID, @@ -404,6 +427,7 @@ export const layer = Layer.effect( for (const [id, entry] of s.pending) { if (input.sessionID && entry.info.sessionID !== input.sessionID) continue if (ConfigProtection.isRequest(entry.info)) continue + if (entry.info.patterns.some((pattern) => veto(entry.info.permission, pattern, entry.hardRuleset))) continue // kilocode_change s.pending.delete(id) yield* bus.publish(Event.Replied, { sessionID: entry.info.sessionID, diff --git a/packages/opencode/src/session/prompt.ts b/packages/opencode/src/session/prompt.ts index 443db3e0064..83d9d5e9ab5 100644 --- a/packages/opencode/src/session/prompt.ts +++ b/packages/opencode/src/session/prompt.ts @@ -405,6 +405,7 @@ NOTE: At any point in time through this workflow you should feel free to ask the input.agent.permission, KiloSessionPrompt.guardPermissions({ agent: input.agent, session: input.session }), ), + hardRuleset: KiloSessionPrompt.hardPermissions({ agent: input.agent }), // kilocode_change end }) .pipe(Effect.orDie), @@ -624,12 +625,13 @@ NOTE: At any point in time through this workflow you should feel free to ask the permission .ask({ ...req, - sessionID, // kilocode_change start - reapply Ask/Plan subagent guards after session permissions + sessionID, ruleset: Permission.merge( taskAgent.permission, KiloSessionPrompt.guardPermissions({ agent: taskAgent, session }), ), + hardRuleset: KiloSessionPrompt.hardPermissions({ agent: taskAgent }), // kilocode_change end }) .pipe(Effect.orDie), @@ -1391,12 +1393,14 @@ NOTE: At any point in time through this workflow you should feel free to ask the const lastAssistantMsg = msgs.findLast( (msg) => msg.info.role === "assistant" && msg.info.id === lastAssistant?.id, ) + // kilocode_change start - keep provider-executed tools from forcing a re-loop // Some providers return "stop" even when the assistant message contains tool calls. // Keep the loop running so tool results can be sent back to the model. // Skip provider-executed tool parts — those were fully handled within the // provider's stream (e.g. DWS Agent Platform) and don't need a re-loop. const hasToolCalls = lastAssistantMsg?.parts.some((part) => part.type === "tool" && !part.metadata?.providerExecuted) ?? false + // kilocode_change end // kilocode_change start - plan_exit is a hard stop before another model call if ( @@ -1583,11 +1587,11 @@ NOTE: At any point in time through this workflow you should feel free to ask the ]) const system = [...env, ...(skills ? [skills] : []), ...instructions] const format = lastUser.format ?? { type: "text" as const } - if (format.type === "json_schema") system.push(STRUCTURED_OUTPUT_SYSTEM_PROMPT) - const result = yield* handle.process({ + if (format.type === "json_schema") system.push(STRUCTURED_OUTPUT_SYSTEM_PROMPT) // kilocode_change + const result = yield* handle.process({ // kilocode_change + // kilocode_change start - keep Ask/Plan tool filtering hardened against session allows user: lastUser, agent, - // kilocode_change start - keep Ask/Plan tool filtering hardened against session allows permission: KiloSessionPrompt.guardPermissions({ agent, session }), // kilocode_change end sessionID, diff --git a/packages/opencode/test/kilocode/permission/next.always-rules.test.ts b/packages/opencode/test/kilocode/permission/next.always-rules.test.ts index 60e99fa5f8c..53789c4a501 100644 --- a/packages/opencode/test/kilocode/permission/next.always-rules.test.ts +++ b/packages/opencode/test/kilocode/permission/next.always-rules.test.ts @@ -239,6 +239,76 @@ describe("saveAlwaysRules", () => { ), ) + it.live("saved always approval does not override hard deny ruleset", () => + withDir({ git: true }, () => + Effect.gen(function* () { + const asking = yield* ask({ + id: PermissionID.make("permission_hard_deny_seed"), + sessionID: SessionID.make("session_test"), + permission: "bash", + patterns: ["printf seed"], + metadata: {}, + always: ["printf *"], + ruleset: [{ permission: "bash", pattern: "*", action: "ask" }], + }).pipe(Effect.forkScoped) + + yield* waitForPending(1) + yield* reply({ requestID: PermissionID.make("permission_hard_deny_seed"), reply: "always" }) + yield* Fiber.join(asking) + + const exit = yield* ask({ + sessionID: SessionID.make("session_test"), + permission: "bash", + patterns: ["printf bypass > ask-saved-bypass.txt"], + metadata: {}, + always: [], + ruleset: [{ permission: "bash", pattern: "*", action: "ask" }], + hardRuleset: [ + { permission: "bash", pattern: "*", action: "deny" }, + { permission: "bash", pattern: "printf *", action: "allow" }, + { permission: "bash", pattern: "*>*", action: "deny" }, + { permission: "bash", pattern: "* > *", action: "deny" }, + ], + }).pipe(Effect.exit) + expectFailure(exit, Permission.DeniedError) + }), + ), + ) + + it.live("saved always approval still works when hard ruleset does not deny", () => + withDir({ git: true }, () => + Effect.gen(function* () { + const asking = yield* ask({ + id: PermissionID.make("permission_hard_ask_seed"), + sessionID: SessionID.make("session_test"), + permission: "bash", + patterns: ["gh issue list"], + metadata: {}, + always: ["gh *"], + ruleset: [{ permission: "bash", pattern: "*", action: "ask" }], + }).pipe(Effect.forkScoped) + + yield* waitForPending(1) + yield* reply({ requestID: PermissionID.make("permission_hard_ask_seed"), reply: "always" }) + yield* Fiber.join(asking) + + const result = yield* ask({ + sessionID: SessionID.make("session_test"), + permission: "bash", + patterns: ["gh pr list"], + metadata: {}, + always: [], + ruleset: [{ permission: "bash", pattern: "*", action: "ask" }], + hardRuleset: [ + { permission: "bash", pattern: "*", action: "deny" }, + { permission: "bash", pattern: "gh *", action: "ask" }, + ], + }) + expect(result).toBeUndefined() + }), + ), + ) + it.live("accepts hierarchy patterns from metadata.rules", () => withDir({ git: true }, () => Effect.gen(function* () { diff --git a/packages/opencode/test/session/prompt-effect.test.ts b/packages/opencode/test/session/prompt-effect.test.ts index 304d4f0baff..bee736c7dd4 100644 --- a/packages/opencode/test/session/prompt-effect.test.ts +++ b/packages/opencode/test/session/prompt-effect.test.ts @@ -105,6 +105,7 @@ function errorTool(parts: MessageV2.Part[]) { return part?.state.status === "error" ? (part as ErrorToolPart) : undefined } +// kilocode_change start - helpers for Ask/Plan guard regression tests function names(input: Record | undefined) { const tools = input?.tools if (!Array.isArray(tools)) return [] @@ -129,6 +130,7 @@ const waitQuestion = Effect.fn("test.waitQuestion")(function* (sessionID: Sessio yield* Effect.sleep("10 millis") } }) +// kilocode_change end const mcp = Layer.succeed( MCP.Service, @@ -538,6 +540,7 @@ it.live("glob tool keeps instance context during prompt runs", () => ), ) +// kilocode_change start - assistant tool parts should continue loop despite stop finish it.live("loop continues when finish is stop but assistant has tool parts", () => provideTmpdirServer( Effect.fnUntraced(function* ({ llm }) { @@ -567,7 +570,9 @@ it.live("loop continues when finish is stop but assistant has tool parts", () => { git: true, config: providerCfg }, ), ) +// kilocode_change end +// kilocode_change start - Ask mode guard coverage unix("ask agent denies bash redirection during prompt loop", () => provideTmpdirServer( ({ dir, llm }) => @@ -631,6 +636,167 @@ it.live("ask agent hides edit tools despite session allow-all", () => { git: true, config: providerCfg }, ), ) +// kilocode_change end + +// kilocode_change start - saved permissions must not bypass Ask/Plan guards +unix("ask agent denies bash redirection despite global allow-everything", () => + provideTmpdirServer( + ({ dir, llm }) => + withSh(() => + Effect.gen(function* () { + const permission = yield* Permission.Service + const prompt = yield* SessionPrompt.Service + const sessions = yield* Session.Service + const session = yield* sessions.create({ title: "Ask global guard" }) + const file = path.join(dir, "ask-global-bug.txt") + + yield* permission.allowEverything({ enable: true }) + yield* prompt.prompt({ + sessionID: session.id, + agent: "ask", + noReply: true, + parts: [{ type: "text", text: "Explain the repo" }], + }) + yield* llm.tool("bash", { command: "echo bug > ask-global-bug.txt", description: "write ask bug" }) + yield* llm.text("done") + + yield* prompt.loop({ sessionID: session.id }) + yield* permission.allowEverything({ enable: false }) + expect(yield* Effect.promise(() => Bun.file(file).exists())).toBe(false) + + const msgs = yield* MessageV2.filterCompactedEffect(session.id) + const tool = msgs + .flatMap((msg) => msg.parts) + .find((part): part is MessageV2.ToolPart => part.type === "tool" && part.tool === "bash") + expect(tool?.state.status).toBe("error") + }), + ), + { git: true, config: providerCfg }, + ), +) + +unix("ask agent denies bash redirection despite session allow-everything", () => + provideTmpdirServer( + ({ dir, llm }) => + withSh(() => + Effect.gen(function* () { + const permission = yield* Permission.Service + const prompt = yield* SessionPrompt.Service + const sessions = yield* Session.Service + const session = yield* sessions.create({ + title: "Ask local guard", + permission: [{ permission: "*", pattern: "*", action: "allow" }], + }) + const file = path.join(dir, "ask-session-bug.txt") + + yield* permission.allowEverything({ enable: true, sessionID: session.id }) + yield* prompt.prompt({ + sessionID: session.id, + agent: "ask", + noReply: true, + parts: [{ type: "text", text: "Explain the repo" }], + }) + yield* llm.tool("bash", { command: "echo bug > ask-session-bug.txt", description: "write ask bug" }) + yield* llm.text("done") + + yield* prompt.loop({ sessionID: session.id }) + yield* permission.allowEverything({ enable: false, sessionID: session.id }) + expect(yield* Effect.promise(() => Bun.file(file).exists())).toBe(false) + + const msgs = yield* MessageV2.filterCompactedEffect(session.id) + const tool = msgs + .flatMap((msg) => msg.parts) + .find((part): part is MessageV2.ToolPart => part.type === "tool" && part.tool === "bash") + expect(tool?.state.status).toBe("error") + }), + ), + { git: true, config: providerCfg }, + ), +) + +it.live("plan agent denies non-plan write despite global allow-everything", () => + provideTmpdirServer( + ({ dir, llm }) => + Effect.gen(function* () { + const permission = yield* Permission.Service + const prompt = yield* SessionPrompt.Service + const sessions = yield* Session.Service + const session = yield* sessions.create({ title: "Plan global guard" }) + const plan = Session.plan(session) + const file = path.join(dir, "plan-global-bug.txt") + + yield* permission.allowEverything({ enable: true }) + yield* prompt.prompt({ + sessionID: session.id, + agent: "plan", + noReply: true, + parts: [{ type: "text", text: "Create a plan only" }], + }) + yield* llm.tool("write", { filePath: plan, content: "# Plan\n\n- Stay in plan mode.\n" }) + yield* llm.tool("write", { filePath: file, content: "bug\n" }) + yield* llm.text("done") + + yield* prompt.loop({ sessionID: session.id }) + yield* permission.allowEverything({ enable: false }) + expect(yield* Effect.promise(() => Bun.file(plan).exists())).toBe(true) + expect(yield* Effect.promise(() => Bun.file(file).exists())).toBe(false) + + const msgs = yield* MessageV2.filterCompactedEffect(session.id) + const tools = msgs + .flatMap((msg) => msg.parts) + .filter((part): part is MessageV2.ToolPart => part.type === "tool" && part.tool === "write") + const good = tools.find((part) => (part.state.input as { filePath?: string } | undefined)?.filePath === plan) + const bad = tools.find((part) => (part.state.input as { filePath?: string } | undefined)?.filePath === file) + expect(good?.state.status).toBe("completed") + expect(bad?.state.status).toBe("error") + }), + { git: true, config: providerCfg }, + ), +) + +it.live("plan agent denies non-plan write despite session allow-everything", () => + provideTmpdirServer( + ({ dir, llm }) => + Effect.gen(function* () { + const permission = yield* Permission.Service + const prompt = yield* SessionPrompt.Service + const sessions = yield* Session.Service + const session = yield* sessions.create({ + title: "Plan local guard", + permission: [{ permission: "*", pattern: "*", action: "allow" }], + }) + const plan = Session.plan(session) + const file = path.join(dir, "plan-session-bug.txt") + + yield* permission.allowEverything({ enable: true, sessionID: session.id }) + yield* prompt.prompt({ + sessionID: session.id, + agent: "plan", + noReply: true, + parts: [{ type: "text", text: "Create a plan only" }], + }) + yield* llm.tool("write", { filePath: plan, content: "# Plan\n\n- Stay in plan mode.\n" }) + yield* llm.tool("write", { filePath: file, content: "bug\n" }) + yield* llm.text("done") + + yield* prompt.loop({ sessionID: session.id }) + yield* permission.allowEverything({ enable: false, sessionID: session.id }) + expect(yield* Effect.promise(() => Bun.file(plan).exists())).toBe(true) + expect(yield* Effect.promise(() => Bun.file(file).exists())).toBe(false) + + const msgs = yield* MessageV2.filterCompactedEffect(session.id) + const tools = msgs + .flatMap((msg) => msg.parts) + .filter((part): part is MessageV2.ToolPart => part.type === "tool" && part.tool === "write") + const good = tools.find((part) => (part.state.input as { filePath?: string } | undefined)?.filePath === plan) + const bad = tools.find((part) => (part.state.input as { filePath?: string } | undefined)?.filePath === file) + expect(good?.state.status).toBe("completed") + expect(bad?.state.status).toBe("error") + }), + { git: true, config: providerCfg }, + ), +) +// kilocode_change end unix("plan follow-up stops before queued mutations after plan_exit", () => provideTmpdirServer(