fix(websocket): handle errors in handleUpgrade #823
Conversation
| function isSocketLike(obj: any): obj is Socket { | ||
| return obj && typeof obj.write === 'function' && !('writeHead' in obj); | ||
| } |
There was a problem hiding this comment.
We can't just check for instanceof Socket since per the documentation, the user may be able to provide a different socket implementation:
This event is guaranteed to be passed an instance of the <net.Socket> class, a subclass of <stream.Duplex>, unless the user specifies a socket type other than <net.Socket>.
| @@ -97,15 +97,14 @@ describe('E2E WebSocket proxy', () => { | |||
|
|
|||
| describe('with router and pathRewrite', () => { | |||
| beforeEach(() => { | |||
| // override | |||
| proxyServer = createApp( | |||
| const proxyMiddleware = createProxyMiddleware({ | |||
There was a problem hiding this comment.
This was a bug with the existing tests: because it didn't reassign proxyMiddleware, the proxyMiddleware.upgrade access a few lines after this was reading from the wrong object.
|
@chimurai 👋 can I do anything to help this along? |
|
Sorry for the late reply. I'll need some time to review 🙏 |
|
@chimurai thanks! |
|
@chimurai is there any chance you'd be able to review soon? If so, I'll resolve the merge conflicts with master. |
|
@chimurai sorry to poke again, but I'd like to understand if there's a chance that this will be merged, or if I'll have to fork. Thanks for your work maintaining this package! |
commit: |
|
Thanks for the PR and patience @nwalters512 PR looks good 🙏 Could you verify if your fix works with the package preview from Once verified, I'll merge and publish a new version |
There was a problem hiding this comment.
Pull Request Overview
This PR addresses issues with error handling during WebSocket upgrades by catching errors arising from user-provided router or path rewriting functions and propagating them through the proxy’s error event.
- Introduces a try/catch block in the WebSocket upgrade handler to catch errors from prepareProxyRequest.
- Updates the error-response-plugin to distinguish between HTTP response and socket objects for error handling.
- Adds a new test case to ensure that errors in the router are properly handled by the proxy.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| test/e2e/websocket.spec.ts | Adds a test validating that an error thrown in the router is handled correctly. |
| src/plugins/default/error-response-plugin.ts | Refactors error handling to differentiate between HTTP responses and sockets. |
| src/http-proxy-middleware.ts | Wraps the WebSocket upgrade handling in a try/catch to emit errors appropriately. |
Comments suppressed due to low confidence (3)
test/e2e/websocket.spec.ts:145
- [nitpick] Consider asserting specific properties of the error (e.g., the error message) to ensure that the error handling path in handleUpgrade is fully validated.
ws.on('error', (err) => {
src/plugins/default/error-response-plugin.ts:30
- Confirm that destroying the socket here is the intended behavior and consider adding a comment or logging to clarify why a socket is destroyed rather than sending an error response.
res.destroy();
src/http-proxy-middleware.ts:107
- [nitpick] Consider logging the caught error within the catch block to aid in debugging before emitting it via this.proxy.emit.
catch (err) {
|
@chimurai unfortunately I haven't been able to deal with the breaking changes from v3 yet, so I'm unable to test this in the context of my application. Now that I know this fix is coming, that's probably enough motivation to get me to upgrade 🙂 let me try to do that, and I'll test this if I succeed. |
|
Hope the following guides are helpful to upgrade: https://github.com/chimurai/http-proxy-middleware/blob/master/MIGRATION.md Let me know if you bump into issues 🙏 |
|
@chimurai I got myself upgraded and tried out this PR, it's working exactly how I want it to 🙏 I added some artificial errors to the bits of my code that deal with websocket routing and confirmed that they're now passed to the error handler I configured. |
|
Thanks @nwalters512 for contribution 🙏 |
|
@chimurai for my own planning, do you have a plan for when you'll cut the next release with this change? |
|
Hi @nwalters512. I hope to create a release somewhere this week. |
|
@nwalters512 just released https://github.com/chimurai/http-proxy-middleware/releases/tag/v3.0.4 thanks again for your contribution 🙏 |
Description
When there is an error from user-provided code (
pathFilter,rewritePath, orrouter), that error is now handled and exposed to the user viaproxy.emit('error', ...).error-response-pluginwas also updated to handle the fact that it might receive a socket. If it does, it will callsocket.destroy().Motivation and Context
See #822 for a description of the issue being fixed. This PR closes #822.
How has this been tested?
I added a new test case. It fails on master, and works correctly with my new changes.
Types of changes
I'm split on whether or not this should be considered a breaking change. Folks could have written code assuming that the error handler would always get a
Responseas the third argument, whereas now it would get aSocket. That said, the TypeScript types fromhttp-proxydo reflect that that could be aSocket, so it shouldn't come as too much of a surprise.Checklist: