Skip to content

Add regression test asserting Settings debug output excludes secret values - #1181

Open
dhruv8sh wants to merge 4 commits into
mainfrom
test/settings-debug-output-redacts-secrets
Open

dhruv8sh wants to merge 4 commits into
mainfrom
test/settings-debug-output-redacts-secrets

Conversation

@dhruv8sh

@dhruv8sh dhruv8sh commented Sep 18, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

  • Add a test that formats a fully-populated Settings with a distinct canary in every Redacted<String> secret field and asserts none of them survive Debug formatting, plus a canary pinning the hand-written IntegrationSettings Debug impl that keeps resolved integration secrets (e.g. DataDome's server-side key) out of the same output.
  • Locks in the invariant that a new secret-bearing field added directly to Settings without the Redacted<T> wrapper will print in cleartext. Two other tests already cover Redacted debug behavior outside the Settings tree (s3_sigv4.rs::credentials_debug_redacts_secret_material, ec/registry.rs::partner_debug_output_redacts_pull_token); this is the first test covering the fields reachable through Settings itself, plus the IntegrationSettings boundary.

Changes

File Change
crates/trusted-server-core/src/settings.rs Add settings_debug_output_redacts_every_secret_field, covering publisher.proxy_secret, ec.passphrase, handlers[].username/password, ec.partners[].api_token/ts_pull_token, trusted_client_ip.shared_secret, proxy.asset_routes[].auth (S3 access_key_id/secret_access_key/session_token), tinybird.auction_token_secret/access_token_secret, and integrations.datadome.server_side_key_secret_name (via the IntegrationSettings opaque-JSON boundary)

Closes

Closes #476

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: manually confirmed the test can fail — temporarily made Redacted<T>'s Debug impl print the inner value, reran the test to confirm all 13 canaries fired, then reverted. Separately confirmed the DataDome canary specifically: swapping IntegrationSettings's hand-written Debug impl for a derive makes the test fail, then reverted.

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

…alues

Signed-off-by: dhruv8sh <dhruv8sh@proton.me>

@ChristianPavilonis ChristianPavilonis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

The current 12 Redacted<String> fields reachable through Settings are covered, and the focused redaction tests pass against the PR revision. I found one non-blocking coverage gap and noted it inline.

Comment thread crates/trusted-server-core/src/settings.rs

@prk-Jr prk-Jr left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Reviewed commit 16437172621a48efc1083bb4e92cd962e4ebc8b5.

The fixture covers all 12 current Redacted<String> fields reachable through Settings debug formatting with distinct canaries and field-labelled assertions. Integration configuration values are hidden by the existing IDs-only Debug implementation. No present-day redaction defect found.

The focused redaction test passed locally. This guards the enumerated current fields; it does not automatically detect every future secret field added through defaulted helper construction. That previously discussed limitation does not invalidate the current-field canary coverage.

General adapter/build/lint gates rely on remote CI.

CI Status

@aram356 aram356 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Adds a single test that plants a distinct canary in each Redacted<String> field reachable from Settings and asserts none survives Debug formatting. The test is correct and every assertion is load-bearing, but its own framing claims more than it delivers: it does not catch a new secret field added without the Redacted wrapper, and it skips the one redaction boundary in Settings that is genuinely fragile.

1 of the inline comments below carries a one-click GitHub suggestion — use Commit suggestion to apply it as a commit on the PR branch. The remaining comments describe the fix in prose because the change spans several non-contiguous ranges in the file and can't be auto-applied.

Verification performed

  • Mutation-tested all 12 canaries. Replaced Redacted's Debug with one that still prints [REDACTED] but appends the inner value. All 12 assertions fired; none is vacuous.
  • Independently enumerated every Redacted<String> reachable from Settings. The 12 covered are complete. s3_sigv4.rs:42,44 and ec/registry.rs:49 are runtime types not reachable from Settings and are correctly out of scope.
  • Confirmed all five struct literals are exhaustive and that every struct on the traversal path uses a derived Debug.
  • Test passes; cargo fmt --check and cargo clippy-fastly clean.

Blocking

🔧 wrench

  • Test skips the only fragile redaction boundary in Settings — see inline at crates/trusted-server-core/src/settings.rs:3893
  • Leading comment claims an enforcement the test cannot provide — see inline at crates/trusted-server-core/src/settings.rs:3809

❓ question

  • handlers[].username is Redacted but absent from secret_fields() — see inline at crates/trusted-server-core/src/settings.rs:3843

Cross-cutting / body-level findings

  • 📝 PR description wording — The description says "previously only one field (trusted_client_ip.shared_secret) had this coverage." That holds when read as "only one field of Settings", but two other tests already assert Redacted debug behaviour on types outside the Settings tree: crates/trusted-server-core/src/s3_sigv4.rs:314-330 (credentials_debug_redacts_secret_material) and crates/trusted-server-core/src/ec/registry.rs:612-634 (partner_debug_output_redacts_pull_token). Neither is made redundant by this PR, and neither should be deleted.

    Worth borrowing from s3_sigv4.rs:314: it asserts redaction is selective — assert!(debug_output.contains("AKIAIOSFODNN7EXAMPLE"), "should leave non-secret access key ID visible"). This PR only asserts secrets are absent, so a Debug impl that blanket-suppressed every field would pass it. A positive assertion that a known non-secret value (for example handlers[].path or tinybird.api_host) is still visible would close that hole cheaply.

    Also note crates/trusted-server-core/src/settings.rs:3784-3807 (trusted_client_ip_parses_and_redacts_shared_secret_in_debug_output) covers the TOML deserialization path via Settings::from_toml, whereas this test constructs structs directly and never exercises serde. Keep both.

CI Status

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

Comment thread crates/trusted-server-core/src/settings.rs Outdated
Comment thread crates/trusted-server-core/src/settings.rs
Comment thread crates/trusted-server-core/src/settings.rs
…ment claims

Adds a canary that pins IntegrationSettings hand-written Debug impl, which is the only thing keeping resolved DataDome credentials out of Settings debug output. Rewrites the leading comment to state what the canary list actually guarantees instead of an enforcement it cannot provide, and adds a positive assertion that a non-secret field stays visible so a blanket-redacting Debug impl would not pass unnoticed.

Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
@dhruv8sh
dhruv8sh requested a review from aram356 September 21, 2026 19:14

@aram356 aram356 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

All three findings from the previous pass are resolved. Re-reviewed at b70cb57a against main.

Verification performed

  • Mutation-tested the new DataDome canary. Replaced IntegrationSettings's hand-written Debug with #[derive(Debug)] in a scratch worktree; the test now fails with should redact integrations.datadome.server_side_key_secret_name. The boundary is genuinely pinned.
  • Mutation-tested the new selective-redaction assertion. Gave Handler a blanket-suppressing Debug impl; the test fails with should leave non-secret handler path visible. It catches over-redaction, not just under-redaction.
  • Re-ran the full canary leak mutation. 12 of the 13 canaries leak when Redacted's Debug is made to print its inner value. The DataDome canary correctly does not, because it is raw JSON guarded by IntegrationSettings::fmt rather than by Redacted — two distinct mechanisms, each pinned by its own canary.
  • Local gate: cargo fmt --all -- --check clean, cargo clippy-fastly clean, cargo test-fastly 2,879 passed, cargo test-axum 41 passed.

Resolution of prior findings

  • IntegrationSettings debug boundary — fixed. Canary applied and independently mutation-verified.
  • Leading comment overstated its guarantee — fixed. The comment now states this is a regression guard over the listed fields, not a completeness guarantee, and says plainly that a new unwrapped secret field will pass while leaking.
  • handlers[].username vs secret_fields() — answered, no change needed. The username is a login identifier wrapped for defense-in-depth alongside the password, matching the existing treatment of proxy.asset_routes[].auth.access_key_id at settings.rs:883, which is also Redacted without being provisioned through the secret store. secret_fields() staying password-only is consistent with that.

Non-blocking

📝 note

  • Second DataDome secret leaf has no canary — see inline at crates/trusted-server-core/src/settings.rs:3908

CI Status

All 20 checks PASS on b70cb57a:

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

"datadome",
&json!({
"enabled": true,
"server_side_key_secret_name": CANARY_DATADOME_SERVER_SIDE_KEY,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📝 note — crates/trusted-server-core/src/config.rs:196-199 declares a second DataDome secret leaf, integrations.datadome.protection_test_bypass.credential_secret_name, which has no canary here.

I checked whether that matters and it does not, today: both leaves sit behind the same IntegrationSettings::fmt boundary (settings.rs:224-233), so the server_side_key_secret_name canary already pins the impl that protects both. Adding a nested protection_test_bypass.credential_secret_name canary and re-running confirmed it never appears in the debug output for the same reason the first one doesn't.

No change requested. Worth knowing only if that Debug impl is ever split per-integration or made field-selective, at which point the two leaves would stop sharing a guard and this canary would no longer cover the bypass credential.

@dhruv8sh
dhruv8sh changed the base branch from main to rc/202609 September 29, 2026 11:39
@dhruv8sh
dhruv8sh added this pull request to stack #1218 September 29, 2026 11:40
@dhruv8sh
dhruv8sh removed this pull request from stack #1218 September 29, 2026 11:51
dhruv8sh added a commit that referenced this pull request Sep 29, 2026
Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
@dhruv8sh

dhruv8sh commented Oct 1, 2026

Copy link
Copy Markdown
Collaborator Author

Squash-merged into rc/202609 as 799ceaa

@dhruv8sh dhruv8sh closed this Oct 1, 2026
@dhruv8sh dhruv8sh added this to the 202609 milestone Oct 1, 2026
@dhruv8sh dhruv8sh reopened this Oct 1, 2026
@dhruv8sh
dhruv8sh changed the base branch from rc/202609 to main October 1, 2026 10:38

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add regression test asserting Settings debug output excludes secret values

4 participants