Conversation
reject_placeholder_secrets checked ec.partners[].api_token but never ts_pull_token, and the placeholder list didn't include the literal values trusted-server.example.toml actually ships. An operator who copied the template verbatim could deploy with a template string as the live pull-sync bearer token. Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
prk-Jr
left a comment
There was a problem hiding this comment.
Review: approve
Correct, minimal, and on the right layer. API_TOKEN_PLACEHOLDERS gains the two literals the template actually ships, and the new ts_pull_token branch in reject_placeholder_secrets is symmetric with the api_token branch that already shipped. One non-blocking suggestion inline; nothing here should hold the merge.
Verification
Mutation-tested both halves of the fix in a scratch worktree at a9715d55 — neither new test is vacuous:
| Mutation | Result |
|---|---|
| Baseline (unmodified head) | 2670 passed; 0 failed |
Remove the two new API_TOKEN_PLACEHOLDERS entries |
example_toml_... and ..._includes_partner_pull_tokens FAIL |
Remove the ts_pull_token branch, keep the placeholders |
..._includes_partner_pull_tokens FAILS |
Remote CI is 20/20 green, including all four required checks (cargo fmt, cargo test, format-typescript, format-docs) plus parity, spin, cloudflare, integration, browser, vitest and CodeQL.
Notes (no action required)
📝 The doc comment on API_TOKEN_PLACEHOLDERS no longer matches its contents. It reads "Known partner API token placeholders", and the predicate is is_placeholder_api_token, but the list now also holds partner_ts_pull_token and is consulted for ts_pull_token. Sharing one list across both fields is the right call; only the prose under-describes it. Renaming the pub const / pub fn would be a breaking API change and isn't worth it. Suggested wording, if you touch the file again:
/// Known partner secret placeholders (`api_token` and `ts_pull_token`) that
/// must not be used in deployments.
pub const API_TOKEN_PLACEHOLDERS: &[&str] = &[🤔 The description's threat model is a little wider than the code path. Tracing it: config_payload.rs:59 is the only production caller of validate_settings_for_runtime → reject_placeholder_secrets, and it runs after resolve_secret_references. secret_resolution.rs:169-181 (resolve_leaf) hard-errors on a missing key and never falls back to the config literal, so the runtime ts_pull_token is always a resolved secret-store value, never the template string. The docs (docs/guide/ec-setup-guide.md:34,63,81, docs/guide/configuration.md:607) present partner_api_token / partner_ts_pull_token as intended secret-store key names.
So the new check fires only when an operator provisioned the secret-store value as the literal template string — still worth rejecting as defense in depth, and I confirmed it cannot false-positive on the documented key-reference setup, since reject_placeholder_secrets never sees unresolved key names. Flagging only so the coverage isn't over-credited.
Related, also checked: the new branch validates ts_pull_token unconditionally, ignoring pull_sync_enabled. No false positive — config_payload.rs:76-88 (remove_inactive_secret_references) strips ts_pull_token from the blob whenever pull_sync_enabled is not true, before validation runs.
🌱 The drift guard covers the TOML template only, not the docs. docs/guide/ec-setup-guide.md:34,81 and docs/guide/configuration.md:606-607 ship the same literals and aren't scanned. They agree with the template today, so nothing to fix; a follow-up could widen EXAMPLE_TOML into a slice of include_str!'d sources.
👍
The new tests mirror the existing reject_placeholder_secrets_includes_handler_passwords shape exactly, use expect / expect_err with "should ..." messages, and assert on the field name rather than the token value, so no secret can leak into a failure message. example_toml_partner_secret_examples_are_recognized_placeholders is the right instinct — a self-maintaining guard beats a hand-copied constant.
What I did not verify
Runtime behaviour end to end. The claim that reject_placeholder_secrets gates the live pull-sync path comes from static call-graph tracing, not from executing a request with a placeholder token in a secret store.
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Review summary
Approved. Reviewed a9715d551ea69f4e37269b0bdcd494d52da9d4fb against 6cae7f5da8911c746cf873581885f90c3820dd96. The change correctly rejects resolved partner pull-token template values before they can reach outbound pull sync, while retaining realistic tokens. No actionable issues found.
Validation passed: focused Fastly-target tests for placeholder rejection, realistic-token acceptance, template drift coverage, runtime placeholder validation, and config-blob secret resolution; cargo clippy-fastly; cargo fmt --all -- --check; and git diff --check. All 20 reported CI checks are green.
Summary
reject_placeholder_secretscheckedec.partners[].api_tokenfor known placeholder values but never checkedec.partners[].ts_pull_token, so an unmodified template value for that field passed startup and was sent as the outbound pull-sync bearer token.partner_api_token/partner_ts_pull_token, the literal valuestrusted-server.example.tomlactually ships — so even the field that was checked would have missed the value an operator is most likely to leave behind.Changes
crates/trusted-server-core/src/settings.rspartner_api_token/partner_ts_pull_tokentoEcPartner::API_TOKEN_PLACEHOLDERS; added ats_pull_tokenbranch to the partner loop inreject_placeholder_secrets, reporting it asec.partners[<source_domain>].ts_pull_token; added 3 tests: placeholder pull token rejected, realistic pull token accepted, and a regex-based check that everyapi_token/ts_pull_tokenexample value intrusted-server.example.tomlis a recognized placeholderCloses
Closes #1144
Test plan
cargo test-fastly && cargo test-axumcargo clippy-fastly && cargo clippy-axumcargo fmt --all -- --checkcd crates/trusted-server-js/lib && npx vitest runcd crates/trusted-server-js/lib && npm run formatcd docs && npm run formatcargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1fastly compute serveChecklist
unwrap()in production code — useexpect("should ...")tracingmacros (notprintln!)