Harden the Edge Cookie withdrawal write path - #1113
Conversation
Tombstone only an identity the graph already holds. The marker exists to stop later reads of a real row, so writing one for an identifier that was never issued enforces nothing while still consuming a write and a row, and the identifier arrives in a client-supplied cookie. Confirm existence with the list API rather than a lookup. A lookup is eventually consistent, so a stale miss would discard a genuine withdrawal; the list is strongly consistent. Reject anything that is not a well-formed EC ID before querying, since this is a prefix query and an empty or truncated value would match unrelated keys. When the list cannot answer, re-check with a lookup instead of writing regardless. Eventual consistency yields false negatives, never false positives, so a hit is proof the identity exists while no fabricated identifier can produce one. If neither can answer, report the withdrawal unconfirmed and write nothing; the browser cookie is expired either way and remains the primary enforcement. Split the unusable-consent branch out of ec_finalize_response and route the per-identity result through one place, so an unconfirmed identity is logged as a fault while an unknown one is not.
The degraded path dropped both the list and lookup errors, leaving a store outage undiagnosable. Log both, and use the raw lookup so a corrupt-but-present row is not read as absent. Add a test pinning the fixed-width assumption the prefix check relies on.
Counting keys by prefix reported a held identity whenever any longer key started with the one asked for, so a withdrawal for an identity that was never issued still wrote a row. Add an exact, strongly consistent `key_exists` to the store and use it. This also drops the dependency on the identifier grammar: the check no longer cares what shape an identifier takes, only whether that key is present. Redact the key in the Fastly lookup error, matching the list error. Carry the reason for an unconfirmed withdrawal so it is logged once.
Scanning one page of prefix matches assumed the exact key would be in it. Nothing guarantees that when other keys share the prefix, and stopping early reports a held identity as missing, discarding its withdrawal. Iterate the pages instead. Also correct two test comments that still described the removed grammar gate.
A byte index landing inside a multi-byte character makes `get` return `None`, and the fallback printed the whole identifier — the opposite of what the redaction is for. Truncate by character, and reuse the helper in the Fastly store rather than repeating the byte form there. Prove the bounds check avoids a store call with a counting backend, and rename the test that claimed to cover a grammar gate that no longer exists.
|
Sequencing note on this PR and the provider stack. This PR and the open provider stack (#1043 to #1047, #1084, #1094) rework the same Edge Cookie finalize flow. A merge simulation of this PR's head (f6181d1, and identically its earlier head b5b68bb) against six of the seven stack heads conflicts in one file, Reproduce: The request is the one we have made on #885, #940 and #1094. The stack has been open since 19 August, carrying work that has been under review since 2 July as #838, is green on required CI, and its branches are kept rebased close to |
The exact-key scan lived in the Fastly store, where no test double can exercise paging. Extract it as `contains_exact_key` and have the backend supply pages to it, so multi-page matches, prefix-only keys, early exit and page errors are all covered natively.
|
Closes #1116 |
A third `Ok` variant was discarded by any caller inspecting only the error case, dropping a possibly-unrecorded withdrawal in silence. Returning `Err` keeps the underlying reports intact instead of flattening them into a string. Finish the redaction pass — insert, delete, and the deserialize paths still embedded the raw key — treat an empty prefix listing as absent rather than a failure, bound the pages an existence check will walk, and put the store trait's doc comment back on the trait.
The exact-key check followed a listing for a bounded number of pages and read running out of budget as absence. Absence is what tells the withdrawal path the identity was never issued, so a real identity on an unread page had its tombstone silently dropped — while the constant's own note and the tombstone docs both said the caller would treat that case as unconfirmed. Report it as a third outcome and map it to an error at the adapter, which puts it on the path that already re-checks by lookup. The page budget moves into the checked function so the listing is passed untruncated and there is no count to keep in agreement at the call site.
Nine messages interpolated the whole identifier: a duplicate create, upserts naming a missing or withdrawn key, and the CAS-exhaustion paths. Callers log these reports with debug formatting, so each one put a full identifier in a log line. The existing test only drove an injected backend failure, which never reaches them, so the module's claim that every message goes through the truncating helper held for the wrong reason. Route them through it too, and cover the paths a request can actually reach.
The test drove three of the message paths, so the other five held only by inspection. Extend it to the batched upserts and the three CAS-exhaustion terminal errors, which needed a conflict-injecting store that can hold a live entry rather than only a tombstone.
The conditional partner upsert's CAS-exhaustion error used the redacted template but no test executed it, so it was the one message still holding by inspection alone.
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
Gating the withdrawal tombstone on an existence check is the right call, and the premise holds up: handle_batch_sync maps both UpsertResult::NotFound and UpsertResult::ConsentWithdrawn to the same REASON_INELIGIBLE (crates/trusted-server-core/src/ec/batch_sync.rs:211), so skipping the write for a row that does not exist costs no enforcement. The identifier-redaction sweep is thorough, and the negative-case tests are well chosen.
The objection is to the mechanism rather than the goal. Choosing a prefix list over the lookup already on the trait introduces a reachable defect in the only production backend, doubles the happy-path round trips on the withdrawal response path, and pulls in the paging machinery, the third Undetermined state, the new trait method, and the expanded public surface that come with it. Switching the check to lookup_raw resolves the first finding and deletes the rest.
Verified locally against 7b5ea5720: cargo clippy-fastly clean, cargo fmt --all -- --check clean, cargo test -p trusted-server-core --target wasm32-wasip1 2280 passed, cross-adapter parity 13 passed.
1 of the inline comments below carries a one-click GitHub
suggestion— use Commit suggestion to apply it. The remaining comments describe the fix in prose because the change spans multiple files or lines outside the diff and cannot be auto-applied.
Blocking
🔧 wrench
ItemNotFoundmapped to an empty page re-issues the list instead of ending it — see inline atcrates/trusted-server-adapter-fastly/src/ec_kv.rs:160lookupanswers existence in one round trip, and the strong-consistency rationale is not maintained end to end — see inline atcrates/trusted-server-core/src/ec/kv.rs:666- The only production
EcKvStoreimplementation has no test coverage — see Cross-cutting below
Non-blocking
🤔 thinking / ♻️ refactor / 📝 note
MAX_EC_ID_LENis unreachable from the production call graph — see inline atcrates/trusted-server-core/src/ec/kv.rs:131warnfor a successful, expected fallback inverts this PR's own level convention — see inline atcrates/trusted-server-core/src/ec/kv.rs:735- Redaction verified clean at every reachable construction site — see inline at
crates/trusted-server-core/src/ec/kv.rs:746
Cross-cutting / body-level findings
-
🔧 The only production
EcKvStoreimplementation has no test coverage.crates/trusted-server-adapter-fastly/src/ec_kv.rshas no#[cfg(test)] mod tests, andFastlyEcKvStoreis the sole non-test implementor of the trait —crates/trusted-server-adapter-{axum,cloudflare,spin}contain noEcKvStoreimplementation at all, and the only production construction sites arecrates/trusted-server-adapter-fastly/src/main.rs:445and:477.The PR notes the pagination loop as a known gap, but the gap is wider than pagination.
contains_exact_keyis a pure function over an already-materialized page iterator and carries 8 tests; the untested code is the adapter glue that builds that iterator, decides what anErrpage means, and convertsUndeterminedinto aReport— which is exactly where the first finding above lives. A test double implementing theIterator<Item = Result<ListPage, KVStoreError>>shape and asserting the round-trip count would have caught it without a live Fastly store.
CI Status
- Analyze (actions): PASS
- Analyze (javascript-typescript): PASS
- Analyze (rust): PENDING
- CodeQL: SKIPPED
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- cargo fmt: PASS (required)
- cargo test: PASS (required)
- cargo test (axum native): PENDING
- cargo test (cross-adapter parity): PASS
- cargo test (ts CLI, native): PASS
- format-docs: PASS (required)
- format-typescript: PASS (required)
- prepare integration artifacts: PENDING
- vitest: PASS
No failing checks. The three pending gates were still in progress at review time.
| // the key is absent, which is what `lookup` already does for a | ||
| // missing key. Treating it as a failure would send every | ||
| // withdrawal naming an unissued identity down the error path. | ||
| Err(fastly::kv_store::KVStoreError::ItemNotFound) => Ok(Vec::new()), |
There was a problem hiding this comment.
🔧 wrench — Mapping ItemNotFound to an empty page does not end the listing; it makes the SDK re-issue the list call.
fastly-0.12.1's ListResponse::next() declares iterator_did_error but never assigns true to it — the field appears only three times in the SDK (declaration at kv_store.rs:139, init at :170, read at :185). Its error path returns Some(Err(..)) early, before the keys.is_empty() -> None check at kv_store.rs:215-217 that would terminate iteration. So an error yielded from a page does not close the iterator: the next next() call issues another handle.list(...) with the same cursor.
Converting that error into Ok(Vec::new()) here therefore does not mean "absent" — it means contains_exact_key sees a non-matching page and asks for another one, repeatedly, until the page budget trips.
I reproduced this with a harness replicating the SDK's next() control flow exactly:
A: persistent ItemNotFound -> result=Ok(Undetermined) backend_calls=5
B: persistent TooManyRequests -> result=Err("TooManyRequests") backend_calls=1
C: found on page 1 -> result=Ok(Found) backend_calls=1
Case A is the intended-benign path. KvSysError::NotFound maps to KVStoreError::ItemNotFound in the generic From<KvSysError> impl used by every KV operation including list (fastly-0.12.1/src/kv_store/handle.rs:75), so this arm is reachable. The outcome is the opposite of what the comment above it states: instead of avoiding the error path, a withdrawal naming an unissued identity costs 5 list round trips and then goes down the error path via Undetermined, into the lookup_raw fallback.
Apply manually — cannot be expressed as a single-file suggestion, because the durable fix is the one in the kv.rs:666 comment (use lookup), which removes this method. If the list approach is kept, this arm must terminate the iteration rather than yield an empty page — e.g. by taking the pages via a .take_while/scan that stops on the first ItemNotFound, or by treating ItemNotFound as a definitive Absent result before contains_exact_key is entered.
There was a problem hiding this comment.
Confirmed against the SDK before acting on it. ListResponse::next (fastly-0.12.1/src/kv_store.rs:178-224) declares iterator_did_error but never assigns it, and the error arm return Some(Err(e)) sits above the keys.is_empty() -> None check, so a page error leaves the iterator open with next_cursor and data untouched — the next call re-issues handle.list(...) with the same cursor. And the arm is reachable: KvSysError::NotFound -> KVStoreError::ItemNotFound is in the generic From<KvSysError> impl at kv_store/handle.rs:75, shared by every KV operation including list.
So the comment above that arm described the opposite of what it did. Resolved by the durable fix you pointed at rather than by terminating the iteration: key_exists is gone from the trait and from this backend, along with both EXACT_MATCH_* constants.
Commit 0031fc3.
| /// # Errors | ||
| /// | ||
| /// Returns [`TrustedServerError::KvStore`] on store error. | ||
| fn key_exists_confirmed(&self, ec_id: &str) -> Result<bool, Report<TrustedServerError>> { |
There was a problem hiding this comment.
🔧 wrench — EcKvStore::lookup already answers this question in one round trip, and the strong-consistency argument for preferring list is not maintained end to end.
Cost. On the withdrawal response path at the edge:
| Case | Before this PR | After |
|---|---|---|
| Identity held (the normal withdrawal) | 1 write | 1 list + 1 write = 2 |
| Identity not held | 1 write | 1 list |
| Degraded (list failing or over budget) | 1 write | up to 5 lists + 1 lookup + 1 write = ~7 |
build_list reads from the primary data source, which the SDK documents as the slower path (fastly-0.12.1/src/kv_store.rs:495).
The consistency rationale. The doc comment at kv.rs:654-657 rejects lookup because a lagging read "would report a freshly written identity as absent, discarding a genuine withdrawal." But the fallback immediately below (kv.rs:733-744) does exactly that: when the exact check fails it consults lookup_raw, and treats Ok(None) as grounds for not writing. The guarantee is therefore held only on the happy path and abandoned precisely when the store is degraded. It is not load-bearing.
Note also that the check and the write are already non-atomic (acknowledged at kv.rs:690-695), so a strong read at the check does not close the TOCTOU window either way.
What this buys. EcKvStore::lookup is already on the trait (kv_backend.rs:134), already implemented by FastlyEcKvStore (ec_kv.rs:64-88), already maps ItemNotFound -> Ok(None), and is already exposed here as lookup_raw (kv.rs:201). Existence becomes:
fn key_exists_confirmed(&self, ec_id: &str) -> Result<bool, Report<TrustedServerError>> {
Ok(self.lookup_raw(ec_id)?.is_some())
}Exact-key by construction, one round trip always, no prefix-collision concern. That change also deletes contains_exact_key, ExactKeyMatch, EXACT_MATCH_PAGE_SIZE, EXACT_MATCH_MAX_PAGES, the Undetermined third state, the FastlyEcKvStore::key_exists body that carries the ec_kv.rs:160 defect, the new EcKvStore::key_exists trait method along with its five test-double implementations, and the pub widening of crates/trusted-server-core/src/ec/kv_backend.rs.
Worth weighing explicitly: the residual risk of using lookup is a missed tombstone for an identity issued within the replication lag and withdrawn in that same window. The browser cookie is expired unconditionally in every one of those cases (finalize.rs:158), which this code itself calls the primary enforcement, so the exposure is limited to the batch-sync window.
Apply manually — spans kv.rs, kv_backend.rs, and ec_kv.rs together; cannot be a single-file suggestion.
There was a problem hiding this comment.
Adopted. key_exists_confirmed is now Ok(self.lookup_raw(ec_id)?.is_some()) (kv.rs:654).
Agreed the strong read was not load-bearing: the Ok(None) arm of the fallback returned without writing, so the guarantee was held on the happy path and dropped exactly when the store was degraded. And the check/write pair is non-atomic regardless.
That swap deleted everything it was holding up: contains_exact_key, ExactKeyMatch, EXACT_MATCH_PAGE_SIZE, EXACT_MATCH_MAX_PAGES, the Undetermined state, EcKvStore::key_exists and its five test-double impls, the pub widening in kv_backend.rs, and the entire lookup fallback block — which is where the comments at :735 and :746 lived. Net -186 lines across the two commits.
The residual risk you named is now in the doc comment rather than implicit: an identity issued and withdrawn inside the replication lag is reported unknown and gets no tombstone; the cookie is expired unconditionally, so the exposure is the batch-sync window.
Added a round-trip budget test as a regression guard (a_withdrawal_costs_one_read_whether_or_not_the_identity_is_held) so a future change cannot quietly reintroduce a two-read gate.
Commit 0031fc3.
| /// Bounds the work a caller-supplied cookie can ask for. It is not a format | ||
| /// check: the existence check is exact, so it does not depend on the shape of | ||
| /// an identifier. | ||
| const MAX_EC_ID_LEN: usize = 256; |
There was a problem hiding this comment.
🤔 thinking — This bound cannot be reached from any production caller, so it is not bounding client-supplied work.
key_exists_confirmed is private (fn, not pub(crate)), and its only non-test caller is write_withdrawal_tombstone. That in turn has exactly one production call site: crates/trusted-server-core/src/ec/finalize.rs:167. Every other call is inside a #[cfg(test)] module (admin.rs:1126, and the tests in this file).
That single production path sources its identifiers from withdrawal_ec_ids (finalize.rs:205-221), which gates both the cookie value and the active value through is_valid_ec_id. That predicate (crates/trusted-server-core/src/ec/generation.rs:151-171) requires exactly two dot-separated segments, a 64-character ASCII-hex first segment and a 6-character alphanumeric second — pinning every identifier that reaches this function to exactly 71 ASCII bytes. Never empty, never above 256.
So the doc comment here and the PR description both describe the bound as bounding "the work a caller-supplied cookie can ask for", but the cookie is already constrained to 71 characters two frames up, before the value arrives. The tests at kv.rs:1558 and kv.rs:1586 cover a branch production cannot take.
Not harmful — it is O(1) on a private function. But it reads as a load-bearing safety check and is not one, and if the lookup-based check above is adopted the length bound stops mattering entirely. Worth either dropping it or restating the comment as defence-in-depth for a hypothetical future caller.
There was a problem hiding this comment.
Checked and it holds: key_exists_confirmed is private, its only non-test caller is write_withdrawal_tombstone, and that has one production call site (finalize.rs:167) whose identifiers come from withdrawal_ec_ids, gated through is_valid_ec_id — exactly 71 ASCII bytes, never empty.
MAX_EC_ID_LEN and the guard are dropped, along with the branch-only test. With the lookup-based check nothing is scanned, so there is no work for a bound to bound.
Commit 0031fc3.
| // silence. | ||
| match self.lookup_raw(ec_id) { | ||
| Ok(Some(_)) => { | ||
| log::warn!( |
There was a problem hiding this comment.
♻️ refactor — A confirmed identity that then gets written is a success, and logging it at warn inverts the level convention this PR establishes elsewhere.
This line is reached on the fallback path the comment directly above describes as the intended degraded behaviour, and it fires only when the lookup succeeded and the tombstone write proceeds. Compare log_tombstone_outcome in finalize.rs, added in this same PR, which puts the expected UnknownIdentity case at debug (finalize.rs:186) and reserves error for the case where nothing was recorded (finalize.rs:196). By that rubric a confirmed-and-written fallback belongs at debug.
Volume: withdrawal_ec_ids returns a HashSet of up to 2 identifiers (finalize.rs:205-221), and apply_withdrawal_tombstones calls the writer once per entry, so a store-wide list outage produces up to 2 warn lines per withdrawal request across every POP, unsampled — while operators are reading logs to find the actual fault. It scales with withdrawal traffic rather than total traffic, so it is bounded, but it is per-request noise at warn for a non-fault.
| log::warn!( | |
| log::debug!( |
(Verified in a scratch worktree: cargo fmt --all -- --check clean, cargo clippy-fastly clean, cargo test -p trusted-server-core --target wasm32-wasip1 2280 passed.)
There was a problem hiding this comment.
The level convention point is right, and the line is gone rather than lowered — the whole lookup fallback block went with the switch to a lookup-based check, so there is no successful-fallback path left to log at any level.
log_tombstone_outcome in finalize.rs keeps the rubric you described: debug for the expected UnknownIdentity, error only when nothing was recorded.
Commit 0031fc3.
| )); | ||
| } | ||
| Err(lookup_error) => { | ||
| return Err(lookup_error.attach(format!( |
There was a problem hiding this comment.
📝 note — No leak here today; flagging the invariant this line now depends on.
current_context() returns &TrustedServerError, and KvStore's Display renders the message field verbatim (crates/trusted-server-core/src/error.rs:57-58), so whatever a backend put in message lands in this string.
I traced every construction site reachable as list_error — it originates from self.store.key_exists(ec_id), so on Fastly that is ec_kv.rs:165, ec_kv.rs:176-179, and open_store() at ec_kv.rs:36/:41. The first two redact through log_id; the third carries no identifier. I also checked the remaining message: constructions in ec_kv.rs (73, 115, 134, 190), kv.rs (172, 178, 184, 232, 238, 270, 768), and kv_backend.rs:295. All are redacted or identifier-free, so there is no disclosure today.
The structural point is that this converts a typed error into an opaque interpolated string, so the redaction invariant now rests on every present and future author of a TrustedServerError::KvStore { message } remembering to call log_id, with nothing at the type level enforcing it. A second backend implementing this trait would break it silently. If key_exists goes away per the kv.rs:666 comment, so does this line.
Minor inconsistency alongside it: the Ok(None) arm just above attaches a static string and discards the lookup detail, while this arm interpolates the other error's rendering. Both are defensible, but they preserve different amounts of context.
There was a problem hiding this comment.
The line is gone with the fallback block, so both the interpolation and the asymmetry between the two arms go with it.
The structural point stands on its own though, and I have not addressed it: nothing at the type level stops a future TrustedServerError::KvStore { message } from carrying an unredacted identifier. Every present site is redacted or identifier-free — including the ones you traced — but that rests on authorial discipline. Worth a tracked follow-up (a newtype that redacts on construction, or a Display that does) rather than something to bolt onto this PR.
Commit 0031fc3.
The prefix listing was chosen for its strong consistency, but that guarantee was never held end to end: when the listing could not answer, the fallback consulted `lookup` and treated `Ok(None)` as grounds for not writing, so the strong read was abandoned precisely when the store was degraded. It also carried a defect in the only production backend. `ListResponse::next` in fastly-0.12.1 declares `iterator_did_error` but never assigns it, and its error arm returns `Some(Err(..))` before the `keys.is_empty()` check that ends iteration, so a page error does not close the iterator — the next call re-issues the list with the same cursor. Mapping `ItemNotFound` to an empty page therefore did not mean "absent"; it meant "ask again", until the page budget tripped. That arm was reachable, because `KvSysError::NotFound` maps to `KVStoreError::ItemNotFound` in the `From` impl every KV operation shares. `lookup` is exact by construction and answers in one round trip, so it needs no page budget, no third `Undetermined` state, no trait method, and no bound on the identifier's length — nothing is scanned. A withdrawal for a held identity now costs one read and one write instead of two round trips, on the slower of the two read paths. The residual risk is stated in the doc comment rather than hidden: an identity issued and withdrawn inside the replication lag is reported as unknown and gets no tombstone. The browser cookie is expired unconditionally either way, which is the primary enforcement, so the exposure is the batch-sync window.
`FastlyEcKvStore` is the only non-test implementor of `EcKvStore` and had no tests at all, so the adapter glue — what an error from the platform means, which failures are control flow and which are faults — held only by inspection. It needs no live service to exercise: `fastly.toml` already declares `ec_identity_store` for the local simulator, and the Fastly adapter's tests run under Viceroy, so the backend can be driven against a real KV store. Cover the unlinked store, a missing key reading as absent rather than as a failure, an insert/lookup/delete round trip, both precondition modes, and prefix counting.
|
Addressed all five findings. Two commits: 0031fc3 swaps the check, cee87c2 adds the adapter coverage. Net -186 lines. Blocking
Non-blocking
Also added a round-trip budget test pinning one read per withdrawal, so a future change cannot quietly restore a two-read gate. Verification: |
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Summary
The write gate correctly prevents arbitrary client-selected identities from creating tombstones, and the redaction and local outcome handling look sound. I found one blocking correctness issue in the existence decision; details are inline.
| /// | ||
| /// Returns [`TrustedServerError::KvStore`] on store error. | ||
| fn key_exists_confirmed(&self, ec_id: &str) -> Result<bool, Report<TrustedServerError>> { | ||
| Ok(self.lookup_raw(ec_id)?.is_some()) |
There was a problem hiding this comment.
P1 — An eventually consistent miss can discard a genuine withdrawal
lookup_raw uses Fastly’s eventually consistent lookup path. If an identity is withdrawn shortly after issuance, a lagging POP can return None, causing line 703 to classify the real identity as UnknownIdentity and skip the tombstone.
This is not limited to the browser response. The issuance request can already have disclosed the EC ID through post-response pull sync (crates/trusted-server-adapter-fastly/src/main.rs:221-222). Once the original row becomes visible, a later batch sync sees consent.ok == true (kv.rs:593-600), accepts a partner mapping, and writes it with the one-year live-entry TTL (kv.rs:620-624). The missed withdrawal can therefore leave the identity live and writable for up to a year, with a later write refreshing that TTL.
Please use a strongly consistent exact-key existence operation. If paginated listing is restored, handle Fastly ItemNotFound by explicitly terminating the operation rather than yielding an empty iterator page, and return an error when the page budget is exhausted instead of falling back to eventual lookup. A regression test should model ordinary lookup missing a newly written row while the strongly consistent existence path still finds it.
4 participants
Summary
ts-eccookie named, whether or not the identity graph held that identity. The identifier is client-supplied and only shape-checked.How existence is determined
Worth reading before the diff, because the obvious approaches are wrong in ways that matter.
An exact check, not a prefix count. Counting keys by prefix reports a held identity whenever any longer key starts with the one asked for, so a withdrawal for an identity that was never issued would still write a row.
EcKvStoregainskey_exists, and the Fastly implementation lists by prefix and compares the returned keys for equality.Every page is followed. Nothing guarantees the exact key lands in the first page when other keys share its prefix, and stopping early would report a held identity as missing and discard its withdrawal.
The list, not a lookup. On Fastly a lookup is eventually consistent while
build_list()defaults toListMode::Strong. A stale lookup would report an identity issued moments earlier as missing.A raw lookup as fallback when the exact check fails. An unanswerable check is not evidence of absence, but writing regardless would restore the unconditional write whenever the store can be made to fail. A lagging lookup may miss a very recent write, and may return a row already deleted at the primary, but it cannot report an identifier this deployment never issued. The raw form is used because a row whose body no longer deserializes is still a row.
If neither can answer, nothing is written. The outcome is reported as unconfirmed with the reason, logged once at the request boundary. The browser cookie is expired in every case and remains the primary enforcement; only the batch-sync revocation marker is at stake.
Behaviour
write_withdrawal_tombstonereturns aTombstoneOutcome(Written/UnknownIdentity/Unconfirmed { reason }) rather than(), so the caller can tell expected traffic from a fault. An unknown identity is ordinary — the cookie is client-supplied — and logs at debug. An unconfirmed one means the store could not be read and logs at error.Changes
crates/trusted-server-core/src/ec/kv_backend.rsEcKvStore::key_exists— exact, strongly consistent existencecrates/trusted-server-core/src/ec/kv.rsTombstoneOutcome; bound the identifier length to cap the work a cookie can ask forcrates/trusted-server-core/src/ec/finalize.rsfinalize_unusable_consent(ec_finalize_response80 → 48 lines)crates/trusted-server-core/src/ec/mod.rslog_idtruncates by character — a byte index inside a multi-byte character made it emit the whole identifiercrates/trusted-server-adapter-fastly/src/ec_kv.rskey_existswith pagination; reuselog_idinstead of three local byte truncationsTest plan
cargo test-fastly && cargo test-axum && cargo test-cloudflare && cargo test-spintrusted-server-clicargo fmt --all -- --check19 tests added across both layers. The negative cases carry the regression protection; the happy-path ones exist to catch the gate over-blocking, which is the real risk here — a dropped opt-out is worse than an extra row. One test drives a store that cannot answer at all, asserting
Max-Age=0and header removal survive it.Known coverage gap: the pagination loop cannot be exercised from a test double, since the in-memory backend has no paging. It rests on the SDK contract and code review.
Closes
Closes #1116
Reviewer note — three PRs touch this code
is_valid_ec_idto the built-in provider's grammar. The KV gate no longer depends on it, so that is retired insidekv.rs— but resolving the finalize-side merge toward this branch'swithdrawal_ec_idswould reinstate the grammar filter a layer up and drop vendor identities.if let Errat the shared call site still compiles against the three-variant return, so a merge toward that version drops the outcome handling with no compile error.Checklist
unwrap()in production codelogmacros (notprintln!)