HttpServer: match the Upgrade header value case-insensitively#584
HttpServer: match the Upgrade header value case-insensitively#584AudriusButkevicius wants to merge 1 commit into
Conversation
RFC 6455 section 4.2.1 requires the server to look for "an |Upgrade| header field containing the value 'websocket', treated as an ASCII case-insensitive value"; RFC 9110 section 7.8 likewise defines Upgrade protocol names as case-insensitive. HttpServer::handleConnection compared the value case-sensitively with "websocket", so a client spelling it differently was routed to the plain HTTP callback instead of being upgraded. Chrome's remote debugging proxy (what chrome://inspect attaches through) sends "Upgrade: WebSocket", so an HttpServer-fronted DevTools target answered the attach with a 404 from the HTTP handler instead of completing the websocket handshake. WebSocketHandshake::insensitiveStringCompare had a related defect: it implemented equality as CaseInsensitiveLess::cmp(a, b) == 0, but cmp is a lexicographical less-than, so the expression accepted any value that sorts at or above the expected one. Derive equivalence from the strict weak ordering properly: !cmp(a, b) && !cmp(b, a).
| @@ -98,7 +99,11 @@ namespace ix | |||
| { | |||
| auto request = std::get<2>(ret); | |||
| std::shared_ptr<ix::HttpResponse> response; | |||
There was a problem hiding this comment.
The headers variable should be a 'case insensitive' map library already, so something is wrong there.
There was a problem hiding this comment.
Ah I got confused, the problem is about the value.
This vibe coded code is impossible to read, we need a function that does if (stringEqual(a, b, true)) with the last arg being case sentisitive or something). Can you look at golang to see if they have such a function ?
There was a problem hiding this comment.
Apologies, lacking context, what does go have to do with this? I can add a new function that is case insensitive comparison, but I don't know of a good place for it, so just used compare twice.
bsergean
left a comment
bsergean
left a comment
There was a problem hiding this comment.
Thanks for the PR but I'm like a fix that is easier to understand than what claude vibe coded
| @@ -98,7 +99,11 @@ namespace ix | |||
| { | |||
| auto request = std::get<2>(ret); | |||
| std::shared_ptr<ix::HttpResponse> response; | |||
There was a problem hiding this comment.
Ah I got confused, the problem is about the value.
This vibe coded code is impossible to read, we need a function that does if (stringEqual(a, b, true)) with the last arg being case sentisitive or something). Can you look at golang to see if they have such a function ?
2 participants
RFC 6455 section 4.2.1 requires the server to look for "an |Upgrade| header field containing the value 'websocket', treated as an ASCII case-insensitive value"; RFC 9110 section 7.8 likewise defines Upgrade protocol names as case-insensitive.
HttpServer::handleConnection compared the value case-sensitively with "websocket", so a client spelling it differently was routed to the plain HTTP callback instead of being upgraded. Chrome's remote debugging proxy (what chrome://inspect attaches through) sends "Upgrade: WebSocket", so an HttpServer-fronted DevTools target answered the attach with a 404 from the HTTP handler instead of completing the websocket handshake.
WebSocketHandshake::insensitiveStringCompare had a related defect: it implemented equality as CaseInsensitiveLess::cmp(a, b) == 0, but cmp is a lexicographical less-than, so the expression accepted any value that sorts at or above the expected one. Derive equivalence from the strict weak ordering properly: !cmp(a, b) && !cmp(b, a).
There might be other headers that suffer from this.
Vibed with Claude.
Fixes #583