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