Skip to content

Conversation

minrk
Copy link
Contributor

@minrk minrk commented Sep 19, 2025

relay response status and body back to requester

Honestly, the most relevant part of this is just closing the socket on the response event, because browser websockets don't really handle HTTP errors at all, they are all just treated as "not a websocket".

But in this PR, the status and body are proxied back. It doesn't go through the full behavior of proxy.web (notably: headers are not preserved). I couldn't figure out the easiest way to properly preserve proxy passes from web-outgoing since we don't have a res object. If a res can be accessed and/or mocked so that the web-outgoing passes can be applied as-is, that should be fully proxied. In the absence of correctly proxied headers, I'd also be okay discarding the body and just proxying the status line with Connection: close.

closes #26

relay response status and body back to requester
@minrk
Copy link
Contributor Author

minrk commented Sep 20, 2025

Looks like this was removed in the typescript refactor.

The code exactly as-is from node-http-proxy doesn't work anymore due to mishandling of transfer-encoding: chunked in proxyRes.pipe(socket), I believe.

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds functionality to handle WebSocket upgrade requests that are made to non-WebSocket backends. When a WebSocket connection attempt is made to an HTTP-only endpoint, the proxy now relays the HTTP response status and body back to the client before closing the connection.

  • Added response event handler in the WebSocket proxy to detect non-WebSocket responses
  • Implemented HTTP response forwarding with status code and body preservation
  • Added comprehensive test coverage for WebSocket-to-HTTP scenarios

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
lib/test/websocket/websocket-proxy-http.test.ts New test file covering WebSocket client connections to non-WebSocket backends
lib/http-proxy/passes/ws-incoming.ts Added response event handler to relay HTTP responses when backend doesn't support WebSockets

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@williamstein
Copy link
Contributor

The code exactly as-is from node-http-proxy doesn't work anymore due to mishandling of transfer-encoding: chunked in proxyRes.pipe(socket), I believe.

I didn't quite follow how this relates to the rest of your PR.

@minrk
Copy link
Contributor Author

minrk commented Sep 20, 2025

I didn't quite follow how this relates to the rest of your PR.

Just that one solution is to put back exactly what was there instead of what I did here, but it leads to:

Serialized Error: { bytesParsed: 167, code: 'HPE_INVALID_CHUNK_SIZE', reason: 'Invalid character in chunk size', rawPacket: '<Buffer(183) ...>' }

applies full web-outgoing passes

needs to declare an EditableResponse subset of Response
in order to mock it for the proxy response
@minrk
Copy link
Contributor Author

minrk commented Sep 20, 2025

I figured out a way to fully proxy the response (unlike node-http-proxy, which didn't put response headers through the web-outgoing passes). I don't fully understand the transfer-encoding problem. Removing the transfer-encoding header from the response avoids an error. I suspect it has to do with using proxyRes.pipe(socket) instead of proxyRes.pipe(res) which should handle transfer encoding? I don't know. Or maybe the header is wrong.

@minrk
Copy link
Contributor Author

minrk commented Sep 20, 2025

ok, I understand transfer-encoding better now - since it's hop-by-hop, the outgoing body shouldn't have any relation to the transfer-encoding of the proxied request, so removing this header is the right thing to do (always, not just for this websocket case).

I also realize this issue: http-party/node-http-proxy#1488 is still unaddressed and that there are other headers that should likely be unconditionally rewritten (or just removed) in web-outgoing, but that's another PR:

  • Connection
  • Keep-Alive
  • Proxy-Authenticate
  • Proxy-Authorization
  • TE
  • Trailers
  • Transfer-Encoding
  • Upgrade

- proxied response transfer-encoding is never relevant for the client response
- need to reimplement transfer-encoding when proxying directly to the socket instead of the Response object
@minrk
Copy link
Contributor Author

minrk commented Sep 20, 2025

opened #28 for the hop-by-hop headers in general, but fixed transfer-encoding here because it's actually needed when writing directly to a socket instead of to a Response, which implements the transfer encoding itself.

@williamstein
Copy link
Contributor

OK, this looks good to me.

@williamstein williamstein merged commit ba5f091 into sagemathinc:main Sep 25, 2025
3 checks passed
@minrk minrk deleted the ws-to-http branch September 25, 2025 05:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

websocket connection to non-websocket backend doesn't close

2 participants