diff --git a/src/cli/crud.ts b/src/cli/crud.ts index 9c7ed99..4889bf0 100644 --- a/src/cli/crud.ts +++ b/src/cli/crud.ts @@ -52,6 +52,12 @@ export interface ResourceDef { description: string; /** Primary key column name. */ idColumn: string; + /** + * Column that carries the agent group ID for group-scope enforcement. + * Required on every resource in the CLI whitelist (groups, sessions, + * destinations, members). When absent, post-handler filtering fails closed. + */ + scopeField?: string; columns: ColumnDef[]; /** Which standard CRUD operations are enabled. */ operations: { @@ -226,6 +232,7 @@ export function registerResource(def: ResourceDef): void { description: `List all ${def.plural}.`, access: def.operations.list, resource: def.plural, + generic: 'list', parseArgs: (raw) => normalizeArgs(raw), handler: genericList(def), }); @@ -237,6 +244,7 @@ export function registerResource(def: ResourceDef): void { description: `Get a ${def.name} by ID.`, access: def.operations.get, resource: def.plural, + generic: 'get', parseArgs: (raw) => normalizeArgs(raw), handler: genericGet(def), }); diff --git a/src/cli/dispatch.test.ts b/src/cli/dispatch.test.ts index b63d712..20ff187 100644 --- a/src/cli/dispatch.test.ts +++ b/src/cli/dispatch.test.ts @@ -21,6 +21,13 @@ vi.mock('../db/sessions.js', () => ({ getSession: (...args: unknown[]) => mockGetSession(...args), })); +// dispatch's post-handler looks up the resource's `scopeField` via getResource. +// The real resources aren't registered in this unit test, so mock it. +const mockGetResource = vi.fn(); +vi.mock('./crud.js', () => ({ + getResource: (...args: unknown[]) => mockGetResource(...args), +})); + vi.mock('../modules/approvals/index.js', () => ({ registerApprovalHandler: vi.fn(), requestApproval: vi.fn(), @@ -97,6 +104,7 @@ register({ description: 'returns mock group rows', resource: 'groups', access: 'open', + generic: 'list', parseArgs: (raw) => raw, handler: async () => [ { id: 'g1', name: 'my-group' }, @@ -109,6 +117,7 @@ register({ description: 'returns a mock session row', resource: 'sessions', access: 'open', + generic: 'get', parseArgs: (raw) => raw, handler: async (args) => ({ id: args.id, @@ -116,11 +125,43 @@ register({ }), }); +// A custom op under the `groups` resource that returns a config-shaped object +// (no `id` key). The post-handler must not touch this — only `generic` handlers. +register({ + name: 'groups-config-get', + description: 'custom op returning a config object (no id)', + resource: 'groups', + access: 'open', + parseArgs: (raw) => raw, + handler: async () => ({ agent_group_id: 'g1', model: 'opus' }), +}); + +// The real `sessions-get` name — triggers the pre-handler ownership check. +register({ + name: 'sessions-get', + description: 'generic sessions get', + resource: 'sessions', + access: 'open', + generic: 'get', + parseArgs: (raw) => raw, + handler: async (args) => ({ id: (args as Record).id, agent_group_id: 'g1' }), +}); + import { dispatch } from './dispatch.js'; import type { CallerContext } from './frame.js'; beforeEach(() => { vi.clearAllMocks(); + // Default: the four CLI-whitelisted resources with their real scopeFields. + const scopeFields: Record = { + groups: 'id', + sessions: 'agent_group_id', + destinations: 'agent_group_id', + members: 'agent_group_id', + }; + mockGetResource.mockImplementation((plural: string) => + scopeFields[plural] ? { scopeField: scopeFields[plural] } : undefined, + ); }); // --- Helpers --- @@ -402,4 +443,72 @@ describe('CLI scope enforcement', () => { expect(data).toHaveLength(2); // both groups returned } }); + + // --- Custom ops bypass post-handler row filtering (regression: #2392 review) --- + + it('group: a custom op returning a non-row object is not falsely rejected', async () => { + mockGetContainerConfig.mockReturnValue({ cli_scope: 'group' }); + + // groups-config-get is access:open and reachable by a group-scoped agent; + // it returns { agent_group_id, model } with no `id` field. Before this fix + // the post-handler compared data['id'] (undefined) and returned forbidden. + const resp = await dispatch({ id: '1', command: 'groups-config-get', args: {} }, agentCtx()); + + expect(resp.ok).toBe(true); + if (resp.ok) { + expect((resp.data as { model: string }).model).toBe('opus'); + } + }); + + // --- sessions-get pre-handler ownership check (no existence oracle) --- + + it('group: sessions-get returns "session not found" for a foreign session UUID', async () => { + mockGetContainerConfig.mockReturnValue({ cli_scope: 'group' }); + mockGetSession.mockReturnValue({ id: 's-x', agent_group_id: 'other-group' }); + + const resp = await dispatch({ id: '1', command: 'sessions-get', args: { id: 's-x' } }, agentCtx()); + + expect(resp.ok).toBe(false); + if (!resp.ok) { + expect(resp.error.code).toBe('handler-error'); + expect(resp.error.message).toContain('session not found'); + } + }); + + it('group: sessions-get returns "session not found" for a non-existent UUID', async () => { + mockGetContainerConfig.mockReturnValue({ cli_scope: 'group' }); + mockGetSession.mockReturnValue(undefined); + + const resp = await dispatch({ id: '1', command: 'sessions-get', args: { id: 's-nope' } }, agentCtx()); + + expect(resp.ok).toBe(false); + if (!resp.ok) { + expect(resp.error.code).toBe('handler-error'); + expect(resp.error.message).toContain('session not found'); + } + }); + + it('group: sessions-get allows the caller’s own session', async () => { + mockGetContainerConfig.mockReturnValue({ cli_scope: 'group' }); + mockGetSession.mockReturnValue({ id: 's-mine', agent_group_id: 'g1' }); + + const resp = await dispatch({ id: '1', command: 'sessions-get', args: { id: 's-mine' } }, agentCtx()); + + expect(resp.ok).toBe(true); + }); + + // --- Fail-closed regression guard for a missing scopeField --- + + it('group: generic list/get fails closed when the resource declares no scopeField', async () => { + mockGetContainerConfig.mockReturnValue({ cli_scope: 'group' }); + mockGetResource.mockReturnValue(undefined); // a whitelisted resource that forgot scopeField + + const resp = await dispatch({ id: '1', command: 'groups-list-data', args: {} }, agentCtx()); + + expect(resp.ok).toBe(false); + if (!resp.ok) { + expect(resp.error.code).toBe('forbidden'); + expect(resp.error.message).toContain('not available in group scope'); + } + }); }); diff --git a/src/cli/dispatch.ts b/src/cli/dispatch.ts index 68db969..7878a9b 100644 --- a/src/cli/dispatch.ts +++ b/src/cli/dispatch.ts @@ -11,6 +11,7 @@ import { getAgentGroup } from '../db/agent-groups.js'; import { getSession } from '../db/sessions.js'; import { registerApprovalHandler, requestApproval } from '../modules/approvals/index.js'; import type { CallerContext, ErrorCode, RequestFrame, ResponseFrame } from './frame.js'; +import { getResource } from './crud.js'; import { lookup } from './registry.js'; export async function dispatch(req: RequestFrame, ctx: CallerContext): Promise { @@ -87,6 +88,16 @@ export async function dispatch(req: RequestFrame, ctx: CallerContext): Promise @@ -139,7 +164,7 @@ export async function dispatch(req: RequestFrame, ctx: CallerContext): Promise)[groupField] === ctx.agentGroupId, ); - } else if (data && typeof data === 'object' && groupField in (data as Record)) { + } else if (data && typeof data === 'object') { if ((data as Record)[groupField] !== ctx.agentGroupId) { return err(req.id, 'forbidden', 'Resource belongs to a different agent group.'); } diff --git a/src/cli/registry.ts b/src/cli/registry.ts index a60e74a..bf4ae2c 100644 --- a/src/cli/registry.ts +++ b/src/cli/registry.ts @@ -15,6 +15,13 @@ export type CommandDef = { access: Access; /** Resource this command belongs to (for help grouping). */ resource?: string; + /** + * Set on the auto-generated `list` / `get` handlers (see `registerResource`). + * These return raw DB rows that carry the resource's `scopeField`, so the + * dispatcher applies post-handler group-scope filtering to their output. + * Custom operations return ad-hoc shapes and leave this undefined. + */ + generic?: 'list' | 'get'; /** Validates `frame.args` and produces the typed handler input. Throws on invalid. */ parseArgs: (raw: Record) => TArgs; handler: (args: TArgs, ctx: CallerContext) => Promise; diff --git a/src/cli/resources/destinations.ts b/src/cli/resources/destinations.ts index 4a56029..0ac95f3 100644 --- a/src/cli/resources/destinations.ts +++ b/src/cli/resources/destinations.ts @@ -8,6 +8,7 @@ registerResource({ description: 'Agent destination — per-agent routing entry and ACL. Each row authorizes an agent to send messages to a target (channel or another agent) and assigns a local name the agent uses to address it. Names are scoped to the source agent — two agents can have different local names for the same target. Created automatically when wiring channels or when agents create child agents.', idColumn: 'agent_group_id', + scopeField: 'agent_group_id', columns: [ { name: 'agent_group_id', diff --git a/src/cli/resources/groups.ts b/src/cli/resources/groups.ts index 83344e0..031ccb5 100644 --- a/src/cli/resources/groups.ts +++ b/src/cli/resources/groups.ts @@ -38,6 +38,7 @@ registerResource({ description: 'Agent group — a logical agent identity. Each group has its own workspace folder (CLAUDE.md, skills, container config), conversation history, and container image. Multiple messaging groups can be wired to one agent group.', idColumn: 'id', + scopeField: 'id', columns: [ { name: 'id', type: 'string', description: 'UUID.', generated: true }, { diff --git a/src/cli/resources/members.ts b/src/cli/resources/members.ts index ac529be..2a93430 100644 --- a/src/cli/resources/members.ts +++ b/src/cli/resources/members.ts @@ -8,6 +8,7 @@ registerResource({ description: 'Agent group member — grants an unprivileged user permission to interact with an agent group. Users with admin or owner roles on the group are implicitly members and do not need a separate membership row. Membership is checked by the router when sender_scope is "known".', idColumn: 'user_id', + scopeField: 'agent_group_id', columns: [ { name: 'user_id', diff --git a/src/cli/resources/sessions.ts b/src/cli/resources/sessions.ts index f60fccc..33e2407 100644 --- a/src/cli/resources/sessions.ts +++ b/src/cli/resources/sessions.ts @@ -7,6 +7,7 @@ registerResource({ description: 'Session — the runtime unit. Maps one (agent_group, messaging_group, thread) combination to a container with its own inbound.db and outbound.db. Created automatically by the router when a message arrives.', idColumn: 'id', + scopeField: 'agent_group_id', columns: [ { name: 'id', type: 'string', description: 'UUID.', generated: true }, { name: 'agent_group_id', type: 'string', description: 'Agent group this session runs.' },