diff --git a/crates/common/src/cookies.rs b/crates/common/src/cookies.rs index 1b33d99b..2a1b78c9 100644 --- a/crates/common/src/cookies.rs +++ b/crates/common/src/cookies.rs @@ -61,13 +61,57 @@ pub fn handle_request_cookies( /// 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}`). Any stripped +/// characters are logged as a warning so header/cookie mismatches can be detected. +/// +/// 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_common::cookies::create_synthetic_cookie; +/// # use trusted_server_common::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 { + // Sanitize synthetic_id at runtime using an allowlist: only ASCII alphanumeric + // and `.`, `-`, `_` are permitted. This is stricter than a denylist and covers + // NUL bytes, spaces, tabs, and other control characters that a denylist would miss. + // Synthetic IDs originating from the x-synthetic-id request header are untrusted. + let safe_id: String = synthetic_id + .chars() + .filter(|c| c.is_ascii_alphanumeric() || matches!(c, '.' | '-' | '_')) + .collect(); + if safe_id.len() != synthetic_id.len() { + log::warn!( + "Stripped disallowed characters from synthetic_id before setting cookie (len {} -> {}); \ + the x-synthetic-id response header may differ from the cookie value", + synthetic_id.len(), + safe_id.len(), + ); + } format!( - "{}={}; Domain={}; Path=/; Secure; SameSite=Lax; Max-Age={}", - COOKIE_SYNTHETIC_ID, synthetic_id, settings.publisher.cookie_domain, COOKIE_MAX_AGE, + "{}={}; Domain={}; Path=/; Secure; HttpOnly; SameSite=Lax; Max-Age={}", + COOKIE_SYNTHETIC_ID, safe_id, settings.publisher.cookie_domain, COOKIE_MAX_AGE, ) } @@ -174,12 +218,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(); diff --git a/crates/common/src/settings.rs b/crates/common/src/settings.rs index 536cfcdc..31b35e91 100644 --- a/crates/common/src/settings.rs +++ b/crates/common/src/settings.rs @@ -18,6 +18,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, @@ -448,6 +449,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"); @@ -1242,6 +1255,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 {