security(relay): minor hardening follow-ups from PR review
- Uniform auth responses remove the session-enumeration oracle: unknown session and wrong password now return an identical 401 at the API layer and an identical WS handshake/close; desktop-status and the 503 "not connected" signal are only exposed after successful auth. - Session-count cap re-checked after bcrypt.hash so concurrent creates can't overshoot maxSessions. - Relay web client reconnect backoff resets only after a successful auth response (an accept-then-close server no longer defeats the backoff). - idGenerator comment corrected to match the rejection-sampling; drop unused recordFailure return value. - DEPLOY.md: document ALLOWED_ORIGINS for reverse-proxy deployments. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -80,6 +80,19 @@ Set these in your container configuration:
|
|||||||
- `DATA_DIR` - Data storage path (default: ./data)
|
- `DATA_DIR` - Data storage path (default: ./data)
|
||||||
- `NODE_ENV` - production or development
|
- `NODE_ENV` - production or development
|
||||||
- `LOG_LEVEL` - info, debug, error
|
- `LOG_LEVEL` - info, debug, error
|
||||||
|
- `ALLOWED_ORIGINS` - Comma-separated list of allowed browser origins for the
|
||||||
|
WebSocket Origin check (e.g. `https://macropad.example.com`). **Set this to
|
||||||
|
your public origin(s) when running behind a reverse proxy** (see note below).
|
||||||
|
|
||||||
|
> **Reverse proxy note (`ALLOWED_ORIGINS`):** When `ALLOWED_ORIGINS` is unset,
|
||||||
|
> the WebSocket upgrade Origin check falls back to comparing the browser's
|
||||||
|
> `Origin` host against the raw `Host` header seen by the app. Behind a reverse
|
||||||
|
> proxy the internal `Host` (e.g. `localhost:3000`) often differs from the
|
||||||
|
> public origin the browser sends (e.g. `https://macropad.example.com`), so
|
||||||
|
> legitimate browser WebSocket upgrades get **rejected**. Set `ALLOWED_ORIGINS`
|
||||||
|
> to your public origin(s) to fix this. (Forwarding the public host via
|
||||||
|
> `proxy_set_header Host $host;`, as in the Nginx example below, also helps, but
|
||||||
|
> setting `ALLOWED_ORIGINS` explicitly is the reliable fix.)
|
||||||
|
|
||||||
## Test It Works
|
## Test It Works
|
||||||
|
|
||||||
|
|||||||
@@ -10,6 +10,13 @@ class MacroPadApp {
|
|||||||
this.desktopConnected = false;
|
this.desktopConnected = false;
|
||||||
this.wsAuthenticated = false;
|
this.wsAuthenticated = false;
|
||||||
|
|
||||||
|
// Reconnect backoff. Reset only after a successful AUTH (not on
|
||||||
|
// socket open): a server that accepts the socket and then closes it
|
||||||
|
// would otherwise reset the backoff on every open and defeat it.
|
||||||
|
this.baseReconnectDelay = 3000;
|
||||||
|
this.maxReconnectDelay = 30000;
|
||||||
|
this.reconnectDelay = this.baseReconnectDelay;
|
||||||
|
|
||||||
// Get session ID from URL
|
// Get session ID from URL
|
||||||
const pathMatch = window.location.pathname.match(/^\/([a-zA-Z0-9]+)/);
|
const pathMatch = window.location.pathname.match(/^\/([a-zA-Z0-9]+)/);
|
||||||
this.sessionId = pathMatch ? pathMatch[1] : null;
|
this.sessionId = pathMatch ? pathMatch[1] : null;
|
||||||
@@ -117,7 +124,12 @@ class MacroPadApp {
|
|||||||
this.ws.onclose = () => {
|
this.ws.onclose = () => {
|
||||||
this.wsAuthenticated = false;
|
this.wsAuthenticated = false;
|
||||||
this.updateConnectionStatus(false);
|
this.updateConnectionStatus(false);
|
||||||
setTimeout(() => this.setupWebSocket(), 3000);
|
// Reconnect using the current backoff, then grow it (capped) for
|
||||||
|
// the next attempt. The backoff is only reset on a successful
|
||||||
|
// auth response, so accept-then-close cycles keep backing off.
|
||||||
|
const delay = this.reconnectDelay;
|
||||||
|
this.reconnectDelay = Math.min(this.reconnectDelay * 2, this.maxReconnectDelay);
|
||||||
|
setTimeout(() => this.setupWebSocket(), delay);
|
||||||
};
|
};
|
||||||
|
|
||||||
this.ws.onerror = () => this.updateConnectionStatus(false);
|
this.ws.onerror = () => this.updateConnectionStatus(false);
|
||||||
@@ -133,6 +145,9 @@ class MacroPadApp {
|
|||||||
case 'auth_response':
|
case 'auth_response':
|
||||||
if (data.success) {
|
if (data.success) {
|
||||||
this.wsAuthenticated = true;
|
this.wsAuthenticated = true;
|
||||||
|
// Reset the reconnect backoff only after a confirmed,
|
||||||
|
// successful auth (not merely on socket open).
|
||||||
|
this.reconnectDelay = this.baseReconnectDelay;
|
||||||
this.updateConnectionStatus(this.desktopConnected);
|
this.updateConnectionStatus(this.desktopConnected);
|
||||||
} else {
|
} else {
|
||||||
this.handleAuthError();
|
this.handleAuthError();
|
||||||
|
|||||||
@@ -12,26 +12,22 @@ export function createApiProxy(
|
|||||||
return async (req: Request, res: Response, next: NextFunction) => {
|
return async (req: Request, res: Response, next: NextFunction) => {
|
||||||
const sessionId = req.params.sessionId;
|
const sessionId = req.params.sessionId;
|
||||||
|
|
||||||
// Check session exists
|
|
||||||
const session = sessionManager.getSession(sessionId);
|
|
||||||
if (!session) {
|
|
||||||
return res.status(404).json({ error: 'Session not found' });
|
|
||||||
}
|
|
||||||
|
|
||||||
// Credentials must be supplied via the header (never the URL/query,
|
// Credentials must be supplied via the header (never the URL/query,
|
||||||
// which leaks into logs, history and referrers).
|
// which leaks into logs, history and referrers).
|
||||||
const password = req.headers['x-macropad-password'] as string;
|
const password = req.headers['x-macropad-password'] as string;
|
||||||
|
|
||||||
if (!password) {
|
// Uniform auth failure. A missing session, a missing password, and a
|
||||||
return res.status(401).json({ error: 'Password required' });
|
// wrong password all return the SAME status + generic message so a
|
||||||
|
// caller cannot infer whether a given session id exists (no
|
||||||
|
// enumeration oracle). validatePassword() safely returns false for an
|
||||||
|
// unknown session id.
|
||||||
|
if (!password || !(await sessionManager.validatePassword(sessionId, password))) {
|
||||||
|
return res.status(401).json({ error: 'Authentication failed' });
|
||||||
}
|
}
|
||||||
|
|
||||||
const valid = await sessionManager.validatePassword(sessionId, password);
|
// Only AFTER successful auth do we reveal desktop connectivity, so the
|
||||||
if (!valid) {
|
// 503 "not connected" signal cannot itself be used to probe which
|
||||||
return res.status(401).json({ error: 'Invalid password' });
|
// session ids exist.
|
||||||
}
|
|
||||||
|
|
||||||
// Check desktop is connected
|
|
||||||
const desktop = connectionManager.getDesktopBySessionId(sessionId);
|
const desktop = connectionManager.getDesktopBySessionId(sessionId);
|
||||||
if (!desktop) {
|
if (!desktop) {
|
||||||
return res.status(503).json({ error: 'Desktop not connected' });
|
return res.status(503).json({ error: 'Desktop not connected' });
|
||||||
|
|||||||
@@ -32,31 +32,19 @@ export function handleWebClientConnection(
|
|||||||
// client IP. This prevents one abusive IP (which knows the shareable session
|
// client IP. This prevents one abusive IP (which knows the shareable session
|
||||||
// id) from locking out other web clients or the desktop for this session.
|
// id) from locking out other web clients or the desktop for this session.
|
||||||
const throttleKey = `web:${sessionId}:${clientIp}`;
|
const throttleKey = `web:${sessionId}:${clientIp}`;
|
||||||
// Check if session exists
|
|
||||||
const session = sessionManager.getSession(sessionId);
|
|
||||||
if (!session) {
|
|
||||||
socket.send(JSON.stringify({
|
|
||||||
type: 'error',
|
|
||||||
error: 'Session not found'
|
|
||||||
}));
|
|
||||||
socket.close();
|
|
||||||
return;
|
|
||||||
}
|
|
||||||
|
|
||||||
// Add client (not authenticated yet)
|
// Do NOT reveal whether the session exists. A non-existent session id and
|
||||||
|
// an existing one must be indistinguishable to an unauthenticated client so
|
||||||
|
// session ids cannot be enumerated: we always run the same handshake and let
|
||||||
|
// the (uniform) auth response be the only signal. validatePassword() safely
|
||||||
|
// returns false for an unknown session id, and desktop connectivity is only
|
||||||
|
// disclosed AFTER a successful auth (see the 'auth' case below).
|
||||||
const client = connectionManager.addWebClient(sessionId, socket, false);
|
const client = connectionManager.addWebClient(sessionId, socket, false);
|
||||||
|
|
||||||
// Track failed auth attempts on this individual socket.
|
// Track failed auth attempts on this individual socket.
|
||||||
let socketAuthFailures = 0;
|
let socketAuthFailures = 0;
|
||||||
|
|
||||||
// Check if desktop is connected
|
// Request authentication.
|
||||||
const desktop = connectionManager.getDesktopBySessionId(sessionId);
|
|
||||||
socket.send(JSON.stringify({
|
|
||||||
type: 'desktop_status',
|
|
||||||
status: desktop ? 'connected' : 'disconnected'
|
|
||||||
}));
|
|
||||||
|
|
||||||
// Request authentication
|
|
||||||
socket.send(JSON.stringify({
|
socket.send(JSON.stringify({
|
||||||
type: 'auth_required'
|
type: 'auth_required'
|
||||||
}));
|
}));
|
||||||
@@ -76,6 +64,12 @@ export function handleWebClientConnection(
|
|||||||
}
|
}
|
||||||
} else {
|
} else {
|
||||||
socketAuthFailures = 0;
|
socketAuthFailures = 0;
|
||||||
|
// Authenticated: only now do we disclose desktop connectivity.
|
||||||
|
const desktop = connectionManager.getDesktopBySessionId(sessionId);
|
||||||
|
socket.send(JSON.stringify({
|
||||||
|
type: 'desktop_status',
|
||||||
|
status: desktop ? 'connected' : 'disconnected'
|
||||||
|
}));
|
||||||
}
|
}
|
||||||
break;
|
break;
|
||||||
}
|
}
|
||||||
@@ -142,7 +136,7 @@ async function handleAuth(
|
|||||||
socket.send(JSON.stringify({
|
socket.send(JSON.stringify({
|
||||||
type: 'auth_response',
|
type: 'auth_response',
|
||||||
success: false,
|
success: false,
|
||||||
error: 'Invalid password'
|
error: 'Authentication failed'
|
||||||
}));
|
}));
|
||||||
return true;
|
return true;
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -136,6 +136,21 @@ export class SessionManager {
|
|||||||
} while (this.sessions.has(id));
|
} while (this.sessions.has(id));
|
||||||
|
|
||||||
const passwordHash = await bcrypt.hash(password, config.bcryptRounds);
|
const passwordHash = await bcrypt.hash(password, config.bcryptRounds);
|
||||||
|
|
||||||
|
// Re-check the cap AFTER the (awaited) hash. bcrypt.hash is the only
|
||||||
|
// suspension point between the first cap check and the set() below, so
|
||||||
|
// concurrent createSession calls could each have passed the earlier check
|
||||||
|
// and then filled the map while we were hashing. Re-checking here — with
|
||||||
|
// no await between this check and the synchronous set() — makes the
|
||||||
|
// check-and-insert atomic under Node's single-threaded model, so the cap
|
||||||
|
// cannot be overshot.
|
||||||
|
if (this.sessions.size >= config.maxSessions) {
|
||||||
|
this.pruneExpired();
|
||||||
|
if (this.sessions.size >= config.maxSessions) {
|
||||||
|
throw new Error('Session limit reached');
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
const now = new Date().toISOString();
|
const now = new Date().toISOString();
|
||||||
|
|
||||||
const session: Session = {
|
const session: Session = {
|
||||||
|
|||||||
@@ -33,10 +33,12 @@ export function getLockoutRemaining(key: string): number {
|
|||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Record a failed auth attempt. Returns the lockout remaining (ms) after
|
* Record a failed auth attempt. Locks the key out for
|
||||||
* this failure is applied (0 if still not locked).
|
* `authLockoutDurationMs` once `authLockoutMaxAttempts` failures accumulate
|
||||||
|
* within `authLockoutWindowMs`. Callers query the resulting lockout via
|
||||||
|
* getLockoutRemaining(), so this returns nothing.
|
||||||
*/
|
*/
|
||||||
export function recordFailure(key: string): number {
|
export function recordFailure(key: string): void {
|
||||||
const now = Date.now();
|
const now = Date.now();
|
||||||
let rec = records.get(key);
|
let rec = records.get(key);
|
||||||
|
|
||||||
@@ -51,8 +53,6 @@ export function recordFailure(key: string): number {
|
|||||||
if (rec.count >= config.authLockoutMaxAttempts) {
|
if (rec.count >= config.authLockoutMaxAttempts) {
|
||||||
rec.lockedUntil = now + config.authLockoutDurationMs;
|
rec.lockedUntil = now + config.authLockoutDurationMs;
|
||||||
}
|
}
|
||||||
|
|
||||||
return rec.lockedUntil > now ? rec.lockedUntil - now : 0;
|
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
|
|||||||
@@ -18,7 +18,11 @@ export function generateSessionId(length: number = 12): string {
|
|||||||
let result = '';
|
let result = '';
|
||||||
|
|
||||||
while (result.length < length) {
|
while (result.length < length) {
|
||||||
// Over-fetch a little to reduce the number of syscalls when rejecting.
|
// Fetch exactly `length` random bytes per pass. Bytes at or above the
|
||||||
|
// rejection threshold are skipped to avoid modulo bias, so a pass may
|
||||||
|
// yield fewer than `length` characters; the outer while then runs another
|
||||||
|
// pass until we have enough. (No over-fetching — each pass draws exactly
|
||||||
|
// `length` bytes.)
|
||||||
const bytes = randomBytes(length);
|
const bytes = randomBytes(length);
|
||||||
for (let i = 0; i < bytes.length && result.length < length; i++) {
|
for (let i = 0; i < bytes.length && result.length < length; i++) {
|
||||||
const byte = bytes[i];
|
const byte = bytes[i];
|
||||||
|
|||||||
Reference in New Issue
Block a user