4.x: Ignore protocol upgrades when initial request contains a payload (…

…helidon-io#9359) (helidon-io#9412)

* Ignore protocol upgrades when initial request contains a payload. Payloads should not be sent until after the negotiation is complete to avoid interfering with the upgrade mechanism. Logs decision to ignore upgrade. New test.

Co-authored-by: Santiago Pericas-Geertsen <[email protected]>

webserver/tests/upgrade/src/test/java/io/helidon/webserver/tests/upgrade/test/Http2UpgradeTest.java

@@ -0,0 +1,84 @@
+/*
+ * Copyright (c) 2024 Oracle and/or its affiliates.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package io.helidon.webserver.tests.upgrade.test;
+
+import io.helidon.http.HeaderNames;
+import io.helidon.http.Status;
+import io.helidon.logging.common.LogConfig;
+import io.helidon.webclient.http1.Http1Client;
+import io.helidon.webclient.http2.Http2Client;
+import io.helidon.webclient.http2.Http2ClientProtocolConfig;
+import io.helidon.webserver.WebServer;
+import io.helidon.webserver.http.HttpRouting;
+import io.helidon.webserver.http2.Http2Route;
+
+import org.junit.jupiter.api.Test;
+
+import static io.helidon.http.Method.POST;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.is;
+
+class Http2UpgradeTest {
+
+    @Test
+    void testHttp2UpgradeWithPayload() {
+        LogConfig.configureRuntime();
+
+        WebServer webServer = null;
+        try {
+            // define HTTP/2 route only
+            var routing = HttpRouting.builder()
+                    .route(Http2Route.route(POST, "/echo", (req, res) -> {
+                        String s = req.content().as(String.class);
+                        res.send(s);
+                    }));
+
+            // start server and get port
+            webServer = WebServer.builder()
+                    .routing(routing)
+                    .build()
+                    .start();
+            int port = webServer.port();
+
+            // verify not found when payload in initial request and no HTTP/1 route
+            Http1Client http1Client = Http1Client.builder()
+                    .baseUri("http://localhost:" + port + "/echo")
+                    .build();
+            try (var res = http1Client.post()
+                    .header(HeaderNames.UPGRADE, "h2c")     // upgrade pretend
+                    .submit("payload")) {
+                assertThat(res.status(), is(Status.NOT_FOUND_404));
+            }
+
+            // verify success with smart client when payload is sent after upgrade
+            Http2Client http2Client = Http2Client.builder()
+                    .protocolConfig(Http2ClientProtocolConfig.builder()
+                                            .priorKnowledge(false)
+                                            .build())
+                    .baseUri("http://localhost:" + port + "/echo")
+                    .build();
+            try (var res = http2Client.post().submit("payload")) {
+                assertThat(res.status(), is(Status.OK_200));
+                assertThat(res.entity().as(String.class), is("payload"));
+            }
+        } finally {
+            if (webServer != null) {
+                webServer.stop();
+            }
+        }
+    }
+}

webserver/webserver/src/main/java/io/helidon/webserver/http1/Http1Connection.java

@@ -66,6 +66,7 @@
 import static io.helidon.http.HeaderNames.X_FORWARDED_FOR;
 import static io.helidon.http.HeaderNames.X_FORWARDED_PORT;
 import static io.helidon.http.HeaderNames.X_HELIDON_CN;
+import static java.lang.System.Logger.Level.DEBUG;
 import static java.lang.System.Logger.Level.TRACE;
 import static java.lang.System.Logger.Level.WARNING;
 
@@ -180,8 +181,8 @@ public void handle(Limit limit) throws InterruptedException {
                     }
                 }
 
-                if (canUpgrade) {
-                    if (headers.contains(HeaderNames.UPGRADE)) {
+                if (canUpgrade && headers.contains(HeaderNames.UPGRADE)) {
+                    if (!upgradeHasEntity(headers)) {
                         Http1Upgrader upgrader = upgradeProviderMap.get(headers.get(HeaderNames.UPGRADE).get());
                         if (upgrader != null) {
                             ServerConnection upgradeConnection = upgrader.upgrade(ctx, prologue, headers);
@@ -196,6 +197,8 @@ public void handle(Limit limit) throws InterruptedException {
                                 return;
                             }
                         }
+                    } else {
+                        ctx.log(LOGGER, DEBUG, "Protocol upgrade for a request with a payload ignored");
                     }
                 }
 
@@ -283,6 +286,19 @@ void reset() {
         currentEntitySizeRead = 0;
     }
 
+    /**
+     * Only accept protocol upgrades if no entity is present. Otherwise, a successful
+     * upgrade may result in the request entity interpreted as part of the new protocol
+     * data, resulting in a failure.
+     *
+     * @param headers the HTTP headers in the prologue
+     * @return whether to accept or reject the upgrade
+     */
+    static boolean upgradeHasEntity(WritableHeaders<?> headers) {
+        return headers.contains(HeaderNames.CONTENT_LENGTH) && !headers.contains(HeaderValues.CONTENT_LENGTH_ZERO)
+                || headers.contains(HeaderValues.TRANSFER_ENCODING_CHUNKED);
+    }
+
     static void validateHostHeader(HttpPrologue prologue, WritableHeaders<?> headers, boolean fullValidation) {
         if (fullValidation) {
             try {