Skip to content

Conversation

stackotter
Copy link
Contributor

RFC 6455 section 4.2.1 prescribes which headers are mandatory for a WebSocket handshake request to be correct. It also details what each of these headers should contain. This PR brings WebSocketHTTPHander up to spec (which has a typo by the way, but I didn't change that to keep this PR focussed).

I have documented the new code in a way that makes it easy to match up each check with its corresponding RFC rule (see WebSocketHTTPHander.verifyHandshakeRequestHeaders).

There is one conundrum that I ran into while implementing the checks, which is that rule 1 mandates that websocket connections must begin as GET requests, however in FlyingFox the method for websocket endpoints is up to users of the framework. We could add some kind of check to appendRoute, or we could just leave it given that developers will probably realise pretty quickly that their websocket won't work as a POST endpoint. Another option would be creating a separate method for hosting websocket endpoints such as appendWebSocketRoute which takes only a path and doesn't let developers specify the method.

Previously the handler did not strictly check for all
required headers, and did not verify their values.
@codecov
Copy link

codecov bot commented Apr 8, 2022

Codecov Report

Merging #29 (9d7042e) into main (0984529) will decrease coverage by 0.08%.
The diff coverage is 96.82%.

@@            Coverage Diff             @@
##             main      #29      +/-   ##
==========================================
- Coverage   99.75%   99.66%   -0.09%     
==========================================
  Files          42       42              
  Lines        2002     2062      +60     
==========================================
+ Hits         1997     2055      +58     
- Misses          5        7       +2     
Impacted Files Coverage Δ
...yingFox/Sources/Handlers/WebSocketHTTPHander.swift 97.59% <96.82%> (-2.41%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0984529...9d7042e. Read the comment docs.

@stackotter
Copy link
Contributor Author

I also added a test for the new checks, but it would probably be better if I split it into a test per rule (and updated it to use validateHandshakeRequestHeaders now that I've refactored through checks into their own function).

Copy link
Owner

@swhitty swhitty left a comment

Choose a reason for hiding this comment

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

Looks great. Thank you for these RFC fixes.

A month ago I had no idea how websockets actually worked so this really helps 🙏🏻.

@swhitty swhitty merged commit 6f1499c into swhitty:main Apr 8, 2022
@swhitty
Copy link
Owner

swhitty commented Apr 8, 2022

RE: the requirement for WebSockets requiring the GET HTTPMethod. I do think it is probably OK to allow developers to choose the HTTP Method, even if it is not compliant. We could use HTTPLogger to output a warning.

One use case I have for FlyingFox is to assist within testing. WebSocketHTTPHandler* should be as close to RFC compliant as possible, but I see some utility in allowing any HTTPHandler to switch to a web socket connection.

@swhitty
Copy link
Owner

swhitty commented Apr 8, 2022

And I have renamed the handler here.

@stackotter
Copy link
Contributor Author

Looks great. Thank you for these RFC fixes.

A month ago I had no idea how websockets actually worked so this really helps 🙏🏻.

You're welcome! While reading through the RFC I also found out that I had no idea how it actually worked hahah, there's certainly a lot to learn by reading the RFC documents, but I will admit that they can be quite bland sometimes. I never knew that websocket connections started as HTTP requests and then switched, the more you know.

@stackotter
Copy link
Contributor Author

RE: the requirement for WebSockets requiring the GET HTTPMethod. I do think it is probably OK to allow developers to choose the HTTP Method, even if it is not compliant. We could use HTTPLogger to output a warning.

One use case I have for FlyingFox is to assist within testing. WebSocketHTTPHandler* should be as close to RFC compliant as possible, but I see some utility in allowing any HTTPHandler to switch to a web socket connection.

I agree with that reasoning, I don't see any issues with straying from the RFC in that regard 👍

@stackotter stackotter deleted the follow-websocket-rfc branch April 8, 2022 06:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants