diff --git a/crates/trusted-server-core/src/cookies.rs b/crates/trusted-server-core/src/cookies.rs index 8757cae5..83b23cd7 100644 --- a/crates/trusted-server-core/src/cookies.rs +++ b/crates/trusted-server-core/src/cookies.rs @@ -3,6 +3,8 @@ //! This module provides functionality for parsing and creating cookies //! used in the trusted server system. +use std::borrow::Cow; + use cookie::{Cookie, CookieJar}; use error_stack::{Report, ResultExt}; use fastly::http::header; @@ -28,6 +30,42 @@ pub const CONSENT_COOKIE_NAMES: &[&str] = &[ const COOKIE_MAX_AGE: i32 = 365 * 24 * 60 * 60; // 1 year +fn is_allowed_synthetic_id_char(c: char) -> bool { + c.is_ascii_alphanumeric() || matches!(c, '.' | '-' | '_') +} + +#[must_use] +pub(crate) fn synthetic_id_has_only_allowed_chars(synthetic_id: &str) -> bool { + synthetic_id.chars().all(is_allowed_synthetic_id_char) +} + +fn sanitize_synthetic_id_for_cookie(synthetic_id: &str) -> Cow<'_, str> { + if synthetic_id_has_only_allowed_chars(synthetic_id) { + return Cow::Borrowed(synthetic_id); + } + + let safe_id = synthetic_id + .chars() + .filter(|c| is_allowed_synthetic_id_char(*c)) + .collect::(); + + log::warn!( + "Stripped disallowed characters from synthetic_id before setting cookie (len {} -> {}); \ + callers should reject invalid request IDs before cookie creation", + synthetic_id.len(), + safe_id.len(), + ); + + Cow::Owned(safe_id) +} + +fn synthetic_cookie_attributes(settings: &Settings, max_age: i32) -> String { + format!( + "Domain={}; Path=/; Secure; HttpOnly; SameSite=Lax; Max-Age={max_age}", + settings.publisher.cookie_domain, + ) +} + /// Parses a cookie string into a [`CookieJar`]. /// /// Returns an empty jar if the cookie string is unparseable. @@ -128,13 +166,46 @@ pub fn forward_cookie_header(from: &Request, to: &mut Request, strip_consent: bo /// Creates a synthetic ID cookie string. /// -/// Generates a properly formatted cookie with security attributes -/// for storing the synthetic ID. +/// Generates a `Set-Cookie` header value with the following security attributes: +/// - `Secure`: transmitted over HTTPS only. +/// - `HttpOnly`: inaccessible to JavaScript (`document.cookie`), blocking XSS exfiltration. +/// Safe to set because integrations receive the synthetic ID via the `x-synthetic-id` +/// response header instead of reading it from the cookie directly. +/// - `SameSite=Lax`: sent on same-site requests and top-level cross-site navigations. +/// `Strict` is intentionally avoided — it would suppress the cookie on the first +/// request when a user arrives from an external page, breaking first-visit attribution. +/// - `Max-Age`: 1 year retention. +/// +/// The `synthetic_id` is sanitized via an allowlist before embedding in the cookie value. +/// Only ASCII alphanumeric characters and `.`, `-`, `_` are permitted — matching the +/// known synthetic ID format (`{64-char-hex}.{6-char-alphanumeric}`). Request-sourced IDs +/// with disallowed characters are rejected earlier in [`crate::synthetic::get_synthetic_id`]; +/// this sanitization remains as a defense-in-depth backstop for unexpected callers. +/// +/// The `cookie_domain` is validated at config load time via [`validator::Validate`] on +/// [`crate::settings::Publisher`]; bad config fails at startup, not per-request. +/// +/// # Examples +/// +/// ```no_run +/// # use trusted_server_core::cookies::create_synthetic_cookie; +/// # use trusted_server_core::settings::Settings; +/// // `settings` is loaded at startup via `Settings::from_toml_and_env`. +/// # fn example(settings: &Settings) { +/// let cookie = create_synthetic_cookie(settings, "abc123.xk92ab"); +/// assert!(cookie.contains("HttpOnly")); +/// assert!(cookie.contains("Secure")); +/// # } +/// ``` #[must_use] pub fn create_synthetic_cookie(settings: &Settings, synthetic_id: &str) -> String { + let safe_id = sanitize_synthetic_id_for_cookie(synthetic_id); + format!( - "{}={}; Domain={}; Path=/; Secure; SameSite=Lax; Max-Age={}", - COOKIE_SYNTHETIC_ID, synthetic_id, settings.publisher.cookie_domain, COOKIE_MAX_AGE, + "{}={}; {}", + COOKIE_SYNTHETIC_ID, + safe_id, + synthetic_cookie_attributes(settings, COOKIE_MAX_AGE), ) } @@ -159,8 +230,9 @@ pub fn set_synthetic_cookie( /// on receipt of this header. pub fn expire_synthetic_cookie(settings: &Settings, response: &mut fastly::Response) { let cookie = format!( - "{}=; Domain={}; Path=/; Secure; SameSite=Lax; Max-Age=0", - COOKIE_SYNTHETIC_ID, settings.publisher.cookie_domain, + "{}=; {}", + COOKIE_SYNTHETIC_ID, + synthetic_cookie_attributes(settings, 0), ); response.append_header(header::SET_COOKIE, cookie); } @@ -253,12 +325,43 @@ mod tests { assert_eq!( result, format!( - "{}=12345; Domain={}; Path=/; Secure; SameSite=Lax; Max-Age={}", + "{}=12345; Domain={}; Path=/; Secure; HttpOnly; SameSite=Lax; Max-Age={}", COOKIE_SYNTHETIC_ID, settings.publisher.cookie_domain, COOKIE_MAX_AGE, ) ); } + #[test] + fn test_create_synthetic_cookie_sanitizes_disallowed_chars_in_id() { + let settings = create_test_settings(); + // Allowlist permits only ASCII alphanumeric, '.', '-', '_'. + // ';', '=', '\r', '\n', spaces, NUL bytes, and other control chars are all stripped. + let result = create_synthetic_cookie(&settings, "evil;injected\r\nfoo=bar\0baz"); + // Extract the value portion anchored to the cookie name constant to + // avoid false positives from disallowed chars in cookie attributes. + let value = result + .strip_prefix(&format!("{}=", COOKIE_SYNTHETIC_ID)) + .and_then(|s| s.split_once(';').map(|(v, _)| v)) + .expect("should have cookie value portion"); + assert_eq!( + value, "evilinjectedfoobarbaz", + "should strip disallowed characters and preserve safe chars" + ); + } + + #[test] + fn test_create_synthetic_cookie_preserves_well_formed_id() { + let settings = create_test_settings(); + // A well-formed ID should pass through the allowlist unmodified. + let id = "abc123def0123456789abcdef0123456789abcdef0123456789abcdef01234567.xk92ab"; + let result = create_synthetic_cookie(&settings, id); + let value = result + .strip_prefix(&format!("{}=", COOKIE_SYNTHETIC_ID)) + .and_then(|s| s.split_once(';').map(|(v, _)| v)) + .expect("should have cookie value portion"); + assert_eq!(value, id, "should not modify a well-formed synthetic ID"); + } + #[test] fn test_set_synthetic_cookie() { let settings = create_test_settings(); @@ -279,6 +382,30 @@ mod tests { ); } + #[test] + fn test_expire_synthetic_cookie_matches_security_attributes() { + let settings = create_test_settings(); + let mut response = fastly::Response::new(); + + expire_synthetic_cookie(&settings, &mut response); + + let cookie_header = response + .get_header(header::SET_COOKIE) + .expect("Set-Cookie header should be present"); + let cookie_str = cookie_header + .to_str() + .expect("header should be valid UTF-8"); + + assert_eq!( + cookie_str, + format!( + "{}=; Domain={}; Path=/; Secure; HttpOnly; SameSite=Lax; Max-Age=0", + COOKIE_SYNTHETIC_ID, settings.publisher.cookie_domain, + ), + "expiry cookie should retain the same security attributes as the live cookie" + ); + } + // --------------------------------------------------------------- // strip_cookies tests // --------------------------------------------------------------- diff --git a/crates/trusted-server-core/src/integrations/registry.rs b/crates/trusted-server-core/src/integrations/registry.rs index 158a1e29..b0ddff0f 100644 --- a/crates/trusted-server-core/src/integrations/registry.rs +++ b/crates/trusted-server-core/src/integrations/registry.rs @@ -1295,6 +1295,57 @@ mod tests { ); } + #[test] + fn handle_proxy_replaces_invalid_request_header_with_matching_response_cookie() { + let settings = create_test_settings(); + let routes = vec![( + Method::GET, + "/integrations/test/synthetic", + ( + Arc::new(SyntheticIdTestProxy) as Arc, + "synthetic_id_test", + ), + )]; + let registry = IntegrationRegistry::from_routes(routes); + + let mut req = Request::get("https://test-publisher.com/integrations/test/synthetic"); + req.set_header(HEADER_X_SYNTHETIC_ID, "evil;injected"); + + let result = futures::executor::block_on(registry.handle_proxy( + &Method::GET, + "/integrations/test/synthetic", + &settings, + req, + )) + .expect("should handle proxy request"); + + let response = result.expect("handler should succeed"); + let response_header = response + .get_header(HEADER_X_SYNTHETIC_ID) + .expect("response should have x-synthetic-id header") + .to_str() + .expect("header should be valid UTF-8") + .to_string(); + let cookie_header = response + .get_header(header::SET_COOKIE) + .expect("response should have Set-Cookie header") + .to_str() + .expect("header should be valid UTF-8"); + let cookie_value = cookie_header + .strip_prefix(&format!("{}=", COOKIE_SYNTHETIC_ID)) + .and_then(|s| s.split_once(';').map(|(value, _)| value)) + .expect("should contain the synthetic_id cookie value"); + + assert_ne!( + response_header, "evil;injected", + "should not reflect the tampered request header" + ); + assert_eq!( + response_header, cookie_value, + "response header and cookie should carry the same effective synthetic ID" + ); + } + #[test] fn handle_proxy_always_sets_cookie() { let settings = create_test_settings(); diff --git a/crates/trusted-server-core/src/settings.rs b/crates/trusted-server-core/src/settings.rs index 0b1dda53..512f9a39 100644 --- a/crates/trusted-server-core/src/settings.rs +++ b/crates/trusted-server-core/src/settings.rs @@ -20,6 +20,7 @@ pub const ENVIRONMENT_VARIABLE_SEPARATOR: &str = "__"; #[derive(Debug, Default, Clone, Deserialize, Serialize, Validate)] pub struct Publisher { pub domain: String, + #[validate(custom(function = validate_cookie_domain))] pub cookie_domain: String, #[validate(custom(function = validate_no_trailing_slash))] pub origin_url: String, @@ -512,6 +513,18 @@ impl Settings { } } +fn validate_cookie_domain(value: &str) -> Result<(), ValidationError> { + // `=` is excluded: it only has special meaning in the name=value pair, + // not within the Domain attribute value. + if value.contains([';', '\n', '\r']) { + let mut err = ValidationError::new("cookie_metacharacters"); + err.message = + Some("cookie_domain must not contain cookie metacharacters (;, \\n, \\r)".into()); + return Err(err); + } + Ok(()) +} + fn validate_no_trailing_slash(value: &str) -> Result<(), ValidationError> { if value.ends_with('/') { let mut err = ValidationError::new("trailing_slash"); @@ -1371,6 +1384,32 @@ mod tests { ); } + #[test] + fn test_publisher_rejects_cookie_domain_with_metacharacters() { + for bad_domain in [ + "evil.com;\nSet-Cookie: bad=1", + "evil.com\r\nX-Injected: yes", + "evil.com;path=/", + ] { + let mut settings = create_test_settings(); + settings.publisher.cookie_domain = bad_domain.to_string(); + assert!( + settings.validate().is_err(), + "should reject cookie_domain containing metacharacters: {bad_domain:?}" + ); + } + } + + #[test] + fn test_publisher_accepts_valid_cookie_domain() { + let mut settings = create_test_settings(); + settings.publisher.cookie_domain = ".example.com".to_string(); + assert!( + settings.validate().is_ok(), + "should accept a valid cookie_domain" + ); + } + /// Helper that returns a settings TOML string WITHOUT any admin handler, /// for tests that need to verify uncovered-admin-endpoint behaviour. fn settings_str_without_admin_handler() -> String { diff --git a/crates/trusted-server-core/src/synthetic.rs b/crates/trusted-server-core/src/synthetic.rs index 38a3456f..776005ce 100644 --- a/crates/trusted-server-core/src/synthetic.rs +++ b/crates/trusted-server-core/src/synthetic.rs @@ -16,7 +16,7 @@ use sha2::Sha256; use uuid::Uuid; use crate::constants::{COOKIE_SYNTHETIC_ID, HEADER_X_SYNTHETIC_ID}; -use crate::cookies::handle_request_cookies; +use crate::cookies::{handle_request_cookies, synthetic_id_has_only_allowed_chars}; use crate::error::TrustedServerError; use crate::settings::Settings; @@ -114,34 +114,50 @@ pub fn generate_synthetic_id( Ok(synthetic_id) } -/// Gets or creates a synthetic ID from the request. +fn validate_existing_synthetic_id(source: &str, synthetic_id: &str) -> Option { + if synthetic_id_has_only_allowed_chars(synthetic_id) { + return Some(synthetic_id.to_string()); + } + + log::warn!( + "Rejected synthetic_id from {source} with disallowed characters; \ + ignoring it and using another valid source or a fresh synthetic ID" + ); + None +} + +/// Gets an existing synthetic ID from the request. /// /// Attempts to retrieve an existing synthetic ID from: /// 1. The `x-synthetic-id` header /// 2. The `synthetic_id` cookie /// -/// If neither exists, generates a new synthetic ID. +/// Existing values that contain disallowed characters are rejected and ignored. +/// If no valid existing value remains, returns [`None`]. /// /// # Errors /// -/// - [`TrustedServerError::Template`] if template rendering fails during generation -/// - [`TrustedServerError::SyntheticId`] if ID generation fails +/// - [`TrustedServerError::InvalidHeaderValue`] if the Cookie header contains invalid UTF-8 pub fn get_synthetic_id(req: &Request) -> Result, Report> { if let Some(synthetic_id) = req .get_header(HEADER_X_SYNTHETIC_ID) .and_then(|h| h.to_str().ok()) { - let id = synthetic_id.to_string(); - log::trace!("Using existing Synthetic ID from header: {}", id); - return Ok(Some(id)); + if let Some(id) = validate_existing_synthetic_id("x-synthetic-id header", synthetic_id) { + log::trace!("Using existing Synthetic ID from header: {}", id); + return Ok(Some(id)); + } } match handle_request_cookies(req)? { Some(jar) => { if let Some(cookie) = jar.get(COOKIE_SYNTHETIC_ID) { - let id = cookie.value().to_string(); - log::trace!("Using existing Trusted Server ID from cookie: {}", id); - return Ok(Some(id)); + if let Some(id) = + validate_existing_synthetic_id("synthetic_id cookie", cookie.value()) + { + log::trace!("Using existing Trusted Server ID from cookie: {}", id); + return Ok(Some(id)); + } } } None => { @@ -158,7 +174,8 @@ pub fn get_synthetic_id(req: &Request) -> Result, Report