Skip to content

Race condition for handleUpgrade when using websockets #818

Description

@jetibest

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: nodejs/node#6339
Which - according to my knowledge - has not been solved.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions