Redact secrets from logs and downgrade PII log levels by ChristianPavilonis · Pull Request #468 · IABTechLab/trusted-server

ChristianPavilonis · 2026-03-10T01:52:26Z

Summary

Security fix: Secrets (proxy_secret, secret_key, handler passwords) were logged in plaintext at INFO/DEBUG level. Introduced a Redacted<T> wrapper that prints [REDACTED] in Debug/Display while preserving transparent serde and .expose() access.
Privacy fix: PII (client IP, user-agent) and full request/response payloads were logged at INFO/DEBUG. Downgraded PII logs to debug! and payload logs to trace! so they no longer appear in production.
Logger level: Changed production logger from Debug to Info to prevent verbose output in production by default.

Changes

File	Change
`crates/common/src/redacted.rs`	New `Redacted<T>` wrapper type with `[REDACTED]` Debug/Display, transparent serde, `.expose()`, and tests
`crates/common/src/lib.rs`	Added `mod redacted`
`crates/common/src/settings.rs`	Changed `proxy_secret`, `secret_key`, `username`, `password`, `secret_store_id` to `Redacted<String>`; updated validators
`crates/common/build.rs`	Added redacted module `#[path]` for build
`crates/common/src/auth.rs`	Use `.expose()` for credential comparison
`crates/common/src/http_util.rs`	Use `.expose()` for proxy_secret access
`crates/common/src/settings_data.rs`	Use `.expose()` for secret_key comparison
`crates/common/src/request_signing/endpoints.rs`	Use `.expose()` for secret_store_id
`crates/common/src/synthetic.rs`	Downgraded 5 PII log calls (client IP, user-agent, synthetic IDs) from `info!` to `debug!`
`crates/common/src/integrations/prebid.rs`	Downgraded payload logging from `debug!` to `trace!`
`crates/common/src/integrations/aps.rs`	Downgraded payload logging from `debug!` to `trace!`
`crates/common/src/integrations/adserver_mock.rs`	Downgraded payload logging from `debug!` to `trace!`
`crates/fastly/src/main.rs`	Logger level `Debug` → `Info`; settings log `info!` → `debug!`
`AGENTS.md`	Fixed incorrect `tracing` reference to `log`

Closes

Closes #404

Test plan

cargo test --workspace
cargo clippy --all-targets --all-features -- -D warnings
cargo fmt --all -- --check
JS tests: cd crates/js/lib && npx vitest run
JS format: cd crates/js/lib && npm run format
Docs format: cd docs && npm run format
WASM build: cargo build --bin trusted-server-fastly --release --target wasm32-wasip1
Manual testing via fastly compute serve

Checklist

Changes follow CLAUDE.md conventions
No unwrap() in production code — use expect("should ...")
Uses log macros (not println!)
New code has tests
No secrets or credentials committed

aram356

Summary

This PR introduces a Redacted<T> wrapper type for secrets, downgrades PII/payload log levels from info!/debug! to debug!/trace!, and hard-caps the production log level to Info. Good foundational work — two issues need addressing before merge.

Blocking

1. PII/payload leak at WARN level in Prebid error path
crates/common/src/integrations/prebid.rs lines 879-884

log::warn! logs up to 1000 chars of raw response body on non-2xx status. This bypasses the debug/trace downgrades and will emit in production with the current Info max level. If Prebid returns echoed request/debug blobs, this can include user/device data.

Suggestion: only log the status code at warn! and move the body preview to trace!.

2. Insecure-secret sentinel checks are inconsistent
crates/common/src/settings.rs line 200 + crates/common/src/settings_data.rs line 37

See inline comments. validate_secret_key rejects "secret_key" (underscore) while settings_data.rs rejects "secret-key" (hyphen) — two different strings guarding against the same mistake with split behavior.

Non-blocking

See inline comments for details on each:

ValidationError codes (settings.rs:199): new() takes a code, not a human message
Missing regression test (cross-cutting): no test asserting format!("{:?}", Settings) excludes secret literals across all secret-bearing fields — if a future field forgets Redacted<T>, there is no safety net
build.rs #[path] coupling (build.rs:9): fragile inclusion of redacted.rs; needs a guard comment
Test code still logs synthetic ID (synthetic.rs:266): log::info!("Generated synthetic ID: {}", synthetic_id) in test — minor inconsistency with PR goal
PII at debug level (synthetic.rs:99): template input with raw IP/UA downgraded to debug! but not trace! like other payload logs
Hard-capped log level (main.rs:177): follow-up to make max level configurable for incident debugging
Non-constant-time auth comparison (auth.rs:17): out of scope, follow-up for subtle::ConstantTimeEq

