Document the config template and harden config validation#870
Open
aram356 wants to merge 9 commits into
Open
Conversation
a9331e7 to
387aa14
Compare
…tions Rework the app-config template so the required minimum (publisher, ec passphrase, admin handler) is active and documented, while every optional section and integration is commented out with a one-line description. Restore sections that still map to live config structs but had been dropped from the template: tester_cookie, rewrite, consent, image_optimizer, tinybird, osano, publisher.max_buffered_body_bytes, sourcepoint.auth_cookie_name, datadome protection fields, and prebid override rules. All example hosts stay on example.com. Keep the gpt, didomi, datadome, and google_tag_manager sections active with enabled = false so the ts audit CLI can still flip them in place.
387aa14 to
edd7250
Compare
Extend deploy-time validation (`ts config validate`/`push`, via `validate_settings_for_deploy`) to reject template placeholder values that previously validated OK and only failed later at runtime: - publisher.domain / cookie_domain / origin_url left as the example.com template defaults (deploy-only, in reject_placeholder_secrets, so the embedded example config still parses via Settings::from_toml) - request_signing.config_store_id / secret_store_id when the block is enabled (empty or the <management-...> placeholders) - integrations.aps.pub_id when APS is enabled: non-empty is enforced with the built-in validator `length(min = 1)`, and a custom validator rejects the reserved "your-aps-publisher-id" placeholder (no built-in expresses a reserved-value check). Integration configs validate lazily via get_typed, so this is naturally enabled-gated. Publisher and request_signing checks stay imperative because they are parse-time-sensitive or cross-field (gated on a sibling `enabled`) and cannot be expressed as single-field validator attributes. Closes #871
The GTM container_id and Prebid external_bundle_sha256 field validators were
custom functions that just ran a regex/hex-format check. Swap both to the
validator crate's built-in `regex` validator (validator 0.20 ships regex
support and an AsRegex impl for LazyLock<Regex>), preserving the operator-facing
error messages via the `message` attribute. Add tests asserting the accepted
and rejected inputs for each.
- GTM: custom validate_container_id -> regex(path = *GTM_CONTAINER_ID_PATTERN)
- Prebid: custom validate_external_bundle_sha256 -> regex against a new
^[0-9a-fA-F]{64}$ pattern
…dator testlight and datadome already constrain their timeouts via the validator crate's range validator; aps, prebid, adserver_mock, and auction did not. Add `range(min = 1, max = 60000)` to those timeout_ms fields for consistency (catches 0 and absurd values). aps/prebid/adserver_mock already derive Validate and validate lazily via get_typed; AuctionConfig now derives Validate and is wired into Settings validation with #[validate(nested)]. All existing configs use 500-2000 ms, well within range.
The test module's `use validator::Validate as _` was redundant with `use super::*` (which brings the trait in via the parent's import for the derive). Under CI's warnings-as-errors it broke both the clippy gate and `cargo test-fastly`.
ChristianPavilonis
left a comment
Collaborator
There was a problem hiding this comment.
Summary
Reviewed the config hardening and template changes. I found one high-severity configuration compatibility regression and three medium validation/template gaps; details are inline.
prk-Jr
requested changes
Jul 10, 2026
prk-Jr
left a comment
Collaborator
There was a problem hiding this comment.
Summary
The configuration-template and validation hardening is well scoped, and all CI checks pass. One enabled APS configuration edge case still bypasses the new publisher-ID validation.
Blocking
🔧 wrench
- Reject whitespace-only APS publisher IDs: the built-in length validator accepts non-empty whitespace, while the custom validator trims only for placeholder comparison (
crates/trusted-server-core/src/integrations/aps.rs:204).
CI Status
- fmt: PASS
- clippy / cargo checks: PASS
- rust tests: PASS
- js tests: PASS
- integration tests: PASS
Validate integration fields only once the config resolves to enabled. `get_typed` short-circuited solely on an explicit `enabled = false`, so a section that omitted the flag and fell back to a `false` serde default was still field-validated. That rejected documented template placeholders in integrations that were not actually on, breaking existing configs on upgrade. Reject blank APS publisher IDs in the custom validator: the built-in `length` validator counts whitespace, so a whitespace-only `pub_id` passed deploy validation. Check request-signing store IDs whenever the block is present rather than only when it is enabled. The key rotate/deactivate admin routes register unconditionally and read the IDs without consulting `enabled`, so placeholder IDs behind a disabled block still reached key management at runtime. Drop `cdn_origin` from the documented Sourcepoint block. The upstream host is pinned, so the example value made the block fail validation as written.
# Conflicts: # crates/trusted-server-core/src/integrations/prebid.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #869. Closes #871.
Config-surface cleanup in four focused commits: document the app-config template, close deploy-validation gaps that let placeholder values through, and lean the config structs onto the
validatorcrate's built-in validators instead of hand-rolled functions.1 · Document
trusted-server.example.tomland restore supported sectionsReworks the source-controlled app-config template (used by
ts config init, embedded into the Cloudflare/Spin adapters viainclude_str!, and text-patched byts audit).[[handlers]](admin),[publisher],[ec].[tester_cookie],[rewrite],[consent],[image_optimizer],[tinybird],[integrations.osano],publisher.max_buffered_body_bytes,sourcepoint.auth_cookie_name, DataDome protection fields, Prebid override rules.gpt/didomi/datadome/google_tag_manageras activeenabled = falsestubs sots auditcan flip them in place. All hosts areexample.com.2 · Reject fail-open placeholder config values at deploy validation
ts config validate/pushroute throughTrustedServerAppConfig::validate→validate_settings_for_deploy. That path now rejects template placeholders that previously validated OK and only failed later at runtime:publisher.domain/cookie_domain/origin_urlleft at theexample.comtemplate defaults.request_signing.config_store_id/secret_store_idwhen the block is enabled (empty or the<management-...>placeholders).integrations.aps.pub_idwhen APS is enabled — non-empty via the built-inlength(min = 1)validator, plus a custom validator for the reservedyour-aps-publisher-idplaceholder.Why some checks use
#[validate]and some don't: APS validates lazily throughget_typed(enabled-gated), so an attribute fires only when APS is on. Publisher / request_signing stay in the deploy-onlyreject_placeholder_secrets—Publisher::validate()runs at parse time on the embeddedexample.comtemplate (an attribute would reject the template itself), and request_signing rejection must be gated on the siblingenabledflag. This mirrors how the existing placeholder-secret rejection already works.3 · Replace hand-rolled GTM/Prebid validators with the built-in
regexvalidatorcontainer_idand Prebidexternal_bundle_sha256used custom validator functions that just ran a regex / hex-format check. Both now use#[validate(regex(...))](validator 0.20 ships regex support + anAsRegeximpl forLazyLock<Regex>); operator-facing messages preserved viamessage.4 · Bound
timeout_mswith the built-inrangevalidatortestlight/datadome already constrain their timeouts via
range;aps,prebid,adserver_mock, andauctiondid not. Addedrange(min = 1, max = 60000)(catches0and absurd values) for consistency.AuctionConfignow derivesValidateand is wired via#[validate(nested)]; all existing configs use 500–2000 ms.Intentionally left imperative
Tinybird, image-optimizer, proxy asset-route, creative-opportunities, and consent validators were not converted to attributes. Their logic interleaves
normalize()mutation, charset checks,enabled-gating, cross-field rules, and contextual per-item error messages (e.g. "slotXmust have positive width") with the one or two convertible checks. Converting them would fragment validation across#[validate]attrs andprepare_runtime/validate_runtime, cascadeValidatederives up the parent chains, and downgrade contextual errors to generic ones — a regression, not a win. (consentadditionally clamps rather than rejects, so arangeattribute would change its behavior.) That code isn't reimplementing a built-in; it does things built-ins can't express.Verification
cargo test -p trusted-server-core --lib→ 1631 pass, 0 fail (incl. new deploy-validation, GTM/Prebid regex, and timeout-range tests).cargo test --package trusted-server-cli … audit→ pass (ts auditstill patches the template).Settings::from_tomlon the template → pass (deploy rejection is separate from parse).cargo clippy -p trusted-server-core --libwith-D warnings→ clean.Not yet run: the full
clippy-fastly/ integration-parity CI gates — theclippy-fastlyalias hits a pre-existing local env error compiling the JS build script, unrelated to these changes.