Repository: netty/netty
Issue: Change HttpServerUpgradeHandler.UpgradeCodec to allow aborting upgrade
Labels: defect

I noticed that `Http2ServerUpgradeCodec.prepareUpgradeResponse()` [doesn't quite have the right behavior](https://github.com/netty/netty/blob/43ebbc3fa065155fa67732b0cbd7c12843b0f3f7/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2ServerUpgradeCodec.java#L95), since failures in the UPGRADE protocol generally abort the upgrade instead of responding with an error. Primarily:

> A server MUST NOT upgrade the connection to HTTP/2 if this header field is not present or if more than one is present.

That said, the spec does not specify what should happen if the value of `HTTP2-Settings` is invalid (either invalid base64 or invalid SETTINGS payload).

To fix this behavior seems like it would be necessary to change the UpgradeCodec interface in codec-http. `prepareUpgradeResponse()` could return a `boolean` to indicate whether the upgrade is successful. If `false`, `HttpServerUpgradeHandler.upgrade()` would return false and the connection would continue using HTTP/1.


Issue comments:
Let me take care.

Fixed


Pull request review comments:
not anything I changed so I would just keep it.

How about:

```
Performs the preparatory steps required for performing a protocol update. This method returns a
boolean value to proceed or abort the upgrade in progress. If {@code false} is returned, the
upgrade is aborted and the {@code upgradeRequest} will be passed through the inbound pipeline
as if no upgrade was performed. If {@code true} is returned, the upgrade will proceed to the next
step which invokes {@link #upgradeTo}. When returning {@code true}, you can add headers to
the {@code upgradeHeaders} so that they are added to the 101 Switching protocols response.
```

I'm not a native speaker, so please feel free to fix this as well.

+1 good docs :)

`Performs the preparatory steps required for performing a protocol update.` -> `Prepares the {@code upgradeHeaders} for a protocol update based upon the contents of {@code upgradeRequest}`.

"Perform" is used multiple times ... consider being more explicit upfront.

nit: invert condition and then return `false`. That matches the rest of the method and reduces the indentation level.

Checking if info is enabled here seems useless. Why not just log blindly?

make constructor package private?

yep why not... 

sure why not ....


