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)

[aram356]

🔧 wrench — The original test_get_settings was the only test exercising the full get_settings() end-to-end (embedded bytes → UTF-8 → parse → validate → placeholder check). The new tests only call Settings::from_toml() + reject_placeholder_secrets() directly.

Consider adding back a smoke test:

#[test]
fn get_settings_loads_embedded_config() {
    let settings = super::get_settings();
    assert!(settings.is_ok(), "should load settings from embedded TOML");
}

[aram356]

❓ question — str::replace() returns the original string unchanged if the pattern doesn't match. If crate_test_settings_str() ever changes quoting or default values, these replacements silently no-op.

The accepts_non_placeholder_secrets test is especially vulnerable: it replaces "test-secret-key" with "production-secret-key" — both non-placeholders — so even if replacement fails, the test passes.

Consider asserting the replacement actually took effect:

fn toml_with_secrets(secret_key: &str, proxy_secret: &str) -> String {
    let original = crate_test_settings_str();
    let result = original
        .replace(/* ... */)
        .replace(/* ... */);
    assert_ne!(result, original, "should have replaced at least one secret value");
    result
}

[aram356]

🤔 thinking — Placeholder matching is case-sensitive. Someone setting secret_key = "Secret-Key" bypasses the check while still using a predictable value. Consider case-insensitive matching:

pub fn is_placeholder_secret_key(secret_key: &str) -> bool {
    Self::SECRET_KEY_PLACEHOLDERS
        .iter()
        .any(|p| p.eq_ignore_ascii_case(secret_key))
}

[aram356]

⛏ nitpick — Double space after period: "field. This" → "field. This"