Closed marcolanaro closed 1 year ago
Thanks for reporting! Would you like to send a Pull Request to address this issue?
Also a unit test would be amazing.
I can reliably crash mercurius locally with the following snippet:
const WebSocket = require('ws');
const ws = new WebSocket('ws://127.0.0.1:1337/graphql');
ws.on('open', function open() {
ws._socket.write(Buffer.from([0x92, 0x00]));
});
Hopefully will help us finding a resolution!
I think I got it.
In @fastify/websocket
we would need to add this:
connection.on('error', (error) => {
fastify.log.error(error);
});
in https://github.com/fastify/fastify-websocket/blob/master/index.js#L63
I'll work on a PR later.
I've opened a PR on @fastify/websocket
: https://github.com/fastify/fastify-websocket/pull/228.
@mcollina thank you for merging the PR. As soon as you release a new version I'll be happy to create a PR here. I would love to get my fix out before the weekend :)
The @fastify/websocket
change shipped!
@mcollina the PR is ready ❤️
This was fixed in @fastify/websocket
v7.1.1.
I just figured out the issue still happen in some scenarios. Will dig more tomorrow, but I'm able to crash my server defining the protocol, e.g.:
const WebSocket = require('ws');
const ws = new WebSocket('ws://127.0.0.1:1337/graphql', 'graphql-ws');
ws.on('open', function open() {
ws._socket.write(Buffer.from([0xa2, 0x00]));
});
Good afternoon, I'm writing to report an important websocket issue. I would have loved to send a PR, but the reality of the facts is that so far I've not been able to even reproduce the issue. I'm sure this is going to be useful for someone else and we can find some workaround together.
Impact
The server crash with:
I believe this started happening since an upgrade to
node 18
.Preliminary Analysis
Digging over the internet, I found some recent resources that well describe what happens. Recent conversation on
websocket/ws issue 1354
. Recent workaround on trpc 2522.Summary: it looks like some safari browsers are buggy and set some bits without negotiating them with the server.
If the workaround on trpc actually works, I'm not sure why this would not be enough: https://github.com/mercurius-js/mercurius/blob/master/lib/subscription-client.js#L86