Log warning instead of returning error when non-utf8 header passed through externalize_header_map

.changesets/fix_rohan_b99_allow_non_utf8_characters_in_headers.md

@@ -0,0 +1,6 @@
+### Log warning instead of returning error when non-utf8 header passed through externalize_header_map ([PR #8828](https://github.com/apollographql/router/pull/8828))
+
+- A warning log with the name of the header will now be emitted
+- The remaining valid headers will be returned now instead of an error, which is more consistent with the router's default behaviour when a coprocessor is not used.
+
+By [@rohan-b99](https://github.com/rohan-b99) in https://github.com/apollographql/router/pull/8828

apollo-router/src/plugins/coprocessor/execution.rs

@@ -212,8 +212,7 @@ where
 
     let headers_to_send = request_config
         .headers
-        .then(|| externalize_header_map(&parts.headers))
-        .transpose()?;
+        .then(|| externalize_header_map(&parts.headers));
 
     let body_to_send = request_config
         .body
@@ -369,8 +368,7 @@ where
     // Encode headers, body, status, context, sdl to create a payload
     let headers_to_send = response_config
         .headers
-        .then(|| externalize_header_map(&parts.headers))
-        .transpose()?;
+        .then(|| externalize_header_map(&parts.headers));
     let body_to_send = response_config
         .body
         .then(|| serde_json_bytes::to_value(&first).expect("serialization will not fail"));

apollo-router/src/plugins/coprocessor/mod.rs

@@ -854,8 +854,7 @@ where
 
     let headers_to_send = request_config
         .headers
-        .then(|| externalize_header_map(&parts.headers))
-        .transpose()?;
+        .then(|| externalize_header_map(&parts.headers));
 
     // HTTP GET requests don't have a body
     let body_to_send = request_config
@@ -1036,8 +1035,7 @@ where
     // Encode headers, body, status, context, sdl to create a payload
     let headers_to_send = response_config
         .headers
-        .then(|| externalize_header_map(&parts.headers))
-        .transpose()?;
+        .then(|| externalize_header_map(&parts.headers));
     let body_to_send = response_config
         .body
         .then(|| std::str::from_utf8(&bytes).map(|s| s.to_string()))
@@ -1215,8 +1213,7 @@ where
 
     let headers_to_send = request_config
         .headers
-        .then(|| externalize_header_map(&parts.headers))
-        .transpose()?;
+        .then(|| externalize_header_map(&parts.headers));
 
     let body_to_send = request_config
         .body
@@ -1377,8 +1374,7 @@ where
 
     let headers_to_send = response_config
         .headers
-        .then(|| externalize_header_map(&parts.headers))
-        .transpose()?;
+        .then(|| externalize_header_map(&parts.headers));
 
     let status_to_send = response_config.status_code.then(|| parts.status.as_u16());
 

apollo-router/src/plugins/coprocessor/supergraph.rs

@@ -219,8 +219,7 @@ where
 
     let headers_to_send = request_config
         .headers
-        .then(|| externalize_header_map(&parts.headers))
-        .transpose()?;
+        .then(|| externalize_header_map(&parts.headers));
 
     let body_to_send = request_config
         .body
@@ -375,8 +374,7 @@ where
     // Encode headers, body, status, context, sdl to create a payload
     let headers_to_send = response_config
         .headers
-        .then(|| externalize_header_map(&parts.headers))
-        .transpose()?;
+        .then(|| externalize_header_map(&parts.headers));
     let body_to_send = response_config
         .body
         .then(|| serde_json_bytes::to_value(&first).expect("serialization will not fail"));

apollo-router/src/plugins/coprocessor/test.rs

@@ -3462,7 +3462,7 @@ mod tests {
 
         external_form.append(ACCEPT, HeaderValue::from_static(TEXT_HTML.essence_str()));
 
-        let actual = externalize_header_map(&external_form).expect("externalized header map");
+        let actual = externalize_header_map(&external_form);
 
         assert_eq!(expected, actual);
     }

apollo-router/src/services/external.rs

@@ -335,14 +335,20 @@ where
 /// Convert a HeaderMap into a HashMap
 pub(crate) fn externalize_header_map(
     input: &HeaderMap<HeaderValue>,
-) -> Result<HashMap<String, Vec<String>>, BoxError> {
-    let mut output = HashMap::new();
+) -> HashMap<String, Vec<String>> {
+    let mut output = HashMap::with_capacity(input.keys_len());
     for (k, v) in input {
         let k = k.as_str().to_owned();
-        let v = String::from_utf8(v.as_bytes().to_vec()).map_err(|e| e.to_string())?;
-        output.entry(k).or_insert_with(Vec::new).push(v)
+        match String::from_utf8(v.as_bytes().to_vec()) {
+            Ok(v) => output.entry(k).or_insert_with(Vec::new).push(v),
+            Err(e) => tracing::warn!(
+                "unable to convert header value to utf-8 for {}, will not be sent to coprocessor: {}",
+                k,
+                e
+            ),
+        }
     }
-    Ok(output)
+    output
 }
 
 #[cfg(test)]
@@ -353,6 +359,7 @@ mod test {
 
     use super::*;
     use crate::assert_snapshot_subscriber;
+    use crate::test_harness::tracing_test;
 
     #[test]
     fn it_will_build_router_externalizable_correctly() {
@@ -445,4 +452,31 @@ mod test {
         .with_subscriber(assert_snapshot_subscriber!())
         .await;
     }
+
+    #[test]
+    fn it_will_externalize_headers_correctly() {
+        let _guard = tracing_test::dispatcher_guard();
+
+        let mut headers = HeaderMap::new();
+        headers.insert("content-type", HeaderValue::from_static("application/json"));
+        // Hyper uses this function internally to create HeaderValue structs
+        headers.insert("x-test-header", unsafe {
+            HeaderValue::from_maybe_shared_unchecked(b"invalid\xc0\xaf")
+        });
+
+        let externalized = externalize_header_map(&headers);
+
+        // x-test-header should be dropped because it is not valid UTF-8
+        assert_eq!(
+            externalized,
+            HashMap::from([(
+                "content-type".to_string(),
+                vec!["application/json".to_string()]
+            )])
+        );
+
+        assert!(tracing_test::logs_contain(
+            "unable to convert header value to utf-8 for x-test-header, will not be sent to coprocessor: invalid utf-8 sequence of 1 bytes from index 7"
+        ));
+    }
 }

docs/source/routing/customization/coprocessor.mdx

@@ -958,6 +958,8 @@
 
 Ensure headers are handled like HTTP headers in general. For example, normalize header case before your coprocessor operates on them.
 
+Any headers with non-UTF-8 values are omitted from coprocessor requests, and the router logs a warning when this occurs.
+
 If your coprocessor [returns a _different_ value](#responding-to-coprocessor-requests) for `headers`, the router replaces the existing headers with that value.
 
 > The router discards any `content-length` headers sent by coprocessors because incorrect `content-length` values can lead to HTTP request failures.

docs/source/routing/customization/coprocessor/reference.mdx

@@ -219,6 +219,8 @@
 
 Ensure headers are handled like HTTP headers in general. For example, normalize header case before your coprocessor operates on them.
 
+Any headers with non-UTF-8 values are omitted from coprocessor requests, and the router logs a warning when this occurs.
+
 If your coprocessor [returns a _different_ value](#responding-to-coprocessor-requests) for `headers`, the router replaces the existing headers with that value.
 
 > The router discards any `content-length` headers sent by coprocessors because incorrect `content-length` values can lead to HTTP request failures.