Skip to content
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

WebSocket Cookies #7632

Open
Nerixyz opened this issue Sep 22, 2020 · 18 comments
Open

WebSocket Cookies #7632

Nerixyz opened this issue Sep 22, 2020 · 18 comments
Labels
cli related to cli/ dir suggestion suggestions for new features (yet to be agreed)

Comments

@Nerixyz
Copy link
Contributor

Nerixyz commented Sep 22, 2020

Since Deno 1.4, there's the Browser's WebSocket api in Deno which is now the preferred way of using WebSockets.
Now in Browsers, it's possible for a site to set cookies which are sent (if valid) with the WebSocket request. In Deno, there's no way of setting these cookies (as there's no "cookie store"), so one has to use the std/ws module which isn't designed for clients. It would be appreciated to have some kind of way to set cookies with the request. I know this isn't straight forward as for example adding a constructor field would break the standard and purpose of implementing the api.

@crowlKats
Copy link
Member

crowlKats commented Sep 22, 2020

Now in Browsers, it's possible for a site to set cookies which are sent (if valid) with the WebSocket request

How does that work? The spec does not mention anything of the sort.

@lucacasonato
Copy link
Member

I think in browsers your standard origin cookie jar is sent along.

@Nerixyz
Copy link
Contributor Author

Nerixyz commented Sep 22, 2020

How does that work? The spec does not mention anything of the sort.

There are cookies sent with the request:

The headers to send appropriate cookies must be a Cookie header whose value is the cookie-string computed from the user's cookie store and the URL url; for these purposes this is not a "non-HTTP" API.

Source

@kitsonk kitsonk added cli related to cli/ dir suggestion suggestions for new features (yet to be agreed) labels Sep 22, 2020
@Nerixyz
Copy link
Contributor Author

Nerixyz commented Sep 27, 2020

To remain compatible with the web, the WebSocket constructor could accept options as a third (or second) parameter. This it at least how it's done in the Node ws library (headers is an option on http.request().

The options could contain a headers field. This would allow users to set the Cookie header and possibly additional ones.

@Pangoraw
Copy link

Pangoraw commented Feb 2, 2021

Is there a consensus on what the API should look like? I would like to create an authenticated WebSocket client from deno but the current API provides no way to modify the underlying initial HTTP request.

I can submit a patch to propose the changes if the API has been validated 👍

@Nerixyz
Copy link
Contributor Author

Nerixyz commented Feb 2, 2021

Is there a consensus on what the API should look like?

No. The WebSocket API builds on the fetch API and it will take the cookies of the current context into consideration (that's at least what I think is meant by credentials mode is "include"). As Deno isn't a Browser, the cookies (that aren't present) aren't sent.

I think the most acceptable thing would be the method I proposed. But this would mean, Deno wouldn't have the same implementation as the Browsers. But due to the nature of the WebSocket constructor (it will immediately connect), there's no other step, where the configuration could happen.

@crowlKats
Copy link
Member

This is somewhat related to whatwg/html#5862 i guess

@Pangoraw
Copy link

Pangoraw commented Feb 2, 2021

With fetch, you can at least workaround it by providing your own Cookie header in the ConnectionInit parameter. Whereas for the current websocket implementation there is nothing you can do. Using a shared cookie jar, it is not possible to make parallel requests as multiple users for example unlike with passing the cookie explicitely.

@crowlKats
Copy link
Member

Found this whatwg/websockets#16

@scientific-dev
Copy link

scientific-dev commented Apr 21, 2021

Well. In this case you can either use the deno's 0.68.0 standard library where you can send cookies in Headers.
https://deno.land/[email protected]/ws (I am not sure is it stable but it works fine).
I attempted to make a similar implementation to it: https://deno.land/x/custom_socket (I am not sure is it stable but it works fine).

@iacore
Copy link

iacore commented Mar 26, 2022

It's already supported in unstable API as WebSocketStream: #11887.

@e3dio
Copy link

e3dio commented May 25, 2023

3 years and 1 line of code later, we now have working Websocket headers ;) running this in production

const headers = new Headers({ 'Set-Cookie': 'Hello=123' });

const { socket, response } = Deno.upgradeWebSocket(request, { headers });
return response;

@e3dio
Copy link

e3dio commented May 26, 2023

I opened different issue to address specifically Server setting headers for Websocket requests. There is no debate that the Server should be allowed to set headers, but there is still debate whether Client should be allowed to set headers, which I don't have an opinion on. I just need the basic functionality of the Server setting headers: #19277

@e3dio
Copy link

e3dio commented May 26, 2023

Regarding allowing the client to set headers on new connections, you should probably do the same as https://github.com/websockets/ws and https://github.com/oven-sh/bun which is allow a 2nd/3rd options parameter new WebSocket(url, { headers })

@crowlKats
Copy link
Member

Regarding allowing the client to set headers on new connections, you should probably do the same as https://github.com/websockets/ws and https://github.com/oven-sh/bun which is allow a 2nd/3rd options parameter new WebSocket(url, { headers }). The client can manually send cookies this way if they want to.

This would be going against the spec.

@josephrocca
Copy link
Contributor

josephrocca commented Aug 30, 2023

@crowlKats Maybe I misunderstand, but why is that going against the spec any more than this is?

Surely if WebSocket were to ever get headers, the second parameter (currently an array, protocols) will be allowed to be an options bag with protocols and headers keys?

There's abundent precedent in JS for changing a parameter to an options bag while still allowing the original for backwards-compatibility - e.g.

addEventListener(type, listener)
addEventListener(type, listener, useCapture) // old
addEventListener(type, listener, options) // new

I think Bun has taken the practical approach here that is most helpful for users while being least likely to cause any future problems.

As @lucacasonato said RE WebSocketStream:

This makes sense to me. It feels like a natural extension to the spec. If the spec ever adds header support, it would likely be in this format.

Would that not apply to WebSocket too? Again, I might have misunderstood your point here, so apologies if so.

@crowlKats
Copy link
Member

@josephrocca if you currently do new WebSocket("ws://localhost:8000", {foo: "bar"}) in a browser, this will throw an error.
On the case of WebSocketStream, object are naturally extensible, and additional fields are ignored, making it not a problem, but changing the parameter type itself does cause issues.

@josephrocca
Copy link
Contributor

if you currently do new WebSocket("ws://localhost:8000", {foo: "bar"}) in a browser this will throw an error

This is actually a good thing - because it means that the Deno/Bun (vs browser) difference can't lead to weird problems where e.g. the indices/keys of a protocol array are treated as keys of the option object, or something strange like that.

If that code didn't throw an error in browsers, it could be a more dangerous extension to add, because it could lead to weird backwards-incompatible behaviour that browsers would never support. The explicit/up-front error makes us safe from that.

For context: I have a bunch of WebSocket code and have suddenly learned that a container platform that I'm moving to requires an authentication header on every request to get through the gateway. This situation is a bit more painful than I thought it'd be due to Deno's choices here. Bun's choice just seems plainly correct to me.

@bartlomieju if you have any thoughts here it'd be good to hear them - if not apologies for ping and please ignore.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli related to cli/ dir suggestion suggestions for new features (yet to be agreed)
Projects
None yet
Development

No branches or pull requests

9 participants