Conversation
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.
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
| /// | ||
| /// # Errors | ||
| /// | ||
| /// Returns an error naming the field and cookie when a name is invalid or repeated. |
There was a problem hiding this comment.
⛏ nitpick — The # Errors section predates the identity-cookie rejection added in cd9ccd4.
| /// Returns an error naming the field and cookie when a name is invalid or repeated. | |
| /// Returns an error naming the field and cookie when a name is invalid or repeated, | |
| /// or when the key list names a Trusted Server identity cookie. |
| /// blank but consumed by a default path or `{network_id}` template; when a | ||
| /// slot has an invalid identifier, page pattern set, format list, or | ||
| /// dimensions; when `template_cache_max_age_seconds` falls outside 1–86,400; | ||
| /// dimensions; when cookie policy names are invalid, duplicated, or overlapping; |
There was a problem hiding this comment.
⛏ nitpick — Same gap here: validate_runtime now also fails when the key list names a TS identity cookie.
| /// dimensions; when cookie policy names are invalid, duplicated, or overlapping; | |
| /// dimensions; when cookie policy names are invalid, duplicated, or overlapping, | |
| /// or when the key list names a Trusted Server identity cookie; |
| # Invalid names, duplicates, and cross-list overlap reject configuration. | ||
| # Named policies bypass ambiguous/nonconforming Cookie fields, including duplicates. |
There was a problem hiding this comment.
⛏ nitpick — Two lines went stale with cd9ccd4: only duplicate key cookies bypass now, and the identity-cookie rejection is missing from the configuration-rejection list.
| # Invalid names, duplicates, and cross-list overlap reject configuration. | |
| # Named policies bypass ambiguous/nonconforming Cookie fields, including duplicates. | |
| # Invalid names, duplicates, cross-list overlap, and TS identity cookies in the | |
| # key list reject configuration. Named policies bypass ambiguous/nonconforming | |
| # Cookie fields, including duplicate key cookies. |
(scratch-verified: cargo fmt --check, cargo clippy-fastly, cargo check-axum, the template_cookie tests, and a TOML parse of the example file, alone and together with the other two suggestions)
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.