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]

P1 — debug_assert! provides no protection in release builds

debug_assert! is compiled out in --release, which is exactly how this runs on Fastly Compute. The synthetic_id value comes from untrusted input (x-synthetic-id request header or the inbound cookie), so a crafted value like evil; Domain=.attacker.com would pass straight through in production.

Consider runtime sanitization here, e.g.:

let safe_id: String = synthetic_id
    .chars()
    .filter(|c| !matches!(c, ';' | '=' | '\n' | '\r'))
    .collect();

Or validate upstream in get_synthetic_id() with an allowlist pattern. The cookie_domain assert below is lower risk (config-sourced), but even there a regular assert! would catch config mistakes in production.

[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]

P2 — Asymmetric metacharacter lists deserve a comment

The synthetic_id check includes '=' but this one omits it. This is actually correct — = separates name from value so it only has special meaning in the value position, not in a domain attribute — but the asymmetry is subtle. A brief inline comment would help future readers:

// `=` is excluded: it only has special meaning in the name=value pair,
// not within an attribute like Domain.

[ChristianPavilonis]

P2 — No test coverage for the assertion guards

Consider adding #[should_panic] tests to document the invariant and verify the assertions fire:

#[test]
#[should_panic(expected = "should not contain cookie metacharacters")]
fn test_create_synthetic_cookie_rejects_semicolon_in_id() {
    let settings = create_test_settings();
    create_synthetic_cookie(&settings, "evil;injected");
}