diff --git a/.changeset/hardening-permission-enforcement.md b/.changeset/hardening-permission-enforcement.md new file mode 100644 index 00000000..9c9f4dfd --- /dev/null +++ b/.changeset/hardening-permission-enforcement.md @@ -0,0 +1,14 @@ +--- +'@moonshot-ai/agent-core-v2': minor +--- + +Make the permission layer enforce what it is configured to enforce: + +- Seed each agent with the user's `[permission]` rules from config. The rules Op + is not persisted, so the model always started empty and configured + allow/deny/ask rules never reached the policy chain. +- Order the sensitive-file and git-control asks ahead of the blanket auto-mode + approval, so auto mode no longer waives them. An explicit prior approval or a + user `allow` rule still wins, so nothing already approved re-prompts. +- Fail closed when no approval broker is bound: a call that reached the "ask" + path is now blocked instead of being treated as approved. diff --git a/packages/agent-core-v2/src/agent/permissionPolicy/permissionPolicyService.ts b/packages/agent-core-v2/src/agent/permissionPolicy/permissionPolicyService.ts index c687fa19..9f53b74b 100644 --- a/packages/agent-core-v2/src/agent/permissionPolicy/permissionPolicyService.ts +++ b/packages/agent-core-v2/src/agent/permissionPolicy/permissionPolicyService.ts @@ -42,15 +42,24 @@ export class AgentPermissionPolicyService @IInstantiationService private readonly instantiation: IInstantiationService, ) { super(); + // Order matters: the first policy to return a result wins. + // + // `AutoModeApprove` sits after the two content-sensitive asks (secrets on + // disk, and the .git control directory) rather than ahead of them, so + // enabling auto mode speeds up ordinary work without also silently + // waiving the checks that exist for the highest-consequence paths. + // Everything else keeps its previous relative order: an explicit prior + // approval (`SessionApprovalHistory`) or a user `allow` rule still wins, + // so this does not re-prompt for something already approved. this.policies = [ this.instantiation.createInstance(AutoModeAskUserQuestionDenyPermissionPolicyService), this.instantiation.createInstance(UserConfiguredDenyPermissionPolicyService), - this.instantiation.createInstance(AutoModeApprovePermissionPolicyService), this.instantiation.createInstance(SessionApprovalHistoryPermissionPolicyService), this.instantiation.createInstance(UserConfiguredAskPermissionPolicyService), this.instantiation.createInstance(UserConfiguredAllowPermissionPolicyService), this.instantiation.createInstance(SensitiveFileAccessAskPermissionPolicyService), this.instantiation.createInstance(GitControlPathAccessAskPermissionPolicyService), + this.instantiation.createInstance(AutoModeApprovePermissionPolicyService), this.instantiation.createInstance(YoloModeApprovePermissionPolicyService), this.instantiation.createInstance(DefaultToolApprovePermissionPolicyService), this.instantiation.createInstance(GitCwdWriteApprovePermissionPolicyService), diff --git a/packages/agent-core-v2/src/agent/toolApproval/toolApprovalService.ts b/packages/agent-core-v2/src/agent/toolApproval/toolApprovalService.ts index 354d0bfc..863fc274 100644 --- a/packages/agent-core-v2/src/agent/toolApproval/toolApprovalService.ts +++ b/packages/agent-core-v2/src/agent/toolApproval/toolApprovalService.ts @@ -131,7 +131,14 @@ export class AgentToolApprovalService extends Service implements IAgentToolAppro let response: ApprovalResponse; const approvalService = this.tryApprovalService(); if (approvalService === undefined) { - response = { decision: 'approved' }; + // Fail closed. Reaching here means a policy decided this call needs + // confirmation, but no broker is bound to ask (an embedding host that + // never wired one up). Treating "nobody to ask" as consent would let + // every gated tool call through unreviewed. + response = { + decision: 'rejected', + feedback: 'No approval broker is available to confirm this tool call.', + }; } else { this.eventBus.publish({ type: 'permission.approval.requested', ...approvalContext }); try { diff --git a/packages/agent-core-v2/src/session/agentLifecycle/agentLifecycleService.ts b/packages/agent-core-v2/src/session/agentLifecycle/agentLifecycleService.ts index 6c7a0d6e..f8bed4aa 100644 --- a/packages/agent-core-v2/src/session/agentLifecycle/agentLifecycleService.ts +++ b/packages/agent-core-v2/src/session/agentLifecycle/agentLifecycleService.ts @@ -36,6 +36,8 @@ import { IConfigService } from '#/app/config/config'; import { IEventBus } from '#/app/event/eventBus'; import { DEFAULT_PERMISSION_MODE_SECTION } from '#/agent/permissionMode/configSection'; import { PermissionModeConfiguredModel } from '#/agent/permissionMode/permissionModeOps'; +import { PERMISSION_SECTION, type PermissionConfig } from '#/agent/permissionRules/configSection'; +import { IAgentPermissionRulesService } from '#/agent/permissionRules/permissionRules'; import type { PermissionMode } from '#/agent/permissionPolicy/types'; import { IAgentTaskService } from '#/agent/task/task'; import { ISessionContext } from '#/session/sessionContext/sessionContext'; @@ -196,6 +198,14 @@ export class AgentLifecycleService extends Disposable implements IAgentLifecycle if (permissionMode !== undefined && !hasRestoredPermissionMode) { handle.accessor.get(IAgentPermissionModeService).setMode(permissionMode); } + // Seed the agent with the user's persisted `[permission]` rules. The + // `permission.rules.add` Op is not persisted, so a rules model always + // starts empty and has to be filled here — otherwise the config section + // parses fine but the user-configured policies never see a rule to match. + const configuredRules = this.config.get(PERMISSION_SECTION)?.rules; + if (configuredRules !== undefined && configuredRules.length > 0) { + handle.accessor.get(IAgentPermissionRulesService).addRules(configuredRules); + } } async fork(sourceAgentId: string, opts?: ForkAgentOptions): Promise { diff --git a/packages/agent-core-v2/test/agent/permissionPolicy/permissionPolicyService.test.ts b/packages/agent-core-v2/test/agent/permissionPolicy/permissionPolicyService.test.ts index a95a124f..2267370d 100644 --- a/packages/agent-core-v2/test/agent/permissionPolicy/permissionPolicyService.test.ts +++ b/packages/agent-core-v2/test/agent/permissionPolicy/permissionPolicyService.test.ts @@ -298,6 +298,32 @@ describe('AgentPermissionPolicyService git cwd write approval', () => { }); }); + it('still asks for sensitive files in auto mode', async () => { + // Auto mode speeds up ordinary work; it must not silently waive the + // secrets check. + mode = 'auto'; + await expect(evaluate({ + toolName: 'Write', + args: { path: '.env', content: 'SECRET=1' }, + accesses: ToolAccesses.writeFile(join(workspaceDir, '.env')), + })).resolves.toMatchObject({ + policyName: 'sensitive-file-access-ask', + result: { kind: 'ask' }, + }); + }); + + it('still asks for git control files in auto mode', async () => { + mode = 'auto'; + await expect(evaluate({ + toolName: 'Write', + args: { path: '.git/config', content: 'x' }, + accesses: ToolAccesses.writeFile(join(workspaceDir, '.git/config')), + })).resolves.toMatchObject({ + policyName: 'git-control-path-access-ask', + result: { kind: 'ask' }, + }); + }); + it('does not use git-cwd approval in auto mode', async () => { mode = 'auto'; await expect(evaluate({ diff --git a/packages/agent-core-v2/test/agent/toolApproval/toolApproval.test.ts b/packages/agent-core-v2/test/agent/toolApproval/toolApproval.test.ts index 3adc2302..b47d08ee 100644 --- a/packages/agent-core-v2/test/agent/toolApproval/toolApproval.test.ts +++ b/packages/agent-core-v2/test/agent/toolApproval/toolApproval.test.ts @@ -241,13 +241,22 @@ describe('AgentToolApprovalService', () => { }); describe('requestToolApproval', () => { - it('auto-approves when no approval broker is registered', async () => { + it('fails closed when no approval broker is registered', async () => { + // A policy already decided this call needs confirmation. With no broker + // to ask, the call must be blocked rather than treated as approved. const events = subscribeApprovalEvents(); const svc = make(); await expect( svc.requestToolApproval(makeContext('Bash', { command: 'printf hi' }), ask(), 'fallback-ask'), - ).resolves.toBeUndefined(); + ).resolves.toEqual({ + veto: { + output: + 'Tool "Bash" was not run because the user rejected the approval request. ' + + 'Reason: No approval broker is available to confirm this tool call.', + isError: true, + }, + }); expect(events.requested).not.toHaveBeenCalled(); expect(events.resolved).not.toHaveBeenCalled(); @@ -255,14 +264,14 @@ describe('AgentToolApprovalService', () => { expect(recorded[0]).toMatchObject({ toolName: 'Bash', sessionApprovalRule: undefined, - result: { decision: 'approved' }, + result: { decision: 'rejected' }, }); expect(records).toContainEqual({ event: 'permission_approval_result', properties: expect.objectContaining({ policy_name: 'fallback-ask', tool_name: 'Bash', - result: 'approved', + result: 'rejected', session_cache_written: false, }), }); diff --git a/packages/agent-core-v2/test/session/agentLifecycle/agentLifecycle.test.ts b/packages/agent-core-v2/test/session/agentLifecycle/agentLifecycle.test.ts index 628b5af2..4e55ecc4 100644 --- a/packages/agent-core-v2/test/session/agentLifecycle/agentLifecycle.test.ts +++ b/packages/agent-core-v2/test/session/agentLifecycle/agentLifecycle.test.ts @@ -22,6 +22,9 @@ import { IAgentMcpService } from '#/agent/mcp/mcp'; import { McpConnectionManager } from '#/mcpCore/connection-manager'; import { IAgentPermissionModeService } from '#/agent/permissionMode/permissionMode'; import '#/agent/permissionMode/permissionModeOps'; +import { PERMISSION_SECTION } from '#/agent/permissionRules/configSection'; +import { IAgentPermissionRulesService } from '#/agent/permissionRules/permissionRules'; +import '#/agent/permissionRules/permissionRulesService'; import { IAgentStateService } from '#/agent/state/agentState'; import { AgentStateService } from '#/agent/state/agentStateService'; import { ISessionStateService } from '#/session/state/sessionState'; @@ -586,6 +589,32 @@ describe('AgentLifecycleService', () => { expect(permissionModeSetMode).toHaveBeenCalledWith('auto'); }); + it('seeds the agent with the permission rules from config', async () => { + // The rules Op is not persisted, so without this seeding a user's + // `[permission]` deny/allow/ask config would parse but never reach the + // policy chain. + ix.stub(IConfigService, { + ready: Promise.resolve(), + get: ((section: string) => + section === PERMISSION_SECTION + ? { rules: [{ decision: 'deny', scope: 'user', pattern: 'Bash' }] } + : undefined) as IConfigService['get'], + onDidSectionChange: (() => ({ dispose: () => {} })) as IConfigService['onDidSectionChange'], + } as unknown as IConfigService); + + const handle = await ix.get(IAgentLifecycleService).create({ agentId: 'main' }); + + expect(handle.accessor.get(IAgentPermissionRulesService).rules).toEqual([ + { decision: 'deny', scope: 'user', pattern: 'Bash' }, + ]); + }); + + it('leaves the rules empty when config has no permission section', async () => { + const handle = await ix.get(IAgentLifecycleService).create({ agentId: 'main' }); + + expect(handle.accessor.get(IAgentPermissionRulesService).rules).toEqual([]); + }); + it('keeps the restored permission mode instead of overwriting it with the default', async () => { ix.stub(IAppendLogStore, recordingAppendLog([ createWireMetadataRecord(1),