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

Fix for websocket server reading Sec-WebSocket-Key in wrong place when missing, and handling common mistakes from clients #17

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

SoylentGraham
Copy link

#12

This is a fix for SimpleWebTransport attempting to read the websocket handshake key "Sec-WebSocket-Key: " from the response headers, but

  • Some servers respond with different case (typically. Sec-Websocket-Key instead of Sec-WebSocket-Key).
    • Specfifically in our case, Ngrok sends Websocket
      -gr: I can't remember off hand which is the correct case, but that's a little irrelevent when established servers & clients send the wrong things
  • The code previously didn't actually check if this required header was missing, and thus would read the key from -1 + offset, which is obviously wrong. (could read out of bounds & throw, could just read garbage)
    • As it's a requirement, I made it throw.

@SoylentGraham SoylentGraham changed the title Ngrok websocket server fix Fix for websocket server reading Sec-WebSocket-Key in wrong place when missing, and handling common mistakes from servers Jul 7, 2024
@SoylentGraham SoylentGraham changed the title Fix for websocket server reading Sec-WebSocket-Key in wrong place when missing, and handling common mistakes from servers Fix for websocket server reading Sec-WebSocket-Key in wrong place when missing, and handling common mistakes from clients Jul 12, 2024
@SoylentGraham
Copy link
Author

Small update on this, ive rewritten in my fork a chunk of this to do proper http header parsing and the code is far far cleaner.

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.

1 participant