diff --git a/CHANGELOG.md b/CHANGELOG.md index 1cca33c9f06..640c05f903a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,8 @@ All notable changes to NanoClaw will be documented in this file. ## [Unreleased] +- **The guard seam.** Every privileged action crossing the container or channel boundary now passes one decision function — `guard()` in `src/guard/` — before it executes: `allow`, `hold` (the existing approval flows), or `deny`. Today's checks are preserved verbatim as each action's decision; ncl commands and delivery actions cannot register without a guard, and approved replays re-enter carrying the approval row as a **grant** (the forgeable `approved: true` boolean is deleted) with the checks re-run live. Two deliberate outcome changes: **(a)** approving a held a2a message after its destination was revoked no longer delivers it — the requester is told "approved, but not delivered" and the host logs a warning; **(b)** a forged, already-consumed, or mismatched grant refuses the replay instead of executing. +- **Delivery-registry hardening.** Re-registering a guard-wrapped delivery action *without* a guard spec now throws instead of silently disarming the guard. - [BREAKING] **`whatsapp-formatting` and `slack-formatting` container skills moved from trunk to the `channels` branch.** They now install with their channel — `/add-whatsapp` / `/add-slack` and the setup installers copy them in — so installs without those channels stop carrying channel-specific formatting instructions in every agent's context. **Migration — only if this install has the channel wired** (check `src/channels/whatsapp.ts` / `src/channels/slack.ts`): updating removes both skills from the working tree — WhatsApp agents lose the formatting fragment from their composed CLAUDE.md on next spawn, Slack agents lose the mrkdwn skill from `~/.claude/skills`. Re-run the matching skill, `/add-whatsapp` or `/add-slack` (idempotent), to restore. Installs without the channel need nothing — do NOT run the add-skill just in case; it installs the full channel adapter. - **Pre-task script failures back their series off instead of spinning.** A `--script` that errors lands the occurrence as a failed run (`script-skip:error` ack → `failed` status); recurrence reads the series' trailing failed streak and re-arms at `max(cron next, now + 2·2^(n−1) min, cap 60)`; after 8 consecutive failures the series is auto-paused with a host-written note in its run log (`ncl tasks resume` revives it). A deliberate `wakeAgent:false` gate is a normal run and never backs off. Also fixed: an explicitly-addressed `` in a task fire's final text now delivers as a deliberate send (previously suppressed as a turn-reply echo → zero delivery when the agent skipped the MCP tool); identical echoes of an MCP send are dropped in the runner, where the duplication originates. - [BREAKING] **Scheduled tasks moved from MCP tools to `ncl tasks`.** The six scheduling MCP tools are no longer exposed to agent containers; agents and operators manage tasks with `ncl tasks list/get/create/update/cancel/pause/resume/delete`. New tasks run from a per-agent-group system session rather than waking the chat session that created them, and task writes are not approval-gated inside the owning group. **Migration:** [docs/ncl-tasks-migration.md](docs/ncl-tasks-migration.md). diff --git a/CLAUDE.md b/CLAUDE.md index 4416e14c817..0772957a1eb 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -62,10 +62,12 @@ For ad-hoc queries from skills or scripts, use the in-tree wrapper rather than t | `src/index.ts` | Entry point: init DB, migrations, channel adapters, delivery polls, sweep, shutdown | | `src/router.ts` | Inbound routing: messaging group → agent group → session → `inbound.db` → wake | | `src/delivery.ts` | Polls `outbound.db`, delivers via adapter, handles system actions (schedule, approvals, etc.) | +| `src/delivery-guard.ts` | `DeliveryGuardSpec` + `runGuarded` — the guard-consult pipeline for privileged delivery actions (registry stays in `delivery.ts`) | | `src/host-sweep.ts` | 60s sweep: `processing_ack` sync, stale detection, due-message wake, recurrence | | `src/session-manager.ts` | Resolves sessions; opens `inbound.db` / `outbound.db`; manages heartbeat path | | `src/container-runner.ts` | Spawns per-agent-group Docker containers with session DB + outbox mounts, OneCLI `ensureAgent` | | `src/container-runtime.ts` | Docker CLI wrapper (runtime binary, host-gateway args, mount args), orphan cleanup | +| `src/guard/` | Privileged-action decision seam: `guard(action, input)` → allow \| hold \| deny. Module-edge `guard.ts` adapters (cli, agent-to-agent, self-mod, permissions) define each action's decision; ncl commands + delivery actions demand a guard at registration; approved replays carry the approval row as a grant and re-run the checks. Conformance test: `src/guard/conformance.test.ts` | | `src/modules/permissions/access.ts` | `canAccessAgentGroup` — owner / global admin / scoped admin / member resolution against `user_roles` + `agent_group_members` | | `src/modules/approvals/primitive.ts` | `pickApprover`, `pickApprovalDelivery`, `requestApproval`, approval-handler registry | | `src/command-gate.ts` | Router-side admin command gate — queries `user_roles` directly (no env var, no container-side check) | diff --git a/src/cli/crud.ts b/src/cli/crud.ts index fd786232943..587f5be1cd8 100644 --- a/src/cli/crud.ts +++ b/src/cli/crud.ts @@ -402,6 +402,7 @@ export function registerResource(def: ResourceDef): void { if (def.operations.list) { register({ name: `${def.plural}-list`, + action: `${def.plural}.list`, description: `List all ${def.plural}.`, access: def.operations.list, resource: def.plural, @@ -414,6 +415,7 @@ export function registerResource(def: ResourceDef): void { if (def.operations.get) { register({ name: `${def.plural}-get`, + action: `${def.plural}.get`, description: `Get a ${def.name} by ID.`, access: def.operations.get, resource: def.plural, @@ -426,6 +428,7 @@ export function registerResource(def: ResourceDef): void { if (def.operations.create) { register({ name: `${def.plural}-create`, + action: `${def.plural}.create`, description: `Create a new ${def.name}.`, access: def.operations.create, resource: def.plural, @@ -437,6 +440,7 @@ export function registerResource(def: ResourceDef): void { if (def.operations.update) { register({ name: `${def.plural}-update`, + action: `${def.plural}.update`, description: `Update a ${def.name}.`, access: def.operations.update, resource: def.plural, @@ -448,6 +452,7 @@ export function registerResource(def: ResourceDef): void { if (def.operations.delete) { register({ name: `${def.plural}-delete`, + action: `${def.plural}.delete`, description: `Delete a ${def.name}.`, access: def.operations.delete, resource: def.plural, @@ -464,6 +469,7 @@ export function registerResource(def: ResourceDef): void { const declared = op.args; register({ name: `${def.plural}-${verb.replace(/ /g, '-')}`, + action: `${def.plural}.${verb.replace(/ /g, '.')}`, description: op.description, access: op.access, resource: def.plural, diff --git a/src/cli/delivery-action.ts b/src/cli/delivery-action.ts index 5c693be6f1c..5a759cb21de 100644 --- a/src/cli/delivery-action.ts +++ b/src/cli/delivery-action.ts @@ -8,52 +8,57 @@ import type Database from 'better-sqlite3'; import { registerDeliveryAction } from '../delivery.js'; +import { unguarded } from '../guard/index.js'; import { insertMessage } from '../db/session-db.js'; import { log } from '../log.js'; import { dispatch } from './dispatch.js'; import type { RequestFrame } from './frame.js'; import type { Session } from '../types.js'; -registerDeliveryAction('cli_request', async (content, session, inDb) => { - const requestId = content.requestId as string; - const command = content.command as string; - const args = (content.args as Record) ?? {}; - - if (!requestId || !command) { - log.warn('cli_request missing requestId or command', { sessionId: session.id }); - return; - } - - const req: RequestFrame = { id: requestId, command, args }; - const ctx = { - caller: 'agent' as const, - sessionId: session.id, - agentGroupId: session.agent_group_id, - messagingGroupId: session.messaging_group_id ?? '', - }; - - log.info('CLI request from agent', { requestId, command, sessionId: session.id }); - - const response = await dispatch(req, ctx); - - // Write response to inbound.db so the container can read it. - // trigger=0: don't wake the agent — this is an inline response to a tool call. - insertMessage(inDb, { - id: `cli-resp-${requestId}`, - kind: 'system', - timestamp: new Date().toISOString(), - platformId: null, - channelType: null, - threadId: null, - content: JSON.stringify({ - type: 'cli_response', - requestId, - frame: response, - }), - processAfter: null, - recurrence: null, - trigger: 0, - }); - - log.info('CLI response written', { requestId, ok: response.ok, sessionId: session.id }); -}); +registerDeliveryAction( + 'cli_request', + async (content, session, inDb) => { + const requestId = content.requestId as string; + const command = content.command as string; + const args = (content.args as Record) ?? {}; + + if (!requestId || !command) { + log.warn('cli_request missing requestId or command', { sessionId: session.id }); + return; + } + + const req: RequestFrame = { id: requestId, command, args }; + const ctx = { + caller: 'agent' as const, + sessionId: session.id, + agentGroupId: session.agent_group_id, + messagingGroupId: session.messaging_group_id ?? '', + }; + + log.info('CLI request from agent', { requestId, command, sessionId: session.id }); + + const response = await dispatch(req, ctx); + + // Write response to inbound.db so the container can read it. + // trigger=0: don't wake the agent — this is an inline response to a tool call. + insertMessage(inDb, { + id: `cli-resp-${requestId}`, + kind: 'system', + timestamp: new Date().toISOString(), + platformId: null, + channelType: null, + threadId: null, + content: JSON.stringify({ + type: 'cli_response', + requestId, + frame: response, + }), + processAfter: null, + recurrence: null, + trigger: 0, + }); + + log.info('CLI response written', { requestId, ok: response.ok, sessionId: session.id }); + }, + unguarded('transport envelope — every inner command is guarded at dispatch'), +); diff --git a/src/cli/dispatch.test.ts b/src/cli/dispatch.test.ts index 4da3c1dca1a..558154d3770 100644 --- a/src/cli/dispatch.test.ts +++ b/src/cli/dispatch.test.ts @@ -9,6 +9,7 @@ const approvalState = vi.hoisted(() => ({ | ((args: { session: unknown; payload: Record; + approval: Record; userId: string; notify: (text: string) => void; }) => Promise), @@ -18,6 +19,7 @@ const approvalState = vi.hoisted(() => ({ handler: (args: { session: unknown; payload: Record; + approval: Record; userId: string; notify: (text: string) => void; }) => Promise, @@ -43,8 +45,11 @@ vi.mock('../db/agent-groups.js', () => ({ })); const mockGetSession = vi.fn(); +// The guard's grant check re-fetches the approval row to prove it's live. +const mockGetPendingApproval = vi.fn(); vi.mock('../db/sessions.js', () => ({ getSession: (...args: unknown[]) => mockGetSession(...args), + getPendingApproval: (...args: unknown[]) => mockGetPendingApproval(...args), })); // dispatch's post-handler looks up the resource's `scopeField` via getResource. @@ -495,10 +500,20 @@ describe('CLI scope enforcement', () => { callerContext: ctx, }); + // The approve path hands the handler the live approval row — the grant + // the replay carries back into dispatch. + const grantRow = { + approval_id: 'appr-t1', + action: 'cli_command', + payload: JSON.stringify(approval.payload), + }; + mockGetPendingApproval.mockReturnValue(grantRow); + expect(approvalState.approvalHandler).toBeTypeOf('function'); await approvalState.approvalHandler!({ session: { id: 's1', agent_group_id: 'g1', messaging_group_id: 'mg1' }, payload: approval.payload, + approval: grantRow, userId: 'telegram:admin', notify: vi.fn(), }); @@ -507,6 +522,73 @@ describe('CLI scope enforcement', () => { expect(approvalState.requestApproval).toHaveBeenCalledTimes(1); }); + // --- Grant-carrying replay (the `approved: true` boolean no longer exists) --- + + it('replay with a dead grant (row deleted) refuses instead of re-holding', async () => { + mockGetContainerConfig.mockReturnValue({ cli_scope: 'group' }); + mockGetSession.mockReturnValue({ id: 's1', agent_group_id: 'g1', messaging_group_id: 'mg1' }); + mockGetAgentGroup.mockReturnValue({ id: 'g1', name: 'Group One' }); + + const ctx = agentCtx(); + await dispatch({ id: '1', command: 'approval-context-command', args: {} }, ctx); + const approval = approvalState.requestApproval.mock.calls[0][0] as { payload: Record }; + + mockGetPendingApproval.mockReturnValue(undefined); // resolution already deleted the row + const notify = vi.fn(); + await approvalState.approvalHandler!({ + session: { id: 's1', agent_group_id: 'g1', messaging_group_id: 'mg1' }, + payload: approval.payload, + approval: { approval_id: 'appr-dead', action: 'cli_command', payload: JSON.stringify(approval.payload) }, + userId: 'telegram:admin', + notify, + }); + + expect(approvalState.observedContexts).toHaveLength(0); // handler never ran + expect(approvalState.requestApproval).toHaveBeenCalledTimes(1); // no second card + expect(notify.mock.calls[0][0]).toContain('failed'); + }); + + it("a grant approved for one command doesn't transfer to another", async () => { + mockGetContainerConfig.mockReturnValue({ cli_scope: 'group' }); + + // A live cli_command row, but held for a DIFFERENT command. + const grantRow = { + approval_id: 'appr-other', + action: 'cli_command', + payload: JSON.stringify({ frame: { command: 'members-add' } }), + }; + mockGetPendingApproval.mockReturnValue(grantRow); + + const resp = await dispatch({ id: '1', command: 'approval-context-command', args: {} }, agentCtx(), { + grant: grantRow as never, + }); + + expect(resp.ok).toBe(false); + if (!resp.ok) { + expect(resp.error.code).toBe('forbidden'); + expect(resp.error.message).toContain('grant'); + } + expect(approvalState.observedContexts).toHaveLength(0); + }); + + it('a fabricated grant object without a live row is refused', async () => { + mockGetContainerConfig.mockReturnValue({ cli_scope: 'group' }); + mockGetPendingApproval.mockReturnValue(undefined); + + const forged = { + approval_id: 'appr-forged', + action: 'cli_command', + payload: JSON.stringify({ frame: { command: 'approval-context-command' } }), + }; + const resp = await dispatch({ id: '1', command: 'approval-context-command', args: {} }, agentCtx(), { + grant: forged as never, + }); + + expect(resp.ok).toBe(false); + if (!resp.ok) expect(resp.error.code).toBe('forbidden'); + expect(approvalState.requestApproval).not.toHaveBeenCalled(); // refusal, not a fresh hold + }); + // --- Post-handler filtering --- it('group: groups list filters out other groups', async () => { diff --git a/src/cli/dispatch.ts b/src/cli/dispatch.ts index 3885e931ea2..290756c81bc 100644 --- a/src/cli/dispatch.ts +++ b/src/cli/dispatch.ts @@ -3,24 +3,38 @@ * the per-session DB poller (container caller) call dispatch() with the * same frame and a transport-supplied CallerContext. * - * Approval gating for risky calls from the container is the only branch - * that differs by caller. Host callers and `open` commands run inline. + * Every command passes the guard before its handler runs — the decision + * (allow / hold / deny) comes from the command's catalog entry, derived at + * registration (see cli/guard.ts). Dispatch keeps the mechanics: arg + * auto-fill, the sessions-get existence oracle, `--help` interception, + * parseArgs, and post-handler row filtering. An approved replay re-enters + * here carrying the verified approval row as its grant — the guard re-checks + * the structural checks live, and the `approved: true` boolean no longer + * exists. */ import { getContainerConfig } from '../db/container-configs.js'; import { getAgentGroup } from '../db/agent-groups.js'; import { getSession } from '../db/sessions.js'; +import { guard, type GuardActor } from '../guard/index.js'; import { registerApprovalHandler, requestApproval } from '../modules/approvals/index.js'; +import type { PendingApproval } from '../types.js'; import type { CallerContext, ErrorCode, RequestFrame, ResponseFrame } from './frame.js'; import { localizeIsoTimestamps } from './format.js'; import { getResource } from './crud.js'; import { listVerbs, renderVerbHelp } from './help-render.js'; -import { GROUP_SCOPE_RESOURCES, listCommands, lookup } from './registry.js'; +import { commandGuard, listCommands, lookup } from './registry.js'; type DispatchOptions = { - /** True when a command is being replayed after approval. */ - approved?: boolean; + /** Verified approval row when a command is replayed after approval. */ + grant?: PendingApproval; }; +function actorFor(ctx: CallerContext): GuardActor { + return ctx.caller === 'host' + ? { kind: 'host' } + : { kind: 'agent', agentGroupId: ctx.agentGroupId, sessionId: ctx.sessionId }; +} + export async function dispatch( req: RequestFrame, ctx: CallerContext, @@ -55,43 +69,13 @@ export async function dispatch( return err(req.id, 'unknown-command', unknownCommandMessage(req.command)); } - // CLI scope enforcement for agent callers + // Group-scope mechanics for agent callers (visibility, not policy — the + // allow/hold/deny decisions live in the guard decision, cli/guard.ts). if (ctx.caller === 'agent') { const configRow = getContainerConfig(ctx.agentGroupId); const cliScope = configRow?.cli_scope ?? 'group'; - if (cliScope === 'disabled') { - return err(req.id, 'forbidden', 'CLI access is disabled for this agent group.'); - } - if (cliScope === 'group') { - // Only allow whitelisted resources and general commands (no resource, like help) - if (cmd.resource && !GROUP_SCOPE_RESOURCES.has(cmd.resource)) { - return err(req.id, 'forbidden', `CLI access is scoped to this agent group. Cannot access "${cmd.resource}".`); - } - - // Enforce group scope on all agent-group-related args. - // Different resources use different arg names for the agent group ID. - // Only check --id for resources where it IS the agent group ID. - const groupArgs = ['agent_group_id', 'group'] as const; - for (const key of groupArgs) { - if (req.args[key] && req.args[key] !== ctx.agentGroupId) { - return err(req.id, 'forbidden', 'CLI access is scoped to this agent group.'); - } - } - if ( - (cmd.resource === 'groups' || cmd.resource === 'destinations') && - req.args.id && - req.args.id !== ctx.agentGroupId - ) { - return err(req.id, 'forbidden', 'CLI access is scoped to this agent group.'); - } - - // Block cli_scope changes from group-scoped agents (privilege escalation) - if (req.args.cli_scope !== undefined || req.args['cli-scope'] !== undefined) { - return err(req.id, 'forbidden', 'Cannot change cli_scope from a group-scoped agent.'); - } - // Auto-fill agent-group-related args so the agent doesn't need // to pass its own group ID explicitly. const fill: Record = { @@ -117,9 +101,19 @@ export async function dispatch( } } + const decision = guard(commandGuard(cmd.name), { + actor: actorFor(ctx), + payload: req.args, + grant: opts.grant ?? null, + }); + + if (decision.effect === 'deny') { + return err(req.id, 'forbidden', decision.reason); + } + // `--help` interception: answer with the command's generated help instead of - // executing. Placed after scope enforcement (a group-scoped agent can't probe - // forbidden resources) and BEFORE approval gating — asking for help on an + // executing. Placed after the guard's deny (a group-scoped agent can't probe + // forbidden resources) and BEFORE hold execution — asking for help on an // approval-gated verb must never mint an approval card. if (req.args.help === true) { // Carry the help text in `human` too, so both clients print it verbatim @@ -128,7 +122,12 @@ export async function dispatch( return { id: req.id, ok: true, data: helpText, human: helpText }; } - if (ctx.caller !== 'host' && cmd.access === 'approval' && !opts.approved) { + if (decision.effect === 'hold') { + if (ctx.caller !== 'agent') { + // Holds only arise for agent callers; anything else is a guard bug — + // fail closed rather than card a ghost. + return err(req.id, 'forbidden', decision.reason); + } const session = getSession(ctx.sessionId); if (!session) { return err(req.id, 'handler-error', 'Session not found.'); @@ -216,10 +215,10 @@ export async function dispatch( } } -registerApprovalHandler('cli_command', async ({ payload, notify }) => { +registerApprovalHandler('cli_command', async ({ payload, approval, notify }) => { const frame = payload.frame as RequestFrame; const callerContext = parseCallerContext(payload.callerContext) ?? { caller: 'host' }; - const response = await dispatch(frame, callerContext, { approved: true }); + const response = await dispatch(frame, callerContext, { grant: approval }); if (response.ok) { const localized = localizeIsoTimestamps(response.data); diff --git a/src/cli/guard.ts b/src/cli/guard.ts new file mode 100644 index 00000000000..49c8602b328 --- /dev/null +++ b/src/cli/guard.ts @@ -0,0 +1,86 @@ +/** + * CLI guard adapter — the command registry's catalog derivation and + * structural decision, moved verbatim out of dispatch.ts. + * Declaration is registration: registry.register() derives one + * catalog entry per command from the CommandDef itself; no second file is + * edited when a command is added. + * + * The decide fn carries today's decisions exactly: + * host caller → allow (the 0600 socket is the auth story — in code, + * unremovable by data); + * cli_scope 'disabled' → deny; 'group' → resource allowlist, cross-group + * arg denial, cli_scope-change denial; + * access 'approval' for agent callers → hold for the group's admin chain. + * + * Arg auto-fill, the sessions-get existence oracle, and post-handler row + * filtering stay in dispatch.ts — mechanics, not policy. + */ +import { getContainerConfig } from '../db/container-configs.js'; +import { ALLOW, DENY, HOLD, type GuardedActionSpec, type GuardInput } from '../guard/index.js'; +import { GROUP_SCOPE_RESOURCES, type CommandDef } from './registry.js'; + +/** Dotted catalog action name for a command. */ +export function commandGuardAction(cmd: Pick): string { + return cmd.action ?? `cli.${cmd.name}`; +} + +/** Catalog entry derived from a CommandDef at registration time. */ +export function commandGuardSpec(cmd: CommandDef): GuardedActionSpec { + return { + action: commandGuardAction(cmd), + grantActionName: cmd.access === 'approval' ? 'cli_command' : undefined, + // Bind a cli_command grant to the exact command it was approved for. + grantCoversRequest: (grant) => { + try { + const payload = JSON.parse(grant.payload) as { frame?: { command?: string } }; + return payload.frame?.command === cmd.name; + } catch { + return false; + } + }, + decide: (input) => commandDecide(cmd, input), + }; +} + +function commandDecide(cmd: CommandDef, input: GuardInput) { + const { actor } = input; + if (actor.kind === 'host') return ALLOW('host caller (trusted socket)'); + if (actor.kind !== 'agent') return DENY('CLI commands accept host or agent callers only.'); + + const args = input.payload; + const cliScope = getContainerConfig(actor.agentGroupId)?.cli_scope ?? 'group'; + + if (cliScope === 'disabled') { + return DENY('CLI access is disabled for this agent group.'); + } + + if (cliScope === 'group') { + // Only allow whitelisted resources and general commands (no resource, like help) + if (cmd.resource && !GROUP_SCOPE_RESOURCES.has(cmd.resource)) { + return DENY(`CLI access is scoped to this agent group. Cannot access "${cmd.resource}".`); + } + + // Enforce group scope on all agent-group-related args. + // Different resources use different arg names for the agent group ID. + // Only check --id for resources where it IS the agent group ID. + for (const key of ['agent_group_id', 'group'] as const) { + if (args[key] && args[key] !== actor.agentGroupId) { + return DENY('CLI access is scoped to this agent group.'); + } + } + if ((cmd.resource === 'groups' || cmd.resource === 'destinations') && args.id && args.id !== actor.agentGroupId) { + return DENY('CLI access is scoped to this agent group.'); + } + + // Block cli_scope changes from group-scoped agents (privilege escalation) + if (args.cli_scope !== undefined || args['cli-scope'] !== undefined) { + return DENY('Cannot change cli_scope from a group-scoped agent.'); + } + } + + if (cmd.access === 'approval') { + return HOLD(`agent-initiated "${cmd.name}" requires admin approval`); + } + + return ALLOW('open command'); +} diff --git a/src/cli/registry.ts b/src/cli/registry.ts index e108e114321..8a436ec3257 100644 --- a/src/cli/registry.ts +++ b/src/cli/registry.ts @@ -8,6 +8,8 @@ * registers the help commands, so the registry is populated before the host's * CLI server accepts connections. */ +import { defineGuardedAction, type GuardedAction } from '../guard/index.js'; +import { commandGuardSpec } from './guard.js'; import type { CallerContext } from './frame.js'; /** @@ -23,6 +25,13 @@ export type CommandDef = { name: string; description: string; access: Access; + /** + * Dotted guard-catalog action name (e.g. `roles.grant`, + * `groups.config.add-mcp-server`). Set by registerResource from the + * resource + verb; commands registered directly (help) fall back to + * `cli.`. + */ + action?: string; /** * The group-scope whitelist key. Under `cli_scope: 'group'` the dispatcher * only lets an agent run commands whose `resource` is on the whitelist @@ -52,12 +61,26 @@ export type CommandDef = { }; const registry = new Map(); +const commandGuards = new Map(); export function register(def: CommandDef): void { if (registry.has(def.name)) { throw new Error(`CLI command "${def.name}" already registered`); } registry.set(def.name, def as CommandDef); + // Declaration is registration: every command gets a guard-catalog entry + // derived from its own definition, in the same call that registers it — a + // command cannot exist without a guard, and dispatch consults it by value. + commandGuards.set(def.name, defineGuardedAction(commandGuardSpec(def as CommandDef))); +} + +/** The guard defined for a registered command — total for anything register() accepted. */ +export function commandGuard(name: string): GuardedAction { + const g = commandGuards.get(name); + if (!g) { + throw new Error(`CLI command "${name}" has no guard — was it registered through register()?`); + } + return g; } export function lookup(name: string): CommandDef | undefined { diff --git a/src/delivery-actions.test.ts b/src/delivery-actions.test.ts index 6a89506cd5c..6bd06fc9067 100644 --- a/src/delivery-actions.test.ts +++ b/src/delivery-actions.test.ts @@ -4,7 +4,9 @@ * `registerDeliveryAction` is the hook modules use to handle system-kind * outbound messages; `getDeliveryAction` is the read side that makes those * registrations behavior-testable. Goes red if either half of the registry - * is removed or the two stop sharing the same map. + * is removed or the two stop sharing the same map. Every registration now + * carries a guard spec or an explicit unguarded() declaration — + * omission is a type error. */ import { describe, it, expect, vi } from 'vitest'; @@ -16,11 +18,14 @@ vi.mock('./container-runner.js', () => ({ })); import { registerDeliveryAction, getDeliveryAction, type DeliveryActionHandler } from './delivery.js'; +import { defineGuardedAction, HOLD, unguarded } from './guard/index.js'; + +const testUnguarded = unguarded('test — registry mechanics only'); describe('delivery action registry', () => { it('getDeliveryAction returns the handler registerDeliveryAction registered', () => { const handler: DeliveryActionHandler = async () => {}; - registerDeliveryAction('test_registry_action', handler); + registerDeliveryAction('test_registry_action', handler, testUnguarded); expect(getDeliveryAction('test_registry_action')).toBe(handler); }); @@ -31,8 +36,34 @@ describe('delivery action registry', () => { it('re-registering an action overwrites the previous handler', () => { const first: DeliveryActionHandler = async () => {}; const second: DeliveryActionHandler = async () => {}; - registerDeliveryAction('test_overwrite_action', first); - registerDeliveryAction('test_overwrite_action', second); + registerDeliveryAction('test_overwrite_action', first, testUnguarded); + registerDeliveryAction('test_overwrite_action', second, testUnguarded); expect(getDeliveryAction('test_overwrite_action')).toBe(second); }); + + it('refuses to replace a guard-wrapped action with an unguarded handler', () => { + const guardAction = defineGuardedAction({ + action: 'test.guarded-overwrite', + decide: () => HOLD('t'), + }); + registerDeliveryAction('test_guarded_overwrite', async () => {}, { + guardAction, + requestHold: async () => {}, + }); + + // Disarming the guard by re-registering unguarded must throw — otherwise + // the action's catalog entry would still exist while the live path runs + // unguarded. + expect(() => registerDeliveryAction('test_guarded_overwrite', async () => {}, testUnguarded)).toThrow( + /disarm the guard/, + ); + + // Re-registering WITH a spec stays allowed (a legitimate replacement + // keeps the action guarded). + registerDeliveryAction('test_guarded_overwrite', async () => {}, { + guardAction, + requestHold: async () => {}, + }); + expect(getDeliveryAction('test_guarded_overwrite')).toBeDefined(); + }); }); diff --git a/src/delivery-guard.ts b/src/delivery-guard.ts new file mode 100644 index 00000000000..e4a99d72554 --- /dev/null +++ b/src/delivery-guard.ts @@ -0,0 +1,63 @@ +/** + * The guard-consult path for privileged delivery actions. + * + * The registry itself — registration, lookup, approved-replay re-entry — + * stays in delivery.ts, close to main's shape. This file holds the new + * guard logic: the spec a privileged registration carries, and runGuarded, + * the precheck → guard → deny/hold/allow pipeline every consult runs. + */ +import { guard, type GuardedAction } from './guard/index.js'; +import { log } from './log.js'; +import type { PendingApproval, Session } from './types.js'; + +/** Handler shape for guard-wrapped actions — must not touch inDb (replays run without one). */ +export type GuardedDeliveryHandler = (content: Record, session: Session) => Promise; + +export interface DeliveryGuardSpec { + /** Guard action consulted before the handler runs — the defined value, not a name. */ + guardAction: GuardedAction; + /** + * Domain validation that runs before the guard — malformed requests are + * answered (notify) without ever creating a hold. Return false to stop. + */ + precheck?: (content: Record, session: Session) => boolean | Promise; + /** Create the hold (the domain's requestApproval call — card text lives with the domain). */ + requestHold: (content: Record, session: Session) => Promise; + /** Tell the requester about a deny. */ + onDeny?: (content: Record, session: Session, reason: string) => void; +} + +/** + * Run a guarded delivery action: precheck, consult the guard, then route the + * decision — deny → onDeny, hold → requestHold, allow → handler. A fresh + * dispatch passes grant=null; an approved replay passes the approval row, + * which satisfies a hold but never a deny (the structural checks re-run + * live, so approve-then-revoke does not execute). + */ +export async function runGuarded( + action: string, + spec: DeliveryGuardSpec, + handler: GuardedDeliveryHandler, + content: Record, + session: Session, + grant: PendingApproval | null, +): Promise { + if (spec.precheck && !(await spec.precheck(content, session))) return; + + const decision = guard(spec.guardAction, { + actor: { kind: 'agent', agentGroupId: session.agent_group_id, sessionId: session.id }, + payload: content, + grant, + }); + + if (decision.effect === 'deny') { + log.warn('Delivery action denied by guard', { action, reason: decision.reason }); + spec.onDeny?.(content, session, decision.reason); + return; + } + if (decision.effect === 'hold') { + await spec.requestHold(content, session); + return; + } + await handler(content, session); +} diff --git a/src/delivery.ts b/src/delivery.ts index d9a0d15f270..4ff8a935448 100644 --- a/src/delivery.ts +++ b/src/delivery.ts @@ -20,12 +20,14 @@ import { markDeliveryFailed, migrateDeliveredTable, } from './db/session-db.js'; +import { runGuarded, type DeliveryGuardSpec, type GuardedDeliveryHandler } from './delivery-guard.js'; +import { isUnguarded, type Unguarded } from './guard/index.js'; import { log } from './log.js'; import { normalizeOptions } from './channels/ask-question.js'; import { clearOutbox, openInboundDb, openOutboundDb, readOutboxFiles } from './session-manager.js'; import { pauseTypingRefreshAfterDelivery, setTypingAdapter } from './modules/typing/index.js'; import type { OutboundFile } from './channels/adapter.js'; -import type { Session } from './types.js'; +import type { PendingApproval, Session } from './types.js'; const ACTIVE_POLL_MS = 1000; const SWEEP_POLL_MS = 60_000; @@ -393,14 +395,19 @@ async function deliverMessage( * Delivery action registry. * * Modules register handlers for system-kind outbound message actions via - * `registerDeliveryAction`. Core checks the registry first in - * `handleSystemAction` and falls through to the inline switch when no - * handler is registered. The switch will shrink as modules are extracted - * (scheduling, approvals, agent-to-agent) and eventually only its default - * branch remains. + * `registerDeliveryAction`. Unknown actions log "Unknown system action". * - * Default when no handler registered and the switch doesn't match: log - * "Unknown system action" and return. + * Privileged delivery actions (create_agent, install_packages, + * add_mcp_server) register with a guard spec: every path to the handler body + * — dispatch, approved replay, test lookup — goes through the guard consult + * (allow / hold / deny), so there is no unguarded route to it. On approve, + * the continuation re-enters the same entry carrying the approval row as its + * grant (`reenterGuardedDeliveryAction`), so the structural checks are + * re-run live. Plain actions (the cli_request bridge — its inner + * commands are guarded at dispatch) register with an + * explicit `unguarded()` declaration instead of a spec — omission is + * not representable, so the decision to run unguarded is visible, and + * justified, at the registration site. */ export type DeliveryActionHandler = ( content: Record, @@ -408,18 +415,70 @@ export type DeliveryActionHandler = ( inDb: Database.Database, ) => Promise; -const actionHandlers = new Map(); +type DeliveryEntry = + | { guard: Unguarded; handler: DeliveryActionHandler } + | { guard: DeliveryGuardSpec; handler: GuardedDeliveryHandler }; -export function registerDeliveryAction(action: string, handler: DeliveryActionHandler): void { - if (actionHandlers.has(action)) { +const deliveryActions = new Map(); + +function isUnguardedEntry(entry: DeliveryEntry): entry is Extract { + return isUnguarded(entry.guard); +} + +export function registerDeliveryAction(action: string, handler: DeliveryActionHandler, unguardedDecl: Unguarded): void; +export function registerDeliveryAction(action: string, handler: GuardedDeliveryHandler, spec: DeliveryGuardSpec): void; +export function registerDeliveryAction( + action: string, + handler: DeliveryActionHandler | GuardedDeliveryHandler, + guardDecl: DeliveryGuardSpec | Unguarded, +): void { + const existing = deliveryActions.get(action); + if (existing) { + // Replacing a guard-wrapped action with an unguarded handler would + // disarm the guard while its catalog entry still exists — refuse. A + // skill that wants to extend a guarded action must compose at the + // module's exported functions instead, or re-register with a guard spec + // of its own. + if (isUnguarded(guardDecl) && !isUnguardedEntry(existing)) { + throw new Error( + `delivery action "${action}" is guard-wrapped; re-registering it without a guard spec would disarm the guard`, + ); + } log.warn('Delivery action handler overwritten', { action }); } - actionHandlers.set(action, handler); + // The overloads pair each handler shape with its declaration; the merged + // implementation signature erases that pairing, hence the one cast. + deliveryActions.set(action, { guard: guardDecl, handler } as DeliveryEntry); } -/** Look up a registered delivery-action handler. Lets module registrations be behavior-tested. */ +/** + * Approve continuation for a guard-wrapped delivery action: re-enter the + * entry with the approval row as the grant. The guard treats the grant as + * hold-satisfied but re-runs the structural checks, so approve-then-revoke + * does not execute. Domains register this as their approval handler in the + * same line that registers the action. + */ +export function reenterGuardedDeliveryAction(action: string) { + return async (ctx: { session: Session; payload: Record; approval: PendingApproval }) => { + const entry = deliveryActions.get(action); + if (!entry || isUnguardedEntry(entry)) { + log.warn('Approved replay for an action that is not guard-wrapped — dropping', { action }); + return; + } + await runGuarded(action, entry.guard, entry.handler, ctx.payload, ctx.session, ctx.approval); + }; +} + +/** + * The invocable for a registered action — the raw handler for unguarded + * entries, the guard-consulting path for guarded ones. Dispatch and tests + * both come through here; there is no route around the guard. + */ export function getDeliveryAction(action: string): DeliveryActionHandler | undefined { - return actionHandlers.get(action); + const entry = deliveryActions.get(action); + if (!entry) return undefined; + if (isUnguardedEntry(entry)) return entry.handler; + return (content, session) => runGuarded(action, entry.guard, entry.handler, content, session, null); } /** @@ -435,7 +494,7 @@ async function handleSystemAction( const action = content.action as string; log.info('System action from agent', { sessionId: session.id, action }); - const registered = actionHandlers.get(action); + const registered = getDeliveryAction(action); if (registered) { await registered(content, session, inDb); return; diff --git a/src/guard/conformance.test.ts b/src/guard/conformance.test.ts new file mode 100644 index 00000000000..086f0a40db3 --- /dev/null +++ b/src/guard/conformance.test.ts @@ -0,0 +1,64 @@ +/** + * Guard conformance — checked with the real registries. + * + * The old registry walk is gone: an unmapped consult or an undeclared + * unguarded registration is now unconstructible — guard() takes the defined + * GuardedAction value (a dropped module-edge import or typo'd name is a + * compile error), and the keyed registries require a guard spec or an + * explicit unguarded() declaration. What's left to verify is the + * cross-registry pairing the compiler can't see: every holding action has a + * registered approve continuation. (At runtime a missing continuation is + * handled loudly at click time — the requester is told no handler is + * installed; this test keeps the tree from shipping that state.) + */ +import { describe, expect, it } from 'vitest'; + +// Production barrels — side-effect imports populate the real registries. +import '../cli/commands/index.js'; +import '../modules/index.js'; +import '../cli/delivery-action.js'; +import '../cli/dispatch.js'; // registers the cli_command approval handler + +import { commandGuard, listCommands } from '../cli/registry.js'; +import { getApprovalHandler } from '../modules/approvals/primitive.js'; +import { defineGuardedAction, listGuardedActions } from './guard-actions.js'; +import { HOLD } from './types.js'; + +describe('guard conformance', () => { + it('every holding action pairs with a registered approval handler', () => { + const holding = listGuardedActions().filter((spec) => spec.grantActionName); + expect(holding.length).toBeGreaterThan(0); + + const dangling = holding.filter((spec) => !getApprovalHandler(spec.grantActionName as string)); + expect(dangling.map((s) => s.action)).toEqual([]); + }); + + it('every mutating ncl command derives a guard that holds via cli_command', () => { + const mutating = listCommands().filter((cmd) => cmd.access === 'approval'); + expect(mutating.length).toBeGreaterThan(0); + + const wrong = mutating.filter((cmd) => commandGuard(cmd.name).grantActionName !== 'cli_command'); + expect(wrong.map((c) => c.name)).toEqual([]); + }); + + it('the domain catalog entries are defined once the module barrels load', () => { + const actions = new Set(listGuardedActions().map((s) => s.action)); + for (const expected of [ + 'agents.create', + 'a2a.send', + 'self_mod.install_packages', + 'self_mod.add_mcp_server', + 'senders.admit', + 'channels.register', + ]) { + expect(actions.has(expected), `catalog is missing "${expected}"`).toBe(true); + } + }); + + it('defining the same action twice throws — names are the catalog key', () => { + defineGuardedAction({ action: 'test.dup-define', decide: () => HOLD('x') }); + expect(() => defineGuardedAction({ action: 'test.dup-define', decide: () => HOLD('x') })).toThrow( + /already defined/, + ); + }); +}); diff --git a/src/guard/guard-actions.ts b/src/guard/guard-actions.ts new file mode 100644 index 00000000000..09f139a3ed0 --- /dev/null +++ b/src/guard/guard-actions.ts @@ -0,0 +1,73 @@ +/** + * The action catalog — the enforcement boundary. + * + * An action either is defined here (and every consult passes its decision) + * or cannot be consulted at all: guard() takes the GuardedAction VALUE + * returned by defineGuardedAction, so the wiring between a consult site and + * its decide fn is a symbol reference the compiler checks. A dropped + * module-edge import or a typo'd action name is a build error, not a + * runtime fail-open — there is no lookup that can miss. + * + * Definitions are still recorded by name so the catalog can be enumerated: + * the conformance test pairs every holding action with its registered + * approval handler, and duplicate names are refused at definition time + * (grants match on the name). + */ +import type { GuardDecision, GuardInput } from './types.js'; +import type { PendingApproval } from '../types.js'; + +export interface GuardedActionSpec { + /** Dotted action name, e.g. 'roles.grant', 'agents.create', 'a2a.send'. */ + action: string; + /** + * Today's structural checks for this action, verbatim — the only source of + * allow. Runs on every consult, including approved replays (a grant + * satisfies a hold, never a deny). + */ + decide: (input: GuardInput) => GuardDecision; + /** + * The pending_approvals.action its holds resolve through — a grant is only + * accepted when its row carries this action. Omit for actions that can + * never be held (deny/allow-only decisions). + */ + grantActionName?: string; + /** + * Extra domain binding between a grant and the replayed input (e.g. the + * a2a target must match the held message). Runs in addition to the + * grantActionName + live-row checks. + */ + grantCoversRequest?: (grant: PendingApproval, input: GuardInput) => boolean; +} + +declare const guardedActionBrand: unique symbol; +/** + * A defined guarded action — only defineGuardedAction can mint one. The + * brand makes the type nominal: a hand-rolled { action, decide } object + * does not typecheck at a consult site, and fails the runtime check too. + */ +export type GuardedAction = Readonly & { readonly [guardedActionBrand]: true }; + +const defined = new Map(); +const minted = new WeakSet(); + +export function defineGuardedAction(spec: GuardedActionSpec): GuardedAction { + if (defined.has(spec.action)) { + throw new Error(`guarded action "${spec.action}" is already defined — action names are the catalog key`); + } + const def = Object.freeze({ ...spec }) as GuardedAction; + minted.add(def); + defined.set(spec.action, def); + return def; +} + +/** + * Runtime backstop for callers outside the type system (plain JS, casts): + * only values minted by defineGuardedAction pass — guard() denies the rest. + */ +export function isGuardedAction(value: unknown): value is GuardedAction { + return typeof value === 'object' && value !== null && minted.has(value); +} + +export function listGuardedActions(): GuardedAction[] { + return [...defined.values()].sort((a, b) => a.action.localeCompare(b.action)); +} diff --git a/src/guard/guard.test.ts b/src/guard/guard.test.ts new file mode 100644 index 00000000000..11b3f909057 --- /dev/null +++ b/src/guard/guard.test.ts @@ -0,0 +1,160 @@ +/** + * Guard decision-function unit tests: decide is the decision (allow / + * hold / deny returned as-is), grant semantics (satisfies holds, never + * denies; invalid → refuse), the runtime backstop against forged action + * values, and the fail-closed posture on a throwing decide. + * + * Uses synthetic actions defined per test — the catalog is per-worker module + * state with no reset, so action names are unique. + */ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +import { guard } from './guard.js'; +import { defineGuardedAction, type GuardedAction } from './guard-actions.js'; +import { ALLOW, DENY, HOLD, type GuardInput } from './types.js'; + +const mockGetPendingApproval = vi.fn(); +vi.mock('../db/sessions.js', () => ({ + getPendingApproval: (...args: unknown[]) => mockGetPendingApproval(...args), +})); +vi.mock('../log.js', () => ({ + log: { info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() }, +})); + +const AGENT = { kind: 'agent', agentGroupId: 'ag-1', sessionId: 'sess-1' } as const; + +function input(extra: Partial = {}): GuardInput { + return { actor: AGENT, payload: {}, ...extra }; +} + +beforeEach(() => { + mockGetPendingApproval.mockReset(); +}); + +afterEach(() => { + vi.clearAllMocks(); +}); + +describe('decide is the decision', () => { + it('decide allow → allow', () => { + const action = defineGuardedAction({ action: 't.allow1', decide: () => ALLOW('ok') }); + expect(guard(action, input()).effect).toBe('allow'); + }); + + it('decide hold → hold, default approver chain', () => { + const action = defineGuardedAction({ action: 't.hold1', decide: () => HOLD('needs approval') }); + const d = guard(action, input()); + expect(d.effect).toBe('hold'); + if (d.effect === 'hold') { + expect(d.reason).toBe('needs approval'); + expect(d.approverUserId).toBeUndefined(); + } + }); + + it('decide hold → hold, carrying a named approver', () => { + const action = defineGuardedAction({ action: 't.hold2', decide: () => HOLD('policy row', 'telegram:dana') }); + const d = guard(action, input()); + expect(d.effect).toBe('hold'); + if (d.effect === 'hold') expect(d.approverUserId).toBe('telegram:dana'); + }); + + it('decide deny → deny, carrying the reason', () => { + const action = defineGuardedAction({ action: 't.deny1', decide: () => DENY('structurally unauthorized') }); + const d = guard(action, input()); + expect(d.effect).toBe('deny'); + if (d.effect === 'deny') expect(d.reason).toBe('structurally unauthorized'); + }); + + it('a forged action value (not from defineGuardedAction) is denied', () => { + const forged = { action: 't.forged', decide: () => ALLOW('never vetted') } as unknown as GuardedAction; + const d = guard(forged, input()); + expect(d.effect).toBe('deny'); + if (d.effect === 'deny') expect(d.reason).toContain('undefined action'); + }); +}); + +describe('grants', () => { + const grantRow = (action: string) => + ({ approval_id: 'appr-1', action, payload: '{}' }) as unknown as NonNullable; + + it('a valid live grant satisfies a hold', () => { + const action = defineGuardedAction({ + action: 't.g1', + grantActionName: 'g1_approved', + decide: () => HOLD('b'), + }); + const grant = grantRow('g1_approved'); + mockGetPendingApproval.mockReturnValue(grant); + expect(guard(action, input({ grant })).effect).toBe('allow'); + }); + + it('a grant never satisfies a deny — the checks re-run live', () => { + const action = defineGuardedAction({ + action: 't.g2', + grantActionName: 'g2_approved', + decide: () => DENY('revoked since'), + }); + const grant = grantRow('g2_approved'); + mockGetPendingApproval.mockReturnValue(grant); + const d = guard(action, input({ grant })); + expect(d.effect).toBe('deny'); + if (d.effect === 'deny') expect(d.reason).toBe('revoked since'); + }); + + it('a dead grant (row deleted) refuses instead of re-holding', () => { + const action = defineGuardedAction({ + action: 't.g3', + grantActionName: 'g3_approved', + decide: () => HOLD('b'), + }); + mockGetPendingApproval.mockReturnValue(undefined); + const d = guard(action, input({ grant: grantRow('g3_approved') })); + expect(d.effect).toBe('deny'); + }); + + it("a grant for a different action doesn't transfer", () => { + const action = defineGuardedAction({ + action: 't.g4', + grantActionName: 'g4_approved', + decide: () => HOLD('b'), + }); + const grant = grantRow('other_action'); + mockGetPendingApproval.mockReturnValue(grant); + expect(guard(action, input({ grant })).effect).toBe('deny'); + }); + + it('a domain grantCoversRequest binding can refuse a payload mismatch', () => { + const action = defineGuardedAction({ + action: 't.g5', + grantActionName: 'g5_approved', + grantCoversRequest: () => false, + decide: () => HOLD('b'), + }); + const grant = grantRow('g5_approved'); + mockGetPendingApproval.mockReturnValue(grant); + expect(guard(action, input({ grant })).effect).toBe('deny'); + }); + + it('a grant on an already-allowed action is a no-op', () => { + const action = defineGuardedAction({ + action: 't.g6', + grantActionName: 'g6_approved', + decide: () => ALLOW('ok'), + }); + const grant = grantRow('g6_approved'); + mockGetPendingApproval.mockReturnValue(grant); + expect(guard(action, input({ grant })).effect).toBe('allow'); + }); +}); + +describe('fail-closed posture', () => { + it('a throwing decide denies', () => { + const action = defineGuardedAction({ + action: 't.f1', + decide: () => { + throw new Error('boom'); + }, + }); + expect(guard(action, input()).effect).toBe('deny'); + }); +}); diff --git a/src/guard/guard.ts b/src/guard/guard.ts new file mode 100644 index 00000000000..327c698b1ea --- /dev/null +++ b/src/guard/guard.ts @@ -0,0 +1,72 @@ +/** + * guard() — the one decision function every privileged action consults. + * + * The decision is the action's decide fn — today's code checks, + * defined per action at the module edges. The consult site holds the + * GuardedAction value itself (defineGuardedAction), so there is no name + * lookup and no fail-open path for an unknown action: an unwired consult is + * a compile error, and a value that didn't come from defineGuardedAction is + * denied at runtime. Policy-as-data (tighten-only rule sources composing + * with the decision) is deliberately deferred — a generalized rules table + * can arrive later, with its first operator-visible consumer; until then + * the one policy table (agent_message_policies) is consulted inside + * a2a.send's decide. + * + * Grants: an approved replay carries the verified approval row. A valid + * grant (live pending row whose action matches the entry's approval action, + * plus any domain binding) satisfies a hold — the human already decided — + * but NEVER a deny: the checks re-run live, so approve-then-revoke + * no longer executes. A grant that is present but invalid fails closed to + * deny (no second card). + * + * The guard itself fails closed: a throwing decide denies. + */ +import { getPendingApproval } from '../db/sessions.js'; +import { log } from '../log.js'; +import { isGuardedAction, type GuardedAction } from './guard-actions.js'; +import { ALLOW, DENY, type GuardDecision, type GuardInput } from './types.js'; + +export function guard(action: GuardedAction, input: GuardInput): GuardDecision { + if (!isGuardedAction(action)) { + // JS-level backstop — the branded type already forbids this. A + // hand-rolled object must not carry a decide fn never vetted at + // definition time. + log.error('Guard consulted with an undefined action — failing closed', { + action: (action as { action?: unknown } | null)?.action, + }); + return DENY('guard consulted with an undefined action (failing closed)'); + } + + let decision: GuardDecision; + try { + decision = action.decide(input); + } catch (err) { + log.error('Guard evaluation threw — failing closed', { action: action.action, err }); + return DENY('guard failure (failing closed)'); + } + + if (!input.grant || decision.effect !== 'hold') { + // A grant never loosens a deny (the checks re-run live), and a + // grant on an already-allowed action is a no-op. + return decision; + } + + // An invalid grant on a replay is a refusal, not a fresh hold — approved + // replays must execute exactly once. + if (grantSatisfies(action, input)) { + return ALLOW(`hold satisfied by approval ${input.grant.approval_id}`); + } + return DENY('replay carried an invalid or mismatched grant'); +} + +function grantSatisfies(action: GuardedAction, input: GuardInput): boolean { + const grant = input.grant; + if (!grant || !action.grantActionName) return false; + if (grant.action !== action.grantActionName) return false; + // The row must still be live — resolution deletes it, so a grant can only + // execute once and a fabricated row object doesn't pass. + const live = getPendingApproval(grant.approval_id); + if (!live || live.action !== action.grantActionName) return false; + if (action.grantCoversRequest && !action.grantCoversRequest(grant, input)) return false; + return true; +} diff --git a/src/guard/index.ts b/src/guard/index.ts new file mode 100644 index 00000000000..909ce5724ac --- /dev/null +++ b/src/guard/index.ts @@ -0,0 +1,29 @@ +/** + * Guard — the privileged-action decision seam. + * + * One decision function (guard.ts) and a definition-derived action + * catalog (guard-actions.ts). Consults carry the GuardedAction value returned by + * defineGuardedAction — never a name to look up — so mis-wiring is a build + * error, not a runtime fail-open. + * Domain-free leaf: domain decisions are defined at the domain modules' edges. + */ +export { guard } from './guard.js'; +export { + defineGuardedAction, + isGuardedAction, + listGuardedActions, + type GuardedAction, + type GuardedActionSpec, +} from './guard-actions.js'; +export { + ALLOW, + DENY, + GuardDenyError, + HOLD, + isUnguarded, + unguarded, + type GuardActor, + type GuardDecision, + type GuardInput, + type Unguarded, +} from './types.js'; diff --git a/src/guard/types.ts b/src/guard/types.ts new file mode 100644 index 00000000000..121ed39edea --- /dev/null +++ b/src/guard/types.ts @@ -0,0 +1,88 @@ +/** + * Guard vocabulary — the decision seam every privileged action passes. + * + * The guard is a domain-free leaf: this module may import the DB read layer, + * config, log, and shared types — never src/cli/* or src/modules/*. Domain + * knowledge (what an action's decide fn checks) arrives via + * definition: domain modules call defineGuardedAction (guard-actions.ts) at + * their module edges and pass the returned value to every consult and + * registration site — the wiring is a symbol reference the compiler checks. + */ +import type { PendingApproval } from '../types.js'; + +/** Who is attempting the action. Mirrors the CLI CallerContext + click identities. */ +export type GuardActor = + | { kind: 'host' } + | { kind: 'agent'; agentGroupId: string; sessionId?: string } + | { kind: 'human'; userId: string } + | { kind: 'system' }; + +export interface GuardInput { + actor: GuardActor; + /** Domain resource reference, e.g. { from, to } for a2a.send. */ + resource?: Record; + /** Action arguments — what the card summarizes and rules may later match on. */ + payload: Record; + /** + * Verified approval row carried by an approved replay. A valid grant + * satisfies a hold (the human already decided) but never a deny — the + * structural checks re-run live on every replay. + */ + grant?: PendingApproval | null; +} + +const unguardedBrand = Symbol('unguarded'); +/** + * A registration that deliberately carries no guard. Where a registry takes + * a declaration (delivery actions), omission is not representable — + * registration requires either a guard spec or this marker, so the decision + * to run unguarded is visible, and justified, in the diff that registers + * the handler. The reason travels with the registration; + * `grep "unguarded("` is the complete inventory. + */ +export type Unguarded = { readonly reason: string; readonly [unguardedBrand]: true }; + +export function unguarded(reason: string): Unguarded { + return Object.freeze({ reason, [unguardedBrand]: true as const }); +} + +/** + * The one runtime discriminator for guard declarations. The brand symbol is + * module-private, so `unguarded()` is the only mint — a look-alike + * `{ reason }` object (or a guard spec that someday grows a `reason` field) + * doesn't pass. + */ +export function isUnguarded(decl: object): decl is Unguarded { + return unguardedBrand in decl; +} + +export type GuardDecision = + | { effect: 'allow'; reason: string } + | { effect: 'hold'; reason: string; approverUserId?: string } + | { effect: 'deny'; reason: string }; + +export const ALLOW = (reason: string): GuardDecision => ({ effect: 'allow', reason }); +export const DENY = (reason: string): GuardDecision => ({ effect: 'deny', reason }); +/** + * approverUserId names an exclusive approver for the hold (the a2a policy + * row's named approver). Absent, the hold goes to the approvals primitive's + * default chain (scoped admins → global admins → owners). + */ +export const HOLD = (reason: string, approverUserId?: string): GuardDecision => ({ + effect: 'hold', + reason, + approverUserId, +}); + +/** + * A guard deny travelling as an exception — for flows whose entry point + * signals refusal by throwing (the a2a route). Catching it lets a caller + * distinguish "the guard refused, as designed" (report to the requester, + * log a warning) from a real runtime failure (crash path, stack trace). + */ +export class GuardDenyError extends Error { + constructor(reason: string) { + super(reason); + this.name = 'GuardDenyError'; + } +} diff --git a/src/modules/agent-to-agent/agent-route.ts b/src/modules/agent-to-agent/agent-route.ts index 5ad169a1444..a2b45dad4b2 100644 --- a/src/modules/agent-to-agent/agent-route.ts +++ b/src/modules/agent-to-agent/agent-route.ts @@ -27,14 +27,15 @@ import { getAgentGroup } from '../../db/agent-groups.js'; import { getInboundSourceSessionId, getMostRecentPeerSourceSessionId } from '../../db/session-db.js'; import { getSession } from '../../db/sessions.js'; import { wakeContainer } from '../../container-runner.js'; +import { GuardDenyError, guard } from '../../guard/index.js'; import { log } from '../../log.js'; import { openInboundDb, resolveSession, sessionDir, writeSessionMessage } from '../../session-manager.js'; -import type { Session } from '../../types.js'; +import type { PendingApproval, Session } from '../../types.js'; import { requestApproval } from '../approvals/index.js'; -import { hasDestination } from './db/agent-destinations.js'; -import { getMessagePolicy } from './db/agent-message-policies.js'; +import { A2A_MESSAGE_GATE_ACTION, a2aSend } from './guard.js'; export { isSafeAttachmentName }; +export { A2A_MESSAGE_GATE_ACTION } from './guard.js'; export interface ForwardedAttachment { name: string; @@ -230,56 +231,64 @@ function resolveTargetSession(msg: RoutableAgentMessage, sourceSession: Session, return resolveSession(targetAgentGroupId, null, null, 'agent-shared').session; } -export async function routeAgentMessage(msg: RoutableAgentMessage, session: Session): Promise { +export async function routeAgentMessage( + msg: RoutableAgentMessage, + session: Session, + opts: { grant?: PendingApproval } = {}, +): Promise { const sourceAgentGroupId = session.agent_group_id; const targetAgentGroupId = msg.platform_id; if (!targetAgentGroupId) { throw new Error(`agent-to-agent message ${msg.id} is missing a target agent group id`); } - const isSelf = targetAgentGroupId === sourceAgentGroupId; - if (!isSelf && !hasDestination(sourceAgentGroupId, 'agent', targetAgentGroupId)) { - throw new Error(`unauthorized agent-to-agent: ${sourceAgentGroupId} has no destination for ${targetAgentGroupId}`); - } - if (!getAgentGroup(targetAgentGroupId)) { - throw new Error(`target agent group ${targetAgentGroupId} not found for message ${msg.id}`); + + // The a2a.send decision (guard.ts) carries the checks verbatim in their + // original order: destination ACL deny, target-exists deny, self-send + // allow, agent_message_policies hold. An approved replay carries the + // grant — the hold is satisfied but the structure is re-checked live, so + // revoking a destination between hold and approve blocks delivery. + const decision = guard(a2aSend, { + actor: { kind: 'agent', agentGroupId: sourceAgentGroupId, sessionId: session.id }, + resource: { from: sourceAgentGroupId, to: targetAgentGroupId }, + payload: { id: msg.id, platform_id: targetAgentGroupId, content: msg.content, in_reply_to: msg.in_reply_to }, + grant: opts.grant ?? null, + }); + + if (decision.effect === 'deny') { + throw new GuardDenyError(decision.reason); } // Gated edge: hold the message and return (not throw) so the delivery loop - // consumes the outbound row; `applyA2aMessageGate` re-routes it on approve. - if (!isSelf) { - const policy = getMessagePolicy(sourceAgentGroupId, targetAgentGroupId); - if (policy) { - const { approver } = policy; - const sourceName = getAgentGroup(sourceAgentGroupId)?.name ?? sourceAgentGroupId; - const targetName = getAgentGroup(targetAgentGroupId)?.name ?? targetAgentGroupId; - await requestApproval({ - session, - agentName: sourceName, - action: A2A_MESSAGE_GATE_ACTION, - approverUserId: approver, - title: 'Message approval', - question: buildGateQuestion(sourceName, targetName, msg.content), - payload: { - id: msg.id, - platform_id: targetAgentGroupId, - content: msg.content, - in_reply_to: msg.in_reply_to, - }, - }); - log.info('Agent message held for approval', { - from: sourceAgentGroupId, - to: targetAgentGroupId, - msgId: msg.id, - }); - return; - } + // consumes the outbound row; `applyA2aMessageGate` re-enters here with the + // grant on approve. + if (decision.effect === 'hold') { + const sourceName = getAgentGroup(sourceAgentGroupId)?.name ?? sourceAgentGroupId; + const targetName = getAgentGroup(targetAgentGroupId)?.name ?? targetAgentGroupId; + await requestApproval({ + session, + agentName: sourceName, + action: A2A_MESSAGE_GATE_ACTION, + approverUserId: decision.approverUserId, + title: 'Message approval', + question: buildGateQuestion(sourceName, targetName, msg.content), + payload: { + id: msg.id, + platform_id: targetAgentGroupId, + content: msg.content, + in_reply_to: msg.in_reply_to, + }, + }); + log.info('Agent message held for approval', { + from: sourceAgentGroupId, + to: targetAgentGroupId, + msgId: msg.id, + }); + return; } await performAgentRoute(msg, session, targetAgentGroupId); } -export const A2A_MESSAGE_GATE_ACTION = 'a2a_message_gate'; - const GATE_CARD_BODY_MAX = 1500; function parseMessageContent(contentStr: string): { text: string; files: string[] } { @@ -308,9 +317,11 @@ function buildGateQuestion(sourceName: string, targetName: string, contentStr: s /** * Cross-session route: pick the target session, forward files, write to its - * inbound DB, wake it. Authorization is the caller's responsibility. + * inbound DB, wake it. Module-private — the only door is routeAgentMessage's + * guard decision (the approve continuation re-enters with a grant rather + * than calling this directly). */ -export async function performAgentRoute( +async function performAgentRoute( msg: RoutableAgentMessage, session: Session, targetAgentGroupId: string, diff --git a/src/modules/agent-to-agent/create-agent.test.ts b/src/modules/agent-to-agent/create-agent.test.ts index f9f6219c784..96ebb920f6c 100644 --- a/src/modules/agent-to-agent/create-agent.test.ts +++ b/src/modules/agent-to-agent/create-agent.test.ts @@ -2,26 +2,48 @@ * Tests for create_agent host-side authorization. * * Regression guard for the audit finding: `create_agent` is a privileged - * central-DB write with no host-side authz. The fix authorizes by CLI scope — - * trusted owner agent groups ('global') create directly; confined groups - * ('group', the default and the prompt-injection victim) must get admin - * approval. These tests pin that branch decision. + * central-DB write with no host-side authz. Authorization is the guard's + * `agents.create` decision — trusted owner agent groups ('global') create + * directly; confined groups ('group', the default and the prompt-injection + * victim) hold for admin approval. These tests drive the REAL wrapped + * delivery action (the only reachable path) and the approve continuation's + * grant-carrying re-entry. */ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; -import type { Session } from '../../types.js'; +import type { PendingApproval, Session } from '../../types.js'; // Mocks for the collaborators the branch decides between / depends on. -const mockRequestApproval = vi.fn().mockResolvedValue(undefined); -const mockGetContainerConfig = vi.fn(); -const mockCreateAgentGroup = vi.fn(); -const mockInitGroupFilesystem = vi.fn(); -const mockUpdateScalars = vi.fn(); -const mockWriteDestinations = vi.fn(); -const mockNotifyWrite = vi.fn(); +// vi.hoisted: the module barrel import below runs before this file's const +// initializers, and the mock factories close over this state. +const { + mockRequestApproval, + mockGetContainerConfig, + mockCreateAgentGroup, + mockInitGroupFilesystem, + mockUpdateScalars, + mockWriteDestinations, + mockNotifyWrite, + liveApprovals, + approvalHandlers, +} = vi.hoisted(() => ({ + mockRequestApproval: vi.fn().mockResolvedValue(undefined), + mockGetContainerConfig: vi.fn(), + mockCreateAgentGroup: vi.fn(), + mockInitGroupFilesystem: vi.fn(), + mockUpdateScalars: vi.fn(), + mockWriteDestinations: vi.fn(), + mockNotifyWrite: vi.fn(), + liveApprovals: new Map(), + approvalHandlers: new Map) => Promise>(), +})); vi.mock('../approvals/index.js', () => ({ requestApproval: (...a: unknown[]) => mockRequestApproval(...a), + notifyAgent: vi.fn(), + registerApprovalHandler: (action: string, handler: (ctx: Record) => Promise) => { + approvalHandlers.set(action, handler); + }, })); vi.mock('../../db/container-configs.js', () => ({ getContainerConfig: (...a: unknown[]) => mockGetContainerConfig(...a), @@ -42,36 +64,81 @@ vi.mock('./write-destinations.js', () => ({ vi.mock('./db/agent-destinations.js', () => ({ getDestinationByName: () => undefined, createDestination: vi.fn(), + hasDestination: () => true, normalizeName: (s: string) => s.toLowerCase().replace(/[^a-z0-9]+/g, '-'), })); // notifyAgent writes to the session inbound.db + wakes the container; stub both. +// delivery.ts and agent-route.ts pull more session-manager exports at import time. vi.mock('../../session-manager.js', () => ({ writeSessionMessage: (...a: unknown[]) => mockNotifyWrite(...a), + openInboundDb: vi.fn(), + openOutboundDb: vi.fn(), + clearOutbox: vi.fn(), + readOutboxFiles: vi.fn().mockReturnValue([]), + resolveSession: vi.fn(), + sessionDir: vi.fn().mockReturnValue('/tmp/nowhere'), + inboundDbPath: vi.fn().mockReturnValue('/tmp/nowhere/inbound.db'), })); vi.mock('../../container-runner.js', () => ({ wakeContainer: vi.fn().mockResolvedValue(undefined), })); vi.mock('../../db/sessions.js', () => ({ getSession: (id: string) => ({ id, agent_group_id: 'ag-1' }), + getPendingApproval: (id: string) => liveApprovals.get(id), + getRunningSessions: () => [], + getActiveSessions: () => [], + createPendingQuestion: vi.fn(), })); -import { handleCreateAgent } from './create-agent.js'; +// The a2a module barrel registers ./guard.js (catalog entries) and the +// guard-wrapped create_agent delivery action — the path under test. +import './index.js'; +import { getDeliveryAction } from '../../delivery.js'; const SESSION = { id: 'sess-1', agent_group_id: 'ag-1' } as Session; +async function runCreateAgent(content: Record): Promise { + const wrapped = getDeliveryAction('create_agent'); + expect(wrapped).toBeDefined(); + await wrapped!(content, SESSION, undefined as never); +} + +function liveGrant(approvalId: string, payload: Record): PendingApproval { + const row = { + approval_id: approvalId, + session_id: SESSION.id, + request_id: approvalId, + action: 'create_agent', + payload: JSON.stringify(payload), + created_at: new Date().toISOString(), + agent_group_id: 'ag-1', + channel_type: null, + platform_id: null, + platform_message_id: null, + expires_at: null, + status: 'pending', + title: '', + options_json: '[]', + approver_user_id: null, + } as PendingApproval; + liveApprovals.set(approvalId, row); + return row; +} + beforeEach(() => { vi.clearAllMocks(); + liveApprovals.clear(); }); afterEach(() => { vi.restoreAllMocks(); }); -describe('handleCreateAgent — scope-based authorization', () => { +describe('create_agent — guard-based authorization (wrapped delivery action)', () => { it('global scope: creates directly, no approval requested', async () => { mockGetContainerConfig.mockReturnValue({ cli_scope: 'global' }); - await handleCreateAgent({ name: 'Scout', instructions: 'help' }, SESSION); + await runCreateAgent({ name: 'Scout', instructions: 'help' }); expect(mockRequestApproval).not.toHaveBeenCalled(); expect(mockCreateAgentGroup).toHaveBeenCalledTimes(1); @@ -84,7 +151,7 @@ describe('handleCreateAgent — scope-based authorization', () => { // dropping the inheritance leaves the child provider-less (→ claude). mockGetContainerConfig.mockReturnValue({ cli_scope: 'global', provider: 'codex' }); - await handleCreateAgent({ name: 'Scout', instructions: 'help' }, SESSION); + await runCreateAgent({ name: 'Scout', instructions: 'help' }); expect(mockInitGroupFilesystem).toHaveBeenCalledWith( expect.anything(), @@ -96,7 +163,7 @@ describe('handleCreateAgent — scope-based authorization', () => { it('claude creator leaves the child provider unset (built-in default)', async () => { mockGetContainerConfig.mockReturnValue({ cli_scope: 'global' }); // no provider - await handleCreateAgent({ name: 'Scout', instructions: 'help' }, SESSION); + await runCreateAgent({ name: 'Scout', instructions: 'help' }); expect(mockUpdateScalars).not.toHaveBeenCalled(); }); @@ -104,7 +171,7 @@ describe('handleCreateAgent — scope-based authorization', () => { it('group scope (default): requires approval, does NOT create directly', async () => { mockGetContainerConfig.mockReturnValue({ cli_scope: 'group' }); - await handleCreateAgent({ name: 'Scout', instructions: 'help' }, SESSION); + await runCreateAgent({ name: 'Scout', instructions: 'help' }); expect(mockRequestApproval).toHaveBeenCalledTimes(1); expect(mockRequestApproval.mock.calls[0][0]).toMatchObject({ action: 'create_agent' }); @@ -115,7 +182,7 @@ describe('handleCreateAgent — scope-based authorization', () => { it('missing config: fails closed to approval (no direct create)', async () => { mockGetContainerConfig.mockReturnValue(undefined); - await handleCreateAgent({ name: 'Scout' }, SESSION); + await runCreateAgent({ name: 'Scout' }); expect(mockRequestApproval).toHaveBeenCalledTimes(1); expect(mockCreateAgentGroup).not.toHaveBeenCalled(); @@ -124,7 +191,7 @@ describe('handleCreateAgent — scope-based authorization', () => { it('disabled/other scope: requires approval', async () => { mockGetContainerConfig.mockReturnValue({ cli_scope: 'disabled' }); - await handleCreateAgent({ name: 'Scout' }, SESSION); + await runCreateAgent({ name: 'Scout' }); expect(mockRequestApproval).toHaveBeenCalledTimes(1); expect(mockCreateAgentGroup).not.toHaveBeenCalled(); @@ -133,9 +200,58 @@ describe('handleCreateAgent — scope-based authorization', () => { it('empty name: neither creates nor requests approval', async () => { mockGetContainerConfig.mockReturnValue({ cli_scope: 'global' }); - await handleCreateAgent({ name: '' }, SESSION); + await runCreateAgent({ name: '' }); expect(mockRequestApproval).not.toHaveBeenCalled(); expect(mockCreateAgentGroup).not.toHaveBeenCalled(); }); }); + +describe('create_agent — approved replay (grant-carrying re-entry)', () => { + it('valid grant executes exactly once — decide hold is satisfied, create runs', async () => { + mockGetContainerConfig.mockReturnValue({ cli_scope: 'group' }); + const payload = { name: 'Scout', instructions: 'help' }; + const approval = liveGrant('appr-ca-1', payload); + + const continuation = approvalHandlers.get('create_agent'); + expect(continuation).toBeDefined(); + await continuation!({ session: SESSION, payload, approval, userId: 'telegram:admin', notify: vi.fn() }); + + expect(mockCreateAgentGroup).toHaveBeenCalledTimes(1); + expect(mockRequestApproval).not.toHaveBeenCalled(); // no second card + }); + + it('dead grant (row already resolved) refuses the replay', async () => { + mockGetContainerConfig.mockReturnValue({ cli_scope: 'group' }); + const payload = { name: 'Scout', instructions: 'help' }; + const approval = liveGrant('appr-ca-2', payload); + liveApprovals.delete('appr-ca-2'); // resolution consumed the row + + await approvalHandlers.get('create_agent')!({ + session: SESSION, + payload, + approval, + userId: 'telegram:admin', + notify: vi.fn(), + }); + + expect(mockCreateAgentGroup).not.toHaveBeenCalled(); + expect(mockRequestApproval).not.toHaveBeenCalled(); // refused, not re-held + }); + + it('mismatched grant (approved for a different name) refuses the replay', async () => { + mockGetContainerConfig.mockReturnValue({ cli_scope: 'group' }); + const approval = liveGrant('appr-ca-3', { name: 'OtherAgent' }); + + await approvalHandlers.get('create_agent')!({ + session: SESSION, + payload: { name: 'Scout' }, + approval, + userId: 'telegram:admin', + notify: vi.fn(), + }); + + expect(mockCreateAgentGroup).not.toHaveBeenCalled(); + expect(mockRequestApproval).not.toHaveBeenCalled(); + }); +}); diff --git a/src/modules/agent-to-agent/create-agent.ts b/src/modules/agent-to-agent/create-agent.ts index b8044da390d..4f8e557c8c5 100644 --- a/src/modules/agent-to-agent/create-agent.ts +++ b/src/modules/agent-to-agent/create-agent.ts @@ -1,16 +1,18 @@ /** - * `create_agent` delivery-action handler. + * `create_agent` delivery-action bodies. * * SECURITY: `create_agent` writes to the CENTRAL DB (agent_groups, * container_configs, agent_destinations) and scaffolds host filesystem state — * a privileged operation a confined container is otherwise architecturally * barred from. The container's MCP tool gate is inside the (untrusted) * container and is trivially bypassed by writing the outbound system row - * directly, so authorization MUST be enforced host-side. Trusted owner agent - * groups (CLI scope 'global') create directly; every other (confined) group - * requires admin approval via `requestApproval` — matching `ncl groups create` - * (access: 'approval') and the self-mod actions. `applyCreateAgent` runs the - * creation on approve; `performCreateAgent` is the shared body. + * directly, so authorization MUST be enforced host-side: the delivery + * registry wraps this action with the guard, whose `agents.create` decision + * (./guard.ts) is the old cli_scope branch verbatim — trusted global-scope + * groups allow, everything else (including unknown config, fail-closed) + * holds for admin approval. On approve the continuation re-enters the + * wrapped action with the approval row as its grant and `createAgent` runs. + * `performCreateAgent` is the module-private body. */ import path from 'path'; @@ -23,7 +25,7 @@ import { initGroupFilesystem } from '../../group-init.js'; import { log } from '../../log.js'; import { writeSessionMessage } from '../../session-manager.js'; import type { AgentGroup, Session } from '../../types.js'; -import { requestApproval, type ApprovalHandler } from '../approvals/index.js'; +import { requestApproval } from '../approvals/index.js'; import { createDestination, getDestinationByName, normalizeName } from './db/agent-destinations.js'; import { writeDestinations } from './write-destinations.js'; @@ -43,41 +45,27 @@ function notifyAgent(session: Session, text: string): void { } } -/** - * Delivery-action entry. - * - * Authorization depends on the calling group's CLI scope: - * - `global` (set by init-first-agent for trusted owner agent groups): - * create immediately. create_agent is the intended primitive for these - * privileged agents, and an approval tap on every sub-agent spawn would be - * needless friction. - * - anything else (the default `group` scope — the realistic - * prompt-injection victim): require an admin to approve before any - * central-DB write. `applyCreateAgent` runs on approve. - * Unknown/missing config fails closed to the approval path. - */ -export async function handleCreateAgent(content: Record, session: Session): Promise { +/** Guard precheck: malformed requests are answered without ever creating a hold. */ +export function validateCreateAgent(content: Record, session: Session): boolean { const name = typeof content.name === 'string' ? content.name : ''; - const instructions = typeof content.instructions === 'string' ? content.instructions : null; - if (!name) { notifyAgent(session, 'create_agent failed: name is required.'); - return; + return false; } - - const sourceGroup = getAgentGroup(session.agent_group_id); - if (!sourceGroup) { + if (!getAgentGroup(session.agent_group_id)) { notifyAgent(session, 'create_agent failed: source agent group not found.'); log.warn('create_agent failed: missing source group', { sessionAgentGroup: session.agent_group_id, name }); - return; + return false; } + return true; +} - const cliScope = getContainerConfig(session.agent_group_id)?.cli_scope ?? 'group'; - if (cliScope === 'global') { - // Trusted owner agent group — create directly, then notify (+wake) it. - await performCreateAgent(name, instructions, session, sourceGroup, (text) => notifyAgent(session, text)); - return; - } +/** Guard hold: card the requesting group's admin chain. */ +export async function requestCreateAgentHold(content: Record, session: Session): Promise { + const name = typeof content.name === 'string' ? content.name : ''; + const instructions = typeof content.instructions === 'string' ? content.instructions : null; + const sourceGroup = getAgentGroup(session.agent_group_id); + if (!sourceGroup) return; await requestApproval({ session, @@ -89,35 +77,22 @@ export async function handleCreateAgent(content: Record, sessio }); } -/** - * Approval handler: performs the creation once an admin approves a request from - * a confined (non-global) agent group. `session` is the requesting parent. - */ -export const applyCreateAgent: ApprovalHandler = async ({ session, payload, notify }) => { - const name = typeof payload.name === 'string' ? payload.name : ''; - const instructions = typeof payload.instructions === 'string' ? payload.instructions : null; - - if (!name) { - notify('create_agent approved but the request had no name.'); - return; - } - +/** Guard allow body: performs the creation (fresh global-scope call or approved replay). */ +export async function createAgent(content: Record, session: Session): Promise { + const name = typeof content.name === 'string' ? content.name : ''; + const instructions = typeof content.instructions === 'string' ? content.instructions : null; const sourceGroup = getAgentGroup(session.agent_group_id); - if (!sourceGroup) { - notify('create_agent approved but the source agent group no longer exists.'); - log.warn('create_agent apply failed: missing source group', { sessionAgentGroup: session.agent_group_id, name }); - return; - } + if (!name || !sourceGroup) return; // precheck already answered the requester - await performCreateAgent(name, instructions, session, sourceGroup, notify); -}; + await performCreateAgent(name, instructions, session, sourceGroup, (text) => notifyAgent(session, text)); +} /** * Core creation: writes the new agent group + bidirectional destinations and * scaffolds its filesystem, then reports via `notify`. Authorization is the - * CALLER's responsibility (the global-scope shortcut in handleCreateAgent or - * admin approval via applyCreateAgent) — never call this from an unauthorized - * path, as it performs privileged central-DB writes a confined container is + * CALLER's responsibility (the guard's agents.create decision) — never call + * this from an unauthorized path, as it performs privileged central-DB + * writes a confined container is * otherwise barred from. */ async function performCreateAgent( diff --git a/src/modules/agent-to-agent/guard.ts b/src/modules/agent-to-agent/guard.ts new file mode 100644 index 00000000000..c778aa24f26 --- /dev/null +++ b/src/modules/agent-to-agent/guard.ts @@ -0,0 +1,88 @@ +/** + * Agent-to-agent guard adapter — the module's catalog entries, composed at + * the module edge (imported by ./index.ts). + * + * agents.create — the cli_scope branch moved verbatim out of + * create-agent.ts: `global` scope creates directly (create_agent is the + * intended primitive for trusted owner agent groups); anything else — the + * default `group` scope, and unknown/missing config, fail-closed — holds for + * the requesting group's admin chain. + * + * a2a.send — the decision moved verbatim out of routeAgentMessage, in its + * original check order: a missing destination row denies; a missing target + * group denies; self-sends allow without a destination row; an + * agent_message_policies row for the (from, to) pair holds for the row's + * named approver. The ghost-policy edge (policy row with no destination row) + * denies — the destination check precedes the policy check, exactly today's + * outcome. Policy rows can only tighten (hold), never allow: absence of a + * row falls through to the structural checks. + */ +import { getAgentGroup } from '../../db/agent-groups.js'; +import { getContainerConfig } from '../../db/container-configs.js'; +import { ALLOW, DENY, HOLD, defineGuardedAction } from '../../guard/index.js'; +import { hasDestination } from './db/agent-destinations.js'; +import { getMessagePolicy } from './db/agent-message-policies.js'; + +/** + * pending_approvals action string for held a2a messages. Lives here (not in + * agent-route.ts) so agent-route can import this adapter — loading the + * consult site guarantees its catalog entry is registered — without a cycle. + */ +export const A2A_MESSAGE_GATE_ACTION = 'a2a_message_gate'; + +export const agentsCreate = defineGuardedAction({ + action: 'agents.create', + grantActionName: 'create_agent', + // Bind a create_agent grant to the name that was approved. + grantCoversRequest: (grant, input) => { + try { + return (JSON.parse(grant.payload) as { name?: string }).name === input.payload.name; + } catch { + return false; + } + }, + decide: (input) => { + if (input.actor.kind !== 'agent') return DENY('create_agent is a container-originated action.'); + const cliScope = getContainerConfig(input.actor.agentGroupId)?.cli_scope ?? 'group'; + if (cliScope === 'global') { + // Trusted owner agent group — an approval tap on every sub-agent spawn + // would be needless friction. + return ALLOW('trusted global-scope agent group'); + } + // The realistic prompt-injection victim (default `group` scope) — and any + // unknown config value, fail-closed — requires an admin before any + // central-DB write. + return HOLD('agent-initiated create_agent requires admin approval'); + }, +}); + +export const a2aSend = defineGuardedAction({ + action: 'a2a.send', + grantActionName: A2A_MESSAGE_GATE_ACTION, + // Bind an a2a grant to the exact held message target. + grantCoversRequest: (grant, input) => { + try { + return (JSON.parse(grant.payload) as { platform_id?: string }).platform_id === input.resource?.to; + } catch { + return false; + } + }, + decide: (input) => { + if (input.actor.kind !== 'agent') return DENY('agent-to-agent send requires an agent actor'); + const from = input.actor.agentGroupId; + const to = input.resource?.to ?? ''; + const isSelf = to === from; + if (!isSelf && !hasDestination(from, 'agent', to)) { + return DENY(`unauthorized agent-to-agent: ${from} has no destination for ${to}`); + } + if (!getAgentGroup(to)) { + return DENY(`target agent group ${to} not found for message ${String(input.payload.id)}`); + } + if (isSelf) return ALLOW('self-send'); + const policy = getMessagePolicy(from, to); + if (policy) { + return HOLD(`a2a message policy ${from}→${to} holds for ${policy.approver}`, policy.approver); + } + return ALLOW('destination grant exists'); + }, +}); diff --git a/src/modules/agent-to-agent/index.ts b/src/modules/agent-to-agent/index.ts index 95dfd36c679..0a79f77dc7f 100644 --- a/src/modules/agent-to-agent/index.ts +++ b/src/modules/agent-to-agent/index.ts @@ -1,13 +1,14 @@ /** * Agent-to-agent module — inter-agent messaging and on-demand agent creation. * - * Registers one delivery action (`create_agent`) plus its matching approval - * handler — `create_agent` writes central-DB state, so confined (non-global) - * groups require admin approval (the delivery action queues the request; - * `applyCreateAgent` runs on approve); trusted global-scope groups create - * directly. The sibling `channel_type === 'agent'` routing path is NOT a system - * action — core `delivery.ts` dispatches into `./agent-route.js` via a dynamic - * import when it sees `msg.channel_type === 'agent'`. + * Registers its guard-catalog entries (./guard.js) and one guard-wrapped + * delivery action (`create_agent`) — `create_agent` writes central-DB state, + * so the guard's agents.create decision holds confined (non-global) groups + * for admin approval while trusted global-scope groups create directly; the + * approval handler re-enters the wrapped action carrying the approval row as + * its grant. The sibling `channel_type === 'agent'` routing path is NOT a + * system action — core `delivery.ts` dispatches into `./agent-route.js` via + * a dynamic import when it sees `msg.channel_type === 'agent'`. * * Host integration points: * - `src/container-runner.ts::spawnContainer` dynamically imports @@ -20,13 +21,19 @@ * system action logs "Unknown system action", `channel_type='agent'` messages * throw because the module isn't installed. */ -import { registerDeliveryAction } from '../../delivery.js'; -import { registerApprovalHandler } from '../approvals/index.js'; +import { reenterGuardedDeliveryAction, registerDeliveryAction } from '../../delivery.js'; +import { notifyAgent, registerApprovalHandler } from '../approvals/index.js'; import { A2A_MESSAGE_GATE_ACTION } from './agent-route.js'; -import { applyCreateAgent, handleCreateAgent } from './create-agent.js'; +import { createAgent, requestCreateAgentHold, validateCreateAgent } from './create-agent.js'; +import { agentsCreate } from './guard.js'; import { applyA2aMessageGate } from './message-gate.js'; -registerDeliveryAction('create_agent', handleCreateAgent); -registerApprovalHandler('create_agent', applyCreateAgent); +registerDeliveryAction('create_agent', createAgent, { + guardAction: agentsCreate, + precheck: validateCreateAgent, + requestHold: requestCreateAgentHold, + onDeny: (_content, session, reason) => notifyAgent(session, `create_agent denied: ${reason}`), +}); +registerApprovalHandler('create_agent', reenterGuardedDeliveryAction('create_agent')); registerApprovalHandler(A2A_MESSAGE_GATE_ACTION, applyA2aMessageGate); diff --git a/src/modules/agent-to-agent/message-gate.test.ts b/src/modules/agent-to-agent/message-gate.test.ts index fa1a485f686..98bd380c3f9 100644 --- a/src/modules/agent-to-agent/message-gate.test.ts +++ b/src/modules/agent-to-agent/message-gate.test.ts @@ -2,16 +2,17 @@ import Database from 'better-sqlite3'; import fs from 'fs'; import { describe, expect, it, beforeEach, afterEach, vi } from 'vitest'; +import './guard.js'; // register the a2a.send catalog entry (incl. the policy hold) import { routeAgentMessage } from './agent-route.js'; import { createDestination, deleteDestination, deleteAllDestinationsTouching } from './db/agent-destinations.js'; import { getMessagePolicy, removeMessagePolicy, setMessagePolicy } from './db/agent-message-policies.js'; import { applyA2aMessageGate } from './message-gate.js'; import { initTestDb, closeDb, runMigrations, createAgentGroup } from '../../db/index.js'; import { getDb } from '../../db/connection.js'; -import { createSession } from '../../db/sessions.js'; +import { createPendingApproval, createSession, deletePendingApproval, getPendingApproval } from '../../db/sessions.js'; import { requestApproval } from '../approvals/index.js'; import { initSessionFolder, inboundDbPath } from '../../session-manager.js'; -import type { Session } from '../../types.js'; +import type { PendingApproval, Session } from '../../types.js'; vi.mock('../../container-runner.js', () => ({ wakeContainer: vi.fn().mockResolvedValue(undefined), @@ -67,6 +68,23 @@ function makeSession(id: string, agentGroupId: string): Session { }; } +/** Seed a live a2a hold row (what requestApproval writes) and return it as the grant. */ +function seedA2aHold(approvalId: string, payload: Record): PendingApproval { + createPendingApproval({ + approval_id: approvalId, + session_id: 'sess-A', + request_id: approvalId, + action: 'a2a_message_gate', + payload: JSON.stringify(payload), + created_at: now(), + agent_group_id: A, + title: 'Message approval', + options_json: '[]', + approver_user_id: 'telegram:dana', + }); + return getPendingApproval(approvalId)!; +} + describe('agent message policies', () => { let SA: Session; let SB: Session; @@ -129,7 +147,7 @@ describe('agent message policies', () => { expect(requestApproval).not.toHaveBeenCalled(); }); - it('policy present → holds the message and requests approval from the policy approver scoped to the target', async () => { + it('policy present → holds the message and requests approval from the policy approver', async () => { setMessagePolicy(A, B, 'telegram:dana', now()); await routeAgentMessage( @@ -139,7 +157,7 @@ describe('agent message policies', () => { // Held: nothing routed to B. expect(readInbound(B, SB.id)).toHaveLength(0); - // One approval requested, to the policy's approver, scoped to the target group. + // One approval requested, to the policy's approver. expect(requestApproval).toHaveBeenCalledTimes(1); const opts = vi.mocked(requestApproval).mock.calls[0][0]; expect(opts.action).toBe('a2a_message_gate'); @@ -158,21 +176,76 @@ describe('agent message policies', () => { expect(readInbound(A, SA.id)).toHaveLength(1); }); - // ── approve handler re-routes the held message ── + it('ghost policy (policy row, no destination row) still denies — deny beats the policy hold', async () => { + deleteDestination(A, 'b'); // removes A→B — the destination ACL now denies + setMessagePolicy(A, B, 'telegram:dana', now()); // ...but a stale policy row remains + + await expect( + routeAgentMessage({ id: 'ghost', platform_id: B, content: JSON.stringify({ text: 'x' }), in_reply_to: null }, SA), + ).rejects.toThrow(/unauthorized agent-to-agent/); + expect(requestApproval).not.toHaveBeenCalled(); + expect(readInbound(B, SB.id)).toHaveLength(0); + }); + + // ── approve handler re-enters the guarded route with the grant ── + + it('applyA2aMessageGate delivers the held message to the target (valid grant)', async () => { + setMessagePolicy(A, B, 'telegram:dana', now()); + const payload = { id: 'held-1', platform_id: B, content: JSON.stringify({ text: 'approved!' }), in_reply_to: null }; + const approval = seedA2aHold('appr-a2a-1', payload); - it('applyA2aMessageGate delivers the held message to the target', async () => { const notify = vi.fn(); - await applyA2aMessageGate({ - session: SA, - userId: 'slack:dana', - notify, - payload: { id: 'held-1', platform_id: B, content: JSON.stringify({ text: 'approved!' }), in_reply_to: null }, - }); + await applyA2aMessageGate({ session: SA, userId: 'telegram:dana', notify, payload, approval }); const bRows = readInbound(B, SB.id); expect(bRows).toHaveLength(1); expect(JSON.parse(bRows[0].content).text).toBe('approved!'); expect(notify).not.toHaveBeenCalled(); + // The hold is satisfied by the grant — no second card. + expect(requestApproval).not.toHaveBeenCalled(); + }); + + it('destination revoked between hold and approve → refused cleanly, requester told, nothing delivered', async () => { + setMessagePolicy(A, B, 'telegram:dana', now()); + const payload = { id: 'held-2', platform_id: B, content: JSON.stringify({ text: 'stale' }), in_reply_to: null }; + const approval = seedA2aHold('appr-a2a-2', payload); + + deleteDestination(A, 'b'); // revoke A→B while the card is pending + + const notify = vi.fn(); + // An expected policy refusal — resolves (no throw), so the response + // handler never records it as a handler crash. + await applyA2aMessageGate({ session: SA, userId: 'telegram:dana', notify, payload, approval }); + + expect(readInbound(B, SB.id)).toHaveLength(0); + expect(notify).toHaveBeenCalledWith(expect.stringMatching(/not delivered.*no destination for/)); + }); + + it('mismatched grant (held for another target) refuses the replay cleanly', async () => { + setMessagePolicy(A, B, 'telegram:dana', now()); + // Grant was approved for a message to A (different target than the replay). + const approval = seedA2aHold('appr-a2a-3', { id: 'other', platform_id: A, content: '{}', in_reply_to: null }); + const payload = { id: 'held-3', platform_id: B, content: JSON.stringify({ text: 'swap' }), in_reply_to: null }; + + const notify = vi.fn(); + await applyA2aMessageGate({ session: SA, userId: 'telegram:dana', notify, payload, approval }); + + expect(readInbound(B, SB.id)).toHaveLength(0); + expect(notify).toHaveBeenCalledWith(expect.stringMatching(/not delivered.*invalid or mismatched grant/)); + }); + + it('a grant only works while its row is live (executes once)', async () => { + setMessagePolicy(A, B, 'telegram:dana', now()); + const payload = { id: 'held-4', platform_id: B, content: JSON.stringify({ text: 'once' }), in_reply_to: null }; + const approval = seedA2aHold('appr-a2a-4', payload); + + deletePendingApproval(approval.approval_id); // resolution already consumed the row + + const notify = vi.fn(); + await applyA2aMessageGate({ session: SA, userId: 'telegram:dana', notify, payload, approval }); + + expect(readInbound(B, SB.id)).toHaveLength(0); + expect(notify).toHaveBeenCalledWith(expect.stringMatching(/not delivered.*invalid or mismatched grant/)); }); // ── ghost-gate cleanup ── diff --git a/src/modules/agent-to-agent/message-gate.ts b/src/modules/agent-to-agent/message-gate.ts index 74c20c4cb22..93f7d799174 100644 --- a/src/modules/agent-to-agent/message-gate.ts +++ b/src/modules/agent-to-agent/message-gate.ts @@ -1,9 +1,10 @@ /** Approve handler for a held a2a message. (Reject is handled by the generic response-handler path.) */ +import { GuardDenyError } from '../../guard/index.js'; import { log } from '../../log.js'; import type { ApprovalHandler } from '../approvals/index.js'; -import { performAgentRoute, type RoutableAgentMessage } from './agent-route.js'; +import { routeAgentMessage, type RoutableAgentMessage } from './agent-route.js'; -export const applyA2aMessageGate: ApprovalHandler = async ({ session, payload, notify }) => { +export const applyA2aMessageGate: ApprovalHandler = async ({ session, payload, approval, notify }) => { const { id, platform_id, content, in_reply_to } = payload; if (typeof platform_id !== 'string' || !platform_id) { notify('Message approved but the target agent group was missing from the request.'); @@ -18,7 +19,27 @@ export const applyA2aMessageGate: ApprovalHandler = async ({ session, payload, n in_reply_to: typeof in_reply_to === 'string' ? in_reply_to : null, }; - await performAgentRoute(msg, session, platform_id); + // One replay semantics: re-enter the guarded route carrying the approval + // row as the grant. The policy hold is satisfied, but the structural + // checks run live — a deny here (destination revoked while the card was + // pending, dead or mismatched grant) is an EXPECTED policy outcome, not a + // crash: tell the requester, log a warning, and let anything else keep the + // response handler's failure path. + try { + await routeAgentMessage(msg, session, { grant: approval }); + } catch (err) { + if (err instanceof GuardDenyError) { + log.warn('Approved a2a replay refused by the guard', { + from: session.agent_group_id, + to: platform_id, + msgId: msg.id, + reason: err.message, + }); + notify(`Message approved, but not delivered — no longer authorized: ${err.message}`); + return; + } + throw err; + } log.info('Held agent message delivered after approval', { from: session.agent_group_id, to: platform_id, diff --git a/src/modules/approvals/primitive.ts b/src/modules/approvals/primitive.ts index 2d3028a64dc..48047e676dc 100644 --- a/src/modules/approvals/primitive.ts +++ b/src/modules/approvals/primitive.ts @@ -59,6 +59,12 @@ const APPROVAL_OPTIONS: RawOption[] = [ export interface ApprovalHandlerContext { session: Session; payload: Record; + /** + * The verified approval row — the grant an approved continuation carries + * when it re-enters its guarded entry point. Still live here; resolution + * deletes it after the handler returns, so a grant executes exactly once. + */ + approval: PendingApproval; /** User ID of the admin who approved. Empty string if unknown. */ userId: string; /** Send a system chat message to the requesting agent's session. */ diff --git a/src/modules/approvals/response-handler.ts b/src/modules/approvals/response-handler.ts index 52fc705efea..79014597ece 100644 --- a/src/modules/approvals/response-handler.ts +++ b/src/modules/approvals/response-handler.ts @@ -112,7 +112,7 @@ async function handleRegisteredApproval( const payload = JSON.parse(approval.payload); try { - await handler({ session, payload, userId, notify }); + await handler({ session, payload, approval, userId, notify }); log.info('Approval handled', { approvalId: approval.approval_id, action: approval.action, userId }); } catch (err) { log.error('Approval handler threw', { approvalId: approval.approval_id, action: approval.action, err }); diff --git a/src/modules/permissions/channel-approval.test.ts b/src/modules/permissions/channel-approval.test.ts index 103f06d5687..db084787eb3 100644 --- a/src/modules/permissions/channel-approval.test.ts +++ b/src/modules/permissions/channel-approval.test.ts @@ -68,7 +68,11 @@ vi.mock('./user-dm.js', () => ({ vi.mock('../../config.js', async () => { const actual = await vi.importActual('../../config.js'); - return { ...actual, DATA_DIR: '/tmp/nanoclaw-test-channel-approval' }; + return { + ...actual, + DATA_DIR: '/tmp/nanoclaw-test-channel-approval', + GROUPS_DIR: '/tmp/nanoclaw-test-channel-approval/groups', + }; }); const TEST_DIR = '/tmp/nanoclaw-test-channel-approval'; @@ -586,6 +590,108 @@ describe('unknown-channel registration flow', () => { .c; expect(stillPending).toBe(1); }); + + it('create new agent: the free-text name reply creates the group and wires the channel', async () => { + const { routeInbound } = await import('../../router.js'); + const { getResponseHandlers } = await import('../../response-registry.js'); + const { getDb } = await import('../../db/connection.js'); + + await routeInbound(groupMention('chat-create-new')); + await new Promise((r) => setTimeout(r, 10)); + const pending = getDb().prepare('SELECT messaging_group_id FROM pending_channel_approvals').get() as { + messaging_group_id: string; + }; + + // Owner clicks "Connect new agent" → name prompt lands in their DM. + for (const handler of getResponseHandlers()) { + const claimed = await handler({ + questionId: pending.messaging_group_id, + value: 'new_agent', + userId: 'owner', + channelType: 'telegram', + platformId: 'dm-owner', + threadId: null, + }); + if (claimed) break; + } + + // Owner replies with the agent name in the same DM — the interceptor + // captures it and creates. + await routeInbound({ + channelType: 'telegram', + platformId: 'dm-owner', + threadId: null, + message: { + id: 'name-reply-1', + kind: 'chat' as const, + content: JSON.stringify({ senderId: 'owner', senderName: 'Owner', text: 'Newbie' }), + timestamp: now(), + }, + }); + + const created = getDb().prepare("SELECT id FROM agent_groups WHERE name = 'Newbie'").get() as + | { id: string } + | undefined; + expect(created).toBeDefined(); + const mgaCount = ( + getDb() + .prepare('SELECT COUNT(*) AS c FROM messaging_group_agents WHERE messaging_group_id = ? AND agent_group_id = ?') + .get(pending.messaging_group_id, created!.id) as { c: number } + ).c; + expect(mgaCount).toBe(1); + const stillPending = (getDb().prepare('SELECT COUNT(*) AS c FROM pending_channel_approvals').get() as { c: number }) + .c; + expect(stillPending).toBe(0); + }); + + it('a name reply after the registration vanished is consumed without creating anything', async () => { + const { routeInbound } = await import('../../router.js'); + const { getResponseHandlers } = await import('../../response-registry.js'); + const { getDb } = await import('../../db/connection.js'); + + await routeInbound(groupMention('chat-vanished')); + await new Promise((r) => setTimeout(r, 10)); + const pending = getDb().prepare('SELECT messaging_group_id FROM pending_channel_approvals').get() as { + messaging_group_id: string; + }; + + for (const handler of getResponseHandlers()) { + const claimed = await handler({ + questionId: pending.messaging_group_id, + value: 'new_agent', + userId: 'owner', + channelType: 'telegram', + platformId: 'dm-owner', + threadId: null, + }); + if (claimed) break; + } + + // The registration disappears between the click and the reply (rejected + // from another card, group delete cascade, …) — the interceptor no + // longer finds a pending registration, so the reply must not create. + getDb() + .prepare('DELETE FROM pending_channel_approvals WHERE messaging_group_id = ?') + .run(pending.messaging_group_id); + + const agentGroupsBefore = (getDb().prepare('SELECT COUNT(*) AS c FROM agent_groups').get() as { c: number }).c; + await routeInbound({ + channelType: 'telegram', + platformId: 'dm-owner', + threadId: null, + message: { + id: 'name-reply-2', + kind: 'chat' as const, + content: JSON.stringify({ senderId: 'owner', senderName: 'Owner', text: 'Ghost' }), + timestamp: now(), + }, + }); + + const agentGroupsAfter = (getDb().prepare('SELECT COUNT(*) AS c FROM agent_groups').get() as { c: number }).c; + expect(agentGroupsAfter).toBe(agentGroupsBefore); + const mgaCount = (getDb().prepare('SELECT COUNT(*) AS c FROM messaging_group_agents').get() as { c: number }).c; + expect(mgaCount).toBe(0); + }); }); describe('no-owner / no-agent failure modes', () => { diff --git a/src/modules/permissions/guard.ts b/src/modules/permissions/guard.ts new file mode 100644 index 00000000000..810a9cf3f45 --- /dev/null +++ b/src/modules/permissions/guard.ts @@ -0,0 +1,55 @@ +/** + * Permissions guard adapter — the module's catalog entries, composed at the + * module edge (imported by ./index.ts). + * + * senders.admit — the `unknown_sender_policy` switch moved verbatim out of + * handleUnknownSender: `public` allows (short-circuited before the gate + * anyway), `request_approval` holds, `strict` denies. The hold is executed by + * the caller through the module's own pending_sender_approvals flow (card, + * in-flight dedup) — not the approvals primitive — so this entry has no + * grantActionName: the approve continuation adds the member and replays + * routeInbound, which then passes the gate structurally via membership, no + * grant needed. + * + * channels.register — click authorization for the channel-registration flow, + * verbatim from today's response handler: the delivered approver, or an + * admin of the pending row's anchor agent group. Consulted inline by the + * card-click response handler only. The free-text name reply is deliberately + * NOT re-authorized (main's behavior): the click arms the capture and stands + * as the auth — a privilege revoked between the click and the reply still + * completes the flow. + */ +import { ALLOW, DENY, HOLD, defineGuardedAction } from '../../guard/index.js'; +import { getPendingChannelApproval } from './db/pending-channel-approvals.js'; +import { hasAdminPrivilege } from './db/user-roles.js'; + +export const sendersAdmit = defineGuardedAction({ + action: 'senders.admit', + decide: (input) => { + const policy = input.payload.policy; + if (policy === 'public') return ALLOW('public messaging group'); + if (policy === 'request_approval') { + return HOLD( + `unknown sender requires admin approval on messaging group ${String(input.payload.messagingGroupId)}`, + ); + } + return DENY('unknown sender on a strict messaging group'); + }, +}); + +export const channelsRegister = defineGuardedAction({ + action: 'channels.register', + decide: (input) => { + if (input.actor.kind !== 'human') return DENY('channel registration resolves via human clicks/replies'); + const questionId = typeof input.payload.questionId === 'string' ? input.payload.questionId : ''; + const row = getPendingChannelApproval(questionId); + if (!row) return DENY(`no pending channel registration for ${questionId || '(missing questionId)'}`); + if ( + input.actor.userId && + (input.actor.userId === row.approver_user_id || hasAdminPrivilege(input.actor.userId, row.agent_group_id)) + ) { + return ALLOW('delivered approver or anchor-group admin'); + } + return DENY('not an eligible channel-registration approver'); + }, +}); diff --git a/src/modules/permissions/index.ts b/src/modules/permissions/index.ts index 3cd5159b97d..f8d3dbf5b1c 100644 --- a/src/modules/permissions/index.ts +++ b/src/modules/permissions/index.ts @@ -33,6 +33,8 @@ import { registerResponseHandler, type ResponsePayload } from '../../response-re import { getDeliveryAdapter } from '../../delivery.js'; import { log } from '../../log.js'; import type { MessagingGroup, MessagingGroupAgent } from '../../types.js'; +import { guard } from '../../guard/index.js'; +import { channelsRegister, sendersAdmit } from './guard.js'; import { canAccessAgentGroup } from './access.js'; import { buildAgentSelectionOptions, @@ -131,43 +133,49 @@ function handleUnknownSender( agent_group_id: agentGroupId, }; - if (mg.unknown_sender_policy === 'strict') { - log.info('MESSAGE DROPPED — unknown sender (strict policy)', { + // The admission decision is the guard's senders.admit decision (./guard.ts) + // — unknown_sender_policy verbatim: strict → deny, request_approval → hold, + // public → allow (short-circuited before the gate). Drop-recording and the + // hold creation stay here. + const decision = guard(sendersAdmit, { + actor: userId ? { kind: 'human', userId } : { kind: 'system' }, + payload: { messagingGroupId: mg.id, agentGroupId, - userId, - accessReason, - }); - recordDroppedMessage(dropRecord); - return; - } + senderIdentity: userId, + policy: mg.unknown_sender_policy, + }, + }); + + if (decision.effect === 'allow') return; // 'public' — handled before the gate; fall through silently. - if (mg.unknown_sender_policy === 'request_approval') { - log.info('MESSAGE DROPPED — unknown sender (approval requested)', { + log.info( + decision.effect === 'hold' + ? 'MESSAGE DROPPED — unknown sender (approval requested)' + : 'MESSAGE DROPPED — unknown sender (strict policy)', + { messagingGroupId: mg.id, agentGroupId, userId, accessReason, - }); - recordDroppedMessage(dropRecord); - // Fire-and-forget; pick-approver + delivery + row-insert are all async. - // If it fails it logs internally — the user's message still stays dropped - // either way. Requires a resolved userId (senderResolver populates users - // row before the gate fires); if we got here without one, there's nothing - // to identify for approval and we just stay in the "silent strict" branch. - if (userId) { - requestSenderApproval({ - messagingGroupId: mg.id, - agentGroupId, - senderIdentity: userId, - senderName, - event, - }).catch((err) => log.error('Sender-approval flow threw', { err })); - } - return; + }, + ); + recordDroppedMessage(dropRecord); + + // Fire-and-forget; pick-approver + delivery + row-insert are all async. + // If it fails it logs internally — the user's message still stays dropped + // either way. Requires a resolved userId (senderResolver populates users + // row before the gate fires); if we got here without one, there's nothing + // to identify for approval and we just drop silently. + if (decision.effect === 'hold' && userId) { + requestSenderApproval({ + messagingGroupId: mg.id, + agentGroupId, + senderIdentity: userId, + senderName, + event, + }).catch((err) => log.error('Sender-approval flow threw', { err })); } - - // 'public' should have been handled before the gate; fall through silently. } setSenderResolver(extractAndUpsertUser); @@ -411,18 +419,23 @@ async function handleChannelApprovalResponse(payload: ResponsePayload): Promise< const row = getPendingChannelApproval(payload.questionId); if (!row) return false; + // Click authorization is the guard's channels.register decision (./guard.ts): + // the delivered approver, or an admin of the pending row's anchor agent group. const clickerId = payload.userId ? payload.userId.includes(':') ? payload.userId : `${payload.channelType}:${payload.userId}` : null; - const isAuthorized = - clickerId !== null && (clickerId === row.approver_user_id || hasAdminPrivilege(clickerId, row.agent_group_id)); - if (!isAuthorized) { + const decision = guard(channelsRegister, { + actor: { kind: 'human', userId: clickerId ?? '' }, + payload: { questionId: payload.questionId }, + }); + if (!clickerId || decision.effect !== 'allow') { log.warn('Channel registration click rejected — unauthorized clicker', { messagingGroupId: row.messaging_group_id, clickerId, expectedApprover: row.approver_user_id, + reason: decision.reason, }); return true; } diff --git a/src/modules/self-mod/apply.ts b/src/modules/self-mod/apply.ts index c5318ffbfb6..6f5fa7a813c 100644 --- a/src/modules/self-mod/apply.ts +++ b/src/modules/self-mod/apply.ts @@ -1,11 +1,12 @@ /** - * Approval handlers for self-modification actions. + * Guarded handler bodies for self-modification actions. * - * The approvals module calls these when an admin clicks Approve on a - * pending_approvals row whose action matches. Each handler mutates the - * container config in the DB, rebuilds/kills the container as needed, - * and writes an on_wake message so the fresh container picks up where - * the old one left off. + * The delivery registry's guard wrapper runs these only on `allow` — which, + * for self-mod, means an approved replay carrying a valid grant (the + * decision holds unconditionally from the container path; see ./guard.ts). + * Each body mutates the container config in the DB, rebuilds/kills the + * container as needed, and writes an on_wake message so the fresh container + * picks up where the old one left off. * * install_packages: update DB + rebuild image + kill container + on_wake. * add_mcp_server: update DB + kill container + on_wake. @@ -17,18 +18,19 @@ import { getSession } from '../../db/sessions.js'; import type { McpServerConfig } from '../../container-config.js'; import { log } from '../../log.js'; import { writeSessionMessage } from '../../session-manager.js'; -import type { ApprovalHandler } from '../approvals/index.js'; +import type { Session } from '../../types.js'; +import { notifyAgent } from '../approvals/index.js'; -export const applyInstallPackages: ApprovalHandler = async ({ session, payload, userId, notify }) => { +export async function applyInstallPackages(payload: Record, session: Session): Promise { const agentGroup = getAgentGroup(session.agent_group_id); if (!agentGroup) { - notify('install_packages approved but agent group missing.'); + notifyAgent(session, 'install_packages approved but agent group missing.'); return; } const configRow = getContainerConfig(agentGroup.id); if (!configRow) { - notify('install_packages approved but container config missing.'); + notifyAgent(session, 'install_packages approved but container config missing.'); return; } @@ -52,7 +54,7 @@ export const applyInstallPackages: ApprovalHandler = async ({ session, payload, ...((payload.apt as string[] | undefined) || []), ...((payload.npm as string[] | undefined) || []), ].join(', '); - log.info('Package install approved', { agentGroupId: session.agent_group_id, userId }); + log.info('Package install approved', { agentGroupId: session.agent_group_id }); try { await buildAgentGroupImage(session.agent_group_id); writeSessionMessage(session.agent_group_id, session.id, { @@ -75,23 +77,24 @@ export const applyInstallPackages: ApprovalHandler = async ({ session, payload, }); log.info('Container rebuild completed (bundled with install)', { agentGroupId: session.agent_group_id }); } catch (e) { - notify( + notifyAgent( + session, `Packages added to config (${pkgs}) but rebuild failed: ${e instanceof Error ? e.message : String(e)}. Tell the user — an admin will need to retry the install_packages request or inspect the build logs.`, ); log.error('Bundled rebuild failed after install approval', { agentGroupId: session.agent_group_id, err: e }); } -}; +} -export const applyAddMcpServer: ApprovalHandler = async ({ session, payload, userId, notify }) => { +export async function applyAddMcpServer(payload: Record, session: Session): Promise { const agentGroup = getAgentGroup(session.agent_group_id); if (!agentGroup) { - notify('add_mcp_server approved but agent group missing.'); + notifyAgent(session, 'add_mcp_server approved but agent group missing.'); return; } const configRow = getContainerConfig(agentGroup.id); if (!configRow) { - notify('add_mcp_server approved but container config missing.'); + notifyAgent(session, 'add_mcp_server approved but container config missing.'); return; } @@ -122,5 +125,5 @@ export const applyAddMcpServer: ApprovalHandler = async ({ session, payload, use const s = getSession(session.id); if (s) wakeContainer(s); }); - log.info('MCP server add approved', { agentGroupId: session.agent_group_id, userId }); -}; + log.info('MCP server add approved', { agentGroupId: session.agent_group_id }); +} diff --git a/src/modules/self-mod/guard.ts b/src/modules/self-mod/guard.ts new file mode 100644 index 00000000000..bbdb3a11e07 --- /dev/null +++ b/src/modules/self-mod/guard.ts @@ -0,0 +1,32 @@ +/** + * Self-mod guard adapter — the module's catalog entries, composed at the + * module edge (imported by ./index.ts). + * + * The decision is today's behavior verbatim: from the container + * path, self-modification is held unconditionally for the agent group's + * admin chain. (The equivalent host-side mutations — `ncl groups config + * add-package` etc. — are separate catalog actions derived from the command + * registry.) + */ +import { DENY, HOLD, defineGuardedAction, type GuardInput } from '../../guard/index.js'; + +function selfModDecide(label: string) { + return (input: GuardInput) => { + if (input.actor.kind !== 'agent') { + return DENY(`${label} is a container-originated action.`); + } + return HOLD(`${label} always requires admin approval from the container path`); + }; +} + +export const selfModInstallPackages = defineGuardedAction({ + action: 'self_mod.install_packages', + grantActionName: 'install_packages', + decide: selfModDecide('install_packages'), +}); + +export const selfModAddMcpServer = defineGuardedAction({ + action: 'self_mod.add_mcp_server', + grantActionName: 'add_mcp_server', + decide: selfModDecide('add_mcp_server'), +}); diff --git a/src/modules/self-mod/index.ts b/src/modules/self-mod/index.ts index e1f49e212b3..bfdc08797bc 100644 --- a/src/modules/self-mod/index.ts +++ b/src/modules/self-mod/index.ts @@ -2,29 +2,51 @@ * Self-modification module — admin-approved container mutations. * * Optional tier. Depends on the approvals default module for the request/ - * handler plumbing. On install the module registers: - * - Two delivery actions (install_packages, add_mcp_server) that validate - * input and queue an approval via requestApproval(). - * - Two matching approval handlers that run on approve and perform the - * complete follow-up: - * install_packages → update container.json, rebuild image, kill + * handler plumbing and on the guard for the decision. On install the module + * registers: + * - Its guard-catalog entries (./guard.ts): unconditional hold from the + * container path. + * - Two guard-wrapped delivery actions (install_packages, add_mcp_server): + * validation runs as the wrapper's precheck, the hold builders card the + * admin, and the handler bodies (./apply.ts) run only on allow — i.e. on + * an approved replay: + * install_packages → update container_configs, rebuild image, kill * container (next wake respawns on the new image), schedule a * verify-and-report follow-up prompt. - * add_mcp_server → update container.json, kill container. No image + * add_mcp_server → update container_configs, kill container. No image * rebuild — bun runs TS directly, so the new MCP server is wired * by the next container start. + * - Two approval handlers that re-enter the wrapped actions with the + * approval row as the grant (one replay semantics — the guard re-checks + * the structural checks live). * * Without this module: the MCP tools in the container still write outbound * system messages with these actions, but delivery logs "Unknown system * action" and drops them. Admin never sees a card; nothing changes. */ -import { registerDeliveryAction } from '../../delivery.js'; -import { registerApprovalHandler } from '../approvals/index.js'; +import { reenterGuardedDeliveryAction, registerDeliveryAction } from '../../delivery.js'; +import { notifyAgent, registerApprovalHandler } from '../approvals/index.js'; import { applyAddMcpServer, applyInstallPackages } from './apply.js'; -import { handleAddMcpServer, handleInstallPackages } from './request.js'; +import { selfModAddMcpServer, selfModInstallPackages } from './guard.js'; +import { + requestAddMcpServerHold, + requestInstallPackagesHold, + validateAddMcpServer, + validateInstallPackages, +} from './request.js'; -registerDeliveryAction('install_packages', handleInstallPackages); -registerDeliveryAction('add_mcp_server', handleAddMcpServer); +registerDeliveryAction('install_packages', applyInstallPackages, { + guardAction: selfModInstallPackages, + precheck: validateInstallPackages, + requestHold: requestInstallPackagesHold, + onDeny: (_content, session, reason) => notifyAgent(session, `install_packages denied: ${reason}`), +}); +registerDeliveryAction('add_mcp_server', applyAddMcpServer, { + guardAction: selfModAddMcpServer, + precheck: validateAddMcpServer, + requestHold: requestAddMcpServerHold, + onDeny: (_content, session, reason) => notifyAgent(session, `add_mcp_server denied: ${reason}`), +}); -registerApprovalHandler('install_packages', applyInstallPackages); -registerApprovalHandler('add_mcp_server', applyAddMcpServer); +registerApprovalHandler('install_packages', reenterGuardedDeliveryAction('install_packages')); +registerApprovalHandler('add_mcp_server', reenterGuardedDeliveryAction('add_mcp_server')); diff --git a/src/modules/self-mod/request.ts b/src/modules/self-mod/request.ts index 6cd7f05c9c3..21e558c18ff 100644 --- a/src/modules/self-mod/request.ts +++ b/src/modules/self-mod/request.ts @@ -1,12 +1,12 @@ /** - * Delivery-action handlers for agent-initiated self-modification requests. + * Validation + hold-request builders for agent-initiated self-modification. * * Two actions the container can write into messages_out (via the self-mod - * MCP tools): install_packages, add_mcp_server. Each one validates input - * and queues an approval request. The admin's approval triggers the - * matching approval handler in ./apply.ts, which also performs the - * required follow-up (rebuild+restart for install_packages, restart-only - * for add_mcp_server). + * MCP tools): install_packages, add_mcp_server. The delivery registry wraps + * each one with the guard (see ./guard.ts — unconditional hold from the + * container path): validation here runs as the wrapper's precheck, and the + * hold builders create the approval card when the guard holds. On approve, + * the continuation re-enters the wrapped action and ./apply.ts runs. * * Host-side sanitization for install_packages is defense-in-depth — the MCP * tool validates first. Both layers matter: the DB row carries the payload @@ -17,40 +17,48 @@ import { log } from '../../log.js'; import type { Session } from '../../types.js'; import { notifyAgent, requestApproval } from '../approvals/index.js'; -export async function handleInstallPackages(content: Record, session: Session): Promise { +export function validateInstallPackages(content: Record, session: Session): boolean { const agentGroup = getAgentGroup(session.agent_group_id); if (!agentGroup) { notifyAgent(session, 'install_packages failed: agent group not found.'); - return; + return false; } const apt = (content.apt as string[]) || []; const npm = (content.npm as string[]) || []; - const reason = (content.reason as string) || ''; const APT_RE = /^[a-z0-9][a-z0-9._+-]*$/; const NPM_RE = /^(@[a-z0-9][a-z0-9._-]*\/)?[a-z0-9][a-z0-9._-]*$/; const MAX_PACKAGES = 20; if (apt.length + npm.length === 0) { notifyAgent(session, 'install_packages failed: at least one apt or npm package is required.'); - return; + return false; } if (apt.length + npm.length > MAX_PACKAGES) { notifyAgent(session, `install_packages failed: max ${MAX_PACKAGES} packages per request.`); - return; + return false; } const invalidApt = apt.find((p) => !APT_RE.test(p)); if (invalidApt) { notifyAgent(session, `install_packages failed: invalid apt package name "${invalidApt}".`); log.warn('install_packages: invalid apt package rejected', { pkg: invalidApt }); - return; + return false; } const invalidNpm = npm.find((p) => !NPM_RE.test(p)); if (invalidNpm) { notifyAgent(session, `install_packages failed: invalid npm package name "${invalidNpm}".`); log.warn('install_packages: invalid npm package rejected', { pkg: invalidNpm }); - return; + return false; } + return true; +} + +export async function requestInstallPackagesHold(content: Record, session: Session): Promise { + const agentGroup = getAgentGroup(session.agent_group_id); + if (!agentGroup) return; + const apt = (content.apt as string[]) || []; + const npm = (content.npm as string[]) || []; + const reason = (content.reason as string) || ''; const packageList = [...apt.map((p) => `apt: ${p}`), ...npm.map((p) => `npm: ${p}`)].join(', '); await requestApproval({ @@ -63,18 +71,26 @@ export async function handleInstallPackages(content: Record, se }); } -export async function handleAddMcpServer(content: Record, session: Session): Promise { +export function validateAddMcpServer(content: Record, session: Session): boolean { const agentGroup = getAgentGroup(session.agent_group_id); if (!agentGroup) { notifyAgent(session, 'add_mcp_server failed: agent group not found.'); - return; + return false; } const serverName = content.name as string; const command = content.command as string; if (!serverName || !command) { notifyAgent(session, 'add_mcp_server failed: name and command are required.'); - return; + return false; } + return true; +} + +export async function requestAddMcpServerHold(content: Record, session: Session): Promise { + const agentGroup = getAgentGroup(session.agent_group_id); + if (!agentGroup) return; + const serverName = content.name as string; + const command = content.command as string; await requestApproval({ session, agentName: agentGroup.name,