diff --git a/CHANGELOG.md b/CHANGELOG.md index ffce80ff4d3..35c218fffdc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ ### Ads-Client - Added `blocks: Vec` to `ffi::MozAdsRequestOptions`, `AdsClient::request*_ads`, `MARSClient::fetch_ads`, `mars::AdRequest`, and `mars::AdRequest::try_new`. This is serialized and passed to MARS so that it can remove blocks server-side. +- 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. # v156.0 (_2026-08-27_) diff --git a/components/ads-client/integration-tests/tests/mars.rs b/components/ads-client/integration-tests/tests/mars.rs index 6c7a9ced11a..d3cc1829fc6 100644 --- a/components/ads-client/integration-tests/tests/mars.rs +++ b/components/ads-client/integration-tests/tests/mars.rs @@ -216,8 +216,8 @@ fn test_contract_tile_ohttp_prod() { viaduct::ohttp::configure_ohttp_channel( "ads-client".to_string(), viaduct::ohttp::OhttpConfig { - relay_url: "https://mozilla-ohttp.fastly-edge.com/".to_string(), gateway_host: "prod.ohttp-gateway.prod.webservices.mozgcp.net".to_string(), + relay_url: "https://mozilla-ohttp.fastly-edge.com/".to_string(), }, ) .expect("OHTTP channel configuration should succeed"); diff --git a/components/ads-client/src/client.rs b/components/ads-client/src/client.rs index 739b5922040..589015cdda1 100644 --- a/components/ads-client/src/client.rs +++ b/components/ads-client/src/client.rs @@ -98,18 +98,6 @@ where self.client.clear_cache() } - // Shutdown the db connection and drop references to telemetry callbacks. - // Should be used only when dropping the ads client, this may be extended to drop more things. - pub fn shutdown_client(&mut self) -> Result<(), rusqlite::Error> { - // Drop telemetry (within the telemetry wrapper) - self.telemetry.shutdown(); - - // Shutdown DB - self.client.shutdown_db()?; - - Ok(()) - } - pub fn get_context_id(&self) -> context_id::ApiResult { self.context_id_provider.context_id() } @@ -249,6 +237,18 @@ where }) } + // Shutdown the db connection and drop references to telemetry callbacks. + // Should be used only when dropping the ads client, this may be extended to drop more things. + pub fn shutdown_client(&mut self) -> Result<(), rusqlite::Error> { + // Drop telemetry (within the telemetry wrapper) + self.telemetry.shutdown(); + + // Shutdown DB + self.client.shutdown_db()?; + + Ok(()) + } + fn request_ads( &self, placements: Vec, diff --git a/components/ads-client/src/ffi.rs b/components/ads-client/src/ffi.rs index 7f8726cd012..6b42a16df69 100644 --- a/components/ads-client/src/ffi.rs +++ b/components/ads-client/src/ffi.rs @@ -170,7 +170,7 @@ impl MozAdsClientBuilder { } } -#[derive(Clone, Copy, Debug, Default, uniffi::Enum, Eq, PartialEq)] +#[derive(Clone, Copy, Debug, Default, uniffi::Enum, PartialEq, Eq)] pub enum MozAdsEnvironment { #[default] Prod, diff --git a/components/ads-client/src/ffi/telemetry.rs b/components/ads-client/src/ffi/telemetry.rs index 02a6fee2e46..4b06c029d47 100644 --- a/components/ads-client/src/ffi/telemetry.rs +++ b/components/ads-client/src/ffi/telemetry.rs @@ -58,12 +58,6 @@ impl MozAdsTelemetryWrapper { } impl Telemetry for MozAdsTelemetryWrapper { - // MozAdsTelemetry has hanging uniffi callbacks which need to be explicitly dropped before closing. - // This replaces it with a `None` internally, meaning future calls will be noops. - fn shutdown(&self) { - let _dropped = self.inner.write().take(); - } - fn record(&self, event: &dyn Any) { let Some(inner) = self.inner.read().clone() else { return; @@ -146,6 +140,12 @@ impl Telemetry for MozAdsTelemetryWrapper { #[cfg(test)] panic!("Unsupported telemetry event type: {:?}", event.type_id()); } + + // MozAdsTelemetry has hanging uniffi callbacks which need to be explicitly dropped before closing. + // This replaces it with a `None` internally, meaning future calls will be noops. + fn shutdown(&self) { + let _dropped = self.inner.write().take(); + } } #[cfg(test)] diff --git a/components/ads-client/src/http_cache.rs b/components/ads-client/src/http_cache.rs index 81b056bb1df..898e83ea11d 100644 --- a/components/ads-client/src/http_cache.rs +++ b/components/ads-client/src/http_cache.rs @@ -60,10 +60,6 @@ impl HttpCache { Ok(()) } - pub fn shutdown_db(self) -> Result<(), rusqlite::Error> { - self.store.close() - } - pub fn invalidate_by_hash(&self, request_hash: &RequestHash) -> Result<(), rusqlite::Error> { self.store.invalidate_by_hash(request_hash)?; Ok(()) @@ -87,17 +83,17 @@ impl HttpCache { // Apply the cache policy and collect outcomes let (response, mut strategy_outcomes) = match policy { CachePolicy::CacheFirst { ttl } => CacheFirst { + default_ttl: self.default_ttl, + explicit_ttl: *ttl, hash, request, - explicit_ttl: *ttl, - default_ttl: self.default_ttl, } .apply(client, &self.store), CachePolicy::NetworkFirst { ttl } => NetworkFirst { + default_ttl: self.default_ttl, + explicit_ttl: *ttl, hash, request, - explicit_ttl: *ttl, - default_ttl: self.default_ttl, } .apply(client, &self.store), }?; @@ -115,6 +111,10 @@ impl HttpCache { Ok((response, outcomes)) } + + pub fn shutdown_db(self) -> Result<(), rusqlite::Error> { + self.store.close() + } } #[cfg(test)] @@ -192,11 +192,11 @@ mod tests { let hash = RequestHash::new(&("Get", "https://example.com/test")); let response = viaduct::Response { + body: b"test response".to_vec(), + headers: viaduct::Headers::new(), request_method: viaduct::Method::Get, - url: "https://example.com/test".parse().unwrap(), status: 200, - headers: viaduct::Headers::new(), - body: b"test response".to_vec(), + url: "https://example.com/test".parse().unwrap(), }; cache @@ -496,11 +496,11 @@ mod tests { let hash2 = RequestHash::new(&("Post", "https://example.com/api2")); let response = viaduct::Response { + body: b"test response".to_vec(), + headers: viaduct::Headers::new(), request_method: viaduct::Method::Post, - url: "https://example.com/test".parse().unwrap(), status: 200, - headers: viaduct::Headers::new(), - body: b"test response".to_vec(), + url: "https://example.com/test".parse().unwrap(), }; cache diff --git a/components/ads-client/src/http_cache/builder.rs b/components/ads-client/src/http_cache/builder.rs index 97f92f004b0..91f47ea0131 100644 --- a/components/ads-client/src/http_cache/builder.rs +++ b/components/ads-client/src/http_cache/builder.rs @@ -22,10 +22,10 @@ const MAX_TTL: Duration = Duration::from_secs(60 * 60 * 24 * 7); // 7 days #[derive(Debug, thiserror::Error)] pub enum HttpCacheBuilderError { - #[error("Database path cannot be empty")] - EmptyDbPath, #[error("Database error: {0}")] Database(#[from] open_database::Error), + #[error("Database path cannot be empty")] + EmptyDbPath, #[error( "Maximum cache size must be between {min_size} and {max_size}, got {size_bytes} bytes" )] @@ -44,67 +44,19 @@ pub enum HttpCacheBuilderError { pub struct HttpCacheBuilder { db_path: PathBuf, - max_size: Option, default_ttl: Option, + max_size: Option, } impl HttpCacheBuilder { pub fn new(db_path: impl Into) -> Self { Self { db_path: db_path.into(), - max_size: None, default_ttl: None, + max_size: None, } } - pub fn max_size(mut self, max_size: ByteSize) -> Self { - self.max_size = Some(max_size); - self - } - - pub fn default_ttl(mut self, ttl: Duration) -> Self { - self.default_ttl = Some(ttl); - self - } - - fn validate(&self) -> Result<(), HttpCacheBuilderError> { - if self.db_path.to_string_lossy().trim().is_empty() { - return Err(HttpCacheBuilderError::EmptyDbPath); - } - - if let Some(max_size) = self.max_size { - if max_size < MIN_CACHE_SIZE || max_size > MAX_CACHE_SIZE { - return Err(HttpCacheBuilderError::InvalidMaxSize { - size_bytes: max_size.as_u64(), - min_size: MIN_CACHE_SIZE.to_string(), - max_size: MAX_CACHE_SIZE.to_string(), - }); - } - } - - if let Some(ttl) = self.default_ttl { - if !(MIN_TTL..=MAX_TTL).contains(&ttl) { - return Err(HttpCacheBuilderError::InvalidTtl { - ttl: ttl.as_secs(), - min_ttl: format!("{} seconds", MIN_TTL.as_secs()), - max_ttl: format!("{} seconds", MAX_TTL.as_secs()), - }); - } - } - - Ok(()) - } - - fn open_connection(&self) -> Result { - let initializer = HttpCacheConnectionInitializer {}; - let conn = if cfg!(test) { - open_database::open_memory_database(&initializer)? - } else { - open_database::open_database(&self.db_path, &initializer)? - }; - Ok(conn) - } - pub fn build(&self) -> Result { self.validate()?; @@ -135,6 +87,54 @@ impl HttpCacheBuilder { store, }) } + + pub fn default_ttl(mut self, ttl: Duration) -> Self { + self.default_ttl = Some(ttl); + self + } + + pub fn max_size(mut self, max_size: ByteSize) -> Self { + self.max_size = Some(max_size); + self + } + + fn open_connection(&self) -> Result { + let initializer = HttpCacheConnectionInitializer {}; + let conn = if cfg!(test) { + open_database::open_memory_database(&initializer)? + } else { + open_database::open_database(&self.db_path, &initializer)? + }; + Ok(conn) + } + + fn validate(&self) -> Result<(), HttpCacheBuilderError> { + if self.db_path.to_string_lossy().trim().is_empty() { + return Err(HttpCacheBuilderError::EmptyDbPath); + } + + if let Some(max_size) = self.max_size { + if max_size < MIN_CACHE_SIZE || max_size > MAX_CACHE_SIZE { + return Err(HttpCacheBuilderError::InvalidMaxSize { + max_size: MAX_CACHE_SIZE.to_string(), + min_size: MIN_CACHE_SIZE.to_string(), + size_bytes: max_size.as_u64(), + }); + } + } + + if let Some(ttl) = self.default_ttl { + if !(MIN_TTL..=MAX_TTL).contains(&ttl) { + return Err(HttpCacheBuilderError::InvalidTtl { + max_ttl: format!("{} seconds", MAX_TTL.as_secs()), + min_ttl: format!("{} seconds", MIN_TTL.as_secs()), + ttl: ttl.as_secs(), + }); + } + } + + Ok(()) + } } #[cfg(test)] diff --git a/components/ads-client/src/http_cache/bytesize.rs b/components/ads-client/src/http_cache/bytesize.rs index 8c29f8f381c..c1db4e8c6af 100644 --- a/components/ads-client/src/http_cache/bytesize.rs +++ b/components/ads-client/src/http_cache/bytesize.rs @@ -5,7 +5,7 @@ use std::fmt; use std::ops; -#[derive(Clone, Copy, Debug, Eq, Ord, PartialEq, PartialOrd)] +#[derive(Clone, Copy, Debug, PartialEq, Eq, PartialOrd, Ord)] pub struct ByteSize(u64); impl ByteSize { diff --git a/components/ads-client/src/http_cache/cache_control.rs b/components/ads-client/src/http_cache/cache_control.rs index a469ffa312f..a1a0323f14f 100644 --- a/components/ads-client/src/http_cache/cache_control.rs +++ b/components/ads-client/src/http_cache/cache_control.rs @@ -47,13 +47,13 @@ impl From<&Response> for CacheControl { } impl CacheControl { - pub fn should_cache(&self) -> bool { - !self.no_store - } - pub fn max_age_duration(&self) -> Option { self.max_age.map(Duration::from_secs) } + + pub fn should_cache(&self) -> bool { + !self.no_store + } } #[cfg(test)] diff --git a/components/ads-client/src/http_cache/clock.rs b/components/ads-client/src/http_cache/clock.rs index 778a5ce7c96..7505509863f 100644 --- a/components/ads-client/src/http_cache/clock.rs +++ b/components/ads-client/src/http_cache/clock.rs @@ -3,17 +3,14 @@ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ pub trait Clock: Send + Sync + 'static { - fn now_epoch_seconds(&self) -> i64; #[cfg(test)] fn advance(&self, secs: i64); + fn now_epoch_seconds(&self) -> i64; } pub struct CacheClock; impl Clock for CacheClock { - fn now_epoch_seconds(&self) -> i64 { - chrono::Utc::now().timestamp() - } #[cfg(test)] fn advance(&self, _secs: i64) { panic!( @@ -23,6 +20,9 @@ impl Clock for CacheClock { " ) } + fn now_epoch_seconds(&self) -> i64 { + chrono::Utc::now().timestamp() + } } #[cfg(test)] @@ -41,11 +41,11 @@ impl TestClock { #[cfg(test)] impl Clock for TestClock { - fn now_epoch_seconds(&self) -> i64 { - self.now.load(std::sync::atomic::Ordering::Relaxed) - } fn advance(&self, secs: i64) { self.now .fetch_add(secs, std::sync::atomic::Ordering::Relaxed); } + fn now_epoch_seconds(&self) -> i64 { + self.now.load(std::sync::atomic::Ordering::Relaxed) + } } diff --git a/components/ads-client/src/http_cache/connection_initializer.rs b/components/ads-client/src/http_cache/connection_initializer.rs index b5094f771e1..a0620d0b9cc 100644 --- a/components/ads-client/src/http_cache/connection_initializer.rs +++ b/components/ads-client/src/http_cache/connection_initializer.rs @@ -9,14 +9,8 @@ use std::time::Duration; pub struct HttpCacheConnectionInitializer {} impl open_database::ConnectionInitializer for HttpCacheConnectionInitializer { - const NAME: &'static str = "http_cache"; const END_VERSION: u32 = 2; - - fn prepare(&self, conn: &Connection, _db_empty: bool) -> open_database::Result<()> { - conn.execute_batch("PRAGMA journal_mode=wal;")?; - conn.busy_timeout(Duration::from_secs(5))?; - Ok(()) - } + const NAME: &'static str = "http_cache"; fn init(&self, tx: &rusqlite::Transaction<'_>) -> open_database::Result<()> { const SCHEMA: &str = " @@ -45,6 +39,12 @@ impl open_database::ConnectionInitializer for HttpCacheConnectionInitializer { Ok(()) } + fn prepare(&self, conn: &Connection, _db_empty: bool) -> open_database::Result<()> { + conn.execute_batch("PRAGMA journal_mode=wal;")?; + conn.busy_timeout(Duration::from_secs(5))?; + Ok(()) + } + fn upgrade_from( &self, conn: &rusqlite::Transaction<'_>, diff --git a/components/ads-client/src/http_cache/store.rs b/components/ads-client/src/http_cache/store.rs index a80f5a956ca..21b5855fbb5 100644 --- a/components/ads-client/src/http_cache/store.rs +++ b/components/ads-client/src/http_cache/store.rs @@ -14,18 +14,18 @@ use rusqlite::{params, Connection, OptionalExtension, Result as SqliteResult}; use viaduct::{Header, Response}; #[cfg(test)] -#[derive(Clone, Copy, Debug, Eq, PartialEq)] +#[derive(Clone, Copy, Debug, PartialEq, Eq)] pub enum FaultKind { - None, + Cleanup, Lookup, + None, Store, Trim, - Cleanup, } pub struct HttpCacheStore { - conn: Mutex, clock: Arc, + conn: Mutex, #[cfg(test)] fault: parking_lot::Mutex, } @@ -33,41 +33,36 @@ pub struct HttpCacheStore { impl HttpCacheStore { pub fn new(conn: Connection) -> Self { Self { - conn: Mutex::new(conn), clock: Arc::new(CacheClock), + conn: Mutex::new(conn), #[cfg(test)] fault: parking_lot::Mutex::new(FaultKind::None), } } - pub fn close(self) -> Result<(), rusqlite::Error> { - let conn = self.conn.into_inner(); - conn.close().map_err(|(_, err)| err) - } - #[cfg(test)] pub fn new_with_test_clock(conn: Connection) -> Self { use crate::http_cache::clock::TestClock; Self { - conn: Mutex::new(conn), clock: Arc::new(TestClock::new(chrono::Utc::now().timestamp())), + conn: Mutex::new(conn), #[cfg(test)] fault: parking_lot::Mutex::new(FaultKind::None), } } - #[cfg(test)] - pub fn get_clock(&self) -> &dyn Clock { - &*self.clock - } - /// Removes all entries from cache. pub fn clear_all(&self) -> SqliteResult { let conn = self.conn.lock(); conn.execute("DELETE FROM http_cache", []) } + pub fn close(self) -> Result<(), rusqlite::Error> { + let conn = self.conn.into_inner(); + conn.close().map_err(|(_, err)| err) + } + /// Returns total size of the cache in bytes. pub fn current_total_size_bytes(&self) -> SqliteResult { let conn = self.conn.lock(); @@ -92,6 +87,19 @@ impl HttpCacheStore { ) } + #[cfg(test)] + pub fn get_clock(&self) -> &dyn Clock { + &*self.clock + } + + pub fn invalidate_by_hash(&self, request_hash: &RequestHash) -> SqliteResult { + let conn = self.conn.lock(); + conn.execute( + "DELETE FROM http_cache WHERE request_hash = ?1", + params![request_hash.to_string()], + ) + } + /// Lookup is agnostic to expiration. If it exists in the store, it will return the result. pub fn lookup(&self, request_hash: &RequestHash) -> SqliteResult> { #[cfg(test)] @@ -149,6 +157,11 @@ impl HttpCacheStore { .optional() } + #[cfg(test)] + pub fn set_fault(&self, kind: FaultKind) { + *self.fault.lock() = kind; + } + /// Upsert an object into the store with an expires_at defined by the given ttl_seconds. /// Calling this method will always store an object regardless of headers or policy. /// Logic to determine the correct ttl or cache/no-cache should happen before calling this. @@ -210,14 +223,6 @@ impl HttpCacheStore { Ok(()) } - pub fn invalidate_by_hash(&self, request_hash: &RequestHash) -> SqliteResult { - let conn = self.conn.lock(); - conn.execute( - "DELETE FROM http_cache WHERE request_hash = ?1", - params![request_hash.to_string()], - ) - } - /// Trim cache to pub fn trim_to_max_size(&self, max_size: &ByteSize) -> SqliteResult<()> { #[cfg(test)] @@ -240,11 +245,6 @@ impl HttpCacheStore { Ok(()) } - #[cfg(test)] - pub fn set_fault(&self, kind: FaultKind) { - *self.fault.lock() = kind; - } - #[cfg(test)] fn forced_fault_error(msg: &str) -> rusqlite::Error { rusqlite::Error::SqliteFailure( @@ -290,10 +290,10 @@ mod tests { fn create_test_request(url: &str, body: &[u8]) -> Request { Request { + body: Some(body.to_vec()), + headers: Headers::new(), method: Method::Get, url: url.parse().unwrap(), - headers: Headers::new(), - body: Some(body.to_vec()), } } @@ -304,11 +304,11 @@ mod tests { .unwrap(); Response { + body: body.to_vec(), + headers, request_method: Method::Get, - url: "https://example.com/test".parse().unwrap(), status, - headers, - body: body.to_vec(), + url: "https://example.com/test".parse().unwrap(), } } diff --git a/components/ads-client/src/http_cache/strategy.rs b/components/ads-client/src/http_cache/strategy.rs index acf23c82349..d01d38230cd 100644 --- a/components/ads-client/src/http_cache/strategy.rs +++ b/components/ads-client/src/http_cache/strategy.rs @@ -12,10 +12,10 @@ use std::time::Duration; use viaduct::{Client, Request}; pub struct CacheFirst { + pub default_ttl: Duration, + pub explicit_ttl: Option, pub hash: RequestHash, pub request: Request, - pub explicit_ttl: Option, - pub default_ttl: Duration, } impl CacheFirst { @@ -28,10 +28,10 @@ impl CacheFirst { } let network = NetworkFirst { + default_ttl: self.default_ttl, + explicit_ttl: self.explicit_ttl, hash: self.hash, request: self.request, - explicit_ttl: self.explicit_ttl, - default_ttl: self.default_ttl, }; let (response, mut network_outcomes) = network.apply(client, store)?; outcomes.append(&mut network_outcomes); @@ -40,10 +40,10 @@ impl CacheFirst { } pub struct NetworkFirst { + pub default_ttl: Duration, + pub explicit_ttl: Option, pub hash: RequestHash, pub request: Request, - pub explicit_ttl: Option, - pub default_ttl: Duration, } impl NetworkFirst { @@ -52,9 +52,9 @@ impl NetworkFirst { let cache_control = CacheControl::from(&response); let outcome = if cache_control.should_cache() { let ttl = EffectiveTtl { + default: self.default_ttl, explicit: self.explicit_ttl, server_max_age: cache_control.max_age_duration(), - default: self.default_ttl, } .resolve(); if ttl.is_zero() { diff --git a/components/ads-client/src/http_cache/ttl.rs b/components/ads-client/src/http_cache/ttl.rs index 35f57d11344..9beadd147e4 100644 --- a/components/ads-client/src/http_cache/ttl.rs +++ b/components/ads-client/src/http_cache/ttl.rs @@ -16,12 +16,12 @@ pub const MAX_TTL: Duration = Duration::from_secs(7 * 24 * 60 * 60); /// `explicit` comes from the caller, `server_max_age` from the response's /// `Cache-Control` header, and `default` from the cache's configuration. pub struct EffectiveTtl { + /// The cache's configured default TTL. + pub default: Duration, /// Per-request override provided by the caller, if any. pub explicit: Option, /// `Cache-Control: max-age` from the server response, if present. pub server_max_age: Option, - /// The cache's configured default TTL. - pub default: Duration, } impl EffectiveTtl { @@ -47,9 +47,9 @@ mod tests { #[test] fn explicit_overrides_server_max_age_and_default() { let ttl = EffectiveTtl { + default: Duration::from_secs(300), explicit: Some(Duration::from_secs(60)), server_max_age: Some(Duration::from_secs(3600)), - default: Duration::from_secs(300), } .resolve(); assert_eq!(ttl, Duration::from_secs(60)); @@ -58,9 +58,9 @@ mod tests { #[test] fn falls_back_to_server_max_age_when_no_explicit() { let ttl = EffectiveTtl { + default: Duration::from_secs(300), explicit: None, server_max_age: Some(Duration::from_secs(3600)), - default: Duration::from_secs(300), } .resolve(); assert_eq!(ttl, Duration::from_secs(3600)); @@ -69,9 +69,9 @@ mod tests { #[test] fn falls_back_to_default_when_no_explicit_and_no_server_max_age() { let ttl = EffectiveTtl { + default: Duration::from_secs(300), explicit: None, server_max_age: None, - default: Duration::from_secs(300), } .resolve(); assert_eq!(ttl, Duration::from_secs(300)); @@ -81,9 +81,9 @@ mod tests { fn zero_server_max_age_yields_zero() { // Lets the strategy emit NoCache without a network round-trip. let ttl = EffectiveTtl { + default: Duration::from_secs(300), explicit: None, server_max_age: Some(Duration::ZERO), - default: Duration::from_secs(300), } .resolve(); assert_eq!(ttl, Duration::ZERO); @@ -92,9 +92,9 @@ mod tests { #[test] fn server_max_age_is_capped_at_max_ttl() { let ttl = EffectiveTtl { + default: Duration::from_secs(300), explicit: None, server_max_age: Some(Duration::from_secs(365 * 24 * 60 * 60)), - default: Duration::from_secs(300), } .resolve(); assert_eq!(ttl, MAX_TTL); @@ -103,9 +103,9 @@ mod tests { #[test] fn explicit_ttl_is_capped_at_max_ttl() { let ttl = EffectiveTtl { + default: Duration::from_secs(300), explicit: Some(Duration::from_secs(30 * 24 * 60 * 60)), server_max_age: None, - default: Duration::from_secs(300), } .resolve(); assert_eq!(ttl, MAX_TTL); @@ -114,9 +114,9 @@ mod tests { #[test] fn default_ttl_is_capped_at_max_ttl() { let ttl = EffectiveTtl { + default: Duration::from_secs(30 * 24 * 60 * 60), explicit: None, server_max_age: None, - default: Duration::from_secs(30 * 24 * 60 * 60), } .resolve(); assert_eq!(ttl, MAX_TTL); diff --git a/components/ads-client/src/lib.rs b/components/ads-client/src/lib.rs index c59fc6bd856..afeaef93ade 100644 --- a/components/ads-client/src/lib.rs +++ b/components/ads-client/src/lib.rs @@ -52,18 +52,6 @@ impl MozAdsClient { }) } - // Allows the ads-client to unload some references and prepare for a safe shutdown. - // Other methods should not be called after this one. - #[uniffi::method()] - pub fn shutdown(&self) -> AdsClientApiResult<()> { - let mut inner = self.inner.lock(); - if let Err(err) = inner.shutdown_client() { - // Log the error, but continue with shutdown. - error!("Failed to shutdown the ads client: {:?}", err); - } - Ok(()) - } - #[handle_error(ComponentError)] #[uniffi::method(default(options = None))] pub fn record_click( @@ -176,4 +164,16 @@ impl MozAdsClient { .map_err(ComponentError::RequestAds)?; Ok(response.into_iter().map(|(k, v)| (k, v.into())).collect()) } + + // Allows the ads-client to unload some references and prepare for a safe shutdown. + // Other methods should not be called after this one. + #[uniffi::method()] + pub fn shutdown(&self) -> AdsClientApiResult<()> { + let mut inner = self.inner.lock(); + if let Err(err) = inner.shutdown_client() { + // Log the error, but continue with shutdown. + error!("Failed to shutdown the ads client: {:?}", err); + } + Ok(()) + } } diff --git a/components/ads-client/src/mars.rs b/components/ads-client/src/mars.rs index fa178572866..748120d78c1 100644 --- a/components/ads-client/src/mars.rs +++ b/components/ads-client/src/mars.rs @@ -57,10 +57,6 @@ where self.transport.clear_cache() } - pub fn shutdown_db(&mut self) -> Result<(), rusqlite::Error> { - self.transport.shutdown_db() - } - pub fn fetch_ads( &self, context_id: String, @@ -86,7 +82,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)?; @@ -94,6 +90,11 @@ where Ok((ads, request_hash)) } + #[cfg(test)] + pub fn get_telemetry(&self) -> T { + self.telemetry.clone() + } + // TODO: Remove this allow(dead_code) when cache invalidation is re-enabled behind Nimbus experiment #[allow(dead_code)] pub fn invalidate_cache_by_hash( @@ -127,6 +128,10 @@ where Ok(self.make_callback_request(callback, ohttp)?) } + pub fn shutdown_db(&mut self) -> Result<(), rusqlite::Error> { + self.transport.shutdown_db() + } + fn fetch_preflight(&self) -> Result { let response = self.transport.send( PreflightRequest(self.environment.into_url("ads-preflight")), @@ -145,15 +150,10 @@ 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) } - - #[cfg(test)] - pub fn get_telemetry(&self) -> T { - self.telemetry.clone() - } } #[cfg(test)] diff --git a/components/ads-client/src/mars/ad_response.rs b/components/ads-client/src/mars/ad_response.rs index 5e057b7606b..dbb83658be3 100644 --- a/components/ads-client/src/mars/ad_response.rs +++ b/components/ads-client/src/mars/ad_response.rs @@ -149,9 +149,9 @@ pub struct SpocFrequencyCaps { #[derive(Clone, Debug, Deserialize, PartialEq, Serialize)] pub struct SpocRanking { - pub priority: u32, - pub personalization_models: Option>, pub item_score: f64, + pub personalization_models: Option>, + pub priority: u32, } #[derive(Clone, Debug, Deserialize, PartialEq, Serialize)] diff --git a/components/ads-client/src/mars/environment.rs b/components/ads-client/src/mars/environment.rs index 65edf6a52ad..a0a4bcb7ec3 100644 --- a/components/ads-client/src/mars/environment.rs +++ b/components/ads-client/src/mars/environment.rs @@ -11,7 +11,7 @@ static MARS_API_ENDPOINT_PROD: Lazy = Lazy::new(|| url!("https://ads.mozill static MARS_API_ENDPOINT_STAGING: Lazy = Lazy::new(|| url!("https://ads.allizom.org/v1/")); -#[derive(Clone, Copy, Debug, Default, Eq, Hash, PartialEq)] +#[derive(Clone, Copy, Debug, Default, Hash, PartialEq, Eq)] pub enum Environment { #[default] Prod, @@ -24,6 +24,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/error.rs b/components/ads-client/src/mars/error.rs index 1aa81cde4a1..73d131fec6a 100644 --- a/components/ads-client/src/mars/error.rs +++ b/components/ads-client/src/mars/error.rs @@ -19,12 +19,12 @@ pub enum CallbackRequestError { #[error("Could not fetch ads, MARS responded with: {0}")] HTTPError(#[from] HTTPError), - #[error("JSON error: {0}")] - Json(#[from] serde_json::Error), - #[error("Invalid callback URL: {0}")] InvalidUrl(#[from] url::ParseError), + #[error("JSON error: {0}")] + Json(#[from] serde_json::Error), + #[error("Error sending request: {0}")] Request(#[from] viaduct::ViaductError), } @@ -145,11 +145,11 @@ mod tests { fn mock_response(status: u16, body: &str) -> Response { Response { + body: body.as_bytes().to_vec(), + headers: viaduct::Headers::new(), request_method: viaduct::Method::Get, - url: Url::parse("https://example.com").unwrap(), status, - headers: viaduct::Headers::new(), - body: body.as_bytes().to_vec(), + url: Url::parse("https://example.com").unwrap(), } } diff --git a/components/ads-client/src/mars/preflight.rs b/components/ads-client/src/mars/preflight.rs index 4bd399584fe..f140ca0d3e1 100644 --- a/components/ads-client/src/mars/preflight.rs +++ b/components/ads-client/src/mars/preflight.rs @@ -31,17 +31,68 @@ pub struct PreflightResponse { pub normalized_ua: String, } -impl From for Headers { - fn from(preflight: PreflightResponse) -> Self { +impl TryFrom for Headers { + type Error = viaduct::ViaductError; + + /// Fallible: `geo_location` and `normalized_ua` are echoed straight out of + /// the MARS response body, and `Headers::insert` rejects a value that is + /// not printable ASCII. A malformed response must surface as an error, not + /// as a panic in the caller's process. + 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()); } } diff --git a/components/ads-client/src/mars/transport.rs b/components/ads-client/src/mars/transport.rs index 7da0621a4a4..c231a523596 100644 --- a/components/ads-client/src/mars/transport.rs +++ b/components/ads-client/src/mars/transport.rs @@ -29,13 +29,6 @@ impl MARSTransport { } } - pub fn shutdown_db(&mut self) -> Result<(), rusqlite::Error> { - if let Some(cache) = self.http_cache.take() { - cache.shutdown_db()?; - } - Ok(()) - } - pub fn clear_cache(&self) -> Result<(), rusqlite::Error> { if let Some(cache) = &self.http_cache { cache.clear()?; @@ -82,6 +75,13 @@ impl MARSTransport { } } + pub fn shutdown_db(&mut self) -> Result<(), rusqlite::Error> { + if let Some(cache) = self.http_cache.take() { + cache.shutdown_db()?; + } + Ok(()) + } + fn client_for(ohttp: bool) -> Result { if ohttp { Client::with_ohttp_channel(OHTTP_CHANNEL_ID, ClientSettings::default())