Issue #3465 - Negotiation of WebSocket extensions

default behaviour of Negotiation no longer includes all of the
offered extensions as the negotiated extensions but it now takes
only the first extension if there are multiple of the same name,
this is now done when the negotiation is created and can be overwritten
by the negotiator

Throw exception on websocket errors so the proper status code can
be reported back to the client

fix to checking for multiple negotiated extensions of the same name

added tests for core and jetty websockets for the negotiation

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

jetty-websocket/jetty-websocket-tests/src/test/java/org/eclipse/jetty/websocket/tests/JettyWebSocketNegotiationTest.java

@@ -0,0 +1,115 @@
+//
+//  ========================================================================
+//  Copyright (c) 1995-2019 Mort Bay Consulting Pty. Ltd.
+//  ------------------------------------------------------------------------
+//  All rights reserved. This program and the accompanying materials
+//  are made available under the terms of the Eclipse Public License v1.0
+//  and Apache License v2.0 which accompanies this distribution.
+//
+//      The Eclipse Public License is available at
+//      http://www.eclipse.org/legal/epl-v10.html
+//
+//      The Apache License v2.0 is available at
+//      http://www.opensource.org/licenses/apache2.0.php
+//
+//  You may elect to redistribute this code under either of these licenses.
+//  ========================================================================
+//
+
+package org.eclipse.jetty.websocket.tests;
+
+import java.net.URI;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.TimeUnit;
+
+import org.eclipse.jetty.server.HttpChannel;
+import org.eclipse.jetty.server.Server;
+import org.eclipse.jetty.server.ServerConnector;
+import org.eclipse.jetty.servlet.ServletContextHandler;
+import org.eclipse.jetty.util.log.StacklessLogging;
+import org.eclipse.jetty.websocket.api.Session;
+import org.eclipse.jetty.websocket.api.UpgradeRequest;
+import org.eclipse.jetty.websocket.client.ClientUpgradeRequest;
+import org.eclipse.jetty.websocket.client.WebSocketClient;
+import org.eclipse.jetty.websocket.server.JettyWebSocketServerContainer;
+import org.eclipse.jetty.websocket.server.JettyWebSocketServletContainerInitializer;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.containsString;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+
+public class JettyWebSocketNegotiationTest
+{
+    Server server;
+    WebSocketClient client;
+    ServletContextHandler contextHandler;
+
+    @BeforeEach
+    public void start() throws Exception
+    {
+        server = new Server();
+        ServerConnector connector = new ServerConnector(server);
+        connector.setPort(8080);
+        server.addConnector(connector);
+
+        contextHandler = new ServletContextHandler(ServletContextHandler.SESSIONS);
+        contextHandler.setContextPath("/");
+        server.setHandler(contextHandler);
+
+        server.start();
+        client = new WebSocketClient();
+        client.start();
+    }
+
+    @AfterEach
+    public void stop() throws Exception
+    {
+        client.stop();
+        server.stop();
+    }
+
+    @Test
+    public void testBadRequest() throws Exception
+    {
+        JettyWebSocketServerContainer container = JettyWebSocketServletContainerInitializer.configureContext(contextHandler);
+        container.addMapping("/", (req, resp)->new EventSocket.EchoSocket());
+
+        URI uri = URI.create("ws://localhost:8080/filterPath");
+        EventSocket socket = new EventSocket();
+
+        UpgradeRequest upgradeRequest = new ClientUpgradeRequest();
+        upgradeRequest.addExtensions("permessage-deflate;invalidParameter");
+
+        CompletableFuture<Session> connect = client.connect(socket, uri, upgradeRequest);
+        Throwable t = assertThrows(ExecutionException.class, () -> connect.get(5, TimeUnit.SECONDS));
+        assertThat(t.getMessage(), containsString("Failed to upgrade to websocket:"));
+        assertThat(t.getMessage(), containsString("400 Bad Request"));
+    }
+
+
+    @Test
+    public void testServerError() throws Exception
+    {
+        JettyWebSocketServerContainer container = JettyWebSocketServletContainerInitializer.configureContext(contextHandler);
+        container.addMapping("/", (req, resp)->
+        {
+            resp.setAcceptedSubProtocol("errorSubProtocol");
+            return new EventSocket.EchoSocket();
+        });
+
+        URI uri = URI.create("ws://localhost:8080/filterPath");
+        EventSocket socket = new EventSocket();
+
+        try (StacklessLogging stacklessLogging = new StacklessLogging(HttpChannel.class))
+        {
+            CompletableFuture<Session> connect = client.connect(socket, uri);
+            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"));
+        }
+    }
+}

