WebSocket connection error with Apple Safari

[pontscho]

Hi,

first of all thanks for clarifying my previous patch about WebSocket ping.

I found and another not-too-fun "issue" with Lwan's web socket implementation, which isn't a real bug in Lwan but in Apple Safari. For some unknown reasons it isn't send Upgrade header when it starts a new connection but if I comment out lines from lwan-request.c:1344-1347:

     const char *upgrade = lwan_request_get_header(request, "Upgrade");
     if (UNLIKELY(!upgrade || !streq(upgrade, "websocket")))
         return HTTP_BAD_REQUEST;

helps establish web socket connection with Lwan without any other problem.

Commenting out these lines is just a workaround, not a real solution but helps to interoperatibihity between lwan and Safari and strict check isn’t get injured because the next few lines are used to check the websocket connection. If the sec-websocket-key field is missing, it either means a total error on any ways.

What is your opinion about this case ?

pontscho

[lpereira]

This is pretty curious. Are you testing it directly or over some proxy? Have you looked at the raw headers before we parse them?

The Upgrade method is necessary for websockets according to the RFC, so I don't really know if we should remove the check for it. Have you tried other websockets implementations?

On Wed, May 4, 2022, at 6:49 AM, pontscho wrote:

Hi,

first of all thanks for clarifying my previous patch about WebSocket ping.

I found and another not-too-fun "issue" with Lwan's web socket implementation, which isn't a real bug in Lwan but in Apple Safari. For some unknown reasons it isn't send Upgrade header when it starts a new connection but if I comment out lines from lwan-request.c:1344-1347 helps establish web socket connection with Lwan without any other problem.

Commenting out these lines is just a workaround, not a real solution but helps to interoperatibihity between lwan and Safari and strict check isn’t get injured because the next few lines are used to check the websocket connection. If the sec-websocket-key field is missing, it either means a total error on any ways.

What is your opinion about this case ?

pontscho

— Reply to this email directly, view it on GitHub https://github.com/lpereira/lwan/issues/345, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAADVGO7PPIG7OJP2VJ7VDDVIJ55BANCNFSM5VCBC4TQ. You are receiving this because you are subscribed to this thread.Message ID: @.***>

[pontscho]

I've tested it directly with Lwan and yes, I took a look those headers.I've tested it directly with Lwan and yes, I took a look those headers and blowed my mind because Upgrade header was missing from the original request from Safari and everything worked well with Chrome.

The Upgrade method is necessary for websockets according to the RFC, so I don't really know if we should remove the check for it. Have you tried other websockets implementations?

Yes, that's why I'm doubled... I used websocat and openresty’s / chrome's / firefox's ws client implemlementations for testing and everything went fine except Safari.

[lpereira]

Does it have a Sec-WebSocket-Version header? Is the value different than what other clients send?

Do other servers work with Safari even without this header?

On Wed, May 4, 2022, at 3:50 PM, pontscho wrote:

I've tested it directly with Lwan and yes, I took a look those headers.I've tested it directly with Lwan and yes, I took a look those headers and blowed my mind because Upgrade header was missing from the original request from Safari and everything worked well with Chrome.

The Upgrade method is necessary for websockets according to the RFC, so I don't really know if we should remove the check for it. Have you tried other websockets implementations?

Yes, that's why I'm doubled... I used websocat and openresty’s / chrome's / firefox's ws client implemlementations for testing and everything went fine except Safari.

— Reply to this email directly, view it on GitHub https://github.com/lpereira/lwan/issues/345#issuecomment-1117516609, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAADVGITB64ZOE63SOJXJ2TVIKME7ANCNFSM5VCBC4TQ. You are receiving this because you commented.Message ID: @.***>

[pontscho]

Does it have a Sec-WebSocket-Version header? Is the value different than what other clients send?

Yes, with a correct value. It had every necessary headers except the Upgrade.

Do other servers work with Safari even without this header?

I used nginx before for tls termination and it worked but I overwrote http headers with it. I will check it again.

[lpereira]

I just tried the websocket demo with Safari 15.4 here and ot seems to work! The Upgrade header is sent as expected. Maybe you're using an old version?

I checked other servers, like:

• gorilla/websocket
• libwebsockets
• tungstenite-rs

And all of these seem to require the presence of this header.

Now, if removing the check is needed for interoperability, then that's probably OK, although I'd prefer working around broken clients if at all possible.

On Wed, May 4, 2022, at 11:24 AM, pontscho wrote:

Does it have a Sec-WebSocket-Version header? Is the value different than what other clients send?

Yes, with a correct value. It had every necessary headers except the Upgrade.

Do other servers work with Safari even without this header?

I used nginx before for tls termination and it worked but I overwrote http headers with it. I will check it again.

— Reply to this email directly, view it on GitHub https://github.com/lpereira/lwan/issues/345#issuecomment-1117664980, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAADVGIPWTKOVH5SLVA5HHTVIK6GJANCNFSM5VCBC4TQ. You are receiving this because you commented.Message ID: @.***>

[pontscho]

I just tried the websocket demo with Safari 15.4 here and ot seems to work!

I've tested it with Safari 13.1.4, it's quite old but significant number of users use have this version. I've tested it again with other sites that uses websocket and that header is still missing. Apple quality as well ... :) Only reason why I didn't see this error before I use ngx and it sent that Upgrade field, hided this problem.

Now, if removing the check is needed for interoperability, then that's probably OK, although I'd prefer working around broken clients if at all possible.

Thanks.