Add per-cookie template cache policy - #1150
Conversation
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Summary
Approved. The implementation is sound; one non-blocking code-smell note is attached inline.
| ec_context.kv_snapshot().entry_for(identity).is_some(), | ||
| "should preload this reader's identity even during withdrawal on a hit" | ||
| ); | ||
| let mut response = match finalizer { |
There was a problem hiding this comment.
S1. Repeated finalizer switch
The new withdrawal test repeats the existing Finalizer::Streaming and Finalizer::Buffered dispatch at line 9571. Both paths must keep testing streaming and buffered finalization, but duplicating this switch makes later finalizer changes easy to apply in only one location.
Consider extracting the finalizer dispatch into one test helper and calling it from both locations. This adds one helper and some parameter passing.
There was a problem hiding this comment.
Addressed in cd9ccd4. Extracted finalize_test_publisher_response and routed both the shared publisher fixture and the EC withdrawal test through it. Both streaming and buffered finalizers remain covered, including warm-cache withdrawal. The full Fastly test suite passes.
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
Grows the cookie-independence boolean into a per-cookie policy: named key cookies become sorted, length-prefixed dimensions of the shared template key, named bypass cookies force the inline path, and the assertion narrows to unlisted cookies. The runtime logic holds up under tracing: one decision drives lookup, reservation, and store, so a bypass cookie can never read or populate a template; Vary: Cookie refusal is untouched; empty dimensions preserve legacy keys; the fingerprint picks up the new fields through settings serialization; Debug output redacts values. Tests cover the evaluator matrix, key isolation, both finalizers, EC withdrawal on warm hits, and the harness negative control, and CI runs both harness modes.
Nothing here is blocking on its own. The findings below are hit-rate, operator-safety, and readability improvements.
4 of the inline comments below carry a one-click GitHub
suggestion. Use Commit suggestion (or Add suggestion to batch) to apply them. Each was applied in isolation and as a batch in a scratch worktree and passedcargo fmt --check, all six clippy aliases, all four adapter test aliases, the parity suite, and prettier. The remaining comments describe the fix in prose because the change spans ranges outside one hunk.
Non-blocking
♻️ refactor
- Log the cookie bypass reason at the request gate — see inline at
crates/trusted-server-core/src/publisher.rs:4304 - Reject TS identity cookie names in the key list — see inline at
crates/trusted-server-core/src/cookies/template_cache_policy.rs:28
🤔 thinking
- Duplicate unlisted names need not bypass under an independence assertion — see inline at
crates/trusted-server-core/src/cookies/template_cache_policy.rs:73
⛏ nitpick
- Doc sentence for
template_cache_bypass_cookies— see inline atcrates/trusted-server-core/src/creative_opportunities.rs:344 - Unformatted 230-character assert — see inline at
crates/trusted-server-core/src/cookies/template_cache_policy.rs:335 - Primary docs example pairs the lists with
false— see inline atdocs/guide/configuration.md:1912 CookieForwardedvariant name is stale — see inline atcrates/trusted-server-core/src/publisher.rs:5727
Cross-cutting / body-level findings
- 🌱 No bound on key-cookie value length.
is_cookie_valueaccepts any cookie-octet run of any length, so a mistakenly keyed token or ID enters the key unchecked. The spec scopes cardinality controls out, but a cheap bypass above a fixed byte length (64 is generous for experiment arms and region buckets) would catch the accidental case without limiting real variant labels. Future work, not this PR. - 🤔 Named-policy strictness for unlisted values. Under a named policy with independence
true, every unlisted cookie value is still framing-checked inis_ignored_cookie_value: a raw space, a non-ASCII byte, or a backslash in any third-party cookie bypasses the whole request. That is stricter than the legacy boolean path and partially reintroduces the "nearly inert" problem this feature targets. The origin-parser rationale is documented in the spec and the guide, so this is a heads-up on expected hit rate during the canary rather than a request to change it.
CI Status
- browser integration tests: PASS
- integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- CodeQL: PASS
- cargo test (ts CLI, native): PASS
- format-typescript: PASS (required)
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- Analyze (javascript-typescript): PASS
- Analyze (actions): PASS
- cargo test (axum native): PASS
- cargo test: PASS (required)
- format-docs: PASS (required)
- Analyze (rust): PASS
- cargo fmt: PASS (required)
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- cargo test (cross-adapter parity): PASS
- prepare integration artifacts: PASS
- vitest: PASS
| ) { | ||
| TemplateCookieDecision::Bypass => (true, Vec::new()), | ||
| TemplateCookieDecision::Eligible(values) => (false, values), | ||
| }; |
There was a problem hiding this comment.
♻️ refactor — A cookie bypass leaves no log at all. The key is None, so no reservation happens and the store-side template_cache_ttl never runs. Operators canarying this policy see bypass-request with no way to tell which of the six request gates fired, and this gate now has many more reasons to fire (bypass name, unlisted cookie, duplicate, malformed field). The method and request-semantics gates a few lines down already log this way; the message is static, so no cookie bytes reach the log.
| }; | |
| }; | |
| if cookie_disqualifies && matches!(assembly_mode, AssemblyMode::Esi) { | |
| log::debug!( | |
| "template_cache bypass: {}", | |
| TemplateCacheBypassReason::CookieForwarded | |
| ); | |
| } |
(scratch-verified: fmt, all six clippy aliases, all four adapter test aliases, and the parity suite)
There was a problem hiding this comment.
Addressed in cd9ccd4. Added the static debug log at the request cookie gate when ESI mode is active, using the renamed CookiePolicy reason. It includes no cookie names or values and runs even when bypass prevents reservation and store-side checks.
| /// assertion is caught whenever the origin is honest about it, and this only widens | ||
| /// the window where the origin personalizes *silently*. | ||
| /// Empty values still count as present. Names must be unique and must not overlap | ||
| /// [`Self::template_cache_key_cookies`]. Unset or empty names no bypass cookies. |
There was a problem hiding this comment.
⛏ nitpick — The sentence is broken: "Unset or empty names no bypass cookies."
| /// [`Self::template_cache_key_cookies`]. Unset or empty names no bypass cookies. | |
| /// [`Self::template_cache_key_cookies`]. Unset or empty means no bypass cookies. |
There was a problem hiding this comment.
Addressed in cd9ccd4. Corrected the sentence to “Unset or empty means no bypass cookies.”
| for value in [b"".as_slice(), b"\"\"", b"!#$%&'()*+-./0123456789:<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[]^_`abcdefghijklmnopqrstuvwxyz{|}~"] { | ||
| let mut field = b"ab_bucket=".to_vec(); | ||
| field.extend_from_slice(value); | ||
| assert_eq!(evaluate_cookie_policy(&headers(&[&field]), &names(&["ab_bucket"]), &[], false), TemplateCookieDecision::Eligible(vec![dimension("ab_bucket", Some(value))]), "should accept every cookie-octet and preserve quotes"); | ||
| } |
There was a problem hiding this comment.
⛏ nitpick — Line 338 is a 230-character assert_eq! that rustfmt gave up on because the byte-string literal cannot be broken. Binding the literal first lets rustfmt format the rest.
| for value in [b"".as_slice(), b"\"\"", b"!#$%&'()*+-./0123456789:<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[]^_`abcdefghijklmnopqrstuvwxyz{|}~"] { | |
| let mut field = b"ab_bucket=".to_vec(); | |
| field.extend_from_slice(value); | |
| assert_eq!(evaluate_cookie_policy(&headers(&[&field]), &names(&["ab_bucket"]), &[], false), TemplateCookieDecision::Eligible(vec![dimension("ab_bucket", Some(value))]), "should accept every cookie-octet and preserve quotes"); | |
| } | |
| let every_octet: &[u8] = | |
| b"!#$%&'()*+-./0123456789:<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[]^_`abcdefghijklmnopqrstuvwxyz{|}~"; | |
| for value in [b"".as_slice(), b"\"\"", every_octet] { | |
| let mut field = b"ab_bucket=".to_vec(); | |
| field.extend_from_slice(value); | |
| assert_eq!( | |
| evaluate_cookie_policy(&headers(&[&field]), &names(&["ab_bucket"]), &[], false), | |
| TemplateCookieDecision::Eligible(vec![dimension("ab_bucket", Some(value))]), | |
| "should accept every cookie-octet and preserve quotes" | |
| ); | |
| } |
(scratch-verified: cargo fmt --check, cargo clippy-fastly, and the template_cookie_policy tests)
There was a problem hiding this comment.
Addressed in cd9ccd4. Bound the byte-string fixture to every_octet, allowing rustfmt to format the loop and assertion across lines. Cookie-policy regressions and Rust formatting pass.
| # Default false. Enable only after proving unlisted cookies do not change HTML. | ||
| origin_is_cookie_independent = false |
There was a problem hiding this comment.
⛏ nitpick — This primary example pairs both lists with false. With either list present, false bypasses every request carrying any unlisted cookie, which includes the TS identity cookie, so the example as shown shares almost nothing on real traffic. The harness and the downstream-CDN example further down both use true. Keeping false here is fine as the safe placeholder, but the comment should say what it costs.
| # Default false. Enable only after proving unlisted cookies do not change HTML. | |
| origin_is_cookie_independent = false | |
| # Default false. With the lists above, false still bypasses every request that | |
| # carries any other cookie, including the TS identity cookie. Set true only | |
| # after proving those unlisted cookies do not change origin HTML. | |
| origin_is_cookie_independent = false |
(scratch-verified: prettier check)
There was a problem hiding this comment.
Addressed in cd9ccd4. Kept the safe false default and expanded the example comment to explain that any unlisted cookie, including the TS identity cookie, still forces bypass. It now states when enabling the independence assertion is appropriate. Documentation formatting passes.
| if parsed.insert(name, value).is_some() { | ||
| return TemplateCookieDecision::Bypass; | ||
| } |
There was a problem hiding this comment.
🤔 thinking — A duplicate unlisted name bypasses even when independence is asserted. Browsers legitimately send same-name cookies when they exist for different Path or Domain scopes (RFC 6265 §5.4), so a site with a cookie set on both the apex and www bypasses every reader under a named policy. For an unlisted cookie the operator has already asserted the value is irrelevant to origin HTML, so which duplicate wins is irrelevant too. Only key-cookie duplicates are genuinely ambiguous, and bypass names bypass regardless of duplication.
Restricting the duplicate check to key names keeps the safety argument and recovers hit rate:
let keyed = key_names.iter().any(|item| item.as_bytes() == name);
if parsed.insert(name, value).is_some() && keyed {
return TemplateCookieDecision::Bypass;
}Apply manually — the spec (§6, "Any repeated cookie name anywhere in the request causes bypass") and the a=1; a=1 rows in template_cookie_policy_rejects_malformed_and_duplicate_fields would need to change with it.
There was a problem hiding this comment.
Addressed in cd9ccd4. Restricted duplicate detection to key-cookie names. Unlisted duplicates are eligible only with independence asserted, and every occurrence still passes the existing framing checks; bypass names always bypass. Updated the specification, guide, evaluator matrices, and publisher coverage for duplicate unlisted cookies through both finalizers. Duplicate key names still bypass for equal or different values, within one field or across fields.
| for name in names { | ||
| if !is_cookie_name(name.as_bytes()) { | ||
| return Err(format!("{field} contains invalid cookie name `{name}`")); | ||
| } |
There was a problem hiding this comment.
♻️ refactor — Keying on ts-ec is a one-word config mistake that silently creates the per-user cache the design forbids, and the docs can only warn against it. TS knows its own identity cookie names (COOKIE_TS_EC, COOKIE_TS_EIDS, COOKIE_SHAREDID in constants.rs), so validation can reject them in the key list at load time. Bypassing on them is an odd but legitimate choice, so only the key list needs the guard.
if field == "template_cache_key_cookies"
&& [COOKIE_TS_EC, COOKIE_TS_EIDS, COOKIE_SHAREDID].contains(&name.as_str())
{
return Err(format!(
"{field} must not key on Trusted Server identity cookie `{name}`"
));
}Apply manually — needs the crate::constants import and a new case in template_cookie_policy_validates_names, which live outside this range.
There was a problem hiding this comment.
Addressed in cd9ccd4. Configuration validation now rejects COOKIE_TS_EC, COOKIE_TS_EIDS, and COOKIE_SHAREDID in the key list while allowing them in the bypass list. Added regression coverage for all three names and documented the restriction. The error identifies the configuration field and forbidden cookie.
| /// Named bypass cookies, unlisted cookies without an independence assertion, | ||
| /// or ambiguous input under a named policy prohibit both lookup and storage. | ||
| #[display("request cookies disqualified by template cache policy")] | ||
| CookieForwarded, |
There was a problem hiding this comment.
⛏ nitpick — CookieForwarded no longer describes the reason. Its display text now says "disqualified by template cache policy", and the variant fires for bypass names, unlisted cookies, duplicates, and malformed fields, not for forwarding. CookiePolicy would match the new meaning.
Apply manually — the rename also touches the template_cache_ttl return at line 6151 and the existing a_forwarded_request_cookie_disqualifies_even_without_set_cookie test, both outside this hunk.
There was a problem hiding this comment.
Addressed in cd9ccd4. Renamed the variant to CookiePolicy, including the request-gate log, store-side return, and existing bypass test. Local validation passes: all four adapter test aliases, all six Clippy aliases, 13 parity tests, 901 JavaScript tests, the JS build, and Rust/JS/docs formatting.
3 participants
Summary
Vary: Cookierefusal.Changes
crates/trusted-server-core/src/creative_opportunities.rscrates/trusted-server-core/src/cookies.rscrates/trusted-server-core/src/cookies/template_cache_policy.rscrates/trusted-server-core/src/platform/template_cache.rscrates/trusted-server-core/src/platform/mod.rscrates/trusted-server-core/src/publisher.rscrates/trusted-server-adapter-fastly/src/template_cache.rsscripts/template-cache-local-test.shtrusted-server.example.tomldocs/guide/configuration.mddocs/superpowers/specs/2026-09-08-1138-per-cookie-template-cache-policy-design.mddocs/superpowers/plans/2026-09-08-1138-per-cookie-template-cache-policy.mdIgnored-value tolerance applies only when independence is asserted. Names, key values, and cookie framing remain validated; unmatched quotes, unsafe bytes, and comma-delimited cookie assignments still bypass. The origin must treat ignored values as opaque and parse semicolon-separated cookies independently; parsers that stop at nonstandard values cannot safely make this assertion.
Closes
Closes #1138
Test plan
Fresh review-readiness validation on 2026-09-10 used
1f56ee08cb25a35d79cccc19a442a698334a398cplus the harness/documentation follow-up in191380f0. Production Rust and JavaScript are unchanged by that follow-up.cargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1.BID_DELAY=3 ./scripts/template-cache-local-test.sh esi— 22 harness checks passed, including all 17 cookie-matrix requests.BID_DELAY=3 ./scripts/template-cache-local-test.sh inline— 9 harness checks passed, including all 17 cookie-matrix requests with an origin fetch on every request.cargo test-fastly— 2,902 tests/doc-tests passed; 10 existing tests ignored.cargo test-axum,cargo test-cloudflare,cargo test-spin— 41, 44, and 86 tests passed respectively.cargo test --manifest-path crates/trusted-server-integration-tests/Cargo.toml --test parity— 13 passed.cargo fmt --all -- --check.crates/trusted-server-js/lib— 45 files, 901 tests passed; JS and docs formatting.bash -n scripts/template-cache-local-test.shandgit diff --check.The harness now returns distinguishable cookie-selected HTML and
Vary: X-Exp-Variantwithout a client variant header. Both modes verify A/B isolation, absent/empty buckets, ignored compact JSON/comma-list cookies, warm session bypass including empty sessions, and cold session bypass without populating anonymous templates. Each request checks content, cache diagnostics, origin fetch counts, private response policy, and winning-bid assembly. Existing CI runs both harness modes.These runtime runs invoke Viceroy directly, not
fastly compute serve. The earlier headless-browser/Viceroy smoke test reported separate A/Bmiss-stored→hitsequences and sessionbypass-request, all HTTP 200, with page JavaScript disabled to keep cookies stable. Its exact tested commit was not recorded in the previous description; it is historical evidence, not a fresh browser run at the current head.Viceroy/Fastly tests and Axum socket tests passed after granting the local certificate/keychain and loopback access needed outside the sandbox. An initial Vitest invocation used an incompatible Node environment; rerunning with the pinned Node executable from the library directory passed.
Checklist
unwrap()in production code.logmacros, notprintln!, per project conventions.