Merge pull request #2392 from glifocat/fix/cli-scope-hardening

fix(cli-scope): fail-closed scopeField enforcement + sessions-get oracle guard

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),
     });

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<string, unknown>).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<string, string> = {
+    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');
+    }
+  });
 });

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<ResponseFrame> {
@@ -87,6 +88,16 @@ export async function dispatch(req: RequestFrame, ctx: CallerContext): Promise<R
         fill.id = req.args.id ?? ctx.agentGroupId;
       }
       req = { ...req, args: { ...req.args, ...fill } };
+
+      // Fail-closed pre-handler check for sessions-get: returns "not found"
+      // regardless of whether the UUID exists in another group, preventing an
+      // existence oracle across group boundaries.
+      if (cmd.resource === 'sessions' && req.command === 'sessions-get' && req.args.id) {
+        const s = getSession(req.args.id as string);
+        if (!s || s.agent_group_id !== ctx.agentGroupId) {
+          return err(req.id, 'handler-error', `session not found: ${req.args.id}`);
+        }
+      }
     }
   }
 
@@ -124,14 +135,28 @@ export async function dispatch(req: RequestFrame, ctx: CallerContext): Promise<R
   try {
     let data = await cmd.handler(parsed, ctx);
 
-    // Post-handler group scope enforcement: filter/verify results belong
-    // to the caller's agent group. Catches leaks that pre-handler auto-fill
-    // can't prevent (e.g. `groups list` where the id arg is skipped by the
-    // generic list handler, or `sessions get` by UUID).
-    if (ctx.caller === 'agent' && cmd.resource) {
+    // Post-handler group-scope enforcement. Applies only to the auto-generated
+    // `list` / `get` handlers (`cmd.generic`), which return raw DB rows carrying
+    // the resource's `scopeField`:
+    //   - `list` → drop rows that don't belong to the caller's agent group
+    //              (covers `groups list`, where the generic list handler ignores
+    //              the auto-filled `--id`)
+    //   - `get`  → reject if the single row belongs to another group
+    // Custom operations return ad-hoc shapes (e.g. `groups config get` → a config
+    // object with no `id`) and are NOT checked here — they would be falsely
+    // rejected, and they're already pinned to the caller's group by the
+    // pre-handler `--id` auto-fill (groups/destinations) or gated behind approval,
+    // so they can't reach another group's data anyway.
+    if (ctx.caller === 'agent' && cmd.resource && cmd.generic) {
       const configRow = getContainerConfig(ctx.agentGroupId);
       if ((configRow?.cli_scope ?? 'group') === 'group') {
-        const groupField = cmd.resource === 'groups' ? 'id' : 'agent_group_id';
+        const def = getResource(cmd.resource);
+        const groupField = def?.scopeField;
+        if (!groupField) {
+          // Fail closed: a whitelisted resource exposing list/get must declare
+          // `scopeField` so its rows can be filtered.
+          return err(req.id, 'forbidden', `"${cmd.resource}" is not available in group scope.`);
+        }
         if (Array.isArray(data)) {
           data = data.filter(
             (row) =>
@@ -139,7 +164,7 @@ export async function dispatch(req: RequestFrame, ctx: CallerContext): Promise<R
               row !== null &&
               (row as Record<string, unknown>)[groupField] === ctx.agentGroupId,
           );
-        } else if (data && typeof data === 'object' && groupField in (data as Record<string, unknown>)) {
+        } else if (data && typeof data === 'object') {
           if ((data as Record<string, unknown>)[groupField] !== ctx.agentGroupId) {
             return err(req.id, 'forbidden', 'Resource belongs to a different agent group.');
           }

src/cli/registry.ts

@@ -15,6 +15,13 @@ export type CommandDef<TArgs = unknown, TData = unknown> = {
   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<string, unknown>) => TArgs;
   handler: (args: TArgs, ctx: CallerContext) => Promise<TData>;

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',

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 },
     {

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',

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.' },