Issue #4747 - server is allowed to not select a websocket subprotocol

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
jetty · Apr 8, 2020 · 45f822e · 45f822e
1 parent 4e69b48
commit 45f822e
Show file tree

Hide file tree

Showing 3 changed files with 20 additions and 16 deletions.
diff --git a/...cket-core/src/main/java/org/eclipse/jetty/websocket/core/client/ClientUpgradeRequest.java b/...cket-core/src/main/java/org/eclipse/jetty/websocket/core/client/ClientUpgradeRequest.java
@@ -409,8 +409,6 @@ else if (values.length == 1)
 
         // Verify the negotiated subprotocol
         List<String> offeredSubProtocols = getSubProtocols();
-        if (negotiatedSubProtocol == null && !offeredSubProtocols.isEmpty())
-            throw new WebSocketException("Upgrade failed: no subprotocol selected from offered subprotocols ");
         if (negotiatedSubProtocol != null && !offeredSubProtocols.contains(negotiatedSubProtocol))
             throw new WebSocketException("Upgrade failed: subprotocol [" + negotiatedSubProtocol + "] not found in offered subprotocols " + offeredSubProtocols);
 

diff --git a/...re/src/main/java/org/eclipse/jetty/websocket/core/server/internal/AbstractHandshaker.java b/...re/src/main/java/org/eclipse/jetty/websocket/core/server/internal/AbstractHandshaker.java
@@ -96,19 +96,14 @@ public boolean upgradeRequest(WebSocketNegotiator negotiator, HttpServletRequest
             return false;
         }
 
-        // Validate negotiated protocol
+        // Validate negotiated protocol.
         String protocol = negotiation.getSubprotocol();
         List<String> offeredProtocols = negotiation.getOfferedSubprotocols();
         if (protocol != null)
         {
             if (!offeredProtocols.contains(protocol))
                 throw new WebSocketException("not upgraded: selected a protocol not present in offered protocols");
         }
-        else
-        {
-            if (!offeredProtocols.isEmpty())
-                throw new WebSocketException("not upgraded: no protocol selected from offered protocols");
-        }
 
         // validate negotiated extensions
         for (ExtensionConfig config : negotiation.getNegotiatedExtensions())

diff --git a/...bsocket-core/src/test/java/org/eclipse/jetty/websocket/core/WebSocketNegotiationTest.java b/...bsocket-core/src/test/java/org/eclipse/jetty/websocket/core/WebSocketNegotiationTest.java
@@ -317,17 +317,28 @@ public void testSubProtocolNotOffered() throws Exception
     public void testNoSubProtocolSelected() throws Exception
     {
         TestFrameHandler clientHandler = new TestFrameHandler();
-
         ClientUpgradeRequest upgradeRequest = ClientUpgradeRequest.from(client, server.getUri(), clientHandler);
         upgradeRequest.setSubProtocols("testNoSubProtocolSelected");
-
-        try (StacklessLogging stacklessLogging = new StacklessLogging(HttpChannel.class))
+        CompletableFuture<HttpFields> headers = new CompletableFuture<>();
+        upgradeRequest.addListener(new UpgradeListener()
         {
-            CompletableFuture<CoreSession> connect = client.connect(upgradeRequest);
-            Throwable t = assertThrows(ExecutionException.class, () -> connect.get(5, TimeUnit.SECONDS));
-            assertThat(t.getMessage(), containsString("Failed to upgrade to websocket:"));
-            assertThat(t.getMessage(), containsString("500 Server Error"));
-        }
+            @Override
+            public void onHandshakeResponse(HttpRequest request, HttpResponse response)
+            {
+                headers.complete(response.getHeaders());
+            }
+        });
+
+        CoreSession session = client.connect(upgradeRequest).get(5, TimeUnit.SECONDS);
+        session.close(Callback.NOOP);
+        assertTrue(clientHandler.closed.await(5, TimeUnit.SECONDS));
+        assertThat(clientHandler.closeStatus.getCode(), is(CloseStatus.NO_CODE));
+
+        // RFC6455: If the server does not agree to any of the client's requested subprotocols, the only acceptable
+        // value is null. It MUST NOT send back a |Sec-WebSocket-Protocol| header field in its response.
+        HttpFields httpFields = headers.get();
+        assertThat(httpFields.get(HttpHeader.UPGRADE), is("WebSocket"));
+        assertNull(httpFields.get(HttpHeader.SEC_WEBSOCKET_SUBPROTOCOL));
     }
 
     @Test