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

Define the WebSocket client handshake in terms of Fetch #236

Merged
merged 1 commit into from
Mar 9, 2016
Merged

Conversation

annevk
Copy link
Member

@annevk annevk commented Mar 7, 2016

Fixes #235.

@annevk
Copy link
Member Author

annevk commented Mar 7, 2016

@mikewest could you review this?

@domenic should Fetch abstract the entire WebSocket protocol or should HTML reference a mixture of Fetch and the WebSocket protocol? That is, should Fetch reintroduce the hooks HTML needs, perhaps even putting the security limitations on them directly so HTML no longer needs to do that. I think I have a slight preference for that, but it would be a larger change.

@domenic
Copy link
Member

domenic commented Mar 7, 2016

This looks much less painful than anticipated.

The need for "obtain a WebSocket connection" being overridden isn't completely clear. Why is step 1 omitted? And why is resource name excluded?

should Fetch abstract the entire WebSocket protocol or should HTML reference a mixture of Fetch and the WebSocket protocol? That is, should Fetch reintroduce the hooks HTML needs, perhaps even putting the security limitations on them directly so HTML no longer needs to do that. I think I have a slight preference for that, but it would be a larger change.

I'm not really clear what the difference would be, concretely, but in the abstract wrapping everything in fetch sounds a bit nicer. On the other hand, there are a lot of concepts in [WSP] referenced by HTML, and if these two are the only ones you need to override, then just having a no-op forwarding layer between Fetch and WSP would be a bit annoying.

@annevk
Copy link
Member Author

annevk commented Mar 8, 2016

The need for "obtain a WebSocket connection" being overridden isn't completely clear.

Note that "establish a WebSocket connection" in the RFC is the process of obtaining a connection, coupled with sending a client handshake request and validating the response. It's all things intertwined. Fetch starts out with the request and does things with that, and then at some point obtains a connection. The layering is different.

Why is step 1 omitted? And why is resource name excluded?

Step 1 is omitted because we already validated the input. Resource name is not actually used to obtain a connection. It's used for the handshake that is separately overridden (the larger section with all the header appending).

On the other hand, there are a lot of concepts in [WSP] referenced by HTML, and if these two are the only ones you need to override, then just having a no-op forwarding layer between Fetch and WSP would be a bit annoying.

I think I would import some of the subsetting that HTML does around error handling. But yes, there would be some forwarding too.

@domenic
Copy link
Member

domenic commented Mar 8, 2016

Note that "establish a WebSocket connection" in the RFC is the process of obtaining a connection, coupled with sending a client handshake request and validating the response. It's all things intertwined. Fetch starts out with the request and does things with that, and then at some point obtains a connection. The layering is different.

Ah, I see, the structure of that plaintext document makes it hard to understand. OK then.

@mikewest
Copy link
Member

mikewest commented Mar 8, 2016

This looks good, thank you Anne.

Some thoughts:

  1. How do you intend for this to play together with Service Workers? As written, we'd be exposing the initial request to the worker, which might give them an opportunity to do something unexpected.
  2. It would be nice if the HSTS spec mentioned WebSockets. As is, I think the fact that Fetch upgrades the initial request from HTTP to HTTPS means that the WebSocket upgrade will automagically continue over a secure connection (because the scheme will be https, so secure will be true in "obtain a websocket connection").
  3. We'll have to figure out what the CSP hook looks like, as connect-src looks for ws:whatever today. I think Fetch is fine here, as we're passing the whole request to CSP.

Otherwise, LGTM.

@annevk
Copy link
Member Author

annevk commented Mar 9, 2016

  1. Service workers are dealt with by setting the skip-service-worker flag.
  2. HSTS is indeed applied to WebSocket. (It was already by the HTML Standard, this moves it to the Fetch layer from the API layer.)
  3. I think the way type/destination etc. are set this should look similar to XMLHttpRequest so I think connect-src would fall out naturally. But yeah, you might need to update it to no longer look for ws: and such.

@annevk annevk merged commit ce16adc into master Mar 9, 2016
@annevk annevk deleted the websocket branch March 9, 2016 12:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants