Websockets: never send 1005 close code on the network

RESOLVED FIXED in mozilla14

Status

()

Core
Networking: WebSockets
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jduell, Assigned: jduell)

Tracking

unspecified
mozilla14
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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.
Attachment #622195 - Flags: review?(bugs)
(Assignee)

Comment 1

5 years ago
Created attachment 623371 [details] [diff] [review]
v2: fix a couple silly errrors

Yikes!  Important to use ==, not =, here :)
Attachment #622195 - Attachment is obsolete: true
Attachment #622195 - Flags: review?(bugs)
Attachment #623371 - Flags: review?(bugs)

Comment 2

5 years ago
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

5 years ago
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

5 years ago
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?
Attachment #623371 - Flags: review?(bugs) → review-
(Assignee)

Comment 5

5 years ago
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.
Attachment #623371 - Flags: review- → review?(bugs)

Comment 6

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

Ok, thanks for explanation
Attachment #623371 - Flags: review?(bugs) → review+
(Assignee)

Comment 7

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5e203fd9ff9
(Assignee)

Comment 8

5 years ago
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
Attachment #623371 - Flags: approval-mozilla-beta?
Attachment #623371 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 9

5 years ago
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.
https://hg.mozilla.org/mozilla-central/rev/c5e203fd9ff9
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
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.
Attachment #623371 - Flags: approval-mozilla-beta?
Attachment #623371 - Flags: approval-mozilla-beta-
Attachment #623371 - Flags: approval-mozilla-aurora?
Attachment #623371 - Flags: approval-mozilla-aurora+
(Assignee)

Comment 12

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/1b541106975c
(Assignee)

Updated

5 years ago
Target Milestone: mozilla15 → mozilla14
You need to log in before you can comment on or make changes to this bug.