diff --git a/CHANGELOG.md b/CHANGELOG.md index f8860306dc..2a20681017 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ ### Ads-Client - Removed the `MozAdsContextIdProvider` callback interface and the `context_id_provider()` builder method. Wiring it up from Firefox Desktop crashed off the main thread ([Bug 2062806](https://bugzilla.mozilla.org/show_bug.cgi?id=2062806)) and nothing else used it, so the ads client now always uses its embedded `ContextIDComponent`. ([AC-99](https://mozilla-hub.atlassian.net/browse/AC-99)) +- Fixed a panic on the OHTTP request path when the MARS `/v1/ads-preflight` response carries a non-ASCII or CRLF geo location or user agent. The request now fails instead. No binding API change. ### Glean - Updated to v70.0.0 ([#7598](https://github.com/mozilla/application-services/pull/7598)) diff --git a/components/ads-client/src/mars.rs b/components/ads-client/src/mars.rs index a610a13ee1..3f88330bf7 100644 --- a/components/ads-client/src/mars.rs +++ b/components/ads-client/src/mars.rs @@ -87,7 +87,7 @@ where if ohttp { ad_request .headers - .extend(Headers::from(self.fetch_preflight()?)); + .extend(Headers::try_from(self.fetch_preflight()?)?); } let response = self.transport.send(ad_request, &cache_policy, ohttp)?; @@ -146,7 +146,7 @@ where if ohttp { request .headers - .extend(Headers::from(self.fetch_preflight()?)); + .extend(Headers::try_from(self.fetch_preflight()?)?); } self.transport.fire(request, ohttp).map_err(Into::into) } diff --git a/components/ads-client/src/mars/environment.rs b/components/ads-client/src/mars/environment.rs index 303ab9ffa9..c3c23f65bf 100644 --- a/components/ads-client/src/mars/environment.rs +++ b/components/ads-client/src/mars/environment.rs @@ -25,6 +25,8 @@ impl Environment { pub fn into_url(self, path: &str) -> Url { let mut url = self.base_url(); url.path_segments_mut() + // Cannot fail: every `base_url()` arm is an `https` URL, which `url` + // guarantees is hierarchical. .expect("base URL must be hierarchical") .pop_if_empty() .extend(path.split('/').filter(|segment| !segment.is_empty())); diff --git a/components/ads-client/src/mars/preflight.rs b/components/ads-client/src/mars/preflight.rs index 4bd399584f..3ec15f2ca4 100644 --- a/components/ads-client/src/mars/preflight.rs +++ b/components/ads-client/src/mars/preflight.rs @@ -31,17 +31,64 @@ pub struct PreflightResponse { pub normalized_ua: String, } -impl From for Headers { - fn from(preflight: PreflightResponse) -> Self { +impl TryFrom for Headers { + type Error = viaduct::ViaductError; + + fn try_from(preflight: PreflightResponse) -> Result { let mut headers = Headers::new(); - headers - .insert("X-Geo-Location", preflight.geo_location) - .expect("valid header"); + headers.insert("X-Geo-Location", preflight.geo_location)?; if !preflight.normalized_ua.is_empty() { - headers - .insert("X-User-Agent", preflight.normalized_ua) - .expect("valid header"); + headers.insert("X-User-Agent", preflight.normalized_ua)?; } - headers + Ok(headers) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn headers_carry_geo_location_and_normalized_ua() { + let headers = Headers::try_from(PreflightResponse { + geo_location: "US-CA".to_string(), + normalized_ua: "Firefox/140.0".to_string(), + }) + .unwrap(); + + assert_eq!(headers.get("X-Geo-Location"), Some("US-CA")); + assert_eq!(headers.get("X-User-Agent"), Some("Firefox/140.0")); + } + + #[test] + fn empty_normalized_ua_is_omitted() { + let headers = Headers::try_from(PreflightResponse { + geo_location: "US-CA".to_string(), + normalized_ua: String::new(), + }) + .unwrap(); + + assert_eq!(headers.get("X-Geo-Location"), Some("US-CA")); + assert_eq!(headers.get("X-User-Agent"), None); + } + + #[test] + fn non_ascii_geo_location_is_an_error_not_a_panic() { + let result = Headers::try_from(PreflightResponse { + geo_location: "Zürich".to_string(), + normalized_ua: String::new(), + }); + + assert!(result.is_err()); + } + + #[test] + fn header_injection_in_normalized_ua_is_an_error_not_a_panic() { + let result = Headers::try_from(PreflightResponse { + geo_location: "US-CA".to_string(), + normalized_ua: "Firefox/140.0\r\nX-Injected: yes".to_string(), + }); + + assert!(result.is_err()); } }