Reject placeholder partner pull tokens on startup

[dhruv8sh]

Summary

• reject_placeholder_secrets checked ec.partners[].api_token for known placeholder values but never checked ec.partners[].ts_pull_token, so an unmodified template value for that field passed startup and was sent as the outbound pull-sync bearer token.

Changes

File	Change
crates/trusted-server-core/src/settings.rs	Added a ts_pull_token branch to the partner loop in reject_placeholder_secrets, reporting it as ec.partners[<source_domain>].ts_pull_token; added 2 tests: placeholder pull token rejected, realistic pull token accepted

Closes

Closes #1144

Test plan

• cargo test-fastly && cargo test-axum
• cargo clippy-fastly && cargo clippy-axum
• cargo fmt --all -- --check
• JS tests: cd crates/trusted-server-js/lib && npx vitest run
• JS format: cd crates/trusted-server-js/lib && npm run format
• Docs format: cd docs && npm run format
• WASM build: cargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1
• Manual testing via fastly compute serve
• Other: addressed review feedback — partner_api_token/partner_ts_pull_token are secret-store key names shipped in the example config, not resolved secret values, and reject_placeholder_secrets compares against resolved values. Removed those two entries from API_TOKEN_PLACEHOLDERS, retargeted the affected test to an existing value placeholder, and dropped the drift-guard test that asserted the incorrect invariant.

Checklist

• Changes follow AGENTS.md conventions
• No unwrap() in production code — use expect("should ...")
• Uses tracing macros (not println!)
• New code has tests
• No secrets or credentials committed

[prk-Jr]

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]

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.

[aram356]

Summary

The ts_pull_token branch added to reject_placeholder_secrets is a genuine gap-fill: api_token was checked and ts_pull_token was not, and both are declared secret leaves in secret_fields(). That part is correct and should land.

The two new API_TOKEN_PLACEHOLDERS entries are a different matter. partner_api_token / partner_ts_pull_token are secret-store key names, but the list is compared against resolved secret values. Details and the evidence are in the inline comment.

2 of the inline comments below carry a one-click GitHub suggestion — use Commit suggestion (or Add suggestion to batch) to apply them as commits on the PR branch. The remaining comment describes the concern in prose because it is a question about the approach rather than a mechanical edit.

How this was verified

Rather than reason from reading alone, I probed the real blob path (settings_from_config_blob) end to end:

Case	Result
Template key name + secret provisioned under it (the normal operator path)	Resolves to the real secret value. The new entries never fire.
Secret store returns the key name as the stored value	Insecure default … ts_pull_token — the new entries fire

I also removed only the two new list entries and re-ran the suite:

• reject_placeholder_secrets_allows_realistic_partner_pull_token — still passes
• reject_placeholder_secrets_includes_partner_pull_tokens — fails
• example_toml_partner_secret_examples_are_recognized_placeholders — fails

Both failing tests depend entirely on the new entries. Retargeting the first to the pre-existing placeholder partner-api-token-32-bytes-minimum makes it pass without them, which shows the real fix stands on its own and currently has no test independent of the key-name entries.

Blocking

❓ question

• New placeholder entries are secret-store key names, not resolved values — see inline at crates/trusted-server-core/src/settings.rs:453

Non-blocking

⛏ nitpick

• Em dash in assert message — see inline at crates/trusted-server-core/src/settings.rs:5491
• Doc comment grammar and format — see inline at crates/trusted-server-core/src/settings.rs:5470

Cross-cutting / body-level findings

• 📝 The regex itself is sound. Tested empirically against the template: ^\s*#?\s*(?:api_token|ts_pull_token)\s*= matches exactly the two intended lines. Because \s* matches whitespace only, # partner_api_token = "x", tinybird_api_token = "x" and x_ts_pull_token = "z" all correctly fail to match, so the absent leading word boundary is harmless. The checked_any backstop is the right call. Minor blind spot: ## api_token = "x" (two #) and single-quoted or multiline TOML values do not match, so a partial reformat of one key could silently drop that key's coverage while checked_any stays true.

