Make Signaling Server more robust and secure

[chmurson]

Context

There is Signaling Server (in Node.js) that coordinates setup of WebRTC connection between two clients.

We have implemented it in span of two milestones, where priority was fast delivery of the working product.

Problem

To ensure longterm reliability of the server, few improvements should be implemented.

What

Make Signaling server more robust and secure.

What exactly ?

• Potential memory leaks when handling references of connections; MatchedSignalingConnectionsProvider, SignalingConnection, WebSocketJSONHandler
• Protection from DoS: connection limits; rate limiting, message size limit; ide connection timeout
• Additional improvements: CORS/Origin Validation, Enforce Secure Connections, Add Connection Metadata Tracking, Implement Proper Error Handling:
• Simple client-server authorization; via public token to filter out dumb internet connections

For more details look at Claude 4 Code Review below

Code Review: Signaling Server

I've analyzed your signaling server code and found several significant memory leaks and security vulnerabilities. Here's my detailed review:

Claude 4 Code Review full details

🔴 Critical Memory Leaks

1. MatchedSignalingConnectionsProvider - Memory Leak in ID Management

export class MatchedSignalingConnectionsProvider {
  private connections: WeakMap<{ id: string }, SignalingConnection> =
    new WeakMap();
  private ids: Set<{ id: string }> = new Set();

Issue: The ids Set holds strong references to objects, preventing garbage collection even after connections are removed. The WeakMap becomes ineffective.

Fix: Use a Map instead of WeakMap + Set combination:

private connections: Map<string, SignalingConnection> = new Map();

2. SignalingConnection - Circular Reference Memory Leak

  private _state:
    | undefined
    | { mode: "acceptsOffers"; offerSender?: SignalingConnection }
    | { mode: "sendsOffer"; matchedAcceptor: SignalingConnection };

Issue: SignalingConnections hold references to each other. When one closes, it doesn't clean up references in its paired connection.

Fix: Implement proper cleanup:

close() {
  if (this.state?.mode === "acceptsOffers" && this.state.offerSender) {
    // Notify the offer sender to clean up its reference
    this.state.offerSender.cleanupMatchedConnection();
  } else if (this.state?.mode === "sendsOffer") {
    // Notify the acceptor to clean up its reference
    this.state.matchedAcceptor.cleanupMatchedConnection();
  }
  this.state = undefined;
}

private cleanupMatchedConnection() {
  this.state = undefined;
}

3. WebSocketJSONHandler - Unbounded Listener Arrays

  private listeners: {
    onClose: ((code: number, reason: Buffer) => void)[];
    onError: ((error: Error) => void)[];
    onMessage: ((arg: object) => void)[];
    onSpecificMessage: {
      condition: (arg: object) => boolean;
      fn: (arg: object) => void;
    }[];
  } = {

Issue: No way to remove listeners, arrays can grow indefinitely.

Fix: Add cleanup method and return unsubscribe functions:

public onSpecificMessage<T extends object>(
  condition: (payload: object) => payload is T,
  fn: (payload: T) => void,
): () => void {
  const listener = { condition, fn: fn as (payload: object) => void };
  this.listeners.onSpecificMessage.push(listener);
  
  // Return unsubscribe function
  return () => {
    const index = this.listeners.onSpecificMessage.indexOf(listener);
    if (index > -1) {
      this.listeners.onSpecificMessage.splice(index, 1);
    }
  };
}

🔴 Critical Security Vulnerabilities

1. No Authentication or Authorization

console.warn(payload.sessionToken, "sessionToken is ignored for now");

Issue: Anyone can connect and establish WebRTC connections through your server.

Fix: Implement proper authentication:

// Add JWT or API key validation
private async validateSessionToken(token: string): Promise<boolean> {
  // Implement token validation
  return await tokenService.validate(token);
}

2. Denial of Service (DoS) Vulnerabilities

Issues:

• No connection limits
• No rate limiting
• No message size limits
• No idle connection timeout

Fix: Add protection mechanisms:

class SignalingServer {
  private readonly MAX_CONNECTIONS = 1000;
  private readonly MAX_MESSAGE_SIZE = 64 * 1024; // 64KB
  private readonly CONNECTION_TIMEOUT = 60000; // 60 seconds
  
  private setupWebSocketHandlers(): void {
    this.wss.on("connection", (ws: WebSocket, request) => {
      if (this.signalingConnections.length >= this.MAX_CONNECTIONS) {
        ws.close(1008, "Server at capacity");
        return;
      }
      
      // Set message size limit
      ws.setMaxPayloadSize(this.MAX_MESSAGE_SIZE);
      
      // Set idle timeout
      const timeout = setTimeout(() => {
        ws.close(1000, "Idle timeout");
      }, this.CONNECTION_TIMEOUT);
      
      // Reset timeout on activity
      ws.on("message", () => {
        clearTimeout(timeout);
        setTimeout(...);
      });
    });
  }
}

3. Input Validation Issues

  offer: unknown;
  iceCandidates: unknown[];

Issue: No validation of WebRTC offer/answer content, could lead to injection attacks.

Fix: Add proper validation:

function validateOffer(offer: unknown): boolean {
  if (!offer || typeof offer !== 'object') return false;
  // Add SDP validation
  if ('sdp' in offer && typeof offer.sdp === 'string') {
    // Validate SDP format and content
    return isValidSDP(offer.sdp);
  }
  return false;
}

4. Information Disclosure

console.log(
  `[SignalingServer] New signaling connection: ${signalingConnection.localID}; Total connections: ${this.signalingConnections.length}`,
);

Issue: Logs expose internal state and connection information.

Fix: Use structured logging with appropriate levels:

logger.debug('New connection established', { 
  connectionId: hash(signalingConnection.localID),
  timestamp: new Date().toISOString()
});

🟡 Additional Security Recommendations

1. Add CORS/Origin Validation:

this.wss.on("connection", (ws: WebSocket, request) => {
  const origin = request.headers.origin;
  if (!isAllowedOrigin(origin)) {
    ws.close(1008, "Origin not allowed");
    return;
  }
});

2. Enforce Secure Connections:

// Only accept WSS connections in production
if (process.env.NODE_ENV === 'production' && !request.secure) {
  ws.close(1008, "Secure connection required");
  return;
}

3. Add Connection Metadata Tracking:

interface ConnectionMetadata {
  id: string;
  connectedAt: Date;
  lastActivity: Date;
  messageCount: number;
  ipAddress: string;
}

4. Implement Proper Error Handling:

try {
  // Handle message
} catch (error) {
  logger.error('Message handling failed', { error: error.message });
  ws.close(1011, "Internal server error");
}

🟢 Positive Aspects

1. Good TypeScript usage with type guards
2. Clean separation of concerns
3. Proper use of event-driven architecture
4. Good code organization

Summary

The most critical issues to address immediately are:

1. Fix the memory leak in MatchedSignalingConnectionsProvider
2. Implement proper cleanup for circular references in SignalingConnection
3. Add authentication/authorization
4. Implement rate limiting and connection limits
5. Validate all user inputs properly

These fixes will significantly improve both the reliability and security of your signaling server.