diff --git a/CHANGELOG.md b/CHANGELOG.md index 609dc7de0..17598cbe3 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 (guarded-actions phase 2).** Every privileged action crossing the container or channel boundary now passes ONE decision function — `guard()` in the new `src/guard/` leaf — before it executes: `allow`, `hold` (through the flow's existing approval mechanics), or `deny`. The four handler registries (ncl commands, delivery actions, response handlers, message interceptors) wrap their handlers at registration, so the guarded path is the only path by construction, and a registry-walking conformance test fails CI on any unmapped mutating entry. The structural baseline is today's checks verbatim (cli_scope enforcement, create_agent's scope branch, a2a destination ACL + `agent_message_policies` hold, self-mod unconditional hold, unknown_sender_policy, channel-registration click auth, host-as-trusted-caller in code). Policy-as-data (tighten-only rule sources composing with the baseline) is deferred to phase 3, where the generalized rules table arrives with its first operator-visible consumer. Approved replays now carry the verified approval row as a **grant** — the `approved: true` boolean is deleted — and the guard re-runs the structural baseline on every replay. Three outcome changes are inherent to that replay semantics and ship deliberately: **(a)** approving a held a2a message after its destination was revoked no longer delivers it (the live baseline re-check); **(b)** a forged, already-consumed, or wrong-action/wrong-payload grant refuses the replay instead of executing (previously any in-process caller passing `approved: true` executed); **(c)** the channel-registration free-text name reply re-checks approver eligibility at reply time — a privilege revoked between the click and the reply, or a registration that vanished meanwhile, now refuses instead of acting. Zero rules ship by default — the seeded posture is today's behavior. (The D1/D2/D4 click-auth fixes from the guarded-actions defect inventory are NOT in this change — they belong to the approval-contract PR.) +- **Guard conformance is now a boot invariant, not just a CI test.** The host refuses to start (upgrade-tripwire posture: clear banner + exit 1) if any privileged command or delivery action is registered without a guard-catalog mapping. CI can't see skill-installed code — `/add-*` skills register handlers on machines where the test suite never runs — so the same registry walk the conformance test uses (`src/guard-conformance.ts`, one shared exemption list) now runs in `main()` before the host accepts a message, and a bad registration crashes at skill-install time instead of running unguarded. Companion hardening: re-registering a guard-wrapped delivery action *without* a guard spec now throws at registration — previously that silently replaced the guarded handler while the catalog still reported the action as guarded. - **Optional per-container resource caps.** `CONTAINER_CPU_LIMIT` and `CONTAINER_MEMORY_LIMIT` pass through to `docker run` as `--cpus` / `--memory` (`container-runner.ts`). Both empty by default — no flag added, spawn args byte-identical to today — so existing installs are unaffected. Set them to cap an agent container's CPU/memory so one agent can't monopolize the host (e.g. `CONTAINER_CPU_LIMIT=2`, `CONTAINER_MEMORY_LIMIT=8g`). Swap is intentionally not managed here: `--memory` is a hard cap on a swapless host. - [BREAKING] **Chat SDK pinned to `4.29.0` (was `4.26.0` via `^4.24.0`).** `chat` and the `@chat-adapter/*` channel adapters are version-locked — the adapter's `ChatInstance` must match the bridge's, so a mismatched pair fails to typecheck at `createChatSdkBridge(...)`. `chat` is therefore pinned exactly, and the channel-adapter install pins move with it — the `/add-` SKILL.md steps and `setup/*.sh` scripts on `main`, plus the adapter code on the `channels` branch. Core installs with no channel (only `cli`) are unaffected. **Migration:** if any channel is installed (Slack, Discord, Telegram, Teams, …), re-run its `/add-` skill to pull the matching `4.29.0` adapter. - **Budget/billing-exhausted LLM turns now reach the user instead of being silently dropped.** When a turn ends in a non-retryable provider error (e.g. an Anthropic `403 billing_error`) with no `` wrapping, the agent-runner delivers the provider's notice to the originating channel and stops re-nudging the failing gateway. `providers/claude.ts` now surfaces the SDK's `is_error` flag (and the error subtype's `errors[]` text); `poll-loop.ts` delivers that text and skips the re-wrap retry. Fixes the case where a spend-limit notice produced silence plus a turn-after-turn retry loop. diff --git a/CLAUDE.md b/CLAUDE.md index 85188020b..61ba49f7a 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -66,6 +66,7 @@ For ad-hoc queries from skills or scripts, use the in-tree wrapper rather than t | `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()` → allow \| hold \| deny. Domain-free leaf (decision function + registration-derived action catalog); domain baselines register from module-edge `guard.ts` adapters (cli, agent-to-agent, self-mod, permissions). All four handler registries wrap at registration; approved replays carry the approval row as a grant and re-check the structural baseline. Policy-as-data (tighten-only rule sources) is deferred to guarded-actions phase 3. 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 45d64b879..75dd55cc1 100644 --- a/src/cli/crud.ts +++ b/src/cli/crud.ts @@ -327,6 +327,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, @@ -339,6 +340,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, @@ -351,6 +353,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, @@ -362,6 +365,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, @@ -373,6 +377,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, @@ -389,6 +394,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/dispatch.test.ts b/src/cli/dispatch.test.ts index aacb812aa..c368dfa2b 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. @@ -472,10 +477,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(), }); @@ -484,6 +499,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 5ea9f65c3..99ca7b18d 100644 --- a/src/cli/dispatch.ts +++ b/src/cli/dispatch.ts @@ -3,23 +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 baseline 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 { getResource } from './crud.js'; +import { commandGuardAction } from './guard.js'; import { listVerbs, renderVerbHelp } from './help-render.js'; -import { GROUP_SCOPE_RESOURCES, listCommands, lookup } from './registry.js'; +import { 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, @@ -54,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 baseline, 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 = { @@ -116,9 +101,20 @@ export async function dispatch( } } + const decision = guard({ + action: commandGuardAction(cmd), + 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 @@ -127,7 +123,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.'); @@ -215,10 +216,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 data = typeof response.data === 'string' ? response.data : JSON.stringify(response.data, null, 2); diff --git a/src/cli/guard.ts b/src/cli/guard.ts new file mode 100644 index 000000000..2b7ebca39 --- /dev/null +++ b/src/cli/guard.ts @@ -0,0 +1,86 @@ +/** + * CLI guard adapter — the command registry's catalog derivation and + * structural baseline, moved verbatim out of dispatch.ts (guarded-actions + * phase 2). 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 baseline 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), + approvalAction: cmd.access === 'approval' ? 'cli_command' : undefined, + // Bind a cli_command grant to the exact command it was approved for. + grantMatches: (grant) => { + try { + const payload = JSON.parse(grant.payload) as { frame?: { command?: string } }; + return payload.frame?.command === cmd.name; + } catch { + return false; + } + }, + baseline: (input) => commandBaseline(cmd, input), + }; +} + +function commandBaseline(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 fff47ae4a..f80e4e02f 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 { registerGuardedAction } 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 @@ -58,6 +67,10 @@ export function register(def: CommandDef): void { 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 — dispatch consults the guard, and the + // conformance test walks the registry against the catalog. + registerGuardedAction(commandGuardSpec(def as CommandDef)); } export function lookup(name: string): CommandDef | undefined { diff --git a/src/delivery-actions.test.ts b/src/delivery-actions.test.ts index 6a89506cd..f9054978b 100644 --- a/src/delivery-actions.test.ts +++ b/src/delivery-actions.test.ts @@ -35,4 +35,24 @@ describe('delivery action registry', () => { registerDeliveryAction('test_overwrite_action', second); expect(getDeliveryAction('test_overwrite_action')).toBe(second); }); + + it('refuses to replace a guard-wrapped action with an unguarded handler', () => { + registerDeliveryAction('test_guarded_overwrite', async () => {}, { + guardAction: 'test.guarded-overwrite', + requestHold: async () => {}, + }); + + // Disarming the guard by re-registering without a spec must throw — + // otherwise the catalog (and the conformance walk) would still report + // the action guarded while the live path runs unguarded. + expect(() => registerDeliveryAction('test_guarded_overwrite', async () => {})).toThrow(/disarm the guard/); + + // Re-registering WITH a spec stays allowed (a legitimate replacement + // keeps the action guarded). + registerDeliveryAction('test_guarded_overwrite', async () => {}, { + guardAction: 'test.guarded-overwrite', + requestHold: async () => {}, + }); + expect(getDeliveryAction('test_guarded_overwrite')).toBeDefined(); + }); }); diff --git a/src/delivery.ts b/src/delivery.ts index 1046e7558..949450b85 100644 --- a/src/delivery.ts +++ b/src/delivery.ts @@ -20,12 +20,13 @@ import { markDeliveryFailed, migrateDeliveredTable, } from './db/session-db.js'; +import { guard } 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 +394,18 @@ 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: the registry wraps the handler + * so the guard's decision — allow / hold / deny — stands between the + * container's outbound row and the handler body, and the wrapped path is the + * only path (the raw handler is never stored). On approve, the continuation + * re-enters the same wrapped entry carrying the approval row as its grant + * (`reenterGuardedDeliveryAction`), so the structural baseline is re-checked + * live. Plain actions (scheduling self-actions, the cli_request bridge — its + * inner commands are guarded at dispatch) register without a spec and are + * not catalog actions. */ export type DeliveryActionHandler = ( content: Record, @@ -408,13 +413,99 @@ export type DeliveryActionHandler = ( inDb: Database.Database, ) => Promise; -const actionHandlers = new Map(); +/** Handler shape for guard-wrapped actions — must not touch inDb (replays run without one). */ +export type GuardedDeliveryHandler = (content: Record, session: Session) => Promise; -export function registerDeliveryAction(action: string, handler: DeliveryActionHandler): void { +export interface DeliveryGuardSpec { + /** Dotted guard-catalog action consulted before the handler runs. */ + guardAction: string; + /** + * 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; +} + +const actionHandlers = new Map(); +const guardedActions = new Map(); + +export function registerDeliveryAction(action: string, handler: DeliveryActionHandler): void; +export function registerDeliveryAction(action: string, handler: GuardedDeliveryHandler, spec: DeliveryGuardSpec): void; +export function registerDeliveryAction( + action: string, + handler: DeliveryActionHandler | GuardedDeliveryHandler, + spec?: DeliveryGuardSpec, +): void { if (actionHandlers.has(action)) { + // Replacing a guard-wrapped action with an unguarded handler would + // disarm the guard while the catalog (and the conformance walk) still + // report it guarded — 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 (!spec && guardedActions.has(action)) { + 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); + if (!spec) { + actionHandlers.set(action, handler as DeliveryActionHandler); + return; + } + guardedActions.set(action, { spec, handler: handler as GuardedDeliveryHandler }); + actionHandlers.set(action, (content, session) => runGuardedDeliveryAction(action, content, session, null)); +} + +async function runGuardedDeliveryAction( + action: string, + content: Record, + session: Session, + grant: PendingApproval | null, +): Promise { + const entry = guardedActions.get(action); + if (!entry) { + log.warn('Unknown guarded delivery action', { action }); + return; + } + const { spec, handler } = entry; + + if (spec.precheck && !(await spec.precheck(content, session))) return; + + const decision = guard({ + action: 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); +} + +/** + * Approve continuation for a guard-wrapped delivery action: re-enter the + * wrapped entry with the approval row as the grant. The guard treats the + * grant as hold-satisfied but re-runs the structural baseline, 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 }) => { + await runGuardedDeliveryAction(action, ctx.payload, ctx.session, ctx.approval); + }; } /** Look up a registered delivery-action handler. Lets module registrations be behavior-tested. */ @@ -422,6 +513,13 @@ export function getDeliveryAction(action: string): DeliveryActionHandler | undef return actionHandlers.get(action); } +/** Registered delivery actions with their guard mapping, for the conformance test. */ +export function listDeliveryActions(): { action: string; guardAction: string | null }[] { + return [...actionHandlers.keys()] + .sort() + .map((action) => ({ action, guardAction: guardedActions.get(action)?.spec.guardAction ?? null })); +} + /** * Handle system actions from the container agent. * These are written to messages_out because the container can't write to inbound.db. diff --git a/src/guard-conformance.ts b/src/guard-conformance.ts new file mode 100644 index 000000000..8cbf562be --- /dev/null +++ b/src/guard-conformance.ts @@ -0,0 +1,106 @@ +/** + * Guard conformance — the non-bypass invariant, shared by CI and boot. + * + * CI only protects code that goes through the repo's CI, but NanoClaw's + * extension model is skill-installed code: /add-* skills copy modules into + * the user's tree and register handlers on machines where the test suite + * never runs. Running the same walk at boot turns the conformance test's + * guarantee into a runtime invariant at exactly that trust boundary: every + * registration is an import-time side effect, so by the time main() runs + * the registries are complete and an unmapped privileged entry is + * detectable before the host accepts a single message. + * + * Fail-closed: the host refuses to start (the upgrade-tripwire posture). + * A conformance failure is code mis-composition — fixable with the host + * down — and it surfaces at skill-install time, when the installing agent + * is watching, instead of running unguarded until someone runs pnpm test. + * + * Limit: the walk verifies DECLARED mappings ("every delivery action is + * guarded or explicitly exempt") — it cannot infer which actions are + * privileged. That declaration stays on the author; the exemption list + * below is the loud, reviewable escape hatch. + */ +import { commandGuardAction } from './cli/guard.js'; +import { listCommands } from './cli/registry.js'; +import { listDeliveryActions } from './delivery.js'; +import { getGuardedAction } from './guard/index.js'; +import { log } from './log.js'; + +/** + * Delivery actions that deliberately carry no guard mapping (the declared + * exemption class, per the guarded-actions design decision 1): + * - scheduling self-actions — an agent mutating only its own task rows; + * not a privileged action class (yet). + * - cli_request — the transport bridge into dispatch(); every inner + * command is guarded at dispatch, so the envelope carries no privilege. + */ +export const EXEMPT_DELIVERY_ACTIONS = new Set([ + 'schedule_task', + 'cancel_task', + 'pause_task', + 'resume_task', + 'update_task', + 'cli_request', +]); + +/** Walk the live registries against the guard catalog. Empty = conformant. */ +export function guardConformanceViolations(): string[] { + const violations: string[] = []; + + for (const cmd of listCommands()) { + const entry = getGuardedAction(commandGuardAction(cmd)); + if (!entry) { + violations.push(`command "${cmd.name}" has no guard-catalog entry`); + continue; + } + if (cmd.access === 'approval' && entry.approvalAction !== 'cli_command') { + violations.push(`mutating command "${cmd.name}" maps to a catalog entry that cannot hold`); + } + } + + for (const { action, guardAction } of listDeliveryActions()) { + if (guardAction === null) { + if (!EXEMPT_DELIVERY_ACTIONS.has(action)) { + violations.push(`delivery action "${action}" is neither guard-mapped nor on the declared exemption list`); + } + continue; + } + if (!getGuardedAction(guardAction)) { + violations.push(`delivery action "${action}" maps to unregistered guard action "${guardAction}"`); + } + } + + return violations; +} + +/** + * Boot check: refuse to start when any privileged registration is unmapped. + * Call after all import-time registrations (any point in main()). + */ +export function enforceGuardConformance(): void { + const violations = guardConformanceViolations(); + if (violations.length === 0) return; + + console.error( + [ + '', + '='.repeat(64), + 'NanoClaw stopped: guard conformance failure', + '='.repeat(64), + 'A privileged registration is not mapped to the guard catalog —', + 'it would run with no allow/hold/deny decision. This usually means', + 'a skill (or local change) registered a command or delivery action', + 'without a guard spec.', + '', + ...violations.map((v) => ` - ${v}`), + '', + 'Fix the registration (pass a guard spec / derive a catalog entry),', + 'or — only for genuinely unprivileged self-actions — add it to', + 'EXEMPT_DELIVERY_ACTIONS in src/guard-conformance.ts.', + '='.repeat(64), + '', + ].join('\n'), + ); + log.error('Guard conformance failure — refusing to start', { violations }); + process.exit(1); +} diff --git a/src/guard/catalog.ts b/src/guard/catalog.ts new file mode 100644 index 000000000..1a00d4e3e --- /dev/null +++ b/src/guard/catalog.ts @@ -0,0 +1,53 @@ +/** + * The action catalog — the enforcement boundary. + * + * An action either is in the catalog (and passes a decision) or is not (and + * needs none — reads, scheduling self-actions). Declaration is registration: + * entries are derived at the registries' registration sites (command + * registry, delivery actions, response handlers, interceptors, module + * edges), never maintained in a second file. The conformance test walks the + * registries against this catalog so an unmapped privileged action fails CI. + */ +import { log } from '../log.js'; +import type { GuardDecision, GuardInput } from './types.js'; +import type { PendingApproval } from '../types.js'; + +export interface GuardedActionSpec { + /** Dotted action name — the catalog key. */ + 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). + */ + baseline: (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 baselines). + */ + approvalAction?: 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 + * approvalAction + live-row checks. + */ + grantMatches?: (grant: PendingApproval, input: GuardInput) => boolean; +} + +const catalog = new Map(); + +export function registerGuardedAction(spec: GuardedActionSpec): void { + if (catalog.has(spec.action)) { + log.warn('Guarded action re-registered (overwriting)', { action: spec.action }); + } + catalog.set(spec.action, spec); +} + +export function getGuardedAction(action: string): GuardedActionSpec | undefined { + return catalog.get(action); +} + +export function listGuardedActions(): GuardedActionSpec[] { + return [...catalog.values()].sort((a, b) => a.action.localeCompare(b.action)); +} diff --git a/src/guard/conformance.test.ts b/src/guard/conformance.test.ts new file mode 100644 index 000000000..bc945546d --- /dev/null +++ b/src/guard/conformance.test.ts @@ -0,0 +1,89 @@ +/** + * Guard conformance — the non-bypass invariant, checked structurally. + * + * Walks the real command registry and the real delivery-action registry + * (loaded via their production barrels) against the guard catalog. The walk + * itself lives in src/guard-conformance.ts and runs twice: here in CI, and + * at every boot (enforceGuardConformance in index.ts refuses to start on a + * violation) — CI can't see skill-installed registrations, the boot check + * can. A new privileged command or delivery action cannot quietly ship + * ungated: registration derives the catalog entry, and this walk makes + * drift loud. + * + * The declared exemption classes live with the walk + * (EXEMPT_DELIVERY_ACTIONS): scheduling self-actions and the cli_request + * transport bridge (its inner commands are guarded at dispatch). Reads + * (list/get/help) are catalog-mapped via registration too, but their + * baselines allow; the mutating set is what MUST be mapped. + */ +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 { listCommands } from '../cli/registry.js'; +import { commandGuardAction } from '../cli/guard.js'; +import { listDeliveryActions, registerDeliveryAction } from '../delivery.js'; +import { EXEMPT_DELIVERY_ACTIONS, guardConformanceViolations } from '../guard-conformance.js'; +import { getGuardedAction } from './catalog.js'; + +describe('guard conformance', () => { + it('the full walk (shared with the boot check) reports zero violations', () => { + expect(guardConformanceViolations()).toEqual([]); + }); + + it('every mutating ncl command maps to a guard catalog entry that can hold', () => { + const mutating = listCommands().filter((cmd) => cmd.access === 'approval'); + expect(mutating.length).toBeGreaterThan(0); + + const unmapped = mutating.filter((cmd) => { + const entry = getGuardedAction(commandGuardAction(cmd)); + return !entry || entry.approvalAction !== 'cli_command'; + }); + expect(unmapped.map((c) => c.name)).toEqual([]); + }); + + it('every registered command (reads included) has a catalog entry — denied reads still surface as denials', () => { + const unmapped = listCommands().filter((cmd) => !getGuardedAction(commandGuardAction(cmd))); + expect(unmapped.map((c) => c.name)).toEqual([]); + }); + + it('every delivery action is guard-mapped or on the declared exemption list', () => { + const actions = listDeliveryActions(); + expect(actions.length).toBeGreaterThan(0); + + const unmapped = actions.filter( + ({ action, guardAction }) => guardAction === null && !EXEMPT_DELIVERY_ACTIONS.has(action), + ); + expect(unmapped.map((a) => a.action)).toEqual([]); + + const danglingCatalog = actions.filter(({ guardAction }) => guardAction !== null && !getGuardedAction(guardAction)); + expect(danglingCatalog.map((a) => a.action)).toEqual([]); + }); + + it('the privileged delivery actions are the guarded ones', () => { + const guarded = Object.fromEntries( + listDeliveryActions() + .filter((a) => a.guardAction !== null) + .map((a) => [a.action, a.guardAction]), + ); + expect(guarded).toEqual({ + create_agent: 'agents.create', + install_packages: 'self_mod.install_packages', + add_mcp_server: 'self_mod.add_mcp_server', + }); + }); + + // KEEP LAST: registers a rogue action into the shared per-worker registry, + // so every walk after this point sees the violation. + it('the walk names an unguarded, non-exempt delivery action (what the boot check refuses on)', () => { + registerDeliveryAction('test_rogue_privileged_action', async () => {}); + + const violations = guardConformanceViolations(); + expect(violations).toHaveLength(1); + expect(violations[0]).toContain('test_rogue_privileged_action'); + expect(violations[0]).toContain('neither guard-mapped nor on the declared exemption list'); + }); +}); diff --git a/src/guard/guard.test.ts b/src/guard/guard.test.ts new file mode 100644 index 000000000..d072e9537 --- /dev/null +++ b/src/guard/guard.test.ts @@ -0,0 +1,149 @@ +/** + * Guard decision-function unit tests: the baseline is the decision (allow / + * hold / deny returned as-is, non-catalog actions allow), grant semantics + * (satisfies holds, never denies; invalid → refuse), and the fail-closed + * posture on a throwing baseline. + * + * Uses synthetic catalog actions registered per test — the registry 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 { registerGuardedAction } from './catalog.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(action: string, extra: Partial = {}): GuardInput { + return { action, actor: AGENT, payload: {}, ...extra }; +} + +beforeEach(() => { + mockGetPendingApproval.mockReset(); +}); + +afterEach(() => { + vi.clearAllMocks(); +}); + +describe('the baseline is the decision', () => { + it('non-catalog action → allow', () => { + expect(guard(input('test.unregistered-read')).effect).toBe('allow'); + }); + + it('baseline allow → allow', () => { + registerGuardedAction({ action: 't.allow1', baseline: () => ALLOW('ok') }); + expect(guard(input('t.allow1')).effect).toBe('allow'); + }); + + it('baseline hold → hold, default approver chain', () => { + registerGuardedAction({ action: 't.hold1', baseline: () => HOLD('needs approval') }); + const d = guard(input('t.hold1')); + expect(d.effect).toBe('hold'); + if (d.effect === 'hold') { + expect(d.reason).toBe('needs approval'); + expect(d.approverUserId).toBeUndefined(); + } + }); + + it('baseline hold → hold, carrying a named approver', () => { + registerGuardedAction({ action: 't.hold2', baseline: () => HOLD('policy row', 'telegram:dana') }); + const d = guard(input('t.hold2')); + expect(d.effect).toBe('hold'); + if (d.effect === 'hold') expect(d.approverUserId).toBe('telegram:dana'); + }); + + it('baseline deny → deny, carrying the reason', () => { + registerGuardedAction({ action: 't.deny1', baseline: () => DENY('structurally unauthorized') }); + const d = guard(input('t.deny1')); + expect(d.effect).toBe('deny'); + if (d.effect === 'deny') expect(d.reason).toBe('structurally unauthorized'); + }); +}); + +describe('grants', () => { + const grantRow = (action: string) => + ({ approval_id: 'appr-1', action, payload: '{}' }) as unknown as NonNullable; + + it('a valid live grant satisfies a hold', () => { + registerGuardedAction({ + action: 't.g1', + approvalAction: 'g1_approved', + baseline: () => HOLD('b'), + }); + const grant = grantRow('g1_approved'); + mockGetPendingApproval.mockReturnValue(grant); + expect(guard(input('t.g1', { grant })).effect).toBe('allow'); + }); + + it('a grant never satisfies a deny — the baseline is re-checked live', () => { + registerGuardedAction({ action: 't.g2', approvalAction: 'g2_approved', baseline: () => DENY('revoked since') }); + const grant = grantRow('g2_approved'); + mockGetPendingApproval.mockReturnValue(grant); + const d = guard(input('t.g2', { 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', () => { + registerGuardedAction({ + action: 't.g3', + approvalAction: 'g3_approved', + baseline: () => HOLD('b'), + }); + mockGetPendingApproval.mockReturnValue(undefined); + const d = guard(input('t.g3', { grant: grantRow('g3_approved') })); + expect(d.effect).toBe('deny'); + }); + + it("a grant for a different action doesn't transfer", () => { + registerGuardedAction({ + action: 't.g4', + approvalAction: 'g4_approved', + baseline: () => HOLD('b'), + }); + const grant = grantRow('other_action'); + mockGetPendingApproval.mockReturnValue(grant); + expect(guard(input('t.g4', { grant })).effect).toBe('deny'); + }); + + it('a domain grantMatches binding can refuse a payload mismatch', () => { + registerGuardedAction({ + action: 't.g5', + approvalAction: 'g5_approved', + grantMatches: () => false, + baseline: () => HOLD('b'), + }); + const grant = grantRow('g5_approved'); + mockGetPendingApproval.mockReturnValue(grant); + expect(guard(input('t.g5', { grant })).effect).toBe('deny'); + }); + + it('a grant on an already-allowed action is a no-op', () => { + registerGuardedAction({ action: 't.g6', approvalAction: 'g6_approved', baseline: () => ALLOW('ok') }); + const grant = grantRow('g6_approved'); + mockGetPendingApproval.mockReturnValue(grant); + expect(guard(input('t.g6', { grant })).effect).toBe('allow'); + }); +}); + +describe('fail-closed posture', () => { + it('a throwing baseline denies', () => { + registerGuardedAction({ + action: 't.f1', + baseline: () => { + throw new Error('boom'); + }, + }); + expect(guard(input('t.f1')).effect).toBe('deny'); + }); +}); diff --git a/src/guard/guard.ts b/src/guard/guard.ts new file mode 100644 index 000000000..b38a3ccad --- /dev/null +++ b/src/guard/guard.ts @@ -0,0 +1,65 @@ +/** + * guard() — the one decision function every privileged action consults. + * + * The decision is the catalog entry's structural baseline — today's code + * checks, registered per action at the module edges. Non-catalog actions + * (reads, scheduling self-actions) allow. Policy-as-data (tighten-only rule + * sources composing with the baseline) is deliberately deferred to phase 3 + * of the guarded-actions design, where the generalized rules table arrives + * with its first operator-visible consumer; until then the one policy table + * (agent_message_policies) is consulted inside the a2a.send baseline. + * + * Grants: an approved replay carries the verified approval row. A valid + * grant (live pending row whose action matches the catalog entry's approval + * action, plus any domain binding) satisfies a hold — the human already + * decided — but NEVER a deny: the baseline is re-checked 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 baseline denies. + */ +import { getPendingApproval } from '../db/sessions.js'; +import { log } from '../log.js'; +import { getGuardedAction } from './catalog.js'; +import { ALLOW, DENY, type GuardDecision, type GuardInput } from './types.js'; + +export function guard(input: GuardInput): GuardDecision { + const entry = getGuardedAction(input.action); + + let decision: GuardDecision; + try { + decision = entry ? entry.baseline(input) : ALLOW('non-catalog action'); + } catch (err) { + log.error('Guard evaluation threw — failing closed', { action: input.action, err }); + return DENY('guard failure (failing closed)'); + } + + if (!input.grant || decision.effect !== 'hold') { + // A grant never loosens a deny (the baseline re-check is 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 (entry && grantSatisfies(input, entry.approvalAction, entry.grantMatches)) { + return ALLOW(`hold satisfied by approval ${input.grant.approval_id}`); + } + return DENY('replay carried an invalid or mismatched grant'); +} + +function grantSatisfies( + input: GuardInput, + approvalAction: string | undefined, + grantMatches: ((grant: NonNullable, input: GuardInput) => boolean) | undefined, +): boolean { + const grant = input.grant; + if (!grant || !approvalAction) return false; + if (grant.action !== approvalAction) 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 !== approvalAction) return false; + if (grantMatches && !grantMatches(grant, input)) return false; + return true; +} diff --git a/src/guard/index.ts b/src/guard/index.ts new file mode 100644 index 000000000..7bf897e1a --- /dev/null +++ b/src/guard/index.ts @@ -0,0 +1,10 @@ +/** + * Guard — the privileged-action decision seam (guarded-actions phase 2). + * + * See the guarded-actions decisions doc on the team hub. One decision + * function (guard.ts) and a registration-derived action catalog (catalog.ts). + * Domain-free leaf: domain baselines register from the domain modules' edges. + */ +export { guard } from './guard.js'; +export { registerGuardedAction, getGuardedAction, listGuardedActions, type GuardedActionSpec } from './catalog.js'; +export { ALLOW, DENY, HOLD, type GuardActor, type GuardDecision, type GuardInput } from './types.js'; diff --git a/src/guard/types.ts b/src/guard/types.ts new file mode 100644 index 000000000..4925effa7 --- /dev/null +++ b/src/guard/types.ts @@ -0,0 +1,51 @@ +/** + * 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 structural baseline checks) arrives via + * registration: catalog entries (catalog.ts) are registered by the domain + * modules at their module edges. + */ +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 { + /** Dotted catalog action name, e.g. 'roles.grant', 'agents.create', 'a2a.send'. */ + action: string; + 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 baseline is re-checked live on every replay. + */ + grant?: PendingApproval | null; +} + +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, +}); diff --git a/src/index.ts b/src/index.ts index 72887f32b..2ecf46d76 100644 --- a/src/index.ts +++ b/src/index.ts @@ -14,6 +14,7 @@ import { initDb } from './db/connection.js'; import { runMigrations } from './db/migrations/index.js'; import { ensureContainerRuntimeRunning, cleanupOrphans } from './container-runtime.js'; import { startActiveDeliveryPoll, startSweepDeliveryPoll, setDeliveryAdapter, stopDeliveryPolls } from './delivery.js'; +import { enforceGuardConformance } from './guard-conformance.js'; import { startHostSweep, stopHostSweep } from './host-sweep.js'; import { routeInbound } from './router.js'; import { log } from './log.js'; @@ -69,6 +70,12 @@ async function main(): Promise { // outside the sanctioned path (raw `git pull` instead of /update-nanoclaw). enforceUpgradeTripwire(); + // 0.6 Guard conformance — every import-time registration has already run; + // refuse to start if any privileged command / delivery action is unmapped + // (CI can't see skill-installed code — this makes the invariant hold at + // the boundary where third-party registrations enter). + enforceGuardConformance(); + // 1. Init central DB const dbPath = path.join(DATA_DIR, 'v2.db'); const db = initDb(dbPath); diff --git a/src/modules/agent-to-agent/agent-route.ts b/src/modules/agent-to-agent/agent-route.ts index 5ad169a14..feb52ad59 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 { 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 } from './guard.js'; export { isSafeAttachmentName }; +export { A2A_MESSAGE_GATE_ACTION } from './guard.js'; export interface ForwardedAttachment { name: string; @@ -230,56 +231,65 @@ 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 baseline (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({ + action: 'a2a.send', + 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 Error(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 +318,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 f9f6219c7..d67853e0b 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` baseline — 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 — baseline 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 b8044da39..312d1f30b 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` baseline + * (./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 000000000..740405804 --- /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, registerGuardedAction } 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'; + +registerGuardedAction({ + action: 'agents.create', + approvalAction: 'create_agent', + // Bind a create_agent grant to the name that was approved. + grantMatches: (grant, input) => { + try { + return (JSON.parse(grant.payload) as { name?: string }).name === input.payload.name; + } catch { + return false; + } + }, + baseline: (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'); + }, +}); + +registerGuardedAction({ + action: 'a2a.send', + approvalAction: A2A_MESSAGE_GATE_ACTION, + // Bind an a2a grant to the exact held message target. + grantMatches: (grant, input) => { + try { + return (JSON.parse(grant.payload) as { platform_id?: string }).platform_id === input.resource?.to; + } catch { + return false; + } + }, + baseline: (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 95dfd36c6..fa9047ecb 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 baseline 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 './guard.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 { applyA2aMessageGate } from './message-gate.js'; -registerDeliveryAction('create_agent', handleCreateAgent); -registerApprovalHandler('create_agent', applyCreateAgent); +registerDeliveryAction('create_agent', createAgent, { + guardAction: 'agents.create', + 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 fa1a485f6..fd9d5c3a8 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,71 @@ 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 → the approved replay is blocked', 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 + + await expect( + applyA2aMessageGate({ session: SA, userId: 'telegram:dana', notify: vi.fn(), payload, approval }), + ).rejects.toThrow(/unauthorized agent-to-agent/); + expect(readInbound(B, SB.id)).toHaveLength(0); + }); + + it('mismatched grant (held for another target) refuses the replay', 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 }; + + await expect( + applyA2aMessageGate({ session: SA, userId: 'telegram:dana', notify: vi.fn(), payload, approval }), + ).rejects.toThrow(/invalid or mismatched grant/); + expect(readInbound(B, SB.id)).toHaveLength(0); + }); + + 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 + + await expect( + applyA2aMessageGate({ session: SA, userId: 'telegram:dana', notify: vi.fn(), payload, approval }), + ).rejects.toThrow(/invalid or mismatched grant/); + expect(readInbound(B, SB.id)).toHaveLength(0); }); // ── ghost-gate cleanup ── diff --git a/src/modules/agent-to-agent/message-gate.ts b/src/modules/agent-to-agent/message-gate.ts index 74c20c4cb..b93a1a0cc 100644 --- a/src/modules/agent-to-agent/message-gate.ts +++ b/src/modules/agent-to-agent/message-gate.ts @@ -1,9 +1,9 @@ /** Approve handler for a held a2a message. (Reject is handled by the generic response-handler path.) */ 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 +18,12 @@ 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 + // baseline runs live — un-wiring the pair between hold and approve now + // blocks delivery (the throw surfaces via the response handler's + // "approved, but applying it failed" notify). + await routeAgentMessage(msg, session, { grant: approval }); 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 2d3028a64..48047e676 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 52fc705ef..79014597e 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 02e87d373..ac4f6ce98 100644 --- a/src/modules/permissions/channel-approval.test.ts +++ b/src/modules/permissions/channel-approval.test.ts @@ -54,7 +54,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'; @@ -438,6 +442,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 guarded + // interceptor allows (still an eligible approver) 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 guard's baseline 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 000000000..91cface46 --- /dev/null +++ b/src/modules/permissions/guard.ts @@ -0,0 +1,54 @@ +/** + * 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 + * approvalAction: the approve continuation adds the member and replays + * routeInbound, which then passes the gate structurally via membership, no + * grant needed. + * + * channels.register — click/reply 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 by the wrapped + * response handler (card clicks) and the wrapped name-capture interceptor + * (free-text replies), so a privilege revoked mid-flow is re-checked at each + * step. + */ +import { ALLOW, DENY, HOLD, registerGuardedAction } from '../../guard/index.js'; +import { getPendingChannelApproval } from './db/pending-channel-approvals.js'; +import { hasAdminPrivilege } from './db/user-roles.js'; + +registerGuardedAction({ + action: 'senders.admit', + baseline: (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'); + }, +}); + +registerGuardedAction({ + action: 'channels.register', + baseline: (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 6a6f19b43..4ead62019 100644 --- a/src/modules/permissions/index.ts +++ b/src/modules/permissions/index.ts @@ -32,6 +32,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.js'; +import { guard } from '../../guard/index.js'; import { canAccessAgentGroup } from './access.js'; import { buildAgentSelectionOptions, @@ -129,43 +131,50 @@ 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 baseline (./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({ + action: 'senders.admit', + actor: userId ? { kind: 'human', userId } : { kind: 'system' }, + payload: { + messagingGroupId: mg.id, + agentGroupId, + senderIdentity: userId, + policy: mg.unknown_sender_policy, + }, + }); + + if (decision.effect === 'allow') return; // 'public' — handled before the gate; fall through silently. + + 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); - return; - } + }, + ); + recordDroppedMessage(dropRecord); - if (mg.unknown_sender_policy === 'request_approval') { - log.info('MESSAGE DROPPED — unknown sender (approval requested)', { + // 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, - 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; + 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); @@ -311,21 +320,14 @@ async function handleChannelApprovalResponse(payload: ResponsePayload): Promise< const row = getPendingChannelApproval(payload.questionId); if (!row) return false; + // Click authorization is the guard's channels.register baseline (./guard.ts), + // consulted by the response-registry wrapper before this handler runs. 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) { - log.warn('Channel registration click rejected — unauthorized clicker', { - messagingGroupId: row.messaging_group_id, - clickerId, - expectedApprover: row.approver_user_id, - }); - return true; - } + if (!clickerId) return true; // unreachable behind the guard wrapper; fail closed const approverId = clickerId; // ── Reject / Cancel ── @@ -515,13 +517,19 @@ async function handleChannelApprovalResponse(payload: ResponsePayload): Promise< return true; } -registerResponseHandler(handleChannelApprovalResponse); +registerResponseHandler(handleChannelApprovalResponse, { + action: 'channels.register', + claims: (payload) => getPendingChannelApproval(payload.questionId) !== undefined, +}); // ── Free-text name interceptor ── // Captures the next DM from an approver who clicked "Create new agent", -// creates the agent immediately, wires the channel, and replays. +// creates the agent immediately, wires the channel, and replays. The router +// wraps it with the guard: the free-texter must still be an eligible +// channel-registration approver at reply time — a privilege revoked between +// the click and the reply now denies, and the arming is disarmed. -registerMessageInterceptor(async (event: InboundEvent): Promise => { +const captureAgentNameReply = async (event: InboundEvent): Promise => { const userId = extractAndUpsertUser(event); if (!userId) return false; @@ -629,4 +637,20 @@ registerMessageInterceptor(async (event: InboundEvent): Promise => { } } return true; +}; + +registerMessageInterceptor(captureAgentNameReply, { + action: 'channels.register', + claims: (event) => { + const userId = extractAndUpsertUser(event); + if (!userId) return null; + const pending = awaitingNameInput.get(userId); + if (!pending) return null; + if (event.channelType !== pending.dmChannelType || event.platformId !== pending.dmPlatformId) return null; + return { actor: { kind: 'human', userId }, payload: { questionId: pending.channelMgId } }; + }, + onDeny: (event) => { + const userId = extractAndUpsertUser(event); + if (userId) awaitingNameInput.delete(userId); + }, }); diff --git a/src/modules/self-mod/apply.ts b/src/modules/self-mod/apply.ts index c5318ffbf..f3d3bde58 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 + * baseline 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 000000000..85faa3492 --- /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 structural baseline 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, registerGuardedAction, type GuardInput } from '../../guard/index.js'; + +function selfModBaseline(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`); + }; +} + +registerGuardedAction({ + action: 'self_mod.install_packages', + approvalAction: 'install_packages', + baseline: selfModBaseline('install_packages'), +}); + +registerGuardedAction({ + action: 'self_mod.add_mcp_server', + approvalAction: 'add_mcp_server', + baseline: selfModBaseline('add_mcp_server'), +}); diff --git a/src/modules/self-mod/index.ts b/src/modules/self-mod/index.ts index e1f49e212..f5dd53251 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 baseline 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 './guard.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 { + requestAddMcpServerHold, + requestInstallPackagesHold, + validateAddMcpServer, + validateInstallPackages, +} from './request.js'; -registerDeliveryAction('install_packages', handleInstallPackages); -registerDeliveryAction('add_mcp_server', handleAddMcpServer); +registerDeliveryAction('install_packages', applyInstallPackages, { + guardAction: 'self_mod.install_packages', + precheck: validateInstallPackages, + requestHold: requestInstallPackagesHold, + onDeny: (_content, session, reason) => notifyAgent(session, `install_packages denied: ${reason}`), +}); +registerDeliveryAction('add_mcp_server', applyAddMcpServer, { + guardAction: 'self_mod.add_mcp_server', + 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 6cd7f05c9..21e558c18 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, diff --git a/src/response-registry.ts b/src/response-registry.ts index 60e04c998..57c42e511 100644 --- a/src/response-registry.ts +++ b/src/response-registry.ts @@ -7,10 +7,19 @@ * which triggers module registrations that would otherwise happen before * index.ts's own const initializers have run. * - * Keep this file dependency-free (log.js is fine, but nothing from - * modules/* or index.ts itself). Any file imported here must not in turn - * import from src/index.ts, or the cycle returns. + * Keep this file dependency-free (log.js and the guard leaf are fine, but + * nothing from modules/* or index.ts itself). Any file imported here must + * not in turn import from src/index.ts, or the cycle returns. + * + * A handler whose click performs a privileged operation registers with a + * guard spec: the registry wraps it so the guard's decision stands between + * the click and the handler, and the wrapped path is the only path. `claims` + * is the handler's own claim test (does this questionId belong to me?) so an + * unauthorized click is claimed-and-dropped without stealing other handlers' + * responses. */ +import { guard, type GuardActor } from './guard/index.js'; +import { log } from './log.js'; export interface ResponsePayload { questionId: string; @@ -23,10 +32,45 @@ export interface ResponsePayload { export type ResponseHandler = (payload: ResponsePayload) => Promise; +export interface ResponseGuardSpec { + /** Dotted guard-catalog action consulted before the handler runs. */ + action: string; + /** Would this handler claim the response? (Its own row lookup.) */ + claims: (payload: ResponsePayload) => boolean; +} + const responseHandlers: ResponseHandler[] = []; -export function registerResponseHandler(handler: ResponseHandler): void { - responseHandlers.push(handler); +function responseActor(payload: ResponsePayload): GuardActor { + if (!payload.userId) return { kind: 'human', userId: '' }; + const userId = payload.userId.includes(':') ? payload.userId : `${payload.channelType}:${payload.userId}`; + return { kind: 'human', userId }; +} + +export function registerResponseHandler(handler: ResponseHandler, guardSpec?: ResponseGuardSpec): void { + if (!guardSpec) { + responseHandlers.push(handler); + return; + } + responseHandlers.push(async (payload) => { + if (!guardSpec.claims(payload)) return false; + const decision = guard({ + action: guardSpec.action, + actor: responseActor(payload), + payload: { questionId: payload.questionId, value: payload.value }, + }); + if (decision.effect !== 'allow') { + // Claim the response so it's not unclaimed-logged, but do nothing. + log.warn('Response click rejected by guard', { + action: guardSpec.action, + questionId: payload.questionId, + userId: payload.userId, + reason: decision.reason, + }); + return true; + } + return handler(payload); + }); } export function getResponseHandlers(): readonly ResponseHandler[] { diff --git a/src/router.ts b/src/router.ts index d39c6a064..d89db0272 100644 --- a/src/router.ts +++ b/src/router.ts @@ -20,6 +20,7 @@ import { getChannelAdapter } from './channels/channel-registry.js'; import { gateCommand } from './command-gate.js'; import { getAgentGroup } from './db/agent-groups.js'; +import { guard, type GuardActor } from './guard/index.js'; import { recordDroppedMessage } from './db/dropped-messages.js'; import { createMessagingGroup, @@ -117,13 +118,46 @@ export function setSenderScopeGate(fn: SenderScopeGateFn): void { * Used by modules to capture free-text DM replies during multi-step approval * flows — the permissions module (agent naming during channel registration) * and the approvals module (reject-with-reason capture). + * + * An interceptor whose capture performs a privileged operation (the + * channel-registration name capture creates an agent group + wiring) + * registers with a guard spec: the registry wraps it so the guard's decision + * stands between the free-text reply and the handler. `claims` returns the + * guard consult for events the interceptor would act on (null = not mine, + * pass through); a deny consumes the message without acting. */ export type MessageInterceptorFn = (event: InboundEvent) => Promise; +export interface InterceptorGuardSpec { + /** Dotted guard-catalog action consulted before the interceptor acts. */ + action: string; + /** The guard consult for events this interceptor would act on; null = not mine. */ + claims: (event: InboundEvent) => { actor: GuardActor; payload: Record } | null; + /** Domain cleanup when the guard denies (e.g. disarm the capture). */ + onDeny?: (event: InboundEvent) => void; +} + const messageInterceptors: MessageInterceptorFn[] = []; -export function registerMessageInterceptor(fn: MessageInterceptorFn): void { - messageInterceptors.push(fn); +export function registerMessageInterceptor(fn: MessageInterceptorFn, guardSpec?: InterceptorGuardSpec): void { + if (!guardSpec) { + messageInterceptors.push(fn); + return; + } + messageInterceptors.push(async (event) => { + const consult = guardSpec.claims(event); + if (!consult) return fn(event); + const decision = guard({ action: guardSpec.action, actor: consult.actor, payload: consult.payload }); + if (decision.effect !== 'allow') { + log.warn('Interceptor capture rejected by guard — consuming without acting', { + action: guardSpec.action, + reason: decision.reason, + }); + guardSpec.onDeny?.(event); + return true; + } + return fn(event); + }); } /**