jetty-websocket/websocket-core/src/main/java/org/eclipse/jetty/websocket/core/WebSocketExtensionRegistry.java

@@ -18,16 +18,17 @@
 
 package org.eclipse.jetty.websocket.core;
 
-import org.eclipse.jetty.io.ByteBufferPool;
-import org.eclipse.jetty.util.DecoratedObjectFactory;
-import org.eclipse.jetty.util.StringUtil;
-
 import java.util.HashMap;
 import java.util.Iterator;
 import java.util.Map;
 import java.util.ServiceLoader;
 import java.util.Set;
 
+import org.eclipse.jetty.http.BadMessageException;
+import org.eclipse.jetty.io.ByteBufferPool;
+import org.eclipse.jetty.util.DecoratedObjectFactory;
+import org.eclipse.jetty.util.StringUtil;
+
 public class WebSocketExtensionRegistry implements Iterable<Class<? extends Extension>>
 {
     private Map<String, Class<? extends Extension>> availableExtensions;
@@ -100,7 +101,7 @@ public class WebSocketExtensionRegistry implements Iterable<Class<? extends Exte
         }
         catch (Throwable t)
         {
-            throw new WebSocketException("Cannot instantiate extension: " + extClass, t);
+            throw new BadMessageException("Cannot instantiate extension: " + extClass, t);
         }
     }
 

jetty-websocket/websocket-core/src/main/java/org/eclipse/jetty/websocket/core/client/ClientUpgradeRequest.java

@@ -285,6 +285,8 @@ public abstract class ClientUpgradeRequest extends HttpRequest implements Respon
             long numMatch = offeredExtensions.stream().filter(c -> config.getName().equalsIgnoreCase(c.getName())).count();
             if (numMatch < 1)
                 throw new WebSocketException("Upgrade failed: Sec-WebSocket-Extensions contained extension not requested");
+
+            numMatch = extensions.stream().filter(c -> config.getName().equalsIgnoreCase(c.getName())).count();
             if (numMatch > 1)
                 throw new WebSocketException("Upgrade failed: Sec-WebSocket-Extensions contained more than one extension of the same name");
         }

jetty-websocket/websocket-core/src/main/java/org/eclipse/jetty/websocket/core/internal/compress/PerMessageDeflateExtension.java

@@ -169,7 +169,7 @@ public class PerMessageDeflateExtension extends CompressExtension
                 }
                 case "server_no_context_takeover":
                 {
-                    params_negotiated.put("client_no_context_takeover", null);
+                    params_negotiated.put("server_no_context_takeover", null);
                     outgoingContextTakeover = false;
                     break;
                 }

jetty-websocket/websocket-core/src/main/java/org/eclipse/jetty/websocket/core/server/Negotiation.java

@@ -21,7 +21,6 @@ package org.eclipse.jetty.websocket.core.server;
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.List;
-import java.util.ListIterator;
 import java.util.Set;
 import java.util.stream.Collectors;
 
@@ -137,6 +136,15 @@ public class Negotiation
         offeredSubprotocols = subprotocols == null
             ?Collections.emptyList()
             :subprotocols.getValues();
+
+        negotiatedExtensions = new ArrayList<>();
+        for (ExtensionConfig config : offeredExtensions)
+        {
+            long matches = negotiatedExtensions.stream()
+                    .filter(negotiatedConfig->negotiatedConfig.getName().equals(config.getName())).count();
+            if (matches == 0)
+                negotiatedExtensions.add(config);
+        }
     }
 
     public String getKey()
@@ -159,8 +167,6 @@ public class Negotiation
 
     public List<ExtensionConfig> getNegotiatedExtensions()
     {
-        if (negotiatedExtensions == null)
-            return offeredExtensions;
         return negotiatedExtensions;
     }
 
