From c6d5cd7d022a30dcb084693863ea7e5aca0156ea Mon Sep 17 00:00:00 2001 From: glifocat Date: Sun, 10 May 2026 20:47:51 +0200 Subject: [PATCH] fixup(cli-scope): build error, false-positive on custom ops, tests, drop FORK.md MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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)` 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) --- FORK.md | 15 ------ src/cli/crud.ts | 2 + src/cli/dispatch.test.ts | 109 +++++++++++++++++++++++++++++++++++++++ src/cli/dispatch.ts | 23 ++++++--- src/cli/registry.ts | 7 +++ 5 files changed, 134 insertions(+), 22 deletions(-) delete mode 100644 FORK.md diff --git a/FORK.md b/FORK.md deleted file mode 100644 index f92e260..0000000 --- a/FORK.md +++ /dev/null @@ -1,15 +0,0 @@ -# nanoclaw-glifocat - -Personal [NanoClaw](https://github.com/qwibitai/nanoclaw) fork - customizations, upstream contributions and experimentation. - -## About - -I'm a core maintainer of NanoClaw and also maintain the official documentation at [docs.nanoclaw.dev](https://docs.nanoclaw.dev). - -This fork is where I develop fixes and features before contributing them upstream, test experimental integrations, and run my personal agent setup. - -## Links - -- **Upstream repo:** [qwibitai/nanoclaw](https://github.com/qwibitai/nanoclaw) -- **Official docs:** [docs.nanoclaw.dev](https://docs.nanoclaw.dev) -- **Docs repo:** [glifocat/nanoclaw-docs](https://github.com/glifocat/nanoclaw-docs) diff --git a/src/cli/crud.ts b/src/cli/crud.ts index f2bb82b..4889bf0 100644 --- a/src/cli/crud.ts +++ b/src/cli/crud.ts @@ -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), }); 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 13d4761..7878a9b 100644 --- a/src/cli/dispatch.ts +++ b/src/cli/dispatch.ts @@ -94,7 +94,7 @@ export async function dispatch(req: RequestFrame, ctx: CallerContext): Promise).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 = { 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;