Harden WebSocket transport and permessage-deflate for RFC 6455/7692 conformance

httpclient5-testing/src/test/java/org/apache/hc/client5/testing/websocket/H2WebSocketEchoIT.java

@@ -169,7 +169,6 @@ void echoesOverHttp2ExtendedConnectWithPmce() throws Exception {
         final WebSocketClientConfig cfg = WebSocketClientConfig.custom()
                 .enableHttp2(true)
                 .enablePerMessageDeflate(true)
-                .offerClientMaxWindowBits(15)
                 .setCloseWaitTimeout(Timeout.ofSeconds(2))
                 .build();
 

httpclient5-testing/src/test/java/org/apache/hc/client5/testing/websocket/WebSocketClientTest.java

@@ -186,7 +186,6 @@ void echo_compressed_pmce() throws Exception {
                 .enablePerMessageDeflate(true)
                 .offerServerNoContextTakeover(true)
                 .offerClientNoContextTakeover(true)
-                .offerClientMaxWindowBits(15)
                 .build();
 
         try (final CloseableWebSocketClient client = WebSocketClients.createDefault()) {

httpclient5-testing/src/test/java/org/apache/hc/client5/testing/websocket/performance/WsPerfHarness.java

@@ -166,7 +166,6 @@ private static void runClient(
             b.enablePerMessageDeflate(true)
                     .offerClientNoContextTakeover(false)
                     .offerServerNoContextTakeover(false)
-                    .offerClientMaxWindowBits(null)
                     .offerServerMaxWindowBits(null);
         }
         final WebSocketClientConfig cfg = b.build();

httpclient5-websocket/src/main/java/org/apache/hc/client5/http/websocket/api/WebSocketClientConfig.java

@@ -55,7 +55,6 @@ public final class WebSocketClientConfig {
     private final boolean perMessageDeflateEnabled;
     private final boolean offerServerNoContextTakeover;
     private final boolean offerClientNoContextTakeover;
-    private final Integer offerClientMaxWindowBits;
     private final Integer offerServerMaxWindowBits;
 
     // Framing / flow
@@ -82,7 +81,6 @@ private WebSocketClientConfig(
             final boolean perMessageDeflateEnabled,
             final boolean offerServerNoContextTakeover,
             final boolean offerClientNoContextTakeover,
-            final Integer offerClientMaxWindowBits,
             final Integer offerServerMaxWindowBits,
             final int maxFrameSize,
             final int outgoingChunkSize,
@@ -102,7 +100,6 @@ private WebSocketClientConfig(
         this.perMessageDeflateEnabled = perMessageDeflateEnabled;
         this.offerServerNoContextTakeover = offerServerNoContextTakeover;
         this.offerClientNoContextTakeover = offerClientNoContextTakeover;
-        this.offerClientMaxWindowBits = offerClientMaxWindowBits;
         this.offerServerMaxWindowBits = offerServerMaxWindowBits;
         this.maxFrameSize = maxFrameSize;
         this.outgoingChunkSize = outgoingChunkSize;
@@ -169,19 +166,6 @@ public boolean isOfferClientNoContextTakeover() {
         return offerClientNoContextTakeover;
     }
 
-    /**
-     * Optional value for {@code client_max_window_bits} in the PMCE offer.
-     *
-     * <p>Valid values are in range 8..15 when non-null. The client encoder
-     * currently supports only {@code 15} due to JDK Deflater limitations.</p>
-     *
-     * @return offered {@code client_max_window_bits}, or {@code null} if not offered
-     * @since 5.7
-     */
-    public Integer getOfferClientMaxWindowBits() {
-        return offerClientMaxWindowBits;
-    }
-
     /**
      * Optional value for {@code server_max_window_bits} in the PMCE offer.
      *
@@ -332,7 +316,6 @@ public static final class Builder {
         private boolean perMessageDeflateEnabled = true;
         private boolean offerServerNoContextTakeover = true;
         private boolean offerClientNoContextTakeover = true;
-        private Integer offerClientMaxWindowBits = 15;
         private Integer offerServerMaxWindowBits = null;
 
         private int maxFrameSize = 64 * 1024;
@@ -409,21 +392,6 @@ public Builder offerClientNoContextTakeover(final boolean v) {
             return this;
         }
 
-        /**
-         * Offers {@code client_max_window_bits} in the PMCE offer.
-         *
-         * <p>Valid values are in range 8..15 when non-null. The client encoder
-         * currently supports only {@code 15} due to JDK Deflater limitations.</p>
-         *
-         * @param v window bits, or {@code null} to omit the parameter
-         * @return this builder
-         * @since 5.7
-         */
-        public Builder offerClientMaxWindowBits(final Integer v) {
-            this.offerClientMaxWindowBits = v;
-            return this;
-        }
-
         /**
          * Offers {@code server_max_window_bits} in the PMCE offer.
          *
@@ -570,9 +538,6 @@ public Builder enableHttp2(final boolean enabled) {
          * @since 5.7
          */
         public WebSocketClientConfig build() {
-            if (offerClientMaxWindowBits != null && (offerClientMaxWindowBits < 8 || offerClientMaxWindowBits > 15)) {
-                throw new IllegalArgumentException("offerClientMaxWindowBits must be in range [8..15]");
-            }
             if (offerServerMaxWindowBits != null && (offerServerMaxWindowBits < 8 || offerServerMaxWindowBits > 15)) {
                 throw new IllegalArgumentException("offerServerMaxWindowBits must be in range [8..15]");
             }
@@ -600,7 +565,7 @@ public WebSocketClientConfig build() {
             return new WebSocketClientConfig(
                     connectTimeout, subprotocols,
                     perMessageDeflateEnabled, offerServerNoContextTakeover, offerClientNoContextTakeover,
-                    offerClientMaxWindowBits, offerServerMaxWindowBits,
+                    offerServerMaxWindowBits,
                     maxFrameSize, outgoingChunkSize, maxFramesPerTick,
                     directBuffers,
                     autoPong, closeWaitTimeout, maxMessageSize,

httpclient5-websocket/src/main/java/org/apache/hc/client5/http/websocket/client/impl/protocol/Http1UpgradeProtocol.java

@@ -33,7 +33,10 @@
 import java.security.MessageDigest;
 import java.security.SecureRandom;
 import java.util.Base64;
+import java.util.HashSet;
 import java.util.List;
+import java.util.Locale;
+import java.util.Set;
 import java.util.StringJoiner;
 import java.util.concurrent.CompletableFuture;
 import java.util.concurrent.atomic.AtomicBoolean;
@@ -157,9 +160,8 @@ public void completed(final WebSocketEndpointConnector.ProtoEndpoint endpoint) {
                                 if (cfg.isOfferClientNoContextTakeover()) {
                                     ext.append("; client_no_context_takeover");
                                 }
-                                if (cfg.getOfferClientMaxWindowBits() != null) {
-                                    ext.append("; client_max_window_bits=").append(cfg.getOfferClientMaxWindowBits());
-                                }
+                                // client_max_window_bits is never offered: the JDK Deflater cannot
+                                // honor a server-selected window smaller than 15 (RFC 7692 7.2.1).
                                 if (cfg.getOfferServerMaxWindowBits() != null) {
                                     ext.append("; server_max_window_bits=").append(cfg.getOfferServerMaxWindowBits());
                                 }
@@ -359,9 +361,28 @@ public void cancelled() {
         }
     }
 
-    private static String headerValue(final HttpResponse r, final String name) {
-        final Header h = r.getFirstHeader(name);
-        return h != null ? h.getValue() : null;
+    static String headerValue(final HttpResponse r, final String name) {
+        // RFC 6455 section 9.1 permits Sec-WebSocket-Extensions and Sec-WebSocket-Protocol
+        // to be split across multiple header fields; combine them as a comma-separated list.
+        final Header[] headers = r.getHeaders(name);
+        if (headers.length == 0) {
+            return null;
+        }
+        if (headers.length == 1) {
+            return headers[0].getValue();
+        }
+        final StringBuilder buf = new StringBuilder();
+        for (final Header h : headers) {
+            final String value = h.getValue();
+            if (value == null || value.isEmpty()) {
+                continue;
+            }
+            if (buf.length() > 0) {
+                buf.append(", ");
+            }
+            buf.append(value);
+        }
+        return buf.length() > 0 ? buf.toString() : null;
     }
 
     private static boolean containsToken(final HttpResponse r, final String header, final String token) {
@@ -393,10 +414,7 @@ static ExtensionChain buildExtensionChain(final WebSocketClientConfig cfg, final
             return chain;
         }
         boolean pmceSeen = false, serverNoCtx = false, clientNoCtx = false;
-        Integer clientBits = null, serverBits = null;
-        final boolean offerServerNoCtx = cfg.isOfferServerNoContextTakeover();
-        final boolean offerClientNoCtx = cfg.isOfferClientNoContextTakeover();
-        final Integer offerClientBits = cfg.getOfferClientMaxWindowBits();
+        Integer serverBits = null;
         final Integer offerServerBits = cfg.getOfferServerMaxWindowBits();
 
         final String[] tokens = ext.split(",");
@@ -414,19 +432,23 @@ static ExtensionChain buildExtensionChain(final WebSocketClientConfig cfg, final
             }
             pmceSeen = true;
 
+            // RFC 7692 section 7.1: a parameter name must not appear more than once within an
+            // accepted extension; a repeated name makes the response invalid.
+            final Set<String> parameterNames = new HashSet<>();
             for (int i = 1; i < parts.length; i++) {
                 final String p = parts[i].trim();
                 final int eq = p.indexOf('=');
+                final String parameterName = (eq >= 0 ? p.substring(0, eq) : p).trim().toLowerCase(Locale.ROOT);
+                if (!parameterNames.add(parameterName)) {
+                    throw new IllegalStateException("Duplicate permessage-deflate parameter: " + parameterName);
+                }
                 if (eq < 0) {
+                    // RFC 7692 sections 7.1.1.1 and 7.1.1.2 permit the server to include either
+                    // no-context-takeover parameter in the response even when the offer did not;
+                    // both are always safe to honour.
                     if ("server_no_context_takeover".equalsIgnoreCase(p)) {
-                        if (!offerServerNoCtx) {
-                            throw new IllegalStateException("Server selected server_no_context_takeover not offered");
-                        }
                         serverNoCtx = true;
                     } else if ("client_no_context_takeover".equalsIgnoreCase(p)) {
-                        if (!offerClientNoCtx) {
-                            throw new IllegalStateException("Server selected client_no_context_takeover not offered");
-                        }
                         clientNoCtx = true;
                     } else {
                         throw new IllegalStateException("Unsupported permessage-deflate parameter: " + p);
@@ -438,24 +460,13 @@ static ExtensionChain buildExtensionChain(final WebSocketClientConfig cfg, final
                         v = v.substring(1, v.length() - 1); // strip quotes if any
                     }
                     if ("client_max_window_bits".equalsIgnoreCase(k)) {
-                        if (offerClientBits == null) {
-                            throw new IllegalStateException("Server selected client_max_window_bits not offered");
-                        }
-                        try {
-                            if (v.isEmpty()) {
-                                throw new IllegalStateException("client_max_window_bits must have a value");
-                            }
-                            clientBits = Integer.parseInt(v);
-                            if (clientBits < 8 || clientBits > 15) {
-                                throw new IllegalStateException("client_max_window_bits out of range: " + clientBits);
-                            }
-                        } catch (final NumberFormatException nfe) {
-                            throw new IllegalStateException("Invalid client_max_window_bits: " + v, nfe);
-                        }
+                        // The client never offers client_max_window_bits (the JDK Deflater cannot honor
+                        // a window smaller than 15, RFC 7692 section 7.2.1), so a server that selects it
+                        // has echoed a parameter that was never offered.
+                        throw new IllegalStateException("Server selected client_max_window_bits not offered");
                     } else if ("server_max_window_bits".equalsIgnoreCase(k)) {
-                        if (offerServerBits == null) {
-                            throw new IllegalStateException("Server selected server_max_window_bits not offered");
-                        }
+                        // RFC 7692 section 7.1.2.1 permits the server to include this parameter
+                        // uninvited; a server constraining its own window is always acceptable.
                         try {
                             if (v.isEmpty()) {
                                 throw new IllegalStateException("server_max_window_bits must have a value");
@@ -478,21 +489,12 @@ static ExtensionChain buildExtensionChain(final WebSocketClientConfig cfg, final
             if (!cfg.isPerMessageDeflateEnabled()) {
                 throw new IllegalStateException("Server negotiated PMCE but client disabled it");
             }
-            if (clientBits != null) {
-                if (offerClientBits == null) {
-                    throw new IllegalStateException("Server selected client_max_window_bits not offered");
-                }
-                if (!clientBits.equals(offerClientBits)) {
-                    throw new IllegalStateException("Unsupported client_max_window_bits: " + clientBits
-                            + " (offered " + offerClientBits + ")");
-                }
-            }
             if (serverBits != null) {
                 if (offerServerBits != null && serverBits > offerServerBits) {
                     throw new IllegalStateException("server_max_window_bits exceeds offer: " + serverBits);
                 }
             }
-            chain.add(new PerMessageDeflate(true, serverNoCtx, clientNoCtx, clientBits, serverBits));
+            chain.add(new PerMessageDeflate(true, serverNoCtx, clientNoCtx, null, serverBits));
         }
         return chain;
     }

httpclient5-websocket/src/main/java/org/apache/hc/client5/http/websocket/client/impl/protocol/Http2ExtendedConnectProtocol.java

@@ -160,9 +160,8 @@ public CompletableFuture<WebSocket> connect(
             if (cfg.isOfferClientNoContextTakeover()) {
                 ext.append("; client_no_context_takeover");
             }
-            if (cfg.getOfferClientMaxWindowBits() != null && cfg.getOfferClientMaxWindowBits() == 15) {
-                ext.append("; client_max_window_bits=15");
-            }
+            // client_max_window_bits is never offered: the JDK Deflater cannot honor a
+            // server-selected window smaller than 15 (RFC 7692 7.2.1).
             if (cfg.getOfferServerMaxWindowBits() != null) {
                 ext.append("; server_max_window_bits=").append(cfg.getOfferServerMaxWindowBits());
             }
@@ -211,8 +210,9 @@ public void produceRequest(final RequestChannel channel, final HttpContext conte
         public void consumeResponse(final HttpResponse response, final EntityDetails entityDetails,
                                     final HttpContext context) throws HttpException, IOException {
 
-            if (response.getCode() != HttpStatus.SC_OK) {
-                failFuture(new IllegalStateException("Unexpected status: " + response.getCode()));
+            final int code = response.getCode();
+            if (code < HttpStatus.SC_OK || code >= HttpStatus.SC_REDIRECTION) {
+                failFuture(new IllegalStateException("Unexpected status: " + code));
                 return;
             }
 
@@ -321,8 +321,7 @@ public void cancel() {
         }
 
         private static String headerValue(final HttpResponse r, final String name) {
-            final Header h = r.getFirstHeader(name);
-            return h != null ? h.getValue() : null;
+            return Http1UpgradeProtocol.headerValue(r, name);
         }
 
         private void failFuture(final Exception ex) {

httpclient5-websocket/src/main/java/org/apache/hc/client5/http/websocket/transport/DataStreamChannelTransport.java

@@ -28,6 +28,7 @@
 
 import java.io.IOException;
 import java.nio.ByteBuffer;
+import java.nio.channels.CancelledKeyException;
 
 import org.apache.hc.core5.annotation.Internal;
 import org.apache.hc.core5.http.nio.DataStreamChannel;
@@ -51,14 +52,26 @@ public int write(final ByteBuffer src) throws IOException {
         if (ch == null) {
             return 0;
         }
-        return ch.write(src);
+        try {
+            return ch.write(src);
+        } catch (final CancelledKeyException ex) {
+            // A cancelled selection key is a terminal state, not transient back-pressure. Surface it
+            // as an I/O error so the engine takes its close path instead of retaining the frame and
+            // waiting for an output callback that will never arrive.
+            throw new IOException("WebSocket transport channel is closed", ex);
+        }
     }
 
     @Override
     public void requestOutput() {
         final DataStreamChannel ch = channel;
         if (ch != null) {
-            ch.requestOutput();
+            try {
+                ch.requestOutput();
+            } catch (final CancelledKeyException ignore) {
+                // requestOutput is a best-effort nudge; if the channel's selection key was
+                // cancelled by a concurrent shutdown there is nothing left to flush.
+            }
         }
     }