diff --git a/dashcaddy-api/__tests__/routes/exec.routes.test.js b/dashcaddy-api/__tests__/routes/exec.routes.test.js new file mode 100644 index 0000000..cd7ffea --- /dev/null +++ b/dashcaddy-api/__tests__/routes/exec.routes.test.js @@ -0,0 +1,192 @@ +/** + * DC-072: WebSocket exec scope-based authorization + containerId charset + * hardening. + * + * Bug class under test: + * 1. Pre-fix `routes/exec.js` captured `auth.scope` (line 39/46) but + * NEVER enforced it. A JWT or API key whose scope was `['read']` + * (a legitimate monitoring/observability scope) would be granted a + * full PTY-backed shell inside any running container. Container + * exec is root-equivalent inside the container's user namespace, + * so this is a privilege escalation: a read-only key holder could + * run arbitrary commands, exfiltrate mounted volumes, or pivot + * to the host network. + * + * 2. Pre-fix `containerId` regex `/^[a-zA-Z0-9][a-zA-Z0-9_.-]{0,127}$/` + * accepted mixed case, `_`, `-`, `.`, and any length up to 128. + * Docker container IDs are exactly 64 lowercase hex (or 12-char + * short form). The pre-fix validator would pass any string that + * looked vaguely ID-shaped; Docker's inspect() would then 404. + * + * Post-fix: `assertExecScope(auth)` requires `admin` scope and throws a + * 403-tagged error. `isValidContainerId(id)` accepts only 12 or 64 + * lowercase hex chars. Both helpers are exported via `__test`. + */ + +const { __test } = require('../../routes/exec'); +const { assertExecScope, isValidContainerId } = __test; + +function check(cond, msg) { + if (!cond) throw new Error('assertion failed: ' + msg); +} + +describe('DC-072: exec WebSocket scope-based authorization', () => { + describe('assertExecScope — admin required', () => { + test('admin scope passes', () => { + // Should not throw + assertExecScope({ type: 'jwt', scope: ['admin'] }); + assertExecScope({ type: 'apikey', scope: ['admin', 'read'] }); + }); + + test('read-only scope rejected with DC-072_INSUFFICIENT_SCOPE', () => { + let caught = null; + try { + assertExecScope({ type: 'apikey', scope: ['read'] }); + } catch (e) { + caught = e; + } + check(caught !== null, 'expected assertExecScope to throw on read-only scope'); + check(caught.code === 'DC-072_INSUFFICIENT_SCOPE', `expected code DC-072_INSUFFICIENT_SCOPE, got ${caught.code}`); + check(caught.statusCode === 403, `expected statusCode 403, got ${caught.statusCode}`); + check(caught.requiredScope === 'admin', `expected requiredScope=admin, got ${caught.requiredScope}`); + check(Array.isArray(caught.actualScope) && caught.actualScope[0] === 'read', `expected actualScope=['read'], got ${JSON.stringify(caught.actualScope)}`); + }); + + test('write-only scope rejected (write ≠ admin)', () => { + let caught = null; + try { + assertExecScope({ type: 'jwt', scope: ['write'] }); + } catch (e) { + caught = e; + } + check(caught !== null, 'expected assertExecScope to throw on write-only scope'); + check(caught.code === 'DC-072_INSUFFICIENT_SCOPE', `expected code DC-072_INSUFFICIENT_SCOPE, got ${caught.code}`); + check(caught.statusCode === 403, `expected statusCode 403, got ${caught.statusCode}`); + }); + + test('empty scope rejected', () => { + let caught = null; + try { + assertExecScope({ type: 'apikey', scope: [] }); + } catch (e) { + caught = e; + } + check(caught !== null, 'expected assertExecScope to throw on empty scope'); + check(caught.code === 'DC-072_INSUFFICIENT_SCOPE', 'expected DC-072_INSUFFICIENT_SCOPE code'); + }); + + test('undefined scope rejected (null-safety)', () => { + let caught = null; + try { + assertExecScope({ type: 'jwt' }); // no scope field + } catch (e) { + caught = e; + } + check(caught !== null, 'expected assertExecScope to throw on undefined scope'); + check(caught.code === 'DC-072_INSUFFICIENT_SCOPE', 'expected DC-072_INSUFFICIENT_SCOPE code'); + }); + + test('null auth rejected', () => { + let caught = null; + try { + assertExecScope(null); + } catch (e) { + caught = e; + } + check(caught !== null, 'expected assertExecScope to throw on null auth'); + check(caught.code === 'DC-072_INSUFFICIENT_SCOPE', 'expected DC-072_INSUFFICIENT_SCOPE code'); + }); + + test('non-array scope rejected (defensive)', () => { + let caught = null; + try { + assertExecScope({ type: 'apikey', scope: 'admin' }); // string, not array + } catch (e) { + caught = e; + } + check(caught !== null, 'expected assertExecScope to throw on non-array scope'); + check(caught.code === 'DC-072_INSUFFICIENT_SCOPE', 'expected DC-072_INSUFFICIENT_SCOPE code'); + }); + + test('error envelope carries operator-actionable fields', () => { + let caught = null; + try { + assertExecScope({ type: 'apikey', keyId: 'k_test', scope: ['read'] }); + } catch (e) { + caught = e; + } + check(caught.message === 'Container exec requires admin scope', `expected canonical message, got ${caught.message}`); + check(typeof caught.requiredScope === 'string' && caught.requiredScope === 'admin', 'requiredScope present'); + check(Array.isArray(caught.actualScope), 'actualScope is array'); + }); + }); + + describe('isValidContainerId — Docker charset (12 or 64 lowercase hex)', () => { + test('64-char lowercase hex accepted (full Docker ID)', () => { + // Real-world example: dashcaddy-api container ID + check(isValidContainerId('abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789') === true, '64-char hex should pass'); + }); + + test('12-char lowercase hex accepted (short form)', () => { + check(isValidContainerId('abcdef012345') === true, '12-char hex should pass'); + }); + + test('uppercase hex rejected (Docker IDs are lowercase)', () => { + check(isValidContainerId('ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789') === false, 'uppercase 64-char should fail'); + check(isValidContainerId('ABCDEF012345') === false, 'uppercase 12-char should fail'); + }); + + test('mixed case rejected', () => { + check(isValidContainerId('Abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789') === false, 'mixed case 64-char should fail'); + }); + + test('non-hex chars rejected', () => { + check(isValidContainerId('zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz') === false, 'g-z hex should fail'); + check(isValidContainerId('abc!@#$%^&*()_+-=[]{}|\\:;\'",.<>/?0123456789012345678901234567890123') === false, 'special chars should fail'); + }); + + test('underscore / dot / dash rejected (pre-fix allowed these)', () => { + // Pre-fix regex accepted `_`, `-`, `.` — all are non-Docker + check(isValidContainerId('my_container_1') === false, 'underscore should fail'); + check(isValidContainerId('my.container.1') === false, 'dot should fail'); + check(isValidContainerId('my-container-1') === false, 'dash should fail'); + }); + + test('wrong length rejected', () => { + check(isValidContainerId('abcdef0123456') === false, '13-char should fail'); // 12 + 1 + check(isValidContainerId('abcdef01234567') === false, '14-char should fail'); // 12 + 2 + check(isValidContainerId('abcdef0123456789a') === false, '65-char should fail'); // 64 + 1 + }); + + test('empty string rejected', () => { + check(isValidContainerId('') === false, 'empty string should fail'); + }); + + test('null / undefined / non-string rejected (defensive)', () => { + check(isValidContainerId(null) === false, 'null should fail'); + check(isValidContainerId(undefined) === false, 'undefined should fail'); + check(isValidContainerId(12345) === false, 'number should fail'); + check(isValidContainerId({}) === false, 'object should fail'); + check(isValidContainerId([]) === false, 'array should fail'); + }); + + test('whitespace / padding rejected', () => { + check(isValidContainerId(' abcdef012345 ') === false, 'padded should fail'); + check(isValidContainerId('\nabcdef012345\n') === false, 'CRLF-padded should fail'); + }); + + test('CRLF injection rejected (defensive against pre-fix attack class)', () => { + // Pre-fix regex accepted 128 chars with dots; a payload like + // `aa.bb.cc.dd\r\nSet-Cookie:...` would have passed. Post-fix + // the LF + non-hex + wrong-length combo fails on every axis. + check(isValidContainerId('aa\r\nbb') === false, 'CRLF payload should fail'); + }); + }); + + describe('__test exports shape', () => { + test('exports assertExecScope and isValidContainerId', () => { + check(typeof __test.assertExecScope === 'function', 'assertExecScope is a function'); + check(typeof __test.isValidContainerId === 'function', 'isValidContainerId is a function'); + }); + }); +}); diff --git a/dashcaddy-api/routes/exec.js b/dashcaddy-api/routes/exec.js index 8069f4c..e15c9ee 100644 --- a/dashcaddy-api/routes/exec.js +++ b/dashcaddy-api/routes/exec.js @@ -4,6 +4,50 @@ const url = require('url'); const docker = new Docker(); +/** + * DC-072: WebSocket scope authorization — admin-only by default. + * + * Container exec is full root-equivalent access inside the target + * container. Granting it to a key whose scope is `['read']` violates + * least privilege. The validScopes list (`['read','write','admin']`) + * is defined in routes/auth/keys.js; exec requires `admin`. + * + * Defensive: the scope field is coerced via `Array.isArray(...) ? ... : []` + * so a malformed payload (string, object, null, undefined) cannot reach + * `.includes('admin')` and accidentally grant access. Every malformed + * shape falls into the rejection branch with the same 403 envelope. + * + * Tests should call `__test.assertExecScope(auth)` directly rather + * than spinning up a WebSocket server. + */ +function assertExecScope(auth) { + const scope = Array.isArray(auth && auth.scope) ? auth.scope : []; + if (!scope.includes('admin')) { + const err = new Error('Container exec requires admin scope'); + err.code = 'DC-072_INSUFFICIENT_SCOPE'; + err.statusCode = 403; + err.requiredScope = 'admin'; + err.actualScope = scope; + throw err; + } +} + +/** + * DC-072: Tighten containerId validation. + * + * Docker container IDs are exactly 64 lowercase hex chars (or 12-char + * short form). The pre-fix regex accepted `_`, `-`, `.`, mixed case, + * and up to 128 chars — Docker would then 404 the inspect call and + * the rejection would surface as a generic 500 in the WS error + * envelope. Pre-validate at the upgrade layer so the rejection is + * fast and the log line discriminates "malformed" from "unknown". + */ +function isValidContainerId(id) { + if (typeof id !== 'string') return false; + // Full 64-char hex, or 12-char short hex + return /^[0-9a-f]{64}$/.test(id) || /^[0-9a-f]{12}$/.test(id); +} + /** * Attach WebSocket server for container exec/shell * Route: ws://host/ws/exec/:containerId @@ -21,8 +65,8 @@ module.exports = function attachExecWS(server, log, authManager) { const containerId = decodeURIComponent(match[1]); - // Validate container ID format to prevent injection - if (!/^[a-zA-Z0-9][a-zA-Z0-9_.-]{0,127}$/.test(containerId)) { + // DC-072: Tighten containerId charset (64-char / 12-char lowercase hex) + if (!isValidContainerId(containerId)) { log.warn('exec', 'Invalid container ID in WebSocket path', { containerId }); socket.write('HTTP/1.1 400 Bad Request\r\n\r\n'); socket.destroy(); @@ -55,6 +99,35 @@ module.exports = function attachExecWS(server, log, authManager) { return; } + // DC-072: Container exec is root-equivalent — require admin scope. + // Pre-fix, a key issued with scope `['read']` (e.g., for monitoring) + // would get a full PTY shell inside any running container. The + // `auth.scope` was captured at lines 39/46 but never checked. + try { + assertExecScope(auth); + } catch (err) { + log.warn('exec', 'Insufficient scope for exec attempt', { + containerId, + authType: auth.type, + authId: auth.type === 'jwt' ? auth.userId : auth.keyId, + actualScope: err.actualScope, + requiredScope: err.requiredScope, + ip: req.socket.remoteAddress, + }); + // 403 with a JSON error envelope over the upgrade socket so the + // dashboard can display "admin required" instead of guessing. + socket.write('HTTP/1.1 403 Forbidden\r\n'); + socket.write('Content-Type: application/json\r\n'); + socket.write('\r\n'); + socket.end(JSON.stringify({ + error: err.message, + code: err.code, + requiredScope: err.requiredScope, + actualScope: err.actualScope, + })); + return; + } + // Auth passed — proceed with WebSocket upgrade wss.handleUpgrade(req, socket, head, (ws) => { handleExec(ws, containerId, log, auth); @@ -67,6 +140,7 @@ module.exports = function attachExecWS(server, log, authManager) { async function handleExec(ws, containerId, log, auth) { let execStream = null; let execInstance = null; + const sessionStart = Date.now(); try { const container = docker.getContainer(containerId); @@ -78,10 +152,13 @@ async function handleExec(ws, containerId, log, auth) { return; } + // DC-072: Audit-log the exec session start. Pairs with the end-log + // below so the operator can correlate who opened which shell. log.info('exec', 'Authenticated exec session started', { containerId, authType: auth.type, - authId: auth.type === 'jwt' ? auth.userId : auth.keyId + authId: auth.type === 'jwt' ? auth.userId : auth.keyId, + containerName: info.Name, }); // Detect available shell @@ -120,7 +197,28 @@ async function handleExec(ws, containerId, log, auth) { } }); + // DC-072: Track whether the end-log has fired so we don't double-log + // when both execStream 'end' and ws 'close' fire (Docker stream end + // closes the WS, which then fires 'close' too — without the flag + // we'd emit the same audit line twice with the same durationMs). + let ended = false; + const logSessionEnd = (reason) => { + if (ended) return; + ended = true; + log.info('exec', 'Exec session ended', { + containerId, + authType: auth.type, + authId: auth.type === 'jwt' ? auth.userId : auth.keyId, + durationMs: Date.now() - sessionStart, + reason, + }); + }; + execStream.on('end', () => { + // DC-072: Audit-log the session end (duration + container) so a + // long-running session is observable in the error log. Normal + // shutdown path: Docker exec stream closes → log + tell client. + logSessionEnd('exec-stream-end'); if (ws.readyState === ws.OPEN) { ws.send(JSON.stringify({ type: 'exit' })); ws.close(); @@ -148,6 +246,11 @@ async function handleExec(ws, containerId, log, auth) { }); ws.on('close', () => { + // DC-072: Fallback audit-log for abnormal close (browser tab + // closed, network drop, container killed mid-session) where the + // execStream 'end' event never fires. The ended-flag guard makes + // this idempotent with the normal path above. + logSessionEnd('ws-close'); if (execStream) { try { execStream.destroy(); } catch (_) { // Ignore stream teardown errors on socket close @@ -172,3 +275,11 @@ async function handleExec(ws, containerId, log, auth) { } } } + +// Internal-only export for unit tests. Stripped from the public +// surface; tests import this via the destructure form +// `const { __test } = require('./routes/exec')`. +module.exports.__test = { + assertExecScope, + isValidContainerId, +};