Persist the tester cookie for 30 days so it survives browser restarts

[jevansnyc]

Fixes #1136

Problem

GET /_ts/set-tester minted the ts-tester cookie without Max-Age or Expires, making it a session cookie. Safari deletes session cookies when the application quits, so Safari testers lost the cookie on every restart and silently fell back to the baseline arm on their next visit. Chrome and Firefox mask the defect because session restore resurrects session cookies.

Change

• Add TESTER_COOKIE_MAX_AGE_SECONDS (30 days, 2592000) and append Max-Age to the tester cookie in format_tester_cookie
• Update the two tests that assert the exact Set-Cookie string (core tester_cookie test, fastly dispatch_set_tester test)

/_ts/clear-tester already expires the cookie explicitly with Max-Age=0 and is unchanged. The module documents that the cookie only affects tester routing and must not gate sensitive behavior, so the bounded persistent lifetime does not change its security posture.

Testing

• Test-first: updated both assertions, watched both fail, then implemented
• cargo test-fastly: 170 adapter + 2357 core tests pass
• cargo test-axum: pass
• cargo fmt --all -- --check, cargo clippy-fastly, cargo clippy-axum: clean

Generated with Claude Code

[prk-Jr]

Summary

Correct one-line fix for a real Safari defect: the tester cookie was session-scoped, so Safari testers silently fell back to the baseline arm after every browser restart. The Max-Age addition is right, the named const carries its rationale, and /_ts/clear-tester was correctly left alone (already Max-Age=0). One blocking gap: the two published docs pages that quote the exact Set-Cookie line were not updated, so the operator-facing docs now contradict the code.

1 of the inline comments below carries a one-click GitHub suggestion — use Commit suggestion to apply it as a commit on the PR branch. The other comments describe the change in prose because the files are outside this diff or the wording is yours to choose.

Blocking

🔧 wrench

• Published docs still print the pre-Max-Age Set-Cookie line — see the Cross-cutting section below (both files are outside this diff, so there is no inline anchor)

Non-blocking

🤔 thinking

• Safari caps this at 7 days behind a CNAME — see inline at crates/trusted-server-core/src/tester_cookie.rs:22
• 30-day self-service enrollment window — see the Cross-cutting section below

⛏ nitpick

• Public doc links a private const (new rustdoc warning) — see inline at crates/trusted-server-core/src/tester_cookie.rs:45

🌱 seedling

• Keep ts-tester off any future ignorable-cookie cache allowlist — see the Cross-cutting section below

👍 praise

• Test-first, with the why captured in the const — see the Cross-cutting section below

Cross-cutting / body-level findings

• 🔧 Published docs still print the pre-Max-Age Set-Cookie line — Two operator-facing docs quote the emitted header verbatim and were not updated, so both are now factually wrong and both ship to the docs site via deploy-docs.yml:

  • docs/guide/api-reference.md:36 — Set-Cookie: ts-tester=true; Domain=<publisher.cookie_domain>; Path=/; Secure; SameSite=Lax
  • docs/guide/configuration.md:467 — the same string

  The repo's own precedent is that these pages move with the header: docs/guide/api-reference.md was updated in the very commits that introduced these routes (563ec96d, #795 and 3dc1262c, #797).

  Proposed fix — append the attribute to both http blocks:

  Set-Cookie: ts-tester=true; Domain=<publisher.cookie_domain>; Path=/; Secure; SameSite=Lax; Max-Age=2592000
  Cache-Control: no-store, private

  Worth one sentence alongside each block noting the 30-day lifetime and that /_ts/clear-tester is the opt-out, since configuration.md already documents the clear path's Max-Age=0. Run npm run format in docs/ afterwards to satisfy the required format-docs gate. Apply manually — can't be auto-applied as a suggestion because neither file is part of this diff.

• 🤔 30-day self-service enrollment window — /_ts/set-tester is unauthenticated whenever [tester_cookie].enabled = true, and the cookie is scoped to the whole publisher.cookie_domain. Before this change a stray visit self-limited at browser close; now it pins that browser to the tester arm for a month, and /_ts/clear-tester is the only opt-out — which a non-QA visitor has no reason to know about. Shared and kiosk browsers now carry the arm for 30 days.

  That is very likely fine: the module doc already states the cookie only affects tester routing and must not gate sensitive behavior, and the feature is off by default. Worth confirming explicitly that a 30-day contamination window is acceptable for whatever consumes the arm (measurement purity in particular), and that the flag is expected to be enabled only during QA windows rather than left on.