@@ -209,51 +215,18 @@ public class Negotiation
     {
         if (extensionStack == null)
         {
+            // Extension stack can decide to drop any of these extensions or their parameters
             extensionStack = new ExtensionStack(registry);
-            boolean configsFromApplication = true;
-
-            if (negotiatedExtensions == null)
-            {
-                // Has the header been set directly?
-                List<String> extensions = baseRequest.getResponse().getHttpFields()
-                    .getCSV(HttpHeader.SEC_WEBSOCKET_EXTENSIONS, true);
-
-                if (extensions.isEmpty())
-                {
-                    // If the negotiatedExtensions has not been set, just use the offered extensions
-                    negotiatedExtensions = new ArrayList(offeredExtensions);
-                    configsFromApplication = false;
-                }
-                else
-                {
-                    negotiatedExtensions = extensions
-                        .stream()
-                        .map(ExtensionConfig::parse)
-                        .collect(Collectors.toList());
-                }
-            }
-
-            if (configsFromApplication)
-            {
-                // TODO is this really necessary?
-                // Replace any configuration in the negotiated extensions with the offered extensions config
-                for (ListIterator<ExtensionConfig> i = negotiatedExtensions.listIterator(); i.hasNext(); )
-                {
-                    ExtensionConfig config = i.next();
-                    offeredExtensions.stream().filter(c -> c.getName().equalsIgnoreCase(config.getName()))
-                        .findFirst()
-                        .ifPresent(i::set);
-                }
-            }
-
             extensionStack.negotiate(objectFactory, bufferPool, negotiatedExtensions);
             negotiatedExtensions = extensionStack.getNegotiatedExtensions();
+
             if (extensionStack.hasNegotiatedExtensions())
                 baseRequest.getResponse().setHeader(HttpHeader.SEC_WEBSOCKET_EXTENSIONS,
                     ExtensionConfig.toHeaderValue(negotiatedExtensions));
             else
                 baseRequest.getResponse().setHeader(HttpHeader.SEC_WEBSOCKET_EXTENSIONS, null);
         }
+
         return extensionStack;
     }
 

jetty-websocket/websocket-core/src/main/java/org/eclipse/jetty/websocket/core/server/internal/RFC6455Handshaker.java

@@ -46,6 +46,7 @@ import org.eclipse.jetty.websocket.core.Behavior;
 import org.eclipse.jetty.websocket.core.ExtensionConfig;
 import org.eclipse.jetty.websocket.core.FrameHandler;
 import org.eclipse.jetty.websocket.core.WebSocketConstants;
+import org.eclipse.jetty.websocket.core.WebSocketException;
 import org.eclipse.jetty.websocket.core.internal.Negotiated;
 import org.eclipse.jetty.websocket.core.internal.WebSocketChannel;
 import org.eclipse.jetty.websocket.core.internal.WebSocketConnection;
@@ -158,39 +159,24 @@ public final class RFC6455Handshaker implements Handshaker
         if (subprotocol != null)
         {
             if (!negotiation.getOfferedSubprotocols().contains(subprotocol))
-            {
-                // TODO: this message needs to be returned to Http Client
-                LOG.warn("not upgraded: selected subprotocol {} not present in offered subprotocols {}: {}",
-                        subprotocol, negotiation.getOfferedSubprotocols(), baseRequest);
-                return false;
-            }
+                throw new WebSocketException("not upgraded: selected a subprotocol not present in offered subprotocols");
         }
         else
         {
             if (!negotiation.getOfferedSubprotocols().isEmpty())
-            {
-                // TODO: this message needs to be returned to Http Client
-                LOG.warn("not upgraded: no subprotocol selected from offered subprotocols {}: {}",
-                        negotiation.getOfferedSubprotocols(), baseRequest);
-                return false;
-            }
+                throw new WebSocketException("not upgraded: no subprotocol selected from offered subprotocols");
         }
 
         // validate negotiated extensions