• 📌 Pre-existing: a padded ts_pull_token reaches the outbound Bearer header untrimmed. is_placeholder_api_token trims for comparison, but ec/pull_sync.rs:237 sends format!("Bearer {}", token.expose()) with the raw value, so " real-token " passes validation and produces a malformed header. RequestSigning::is_unusable_store_id (settings.rs:772) handles exactly this shape with store_id != store_id.trim(), and the comment at settings.rs:3115-3123 documents the reasoning. I confirmed this behavior is unchanged from main, so it is out of scope for this PR — worth a follow-up issue rather than a change here.

• 📝 Two concerns considered and dismissed, recorded so they are not re-raised. (1) The unconditional check does not break deployments with a disabled partner: remove_inactive_secret_references (config_payload.rs:75-86) strips ts_pull_token before resolution whenever pull_sync_enabled is falsy, so it is always None by the time reject_placeholder_secrets runs; string_false_pull_sync_flag_removes_a_stale_token and inactive_optional_features_do_not_resolve_stale_secret_references both pin this. (2) An unprovisioned secret key already fails startup at secret_resolution.rs:169-181, so the "operator never provisioned the key" case was never reachable as a live placeholder token.

• 🌱 Optional test hardening. No test asserts that a partner with both api_token and ts_pull_token set to placeholders reports both fields — test_partner_with_pull_token hardcodes api_token: None, so nothing pins that the loop accumulates rather than short-circuiting. Separately, test_partner_with_pull_token is roughly the twelfth full-struct EcPartner { … } literal in the crate; config_payload.rs:217 partner_with_pull_sync uses a lighter serde_json::from_value form that survives new fields being added. Both are non-blocking.

CI Status

All checks passing at a9715d55. Locally at the PR head: cargo fmt --all -- --check passes, cargo clippy-axum is clean, and the full trusted-server-core lib suite passes (2670 tests).

• integration tests (Fastly EC lifecycle): PASS
• integration tests: PASS
• browser integration tests: PASS
• CodeQL: PASS
• cargo test (ts CLI, native): PASS
• Analyze (javascript-typescript): PASS
• vitest: PASS
• cargo check (cloudflare native + wasm32-unknown-unknown): PASS
• cargo fmt: PASS (required)
• cargo test (axum native): PASS
• cargo test: PASS (required)
• format-typescript: PASS (required)
• Analyze (rust): PASS
• CLAUDE.md symlink guard: PASS
• format-docs: PASS (required)
• cargo check/build/test (spin native + wasm32-wasip1): PASS
• cargo test (cross-adapter parity): PASS
• prepare integration artifacts: PASS
• Analyze (actions): PASS

[aram356]

Summary

Round 2 at b4413d2a3. All three findings from the previous pass are resolved, and the diff is now a minimal, correctly-layered change: the new ts_pull_token branch in reject_placeholder_secrets, a doc-comment correction, and two tests. API_TOKEN_PLACEHOLDERS is byte-identical to main again (verified by diffing the constant against base), so the only production behavior change is the new check.

The remaining comments are non-blocking and can be resolved as-is or deferred to follow-ups.

Resolution of the previous round

Previous finding	Status
❓ Key-name entries in API_TOKEN_PLACEHOLDERS	Resolved — both removed, list matches main
⛏ Em dash in assert message	Resolved — containing test dropped
⛏ Doc comment grammar	Resolved — containing test dropped

The doc comment now reads "Known partner secret placeholders (api_token and ts_pull_token)", which I confirmed is accurate: the list is consumed at settings.rs:3089 for api_token and settings.rs:3096 for ts_pull_token.

How this round was verified

Mutation test. I deleted the new production block and re-ran the suite: reject_placeholder_secrets_includes_partner_pull_tokens fails. This is the meaningful change from round 1, where both new tests passed only because of the bogus key-name entries — the retargeted test now genuinely guards the production check. settings.rs was restored byte-identically afterward.

End-to-end through the real startup path, not a direct unit call. Driving settings_from_config_blob -> validate_settings_for_runtime with a partner whose ts_pull_token resolves to a real placeholder value:

Insecure default value for: ec.partners[partner.example.com].ts_pull_token

