diff --git a/.changesets/fix_rohan_b99_allow_non_utf8_characters_in_headers.md b/.changesets/fix_rohan_b99_allow_non_utf8_characters_in_headers.md new file mode 100644 index 0000000000..4c280a1dec --- /dev/null +++ b/.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 \ No newline at end of file diff --git a/apollo-router/src/plugins/coprocessor/execution.rs b/apollo-router/src/plugins/coprocessor/execution.rs index 9abb7b1a59..2962e03456 100644 --- a/apollo-router/src/plugins/coprocessor/execution.rs +++ b/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")); diff --git a/apollo-router/src/plugins/coprocessor/mod.rs b/apollo-router/src/plugins/coprocessor/mod.rs index 09d70cedb5..87c17ea325 100644 --- a/apollo-router/src/plugins/coprocessor/mod.rs +++ b/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()); diff --git a/apollo-router/src/plugins/coprocessor/supergraph.rs b/apollo-router/src/plugins/coprocessor/supergraph.rs index 16c154a0ac..ce1639770f 100644 --- a/apollo-router/src/plugins/coprocessor/supergraph.rs +++ b/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")); diff --git a/apollo-router/src/plugins/coprocessor/test.rs b/apollo-router/src/plugins/coprocessor/test.rs index acba6ea954..0211b4a57d 100644 --- a/apollo-router/src/plugins/coprocessor/test.rs +++ b/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); } diff --git a/apollo-router/src/services/external.rs b/apollo-router/src/services/external.rs index 02e939e5ec..54f4c7482d 100644 --- a/apollo-router/src/services/external.rs +++ b/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, -) -> Result>, BoxError> { - let mut output = HashMap::new(); +) -> HashMap> { + 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" + )); + } } diff --git a/docs/source/routing/customization/coprocessor.mdx b/docs/source/routing/customization/coprocessor.mdx index e3b25b9edd..a31c391df3 100644 --- a/docs/source/routing/customization/coprocessor.mdx +++ b/docs/source/routing/customization/coprocessor.mdx @@ -958,6 +958,8 @@ An object mapping of all HTTP header names and values for the corresponding requ 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. diff --git a/docs/source/routing/customization/coprocessor/reference.mdx b/docs/source/routing/customization/coprocessor/reference.mdx index 4f9ecbf501..251e0d670c 100644 --- a/docs/source/routing/customization/coprocessor/reference.mdx +++ b/docs/source/routing/customization/coprocessor/reference.mdx @@ -219,6 +219,8 @@ An object mapping of all HTTP header names and values for the corresponding requ 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.