CI Status

All 10 checks PASS: cargo fmt, cargo test, vitest, CodeQL, format-docs, format-typescript, Analyze (actions, javascript-typescript x2, rust).

crates/common/src/settings.rs

crates/common/src/settings_data.rs

crates/common/build.rs

crates/common/src/synthetic.rs

crates/fastly/src/main.rs

crates/common/src/auth.rs

crates/common/src/redacted.rs

crates/common/src/integrations/prebid.rs

- Split Prebid warn log so status stays at warn! and body moves to trace! - Consolidate secret-key placeholder checks into validate_secret_key - Use proper ValidationError codes (snake_case identifiers) - Remove redundant InsecureSecretKey error variant - Downgrade synthetic template input log from debug! to trace! - Add build.rs #[path] guard comment to redacted.rs - Downgrade test log from info! to debug!

ChristianPavilonis · 2026-03-11T19:12:35Z

Review feedback addressed

All blocking and non-blocking items from the review have been resolved in 544d386:

Blocking fixes

1. PII/payload leak at WARN level in Prebid error path (prebid.rs:878-889)
Split the log: status code stays at warn!, response body preview moved to trace! behind a log_enabled! guard (avoids the from_utf8_lossy allocation in production).

2. Inconsistent secret sentinel checks (settings.rs + settings_data.rs + error.rs)
Consolidated both placeholder values ("secret_key" and "secret-key") into Synthetic::validate_secret_key. Removed the standalone check from settings_data.rs and the now-unused InsecureSecretKey error variant.

Non-blocking fixes

3. ValidationError codes (settings.rs:199-200)
Changed from human-readable messages to proper snake_case codes: "empty_secret_key", "placeholder_secret_key".

4. PII at debug level (synthetic.rs:99)
debug! → trace! for the template-rendered input containing raw IP/UA, consistent with the other payload downgrades in this PR.

5. Guard comment on redacted.rs (redacted.rs:1-2)
Added a comment noting the build.rs #[path] inclusion constraint so future contributors know to keep the file self-contained.

6. Test log level (synthetic.rs:266)
info! → debug! for the test log of the generated synthetic ID.

Deferred to follow-up issues

Make log level configurable via settings for incident/staging debugging #474 — Make log level configurable via settings for incident/staging debugging
Use constant-time comparison for basic auth password check #475 — Use constant-time comparison for basic auth password check
Add regression test asserting Settings debug output excludes secret values #476 — Add regression test asserting Settings debug output excludes secret values

All checks pass locally (fmt, clippy, 481 Rust tests, 239 JS tests, WASM build).

aram356

Summary

The Redacted<T> wrapper and log-level downgrades achieve the stated goal of preventing secrets and PII from leaking in production logs. One validation gap needs addressing before merge.

Blocking

🔧 wrench

proxy_secret missing validation: No #[validate] on Publisher.proxy_secret — an empty or placeholder value silently produces encryption with a known key (crates/common/src/settings.rs:27)

Non-blocking

🤔 thinking

Synthetic IDs at debug!: Template input moved to trace! but the IDs themselves remain at debug! — inconsistent PII treatment (crates/common/src/synthetic.rs:112,135,143,176, crates/common/src/publisher.rs:241)
secret_store_id as Redacted: This is a store identifier, not a secret. Adjacent config_store_id is plain String — inconsistency without clear benefit (crates/common/src/settings.rs:273)

🌱 seedling

Redacted missing PartialEq: Forces *handler.username.expose() == username pattern — could be simplified (crates/common/src/redacted.rs:28)

🏕 camp site

println! in test code: endpoints.rs test module uses println! (~10 instances) adjacent to the .expose() changes. CLAUDE.md says to use log macros.

CI Status

cargo fmt: PASS
cargo clippy: PASS
cargo test: PASS
vitest: PASS
format-docs: PASS
format-typescript: PASS
CodeQL: PASS

aram356 · 2026-03-14T20:23:32Z

crates/common/src/settings.rs

    /// Secret used to encrypt/decrypt proxied URLs in `/first-party/proxy`.
    /// Keep this secret stable to allow existing links to decode.
-    pub proxy_secret: String,
+    pub proxy_secret: Redacted<String>,


🔧 wrench — proxy_secret has no empty-value or placeholder validation.

Publisher.proxy_secret is wrapped in Redacted<String> but has no #[validate] annotation. secret_key validates against empty and placeholder values. Handler::username/password use validate_redacted_not_empty. But proxy_secret has zero validation.

