chimurai / http-proxy-middleware

:zap: The one-liner node.js http-proxy middleware for connect, express, next.js and more
MIT License
10.7k stars 834 forks source link

Race condition for handleUpgrade when using websockets #818

Open jetibest opened 2 years ago

jetibest commented 2 years ago

Checks

Describe the bug (be clear and concise)

When having multiple proxies handling different paths, such as:

/a --> localhost:3001
/b --> localhost:3002
/c --> localhost:3003
/ --> localhost:3000

I found that if I rewrite the path of /b/some/path to /some/path, in half of the cases, it would inadvertedly also run the upgrade event for / --> localhost:3000.

Step-by-step reproduction instructions

I have no time to provide a simple reproducing example.

Expected behavior (be clear and concise)

Once a path is matched, no other upgrade should be handled anymore.

How is http-proxy-middleware used in your project?

http-proxy-middleware@2.0.6

What http-proxy-middleware configuration are you using?

See reproduction.

What OS/version and node/version are you seeing the problem?

Node v12.18.3

Additional context (optional)

I've solved this issue by inspecting and editing the code. In the this.handleUpgrade function, an extra flag should be added, to indicate this.shouldProxy returned true for a given path. This does assume that only one path can be matched, but that seems normal.

  private handleUpgrade = async (req: Request, socket, head) => {
    if (this.shouldProxy(this.proxyOptions.pathFilter, req)) {
      const activeProxyOptions = await this.prepareProxyRequest(req);
      this.proxy.ws(req, socket, head, activeProxyOptions);
      debug('server upgrade event received. Proxying WebSocket');
    }
  };

should be replaced by:

  private handleUpgrade = async (req: Request, socket, head) => {
    if (!socket.preparingProxyRequest && this.shouldProxy(this.proxyOptions.pathFilter, req)) {
      socket.preparingProxyRequest = true;
      const activeProxyOptions = await this.prepareProxyRequest(req);
      this.proxy.ws(req, socket, head, activeProxyOptions);
      debug('server upgrade event received. Proxying WebSocket');
    }
  };

The bug originates from the fact that the upgrade event is not awaited by the Server instance (NodeJS). But the prepareProxyRequest is asynchronous, meaning that with the right timing and rewriting of paths, potentially multiple handleUpgrade functions can be matched and handled.

See also: https://github.com/nodejs/node/issues/6339 Which - according to my knowledge - has not been solved.