Merge dc/DC-072-exec-scope: DC-072 exec scope + containerId hardening (glm-grade=A)

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');
+    });
+  });
+});

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
+    // DC-072: Tighten containerId charset (64-char / 12-char lowercase hex)
-    if (!/^[a-zA-Z0-9][a-zA-Z0-9_.-]{0,127}$/.test(containerId)) {
+    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,
+};