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

[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

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

[jevansnyc]

👍

[aram356]

@aram356 to open new PR

Dismissing to merge it