Skip to content

Commit ab611ea

Browse files
committed
fix(security): enforce folder validator in ncl groups create (CWE-22)
The group resource has no customInsert, so genericCreate runs a raw INSERT with the operator-supplied folder directly, skipping createAgentGroup -> assertValidGroupFolder. `ncl groups create --folder ../../etc` then persists a folder that escapes GROUPS_DIR; buildMounts/group-init bind-mount it RW and mkdir it — a path traversal into a container sandbox escape. Validate at the CLI create chokepoint too.
1 parent ee7f891 commit ab611ea

1 file changed

Lines changed: 13 additions & 0 deletions

File tree

src/cli/resources/groups.ts

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ import { buildAgentGroupImage, killContainer, wakeContainer } from '../../contai
33
import { restartAgentGroupContainers } from '../../container-restart.js';
44
import { getDb, hasTable } from '../../db/connection.js';
55
import { getSession } from '../../db/sessions.js';
6+
import { assertValidGroupFolder } from '../../group-folder.js';
67
import { writeSessionMessage } from '../../session-manager.js';
78
import {
89
getContainerConfig,
@@ -58,6 +59,18 @@ registerResource({
5859
},
5960
{ name: 'created_at', type: 'string', description: 'Auto-set.', generated: true },
6061
],
62+
// SECURITY: the CLI create path must enforce the same folder validator as the
63+
// domain helper createAgentGroup. genericCreate skips that helper, so without
64+
// this `ncl groups create --folder ../../etc` persists a folder that escapes
65+
// GROUPS_DIR and is later bind-mounted RW + mkdir'd into the container
66+
// (CWE-22 path traversal → container sandbox escape).
67+
customInsert: (values) => {
68+
assertValidGroupFolder(values.folder as string);
69+
const cols = Object.keys(values);
70+
getDb()
71+
.prepare(`INSERT INTO agent_groups (${cols.join(', ')}) VALUES (${cols.map((c) => `@${c}`).join(', ')})`)
72+
.run(values as Record<string, string>);
73+
},
6174
// `delete` is intentionally not in `operations` — the generic single-table
6275
// DELETE violates FK constraints (see #2525). The cascading handler is
6376
// provided as `customOperations.delete` below.

0 commit comments

Comments
 (0)