webby icon indicating copy to clipboard operation
webby copied to clipboard

Published 20 hours ago • verified publisher icon deplinenoise

Fix WebSocket bug with Firefox

Open mikesart opened this issue 10 years ago • 8 comments

Creating Websocket with Firefox v28.0 on Linux was failing because Connection header was "keep-alive, Upgrade" and code was looking for "Upgrade".

mikesart avatar Mar 25 '14 05:03 mikesart

I think this change breaks legitimately passing "upgrade" (i.e. in lower case.) strstr() is case sensitive.

deplinenoise avatar Mar 25 '14 18:03 deplinenoise

Yep - good catch. Try #2...

mikesart avatar Mar 26 '14 19:03 mikesart

Maybe it would be cleaner to just lowercase everything as we pull it in, and do away with these slow case-insensitive functions? Also, you seem to have accidentally snuck in another unrelated change here in the pull request.

deplinenoise avatar Mar 26 '14 21:03 deplinenoise

Maybe it would be cleaner to just lowercase everything as we pull it in, and do away with these slow case-insensitive functions?

Lowercase all the headers as they come in or just specific headers like "Connection"? Happy to switch over to doing that - it was just more of a global change and I'm not up on all the RFCs to know whether that's legal everywhere or legal/fast on specific headers or what.

Also, you seem to have accidentally snuck in another unrelated change here in the pull request.

Sorry - that should have been in another branch. Moved it over and created another pull request.

mikesart avatar Mar 26 '14 21:03 mikesart

I was thinking maybe just lowercasing a few of the troublesome ones like this one as we tokenize the data. But maybe that's even uglier? Not sure.

Looking at your code in the mean while, I think I still see a problem. If Connection is set to UpdateFooBar it will be match the Update prefix and return a false positive.

deplinenoise avatar Mar 27 '14 01:03 deplinenoise

Is it just whitespace, comma, or nil that could end the token?

mikesart avatar Mar 27 '14 05:03 mikesart

I don't know; I thought the header was supposed to just say "Upgrade" in this case. Can you dig a little in the RFCs and find out? (Sorry; I'm under water with other projects right now.)

deplinenoise avatar Mar 28 '14 20:03 deplinenoise

The WebSocket Protocol includes these phrases:

  • If the response lacks a |Connection| header field or the |Connection| header field doesn't contain a token ...
  • A |Connection| header field that includes the token "Upgrade" ...
  • A |Connection| header field with value "Upgrade".

That appears to leave room for tokens other than just Upgrade. For whatever it's worth, the Qt test suite sets Connection to "Upgrade,keepalive". (Which I think is a bug - should be Keep-Alive.)

https://qt.gitorious.org/qt/qtwebsockets/source/ea7c77e87f317cd72278ba4f9955b74b9cdfeedf:tests/auto/handshakerequest/tst_handshakerequest.cpp

Documention for Connection and Keep-Alive:

http://tools.ietf.org/html/rfc2068#section-19.7.1

Token grammar documentation: http://www.packetizer.com/rfc/rfc2616/

I'll go through this when I get unburied a bit...

mikesart avatar Apr 17 '14 00:04 mikesart