-        negotiation.getOfferedExtensions();
         for (ExtensionConfig config : negotiation.getNegotiatedExtensions())
         {
-            long numMatch = negotiation.getOfferedExtensions().stream().filter(c -> config.getName().equalsIgnoreCase(c.getName())).count();
-            if (numMatch < 1)
-            {
-                LOG.warn("Upgrade failed: negotiated extension not requested {}: {}", config.getName(), baseRequest);
-                return false;
-            }
-            if (numMatch > 1)
-            {
-                LOG.warn("Upgrade failed: multiple negotiated extensions of the same name {}: {}", config.getName(), baseRequest);
-                return false;
-            }
+            long matches = negotiation.getOfferedExtensions().stream().filter(c -> config.getName().equalsIgnoreCase(c.getName())).count();
+            if (matches < 1)
+                throw new WebSocketException("Upgrade failed: negotiated extension not requested");
+
+            matches = negotiation.getNegotiatedExtensions().stream().filter(c -> config.getName().equalsIgnoreCase(c.getName())).count();
+            if (matches > 1)
+                throw new WebSocketException("Upgrade failed: multiple negotiated extensions of the same name");
         }
 
         Negotiated negotiated = new Negotiated(

jetty-websocket/websocket-core/src/test/java/org/eclipse/jetty/websocket/core/WebSocketNegotiationTest.java

@@ -0,0 +1,287 @@
+package org.eclipse.jetty.websocket.core;
+
+import java.io.IOException;
+import java.util.Collections;
+import java.util.List;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.TimeUnit;
+
+import org.eclipse.jetty.client.HttpRequest;
+import org.eclipse.jetty.client.HttpResponse;
+import org.eclipse.jetty.http.HttpHeader;
+import org.eclipse.jetty.server.HttpChannel;
+import org.eclipse.jetty.util.Callback;
+import org.eclipse.jetty.util.log.StacklessLogging;
+import org.eclipse.jetty.websocket.core.FrameHandler.CoreSession;
+import org.eclipse.jetty.websocket.core.client.ClientUpgradeRequest;
+import org.eclipse.jetty.websocket.core.client.UpgradeListener;
+import org.eclipse.jetty.websocket.core.client.WebSocketCoreClient;
+import org.eclipse.jetty.websocket.core.server.Negotiation;
+import org.eclipse.jetty.websocket.core.server.WebSocketNegotiator;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.is;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+public class WebSocketNegotiationTest
+{
+    public static class EchoFrameHandler extends TestFrameHandler
+    {
+        @Override
+        public void onFrame(Frame frame)
+        {
+            super.onFrame(frame);
+            Frame echo = new Frame(frame.getOpCode(), frame.getPayloadAsUTF8());
+            getCoreSession().sendFrame(echo, Callback.NOOP, false);
+        }
+    }
+
+    private WebSocketServer server;
+    private WebSocketCoreClient client;
+
+    @BeforeEach
+    public void startup() throws Exception
+    {
+        WebSocketNegotiator negotiator = new WebSocketNegotiator.AbstractNegotiator()
+        {
+            @Override
+            public FrameHandler negotiate(Negotiation negotiation) throws IOException
+            {
+                if (negotiation.getOfferedSubprotocols().isEmpty())
+                {
+                    negotiation.setSubprotocol("NotOffered");
+                    return new EchoFrameHandler();
+                }
+
+
+                String subprotocol = negotiation.getOfferedSubprotocols().get(0);
+                negotiation.setSubprotocol(subprotocol);
+                switch (subprotocol)
+                {
+                    case "testExtensionSelection":
+                        negotiation.setNegotiatedExtensions(List.of(ExtensionConfig.parse("permessage-deflate;client_no_context_takeover")));
+                        break;
+
+                    case "testNotOfferedParameter":
+                        negotiation.setNegotiatedExtensions(List.of(ExtensionConfig.parse("permessage-deflate;server_no_context_takeover")));
+                        break;
+
+                    case "testInvalidExtensionParameter":
+                        break;
+
+                    case "testAcceptTwoExtensionsOfSameName":
+                        // We should automatically be selecting just one extension out of these two
+                        break;
+
+                    case "testNotAcceptingExtensions":
+                        negotiation.setNegotiatedExtensions(Collections.EMPTY_LIST);
+                        break;
+
+                    case "testNoSubProtocolSelected":
+                        negotiation.setSubprotocol(null);
+                        break;
+
+                    default:
+                        return null;
+                }
+
+                return new EchoFrameHandler();
+            }
+        };
+
+        server = new WebSocketServer(negotiator);
+        client = new WebSocketCoreClient();
+
+        server.start();
+        client.start();
+    }
+
+    @AfterEach
+    public void shutdown() throws Exception
+    {
+        server.start();
+        client.start();
+    }
+
+    @Test
+    public void testExtensionSelection() throws Exception
+    {
+        TestFrameHandler clientHandler = new TestFrameHandler();
+
+        ClientUpgradeRequest upgradeRequest = ClientUpgradeRequest.from(client, server.getUri(), clientHandler);
+        upgradeRequest.setSubProtocols("testExtensionSelection");
+        upgradeRequest.addExtensions("permessage-deflate;server_no_context_takeover", "permessage-deflate;client_no_context_takeover");
+
+        CompletableFuture<String> extensionHeader = new CompletableFuture<>();
+        upgradeRequest.addListener(new UpgradeListener()
+        {
+            @Override
+            public void onHandshakeResponse(HttpRequest request, HttpResponse response)
+            {
+                extensionHeader.complete(response.getHeaders().get(HttpHeader.SEC_WEBSOCKET_EXTENSIONS));
+            }
+        });
+
+        CompletableFuture<CoreSession> connect = client.connect(upgradeRequest);
+        connect.get(5, TimeUnit.SECONDS);
+
+        clientHandler.sendText("hello world");
+        clientHandler.sendClose();
+        assertTrue(clientHandler.closed.await(5, TimeUnit.SECONDS));
+        assertThat(clientHandler.receivedFrames.size(), is(2));
+        assertNull(clientHandler.getError());
+
+        assertThat(extensionHeader.get(5, TimeUnit.SECONDS), is("permessage-deflate;client_no_context_takeover"));
+    }
+
+    @Test
+    public void testNotOfferedParameter() throws Exception
+    {
+        TestFrameHandler clientHandler = new TestFrameHandler();
+
+        ClientUpgradeRequest upgradeRequest = ClientUpgradeRequest.from(client, server.getUri(), clientHandler);
+        upgradeRequest.setSubProtocols("testNotOfferedParameter");
+        upgradeRequest.addExtensions("permessage-deflate;client_no_context_takeover");
+
+        CompletableFuture<String> extensionHeader = new CompletableFuture<>();
+        upgradeRequest.addListener(new UpgradeListener()
+        {
+            @Override
+            public void onHandshakeResponse(HttpRequest request, HttpResponse response)
+            {
+                extensionHeader.complete(response.getHeaders().get(HttpHeader.SEC_WEBSOCKET_EXTENSIONS));
+            }
+        });
+
+        CompletableFuture<CoreSession> connect = client.connect(upgradeRequest);
+        connect.get(5, TimeUnit.SECONDS);
+        clientHandler.sendText("hello world");
+        clientHandler.sendClose();
+        assertTrue(clientHandler.closed.await(5, TimeUnit.SECONDS));
+        assertThat(clientHandler.receivedFrames.size(), is(2));
+        assertNull(clientHandler.getError());
+
+        assertThat(extensionHeader.get(5, TimeUnit.SECONDS), is("permessage-deflate;server_no_context_takeover"));
+    }
+
+
+    @Test
+    public void testInvalidExtensionParameter() throws Exception
+    {
+        TestFrameHandler clientHandler = new TestFrameHandler();
+
+        ClientUpgradeRequest upgradeRequest = ClientUpgradeRequest.from(client, server.getUri(), clientHandler);
+        upgradeRequest.setSubProtocols("testInvalidExtensionParameter");
+        upgradeRequest.addExtensions("permessage-deflate;invalid_parameter");
+
+        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("400 Bad Request"));
+    }
+
+
+    @Test
+    public void testNotAcceptingExtensions() throws Exception
+    {
+        TestFrameHandler clientHandler = new TestFrameHandler();
+
+        ClientUpgradeRequest upgradeRequest = ClientUpgradeRequest.from(client, server.getUri(), clientHandler);
+        upgradeRequest.setSubProtocols("testNotAcceptingExtensions");
+        upgradeRequest.addExtensions("permessage-deflate;server_no_context_takeover", "permessage-deflate;client_no_context_takeover");
+
+        CompletableFuture<String> extensionHeader = new CompletableFuture<>();
+        upgradeRequest.addListener(new UpgradeListener()
+        {
+            @Override
+            public void onHandshakeResponse(HttpRequest request, HttpResponse response)
+            {
+                extensionHeader.complete(response.getHeaders().get(HttpHeader.SEC_WEBSOCKET_EXTENSIONS));
+            }
+        });
+
+        CompletableFuture<CoreSession> connect = client.connect(upgradeRequest);
+        connect.get(5, TimeUnit.SECONDS);
+
+        clientHandler.sendText("hello world");
+        clientHandler.sendClose();
+        assertTrue(clientHandler.closed.await(5, TimeUnit.SECONDS));
+        assertThat(clientHandler.receivedFrames.size(), is(2));
+        assertNull(clientHandler.getError());
+
+        assertNull(extensionHeader.get(5, TimeUnit.SECONDS));
+    }
+
+
+    @Test
+    public void testAcceptTwoExtensionsOfSameName() throws Exception
+    {
+        TestFrameHandler clientHandler = new TestFrameHandler();
+
+        ClientUpgradeRequest upgradeRequest = ClientUpgradeRequest.from(client, server.getUri(), clientHandler);
+        upgradeRequest.setSubProtocols("testAcceptTwoExtensionsOfSameName");
+        upgradeRequest.addExtensions("permessage-deflate;server_no_context_takeover", "permessage-deflate;client_no_context_takeover");
+
+        CompletableFuture<String> extensionHeader = new CompletableFuture<>();
+        upgradeRequest.addListener(new UpgradeListener()
+        {
+            @Override
+            public void onHandshakeResponse(HttpRequest request, HttpResponse response)
+            {
+                extensionHeader.complete(response.getHeaders().get(HttpHeader.SEC_WEBSOCKET_EXTENSIONS));
+            }
+        });
+
+        CompletableFuture<CoreSession> connect = client.connect(upgradeRequest);
+        connect.get(5, TimeUnit.SECONDS);
+
+        clientHandler.sendText("hello world");
+        clientHandler.sendClose();
+        assertTrue(clientHandler.closed.await(5, TimeUnit.SECONDS));
+        assertThat(clientHandler.receivedFrames.size(), is(2));
+        assertNull(clientHandler.getError());
+
+        assertThat(extensionHeader.get(5, TimeUnit.SECONDS), is("permessage-deflate;server_no_context_takeover"));
+    }
+
+    @Test
+    public void testSubProtocolNotOffered() throws Exception
+    {
+        TestFrameHandler clientHandler = new TestFrameHandler();
+
+        ClientUpgradeRequest upgradeRequest = ClientUpgradeRequest.from(client, server.getUri(), clientHandler);
+
+        try (StacklessLogging stacklessLogging = new StacklessLogging(HttpChannel.class))
+        {
+            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"));
+        }
+    }
+
+    @Test
+    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<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"));
+        }
+    }
+}

