753179 - Websockets: never send 1005 close code on the network

Status: RESOLVED FIXED

(Core :: Networking: WebSockets, defect)

Description

[Jason Duell]

Attachment: Don't reciprocate forbidden close codes.

We've been calling close with whatever code we get from the WebsocketChannel.  But if a server sends an empty close frame, we get code 1005, but should not reply with that value--it's expressly forbidden in the spec.  Added checks for other verboten close codes too, though AFACIT we haven't been putting them on the network since we get them when the network layer has failed.

Comment 1

[Jason Duell]

Attachment: v2: fix a couple silly errrors

Yikes!  Important to use ==, not =, here :)

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 623371 [details] [diff] [review]
v2: fix a couple silly errrors

I don't understand this.
What in the spec says those codes shouldn't be reported?
And what says the reason should be empty string?

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 623371 [details] [diff] [review]
v2: fix a couple silly errrors

Er, ok, I understand the 'code' part, but
not the 'reason' handling. Why empty string?

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 623371 [details] [diff] [review]
v2: fix a couple silly errrors

Er, no, I don't understand. What says that the code/reason shouldn't
propagate up to API level?

Comment 5

[Jason Duell]

Comment on attachment 623371 [details] [diff] [review]
v2: fix a couple silly errrors

Review of attachment 623371 [details] [diff] [review]:
-----------------------------------------------------------------

> What in the spec says those codes shouldn't be reported?

s/reported/sent over the network/:

  http://tools.ietf.org/html/rfc6455#section-7.4.1

> What says that the code/reason shouldn't propagate up to API level?

Not sure what you mean: the 1005/1006/1015 will propagate up to the client JS 'onclose' event, so we do report them to the websocket (see where mCloseEvent{Code|Reason} are set, just above my code changes).  The code I'm changing only prevents us from sending those codes over the network in our reply to a server-initiated close (the general rule is that an endpoint should reply to a incoming close with a outgoing close that has the same code/reason as in the received close: but for all these codes we didn't actually receive a network frame with that code--they are pseudo-codes--and we must not reply with them).

There's no reason to send anything other than an empty string (i.e. don't send any 'reason' bytes on the network), because 1) 1005 means we got no close code/reason from the server, so we should reciprocate with an empty close frame, and 2) 1006 means we didn't get a valid close frame at all from the server, so again, there's no 'reason' value to reciprocate.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 623371 [details] [diff] [review]
v2: fix a couple silly errrors

Ok, thanks for explanation

Comment 7

[Jason Duell]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c5e203fd9ff9

Comment 8

[Jason Duell]

Comment on attachment 623371 [details] [diff] [review]
v2: fix a couple silly errrors

[Approval Request Comment]

~0% risk patch: just corrects our websocket close logic to never send final close frame back to server with close codes that are specifically prohibited in the spec.   Does not cause any errors in practice, but would be nice to have our WS stack be RFC-compliant ASAP, and I don't see any risk at all in the patch.

User impact if declined:  some servers may consider our responses to be erroneous, but since it's the final close frame, they can't do much about it.
Testing completed (on m-c, etc.):  Observed old error and new fix with pywebsocket test server: we can't automate a test easily.
Risk to taking this patch (and alternatives if risky): very very low.
String or UUID changes made by this patch:  none

Comment 9

[Jason Duell]

FYI: w/o this fix, any time a server sends us an empty close frame (fairly common), we'll reply to it with a technically illegal 1005 frame.  So this is low consequence but fairly high frequency.

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/c5e203fd9ff9

Comment 11

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 623371 [details] [diff] [review]
v2: fix a couple silly errrors

[Triage Comment]
we're only taking critical fixes for beta at this point so only approving for aurora.

Comment 12

[Jason Duell]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1b541106975c