Bug 202095

Summary: websockets handshaking broken for responses that omit status text / reason
Product: WebKit Reporter: Joey Korkames <joey>
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED CONFIGURATION CHANGED    
Severity: Normal CC: achristensen, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: Safari 12   
Hardware: Mac   
OS: macOS 10.14   
See Also: https://bugs.webkit.org/show_bug.cgi?id=198568
Attachments:
Description Flags
unparsed status line passed to WebSocketChannel::fail(m_handshake->failureReason()
none
inspector preview shows no parsed response none

Description Joey Korkames 2019-09-22 20:11:22 PDT
Created attachment 379352 [details]
unparsed status line passed to WebSocketChannel::fail(m_handshake->failureReason()

websocket servers that respond with truncated "h2 style" status lines like 'HTTP/1.1 101\r\n' are rejected by Safari:

$ curl -si 'https://web.voice.telephony.goog/websocket' -H 'Host: web.voice.telephony.goog' -H 'Upgrade: websocket' -H 'Connection: Upgrade' -H "Sec-WebSocket-Key: $(openssl rand -base64 16)" -H 'Origin: https://voice.google.com' -H 'Sec-WebSocket-Protocol: sip' -H 'Sec-WebSocket-Version: 13' | head -n1 | hexdump -c
0000000   H   T   T   P   /   1   .   1       1   0   1  \r  \n
000000e

I've cross reported this to the Google Voice forum:
https://support.google.com/voice/thread/14998073?msgid=14998073

That endpoint is for a product that specifically detects-and-degrades on Safari, but maybe that is not such an unusual kind of response line.

Chromium and Firefox use their canonical browsing parsers to validate the handshake's response, and theirs do not
seem to require a status text:
https://chromium.googlesource.com/chromium/chromium/+/fe2e997ac2990d3d5b9c8abe653776830aaf8c28/net/http/http_response_headers.cc#728
https://searchfox.org/mozilla-central/source/netwerk/protocol/http/nsHttpResponseHead.cpp#506

whereas webkit requires the status-code sent between two spaces:
https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/Modules/websockets/WebSocketHandshake.cpp?rev=249013#L453
Comment 1 Joey Korkames 2019-09-22 20:22:02 PDT
Created attachment 379353 [details]
inspector preview shows no parsed response
Comment 2 Radar WebKit Bug Importer 2019-09-23 10:27:24 PDT
<rdar://problem/55627726>
Comment 3 Joey Korkames 2019-09-24 10:45:52 PDT
autobahn.py's WS testsuite* seems to agree with Chrome & FF handshake impls, but not webkit's:
  # Response Line
  #
  sl = self.http_status_line.split()
  if len(sl) < 2:
     return self.failHandshake("Bad HTTP response status line '%s'" % self.http_status_line)

In [1]: len("HTTP/1.1 101".split())
Out[1]: 2

1: https://github.com/crossbario/autobahn-python/blob/f0d15f02735429e3f92ea56502b79b33acc65882/autobahn/websocket/protocol.py#L3674)


Its tyranny of the majority (of UAs), including Safari, if its using NSURLSession & I'm following it accurately:

https://opensource.apple.com/source/CFNetwork/CFNetwork-129.20/HTTP/CFHTTPStream.c.auto.html
httpReceiveResponse is just checking it got all the bytes it can get


nextActionForHeaders() calls CFHTTPMessageGetResponseStatusCode(CFMessageRef headers)...

https://opensource.apple.com/source/CFNetwork/CFNetwork-129.20/HTTP/CFHTTPMessage.c.auto.html

looks for the .flags[status] that *_extractResponseStatusLine got when initializing the CFHTTPMessage (after _parseHeadersFromData was fired on the last append of message's bytes)

*_extractResponseStatusLine(...) seems to parse for just the code numbers

Very hard to follow!

But this is borne out when trying to browse this server in Safari.

it's inspector says:
"Failed to load resource: the server responded with a status of 400 () -- https://web.voice.telephony.goog/favicon.ico"

showing CFNetwork does tolerate the truncated status line.

I suppose there's already a task open to convert webcore:websockets to NSURLSession or CFURLConnection?
Comment 4 Joey Korkames 2021-01-09 22:16:30 PST
The site in the initial report has fixed their status line to be HTTP1.1 conformant.

I don't have another candidate to test against.
Comment 5 youenn fablet 2021-01-11 00:44:41 PST
FWIW, WebKit Cocoa port is working on using NSURLSession WebSocket code path which seem to treat 'HTTP/1.1 101\r\n' as valid.
Comment 6 youenn fablet 2021-01-11 00:45:18 PST
Tested by modifying web socket python scripts like LayoutTests/http/tests/websocket/tests/hybi/handshake-fail-by-no-upgrade-header_wsh.py