Fix weak and inconsistent secret default validation

[ChristianPavilonis]

Summary

• Reject all known placeholder values for synthetic.secret_key ("secret-key", "secret_key", "trusted-server") and publisher.proxy_secret ("change-me-proxy-secret") at runtime startup so misconfigured deployments fail fast.
• Consolidate validation into predicate methods (is_placeholder_secret_key, is_placeholder_proxy_secret) with explicit placeholder lists, replacing the inconsistent checks that missed the actual TOML defaults.
• Improve error reporting by renaming InsecureSecretKey to InsecureDefault { field } so the error message identifies which secret triggered rejection.

Changes

File	Change
crates/common/src/error.rs	Renamed InsecureSecretKey → InsecureDefault { field: String } with a descriptive display message
crates/common/src/settings.rs	Added PROXY_SECRET_PLACEHOLDERS const + is_placeholder_proxy_secret() on Publisher; replaced validate_secret_key() with SECRET_KEY_PLACEHOLDERS const + is_placeholder_secret_key() on Synthetic; added 4 unit tests
crates/common/src/settings_data.rs	Replaced single == "secret-key" check with calls to both predicates; returns field-specific InsecureDefault errors; updated test to assert placeholder rejection
crates/common/build.rs	Added comment explaining why placeholder rejection is intentionally skipped at build time

Closes

Closes #406

Test plan

• cargo test --workspace
• cargo clippy --all-targets --all-features -- -D warnings
• cargo fmt --all -- --check

Checklist

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

[aram356]

Review Summary

#	Severity	File	Finding
1	HIGH	settings.rs	proxy_secret has no validation — empty string passes and produces a deterministic crypto key
2	MEDIUM	settings_data.rs	Test is environment-dependent and can flap when CI overrides secrets
3	MEDIUM	settings.rs	Placeholder policy is scattered across two types and will drift
4	LOW	settings_data.rs	Only the first insecure field is reported; operator must fix-and-redeploy to find the second
5	NIT	settings.rs	Spurious #[allow(dead_code)] on public items that are actually used

[ChristianPavilonis]

Review Feedback Addressed

All 5 items from @aram356's review have been addressed in cbef697:

#	Severity	Finding	Resolution
1	HIGH	proxy_secret has no validation — empty string produces deterministic crypto key	Added #[validate(length(min = 1))] to proxy_secret field
2	MEDIUM	Test is environment-dependent and can flap in CI	Rewrote tests to use explicit TOML input with crate_test_settings_str() + string replacements — fully deterministic
3	MEDIUM	Placeholder policy scattered across two types	Centralized into Settings::reject_placeholder_secrets() — single source of truth for all secret placeholder checks
4	LOW	Only first insecure field reported	reject_placeholder_secrets() collects all violations and reports them in a single error
5	NIT	Spurious #[allow(dead_code)] on public items	Removed all 4 unnecessary annotations

All 480 tests pass, cargo fmt and cargo clippy clean.

[aram356]

Summary

Centralizes placeholder-secret rejection into Settings::reject_placeholder_secrets(), adds proxy_secret validation, and makes tests deterministic. Addresses all findings from the previous review round.

Blocking

🔧 wrench

• Removed test_get_settings smoke test not replaced: The original test_get_settings was the only test exercising the full get_settings() end-to-end path (embedded bytes → UTF-8 → parse → validate → placeholder check). The new tests only exercise Settings::from_toml() + reject_placeholder_secrets() directly. A regression in the glue code would go undetected. (settings_data.rs)

❓ question

• toml_with_secrets replacement is fragile and can silently no-op: str::replace() returns the original string unchanged if the pattern doesn't match. If crate_test_settings_str() changes quoting or default values, replacements silently do nothing. The accepts_non_placeholder_secrets test is especially vulnerable — both old and new values are non-placeholders, so it passes even if replacement fails. (settings_data.rs:56-66)

Non-blocking

🤔 thinking

• Case-sensitive placeholder matching: "Secret-Key" or "SECRET-KEY" bypasses the check while still being predictable. Consider eq_ignore_ascii_case. (settings.rs:205-207)

🌱 seedling

• Minimum length for crypto secrets: min = 1 prevents empty but "x" still passes validation then feeds into Sha256::digest for XChaCha20-Poly1305 key derivation. Consider min = 16 as a follow-up. (settings.rs:27, 194)

⛏ nitpick

• Double space in doc comment: "...offending field. This centralises..." — rest of codebase uses single space. (settings.rs:388)

CI Status

• fmt: PASS
• clippy: PASS
• rust tests: PASS
• js tests: PASS
• CodeQL: FAIL (appears infrastructure-related, not caused by this PR)

[prk-Jr]

Summary

The runtime secret-validation change is directionally right, but I found one blocking regression: the new Settings::reject_placeholder_secrets() helper now makes the branch fail CodeQL with 6 new high-severity cleartext-logging alerts, and one of those sinks is a real full-settings debug log. I also have a couple of API/test-coverage concerns and one docs consistency issue.

Blocking

🔧 wrench

• Secret-taint regression now fails CodeQL: Adding secret-aware validation as a method on Settings taints the whole struct, and the branch now fails CodeQL with 6 new high-severity cleartext-logging alerts. One concrete sink is the full settings debug log in crates/fastly/src/main.rs:42.

Non-blocking

📝 note

• Docs/examples still describe the old placeholder rules: the implementation now rejects synthetic.secret_key = "trusted-server" and publisher.proxy_secret = "change-me-proxy-secret", but the docs still describe only "secret_key" / "secret-key" as invalid and still show placeholder-style proxy-secret examples. See docs/guide/configuration.md:361, docs/guide/configuration.md:922, docs/guide/configuration.md:1033, and docs/guide/error-reference.md:64.

CI Status

• cargo fmt: PASS
• cargo test: PASS
• vitest: PASS
• CodeQL: FAIL (6 new high-severity alerts)