Last Comment Bug 753179 - Websockets: never send 1005 close code on the network
: Websockets: never send 1005 close code on the network
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Networking: WebSockets (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla14
Assigned To: Jason Duell [:jduell] (needinfo? me)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-08 16:20 PDT by Jason Duell [:jduell] (needinfo? me)
Modified: 2012-05-21 19:55 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Don't reciprocate forbidden close codes. (1.40 KB, patch)
2012-05-08 16:20 PDT, Jason Duell [:jduell] (needinfo? me)
no flags Details | Diff | Review
v2: fix a couple silly errrors (1.40 KB, patch)
2012-05-11 17:30 PDT, Jason Duell [:jduell] (needinfo? me)
bugs: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta-
Details | Diff | Review

Description Jason Duell [:jduell] (needinfo? me) 2012-05-08 16:20:53 PDT
Created attachment 622195 [details] [diff] [review]
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 [:jduell] (needinfo? me) 2012-05-11 17:30:21 PDT
Created attachment 623371 [details] [diff] [review]
v2: fix a couple silly errrors

Yikes!  Important to use ==, not =, here :)
Comment 2 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-05-15 07:17:48 PDT
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] (high review load, please consider other reviewers) 2012-05-15 07:20:09 PDT
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] (high review load, please consider other reviewers) 2012-05-15 07:52:38 PDT
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 [:jduell] (needinfo? me) 2012-05-15 15:28:55 PDT
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] (high review load, please consider other reviewers) 2012-05-15 15:42:48 PDT
Comment on attachment 623371 [details] [diff] [review]
v2: fix a couple silly errrors

Ok, thanks for explanation
Comment 7 Jason Duell [:jduell] (needinfo? me) 2012-05-16 17:10:49 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5e203fd9ff9
Comment 8 Jason Duell [:jduell] (needinfo? me) 2012-05-16 17:14:08 PDT
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 [:jduell] (needinfo? me) 2012-05-16 17:16:26 PDT
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] 2012-05-17 03:13:37 PDT
https://hg.mozilla.org/mozilla-central/rev/c5e203fd9ff9
Comment 11 Lukas Blakk [:lsblakk] use ?needinfo 2012-05-21 15:19:12 PDT
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 [:jduell] (needinfo? me) 2012-05-21 15:51:01 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/1b541106975c

Note You need to log in before you can comment on or make changes to this bug.