Bug 87084 – [WebSocket] Receiving reserved close codes, 1005, 1006, and 1015 must appear as code=1006 and wasClean=false

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Takashi Toyoshima]

A hybi thread is here.
http://www.ietf.org/mail-archive/web/hybi/current/msg09634.html

Status codes 1005, 1006, and 1015 are forbidden to be sent as status code of close frame.
If we receive a close frame containing them, we should handle it as a broken frame.
It means that this close frame invoke CloseEvent with code=1006, wasClean=false.

[Takashi Toyoshima]

Created attachment 143224 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Yuta Kitamura]

Comment on attachment 143224 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=143224&action=review

> Source/WebCore/Modules/websockets/WebSocketChannel.cpp:676
> +          m_closeEventCode = CloseEventCodeNoStatusRcvd;

Bad indentation. This happens in two other places in this file.

> Source/WebCore/Modules/websockets/WebSocketChannel.cpp:678
> +          m_closeEventCode = CloseEventCodeAbnormalClosure;

Can't this be fail()?

> Source/WebCore/Modules/websockets/WebSocketChannel.cpp:683
> +            if (m_closeEventCode == CloseEventCodeNoStatusRcvd || m_closeEventCode == CloseEventCodeTlsHandshake)

(This is actually not from this change, but anyway) we don't usually use abbreviations like "Rcvd".

> Source/WebCore/Modules/websockets/WebSocketChannel.h:108
> +        CloseEventCodeMandatoryExt = 1010,

Ext -> Extension?

I'd like to say this as "CloseEventCodeMissingRequiredExtension" to clarify the meaning.

> Source/WebCore/Modules/websockets/WebSocketChannel.h:110
> +        CloseEventCodeTlsHandshake = 1015,

Tls -> TLS? See http://www.webkit.org/coding/coding-style.html#names

I'd like to name this "CloseEventCodeTLSHandshakeFailure"

[James Robinson]

Comment on attachment 143224 [details]
Patch

Chromium WebKit API changes lgtm.

[Takashi Toyoshima]

Comment on attachment 143224 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=143224&action=review

>> Source/WebCore/Modules/websockets/WebSocketChannel.cpp:676
>> +          m_closeEventCode = CloseEventCodeNoStatusRcvd;
> 
> Bad indentation. This happens in two other places in this file.

Fixed.
Thanks.

>> Source/WebCore/Modules/websockets/WebSocketChannel.cpp:678
>> +          m_closeEventCode = CloseEventCodeAbnormalClosure;
> 
> Can't this be fail()?

Good point.
I fix this to call fail().

>> Source/WebCore/Modules/websockets/WebSocketChannel.cpp:683
>> +            if (m_closeEventCode == CloseEventCodeNoStatusRcvd || m_closeEventCode == CloseEventCodeTlsHandshake)
> 
> (This is actually not from this change, but anyway) we don't usually use abbreviations like "Rcvd".

Actually, these enum names follows RFC6455 defined name exactly.
See 11.7 of http://tools.ietf.org/html/rfc6455

> Source/WebCore/Modules/websockets/WebSocketChannel.cpp:684
> +              m_closeEventCode = CloseEventCodeAbnormalClosure;

I should call fail() here too.

>> Source/WebCore/Modules/websockets/WebSocketChannel.h:108
>> +        CloseEventCodeMandatoryExt = 1010,
> 
> Ext -> Extension?
> 
> I'd like to say this as "CloseEventCodeMissingRequiredExtension" to clarify the meaning.

ditto.

>> Source/WebCore/Modules/websockets/WebSocketChannel.h:110
>> +        CloseEventCodeTlsHandshake = 1015,
> 
> Tls -> TLS? See http://www.webkit.org/coding/coding-style.html#names
> 
> I'd like to name this "CloseEventCodeTLSHandshakeFailure"

I agreed on s/Tls/TLS/.
But this name also follows the spec.

[Takashi Toyoshima]

Created attachment 143740 [details]
Patch

[Yuta Kitamura]

Comment on attachment 143224 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=143224&action=review

>>> Source/WebCore/Modules/websockets/WebSocketChannel.cpp:683
>>> +            if (m_closeEventCode == CloseEventCodeNoStatusRcvd || m_closeEventCode == CloseEventCodeTlsHandshake)
>> 
>> (This is actually not from this change, but anyway) we don't usually use abbreviations like "Rcvd".
> 
> Actually, these enum names follows RFC6455 defined name exactly.
> See 11.7 of http://tools.ietf.org/html/rfc6455

Understood.

[Yuta Kitamura]

Comment on attachment 143740 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=143740&action=review

looks good.

> Source/WebCore/Modules/websockets/WebSocketChannel.cpp:679
> +            fail("Receive a broken close frame containing an invalid size body.");

nit: "Received"

> Source/WebCore/Modules/websockets/WebSocketChannel.cpp:687
> +                fail("Receive a broken close frame containing a reserved status code.");

nit: "Received"

> LayoutTests/http/tests/websocket/tests/hybi/close-code-and-reason.html:101
> +    shouldBe("closeEvent.wasClean", "expectedWasClean[testId]");

Optional suggestion: I suggest the following:
    shouldEvaluateTo("closeEvent.wasClean", expectedWasClean[testId]);
which produces more readable output.

[Takashi Toyoshima]

Created attachment 143993 [details]
Patch

[Takashi Toyoshima]

Comment on attachment 143740 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=143740&action=review

>> Source/WebCore/Modules/websockets/WebSocketChannel.cpp:679
>> +            fail("Receive a broken close frame containing an invalid size body.");
> 
> nit: "Received"

fixed.

>> Source/WebCore/Modules/websockets/WebSocketChannel.cpp:687
>> +                fail("Receive a broken close frame containing a reserved status code.");
> 
> nit: "Received"

fixed.

>> LayoutTests/http/tests/websocket/tests/hybi/close-code-and-reason.html:101
>> +    shouldBe("closeEvent.wasClean", "expectedWasClean[testId]");
> 
> Optional suggestion: I suggest the following:
>     shouldEvaluateTo("closeEvent.wasClean", expectedWasClean[testId]);
> which produces more readable output.

Thanks.
I didn't know that. It looks better than current code.
Fixed.

[Kent Tamura]

Comment on attachment 143993 [details]
Patch

rubber stamped.

[WebKit Review Bot]

Comment on attachment 143993 [details]
Patch

Clearing flags on attachment: 143993

Committed r118723: <http://trac.webkit.org/changeset/118723>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.