Local gates at this head: cargo fmt --all -- --check clean, cargo clippy-axum zero lints, full trusted-server-core lib suite 2763 passed / 0 failed.

New-base regression check. The merge commit brought a week of main but made zero changes to settings.rs (git show b4413d2a3 -- <path> is empty). Only one unrelated main commit touched adjacent files (7dbb83efe, a Prebid test fixture in config.rs). The new clippy-cli / clippy-codegen aliases from the updated AGENTS.md are present in .cargo/config.toml.

Blast radius. No in-repo fixture declares ts_pull_token, and none of the five placeholder values appears in any real config or fixture, so extending the list's reach to the new field cannot break an existing deployment or test.

Non-blocking

🌱 seedling

• No test covers both placeholders on the same partner — see inline at crates/trusted-server-core/src/settings.rs:5420

📌 out of scope

• Issue #1144's acceptance criteria are incorrect and will auto-close unamended — see below
• Padded ts_pull_token reaches the outbound Bearer header untrimmed — see below

Cross-cutting / body-level findings

• 📌 Issue #1144's second acceptance criterion is wrong, and merging will close it unamended. The issue requires: "The placeholder lists cover the literals trusted-server.example.toml actually ships (partner_api_token, partner_ts_pull_token)". That conflates secret-store key names with resolved secret values, and this PR is correct to contradict it — the PR body documents the deviation clearly. The concern is only that Closes #1144 will auto-close the issue with its incorrect premise preserved for whoever reads it next. Worth a short correcting comment on #1144 before or after merge, noting that the four sibling secret fields (proxy_secret, passphrase, handler password, trusted_client_ip.shared_secret) all ship key names in the template and none appears in its own placeholder list, so the convention deliberately excludes key names.

• 📌 Pre-existing: a padded ts_pull_token reaches the outbound Bearer header untrimmed. is_placeholder_api_token trims for comparison (settings.rs:460), but ec/pull_sync.rs:237 sends format!("Bearer {}", token.expose()) with the raw value, so " real-token " passes validation and yields a malformed header plus a silent 401 from the partner. RequestSigning::is_unusable_store_id (settings.rs:772) handles exactly this shape with store_id != store_id.trim(), and the comment at settings.rs:3115-3123 documents the reasoning. I confirmed this behavior is unchanged from main, so it is genuinely out of scope for this PR — better as its own issue than as a change here.

CI Status

All 20 checks PASS at b4413d2a3, 0 pending, 0 failures (confirmed after the three integration-test jobs settled). Locally at this head: cargo fmt --check clean, cargo clippy-axum clean, core lib suite 2763/2763.

• Analyze (actions): PASS
• Analyze (javascript-typescript): PASS
• Analyze (rust): PASS
• CLAUDE.md symlink guard: PASS
• CodeQL: PASS
• browser integration tests: PASS
• 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): PASS
• cargo test (cross-adapter parity): PASS
• cargo test (ts CLI, native): PASS
• format-docs: PASS (required)
• format-typescript: PASS (required)
• integration tests: PASS
• integration tests (Fastly EC lifecycle): PASS
• prepare integration artifacts: PASS
• vitest: PASS

[dhruv8sh]

Squash-merged into rc/202609 as e5b8ec4

[aram356]

Summary

Round 3 at 4170c4cb0. Re-approving because my previous approval was pinned to b4413d2a3 and the head has since moved. The production logic is byte-identical to the version already approved (verified by diffing the partner loop against b4413d2a3); everything new is test-only plus two merges of main.

The previous round's 🌱 seedling is closed. The two remaining comments are out-of-scope notes carried forward, resolvable as-is or as follow-ups.

What changed since the last approval

Commit	What
d7dcb183a	Author's own: +30/-1, test-only, closes the seedling
eb2bae2d8, 4170c4cb0	Two merges of main

