verifyClient callback now accepts "headers" parameter #1379
Conversation
.gitignore
Outdated
| @@ -2,4 +2,8 @@ node_modules/ | |||
| .nyc_output/ | |||
| coverage/ | |||
| .vscode/ | |||
| .idea/ | |||
There was a problem hiding this comment.
Can you please remove these? Not sure how .vscode ended here, but I think these should be in the global gitignore.
There was a problem hiding this comment.
No, also the others. I would prefer this file to be unchanged.
doc/ws.md
Outdated
| @@ -49,10 +49,12 @@ if `verifyClient` is provided with two arguments then those are: | |||
| - `cb` {Function} A callback that must be called by the user upon inspection | |||
| of the `info` fields. Arguments in this callback are: | |||
| - `result` {Boolean} Whether or not to accept the handshake. | |||
| - `code` {Number} When `result` is `false` this field determines the HTTP | |||
| - `[code]` {Number} When `result` is `false` this field determines the HTTP | |||
There was a problem hiding this comment.
Please remove the optional syntax, it's not used in the docs. We can specify that it defaults to 401, if wanted.
doc/ws.md
Outdated
| reason phrase. | ||
| - `[headers]` {Object} When `result` is `false` this field determines additional HTTP | ||
| headers to be sent to the client (for example, `{ 'Set-Cookie': 'abortReason=tooManyConnections' }`). |
There was a problem hiding this comment.
Remove () and put the example on a new line:
"... the client. For example,"
lib/websocket-server.js
Outdated
| this.options.verifyClient(info, (verified, code, message) => { | ||
| if (!verified) return abortHandshake(socket, code || 401, message); | ||
| this.options.verifyClient(info, (verified, code, message, headers) => { | ||
| if (!verified) return abortHandshake(socket, code || 401, message, headers); |
There was a problem hiding this comment.
Can we try to keep the limit of 80 chars per line?
lib/websocket-server.js
Outdated
| if (socket.writable) { | ||
| message = message || http.STATUS_CODES[code]; | ||
| headers = Object.assign({}, { |
There was a problem hiding this comment.
The first {} is not needed. I also wonder if it's better to move the defaults after the custom headers to prevent them from being overridden.
Edit: Overriding defaults should be ok.
There was a problem hiding this comment.
Also thought about moving custom headers before common. But decided to have common headers before in order to have them at the top of response (although order of headers is not important)
lib/websocket-server.js
Outdated
| 'Content-type: text/html\r\n' + | ||
| `Content-Length: ${Buffer.byteLength(message)}\r\n` + | ||
| '\r\n' + | ||
| Object.keys(headers).map(header => `${header}: ${headers[header]}`).join('\r\n') + |
There was a problem hiding this comment.
80 chars pls and same below in the test.
|
Left a few comments but overall LGTM. |
… to abortHandshake
|
Few 😄 Fixed. Take a look please |
|
Thanks, will merge later. |
Which is passed to abortHandshake().
Fixes: #695
Fixes: #728