Add HttpOnly flag to synthetic ID cookie

[prk-Jr]

Summary

• The synthetic_id cookie was missing HttpOnly, allowing XSS on a publisher page to exfiltrate the tracking identifier via document.cookie.
• HttpOnly is safe to add because no client-side JS reads this cookie — integrations receive the synthetic ID via the x-synthetic-id response header instead.
• Adds debug_assert! guards against cookie metacharacter injection in both the synthetic_id value and cookie_domain, and documents the rationale for each security attribute in the function's doc comment.

Changes

File	Change
crates/common/src/cookies.rs	Add HttpOnly to cookie format string; add debug_assert! metacharacter guards; update doc comment to enumerate all security attributes with rationale

Closes

Closes #411

Test plan

• cargo test --workspace
• cargo clippy --all-targets --all-features -- -D warnings
• cargo fmt --all -- --check
• JS tests: cd crates/js/lib && npx vitest run — pre-existing ESM/CJS failure on main, unrelated to this change
• JS format: cd crates/js/lib && npm run format
• Docs format: cd docs && npm run format
• WASM build: cargo build --bin trusted-server-fastly --release --target wasm32-wasip1
• Manual testing via fastly compute serve

Checklist

• Changes follow CLAUDE.md conventions
• No unwrap() in production code — use expect("should ...")
• Uses tracing macros (not println!)
• New code has tests
• No secrets or credentials committed

[ChristianPavilonis]

Review Summary

Clean, well-scoped security hardening. The HttpOnly addition is correct, the doc comment is excellent, and the change is minimal. One concern about the debug_assert! approach and a few smaller items below.

0 Blockers · 1 High · 3 Medium

[ChristianPavilonis]

P2 — Missing # Panics doc section

Per project conventions, functions that can panic should document it. The debug_assert! macros below will panic in debug/test builds. Consider adding:

/// # Panics
///
/// Panics in debug builds if `synthetic_id` or `cookie_domain` contains
/// cookie metacharacters (`;`, `=`, `\n`, `\r`).

[ChristianPavilonis]

@prk-Jr sorry the previous review didn't have clear suggestions.

Solid, well-scoped security fix. The HttpOnly addition is correct and the doc comment is excellent. Three actionable findings — two about the sanitization approach, one about the assert!.

[ChristianPavilonis]

🔴 P1 — Silent sanitization creates header/cookie mismatch

In publisher.rs:337-338, the raw synthetic_id is set as the x-synthetic-id response header, then the sanitized value goes into the cookie:

response.set_header(HEADER_X_SYNTHETIC_ID, synthetic_id.as_str()); // raw
set_synthetic_cookie(settings, &mut response, synthetic_id.as_str()); // sanitized inside

If sanitization ever strips characters, the response header and cookie will contain different IDs — silently breaking any downstream system that correlates them.

Minimal fix for this PR: Add a log::warn! when sanitization actually modifies the value, so operators can detect the mismatch:

if safe_id.len() != synthetic_id.len() {
    log::warn!(
        "Stripped cookie metacharacters from synthetic_id (len {} -> {})",
        synthetic_id.len(),
        safe_id.len(),
    );
}

Follow-up: Move sanitization upstream to get_or_generate_synthetic_id() in synthetic.rs so both the header and cookie use the same cleaned value.

[ChristianPavilonis]

🔴 P1 — assert! should be a #[validate] on Publisher, not a per-request panic

This is the only assert! in production (non-test) code in the entire codebase, and it runs on every request. Since cookie_domain is baked into the binary at compile time via include_bytes!(), the value is immutable at runtime — if it's bad, every single request triggers a WASM trap (ungraceful 500, no structured logging, no to_error_response() fallback).

The project already has an established pattern for config validation: Publisher derives Validate, and origin_url uses #[validate(custom(function = validate_no_trailing_slash))]. The cookie_domain field has no validation at all — not even a length check.

Suggestion: Add a custom validator on Publisher::cookie_domain and remove the assert!:

// In settings.rs, on the Publisher struct:
#[validate(length(min = 1), custom(function = validate_cookie_domain))]
pub cookie_domain: String,

// Validator:
fn validate_cookie_domain(domain: &str) -> Result<(), validator::ValidationError> {
    if domain.contains([';', '\n', '\r']) {
        return Err(validator::ValidationError::new("cookie_domain contains cookie metacharacters"));
    }
    Ok(())
}

This way bad config fails the build (from_toml_and_env() calls settings.validate()) and fails at startup (get_settings() calls settings.validate()) — never at request time.

[ChristianPavilonis]

🟡 P2 — Allowlist is safer than denylist for untrusted input

The synthetic_id can arrive from the untrusted x-synthetic-id request header (synthetic.rs:130-136). The current denylist strips ;, =, \n, \r but misses NUL bytes, spaces, tabs, and other control characters that could cause issues in HTTP header serialization.

Since synthetic IDs have a known format ({64-char-hex}.{6-char-alphanumeric} per synthetic.rs:110), an allowlist is strictly safer:

let safe_id: String = synthetic_id
    .chars()
    .filter(|c| c.is_ascii_alphanumeric() || matches!(c, '.' | '-' | '_'))
    .collect();

This is a defense-in-depth improvement — the denylist works for known attack vectors, but the allowlist eliminates entire categories of future risk.

[ChristianPavilonis]

🟢 P3 — Missing # Examples doc section

Per CLAUDE.md line 198: "Add # Examples sections for public API functions." The doc comment is already excellent — thorough security rationale, # Panics section — but lacks # Examples. Minor, and other functions in this file don't have them either, so this is a nit rather than a blocker.