XRPLF / rippled

Decentralized cryptocurrency blockchain daemon implementing the XRP Ledger protocol in C++
https://xrpl.org
ISC License
4.51k stars 1.46k forks source link

WebSocket server header keys should have correct capitalization #116

Closed CodeShark closed 7 years ago

CodeShark commented 11 years ago

Upon trying to connect to rippled from several different websocket clients, I noticed that some clients refused to connect to rippled while having no problem connecting to other servers. Upon looking at the HTTP headers, the reason became clear: rippled is using a version of websocketpp that uses tolower on all the header keys.

0x0000: 4c72 b9b1 e460 108c cf28 fe40 0800 4500 Lr...`...(.@..E. 0x0010: 00d3 fcf1 4000 2b06 4d22 1717 882e b01f ....@.+.M"...... 0x0020: b5ac c821 de71 ecf1 2c63 8a25 b20b 8018 ...!.q..,c.%.... 0x0030: 003d 210a 0000 0101 080a 2b66 d093 7274 .=!.......+f..rt 0x0040: fa40 4854 5450 2f31 2e31 2031 3031 2053 .@HTTP/1.1.101.S 0x0050: 7769 7463 6869 6e67 2050 726f 746f 636f witching.Protoco 0x0060: 6c73 0d0a 636f 6e6e 6563 7469 6f6e 3a20 ls..connection:. 0x0070: 5570 6772 6164 650d 0a73 6563 2d77 6562 Upgrade..sec-web 0x0080: 736f 636b 6574 2d61 6363 6570 743a 2032 socket-accept:.2 0x0090: 6b45 5348 4145 7662 3752 5055 6f33 354b kESHAEvb7RPUo35K 0x00a0: 6644 6b55 7757 4547 756f 3d0d 0a73 6572 fDkUwWEGuo=..ser 0x00b0: 7665 723a 2057 6562 536f 636b 6574 2b2b ver:.WebSocket++ 0x00c0: 2f30 2e32 2e31 6465 760d 0a75 7067 7261 /0.2.1dev..upgra 0x00d0: 6465 3a20 7765 6273 6f63 6b65 740d 0a0d de:.websocket...

If I understand rfc6455 page 18 correctly (http://tools.ietf.org/html/rfc6455#section-4.1), the HTTP header keys require correct capitalzation (Upgrade, not upgrade; Sec-WebSocket-Accept, not sec-websocket-accept; etc...) but the values are case-insensitive (i.e. in the server's response to the client's opening handshake, Upgrade: websocket is just as valid as Upgrade: wEbsoCKet).

Many websocket clients, nonetheless, do not care about the key capitalization (i.e. node ws/wscat/chrome/safari) and so will not cause problems. However, other websocket clients (i.e. some versions of websocketpp) will.

vinniefalco commented 11 years ago

RFC 6455 Page 19: "Please note that according to [RFC2616], all header field names in both HTTP requests and HTTP responses are case-insensitive."

So while in theory the field names are case-insensitive, it sounds like there are some client implementations (notably, earlier versions of websocketpp) which treat them as case-sensitive (an error). Since we can't force people to use perfectly functioning software, it might be in our best interests to output the header field names using the capitalization specified in RFC 6455 instead of relying on the leeway allowed by RFC 2616.

CodeShark commented 11 years ago

Yeah, apparently according to HTTP standards the header key capitalization shouldn't matter. So RFC6455 is overly strict (or perhaps just not sufficiently clear on this point).

I think the best policy is to use conventional capitalization (Upgrade, not upgrade) when writing headers but to accept others' headers without paying attention to case.

CodeShark commented 11 years ago

Pull Request #117 doesn't really deal with all cases - it only deals with either first-letter-of-each-word-is-capital or all-lowercase. It might be a good idea to store everything as lowercase internally but capitalize the first letter of every word when writing a header to the network.

vinniefalco commented 11 years ago

That's a larger change. Why can't we just make sure that our header field names are canonical in the first place?

Or are you referring to parsing incoming headers, in which case why don't we just use a case-insensitive comparison?

CodeShark commented 11 years ago

The problem is if a client gives us something like cOnnection: upgrade it will break. That won't happen if we always store the keys lowercase but just capitalize the first letter of each word (each character a-z that does not have another character a-z to its immediate left) when writing the header keys to the network. (i.e. sed 's/<./\U&/g')

CodeShark commented 11 years ago

Oh, bah...that won't work...Sec-WebSocket-Version. I guess we just have special cases for each of the required headers.

rec commented 9 years ago

I don't think we're going to change this - feel free to reopen with more discussion...

vinniefalco commented 7 years ago

Obsolete, our websocket implementation is rewritten.