expose websocket in node bundle by KhafraDev · Pull Request #2217 · nodejs/undici (original) (raw)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm adding a -1 block mostly because this should be discussed with the rest of the @nodejs/collaborators, and I know more than one will object.

Given Node.js needs, this should be a semver-major for them.

I think targeting a major for undici should be good.

I’ve been wanting Node to include a global WebSocket like browsers for quite a while. Sure it can be semver-major, but what are the arguments against exposing it? It feels along the same lines as fetch.

We might want to consider whether we want Undici’s WebSocket versus incorporating the ws module or other solutions, but are there objections to adding any solution?

ws is (for good reasons) not standard compliant.

What is not spec compliant? Just the extension for passing dispatcher?

ws the npm package, not undici's websocket. lpinca's comment includes a few places where ws breaks from the spec. There's also some non-standard properties exposed, some options which go completely against the spec (ie. disabling masking for performance reasons), and some other things I'm probably forgetting. It's a great library, that I based undici's impl on quite a bit with help from @/lpinca, but exposing it in node would definitely be a mistake.

Assuming there’s no preferable implementation than Undici’s, what are the objections to exposing it in Node?

Should we update this PR to expose the new global WebSocket behind a flag --experimental-websocket? Then it could land and be backported, since the new flag would mean that it wouldn’t be a semver-major change. This would also let us start gathering feedback, for example if the Undici implementation has its own spec issues to fix. We’d have to resolve those (at least as much as we did for fetch) before unflagging; and the unflagging could only happen as semver-major. But at least we would start the process.

Maybe some of the comments in nodejs/node#19308? If anything, it's good to be safe and get feedback before merging this :)

Here goes the question for the @nodejs/tsc:

Given the downsides of nodejs/node#19308 (comment) and @lpinca objection, are we good in bringing a standard compliant implementation of WebSocket as a global?

Should we update this PR to expose the new global WebSocket behind a flag --experimental-websocket? Then it could land and be backported, since the new flag would mean that it wouldn’t be a semver-major change. This would also let us start gathering feedback, for example if the Undici implementation has its own spec issues to fix. We’d have to resolve those (at least as much as we did for fetch) before unflagging; and the unflagging could only happen as semver-major. But at least we would start the process.

Having this approach would simplify our maintenance, as we would be able to not release a major of undici, and simplify backporting.

I'm +1 on this, it's the first step towards compatibility with web standard

This PR doesn't have direct impact on node (except bundle size), right?

nodejs/node#19308 (comment) mentions fetch('wss://'). Is that implemented? Is it actually already available in Node.js?
This would make the exposition of the WebSocket constructor a small addition compared to the actual support of WebSockets.

I agree with @/lpinca on shortcomings, +1 on adding this behind a flag

@targos

mentions fetch('wss://'). Is that implemented? Is it actually already available in Node.js?

