fix(websocket): add Authorization header and fix close event timing #4745
Conversation
When creating a WebSocket connection with embedded credentials in the URL (e.g., ws://foo:bar@localhost:1337/), the Authorization header was not being generated. This aligns the behavior with the ws package and web standards. Fixes: #4744
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4745 +/- ##
==========================================
+ Coverage 92.89% 92.97% +0.08%
==========================================
Files 109 109
Lines 33886 33900 +14
==========================================
+ Hits 31478 31519 +41
+ Misses 2408 2381 -27 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
The WPT server uses pregenerated self-signed certificates. Configure the global dispatcher to trust the CA certificate so that wss:// and https:// connections work properly in WPT tests.
…CTING state When close() is called on a WebSocket in the CONNECTING state, events were firing synchronously during close() instead of being queued, and readyState was transitioning incorrectly (CONNECTING → CLOSED → CLOSING instead of CONNECTING → CLOSING → CLOSED). - Queue onSocketClose() via queueMicrotask in failWebsocketConnection() - Add guard in #onSocketClose to prevent duplicate calls Fixes #4741 Fixes #4742
mcollina
changed the title
| if (this.#handler.readyState === states.CLOSED) { | ||
| return | ||
| } |
There was a problem hiding this comment.
From local testing, this only seems to be called once. If we kept this in we'd be hiding other bugs.
There was a problem hiding this comment.
Sorry, I don't understand what you want to be done here
There was a problem hiding this comment.
The reference is actually wrong, I'll amend.
| // If the URL has credentials, add Authorization header (unless already set by options.headers) | ||
| // @see https://fetch.spec.whatwg.org/#concept-basic-fetch (step 12.2.3) | ||
| if ((url.username || url.password) && !request.headersList.contains('authorization', true)) { | ||
| const credentials = `${url.username}:${url.password}` | ||
| const encoded = Buffer.from(credentials).toString('base64') | ||
| request.headersList.append('authorization', `Basic ${encoded}`, true) | ||
| } |
There was a problem hiding this comment.
See my reasoning for why this should not be set: #4744 (comment)
There was a problem hiding this comment.
step 12.2.3
What is this referring to? scheme fetch doesn't have a 12.2.3
| if (this.#handler.readyState === states.CLOSED) { | ||
| return | ||
| } |
There was a problem hiding this comment.
From local testing, this only seems to be called once. If we kept this in we'd be hiding other bugs.
|
I'm concerned this fix isn't quite following the spec. It adds some extra state transition guards and a microtask. Whereas, the proper per-spec fix would be to add a real task (not microtask) at a higher level. E.g. instead of a microtask just for the I suspect if that sort of fix is implemented, then no specific state transition patches will be necessary, since the state transitions will always be queued properly in the task queue. |
Summary
options.headersclose()is called in CONNECTING state:close()returns (instead of synchronously during)WPT Tests Now Passing
websockets/basic-auth.any.html?wsswebsockets/interfaces/WebSocket/close/close-connecting-async.any.html(all variants)websockets/interfaces/WebSocket/close/close-connecting.html(all variants)websockets/interfaces/WebSocket/close/close-multiple.html(all variants)websockets/interfaces/WebSocket/close/close-nested.html(all variants)Fixes #4744
Fixes #4741
Fixes #4742