Reject DATA Frames for invalid path requests

core/src/main/java/com/linecorp/armeria/server/Http2RequestDecoder.java

@@ -182,6 +182,10 @@ public void onHeadersRead(ChannelHandlerContext ctx, int streamId, Http2Headers
                 ctx.fireChannelRead(req);
             }
         } else {
+            if (!(req instanceof DecodedHttpRequestWriter)) {
+                throw connectionError(PROTOCOL_ERROR,
+                                      "received an HEADERS frame for an invalid stream: %d", streamId);
+            }
             final HttpHeaders trailers = ArmeriaHttpUtil.toArmeria(nettyHeaders, true, endOfStream);
             final DecodedHttpRequestWriter decodedReq = (DecodedHttpRequestWriter) req;
             try {
@@ -193,9 +197,10 @@ public void onHeadersRead(ChannelHandlerContext ctx, int streamId, Http2Headers
                 }
             } catch (Throwable t) {
                 decodedReq.close(t);
-                throw connectionError(INTERNAL_ERROR, t, "failed to consume a HEADERS frame");
+                throw connectionError(INTERNAL_ERROR, t, "failed to consume an HEADERS frame");
             }
         }
+
     }
 
     @Override
@@ -249,6 +254,17 @@ public int onDataRead(
             throw connectionError(PROTOCOL_ERROR, "received a DATA Frame for an unknown stream: %d",
                                   streamId);
         }
+        if (!(req instanceof DecodedHttpRequestWriter)) {
+            // A DATA Frame is not allowed for non-DecodedHttpRequestWriter.
+            if (logger.isDebugEnabled()) {
+                throw connectionError(PROTOCOL_ERROR,
+                                      "received a DATA Frame for an invalid stream: %d. headers: %s",
+                                      streamId, req.headers());
+            } else {
+                throw connectionError(PROTOCOL_ERROR,
+                                      "received a DATA Frame for an invalid stream: %d", streamId);
+            }
+        }
 
         final int dataLength = data.readableBytes();
         if (dataLength == 0) {

core/src/main/java/com/linecorp/armeria/server/HttpServerHandler.java

@@ -498,9 +498,9 @@ private void respond(ChannelHandlerContext ctx, ServiceRequestContext reqCtx,
             respond(reqCtx, false, resHeaders, resContent, cause).addListener(CLOSE);
         }
 
-        if (!isReading) {
+//        if (!isReading) {
             ctx.flush();
-        }
+//        }
     }
 
     private ChannelFuture respond(ServiceRequestContext reqCtx, boolean addKeepAlive,

core/src/test/java12/com/linecorp/armeria/server/InvalidPathWithDataTest.java

@@ -0,0 +1,104 @@
+/*
+ * Copyright 2022 LINE Corporation
+ *
+ * LINE Corporation licenses this file to you 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:
+ *
+ *   https://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 com.linecorp.armeria.server;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.awaitility.Awaitility.await;
+
+import java.net.http.HttpClient;
+import java.net.http.HttpClient.Version;
+import java.net.http.HttpResponse.BodyHandlers;
+
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.RegisterExtension;
+import org.slf4j.LoggerFactory;
+
+import com.linecorp.armeria.common.HttpResponse;
+import com.linecorp.armeria.internal.common.AbstractHttp2ConnectionHandler;
+import com.linecorp.armeria.internal.common.PathAndQuery;
+import com.linecorp.armeria.server.logging.LoggingService;
+import com.linecorp.armeria.testing.junit5.server.ServerExtension;
+
+import ch.qos.logback.classic.Level;
+import ch.qos.logback.classic.Logger;
+import ch.qos.logback.classic.spi.ILoggingEvent;
+import ch.qos.logback.core.read.ListAppender;
+
+class InvalidPathWithDataTest {
+
+    @RegisterExtension
+    static ServerExtension server = new ServerExtension() {
+        @Override
+        protected void configure(ServerBuilder sb) {
+            sb.requestTimeoutMillis(0);
+            sb.decorator(LoggingService.newDecorator());
+            sb.service("/foo", (ctx, req) -> {
+                return HttpResponse.from(req.aggregate().thenApply(agg -> HttpResponse.of(agg.contentUtf8())));
+            });
+        }
+    };
+
+    @Test
+    void invalidPath() throws Exception {
+        final String invalidPath = "/foo?download=../../secret.txt";
+        final PathAndQuery pathAndQuery = PathAndQuery.parse(invalidPath);
+        assertThat(pathAndQuery).isNull();
+
+        final HttpClient client = HttpClient.newHttpClient();
+
+        // Send a normal request to complete an upgrade request successfully.
+        final java.net.http.HttpRequest normalRequest =
+                java.net.http.HttpRequest.newBuilder()
+                                         .version(Version.HTTP_2)
+                                         .uri(server.httpUri().resolve("/foo"))
+                                         .GET()
+                                         .build();
+
+        final ListAppender<ILoggingEvent> logWatcher = new ListAppender<>();
+        logWatcher.start();
+        final Logger logger = (Logger) LoggerFactory.getLogger(AbstractHttp2ConnectionHandler.class);
+        logger.setLevel(Level.DEBUG);
+        logger.addAppender(logWatcher);
+
+        final String bodyNormal = client.send(normalRequest, BodyHandlers.ofString()).body();
+        assertThat(bodyNormal).isEmpty();
+
+        final java.net.http.HttpRequest invalidRequest =
+                java.net.http.HttpRequest.newBuilder()
+                                         .version(Version.HTTP_2)
+                                         .uri(server.httpUri().resolve(invalidPath))
+                                         .POST(java.net.http.HttpRequest.BodyPublishers.ofString(
+                                                 "Hello Armeria!"))
+                                         .build();
+
+        final java.net.http.HttpResponse<String> response =
+                client.send(invalidRequest, BodyHandlers.ofString());
+        assertThat(response.statusCode()).isEqualTo(400);
+        assertThat(response.body()).contains("Invalid request path");
+
+        await().untilAsserted(() -> {
+            assertThat(logWatcher.list)
+                    .anyMatch(event -> {
+                        final String errorMessage = event.getThrowableProxy()
+                                                         .getMessage();
+                        return event.getLevel().equals(Level.WARN) &&
+                               errorMessage.contains("received a DATA Frame for an invalid stream") &&
+                               errorMessage.contains(invalidPath);
+                    });
+        });
+    }
+}

testing-internal/src/main/resources/logback-test.xml

@@ -30,7 +30,7 @@
 
   <logger name="com.linecorp.armeria" level="DEBUG" />
   <logger name="com.linecorp.armeria.logging.traffic.server" level="OFF" />
-  <logger name="com.linecorp.armeria.logging.traffic.server.http2" level="OFF" />
+  <logger name="com.linecorp.armeria.logging.traffic.server.http2" level="TRACE" />
   <logger name="com.linecorp.armeria.logging.traffic.client" level="OFF" />
   <logger name="com.linecorp.armeria.logging.traffic.client.http2" level="OFF" />
   <logger name="com.linecorp.armeria.internal.common.Http2GoAwayHandler" level="INFO" />