Sanitize cookie value before Set-Cookie interpolation

[prk-Jr]

Summary

• A crafted synthetic_id containing RFC 6265-illegal characters (e.g. a semicolon) could inject spurious cookie attributes into the Set-Cookie header, enabling header-injection attacks.
• Adds is_safe_cookie_value() which validates every byte of the ID against the RFC 6265 cookie-octet allowlist before interpolation.
• If validation fails, the cookie is not set and a warning is logged; the X-Synthetic-ID response header is still emitted unchanged.

Changes

File	Change
crates/common/src/cookies.rs	Add is_safe_cookie_value() RFC 6265 validator; gate set_synthetic_cookie() on it; narrow create_synthetic_cookie() visibility to private; expand tests with injection, CRLF, space, non-ASCII, and valid-character cases
crates/common/src/publisher.rs	Add comment clarifying that cookie is intentionally skipped when ID is unsafe
crates/common/src/integrations/registry.rs	Add matching comment at the registry call site

Closes

Closes #413

Test plan

• cargo test --workspace
• cargo clippy --all-targets --all-features -- -D warnings
• cargo fmt --all -- --check
• JS format: cd crates/js/lib && npm run format
• Docs format: cd docs && npm run format

Checklist

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

[ChristianPavilonis]

PR Review — Staff-Engineer Pass

Findings: 0 blockers, 0 high, 3 medium, 2 low

This is a clean, well-executed security fix. The core RFC 6265 validation is correct and placed at the right architectural boundary. The findings below are quality improvements rather than correctness issues.

---

🤔 P2 — Empty string passes validation

crates/common/src/cookies.rs line 71–82

is_safe_cookie_value("") returns true because Iterator::all on an empty iterator is vacuously true. This would produce a Set-Cookie header like synthetic_id=; Domain=... — technically valid per RFC 6265 but semantically meaningless. Since this is a security boundary function, the contract should be explicit:

fn is_safe_cookie_value(value: &str) -> bool {
    !value.is_empty()
        && value
            .bytes()
            .all(|b| matches!(b, 0x21 | 0x23..=0x2B | 0x2D..=0x3A | 0x3C..=0x5B | 0x5D..=0x7E))
}

Add a corresponding test:

#[test]
fn test_is_safe_cookie_value_rejects_empty_string() {
    assert!(!is_safe_cookie_value(""), "should reject empty string");
}

---

🤔 P2 — Missing #[must_use] on is_safe_cookie_value

crates/common/src/cookies.rs line 71

Security-critical validation function whose return value must never be silently discarded. The original create_synthetic_cookie had #[must_use]. Adding it here protects against future callers accidentally ignoring the result:

#[must_use]
fn is_safe_cookie_value(value: &str) -> bool {

---

🤔 P2 — Missing doc comment on create_synthetic_cookie

crates/common/src/cookies.rs line 84

The original function had a doc comment which was removed when visibility was narrowed to private. Even private functions that format security-sensitive headers benefit from a brief doc comment:

/// Formats the `Set-Cookie` header value for the synthetic ID cookie.
fn create_synthetic_cookie(settings: &Settings, synthetic_id: &str) -> String {

---

⛏ P3 — Log message could include value length for diagnostics

crates/common/src/cookies.rs line 105–107

The warning doesn't include any context about the offending value. Logging the byte length helps operators diagnose issues without exposing the raw value:

log::warn!(
    "Rejecting synthetic_id for Set-Cookie: value of {} bytes contains characters illegal in a cookie value",
    synthetic_id.len()
);

---

👍 What's good

• RFC 6265 cookie-octet byte ranges are correct — verified against the spec
• Validation at the right boundary (set_synthetic_cookie) — single point of enforcement
• Visibility narrowing of create_synthetic_cookie to private — no external callers exist
• Explanatory comments at both call sites prevent future confusion about intentional skip
• Comprehensive test coverage: semicolon injection, CRLF, whitespace, non-ASCII, each illegal character class
• Minimal, focused change: 3 files, ~120 lines, no new dependencies, no unrelated refactoring