-
-
Notifications
You must be signed in to change notification settings - Fork 689
fix(websocket): add Authorization header and fix close event timing #4745
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -554,6 +554,11 @@ class WebSocket extends EventTarget { | |
| * @see https://datatracker.ietf.org/doc/html/rfc6455#section-7.1.4 | ||
| */ | ||
| #onSocketClose () { | ||
| // Guard against duplicate calls | ||
| if (this.#handler.readyState === states.CLOSED) { | ||
| return | ||
| } | ||
|
Comment on lines
+558
to
+560
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From local testing, this only seems to be called once. If we kept this in we'd be hiding other bugs.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, I don't understand what you want to be done here
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The reference is actually wrong, I'll amend. |
||
|
|
||
| // If the TCP connection was closed after the | ||
| // WebSocket closing handshake was completed, the WebSocket connection | ||
| // is said to have been closed _cleanly_. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,89 @@ | ||
| 'use strict' | ||
|
|
||
| const { test } = require('node:test') | ||
| const { once } = require('node:events') | ||
| const { createServer } = require('node:http') | ||
| const { WebSocket } = require('../..') | ||
|
|
||
| test('WebSocket sets Authorization header from URL credentials', async (t) => { | ||
| const server = createServer((req, res) => { | ||
| const expected = 'Basic ' + Buffer.from('foo:bar').toString('base64') | ||
| t.assert.strictEqual(req.headers.authorization, expected) | ||
| res.end() | ||
| }).listen(0) | ||
|
|
||
| await once(server, 'listening') | ||
| t.after(() => server.close()) | ||
|
|
||
| const ws = new WebSocket(`ws://foo:bar@localhost:${server.address().port}/`) | ||
| ws.onerror = () => {} // Expected - server doesn't complete WebSocket handshake | ||
|
|
||
| await once(server, 'request') | ||
| }) | ||
|
|
||
| test('WebSocket sets Authorization header with only username', async (t) => { | ||
| const server = createServer((req, res) => { | ||
| const expected = 'Basic ' + Buffer.from('foo:').toString('base64') | ||
| t.assert.strictEqual(req.headers.authorization, expected) | ||
| res.end() | ||
| }).listen(0) | ||
|
|
||
| await once(server, 'listening') | ||
| t.after(() => server.close()) | ||
|
|
||
| const ws = new WebSocket(`ws://foo@localhost:${server.address().port}/`) | ||
| ws.onerror = () => {} | ||
|
|
||
| await once(server, 'request') | ||
| }) | ||
|
|
||
| test('WebSocket sets Authorization header with only password', async (t) => { | ||
| const server = createServer((req, res) => { | ||
| const expected = 'Basic ' + Buffer.from(':bar').toString('base64') | ||
| t.assert.strictEqual(req.headers.authorization, expected) | ||
| res.end() | ||
| }).listen(0) | ||
|
|
||
| await once(server, 'listening') | ||
| t.after(() => server.close()) | ||
|
|
||
| const ws = new WebSocket(`ws://:bar@localhost:${server.address().port}/`) | ||
| ws.onerror = () => {} | ||
|
|
||
| await once(server, 'request') | ||
| }) | ||
|
|
||
| test('WebSocket does not set Authorization header when no credentials', async (t) => { | ||
| const server = createServer((req, res) => { | ||
| t.assert.strictEqual(req.headers.authorization, undefined) | ||
| res.end() | ||
| }).listen(0) | ||
|
|
||
| await once(server, 'listening') | ||
| t.after(() => server.close()) | ||
|
|
||
| const ws = new WebSocket(`ws://localhost:${server.address().port}/`) | ||
| ws.onerror = () => {} | ||
|
|
||
| await once(server, 'request') | ||
| }) | ||
|
|
||
| test('WebSocket custom Authorization header takes precedence over URL credentials', async (t) => { | ||
| const customAuth = 'Bearer mytoken' | ||
| const server = createServer((req, res) => { | ||
| t.assert.strictEqual(req.headers.authorization, customAuth) | ||
| res.end() | ||
| }).listen(0) | ||
|
|
||
| await once(server, 'listening') | ||
| t.after(() => server.close()) | ||
|
|
||
| const ws = new WebSocket(`ws://foo:bar@localhost:${server.address().port}/`, { | ||
| headers: { | ||
| Authorization: customAuth | ||
| } | ||
| }) | ||
| ws.onerror = () => {} | ||
|
|
||
| await once(server, 'request') | ||
| }) |
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my reasoning for why this should not be set: #4744 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this referring to? scheme fetch doesn't have a 12.2.3