fixup(cli-scope): build error, false-positive on custom ops, tests, drop FORK.md
Addresses review feedback on this branch: - Fix TS2352 build error in dispatch.ts: `getSession()` returns `Session`, which has no index signature, so `(s as Record<string, unknown>)` is rejected by tsc. `Session.agent_group_id` exists — read it directly. - Fix a regression introduced by dropping the `groupField in data` guard: the post-handler scope check now runs for *every* command under a whitelisted resource, including custom ops, which return ad-hoc shapes. `ncl groups config get` (access:open, reachable by a group-scoped agent) returns a config object with no `id` field → `data['id'] !== ctx.agentGroupId` → `forbidden`, even on the agent's own config. Fix: tag the auto-generated list/get handlers with `generic: 'list' | 'get'` on `CommandDef` (set in `registerResource`) and run the post-handler check only when `cmd.generic` is set. Generic handlers return raw DB rows that carry `scopeField`; custom ops are already pinned to the caller's group by the pre-handler `--id` auto-fill or the approval gate. Fail-closed-when-`scopeField`-missing is preserved (now scoped to generic list/get). - Tests: `dispatch.test.ts` mocks `getResource` (the real resources aren't registered in this unit), tags the two post-handler test commands as `generic`, and adds coverage for: custom op returning a non-row object not being rejected; `sessions-get` pre-handler returning "session not found" for foreign and non-existent UUIDs (no existence oracle) and allowing the caller's own session; generic list/get failing closed when a resource declares no `scopeField`. Full suite: 323 passing. - Remove FORK.md from the PR diff — it's the fork's personal README, carried in because the branch was cut from the fork's `main` rather than upstream. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -232,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),
|
||||
});
|
||||
@@ -243,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),
|
||||
});
|
||||
|
||||
@@ -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');
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
@@ -94,7 +94,7 @@ export async function dispatch(req: RequestFrame, ctx: CallerContext): Promise<R
|
||||
// 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 as Record<string, unknown>).agent_group_id !== ctx.agentGroupId) {
|
||||
if (!s || s.agent_group_id !== ctx.agentGroupId) {
|
||||
return err(req.id, 'handler-error', `session not found: ${req.args.id}`);
|
||||
}
|
||||
}
|
||||
@@ -135,17 +135,26 @@ 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 def = getResource(cmd.resource);
|
||||
const groupField = def?.scopeField;
|
||||
if (!groupField) {
|
||||
// Fail closed: resource not declared as group-scope safe.
|
||||
// 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)) {
|
||||
|
||||
@@ -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>;
|
||||
|
||||
Reference in New Issue
Block a user