Repository navigation
calling verifyClient callback multiple times results in "RSV1 must be clear" errors #1272
Description
- [ X] I've searched for any related issues and avoided creating a duplicate issue.
Description
Calling the verifyClient callback twice in verifyClient function triggers an "Invalid WebSocket frame: RSV1 must be clear"
It's not clear to me why this leads to that specific error. Calling the callback multiple times should probably be expressly forbidden with a related error message.
The error occurs when this.completeUpgrade is called twice.
I also think this might be the root cause behind #1238.
Reproducible in:
version:
Node.js version(s): 7.4.0
OS version(s): Mac OS X 10.10.5
Steps to reproduce:
- run the following code
let WebSocket = require('ws')
let verify = (info, cb) => {
cb(true)
cb(true)
}
const wss = new WebSocket.Server({ port: 8080, verifyClient: verify});
wss.on('connection', function connection(ws) {
ws.on('message', function incoming(message) {
console.log('received: %s', message);
});
ws.send('something');
});
const ws = new WebSocket('ws://localhost:8080');
ws.on('open', function open() {
ws.send("hello");
})
Expected result:
A different error message, or ignore subsequent calls of the verifyClient callback.
Actual result:
RSV1 must be clear
Activity
[-]calling callback muli[/-][+]calling verifyClient callback multiple times results in "RSV1 must be clear" errors[/+]lpinca commentedon Jan 6, 2018
The error happens because the first call completes the handshake, all other calls write invalid data (handshake headers) on the socket.
We can add a guard to ensure the callback is only called once but in general it's user responsibility.
#1238 is probably caused by nodejs/node#17789. A patch is already available: nodejs/node#17806.
lpinca commentedon Jan 7, 2018
I think this is actually a good thing, but I'm willing to be convinced otherwise. If we prevent the callback from being called multiple times, users will never know that something is wrong on their end.
delasky commentedon Jan 8, 2018
Thats fair. If i have time this week I might put in a PR to guard with a specific error, but at least now people who search in the issues can see this. Don't think its high priority
lpinca commentedon Jan 9, 2018
Ok, thank you.