Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bugfix #8295: WebSocket is closed without an error on protocol violations #9116

Conversation

@ursaj
Copy link
Contributor

commented May 1, 2019

Motivation:

  • Incorrect WebSockets closure affects our production system.
  • Enforced 'close socket on any protocol violation' prevents our custom termination sequence from execution.
  • Huge number of parameters is a nightmare both in usage and in support (decoders configuration).

Modification:

  • Fix violations handling - send proper response codes.
  • Fix for messages leak.
  • Introduce decoder's option to disable default behavior (send close frame) on protocol violations.
  • Encapsulate WebSocket response codes - WebSocketCloseStatus.
  • Encapsulate decoder's configuration into a separate class - WebSocketDecoderConfig.

Result:

Fixes #8295.

Note: previous PR #8308 got stuck - I cannot get any response from reviewers... Reopened it as a new PR (this one).

@netty-bot

This comment has been minimized.

Copy link

commented May 1, 2019

Can one of the admins verify this patch?

@ursaj ursaj changed the title Bugfix 8295/websocket is closed without an error on protocol violations 2 Bugfix #8295: WebSocket is closed without an error on protocol violations May 1, 2019

@@ -0,0 +1,64 @@
/*
* Copyright 2018 The Netty Project

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer May 27, 2019

Member

you will need to update all the headers to 2019

This comment has been minimized.

Copy link
@ursaj

ursaj Jun 8, 2019

Author Contributor

done

this(status, null, cause);
}

public WebSocketCloseStatus getCloseStatus() {

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer May 27, 2019

Member

we usually not use get...., just name it closeStatus()

This comment has been minimized.

Copy link
@ursaj

ursaj Jun 8, 2019

Author Contributor

done

Show resolved Hide resolved ...java/io/netty/handler/codec/http/websocketx/WebSocket08FrameDecoder.java
throw new IllegalArgumentException(
"WebSocket close status code does NOT comply with RFC-6455: " + statusCode);
}
if (reasonText == null) {

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer May 27, 2019

Member

use ObjectUtil.checkNotNull(...)

This comment has been minimized.

Copy link
@ursaj

ursaj Jun 8, 2019

Author Contributor

done

return allowExtensions;
}

boolean closeOnProtocolViolation() {

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer May 27, 2019

Member

why are all methods public except this one ?

This comment has been minimized.

Copy link
@ursaj

ursaj Jun 8, 2019

Author Contributor

unified - changed to public

Show resolved Hide resolved ...o/netty/handler/codec/http/websocketx/WebSocket08EncoderDecoderTest.java
private void transfer(EmbeddedChannel outChannel, EmbeddedChannel inChannel) {
// Transfer encoded data into decoder
// Loop because there might be multiple frames (gathering write)
while (true) {

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer May 27, 2019

Member

please use the same style we do in Netty: for (;;) { ... }

This comment has been minimized.

Copy link
@ursaj

ursaj Jun 8, 2019

Author Contributor

done

@ursaj ursaj force-pushed the ursaj:bugfix-8295/websocket-is-closed-without-an-error-on-protocol-violations-2 branch from c900e8c to 51f62b1 Jun 8, 2019

@ursaj ursaj force-pushed the ursaj:bugfix-8295/websocket-is-closed-without-an-error-on-protocol-violations-2 branch from 51f62b1 to da7cd53 Jun 8, 2019

@ursaj

This comment has been minimized.

Copy link
Contributor Author

commented Jun 13, 2019

Everything has been fixed a week ago. Any chance for approve and merge?

@normanmaurer

This comment has been minimized.

Copy link
Member

commented Jun 14, 2019

@netty-bot test this please...

@normanmaurer

This comment has been minimized.

Copy link
Member

commented Jun 14, 2019

Before pulling this in I would love to have either @slandelle / @ejona86 or @trustin take a look as well.

@normanmaurer normanmaurer added this to the 4.1.37.Final milestone Jun 14, 2019

Show resolved Hide resolved ...etty/handler/codec/http/websocketx/CorruptedWebSocketFrameException.java
Show resolved Hide resolved ...etty/handler/codec/http/websocketx/CorruptedWebSocketFrameException.java
Show resolved Hide resolved ...etty/handler/codec/http/websocketx/CorruptedWebSocketFrameException.java
Show resolved Hide resolved ...java/io/netty/handler/codec/http/websocketx/WebSocket08FrameDecoder.java
Show resolved Hide resolved ...in/java/io/netty/handler/codec/http/websocketx/WebSocketCloseStatus.java
Show resolved Hide resolved ...in/java/io/netty/handler/codec/http/websocketx/WebSocketCloseStatus.java
@Override
public String toString() {
// E.g.: "1000 Bye", "1009 Message too big"
return code() + " " + reasonText();

This comment has been minimized.

Copy link
@slandelle

slandelle Jun 14, 2019

Contributor

final field to it's computed once and for all, as most values should be constants

This comment has been minimized.

Copy link
@ursaj

ursaj Jun 16, 2019

Author Contributor

fixed. copied from io.netty.handler.codec.dns.DnsOpCode.

@normanmaurer

This comment has been minimized.

Copy link
Member

commented Jun 16, 2019

@netty-bot test this please

@normanmaurer

This comment has been minimized.

Copy link
Member

commented Jun 17, 2019

@slandelle PTAL again

@normanmaurer normanmaurer merged commit 7fc718d into netty:4.1 Jun 18, 2019

2 of 3 checks passed

pull request validation (centos6-java11) Build finished.
Details
pull request validation (centos6-java12) Build finished.
Details
pull request validation (centos6-java8) Build finished.
Details
@normanmaurer

This comment has been minimized.

Copy link
Member

commented Jun 18, 2019

@ursaj thanks a lot!

normanmaurer added a commit that referenced this pull request Jun 18, 2019

WebSocket is closed without an error on protocol violations (#9116)
Motivation:

Incorrect WebSockets closure affects our production system.
Enforced 'close socket on any protocol violation' prevents our custom termination sequence from execution.
Huge number of parameters is a nightmare both in usage and in support (decoders configuration).

Modification:

- Fix violations handling - send proper response codes.
- Fix for messages leak.
- Introduce decoder's option to disable default behavior (send close frame) on protocol violations.
- Encapsulate WebSocket response codes - WebSocketCloseStatus.
- Encapsulate decoder's configuration into a separate class - WebSocketDecoderConfig.

Result:

Fixes #8295.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.