Merge pull request #38185 from geoand/#38155

Make sure `quarkus.http.filter` headers don't remove existing headers

extensions/vertx-http/runtime/src/main/java/io/quarkus/vertx/http/runtime/options/HttpServerCommonHandlers.java

@@ -3,6 +3,8 @@
 import static io.quarkus.vertx.core.runtime.context.VertxContextSafetyToggle.setCurrentContextSafe;
 import static io.quarkus.vertx.http.runtime.TrustedProxyCheck.allowAll;
 
+import java.util.LinkedHashSet;
+import java.util.List;
 import java.util.Locale;
 import java.util.Map;
 import java.util.function.Supplier;
@@ -23,6 +25,7 @@
 import io.smallrye.common.vertx.VertxContext;
 import io.vertx.core.Context;
 import io.vertx.core.Handler;
+import io.vertx.core.MultiMap;
 import io.vertx.core.Vertx;
 import io.vertx.core.http.HttpMethod;
 import io.vertx.core.http.HttpServerRequest;
@@ -122,9 +125,10 @@ public static void applyFilters(Map<String, FilterConfig> filtersInConfig, Route
                             .handler(new Handler<RoutingContext>() {
                                 @Override
                                 public void handle(RoutingContext event) {
-                                    event.response().headers().setAll(headers);
+                                    addFilterHeaders(event, headers);
                                     event.next();
                                 }
+
                             });
                 } else {
                     for (var method : methods.get()) {
@@ -133,7 +137,7 @@ public void handle(RoutingContext event) {
                                 .handler(new Handler<RoutingContext>() {
                                     @Override
                                     public void handle(RoutingContext event) {
-                                        event.response().headers().setAll(headers);
+                                        addFilterHeaders(event, headers);
                                         event.next();
                                     }
                                 });
@@ -143,6 +147,27 @@ public void handle(RoutingContext event) {
         }
     }
 
+    private static void addFilterHeaders(RoutingContext event, Map<String, String> headers) {
+        for (var entry : headers.entrySet()) {
+            String key = entry.getKey();
+            String value = entry.getValue();
+            MultiMap responseHeaders = event.response().headers();
+            List<String> oldValues = responseHeaders.getAll(key);
+            if (oldValues.isEmpty()) {
+                responseHeaders.set(key, value);
+            } else {
+                // we need to make sure the new value is not duplicated
+                var newValues = new LinkedHashSet<String>(oldValues);
+                boolean added = newValues.add(value);
+                if (added) {
+                    responseHeaders.set(key, newValues);
+                } else {
+                    // we don't need to do anything here as the value was already in the set
+                }
+            }
+        }
+    }
+
     public static void applyHeaders(Map<String, HeaderConfig> headers, Router httpRouteRouter) {
         if (!headers.isEmpty()) {
             // Creates a handler for each header entry

integration-tests/vertx-http/pom.xml

@@ -52,6 +52,11 @@
             <artifactId>smallrye-mutiny-vertx-web-client</artifactId>
             <scope>test</scope>
         </dependency>
+        <dependency>
+            <groupId>org.assertj</groupId>
+            <artifactId>assertj-core</artifactId>
+            <scope>test</scope>
+        </dependency>
 
         <!-- Minimal test dependencies to *-deployment artifacts for consistent build order -->
         <dependency>

integration-tests/vertx-http/src/main/resources/application.properties

@@ -29,6 +29,15 @@ quarkus.http.filter.cached.header."Cache-Control"=max-age=31536000
 quarkus.http.filter.cached.matches=/filter/(an.*|override)
 quarkus.http.filter.cached.methods=GET
 
+# See io.quarkus.it.vertx.FilterTestCase.testCorsRequest
+quarkus.http.filter.cors.header."Cache-Control"=max-age=31536000
+quarkus.http.filter.cors.header."Access-Control-Allow-Origin"=https://example.org/
+quarkus.http.filter.cors.header."Access-Control-Allow-Methods"=TEST
+quarkus.http.filter.cors.matches=/filter/any
+quarkus.http.filter.cors.methods=GET
+# we want this filter to run after cors, so it can have the chance to see the values the cors filter set
+quarkus.http.filter.cors.order=400
+
 # See io.quarkus.it.vertx.FilterTestCase.testPathOrder
 quarkus.http.filter.just-order.order=10
 quarkus.http.filter.just-order.header."Cache-Control"=max-age=5000

integration-tests/vertx-http/src/test/java/io/quarkus/it/vertx/FilterTestCase.java

@@ -4,9 +4,13 @@
 import static org.hamcrest.Matchers.emptyOrNullString;
 import static org.hamcrest.Matchers.is;
 
+import java.util.List;
+
+import org.assertj.core.api.Assertions;
 import org.junit.jupiter.api.Test;
 
 import io.quarkus.test.junit.QuarkusTest;
+import io.restassured.http.Header;
 
 @QuarkusTest
 public class FilterTestCase {
@@ -22,6 +26,25 @@ void testAnyPathAdditionalHeadersGet() {
 
     }
 
+    @Test
+    void testCorsRequest() {
+        List<Header> corsMethods = given()
+                .header("Origin", "https://example.org/")
+                .header("Access-Control-Request-Method", "GET")
+                .get("/filter/any")
+                .then()
+                .statusCode(200)
+                .header("Cache-Control", is("max-age=31536000"))
+                .header("Access-Control-Allow-Origin",
+                        is("https://example.org/")) // this same header was added by cors but also by a property, we should only have one value
+                .body(is("ok"))
+                .extract()
+                .headers().getList("Access-Control-Allow-Methods");
+        Assertions.assertThat(corsMethods.stream().map(Header::getValue)).containsExactly("POST,GET,PUT,OPTIONS,DELETE",
+                "TEST");
+
+    }
+
     @Test
     void testAnyPathAdditionalHeadersHead() {
         // HEAD requests should not include the header