Since Redacted<String> implements Default (empty string), an unconfigured deployment silently derives an encryption key from SHA-256("") in encode_url/decode_url — encryption with a known key. The default config placeholder "change-me-proxy-secret" is also not caught.

Fix: Add validation, e.g.:

#[validate(custom(function = validate_redacted_not_empty))] pub proxy_secret: Redacted<String>,

Or a dedicated validator that also catches the placeholder value.

aram356 · 2026-03-14T20:23:32Z

crates/common/src/synthetic.rs

    let synthetic_id = format!("{}.{}", hmac_hash, random_suffix);

-    log::info!("Generated fresh ID: {}", synthetic_id);
+    log::debug!("Generated fresh ID: {}", synthetic_id);


🤔 thinking — Synthetic IDs still logged at debug! level.

The PR moved template input (containing client IP + user-agent) to trace! on line 99, but the generated/existing synthetic IDs remain at debug! here and on lines 135, 143, 176, and in publisher.rs:241.

These are user-identifying pseudonymous identifiers. Safe at the new Info default, but if an operator bumps to Debug for troubleshooting, they leak. Inconsistent with the PR's own treatment of the template input.

aram356 · 2026-03-14T20:23:32Z

crates/common/src/settings.rs

    pub enabled: bool,
    pub config_store_id: String,
-    pub secret_store_id: String,
+    pub secret_store_id: Redacted<String>,


🤔 thinking — secret_store_id as Redacted is questionable.

This is a Fastly store identifier (like a table name), not a secret value. The adjacent config_store_id is left as a plain String, making the inconsistency visible. If this is intentional defense-in-depth, a doc comment explaining why would help. Otherwise it adds .expose() friction at both call sites in endpoints.rs (lines 152, 256) without security benefit.

aram356 · 2026-03-14T20:23:32Z

crates/common/src/redacted.rs

+/// assert_eq!(format!("{:?}", secret), "[REDACTED]");
+/// assert_eq!(secret.expose(), "my-secret-key");
+/// ```
+#[derive(Clone, Serialize, Deserialize)]


🌱 seedling — Redacted missing PartialEq.

Auth comparison in auth.rs:17 requires *handler.username.expose() == username. Implementing PartialEq (either Redacted<T> == Redacted<T> or adding an eq(&self, other: &T) -> bool method) would avoid exposing the inner value just for equality checks.

Redact secrets from logs and downgrade PII/payload log levels

5aae719

ChristianPavilonis self-assigned this Mar 10, 2026

ChristianPavilonis requested review from aram356 and prk-Jr March 10, 2026 15:20

Merge branch 'main' into fix/secrets-logged

a4ee384

aram356 requested changes Mar 11, 2026

View reviewed changes

This was referenced Mar 11, 2026

Make log level configurable via settings for incident/staging debugging #474

Open

Use constant-time comparison for basic auth password check #475

Open

Add regression test asserting Settings debug output excludes secret values #476

Open

ChristianPavilonis requested a review from aram356 March 11, 2026 19:36

aram356 requested changes Mar 14, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Redact secrets from logs and downgrade PII log levels#468

Redact secrets from logs and downgrade PII log levels#468
ChristianPavilonis wants to merge 3 commits intomainfrom
fix/secrets-logged

ChristianPavilonis commented Mar 10, 2026 •

edited

Loading

Uh oh!

aram356 left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ChristianPavilonis commented Mar 11, 2026 •

edited

Loading

Uh oh!

aram356 left a comment

Uh oh!

aram356 Mar 14, 2026

Uh oh!

aram356 Mar 14, 2026

Uh oh!

aram356 Mar 14, 2026

Uh oh!

aram356 Mar 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

ChristianPavilonis commented Mar 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Closes

Test plan

Checklist

Uh oh!

aram356 left a comment

Choose a reason for hiding this comment

Summary

Blocking

Non-blocking

CI Status

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ChristianPavilonis commented Mar 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review feedback addressed

Blocking fixes

Non-blocking fixes

Deferred to follow-up issues

Uh oh!

aram356 left a comment

Choose a reason for hiding this comment

Summary

Blocking

🔧 wrench

Non-blocking

🤔 thinking

🌱 seedling

🏕 camp site

CI Status

Uh oh!

aram356 Mar 14, 2026

Choose a reason for hiding this comment

Uh oh!

aram356 Mar 14, 2026

Choose a reason for hiding this comment

Uh oh!

aram356 Mar 14, 2026

Choose a reason for hiding this comment

Uh oh!

aram356 Mar 14, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ChristianPavilonis commented Mar 10, 2026 •

edited

Loading

ChristianPavilonis commented Mar 11, 2026 •

edited

Loading