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

Support setting credentials mode in WebSocket #6896

Closed

Conversation

lucacasonato
Copy link
Member

@lucacasonato lucacasonato commented Jul 23, 2021

This adds configuration of credential mode to the WebSocket constructor. This resolves whatwg/websockets#18.

As far as I can tell this change should be trivial to implement, as it effectively only changes that the "credentials mode" for "establish a WebSocket connection" in fetch is injected from the HTML spec, rather than hard coded.

Is there implementer intrest? We would welcome this change for Deno - not for the feature itself (we don't have credentials or a credentials mode), but for the addition of an options bag to the WebSocket constructor.

  • At least two implementers are interested (and none opposed):
    • Chrome
    • Firefox
    • Safari
  • Tests are written and can be reviewed and commented upon at:
    • TODO, waiting on implementer interest
  • Implementation bugs are filed:
    • Chrome: waiting on implementer interest
    • Firefox: waiting on implementer interest
    • Safari: waiting on implementer interest

/web-sockets.html ( diff )


/web-sockets.html ( diff )

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

This largely looks good to me, modulo nits.

source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
@annevk
Copy link
Member

annevk commented Jul 26, 2021

@ricea can you review as well?

@whatwg whatwg deleted a comment from Thinker1971997 Jul 26, 2021
@ricea
Copy link

ricea commented Jul 26, 2021

credentials: 'same-origin' is weird.

Strictly speaking, the page can never be same-origin with the WebSocket connection as they have a different scheme. So 'same-origin' would mean the same as "omit".

However, the Fetch Standard rewrites the scheme to an HTTP scheme for WebSockets, which may mean that it will actually send credentials if the host + port is the same as the page. Which may even be useful behaviour for someone.

I see a bunch of different options:

  1. Use a different enum that doesn't have "same-origin" in it.
  2. Keep "same-origin" but rewrite it to "omit" before passing to the Fetch Standard (or inside the Fetch Standard) to retain the "logical behaviour".
  3. Make "same-origin" explicitly mean "same origin after rewriting the scheme".

@ricea
Copy link

ricea commented Jul 26, 2021

Putting my implementer hat on, this is not hard to implement as it is a simple matter of plumbing. However, because it will cause WebSockets to hit code paths they haven't been hitting so far, it will introduce bugs. The way to mitigate that is to add more tests. A lot more tests. Which would be a significant amount of work.

I have doubts that this feature would see enough usage to justify the investment.

@annevk
Copy link
Member

annevk commented Jul 26, 2021

That's a good point. I think same-origin should mean same origin after rewriting the scheme for consistency with how we'd enforce policies inside Fetch. I think that's already what it ends up meaning due to how this is integrated with Fetch, but I might be missing something. Either way we should maybe call that out in a note.

@lucacasonato
Copy link
Member Author

@annevk Should I add a note in the fetch spec (to the "Establish a WebSocket connection" section), or in the HTML spec?


@ricea Regarding implementation complexity: I would argue that the cost of implementation and writing tests (the latter of which I would be happy to do) is worth it, because of the following:

  • This adds a security capability which previously was not present. Previously establishing WS connections without credentials was not in any way possible.
  • Signatures between WebSocket and WebSocketStream get aligned.
  • Signature gets aligned to current style guides (use an options bag for fields which might be extended in the future).
  • Options bag allows for further alignment to upcoming WebSocketStream, namely easier addition of AbortSignal down the road.

@annevk
Copy link
Member

annevk commented Jul 26, 2021

That's a good question. Perhaps in both places something is warranted. Though let's put it here for now as long term it should all end up in a single WebSocket standard.

@domenic
Copy link
Member

domenic commented Jul 26, 2021

I am a weak -1 on this as I think evolving WebSocket is a dead end compared to further investment in WebSocketStream.

@lucacasonato
Copy link
Member Author

lucacasonato commented Jul 26, 2021

@domenic I don't disagree, but we are still waiting on an actual spec for WebSocketStream, and there is so far no standards position from Mozilla or WebKit AFAIK.

Implementing this now for WebSocket will make it trivial to adopt for WebSocketStream, as they both use the same codepaths for "Establish a WebSocket connection" in the fetch spec. I don't see the point in artificially limiting the old API, even though it has subjectively worse DX: it does have a significant existing user base and many libraries are built on it.

Implementing this would also allow a credentials mode to work in polyfills for WebSocketStream.

@domenic
Copy link
Member

domenic commented Jul 26, 2021

It's not a matter of limiting the old API; it's a matter of not expending additional implementation effort on it, or making it more attractive.

@ricea
Copy link

ricea commented Jul 26, 2021

I am a weak -1 on this as I think evolving WebSocket is a dead end compared to further investment in WebSocketStream.

Yes, I'd prefer to stick to adding new features to WebSocketStream. But the timeline for shipping WebSocketStream is still up in the air.

@ricea
Copy link

ricea commented Jul 26, 2021

@ricea Regarding implementation complexity: I would argue that the cost of implementation and writing tests (the latter of which I would be happy to do) is worth it, because of the following:

It would be awesome if you would write tests, but in general we're lacking infrastructure to test this kind of stuff in WPT. For example, one test I'd like is verifying that if you have cached auth credentials both for the proxy and the destination site, only the ones for the proxy are sent. This involves configuring a proxy and caching credentials, both things which are difficult to do in an automated cross-browser fashion.

Most testing for auth stuff in Chromium is done at a lower level than the web platform, using either a fake sockets or a WebSocket server we can start up with different configurations per-test.

  • This adds a security capability which previously was not present. Previously establishing WS connections without credentials was not in any way possible.

It's a good capability, but it only helps users if developers actually use it.

@ricea
Copy link

ricea commented Jul 26, 2021

That's a good point. I think same-origin should mean same origin after rewriting the scheme for consistency with how we'd enforce policies inside Fetch.

Since all the credentials we use come from HTTP anyway, I've convinced myself that this is the correct behaviour.

@domenic domenic added addition/proposal New features or enhancements needs implementer interest Moving the issue forward requires implementers to express interest topic: websockets labels Aug 3, 2021
@domenic
Copy link
Member

domenic commented Feb 22, 2022

Closing for now, given that the spec has moved to whatwg/websockets. I transferred the corresponding issue to whatwg/websockets#18.

@domenic domenic closed this Feb 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addition/proposal New features or enhancements needs implementer interest Moving the issue forward requires implementers to express interest topic: websockets
Development

Successfully merging this pull request may close these issues.

Configure credentials of cookies for WebSocket
4 participants