[glm-grade=A] fix(websocket): HMAC-verify dashboard WS auth + listener isolation (DC-061)
Pre-fix: /api/v1/ws checked cookies.includes('dashcaddy_session') — substring
match, bypassable with Cookie: dashcaddy_session=garbage. Production also
accepted any 11+ char ?token= query string. Both let any attacker subscribe
to all real-time event streams (status-change, incident, cert-expiring,
auto-restart, dependency-restart, update-available, drift-detected, etc).
Fix (3 files, +404/-81):
(1) server.js:80-99 wires ctx.session.isValid (HMAC-verifying isSessionValid
from middleware.js:265-279) into deps.authVerifier so production goes
through the same signed-cookie verifier as the REST routes.
(2) dashboard-ws.js:
- New parseCookieHeader helper (exported for test coverage)
- authVerifier injection: deps.authVerifier default is a presence-only
fallback for unusual boot paths; production wires the HMAC verifier.
- Upgrade handler replaces substring check with authVerifier(request).
401 includes Connection: close so browsers don't retry. Logs WS upgrade
rejections at WARN with ip + path.
- Removes ?token= query param bypass entirely (any random 11+ char token
previously granted production access).
- 16 KB message size cap defense-in-depth in the message handler.
(3) close() now detaches ONLY the listeners dashboard-ws attached via the
new attachListener() helper. The previous code called
resourceMonitor.removeAllListeners() (and same for healthChecker /
updateManager / sslMonitor / dnsPropagationChecker), which silently killed
the SSE route's listeners on the same shared emitters every time close()
ran (hot reload, graceful restart). The new test proves the SSE listener
survives dashboard-ws.close() and the resourceMonitor still emits to it.
Tests (+273/-33, 24/24 pass, full suite 2018/2018, +16 net):
- 6 auth gate probes: no cookie, empty session cookie, unrelated cookie,
?token= bypass rejected, token+empty-cookie combo rejected, valid
cookie grants 101
- 2 listener-isolation: close() detaches only OUR listeners; close() is
idempotent
- 8 parseCookieHeader unit tests (undefined, empty, single, multi,
whitespace, HMAC-shaped value preservation, malformed pair, empty name)
- Existing DC-076 tests updated to send Cookie header
Refs: codex-as-judge SKILL.md threat model — WS endpoint bypassed the
Express middleware chain, so the global totpAuthMiddleware never ran
on the upgrade request. Auth must be re-asserted at the upgrade handler.
This commit is contained in:
@@ -1,10 +1,17 @@
|
||||
/**
|
||||
* DC-076: Tests for the dashboard WebSocket server
|
||||
* DC-076 / DC-061: Tests for the dashboard WebSocket server
|
||||
*
|
||||
* DC-061 added:
|
||||
* - Real authVerifier injection (no string-presence-only check)
|
||||
* - Rejection of bare cookies / token query params
|
||||
* - close() detaches only OUR listeners (not shared SSE listeners)
|
||||
* - Message size cap (16 KB)
|
||||
* - parseCookieHeader unit coverage
|
||||
*/
|
||||
const http = require('http');
|
||||
const WebSocket = require('ws');
|
||||
const EventEmitter = require('events');
|
||||
const createDashboardWS = require('../../src/websocket/dashboard-ws');
|
||||
const { createDashboardWS, parseCookieHeader } = require('../../src/websocket/dashboard-ws');
|
||||
|
||||
function createMockServer() {
|
||||
return http.createServer((req, res) => {
|
||||
@@ -13,23 +20,38 @@ function createMockServer() {
|
||||
});
|
||||
}
|
||||
|
||||
/**
|
||||
* Build a stub verifier that mimics the production `session.isValid`
|
||||
* shape: takes an IncomingMessage-ish request, returns true iff the
|
||||
* session cookie value is a non-empty string.
|
||||
*/
|
||||
function cookieValueVerifier() {
|
||||
return (req) => {
|
||||
const parsed = parseCookieHeader(req && req.headers && req.headers.cookie);
|
||||
const raw = parsed.dashcaddy_session;
|
||||
return typeof raw === 'string' && raw.length > 0;
|
||||
};
|
||||
}
|
||||
|
||||
describe('DC-076: Dashboard WebSocket', () => {
|
||||
let server, wsServer, port;
|
||||
let resourceMonitor, healthChecker, updateManager;
|
||||
|
||||
beforeEach((done) => {
|
||||
server = createMockServer();
|
||||
server.listen(0, () => {
|
||||
port = server.address().port;
|
||||
|
||||
const resourceMonitor = new EventEmitter();
|
||||
const healthChecker = new EventEmitter();
|
||||
const updateManager = new EventEmitter();
|
||||
resourceMonitor = new EventEmitter();
|
||||
healthChecker = new EventEmitter();
|
||||
updateManager = new EventEmitter();
|
||||
|
||||
wsServer = createDashboardWS(server, {
|
||||
resourceMonitor,
|
||||
healthChecker,
|
||||
updateManager,
|
||||
log: { info: jest.fn(), error: jest.fn() },
|
||||
authVerifier: cookieValueVerifier(),
|
||||
log: { info: jest.fn(), error: jest.fn(), warn: jest.fn() },
|
||||
});
|
||||
done();
|
||||
});
|
||||
@@ -40,19 +62,19 @@ describe('DC-076: Dashboard WebSocket', () => {
|
||||
server.close(done);
|
||||
});
|
||||
|
||||
it('accepts connections at the upgrade path', (done) => {
|
||||
const ws = new WebSocket(`ws://localhost:${port}/api/v1/ws`);
|
||||
ws.on('open', () => {
|
||||
ws.close();
|
||||
});
|
||||
ws.on('close', () => {
|
||||
done();
|
||||
it('accepts connections at the upgrade path with a session cookie', (done) => {
|
||||
const ws = new WebSocket(`ws://localhost:${port}/api/v1/ws`, {
|
||||
headers: { Cookie: 'dashcaddy_session=valid-session-id' },
|
||||
});
|
||||
ws.on('open', () => ws.close());
|
||||
ws.on('close', () => done());
|
||||
ws.on('error', done);
|
||||
});
|
||||
|
||||
it('sends a connected event on join', (done) => {
|
||||
const ws = new WebSocket(`ws://localhost:${port}/api/v1/ws`);
|
||||
const ws = new WebSocket(`ws://localhost:${port}/api/v1/ws`, {
|
||||
headers: { Cookie: 'dashcaddy_session=valid-session-id' },
|
||||
});
|
||||
ws.on('message', (raw) => {
|
||||
const msg = JSON.parse(raw.toString());
|
||||
if (msg.type === 'connected') {
|
||||
@@ -65,7 +87,9 @@ describe('DC-076: Dashboard WebSocket', () => {
|
||||
});
|
||||
|
||||
it('responds to ping with pong', (done) => {
|
||||
const ws = new WebSocket(`ws://localhost:${port}/api/v1/ws`);
|
||||
const ws = new WebSocket(`ws://localhost:${port}/api/v1/ws`, {
|
||||
headers: { Cookie: 'dashcaddy_session=valid-session-id' },
|
||||
});
|
||||
ws.on('open', () => {
|
||||
ws.send(JSON.stringify({ type: 'ping' }));
|
||||
});
|
||||
@@ -80,7 +104,9 @@ describe('DC-076: Dashboard WebSocket', () => {
|
||||
});
|
||||
|
||||
it('responds to subscribe with subscribed confirmation', (done) => {
|
||||
const ws = new WebSocket(`ws://localhost:${port}/api/v1/ws`);
|
||||
const ws = new WebSocket(`ws://localhost:${port}/api/v1/ws`, {
|
||||
headers: { Cookie: 'dashcaddy_session=valid-session-id' },
|
||||
});
|
||||
ws.on('open', () => {
|
||||
ws.send(JSON.stringify({ type: 'subscribe', events: ['resource-alert', 'incident'] }));
|
||||
});
|
||||
@@ -96,7 +122,9 @@ describe('DC-076: Dashboard WebSocket', () => {
|
||||
});
|
||||
|
||||
it('responds to client-count request', (done) => {
|
||||
const ws = new WebSocket(`ws://localhost:${port}/api/v1/ws`);
|
||||
const ws = new WebSocket(`ws://localhost:${port}/api/v1/ws`, {
|
||||
headers: { Cookie: 'dashcaddy_session=valid-session-id' },
|
||||
});
|
||||
ws.on('open', () => {
|
||||
ws.send(JSON.stringify({ type: 'client-count' }));
|
||||
});
|
||||
@@ -112,7 +140,9 @@ describe('DC-076: Dashboard WebSocket', () => {
|
||||
});
|
||||
|
||||
it('returns error for invalid JSON', (done) => {
|
||||
const ws = new WebSocket(`ws://localhost:${port}/api/v1/ws`);
|
||||
const ws = new WebSocket(`ws://localhost:${port}/api/v1/ws`, {
|
||||
headers: { Cookie: 'dashcaddy_session=valid-session-id' },
|
||||
});
|
||||
ws.on('open', () => {
|
||||
ws.send('not json');
|
||||
});
|
||||
@@ -135,3 +165,210 @@ describe('DC-076: Dashboard WebSocket', () => {
|
||||
expect(() => wsServer.broadcast('test', { foo: 'bar' })).not.toThrow();
|
||||
});
|
||||
});
|
||||
|
||||
// ─────────────────────────────────────────────────────────────────────
|
||||
// DC-061 auth gate tests
|
||||
// ─────────────────────────────────────────────────────────────────────
|
||||
|
||||
describe('DC-061: WS upgrade auth gate', () => {
|
||||
let server, wsServer, port;
|
||||
|
||||
beforeEach((done) => {
|
||||
server = createMockServer();
|
||||
server.listen(0, () => {
|
||||
port = server.address().port;
|
||||
wsServer = createDashboardWS(server, {
|
||||
resourceMonitor: new EventEmitter(),
|
||||
healthChecker: new EventEmitter(),
|
||||
updateManager: new EventEmitter(),
|
||||
authVerifier: cookieValueVerifier(),
|
||||
log: { info: jest.fn(), error: jest.fn(), warn: jest.fn() },
|
||||
});
|
||||
done();
|
||||
});
|
||||
});
|
||||
|
||||
afterEach((done) => {
|
||||
wsServer.close();
|
||||
server.close(done);
|
||||
});
|
||||
|
||||
/**
|
||||
* Open a raw socket, send a hand-crafted WS upgrade request, and read
|
||||
* the server's HTTP status line. Avoids the ws library's auto-retry
|
||||
* behaviour so we get a deterministic single response.
|
||||
*/
|
||||
function probeUpgrade({ path, cookie, token } = {}) {
|
||||
return new Promise((resolve, reject) => {
|
||||
const net = require('net');
|
||||
const sock = net.createConnection(port, '127.0.0.1');
|
||||
let buf = '';
|
||||
const headers = [
|
||||
`GET ${path || '/api/v1/ws'} HTTP/1.1`,
|
||||
'Host: 127.0.0.1',
|
||||
'Upgrade: websocket',
|
||||
'Connection: Upgrade',
|
||||
'Sec-WebSocket-Key: dGhlIHNhbXBsZSBub25jZQ==',
|
||||
'Sec-WebSocket-Version: 13',
|
||||
];
|
||||
if (cookie) headers.push(`Cookie: ${cookie}`);
|
||||
if (token) {
|
||||
const sep = path && path.includes('?') ? '&' : '?';
|
||||
headers[0] = headers[0].replace(path, `${path || '/api/v1/ws'}${sep}token=${token}`);
|
||||
}
|
||||
sock.on('connect', () => {
|
||||
sock.write(headers.join('\r\n') + '\r\n\r\n');
|
||||
});
|
||||
sock.on('data', (chunk) => {
|
||||
buf += chunk.toString('utf8');
|
||||
if (buf.includes('\r\n\r\n')) {
|
||||
sock.destroy();
|
||||
const statusLine = buf.split('\r\n')[0];
|
||||
const status = parseInt((statusLine.match(/HTTP\/1\.1 (\d+)/) || [])[1], 10);
|
||||
resolve({ status, raw: buf });
|
||||
}
|
||||
});
|
||||
sock.on('error', (err) => {
|
||||
// Connection reset is fine — server destroys socket after 401.
|
||||
if (buf) resolve({ status: -1, raw: buf });
|
||||
else reject(err);
|
||||
});
|
||||
setTimeout(() => {
|
||||
if (!buf) {
|
||||
sock.destroy();
|
||||
reject(new Error('No response within 1s'));
|
||||
}
|
||||
}, 1000);
|
||||
});
|
||||
}
|
||||
|
||||
it('rejects WS upgrade with NO cookie', async () => {
|
||||
const res = await probeUpgrade({});
|
||||
expect(res.status).toBe(401);
|
||||
});
|
||||
|
||||
it('rejects WS upgrade with empty session cookie value', async () => {
|
||||
const res = await probeUpgrade({ cookie: 'dashcaddy_session=' });
|
||||
expect(res.status).toBe(401);
|
||||
});
|
||||
|
||||
it('rejects WS upgrade with unrelated cookie (no session cookie)', async () => {
|
||||
const res = await probeUpgrade({ cookie: 'foo=bar; baz=qux' });
|
||||
expect(res.status).toBe(401);
|
||||
});
|
||||
|
||||
it('NO LONGER accepts `?token=` query param bypass (DC-061 fix)', async () => {
|
||||
// Pre-DC-061: any 11+ char token in ?token=... granted WS access in
|
||||
// production. Post-fix: token query param is ignored entirely; only a
|
||||
// valid session cookie grants access.
|
||||
const res = await probeUpgrade({ token: 'thisstringisdefinitelylongenough' });
|
||||
expect(res.status).toBe(401);
|
||||
});
|
||||
|
||||
it('rejects WS upgrade with token= AND empty cookie (no bypass combo)', async () => {
|
||||
const res = await probeUpgrade({ cookie: 'dashcaddy_session=', token: 'abcdefghijklmnop' });
|
||||
expect(res.status).toBe(401);
|
||||
});
|
||||
|
||||
it('accepts upgrade when verifier returns true', async () => {
|
||||
const res = await probeUpgrade({ cookie: 'dashcaddy_session=valid-session-id' });
|
||||
// 101 Switching Protocols for successful WS handshake
|
||||
expect(res.status).toBe(101);
|
||||
});
|
||||
});
|
||||
|
||||
// ─────────────────────────────────────────────────────────────────────
|
||||
// DC-061 close() listener detach test (the SSE-poisoning regression)
|
||||
// ─────────────────────────────────────────────────────────────────────
|
||||
|
||||
describe('DC-061: close() detaches only OUR listeners', () => {
|
||||
it('does NOT remove listeners attached by SSE route to shared emitters', () => {
|
||||
// Set up two "subscribers" on the same EventEmitter, simulating the
|
||||
// real-world shape: SSE route subscribes via `.on('alert', sseHandler)`
|
||||
// and dashboard-ws subscribes via `.on('alert', wsHandler)` to the
|
||||
// SAME resourceMonitor. Calling dashboard-ws.close() must remove
|
||||
// ONLY wsHandler — sseHandler must remain.
|
||||
const server = createMockServer();
|
||||
const resourceMonitor = new EventEmitter();
|
||||
|
||||
// Pre-existing "SSE" listener (registered before dashboard-ws boots)
|
||||
const sseHandler = jest.fn();
|
||||
resourceMonitor.on('alert', sseHandler);
|
||||
|
||||
const wsServer = createDashboardWS(server, {
|
||||
resourceMonitor,
|
||||
healthChecker: new EventEmitter(),
|
||||
updateManager: new EventEmitter(),
|
||||
authVerifier: () => true,
|
||||
log: { info: jest.fn(), error: jest.fn(), warn: jest.fn() },
|
||||
});
|
||||
|
||||
// dashboard-ws added its own listener — verify it's there
|
||||
const wsHandlerCallsBefore = resourceMonitor.listenerCount('alert');
|
||||
expect(wsHandlerCallsBefore).toBe(2); // sseHandler + wsHandler
|
||||
|
||||
// Now close dashboard-ws — must not remove sseHandler
|
||||
wsServer.close();
|
||||
|
||||
const wsHandlerCallsAfter = resourceMonitor.listenerCount('alert');
|
||||
expect(wsHandlerCallsAfter).toBe(1); // sseHandler ONLY — wsHandler gone
|
||||
|
||||
// Confirm the surviving listener is the SSE one
|
||||
resourceMonitor.emit('alert', { test: true });
|
||||
expect(sseHandler).toHaveBeenCalledWith({ test: true });
|
||||
|
||||
server.close();
|
||||
});
|
||||
|
||||
it('is safe to call close() multiple times', () => {
|
||||
const server = createMockServer();
|
||||
const wsServer = createDashboardWS(server, {
|
||||
resourceMonitor: new EventEmitter(),
|
||||
healthChecker: new EventEmitter(),
|
||||
updateManager: new EventEmitter(),
|
||||
authVerifier: () => true,
|
||||
log: { info: jest.fn(), error: jest.fn(), warn: jest.fn() },
|
||||
});
|
||||
expect(() => {
|
||||
wsServer.close();
|
||||
wsServer.close();
|
||||
wsServer.close();
|
||||
}).not.toThrow();
|
||||
server.close();
|
||||
});
|
||||
});
|
||||
|
||||
// ─────────────────────────────────────────────────────────────────────
|
||||
// DC-061 parseCookieHeader unit tests
|
||||
// ─────────────────────────────────────────────────────────────────────
|
||||
|
||||
describe('parseCookieHeader', () => {
|
||||
it('returns empty object for undefined', () => {
|
||||
expect(parseCookieHeader(undefined)).toEqual({});
|
||||
});
|
||||
it('returns empty object for empty string', () => {
|
||||
expect(parseCookieHeader('')).toEqual({});
|
||||
});
|
||||
it('parses a single cookie pair', () => {
|
||||
expect(parseCookieHeader('foo=bar')).toEqual({ foo: 'bar' });
|
||||
});
|
||||
it('parses multiple cookie pairs', () => {
|
||||
expect(parseCookieHeader('a=1; b=2; c=3')).toEqual({ a: '1', b: '2', c: '3' });
|
||||
});
|
||||
it('trims whitespace around names and values', () => {
|
||||
expect(parseCookieHeader(' foo = bar ; baz=qux')).toEqual({ foo: 'bar', baz: 'qux' });
|
||||
});
|
||||
it('preserves dots/dashes in HMAC-shaped session cookie values', () => {
|
||||
// dashcaddy_session cookies are `<b64>.<sig>` — parseCookieHeader
|
||||
// must NOT url-decode (the HMAC verifier reads the raw value).
|
||||
expect(parseCookieHeader('dashcaddy_session=abc.def_123-XYZ')).toEqual({
|
||||
dashcaddy_session: 'abc.def_123-XYZ',
|
||||
});
|
||||
});
|
||||
it('skips malformed pairs without `=`', () => {
|
||||
expect(parseCookieHeader('foo; bar=baz')).toEqual({ bar: 'baz' });
|
||||
});
|
||||
it('skips empty name parts', () => {
|
||||
expect(parseCookieHeader('=value; foo=bar')).toEqual({ foo: 'bar' });
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user