• 🌱 Keep ts-tester off any future ignorable-cookie cache allowlist — No action for this PR; flagging for whoever lands a per-cookie template-cache policy.

  Today persistence is harmless for the shared template cache: TemplateCacheBypassReason::CookieForwarded (crates/trusted-server-core/src/publisher.rs:5706) disqualifies any cookie-bearing request, and template_cache_vary rejects Cookie outright (crates/trusted-server-core/src/creative_opportunities.rs:478).

  But TS forwards Cookie to origin unchanged — there is no Cookie strip on the publisher path — so if a future policy treats some cookies as ignorable for cache-key purposes, ts-tester must not be one of them. An origin that varies the document on the tester arm would otherwise have tester HTML stored under a reader-neutral key and served to every reader. This change widens the window in which a tester's browser carries the cookie from one session to a month, which makes that mistake more likely to be observed rather than less.

• 👍 Test-first, with the why captured in the const — Both assertions were updated and watched failing before the implementation landed; the const carries the Safari session-cookie rationale rather than a bare 2592000; the assertion messages were tightened to state the new contract instead of being left describing only the domain; and format_clear_tester_cookie was correctly left untouched. Verified independently: Set-Cookie also survives EC finalize because crates/trusted-server-core/src/ec/cookies.rs:144,167 use append rather than insert, and nothing rewrites Set-Cookie attributes on egress (response_privacy only downgrades Cache-Control).

CI Status

• cargo fmt: PASS (required)
• cargo test: PASS (required)
• format-docs: PASS (required)
• format-typescript: PASS (required)
• cargo test (axum native): PASS
• cargo test (cloudflare native): PASS — reported as cargo check (cloudflare native + wasm32-unknown-unknown)
• cargo check/build/test (spin native + wasm32-wasip1): PASS
• cargo test (cross-adapter parity): PASS
• cargo test (ts CLI, native): PASS
• integration tests: PASS
• integration tests (Fastly EC lifecycle): PASS
• browser integration tests: PASS
• prepare integration artifacts: PASS
• vitest: PASS
• CodeQL: PASS
• Analyze (rust): PASS
• Analyze (javascript-typescript): PASS
• Analyze (actions): PASS

[prk-Jr]

⛏ nitpick — The public doc on handle_set_tester links a private const, which rustdoc warns about and which renders as plain code rather than a link for readers of the public docs. Verified against this PR head:

warning: public documentation for `handle_set_tester` links to private item `TESTER_COOKIE_MAX_AGE_SECONDS`
  --> crates/trusted-server-core/src/tester_cookie.rs:45:7
   = note: `#[warn(rustdoc::private_intra_doc_links)]` on by default

Not CI-blocking — no workflow runs cargo doc, and 18 other instances of this warning already exist in the crate — but CLAUDE.md asks for cargo doc --no-deps --all-features verification. Making the const pub would be the worse fix (CLAUDE.md: consider visibility carefully, avoid unnecessary pub), so stating the lifetime in prose is the cheaper option:

Suggested change

	/// scoped to `publisher.cookie_domain` and persisted for
	/// [`TESTER_COOKIE_MAX_AGE_SECONDS`].
	/// scoped to `publisher.cookie_domain` and persisted for 30 days.

Scratch-verified at this head: cargo fmt --all -- --check clean, cargo clippy-fastly clean, and cargo doc -p trusted-server-core --no-deps --all-features drops from 19 to 18 private_intra_doc_links warnings with the tester one gone.

[prk-Jr]

🤔 thinking — The rationale names Safari specifically, which is worth pairing with Safari's own cap: Safari 14's CNAME-cloaking defense caps the expiry of cookies set in HTTP responses to 7 days when the setting host is a CNAME to a third party. That is the standard Trusted Server deployment shape — a publisher-owned hostname CNAME'd to the compute platform — so the lifetime a Safari tester actually gets is plausibly 7 days rather than 30.

That doesn't undermine the fix at all: 7 days beats one browser session, and #1136 is still resolved. But the const's doc currently reads as though 30 days is what Safari will honor, and nothing in the repo documents an ITP cap today. A sentence like "Safari may shorten this to 7 days when the TS host is a CNAME to the compute platform (ITP's CNAME-cloaking cap), which is still far better than a session cookie" would keep the next person from re-opening this as "the tester lost the arm after a week."

Worth confirming against your own deployment shape before wording it — the cap only applies to CNAME-cloaked hosts, so a deployment that isn't CNAME'd gets the full 30 days. Apply manually — the wording is yours, and this is doc-only.

[aram356]

Summary

The fix itself is correct and matches #1136: Max-Age=2592000 is appended in format_tester_cookie, the clear path keeps its explicit Max-Age=0, and both exact-string test assertions were updated. Requesting changes for two things the diff leaves open: the lifetime should be operator-configurable rather than a hard-coded constant, and the guide docs still show the old Set-Cookie header verbatim.

None of the inline comments carry a one-click suggestion — both fixes span files outside this diff, so they are described in prose and have to be applied manually.

Blocking

🔧 wrench

• Make the tester-cookie lifetime configurable — see inline at crates/trusted-server-core/src/tester_cookie.rs:22

