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