Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jevansnyc wants to merge 1 commit into
base: main
Choose a base branch
from
+17 −7
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions crates/trusted-server-adapter-fastly/src/app.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2108,8 +2108,9 @@ mod tests {
.to_str()
.expect("should render set-cookie as utf-8");
assert_eq!(
set_cookie, "ts-tester=true; Domain=.test-publisher.com; Path=/; Secure; SameSite=Lax",
"tester cookie should use publisher.cookie_domain"
set_cookie,
"ts-tester=true; Domain=.test-publisher.com; Path=/; Secure; SameSite=Lax; Max-Age=2592000",
"tester cookie should use publisher.cookie_domain and persist across browser restarts"
);
}

Expand Down
19 changes: 14 additions & 5 deletions crates/trusted-server-core/src/tester_cookie.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,18 @@ use crate::constants::COOKIE_TS_TESTER;
use crate::error::TrustedServerError;
use crate::settings::Settings;

/// Lifetime of the tester cookie in seconds (30 days).
///
/// Without an explicit lifetime the cookie is session-scoped, and Safari
/// deletes session cookies when the browser quits, so testers silently fall
/// back to the baseline arm on their next visit.
const TESTER_COOKIE_MAX_AGE_SECONDS: u32 = 2_592_000;

@prk-Jr prk-Jr Sep 9, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


/// Formats the tester cookie `Set-Cookie` header value.
fn format_tester_cookie(domain: &str) -> String {
format!(
"{}=true; Domain={}; Path=/; Secure; SameSite=Lax",
COOKIE_TS_TESTER, domain,
"{}=true; Domain={}; Path=/; Secure; SameSite=Lax; Max-Age={}",
COOKIE_TS_TESTER, domain, TESTER_COOKIE_MAX_AGE_SECONDS,
)
}

Expand All @@ -34,7 +41,8 @@ fn format_clear_tester_cookie(domain: &str) -> String {
///
/// Returns `404 Not Found` while `[tester_cookie].enabled` is false. When the
/// feature is enabled, returns `204 No Content` with `Set-Cookie: ts-tester=true`
/// scoped to `publisher.cookie_domain`.
/// scoped to `publisher.cookie_domain` and persisted for
/// [`TESTER_COOKIE_MAX_AGE_SECONDS`].
Comment on lines +44 to +45

@prk-Jr prk-Jr Sep 9, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

///
/// # Errors
///
Expand Down Expand Up @@ -138,8 +146,9 @@ mod tests {
.to_str()
.expect("should render set-cookie as utf-8");
assert_eq!(
set_cookie, "ts-tester=true; Domain=.tester.example; Path=/; Secure; SameSite=Lax",
"tester cookie should use publisher.cookie_domain"
set_cookie,
"ts-tester=true; Domain=.tester.example; Path=/; Secure; SameSite=Lax; Max-Age=2592000",
"tester cookie should use publisher.cookie_domain and persist across browser restarts"
);
}

Expand Down
Loading