fix(router): fix header propagation for never-join headers like Set-Cookie (#1396)

Author: dotansimha

.changeset/propagate_all_set_cookie_values_from_a_single_subgraph.md

@@ -0,0 +1,12 @@
+---
+hive-router-plan-executor: patch
+hive-router: patch
+---
+
+# Propagate all multi-instance headers from a single subgraph response
+
+When a subgraph responded with multiple instances of a never-join header (`Set-Cookie` or `WWW-Authenticate`), the router only forwarded one of them to the client and silently dropped the rest.
+
+The fix is to propagate all values of a never-join header as separate header fields end-to-end, rather than just the first value.
+
+Fixes https://github.com/graphql-hive/router/issues/1388

bin/router/src/shared_state.rs

@@ -19,6 +19,7 @@ use hive_router_plan_executor::executors::common::InboundRequestFingerprint;
 use hive_router_plan_executor::extensions::{
     compile::compile_extensions_plan, plan::ExtensionsPlan,
 };
+use hive_router_plan_executor::headers::sanitizer::is_never_join_header;
 use hive_router_plan_executor::headers::{
     compile::compile_headers_plan, errors::HeaderRuleCompileError, plan::HeaderRulesPlan,
 };
@@ -147,9 +148,15 @@ impl From<SharedRouterSingleResponse> for web::HttpResponse {
     fn from(shared_response: SharedRouterSingleResponse) -> Self {
         let mut response = web::HttpResponse::Ok();
         response.status(shared_response.status);
+
         for (header_name, header_value) in shared_response.headers.iter() {
-            response.set_header(header_name, header_value);
+            if is_never_join_header(header_name) {
+                response.header(header_name, header_value);
+            } else {
+                response.set_header(header_name, header_value);
+            }
         }
+
         response.body(shared_response.body)
     }
 }
@@ -264,7 +271,11 @@ impl SharedRouterStreamResponse {
         let mut response = web::HttpResponse::Ok();
 
         for (header_name, header_value) in self.headers.iter() {
-            response.set_header(header_name, header_value);
+            if is_never_join_header(header_name) {
+                response.header(header_name, header_value);
+            } else {
+                response.set_header(header_name, header_value);
+            }
         }
 
         // we set content type after so that we can override the shared header

e2e/src/header_propagation.rs

@@ -289,6 +289,70 @@ mod header_propagation_e2e_tests {
         );
     }
 
+    // Regression test for https://github.com/graphql-hive/router/issues/1388
+    #[ntex::test]
+    async fn should_propagate_all_set_cookie_values_from_a_single_subgraph() {
+        let subgraphs = TestSubgraphs::builder().build().start().await;
+        let mut accounts_server = mockito::Server::new_async().await;
+        let host = accounts_server.host_with_port();
+
+        let router = TestRouter::builder()
+            .inline_config(format!(
+                r#"
+                  supergraph:
+                    source: file
+                    path: supergraph.graphql
+                  headers:
+                    all:
+                      response:
+                        - propagate:
+                            named: set-cookie
+                            algorithm: append
+                  override_subgraph_urls:
+                      subgraphs:
+                          accounts:
+                              url: "http://{host}/accounts"
+                  "#
+            ))
+            .with_subgraphs(&subgraphs)
+            .build()
+            .start()
+            .await;
+
+        let accounts_response_mock = accounts_server
+            .mock("POST", "/accounts")
+            .with_status(200)
+            .with_header("content-type", "application/json")
+            .with_header("set-cookie", "access_token=abc; HttpOnly; Secure; Path=/")
+            .with_header("set-cookie", "refresh_token=xyz; HttpOnly; Secure; Path=/")
+            .with_body(r#"{"data":{"users":[]}}"#)
+            .expect(1)
+            .create();
+
+        let res = router
+            .send_graphql_request("{ users { id } }", None, None)
+            .await;
+
+        assert!(res.status().is_success(), "Expected 200 OK");
+
+        accounts_response_mock.assert();
+
+        let cookies: Vec<&str> = res
+            .headers()
+            .get_all("set-cookie")
+            .into_iter()
+            .filter_map(|v| v.to_str().ok())
+            .collect();
+
+        assert_eq!(
+            cookies.len(),
+            2,
+            "expected both Set-Cookie values to be propagated, got {cookies:?}"
+        );
+        assert!(cookies.contains(&"access_token=abc; HttpOnly; Secure; Path=/"));
+        assert!(cookies.contains(&"refresh_token=xyz; HttpOnly; Secure; Path=/"));
+    }
+
     #[ntex::test]
     async fn should_not_override_router_selected_content_type() {
         let subgraphs = TestSubgraphs::builder().build().start().await;

lib/executor/src/headers/response.rs

@@ -95,7 +95,18 @@ impl ApplyResponseHeader for ResponsePropagateNamed {
                 continue;
             }
 
-            if let Some(header_value) = ctx.subgraph_headers.get(header_name) {
+            if is_never_join_header(header_name) {
+                // Never-join-headers (like `set-cookie`) can appear multiple
+                // times in a single subgraph response, so we need to propagate them all
+                for header_value in ctx.subgraph_headers.get_all(header_name) {
+                    matched = true;
+                    accumulator.write(
+                        self.rename.as_ref().unwrap_or(header_name),
+                        header_value,
+                        self.strategy,
+                    );
+                }
+            } else if let Some(header_value) = ctx.subgraph_headers.get(header_name) {
                 matched = true;
                 accumulator.write(
                     self.rename.as_ref().unwrap_or(header_name),