jetty-websocket/websocket-core/src/test/java/org/eclipse/jetty/websocket/core/WebSocketServer.java

@@ -19,6 +19,7 @@
 package org.eclipse.jetty.websocket.core;
 
 import java.io.IOException;
+import java.net.URI;
 import java.util.List;
 
 import org.eclipse.jetty.server.NetworkConnector;
@@ -35,10 +36,12 @@ public class WebSocketServer
 {
     private static Logger LOG = Log.getLogger(WebSocketServer.class);
     private final Server server;
+    private URI serverUri;
 
     public void start() throws Exception
     {
         server.start();
+        serverUri = new URI("ws://localhost:" + getLocalPort());
     }
 
     public void stop() throws Exception
@@ -56,12 +59,12 @@ public class WebSocketServer
         return server;
     }
 
-    public WebSocketServer(FrameHandler frameHandler)
+    public WebSocketServer(FrameHandler frameHandler) throws Exception
     {
         this(new DefaultNegotiator(frameHandler));
     }
 
-    public WebSocketServer(WebSocketNegotiator negotiator)
+    public WebSocketServer(WebSocketNegotiator negotiator) throws Exception
     {
         server = new Server();
         ServerConnector connector = new ServerConnector(server);
@@ -75,6 +78,11 @@ public class WebSocketServer
         context.setHandler(upgradeHandler);
     }
 
