Conversation
…alues Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
ChristianPavilonis
left a comment
There was a problem hiding this comment.
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.
prk-Jr
left a comment
There was a problem hiding this comment.
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
- 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)
- Analyze (javascript-typescript): PASS
aram356
left a comment
There was a problem hiding this comment.
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'sDebugwith one that still prints[REDACTED]but appends the inner value. All 12 assertions fired; none is vacuous. - Independently enumerated every
Redacted<String>reachable fromSettings. The 12 covered are complete.s3_sigv4.rs:42,44andec/registry.rs:49are runtime types not reachable fromSettingsand 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 --checkandcargo clippy-fastlyclean.
Blocking
🔧 wrench
- Test skips the only fragile redaction boundary in
Settings— see inline atcrates/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[].usernameisRedactedbut absent fromsecret_fields()— see inline atcrates/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 ofSettings", but two other tests already assertRedacteddebug behaviour on types outside theSettingstree:crates/trusted-server-core/src/s3_sigv4.rs:314-330(credentials_debug_redacts_secret_material) andcrates/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 aDebugimpl that blanket-suppressed every field would pass it. A positive assertion that a known non-secret value (for examplehandlers[].pathortinybird.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 viaSettings::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)
…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>
aram356
left a comment
There was a problem hiding this comment.
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-writtenDebugwith#[derive(Debug)]in a scratch worktree; the test now fails withshould redact integrations.datadome.server_side_key_secret_name. The boundary is genuinely pinned. - Mutation-tested the new selective-redaction assertion. Gave
Handlera blanket-suppressingDebugimpl; the test fails withshould 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'sDebugis made to print its inner value. The DataDome canary correctly does not, because it is raw JSON guarded byIntegrationSettings::fmtrather than byRedacted— two distinct mechanisms, each pinned by its own canary. - Local gate:
cargo fmt --all -- --checkclean,cargo clippy-fastlyclean,cargo test-fastly2,879 passed,cargo test-axum41 passed.
Resolution of prior findings
IntegrationSettingsdebug 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[].usernamevssecret_fields()— answered, no change needed. The username is a login identifier wrapped for defense-in-depth alongside the password, matching the existing treatment ofproxy.asset_routes[].auth.access_key_idatsettings.rs:883, which is alsoRedactedwithout 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, |
There was a problem hiding this comment.
📝 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.
Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
|
Squash-merged into |
Summary
Settingswith a distinct canary in everyRedacted<String>secret field and asserts none of them surviveDebugformatting, plus a canary pinning the hand-writtenIntegrationSettingsDebugimpl that keeps resolved integration secrets (e.g. DataDome's server-side key) out of the same output.Settingswithout theRedacted<T>wrapper will print in cleartext. Two other tests already coverRedacteddebug behavior outside theSettingstree (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 throughSettingsitself, plus theIntegrationSettingsboundary.Changes
crates/trusted-server-core/src/settings.rssettings_debug_output_redacts_every_secret_field, coveringpublisher.proxy_secret,ec.passphrase,handlers[].username/password,ec.partners[].api_token/ts_pull_token,trusted_client_ip.shared_secret,proxy.asset_routes[].auth(S3access_key_id/secret_access_key/session_token),tinybird.auction_token_secret/access_token_secret, andintegrations.datadome.server_side_key_secret_name(via theIntegrationSettingsopaque-JSON boundary)Closes
Closes #476
Test plan
cargo test-fastly && cargo test-axumcargo clippy-fastly && cargo clippy-axumcargo fmt --all -- --checkcd crates/trusted-server-js/lib && npx vitest runcd crates/trusted-server-js/lib && npm run formatcd docs && npm run formatcargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1fastly compute serveRedacted<T>'sDebugimpl print the inner value, reran the test to confirm all 13 canaries fired, then reverted. Separately confirmed the DataDome canary specifically: swappingIntegrationSettings's hand-writtenDebugimpl for a derive makes the test fail, then reverted.Checklist
unwrap()in production code — useexpect("should ...")tracingmacros (notprintln!)