The seedling fix adopted the suggested shape: test_partner_with_tokens(api_token, ts_pull_token) with test_partner_with_pull_token delegating to it, plus a new reject_placeholder_secrets_reports_both_partner_tokens. One detail beyond what was asked: the new test uses two distinct placeholder values (partner-api-token-32-bytes-minimum and replace-with-partner-api-token-32-bytes-minimum) rather than repeating one. I confirmed both are genuinely present in API_TOKEN_PLACEHOLDERS, so the assertion is not vacuous.

Verification

Mutation testing — the author's own claim, reproduced and extended. The PR comment states that adding a continue after the api_token push makes the new test fail. That holds, and the two mutations are caught by different tests, which confirms the new test covers a gap the existing two miss:

Mutation	Result
continue after the api_token push	only reject_placeholder_secrets_reports_both_partner_tokens fails
Remove the ts_pull_token check entirely	..._includes_partner_pull_tokens and ..._reports_both_partner_tokens fail

settings.rs was restored byte-identically after each mutation.

Merge integrity. The --cc combined diff is empty for both merge commits, and reconstructing the merge independently with git merge-file (ours = eb2bae2d8, theirs = cb9452545, base = their merge-base) reproduces the head's settings.rs exactly: head == pure automatic 3-way merge, zero hand edits. Worth stating explicitly because the b4413d2a3..4170c4cb0 range reads as busy — a /_ts/admin/cache/purge endpoint, a HashSet -> BTreeSet switch, new consent_store rejection tests — none of which belongs to this PR. The true PR-vs-base diff remains one production check, one doc comment, two test helpers, and three tests.

Gates at this head. cargo fmt --all -- --check clean; cargo clippy-axum zero lints; trusted-server-core lib suite 2886 passed / 0 failed. CI 20/20 PASS, 0 pending, 0 failures.

Conventions on added lines. No unwrap(), no println!/eprintln!, no thiserror/anyhow, zero em dashes; every expect/expect_err message follows the "should ..." form; the new helper takes a plain Option<&str> rather than the AGENTS.md-banned impl Into<Option<_>>.

Non-blocking

📌 out of scope

• Issue #1144's second acceptance criterion is incorrect and will auto-close unamended — see below
• Padded ts_pull_token reaches the outbound Bearer header untrimmed — see below

Cross-cutting / body-level findings

• 📌 Issue #1144's second acceptance criterion is wrong, and Closes #1144 will close it unamended. The issue requires: "The placeholder lists cover the literals trusted-server.example.toml actually ships (partner_api_token, partner_ts_pull_token)". That conflates secret-store key names with resolved secret values, and this PR is correct to contradict it — the PR body documents the deviation. Re-checked at this head: the issue is still OPEN with zero comments. The concern is only that it will close with its incorrect premise preserved for whoever reads it next. A short correcting comment would help: the four sibling secret fields (publisher.proxy_secret, ec.passphrase, handler password, trusted_client_ip.shared_secret) all ship key names in the template and none appears in its own placeholder list, so the convention deliberately excludes key names.

• 📌 Pre-existing: a padded ts_pull_token reaches the outbound Bearer header untrimmed. is_placeholder_api_token trims for comparison (settings.rs:460), but ec/pull_sync.rs:237 sends format!("Bearer {}", token.expose()) with the raw value, so " real-token " passes validation and yields a malformed header plus a silent 401 from the partner. RequestSigning::is_unusable_store_id (settings.rs:772) handles exactly this shape with store_id != store_id.trim(), and the comment at settings.rs:3115-3123 documents the reasoning. Re-diffed the bearer region against main at this head: identical, so this is genuinely pre-existing and out of scope for this PR. Better as its own issue than a change here.

CI Status

All 20 checks PASS at 4170c4cb0, 0 pending, 0 failures.

• Analyze (actions): PASS
• Analyze (javascript-typescript): PASS
• Analyze (rust): PASS
• CLAUDE.md symlink guard: PASS
• CodeQL: PASS
• browser integration tests: PASS
• 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): PASS
• cargo test (cross-adapter parity): PASS
• cargo test (ts CLI, native): PASS
• format-docs: PASS (required)
• format-typescript: PASS (required)
• integration tests: PASS
• integration tests (Fastly EC lifecycle): PASS
• prepare integration artifacts: PASS
• vitest: PASS