Non-blocking

🤔 thinking

• Safari may cap the lifetime to 7 days — see inline at crates/trusted-server-core/src/tester_cookie.rs:19

Cross-cutting / body-level findings

• 🔧 Docs still show the pre-change Set-Cookie header — docs/guide/configuration.md:536 and docs/guide/api-reference.md:36 both document the exact set-tester response header (ts-tester=true; Domain=<publisher.cookie_domain>; Path=/; Secure; SameSite=Lax) without Max-Age, so the guide now contradicts the server and will mislead QA reading the wire format. Please update both code blocks to include Max-Age, and once the lifetime is configurable (see the inline comment at crates/trusted-server-core/src/tester_cookie.rs:22) document the max_age_seconds key and its 30-day default in the [tester_cookie] field table at docs/guide/configuration.md:529.

CI Status

• cargo test: PASS (required)
• cargo fmt: PASS (required)
• format-docs: PASS (required)
• format-typescript: PASS (required)
• cargo test (ts CLI, native): PASS
• vitest: PASS
• cargo check (cloudflare native + wasm32-unknown-unknown): PASS
• cargo check/build/test (spin native + wasm32-wasip1): PASS
• cargo test (cross-adapter parity): PASS
• Analyze (javascript-typescript): PASS
• Analyze (actions): PASS
• cargo test (axum native): PENDING
• Analyze (rust): PENDING
• prepare integration artifacts: PENDING
• CodeQL: SKIPPED

[aram356]

🔧 wrench — The 30-day lifetime should be operator-configurable as [tester_cookie] max_age_seconds rather than a hard-coded constant, so publishers can tune tester rotation without a redeploy. The settings module already has this exact pattern (Ec::default_pull_sync_concurrency, settings.rs:551).

Proposed fix (apply manually — spans settings.rs, tester_cookie.rs, both tests, trusted-server.example.toml, and docs; can't be expressed as a single-file suggestion):

// settings.rs
/// Tester-cookie endpoint configuration.
#[derive(Debug, Clone, Deserialize, Serialize)]
pub struct TesterCookieConfig {
    /// Enable tester-cookie endpoints that set and clear `ts-tester`.
    #[serde(default)]
    pub enabled: bool,
    /// Lifetime of the tester cookie in seconds. Defaults to 30 days.
    #[serde(default = "TesterCookieConfig::default_max_age_seconds")]
    pub max_age_seconds: u32,
}

impl TesterCookieConfig {
    /// Default tester-cookie lifetime: 30 days.
    pub const fn default_max_age_seconds() -> u32 {
        TESTER_COOKIE_MAX_AGE_SECONDS
    }
}

impl Default for TesterCookieConfig {
    fn default() -> Self {
        Self {
            enabled: false,
            max_age_seconds: Self::default_max_age_seconds(),
        }
    }
}

// tester_cookie.rs
fn format_tester_cookie(domain: &str, max_age_seconds: u32) -> String {
    format!(
        "{}=true; Domain={}; Path=/; Secure; SameSite=Lax; Max-Age={}",
        COOKIE_TS_TESTER, domain, max_age_seconds,
    )
}

with the call site passing settings.tester_cookie.max_age_seconds.

Two details worth keeping:

• The manual Default impl matters: keeping derive(Default) would give max_age_seconds = 0 whenever the whole [tester_cookie] section is absent from the TOML. That is harmless today only because enabled is then also false, but it is a latent footgun for any future programmatic use of the struct.
• Consider a validation bound (e.g. 1..=31_536_000) the way template_cache_max_age_seconds does in creative_opportunities.rs — a configured 0 silently turns /_ts/set-tester into a clear operation.

Please also add # max_age_seconds = 2592000 under the commented [tester_cookie] block in trusted-server.example.toml (line 143).

[aram356]

🤔 thinking — On the browser that motivated this fix, 30 days may not stick: the typical trusted-server deployment is a publisher subdomain CNAME'd to a third-party edge host, and Safari's ITP CNAME-cloaking mitigation caps Set-Cookie lifetimes from such responses to 7 days. The change still solves the restart-loss problem (7 days is far better than session scope), but it may be worth a sentence in this doc comment so nobody promises Safari testers a 30-day window.

[ChristianPavilonis]

Summary

Reviewed 3c1b2dd3a57d6d69d076036eec2b9d23f1796ce9 against d704d0ab0c5916d429b80a5707c0f2d74b99cab1. No additional actionable findings beyond the issues already recorded in unresolved review feedback.

The Fastly route and core tests confirm that /_ts/set-tester emits Max-Age=2592000, remains non-cacheable, and that /_ts/clear-tester retains the matching scope with Max-Age=0. Focused WASM tests and cargo fmt --all -- --check passed, and all reported CI checks are green.