Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix WebSocket bug with Firefox #13

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mikesart
Copy link
Contributor

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

@deplinenoise
Copy link
Owner

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

@mikesart
Copy link
Contributor Author

Yep - good catch. Try #2...

@deplinenoise
Copy link
Owner

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.

@mikesart
Copy link
Contributor Author

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.

@deplinenoise
Copy link
Owner

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.

@mikesart
Copy link
Contributor Author

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

@deplinenoise
Copy link
Owner

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.)

@mikesart
Copy link
Contributor Author

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...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants