Reject DATA Frames for invalid path requests

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

@@ -176,7 +176,7 @@ public void abortResponse(Throwable cause, boolean cancel) {
     }
 
     @Override
-    public boolean isAggregated() {
+    public boolean needsAggregation() {
         return true;
     }
 

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

@@ -103,7 +103,7 @@ static DecodedHttpRequest of(boolean endOfStream, EventLoop eventLoop, int id, i
     /**
      * Returns whether the request should be fully aggregated before passed to the {@link HttpServerHandler}.
      */
-    boolean isAggregated();
+    boolean needsAggregation();
 
     /**
      * Returns the {@link ExchangeType} that determines whether to stream an {@link HttpRequest} or

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

@@ -188,7 +188,7 @@ public void abortResponse(Throwable cause, boolean cancel) {
     }
 
     @Override
-    public boolean isAggregated() {
+    public boolean needsAggregation() {
         return false;
     }
 

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

@@ -223,7 +223,7 @@ public void channelRead(ChannelHandlerContext ctx, Object msg) throws Exception
                                                            keepAlive, inboundTrafficController, routingCtx);
 
                     // An aggregating request will be fired after all objects are collected.
-                    if (!req.isAggregated()) {
+                    if (!req.needsAggregation()) {
                         ctx.fireChannelRead(req);
                     }
                 } else {
@@ -286,7 +286,7 @@ public void channelRead(ChannelHandlerContext ctx, Object msg) throws Exception
                     }
 
                     decodedReq.close();
-                    if (decodedReq.isAggregated()) {
+                    if (decodedReq.needsAggregation()) {
                         // An aggregated request is now ready to be fired.
                         ctx.fireChannelRead(decodedReq);
                     }

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

@@ -178,16 +178,23 @@ public void onHeadersRead(ChannelHandlerContext ctx, int streamId, Http2Headers
                                         inboundTrafficController, routingCtx);
             requests.put(streamId, req);
             // An aggregating request will be fired later after all objects are collected.
-            if (!req.isAggregated()) {
+            if (!req.needsAggregation()) {
                 ctx.fireChannelRead(req);
             }
         } else {
+            if (!(req instanceof DecodedHttpRequestWriter)) {
+                // Silently ignore the following HEADERS Frames of non-DecodedHttpRequestWriter. The request
+                // stream is closed when receiving the first HEADERS Frame and some responses might be sent
+                // already.
+                logger.debug("{} received a HEADERS Frame for an invalid stream: {}", ctx.channel(), streamId);
+                return;
+            }
             final HttpHeaders trailers = ArmeriaHttpUtil.toArmeria(nettyHeaders, true, endOfStream);
             final DecodedHttpRequestWriter decodedReq = (DecodedHttpRequestWriter) req;
             try {
                 // Trailers is received. The decodedReq will be automatically closed.
                 decodedReq.write(trailers);
-                if (req.isAggregated()) {
+                if (req.needsAggregation()) {
                     // An aggregated request can be fired now.
                     ctx.fireChannelRead(req);
                 }
@@ -251,11 +258,19 @@ public int onDataRead(
         }
 
         final int dataLength = data.readableBytes();
+        if (!(req instanceof DecodedHttpRequestWriter)) {
+            // Silently ignore the following DATA Frames of non-DecodedHttpRequestWriter. The request stream is
+            // closed when receiving the HEADERS Frame and some responses might be sent already.
+            logger.debug("{} received a DATA Frame for an invalid stream: {}. headers: {}",
+                         ctx.channel(), streamId, req.headers());
+            return dataLength + padding;
+        }
+
         if (dataLength == 0) {
             // Received an empty DATA frame
             if (endOfStream) {
                 req.close();
-                if (req.isAggregated()) {
+                if (req.needsAggregation()) {
                     ctx.fireChannelRead(req);
                 }
             }
@@ -291,7 +306,7 @@ public int onDataRead(
             try {
                 // The decodedReq will be automatically closed if endOfStream is true.
                 decodedReq.write(HttpData.wrap(data.retain()).withEndOfStream(endOfStream));
-                if (endOfStream && decodedReq.isAggregated()) {
+                if (endOfStream && decodedReq.needsAggregation()) {
                     // An aggregated request is now ready to be fired.
                     ctx.fireChannelRead(req);
                 }

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

@@ -189,7 +189,7 @@ public void abortResponse(Throwable cause, boolean cancel) {
     }
 
     @Override
-    public boolean isAggregated() {
+    public boolean needsAggregation() {
         return false;
     }
 

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

@@ -0,0 +1,102 @@
+/*
+ * 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.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(Http2RequestDecoder.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 logMessage = event.getFormattedMessage();
+                        return event.getLevel().equals(Level.DEBUG) &&
+                               logMessage.contains("received a DATA Frame for an invalid stream") &&
+                               logMessage.contains(invalidPath);
+                    });
+        });
+    }
+}