Patch that resolved the issue:
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/HttpServerUpgradeHandler.java b/codec-http/src/main/java/io/netty/handler/codec/http/HttpServerUpgradeHandler.java
index 691d37e7675..47670bf14eb 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/HttpServerUpgradeHandler.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/HttpServerUpgradeHandler.java
@@ -60,10 +60,15 @@ public interface UpgradeCodec {
         Collection<CharSequence> requiredUpgradeHeaders();
 
         /**
-         * Adds any headers to the 101 Switching protocols response that are appropriate for this protocol.
+         * Prepares the {@code upgradeHeaders} for a protocol update based upon the contents of {@code upgradeRequest}.
+         * This method returns a boolean value to proceed or abort the upgrade in progress. If {@code false} is
+         * returned, the upgrade is aborted and the {@code upgradeRequest} will be passed through the inbound pipeline
+         * as if no upgrade was performed. If {@code true} is returned, the upgrade will proceed to the next
+         * step which invokes {@link #upgradeTo}. When returning {@code true}, you can add headers to
+         * the {@code upgradeHeaders} so that they are added to the 101 Switching protocols response.
          */
-        void prepareUpgradeResponse(ChannelHandlerContext ctx, FullHttpRequest upgradeRequest,
-                                    FullHttpResponse upgradeResponse);
+        boolean prepareUpgradeResponse(ChannelHandlerContext ctx, FullHttpRequest upgradeRequest,
+                                    HttpHeaders upgradeHeaders);
 
         /**
          * Performs an HTTP protocol upgrade from the source codec. This method is responsible for
@@ -103,7 +108,7 @@ public static final class UpgradeEvent implements ReferenceCounted {
         private final CharSequence protocol;
         private final FullHttpRequest upgradeRequest;
 
-        private UpgradeEvent(CharSequence protocol, FullHttpRequest upgradeRequest) {
+        UpgradeEvent(CharSequence protocol, FullHttpRequest upgradeRequest) {
             this.protocol = protocol;
             this.upgradeRequest = upgradeRequest;
         }
@@ -304,13 +309,15 @@ private boolean upgrade(final ChannelHandlerContext ctx, final FullHttpRequest r
             }
         }
 
-        // Create the user event to be fired once the upgrade completes.
-        final UpgradeEvent event = new UpgradeEvent(upgradeProtocol, request);
-
         // Prepare and send the upgrade response. Wait for this write to complete before upgrading,
         // since we need the old codec in-place to properly encode the response.
         final FullHttpResponse upgradeResponse = createUpgradeResponse(upgradeProtocol);
-        upgradeCodec.prepareUpgradeResponse(ctx, request, upgradeResponse);
+        if (!upgradeCodec.prepareUpgradeResponse(ctx, request, upgradeResponse.headers())) {
+            return false;
+        }
+
+        // Create the user event to be fired once the upgrade completes.
+        final UpgradeEvent event = new UpgradeEvent(upgradeProtocol, request);
 
         final UpgradeCodec finalUpgradeCodec = upgradeCodec;
         ctx.writeAndFlush(upgradeResponse).addListener(new ChannelFutureListener() {
diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2ServerUpgradeCodec.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2ServerUpgradeCodec.java
index fa00076ad71..d2d36f13473 100644
--- a/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2ServerUpgradeCodec.java
+++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2ServerUpgradeCodec.java
@@ -20,8 +20,11 @@
 import io.netty.handler.codec.base64.Base64;
 import io.netty.handler.codec.http.FullHttpRequest;
 import io.netty.handler.codec.http.FullHttpResponse;
+import io.netty.handler.codec.http.HttpHeaders;
 import io.netty.handler.codec.http.HttpServerUpgradeHandler;
 import io.netty.util.CharsetUtil;
+import io.netty.util.internal.logging.InternalLogger;
+import io.netty.util.internal.logging.InternalLoggerFactory;
 
 import java.nio.CharBuffer;
 import java.util.Collection;
@@ -29,7 +32,6 @@
 import java.util.List;
 
 import static io.netty.handler.codec.base64.Base64Dialect.URL_SAFE;
-import static io.netty.handler.codec.http.HttpResponseStatus.BAD_REQUEST;
 import static io.netty.handler.codec.http2.Http2CodecUtil.FRAME_HEADER_LENGTH;
 import static io.netty.handler.codec.http2.Http2CodecUtil.HTTP_UPGRADE_SETTINGS_HEADER;
 import static io.netty.handler.codec.http2.Http2CodecUtil.writeFrameHeader;
@@ -41,6 +43,7 @@
  */
 public class Http2ServerUpgradeCodec implements HttpServerUpgradeHandler.UpgradeCodec {
 
+    private static final InternalLogger logger = InternalLoggerFactory.getInstance(Http2ServerUpgradeCodec.class);
     private static final List<CharSequence> REQUIRED_UPGRADE_HEADERS =
             Collections.singletonList(HTTP_UPGRADE_SETTINGS_HEADER);
 
@@ -77,8 +80,8 @@ public Collection<CharSequence> requiredUpgradeHeaders() {
     }
 
     @Override
-    public void prepareUpgradeResponse(ChannelHandlerContext ctx, FullHttpRequest upgradeRequest,
-            FullHttpResponse upgradeResponse) {
+    public boolean prepareUpgradeResponse(ChannelHandlerContext ctx, FullHttpRequest upgradeRequest,
+            HttpHeaders headers) {
         try {
             // Decode the HTTP2-Settings header and set the settings on the handler to make
             // sure everything is fine with the request.
@@ -89,11 +92,11 @@ public void prepareUpgradeResponse(ChannelHandlerContext ctx, FullHttpRequest up
             }
             Http2Settings settings = decodeSettingsHeader(ctx, upgradeHeaders.get(0));
             connectionHandler.onHttpServerUpgrade(settings);
-            // Everything looks good, no need to modify the response.
-        } catch (Throwable e) {
-            // Send a failed response back to the client.
-            upgradeResponse.setStatus(BAD_REQUEST);
-            upgradeResponse.headers().clear();
+            // Everything looks good.
+            return true;
+        } catch (Throwable cause) {
+            logger.info("Error during upgrade to HTTP/2", cause);
+            return false;
         }
     }
 


Test patch: