Merge pull request #8413 from eclipse/jetty-12.0.x-websocket-upgrade-…

…contract

Jetty 12 : Strengthen WebSocket upgrade contract and other improvements

jetty-core/jetty-websocket/jetty-websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/WebSocketConstants.java

@@ -32,6 +32,10 @@ public final class WebSocketConstants
     public static final Duration DEFAULT_IDLE_TIMEOUT = Duration.ofSeconds(30);
     public static final Duration DEFAULT_WRITE_TIMEOUT = Duration.ZERO;
 
+    // Attributes for storing API requests as attributes on the base jetty-core request.
+    public static final String WEBSOCKET_WRAPPED_REQUEST_ATTRIBUTE = "org.eclipse.jetty.websocket.wrappedRequest";
+    public static final String WEBSOCKET_WRAPPED_RESPONSE_ATTRIBUTE = "org.eclipse.jetty.websocket.wrappedResponse";
+
     /**
      * Globally Unique Identifier for use in WebSocket handshake within {@code Sec-WebSocket-Accept} and <code>Sec-WebSocket-Key</code> http headers.
      * <p>

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

@@ -29,5 +29,33 @@ static Handshaker newInstance()
         return new HandshakerSelector();
     }
 
+    /**
+     * <p>A preliminary check to see if a request is likely to be a valid WebSocket Upgrade Request. If this returns true
+     * the {@link Request} may be a valid upgrade request, but if this returns false returns false you can avoid calling
+     * {@link #upgradeRequest(WebSocketNegotiator, Request, Response, Callback, WebSocketComponents, Configuration.Customizer)}
+     * entirely as it will always fail</p>
+     *
+     * @param request the request
+     * @return true if the request is thought to be a valid websocket upgrade request.
+     */
+    boolean isWebSocketUpgradeRequest(Request request);
+
+    /**
+     * <p>Attempts to upgrade a request to WebSocket.</p>
+     *
+     * <p>Returns {@code true} if the WebSocket upgrade is successful and a successful response is generated and the callback
+     * eventually completed, or if the WebSocket upgrade failed and a failure response is generated and the callback eventually
+     * completed. Returns {@code false} if a response is not generated and the caller is responsible for generating a response
+     * and completing the callback.</p>
+     *
+     * @param negotiator the negotiator
+     * @param request the request
+     * @param response the response
+     * @param callback the callback
+     * @param components the WebSocket components
+     * @param defaultCustomizer the customizer
+     * @return true if a response was generated, false if a response is not generated
+     * @throws IOException there is an error during the upgrade
+     */
     boolean upgradeRequest(WebSocketNegotiator negotiator, Request request, Response response, Callback callback, WebSocketComponents components, Configuration.Customizer defaultCustomizer) throws IOException;
 }

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

@@ -21,7 +21,9 @@
 import org.eclipse.jetty.server.Request;
 import org.eclipse.jetty.util.Attributes;
 import org.eclipse.jetty.websocket.core.ExtensionConfig;
+import org.eclipse.jetty.websocket.core.WebSocketComponents;
 import org.eclipse.jetty.websocket.core.WebSocketConstants;