I think I misinterpreted the spec there; the websocket spec uses fetch internals, which is why it's mentioned in the fetch spec. I thought this meant that fetch would support websockets, which in hindsight doesn't make much sense (there wouldn't be a way to send or receive messages lol). https://fetch.spec.whatwg.org/#websocket-protocol

This PR doesn't have direct impact on node (except bundle size), right?

yeah

@mcollina I don’t see any objections since your block, would you consider lifting it? I think we can add support for this behind an experimental flag. Then we can both backport it and continue to iterate on any spec or other issues.

With regard to the “but it’s only a WebSocket client, not also a server” . . . well, yeah, but that’s fine. If we want to ship a WebSocket server built into Node, we can do so via some other API. To use Deno as a reference:

Provides the API for creating and managing a WebSocket connection to a server, as well as for sending and receiving data on the connection.

If you are looking to create a WebSocket server, please take a look at Deno.upgradeWebSocket().

Exposing this behind a flag would let people start to explore options for solving that use case.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@KhafraDev to be clear, I don’t think this PR can land as is, it needs to still add a flag. Perhaps --experimental-websocket? --experimental-websocket-client? And the docs need updating.

A flag would need to be added once websocket is exposed in node core (I plan on doing this once this lands). There's already an experimental warning added.

A flag would need to be added once websocket is exposed in node core (I plan on doing this once this lands). There’s already an experimental warning added.

Does this PR expose anything that application authors would be able to access?

Okay so if this doesn’t expose it to end users just yet, then 👍 seems fine to land this to me. Before we expose it as a public API let’s please add the flag and docs.

Perhaps --experimental-websocket? --experimental-websocket-client? And the docs need updating.

If we're potentially adding both a client and server, how about --experimental-websocket=client|server (then in code, if getOption(…).includes('client')) and if getOption(…).includes('server'))).

I don't think there is or there should be a server implementation of this. The API is really flawed for that use case.

I don’t think there is or there should be a server implementation of this. The API is really flawed for that use case.

I wouldn’t be so categorical that Node will never have a websocket server of any kind. Deno has one, the ws module provides one. Maybe it won’t be via this API, but I wouldn’t rule out that we might add one at some point.

I’m not sure --experimental-websocket=client|server makes much sense; that seems like you have to enable only one or the other? Or pass this flag twice to enable both? I think we could maybe just have --experimental-websocket that enables both (if we ever get that far) and maybe we could make the client available unflagged when it’s ready and the flag continues to gate the server until that’s ready.

Not blocking that on this.

I wouldn’t be so categorical that Node will never have a websocket server of any kind. Deno has one, the ws module provides one.

Given that we've been waiting for @nodejs/collaborators to act on WebSockets since Mar 12, 2018, I think it would be fair to be categorical about that.

ws is great, had been around a very long time, and meets most needs, but does have a fair amount of quirks. As someone who's been using undici as a dependency (request is so much nicer than fetch) instead of what's exposed via Node directly, I wouldn't mind an undici-dependency-only WebSocket server as part of this package. We've lived with Koa/Express/etc for well over a decade now without anything as comprehensive and complete exposed from Node core, so I'm not really sold that it's needed in Node core. I'd welcome a compliant websocket server package from the same team that's put undici together because it is quite excellent.

For visibility, someone independently proposed a new node:websocket module in nodejs/node#49478.

I like the Deno.upgradeWebSocket(req)

Deno.serve((req) => { if (req.headers.get("upgrade") != "websocket") { return new Response(null, { status: 501 }); } const socket = Deno.upgradeWebSocket(req);

socket instanceof WebSocket // true

socket.addEventListener("open", () => { console.log("a client connected!"); });

socket.addEventListener("message", (event) => { if (event.data === "ping") { socket.send("pong"); } });

return response; });

would like to have something like

server.on('upgrade', (req, socket) => { const socket = req.toWebSocket()

socket.addEventListener("open", () => { console.log("a client connected!") })

socket.addEventListener("message", (event) => { if (event.data === "ping") { socket.send("pong") } }) })

I think no one would recommend the use of WebSocket. Use the ws module, at least in servers. It's a really badly spec'd api.

I don't think undici (an http client) is the correct place to adding a Websocket server. ws is a great module, or node could use uws for a websocket server.

kodiakhq Bot referenced this pull request in X-oss-byte/Canary-nextjs

Oct 1, 2023

kodiakhq Bot referenced this pull request in ascorbic/unpic-img

Oct 2, 2023

kodiakhq Bot referenced this pull request in X-oss-byte/Nextjs

Oct 6, 2023

ascorbic referenced this pull request in ascorbic/unpic-img

Oct 7, 2023

kfcampbell referenced this pull request in octokit/rest.js

Oct 16, 2023

](https://renovatebot.com)

This PR contains the following updates:

Package	Change	Age	Adoption	Passing	Confidence
undici
(source)	[`5.22.1` ->
`5.26.2`](https://renovatebot.com/diffs/npm/undici/5.22.1/5.26.2)
](https://docs.renovatebot.com/merge-confidence/)

](https://docs.renovatebot.com/merge-confidence/)

](https://docs.renovatebot.com/merge-confidence/)

](https://docs.renovatebot.com/merge-confidence/)

GitHub Vulnerability Alerts

CVE-2023-45143

Impact

Undici clears Authorization headers on cross-origin redirects, but does not clear Cookie headers. By design, cookie headers are forbidden request headers, disallowing them to be set in RequestInit.headers in browser environments. Since Undici handles headers more liberally than the specification, there was a disconnect from the assumptions the spec made, and Undici's implementation of fetch.

As such this may lead to accidental leakage of cookie to a 3rd-party site or a malicious attacker who can control the redirection target (ie. an open redirector) to leak the cookie to the 3rd party site.

Patches

This was patched in e041de359221ebeae04c469e8aff4145764e6d76, which is included in version 5.26.2.

Release Notes

nodejs/undici (undici)

Package	Change	Age	Adoption	Passing	Confidence
undici
(source)	[`5.23.0` ->
`5.26.2`](https://renovatebot.com/diffs/npm/undici/5.23.0/5.26.2)
](https://docs.renovatebot.com/merge-confidence/)

](https://docs.renovatebot.com/merge-confidence/)

](https://docs.renovatebot.com/merge-confidence/)

](https://docs.renovatebot.com/merge-confidence/)

Package	Change	Age	Adoption	Passing	Confidence
undici
(source)	[`5.22.0` ->
`5.26.2`](https://renovatebot.com/diffs/npm/undici/5.22.0/5.26.2)
](https://docs.renovatebot.com/merge-confidence/)

](https://docs.renovatebot.com/merge-confidence/)

](https://docs.renovatebot.com/merge-confidence/)

](https://docs.renovatebot.com/merge-confidence/)

expose websocket in node bundle by KhafraDev · Pull Request #2217 · nodejs/undici (original) (raw)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GitHub Vulnerability Alerts

Impact

Patches

Release Notes

What's Changed

What's Changed

New Contributors

What's Changed

New Contributors

What's Changed

New Contributors

What's Changed

What's Changed

New Contributors

Notable Changes

What's Changed

New Contributors

What's Changed

New Contributors

Configuration

GitHub Vulnerability Alerts

Impact

Patches

Release Notes

What's Changed

What's Changed

New Contributors

What's Changed

New Contributors

What's Changed

New Contributors

What's Changed

What's Changed

New Contributors

Notable Changes

What's Changed

New Contributors

Configuration

GitHub Vulnerability Alerts

Impact

Patches

Release Notes

What's Changed

What's Changed

New Contributors

What's Changed

New Contributors

What's Changed

New Contributors

What's Changed

What's Changed

New Contributors

Notable Changes

What's Changed

New Contributors

What's Changed

New Contributors

What's Changed

New Contributors

Configuration