From c97981403d02f4386da8fcb8840118c37a4caf93 Mon Sep 17 00:00:00 2001 From: Kasimir Torri Date: Fri, 2 Jul 2021 13:47:59 +0100 Subject: [PATCH] Improve `PerMessageDeflateClientExtensionHandler` (#11413) Motivation: The `PerMessageDeflateClientExtensionHandler` has the following strange behaviors currently: * The `requestedServerNoContext` parameter doesn't actually add the `server_no_context_takeover` parameter to the client offer; instead it depends on the requested server window size. * The handshake will fail if the server responds with a `server_no_context_takeover` parameter and `requestedServerNoContext` is false. According to RFC 7692 (7.1.1.1) the server may do this, and this means that to cover both cases one needs to use two handshakers in the channel pipeline: one with `requestedServerNoContext = true` and one with `requestedServerNoContext = false`. * The value of the `server_max_window_bits` parameter in the server response is never checked (should be between 8 and 15). And the value of `client_max_window_bits` is checked only in the branch handling the server window parameter. Modification: * Add the `server_no_context_takeover` parameter if `requestedServerNoContext` is true. * Accept a server handshake response which includes the server no context takeover parameter even if we did not request it. * Check the values of the client and server window size in their respective branches and fail the handshake if they are out of bounds. Result: There will be no need to use two handshakers in the pipeline to be lenient in what handshakes are accepted. --- ...ssageDeflateClientExtensionHandshaker.java | 13 ++--- ...eDeflateClientExtensionHandshakerTest.java | 57 +++++++++++++++++++ 2 files changed, 63 insertions(+), 7 deletions(-) diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/extensions/compression/PerMessageDeflateClientExtensionHandshaker.java b/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/extensions/compression/PerMessageDeflateClientExtensionHandshaker.java index 8498020c74..4157676cbb 100644 --- a/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/extensions/compression/PerMessageDeflateClientExtensionHandshaker.java +++ b/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/extensions/compression/PerMessageDeflateClientExtensionHandshaker.java @@ -117,7 +117,7 @@ public final class PerMessageDeflateClientExtensionHandshaker implements WebSock @Override public WebSocketExtensionData newRequestData() { HashMap parameters = new HashMap(4); - if (requestedServerWindowSize != MAX_WINDOW_SIZE) { + if (requestedServerNoContext) { parameters.put(SERVER_NO_CONTEXT, null); } if (allowClientNoContext) { @@ -153,13 +153,16 @@ public final class PerMessageDeflateClientExtensionHandshaker implements WebSock // allowed client_window_size_bits if (allowClientWindowSize) { clientWindowSize = Integer.parseInt(parameter.getValue()); + if (clientWindowSize > MAX_WINDOW_SIZE || clientWindowSize < MIN_WINDOW_SIZE) { + succeed = false; + } } else { succeed = false; } } else if (SERVER_MAX_WINDOW.equalsIgnoreCase(parameter.getKey())) { // acknowledged server_window_size_bits serverWindowSize = Integer.parseInt(parameter.getValue()); - if (clientWindowSize > MAX_WINDOW_SIZE || clientWindowSize < MIN_WINDOW_SIZE) { + if (serverWindowSize > MAX_WINDOW_SIZE || serverWindowSize < MIN_WINDOW_SIZE) { succeed = false; } } else if (CLIENT_NO_CONTEXT.equalsIgnoreCase(parameter.getKey())) { @@ -171,11 +174,7 @@ public final class PerMessageDeflateClientExtensionHandshaker implements WebSock } } else if (SERVER_NO_CONTEXT.equalsIgnoreCase(parameter.getKey())) { // acknowledged server_no_context_takeover - if (requestedServerNoContext) { - serverNoContext = true; - } else { - succeed = false; - } + serverNoContext = true; } else { // unknown parameter succeed = false; diff --git a/codec-http/src/test/java/io/netty/handler/codec/http/websocketx/extensions/compression/PerMessageDeflateClientExtensionHandshakerTest.java b/codec-http/src/test/java/io/netty/handler/codec/http/websocketx/extensions/compression/PerMessageDeflateClientExtensionHandshakerTest.java index 90d1692531..031321f24a 100644 --- a/codec-http/src/test/java/io/netty/handler/codec/http/websocketx/extensions/compression/PerMessageDeflateClientExtensionHandshakerTest.java +++ b/codec-http/src/test/java/io/netty/handler/codec/http/websocketx/extensions/compression/PerMessageDeflateClientExtensionHandshakerTest.java @@ -130,6 +130,63 @@ public class PerMessageDeflateClientExtensionHandshakerTest { assertNull(extension); } + @Test + public void testParameterValidation() { + WebSocketClientExtension extension; + Map parameters; + + PerMessageDeflateClientExtensionHandshaker handshaker = + new PerMessageDeflateClientExtensionHandshaker(6, true, 15, true, false); + + parameters = new HashMap(); + parameters.put(CLIENT_MAX_WINDOW, "15"); + parameters.put(SERVER_MAX_WINDOW, "8"); + extension = handshaker.handshakeExtension(new WebSocketExtensionData(PERMESSAGE_DEFLATE_EXTENSION, parameters)); + + // Test that handshake succeeds when parameters are valid + assertNotNull(extension); + assertEquals(RSV1, extension.rsv()); + assertTrue(extension.newExtensionDecoder() instanceof PerMessageDeflateDecoder); + assertTrue(extension.newExtensionEncoder() instanceof PerMessageDeflateEncoder); + + parameters = new HashMap(); + parameters.put(CLIENT_MAX_WINDOW, "15"); + parameters.put(SERVER_MAX_WINDOW, "7"); + + extension = handshaker.handshakeExtension(new WebSocketExtensionData(PERMESSAGE_DEFLATE_EXTENSION, parameters)); + + // Test that handshake fails when parameters are invalid + assertNull(extension); + } + + @Test + public void testServerNoContextTakeover() { + WebSocketClientExtension extension; + Map parameters; + + PerMessageDeflateClientExtensionHandshaker handshaker = + new PerMessageDeflateClientExtensionHandshaker(6, true, 15, true, false); + + parameters = new HashMap(); + parameters.put(SERVER_NO_CONTEXT, null); + extension = handshaker.handshakeExtension(new WebSocketExtensionData(PERMESSAGE_DEFLATE_EXTENSION, parameters)); + + // Test that handshake succeeds when server responds with `server_no_context_takeover` that we didn't offer + assertNotNull(extension); + assertEquals(RSV1, extension.rsv()); + assertTrue(extension.newExtensionDecoder() instanceof PerMessageDeflateDecoder); + assertTrue(extension.newExtensionEncoder() instanceof PerMessageDeflateEncoder); + + // initialize + handshaker = new PerMessageDeflateClientExtensionHandshaker(6, true, 15, true, true); + + parameters = new HashMap(); + extension = handshaker.handshakeExtension(new WebSocketExtensionData(PERMESSAGE_DEFLATE_EXTENSION, parameters)); + + // Test that handshake fails when client offers `server_no_context_takeover` but server doesn't support it + assertNull(extension); + } + @Test public void testDecoderNoClientContext() { PerMessageDeflateClientExtensionHandshaker handshaker =