From 74a8ee0993a301f572f8a6d132856449de9ab98c Mon Sep 17 00:00:00 2001 From: Hermes VM Date: Sat, 30 May 2026 08:18:50 +0000 Subject: [PATCH] feat(dashboard): close 3 of 5 Phase 5 P2 mitigation items (allow-list, projectPath, audit-log) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the three Phase 5 P2 follow-ups from the DEPLOYMENT.md mitigation roadmap that don't need infra changes. Two P2 items remain (non-root container, docker-proxy daemon) — both genuinely need container/orchestration work and stay queued. 1. Allow-list shell wrapper (P1) New `lib/shell.ts`: - `execAllowed(cmd, args, opts)` — `execFile`-only, no shell, no interpolation. Single escape hatch for ad-hoc invocations. - `dockerRestart(name)` — name validated against `[a-zA-Z0-9][a-zA-Z0-9._-]{0,127}`; throws InvalidShellArgError on anything else (including non-strings, shell metacharacters, command-substitution attempts). Tests cover all of these. - `dockerPrune(kind, {all?})` — kind constrained to {container,image,volume,builder}; `--all` only valid for image. - `runBashScript(path, args, {allowedRoots})` — script path AND cwd both checked against allowed roots; rejects `..` escapes and prefix-matching siblings (`/opt/projects-evil` vs `/opt/projects`). - `runNpmScript(script, {cwd, allowedRoots})` — script ∈ {typecheck,lint,build,test,test:run,start}; cwd inside roots. 17 unit tests cover every rejection path. Module added to the coverage gate (≥95% lines). Migrated highest-risk callers off template-literal `exec`: - `vm/repository.ts:restartContainer` → `dockerRestart`. Was previously `await execAsync(\`docker restart "${name}"\`)` with only a regex check; now goes through the wrapper. - `system/repository.ts:dockerCleanup` → `dockerPrune` per kind + `execAllowed` for `docker system df`. Drops the array of template-literal command strings entirely. - `code-quality/repository.ts` → `runNpmScript` for every lifecycle invocation. cwd is now the resolved (normalised, `..`-collapsed) path, not the raw input. 2. projectPath validation for /code-quality/check (P1) `runCodeQualityCheck` now calls `assertPathInAllowedRoots(projectPath, getAllowedRoots())` before any subprocess spawns. `getAllowedRoots()` reads `CODE_QUALITY_ALLOWED_ROOTS` (colon-separated env, defaults to `/opt/bytelyst`). Rejection happens with a clear error message listing the configured roots so operators know what to allow. 3. Audit-log every privileged shell-out (P2) `audit/types.ts` extended: `action` now includes `'shell-exec'`, `entityType` includes `'host'`. The migration is additive — old audit rows still validate. Three privileged routes now write a `shell-exec` audit row with actor (authUserId / authRole), entity id, and a sanitized details payload before responding: - `POST /docker/cleanup` — `entityId: docker-cleanup:`, details include {type, force, freedSpace}. - `POST /vm/cleanup` — `entityId: vm-cleanup:`. - `POST /vm/containers/:name/restart` — `entityId: container-restart:`, details include {success, message}. Audited even on failure so attempted privileged actions are still recorded. Audit writes are best-effort — a Cosmos hiccup logs a warn but never fails the request the operator was running. Verified: backend typecheck ✅, 74/74 unit tests ✅ (17 new for shell.ts + audit changes), 7/7 E2E ✅, lint 0 errors, coverage gate ≥95% lines on every gated file (which now includes shell.ts). Generated with [Devin](https://cli.devin.ai/docs) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> --- dashboard/DEPLOYMENT.md | 32 +++- dashboard/backend/src/lib/shell.test.ts | 165 +++++++++++++++++ dashboard/backend/src/lib/shell.ts | 170 ++++++++++++++++++ dashboard/backend/src/modules/audit/types.ts | 7 +- .../src/modules/code-quality/repository.ts | 143 ++++++++------- .../backend/src/modules/system/repository.ts | 65 +++---- .../backend/src/modules/system/routes.ts | 20 ++- .../backend/src/modules/vm/repository.ts | 19 +- dashboard/backend/src/modules/vm/routes.ts | 40 ++++- dashboard/backend/vitest.config.ts | 1 + 10 files changed, 545 insertions(+), 117 deletions(-) create mode 100644 dashboard/backend/src/lib/shell.test.ts create mode 100644 dashboard/backend/src/lib/shell.ts diff --git a/dashboard/DEPLOYMENT.md b/dashboard/DEPLOYMENT.md index 23549fd..d59a025 100644 --- a/dashboard/DEPLOYMENT.md +++ b/dashboard/DEPLOYMENT.md @@ -427,13 +427,31 @@ constructs `docker ...` shell strings directly with `execAsync`. ### Mitigation roadmap (incremental, not all at once) -- [ ] **P1 (next):** Allow-list wrapper around shell-outs. Centralize - `docker`/`bash`/`npm` invocations behind a small `lib/shell.ts` that - validates the arg vector against a per-command schema (e.g. - `docker.restart(name)` rejects names not matching `^[a-zA-Z0-9._-]{1,128}$`). -- [ ] **P1:** Validate `/code-quality/check`'s `projectPath` against a - configured set of allowed roots. -- [ ] **P2:** Audit-log every shell-out (command + arg vector + actor + result). +- [x] **P1:** Allow-list wrapper around shell-outs. *(`lib/shell.ts` ships with + `execAllowed` (no shell, just `execFile` with an explicit argv) plus + per-command helpers — `dockerRestart(name)` validates against + `[a-zA-Z0-9][a-zA-Z0-9._-]{0,127}`, `dockerPrune(kind, {all?})` validates + kind ∈ {container,image,volume,builder} and rejects `--all` on non-image, + `runBashScript(path, args, {allowedRoots})` and `runNpmScript(script, + {cwd, allowedRoots})` lock both the script path and cwd to a configured + set of roots. 17 unit tests cover the rejection paths; `vm/restartContainer` + and `system/dockerCleanup` migrated. Module covered by the test:coverage + gate (≥95% lines).)* +- [x] **P1:** Validate `/code-quality/check`'s `projectPath` against a + configured set of allowed roots. *(`runCodeQualityCheck` now calls + `assertPathInAllowedRoots(projectPath, getAllowedRoots())` before any + lifecycle script runs; `getAllowedRoots()` reads + `CODE_QUALITY_ALLOWED_ROOTS` (colon-separated) with a default of + `/opt/bytelyst`. The path is also re-resolved (normalised, `..` + collapsed) before being passed to `runNpmScript`, which lifts it to its + own argv slot — no shell interpolation.)* +- [x] **P2:** Audit-log every shell-out (command + arg vector + actor + result). + *(Audit schema extended with `action: 'shell-exec'` + `entityType: 'host'`. + `POST /docker/cleanup`, `POST /vm/cleanup`, `POST /vm/containers/:name/restart` + now write a Cosmos audit row including the actor (`authUserId`/`authRole`), + entity id (`docker-cleanup:` etc.), and a sanitized details payload. + Audit writes are best-effort — a Cosmos hiccup logs a warn but never + fails the request.)* - [ ] **P2:** Run the backend container as a non-root user with `docker` group membership; rebuild the Dockerfile accordingly. - [ ] **P3:** Move from `docker.sock` to a thin daemon (`docker-proxy`-style) diff --git a/dashboard/backend/src/lib/shell.test.ts b/dashboard/backend/src/lib/shell.test.ts new file mode 100644 index 0000000..7d1688a --- /dev/null +++ b/dashboard/backend/src/lib/shell.test.ts @@ -0,0 +1,165 @@ +import { describe, it, expect, beforeEach, vi } from 'vitest'; + +const execFileMock = vi.hoisted(() => vi.fn()); +vi.mock('child_process', () => ({ execFile: execFileMock })); + +const { + assertPathInAllowedRoots, + dockerPrune, + dockerRestart, + execAllowed, + InvalidShellArgError, + runBashScript, + runNpmScript, +} = await import('./shell.js'); + +function setExec(handler: (cmd: string, args: string[]) => { error?: Error; stdout?: string; stderr?: string }) { + execFileMock.mockImplementation( + ( + command: string, + args: string[], + _opts: unknown, + cb: (err: unknown, result?: { stdout: string; stderr: string }) => void, + ) => { + const res = handler(command, args); + if (res.error) cb(res.error); + else cb(null, { stdout: res.stdout ?? '', stderr: res.stderr ?? '' }); + }, + ); +} + +describe('execAllowed', () => { + beforeEach(() => vi.clearAllMocks()); + + it('passes argv through to execFile without a shell', async () => { + setExec(() => ({ stdout: 'ok' })); + const result = await execAllowed('docker', ['ps', '-a']); + expect(result.stdout).toBe('ok'); + expect(execFileMock).toHaveBeenCalledTimes(1); + const [cmd, args] = execFileMock.mock.calls[0]; + expect(cmd).toBe('docker'); + expect(args).toEqual(['ps', '-a']); + }); +}); + +describe('dockerRestart', () => { + beforeEach(() => vi.clearAllMocks()); + + it('rejects names with shell metacharacters before reaching execFile', async () => { + await expect(dockerRestart('foo; rm -rf /')).rejects.toBeInstanceOf(InvalidShellArgError); + await expect(dockerRestart('foo bar')).rejects.toBeInstanceOf(InvalidShellArgError); + await expect(dockerRestart('$(whoami)')).rejects.toBeInstanceOf(InvalidShellArgError); + await expect(dockerRestart('')).rejects.toBeInstanceOf(InvalidShellArgError); + expect(execFileMock).not.toHaveBeenCalled(); + }); + + it('accepts valid container names and forwards them as a single argv element', async () => { + setExec(() => ({ stdout: 'restarted' })); + await dockerRestart('hermes-gateway'); + const [, args] = execFileMock.mock.calls[0]; + // `restart` and the name are separate argv slots — never one + // concatenated string that could be re-parsed by a shell. + expect(args).toEqual(['restart', 'hermes-gateway']); + }); + + it('non-string input throws InvalidShellArgError', async () => { + // @ts-expect-error — testing runtime guard + await expect(dockerRestart(undefined)).rejects.toBeInstanceOf(InvalidShellArgError); + // @ts-expect-error — testing runtime guard + await expect(dockerRestart(123)).rejects.toBeInstanceOf(InvalidShellArgError); + }); +}); + +describe('dockerPrune', () => { + beforeEach(() => vi.clearAllMocks()); + + it('rejects unknown prune kinds', async () => { + // @ts-expect-error — exercising the runtime check + await expect(dockerPrune('everything')).rejects.toBeInstanceOf(InvalidShellArgError); + }); + + it('emits the documented argv per kind', async () => { + setExec(() => ({ stdout: '' })); + await dockerPrune('container'); + expect(execFileMock.mock.calls.at(-1)![1]).toEqual(['container', 'prune', '-f']); + await dockerPrune('image', { all: true }); + // `docker image prune -a -f` — kind first, then the verb, then -a/-f flags. + expect(execFileMock.mock.calls.at(-1)![1]).toEqual(['image', 'prune', '-a', '-f']); + await dockerPrune('volume'); + expect(execFileMock.mock.calls.at(-1)![1]).toEqual(['volume', 'prune', '-f']); + await dockerPrune('builder'); + expect(execFileMock.mock.calls.at(-1)![1]).toEqual(['builder', 'prune', '-f']); + }); + + it('rejects --all on non-image kinds', async () => { + await expect(dockerPrune('container', { all: true })).rejects.toBeInstanceOf(InvalidShellArgError); + }); +}); + +describe('assertPathInAllowedRoots', () => { + it('accepts paths inside an allowed root', () => { + expect(assertPathInAllowedRoots('/opt/projects/foo', ['/opt/projects'])).toBe('/opt/projects/foo'); + expect(assertPathInAllowedRoots('/opt/projects/foo/bar/baz', ['/opt/projects'])).toBe('/opt/projects/foo/bar/baz'); + expect(assertPathInAllowedRoots('/opt/projects', ['/opt/projects'])).toBe('/opt/projects'); + }); + + it('rejects relative paths', () => { + expect(() => assertPathInAllowedRoots('relative/path', ['/opt/projects'])).toThrow(InvalidShellArgError); + expect(() => assertPathInAllowedRoots('./foo', ['/opt/projects'])).toThrow(InvalidShellArgError); + }); + + it('rejects ../ escape attempts even when prefix-matching the root', () => { + expect(() => assertPathInAllowedRoots('/opt/projects/../etc', ['/opt/projects'])).toThrow(InvalidShellArgError); + expect(() => assertPathInAllowedRoots('/opt/projects/../../etc', ['/opt/projects'])).toThrow(InvalidShellArgError); + }); + + it('rejects sibling directories that share a prefix string', () => { + // /opt/projects-evil should NOT be accepted just because it starts with /opt/projects + expect(() => assertPathInAllowedRoots('/opt/projects-evil/foo', ['/opt/projects'])).toThrow(InvalidShellArgError); + }); + + it('checks every allowed root', () => { + expect(assertPathInAllowedRoots('/srv/app', ['/opt/projects', '/srv/app'])).toBe('/srv/app'); + }); +}); + +describe('runBashScript', () => { + beforeEach(() => vi.clearAllMocks()); + + it('rejects scripts outside allowed roots', async () => { + await expect(runBashScript('/etc/init.d/anything', [], { allowedRoots: ['/opt/projects'] })) + .rejects.toBeInstanceOf(InvalidShellArgError); + }); + + it('runs a script that is inside an allowed root', async () => { + setExec(() => ({ stdout: 'ok' })); + const result = await runBashScript('/opt/projects/deploy.sh', ['--prod'], { allowedRoots: ['/opt/projects'] }); + expect(result.stdout).toBe('ok'); + const [cmd, args] = execFileMock.mock.calls[0]; + expect(cmd).toBe('bash'); + expect(args).toEqual(['/opt/projects/deploy.sh', '--prod']); + }); +}); + +describe('runNpmScript', () => { + beforeEach(() => vi.clearAllMocks()); + + it('rejects npm scripts not in the lifecycle allow-list', async () => { + // @ts-expect-error — exercising the runtime guard + await expect(runNpmScript('publish', { allowedRoots: ['/opt/projects'], cwd: '/opt/projects/foo' })) + .rejects.toBeInstanceOf(InvalidShellArgError); + }); + + it('rejects cwd outside allowed roots', async () => { + await expect(runNpmScript('typecheck', { allowedRoots: ['/opt/projects'], cwd: '/etc' })) + .rejects.toBeInstanceOf(InvalidShellArgError); + }); + + it('runs a whitelisted lifecycle script in an allowed cwd', async () => { + setExec(() => ({ stdout: 'ok' })); + await runNpmScript('typecheck', { allowedRoots: ['/opt/projects'], cwd: '/opt/projects/foo' }); + const [cmd, args] = execFileMock.mock.calls[0]; + expect(cmd).toBe('npm'); + expect(args).toEqual(['run', 'typecheck']); + }); +}); diff --git a/dashboard/backend/src/lib/shell.ts b/dashboard/backend/src/lib/shell.ts new file mode 100644 index 0000000..32c4991 --- /dev/null +++ b/dashboard/backend/src/lib/shell.ts @@ -0,0 +1,170 @@ +// Allow-list wrapper around shell-outs. +// +// Every privileged route in this backend ultimately runs `docker`, `bash`, +// `npm`, etc. on the host. Historically those were issued as template-literal +// strings passed through `child_process.exec`, which means a misvalidated +// path param can become a shell-injection. This module fixes that by: +// +// 1. Always passing argv as a real array to `execFile` (no shell expansion, +// no string templating). `execAllowed()` is the only escape hatch and it +// still uses `execFile`, never `exec`. +// 2. Exposing per-command helpers (`dockerRestart`, `dockerPrune`, +// `runBashScript`, `runNpmScript`) that validate their inputs against +// a per-command allow-list regex. Repos call these instead of building +// `docker ...` strings directly. +// +// This is the "allow-list wrapper" item from the DEPLOYMENT.md privilege- +// surface mitigation roadmap. + +import { execFile } from 'child_process'; +import { isAbsolute, normalize, relative, resolve } from 'path'; +import { promisify } from 'util'; +import { childLogger } from './logger.js'; + +const execFileAsync = promisify(execFile); +const log = childLogger('lib/shell'); + +export interface ShellExecOptions { + cwd?: string; + timeoutMs?: number; + env?: NodeJS.ProcessEnv; +} + +export interface ShellResult { + stdout: string; + stderr: string; +} + +/** + * Run a single command with an explicit `argv` array. No shell expansion, + * no string interpolation. Prefer the per-command helpers below; reach for + * this when the command isn't on the allow-list yet. + */ +export async function execAllowed( + command: string, + args: string[], + options: ShellExecOptions = {}, +): Promise { + log.debug({ command, args, cwd: options.cwd }, 'shell exec'); + const { stdout, stderr } = await execFileAsync(command, args, { + cwd: options.cwd, + timeout: options.timeoutMs ?? 30_000, + env: options.env, + maxBuffer: 10 * 1024 * 1024, + }); + return { + stdout: stdout?.toString?.() ?? String(stdout ?? ''), + stderr: stderr?.toString?.() ?? String(stderr ?? ''), + }; +} + +// --- Docker allow-list ------------------------------------------------------ + +// Container/volume/image names from the docker daemon. Docker's own rule is +// `[a-zA-Z0-9][a-zA-Z0-9_.-]+` but we tighten the leading char too. +const CONTAINER_NAME_RE = /^[a-zA-Z0-9][a-zA-Z0-9._-]{0,127}$/; + +export class InvalidShellArgError extends Error { + constructor(message: string) { + super(message); + this.name = 'InvalidShellArgError'; + } +} + +function assertContainerName(name: string): void { + if (typeof name !== 'string' || !CONTAINER_NAME_RE.test(name)) { + throw new InvalidShellArgError(`Invalid container name: ${JSON.stringify(name)}`); + } +} + +/** `docker restart ` — validated. */ +export async function dockerRestart(name: string): Promise { + assertContainerName(name); + return execAllowed('docker', ['restart', name], { timeoutMs: 30_000 }); +} + +const PRUNE_KINDS = ['container', 'image', 'volume', 'builder'] as const; +export type PruneKind = typeof PRUNE_KINDS[number]; + +/** `docker prune -f` (`-a` only valid for `image`). */ +export async function dockerPrune(kind: PruneKind, opts: { all?: boolean } = {}): Promise { + if (!PRUNE_KINDS.includes(kind)) { + throw new InvalidShellArgError(`Invalid prune kind: ${JSON.stringify(kind)}`); + } + const args: string[] = [kind, 'prune', '-f']; + if (opts.all) { + if (kind !== 'image') throw new InvalidShellArgError('`all` is only valid for image prune'); + args.splice(2, 0, '-a'); + } + return execAllowed('docker', args, { timeoutMs: 60_000 }); +} + +// --- Filesystem-path allow-list -------------------------------------------- + +/** + * Verify that `candidate` is an absolute path that resolves inside one of + * the allowed roots. Used to lock down request-supplied `cwd` values + * (e.g. `/code-quality/check`'s `projectPath`) so callers can't run + * lifecycle scripts in arbitrary directories. + */ +export function assertPathInAllowedRoots(candidate: string, allowedRoots: string[]): string { + if (typeof candidate !== 'string' || !isAbsolute(candidate)) { + throw new InvalidShellArgError(`Path must be absolute: ${JSON.stringify(candidate)}`); + } + const resolved = resolve(normalize(candidate)); + for (const root of allowedRoots) { + const resolvedRoot = resolve(normalize(root)); + const rel = relative(resolvedRoot, resolved); + // Inside the root iff the relative path doesn't escape upward + // (no leading `..`) and isn't an absolute path back out. + if (rel === '' || (!rel.startsWith('..') && !isAbsolute(rel))) { + return resolved; + } + } + throw new InvalidShellArgError( + `Path is not inside an allowed root: ${JSON.stringify(candidate)}`, + ); +} + +// --- bash / npm wrappers ---------------------------------------------------- + +/** + * Run a `bash