+    public URI getUri()
+    {
+        return serverUri;
+    }
+
     private static class DefaultNegotiator extends WebSocketNegotiator.AbstractNegotiator
     {
         private final FrameHandler frameHandler;

jetty-websocket/websocket-servlet/src/main/java/org/eclipse/jetty/websocket/servlet/ServletUpgradeResponse.java

@@ -170,18 +170,13 @@ public class ServletUpgradeResponse
         // This validation is also done later in RFC6455Handshaker but it is better to fail earlier
         for (ExtensionConfig config : configs)
         {
-            int matches = (int)negotiation.getOfferedExtensions().stream()
-                    .filter(e -> e.getName().equals(config.getName())).count();
+            long matches = negotiation.getOfferedExtensions().stream().filter(e -> e.getName().equals(config.getName())).count();
+            if (matches < 1)
+                throw new IllegalArgumentException("Extension not a requested extension");
 
-            switch (matches)
-            {
-                case 0:
-                    throw new IllegalArgumentException("Extension not a requested extension");
-                case 1:
-                    continue;
-                default:
-                    throw new IllegalArgumentException("Multiple extensions of the same name");
-            }
+            matches = negotiation.getNegotiatedExtensions().stream().filter(e -> e.getName().equals(config.getName())).count();
+            if (matches > 1)
+                throw new IllegalArgumentException("Multiple extensions of the same name");
         }
 
         negotiation.setNegotiatedExtensions(configs);

jetty-websocket/websocket-servlet/src/main/java/org/eclipse/jetty/websocket/servlet/WebSocketMapping.java

@@ -219,36 +219,35 @@ public class WebSocketMapping implements Dumpable, LifeCycle.Listener
 
     public boolean upgrade(HttpServletRequest request, HttpServletResponse response, FrameHandler.Customizer defaultCustomizer)
     {
+        // Since this may be a filter, we need to be smart about determining the target path.
+        // We should rely on the Container for stripping path parameters and its ilk before
+        // attempting to match a specific mapped websocket creator.
+        String target = request.getServletPath();
+        if (request.getPathInfo() != null)
+            target = target + request.getPathInfo();
+
+        WebSocketNegotiator negotiator = getMatchedNegotiator(target, pathSpec ->
+        {
+            // Store PathSpec resource mapping as request attribute, for WebSocketCreator
+            // implementors to use later if they wish
+            request.setAttribute(PathSpec.class.getName(), pathSpec);
+        });
+
+        if (negotiator == null)
+            return false;
+
+        if (LOG.isDebugEnabled())
+            LOG.debug("WebSocket Negotiated detected on {} for endpoint {}", target, negotiator);
+
+        // We have an upgrade request
         try
         {
-            // Since this may be a filter, we need to be smart about determining the target path.
-            // We should rely on the Container for stripping path parameters and its ilk before
-            // attempting to match a specific mapped websocket creator.
-            String target = request.getServletPath();
-            if (request.getPathInfo() != null)
-                target = target + request.getPathInfo();
-
-            WebSocketNegotiator negotiator = getMatchedNegotiator(target, pathSpec ->
-            {
-                // Store PathSpec resource mapping as request attribute, for WebSocketCreator
-                // implementors to use later if they wish
-                request.setAttribute(PathSpec.class.getName(), pathSpec);
-            });
-
-            if (negotiator == null)
-                return false;
-
-            if (LOG.isDebugEnabled())
-                LOG.debug("WebSocket Negotiated detected on {} for endpoint {}", target, negotiator);
-
-            // We have an upgrade request
             return handshaker.upgradeRequest(negotiator, request, response, defaultCustomizer);
         }
-        catch (Exception e)
+        catch (IOException e)
         {
-            LOG.warn("Error during upgrade: ", e);
+            throw new WebSocketException(e);
         }
-        return false;
     }
 
     private class Negotiator extends WebSocketNegotiator.AbstractNegotiator