(Bug) include the port in the websocket signing url - #114
Open
jamescamping wants to merge 1 commit into
Open
jamescamping wants to merge 1 commit into
jamescamping wants to merge 1 commit into
Conversation
Websocket.signed_path built the string to sign by formatting the endpoint yarl.URL. yarl omits the port when it matches the default for the scheme, so ws:// on port 80 and wss:// on port 443 signed a url with no port while the server signed one that had it, and the handshake failed with 401. Build the signing string directly from the hostname and port so the port is always present, and derive endpoint from that same string so the two cannot drift apart. Reproduced against Flix Server 8.1.1 deployed on http_port 80: the websocket handshake returned 401 while signed REST requests to the same server on the same port succeeded, since sign_request signs only the path and is therefore unaffected. With this change the handshake succeeds. Deployments on the default port 8080 keep their existing behaviour. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Websocket.signed_pathbuilds the string it signs by formatting theendpointyarl.URL:endpointis constructed asws://{hostname}:{port}/ws, but yarl drops the port when it is the default for the scheme. So the port is silently lost from the signed string:The server includes the port when it computes its own HMAC, so the signatures do not match and the handshake fails:
Note the URL in the error has no port.
This only affects servers running on a scheme-default port —
http_port: 80, or 443 with TLS. On the default 8080 yarl keeps the port and everything works, which is presumably why it has gone unnoticed.REST calls are unaffected, which makes this confusing to diagnose:
sign_requestsignsMETHOD\n\n\ndate\npathwith no scheme, host, or port, so signed HTTP requests to the very same server on the very same port succeed while only the websocket 401s.Reproduction
Against Flix Server 8.1.1 deployed with
http_port: 80, pointing the client directly at a single server (no proxy or load balancer involved), using an access key that works fine for REST:I also confirmed the mismatch directly by replaying the handshake by hand against the server: signing
ws://<server>:80/ws?keyid=...&expiretime=...returns101 Switching Protocols, while the identical string with the port removed returns401. That is the only difference between the two requests.Fix
Build the signing string straight from
hostnameandportrather than round-tripping it throughyarl.URL, and deriveendpointfrom that same string so the two cannot drift apart.The existing comment in
signed_pathalready notes one yarl normalisation that had to be worked around by hand (:escaping); this is the same class of issue.Behaviour on non-default ports is unchanged, and the URL actually used to connect is unchanged — only the string that gets signed is affected.
Testing
Websocket handshake now succeeds against a Flix Server 8.1.1 deployment on port 80; previously 401.
The signed string is byte-identical to the old one for every scheme/port combination except the two broken ones, so nothing else changes:
ws://host/wsws://host:80/wsws://host:443/wsws://host:443/wsws://host:8080/wsws://host:8080/wswss://host:80/wswss://host:80/wswss://host/wswss://host:443/wswss://host:8080/wswss://host:8080/wsruff format --checkclean;ruff checkreports the same set of pre-existing findings before and after this change.I don't have a TLS deployment to hand, so the
wss/ 443 case is reasoned from the same yarl behaviour rather than tested end to end.🤖 Generated with Claude Code