+import org.eclipse.jetty.websocket.core.server.internal.WebSocketNegotiation;
 
 /**
  * Upgrade request used for websocket negotiation.
@@ -41,6 +43,11 @@ public ServerUpgradeRequest(WebSocketNegotiation negotiation, Request baseReques
         this.request = baseRequest;
     }
 
+    public WebSocketComponents getWebSocketComponents()
+    {
+        return negotiation.getWebSocketComponents();
+    }
+
     public void upgrade(Attributes attributes)
     {
         this.attributes.clearAttributes();
@@ -94,7 +101,6 @@ public void clearAttributes()
 
     /**
      * @return The extensions offered
-     * @see WebSocketNegotiation#getOfferedExtensions()
      */
     public List<ExtensionConfig> getExtensions()
     {

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

@@ -20,6 +20,7 @@
 import org.eclipse.jetty.server.Response;
 import org.eclipse.jetty.websocket.core.ExtensionConfig;
 import org.eclipse.jetty.websocket.core.server.internal.WebSocketHttpFieldsWrapper;
+import org.eclipse.jetty.websocket.core.server.internal.WebSocketNegotiation;
 
 /**
  * Upgrade response used for websocket negotiation.

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

@@ -15,23 +15,26 @@
 
 import org.eclipse.jetty.util.Callback;
 
-// TODO: improve javadoc.
 /**
  * Abstract WebSocket creator interface.
  * <p>
- * Should you desire filtering of the WebSocket object creation due to criteria such as origin or sub-protocol,
- * then you will be required to implement a custom WebSocketCreator implementation.
+ * This can be used for filtering of the WebSocket object creation due to criteria such as origin or sub-protocol,
+ * or for choosing a specific WebSocket object based on the upgrade request.
  * </p>
  */
 public interface WebSocketCreator
 {
     /**
      * Create a websocket from the incoming request.
      *
-     * @param req the request details
-     * @param resp the response details
-     * @param callback callback
-     * @return a websocket object to use, or null if no websocket should be created from this request.
+     * <p>If the creator returns null it is responsible for completing the {@link Callback} and sending a response.
+     * But if the creator intends to return non-null WebSocket object, it MUST NOT write content to the response or
+     * complete the {@link Callback}, but it may modify the response headers.</p>
+     *
+     * @param request the request details
+     * @param response the response details
+     * @param callback the callback, should only be completed by the creator if a null WebSocket object is returned.
+     * @return the WebSocket object, or null to take responsibility to send error response if no WebSocket is to be created.
      */
-    Object createWebSocket(ServerUpgradeRequest req, ServerUpgradeResponse resp, Callback callback);
+    Object createWebSocket(ServerUpgradeRequest request, ServerUpgradeResponse response, Callback callback);
 }

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

@@ -229,6 +229,21 @@ public WebSocketNegotiator getMatchedNegotiator(String target, Consumer<PathSpec
         return negotiator;
     }
 
+    /**
+     * <p>Attempts to find a WebSocket mapping and upgrade a request to WebSocket.</p>
+     *
+     * <p>Returns {@code true} if the WebSocket upgrade is successful and a successful response is generated and the callback
+     * eventually completed, or if the WebSocket upgrade failed and a failure response is generated and the callback eventually
+     * completed. Returns {@code false} if a response is not generated and the caller is responsible for generating a response
+     * and completing the callback.</p>
+     *
+     * @param request the request
+     * @param response the response
+     * @param callback the callback
+     * @param defaultCustomizer the customizer
+     * @return true if the WebSocket upgrade was accepted
+     * @throws IOException there is an error during the upgrade
+     */
     public boolean upgrade(Request request, Response response, Callback callback, Configuration.Customizer defaultCustomizer) throws IOException
     {
         String target = request.getPathInContext();
@@ -239,13 +254,25 @@ public boolean upgrade(Request request, Response response, Callback callback, Co
             request.setAttribute(PathSpec.class.getName(), pathSpec);
         });
 
-        if (negotiator == null)
-            return false;
-
-        // We have an upgrade request
-        return handshaker.upgradeRequest(negotiator, request, response, callback, components, defaultCustomizer);
+        return upgrade(negotiator, request, response, callback, defaultCustomizer);
     }
 
