Document the config template and harden config validation#870
Conversation
aram356
force-pushed
the
worktree-config-audit
branch
from
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.
aram356
force-pushed
the
worktree-config-audit
branch
from
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`.
aram356
changed the title
aram356
requested review from
ChristianPavilonis and
prk-Jr
and removed request for
ChristianPavilonis
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
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.
|
|
||
| /// APS publisher ID (accepts both string and integer from config) | ||
| #[serde(deserialize_with = "deserialize_pub_id")] | ||
| #[validate(length(min = 1), custom(function = validate_aps_pub_id))] |
There was a problem hiding this comment.
P1 — APS validation runs even when the integration resolves to disabled
enabled defaults to false, but IntegrationSettings::get_typed only short-circuits raw configs that explicitly contain enabled = false. Otherwise it calls config.validate() before checking config.is_enabled(). Consequently, an APS section that omits enabled but contains this documented placeholder now fails ts config validate even though APS resolves to disabled. This is a backward-incompatible config regression and can prevent application-state construction after an upgrade.
Please check the resolved is_enabled() value before field validation, while retaining the explicit-false fast path, and add regression coverage for APS and adserver_mock sections with omitted enabled.
|
|
||
| /// APS publisher ID (accepts both string and integer from config) | ||
| #[serde(deserialize_with = "deserialize_pub_id")] | ||
| #[validate(length(min = 1), custom(function = validate_aps_pub_id))] |
There was a problem hiding this comment.
P2 — Whitespace-only APS publisher IDs pass the new non-empty check
length(min = 1) counts whitespace, while validate_aps_pub_id trims only for comparison with the reserved placeholder and never rejects an empty trimmed value. I confirmed that enabled APS with pub_id = " " passes ts config validate, leaving the invalid value to fail upstream.
Please reject pub_id.trim().is_empty() in the custom validator and test both empty and whitespace-only values through enabled deploy validation.
| insecure_fields.push("publisher.origin_url".to_owned()); | ||
| } | ||
| if let Some(request_signing) = &self.request_signing { | ||
| if request_signing.enabled { |
There was a problem hiding this comment.
P2 — Disabled request-signing placeholders remain usable by live key-management routes
The placeholder IDs are rejected only when request_signing.enabled is true. However, Fastly registers the rotate/deactivate admin routes unconditionally, and signing_store_ids consumes these IDs without checking enabled. I confirmed that a disabled request-signing block with both documented placeholders passes ts config validate, so authenticated key rotation still reaches invalid management-store IDs and fails later.
Please either reject placeholder/empty IDs whenever [request_signing] is present, or gate the key-management routes on enabled and document that rotation is unavailable while disabled.
| # [integrations.sourcepoint] | ||
| # enabled = true | ||
| # rewrite_sdk = true | ||
| # cdn_origin = "https://cdn.example.com" |
There was a problem hiding this comment.
P2 — The documented Sourcepoint block cannot validate as written
SourcepointConfig requires cdn_origin to use exactly cdn.privacy-mgmt.com. Uncommenting this documented block therefore makes ts config validate fail with cdn_origin host must be cdn.privacy-mgmt.com; unlike deliberate invalid placeholders elsewhere, this override is neither required nor identified as invalid.
Please omit cdn_origin from the example so the valid pinned default is used, and document that the origin is fixed rather than operator-selectable.
prk-Jr
left a comment
prk-Jr
left a comment
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
|
|
||
| /// APS publisher ID (accepts both string and integer from config) | ||
| #[serde(deserialize_with = "deserialize_pub_id")] | ||
| #[validate(length(min = 1), custom(function = validate_aps_pub_id))] |
There was a problem hiding this comment.
🔧 wrench — A whitespace-only pub_id still passes deploy validation. length(min = 1) counts the spaces, while validate_aps_pub_id trims only for the placeholder comparison. An enabled APS integration can therefore start with an unusable publisher ID.
Please reject pub_id.trim().is_empty() in the custom validator and add a whitespace-only regression test.
3 participants
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.