+    /**
+     * <p>Attempts to find a WebSocket mapping and upgrade a request to WebSocket.</p>
+     *
+     * <p>Returns {@code true} if the WebSocket upgrade is successful and a successful response is generated and the callback
+     * eventually completed, or if the WebSocket upgrade failed and a failure response is generated and the callback eventually
+     * completed. Returns {@code false} if a response is not generated and the caller is responsible for generating a response
+     * and completing the callback.</p>
+     *
+     * @param negotiator the negotiator
+     * @param request the request
+     * @param response the response
+     * @param callback the callback
+     * @param defaultCustomizer the customizer
+     * @return true if the WebSocket upgrade was accepted
+     * @throws IOException there is an error during the upgrade
+     */
     public boolean upgrade(WebSocketNegotiator negotiator, Request request, Response response, Callback callback, Configuration.Customizer defaultCustomizer) throws IOException
     {
         if (negotiator == null)

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

@@ -13,38 +13,26 @@
 
 package org.eclipse.jetty.websocket.core.server;
 
-import java.io.IOException;
-import java.util.function.Function;
-
+import org.eclipse.jetty.util.Callback;
 import org.eclipse.jetty.websocket.core.Configuration;
 import org.eclipse.jetty.websocket.core.FrameHandler;
 import org.eclipse.jetty.websocket.core.server.internal.CreatorNegotiator;
 
 public interface WebSocketNegotiator extends Configuration.Customizer
 {
-    FrameHandler negotiate(WebSocketNegotiation negotiation) throws IOException;
-
-    @Override
-    default void customize(Configuration configurable)
-    {
-    }
-
-    static WebSocketNegotiator from(Function<WebSocketNegotiation, FrameHandler> negotiate)
-    {
-        return from(negotiate, null);
-    }
-
-    static WebSocketNegotiator from(Function<WebSocketNegotiation, FrameHandler> negotiate, Configuration.Customizer customizer)
-    {
-        return new AbstractNegotiator(customizer)
-        {
-            @Override
-            public FrameHandler negotiate(WebSocketNegotiation negotiation)
-            {
-                return negotiate.apply(negotiation);
-            }
-        };
-    }
+    /**
+     * <p>Creates a {@link FrameHandler} from the incoming request.</p>
+     *
+     * <p>If the negotiator returns null it is responsible for completing the {@link Callback} and sending a response.
+     * If the negotiator intends to return non-null {@link FrameHandler}, it MUST NOT write content to the response or
+     * complete the {@link Callback}, but it may modify the response headers.</p>
+     *
+     * @param request the request details
+     * @param response the response details
+     * @param callback the callback, should only be completed by the creator if a null WebSocket object is returned.
+     * @return the FrameHandler, or null to take responsibility to send error response if no WebSocket is to be created.
+     */
+    FrameHandler negotiate(ServerUpgradeRequest request, ServerUpgradeResponse response, Callback callback);
 
     static WebSocketNegotiator from(WebSocketCreator creator, FrameHandlerFactory factory)
     {
@@ -56,6 +44,11 @@ static WebSocketNegotiator from(WebSocketCreator creator, FrameHandlerFactory fa
         return new CreatorNegotiator(creator, factory, customizer);
     }
 
+    @Override
+    default void customize(Configuration configurable)
+    {
+    }
+
     abstract class AbstractNegotiator extends Configuration.ConfigurationCustomizer implements WebSocketNegotiator
     {
         final Configuration.Customizer customizer;

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

@@ -43,7 +43,6 @@
 import org.eclipse.jetty.websocket.core.internal.WebSocketConnection;
 import org.eclipse.jetty.websocket.core.internal.WebSocketCoreSession;
 import org.eclipse.jetty.websocket.core.server.Handshaker;
-import org.eclipse.jetty.websocket.core.server.WebSocketNegotiation;
 import org.eclipse.jetty.websocket.core.server.WebSocketNegotiator;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -56,7 +55,7 @@ public abstract class AbstractHandshaker implements Handshaker
     @Override
     public boolean upgradeRequest(WebSocketNegotiator negotiator, Request request, Response response, Callback callback, WebSocketComponents components, Configuration.Customizer defaultCustomizer) throws IOException
     {
-        if (!validateRequest(request))
+        if (!isWebSocketUpgradeRequest(request))
             return false;
 
         WebSocketNegotiation negotiation = newNegotiation(request, response, callback, components);
@@ -68,7 +67,7 @@ public boolean upgradeRequest(WebSocketNegotiator negotiator, Request request, R
             return false;
 
         // Negotiate the FrameHandler
-        FrameHandler handler = negotiator.negotiate(negotiation);
+        FrameHandler handler = negotiator.negotiate(negotiation.getRequest(), negotiation.getResponse(), negotiation.getCallback());
         if (handler == null)
             return true;
 
@@ -156,23 +155,28 @@ public void succeeded()
         return true;
     }
 
-    protected abstract boolean validateRequest(Request request);
-
     protected abstract WebSocketNegotiation newNegotiation(Request request, Response response, Callback callback, WebSocketComponents webSocketComponents);
 
-    protected boolean validateNegotiation(WebSocketNegotiation negotiation)
+    @Override
+    public boolean isWebSocketUpgradeRequest(Request request)
     {
-        if (!negotiation.validateHeaders())
+        String wsVersionHeader = request.getHeaders().get(HttpHeader.SEC_WEBSOCKET_VERSION);
+        if (!WebSocketConstants.SPEC_VERSION_STRING.equals(wsVersionHeader))
         {
             if (LOG.isDebugEnabled())
-                LOG.debug("not upgraded: no upgrade header or connection upgrade {}", negotiation.getRequest());
+                LOG.debug("not upgraded: unsupported version {} {}", wsVersionHeader, request);
             return false;
         }
 
-        if (!WebSocketConstants.SPEC_VERSION_STRING.equals(negotiation.getVersion()))
+        return true;
+    }
+
+    protected boolean validateNegotiation(WebSocketNegotiation negotiation)
+    {
+        if (!negotiation.validateHeaders())
         {
             if (LOG.isDebugEnabled())
-                LOG.debug("not upgraded: unsupported version {} {}", negotiation.getVersion(), negotiation.getRequest());
+                LOG.debug("not upgraded: no upgrade header or connection upgrade {}", negotiation.getRequest());
             return false;
         }
 

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

@@ -13,16 +13,15 @@
 
 package org.eclipse.jetty.websocket.core.server.internal;
 
-import java.io.IOException;
 import java.util.concurrent.atomic.AtomicReference;
 
 import org.eclipse.jetty.server.Context;
+import org.eclipse.jetty.util.Callback;
 import org.eclipse.jetty.websocket.core.FrameHandler;
 import org.eclipse.jetty.websocket.core.server.FrameHandlerFactory;
 import org.eclipse.jetty.websocket.core.server.ServerUpgradeRequest;
 import org.eclipse.jetty.websocket.core.server.ServerUpgradeResponse;
 import org.eclipse.jetty.websocket.core.server.WebSocketCreator;
-import org.eclipse.jetty.websocket.core.server.WebSocketNegotiation;
 import org.eclipse.jetty.websocket.core.server.WebSocketNegotiator;
 
 public class CreatorNegotiator extends WebSocketNegotiator.AbstractNegotiator
@@ -48,28 +47,25 @@ public WebSocketCreator getWebSocketCreator()
     }
 
     @Override
-    public FrameHandler negotiate(WebSocketNegotiation negotiation) throws IOException
+    public FrameHandler negotiate(ServerUpgradeRequest request, ServerUpgradeResponse response, Callback callback)
     {
-        Context context = negotiation.getRequest().getContext();
-        ServerUpgradeRequest upgradeRequest = negotiation.getRequest();
-        ServerUpgradeResponse upgradeResponse = negotiation.getResponse();
-
+        Context context = request.getContext();
         Object websocketPojo;
         try
         {
             AtomicReference<Object> result = new AtomicReference<>();
-            context.run(() -> result.set(creator.createWebSocket(upgradeRequest, upgradeResponse, negotiation.getCallback())));
+            context.run(() -> result.set(creator.createWebSocket(request, response, callback)));
             websocketPojo = result.get();
         }
         catch (Throwable t)
         {
-            negotiation.getCallback().failed(t);
+            callback.failed(t);
             return null;
         }
 
         if (websocketPojo == null)
             return null;
-        return factory.newFrameHandler(websocketPojo, upgradeRequest, upgradeResponse);
+        return factory.newFrameHandler(websocketPojo, request, response);
     }
 
     @Override

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

@@ -33,6 +33,12 @@ public class HandshakerSelector implements Handshaker
     private final RFC6455Handshaker rfc6455 = new RFC6455Handshaker();
     private final RFC8441Handshaker rfc8441 = new RFC8441Handshaker();
 
+    @Override
+    public boolean isWebSocketUpgradeRequest(Request request)
+    {
+        return rfc6455.isWebSocketUpgradeRequest(request) || rfc8441.isWebSocketUpgradeRequest(request);
+    }
+
     @Override
     public boolean upgradeRequest(WebSocketNegotiator negotiator, Request request, Response response, Callback callback, WebSocketComponents components, Configuration.Customizer defaultCustomizer) throws IOException
     {

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

@@ -32,15 +32,14 @@
 import org.eclipse.jetty.websocket.core.internal.WebSocketConnection;
 import org.eclipse.jetty.websocket.core.internal.WebSocketCore;
 import org.eclipse.jetty.websocket.core.internal.WebSocketCoreSession;
-import org.eclipse.jetty.websocket.core.server.WebSocketNegotiation;
 
 public final class RFC6455Handshaker extends AbstractHandshaker
 {
     private static final HttpField UPGRADE_WEBSOCKET = new PreEncodedHttpField(HttpHeader.UPGRADE, "websocket");
     private static final HttpField CONNECTION_UPGRADE = new PreEncodedHttpField(HttpHeader.CONNECTION, HttpHeader.UPGRADE.asString());
 
     @Override
-    protected boolean validateRequest(Request request)
+    public boolean isWebSocketUpgradeRequest(Request request)
     {
         if (!HttpMethod.GET.is(request.getMethod()))
         {
@@ -56,7 +55,7 @@ protected boolean validateRequest(Request request)
             return false;
         }
 
-        return true;
+        return super.isWebSocketUpgradeRequest(request);
     }
 
     @Override

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

@@ -21,7 +21,6 @@
 import org.eclipse.jetty.server.Response;
 import org.eclipse.jetty.util.Callback;
 import org.eclipse.jetty.websocket.core.WebSocketComponents;
-import org.eclipse.jetty.websocket.core.server.WebSocketNegotiation;
 
 public class RFC6455Negotiation extends WebSocketNegotiation
 {