Fix weak and inconsistent secret default validation

crates/common/build.rs

@@ -29,6 +29,11 @@ fn main() {
 
     // Merge base TOML with environment variable overrides and write output.
     // Panics if admin endpoints are not covered by a handler.
+    // Note: placeholder secret rejection is intentionally NOT done here.
+    // The base trusted-server.toml ships with placeholder secrets that
+    // production deployments override via TRUSTED_SERVER__* env vars at
+    // build time. Runtime startup (get_settings) rejects any remaining
+    // placeholders so a misconfigured deployment fails fast.
     let settings = settings::Settings::from_toml_and_env(&toml_content)
         .expect("Failed to parse settings at build time");
 

crates/common/src/error.rs

@@ -33,10 +33,11 @@ pub enum TrustedServerError {
     #[display("GDPR consent error: {message}")]
     GdprConsent { message: String },
 
-    /// The synthetic secret key is using the insecure default value.
-
-    #[display("Synthetic secret key is set to the default value - this is insecure")]
-    InsecureSecretKey,
+    /// A configuration secret is still set to a known placeholder value.
+    #[display(
+        "Configuration field '{field}' is set to a known placeholder value - this is insecure"
+    )]
+    InsecureDefault { field: String },
 
     /// Invalid UTF-8 data encountered.
     #[display("Invalid UTF-8 data: {message}")]
@@ -98,7 +99,7 @@ impl IntoHttpResponse for TrustedServerError {
             Self::Configuration { .. } | Self::Settings { .. } => StatusCode::INTERNAL_SERVER_ERROR,
             Self::Gam { .. } => StatusCode::BAD_GATEWAY,
             Self::GdprConsent { .. } => StatusCode::BAD_REQUEST,
-            Self::InsecureSecretKey => StatusCode::INTERNAL_SERVER_ERROR,
+            Self::InsecureDefault { .. } => StatusCode::INTERNAL_SERVER_ERROR,
             Self::InvalidHeaderValue { .. } => StatusCode::BAD_REQUEST,
             Self::InvalidUtf8 { .. } => StatusCode::BAD_REQUEST,
             Self::KvStore { .. } => StatusCode::SERVICE_UNAVAILABLE,

crates/common/src/settings.rs

@@ -23,10 +23,23 @@ pub struct Publisher {
     pub origin_url: String,
     /// Secret used to encrypt/decrypt proxied URLs in `/first-party/proxy`.
     /// Keep this secret stable to allow existing links to decode.
+    #[validate(length(min = 1))]
     pub proxy_secret: String,
 }
 
 impl Publisher {
+    /// Known placeholder values that must not be used in production.
+    pub const PROXY_SECRET_PLACEHOLDERS: &[&str] = &["change-me-proxy-secret"];
+
+    /// Returns `true` if `proxy_secret` matches a known placeholder value
+    /// (case-insensitive).
+    #[must_use]
+    pub fn is_placeholder_proxy_secret(proxy_secret: &str) -> bool {
+        Self::PROXY_SECRET_PLACEHOLDERS
+            .iter()
+            .any(|p| p.eq_ignore_ascii_case(proxy_secret))
+    }
+
     /// Extracts the host (including port if present) from the `origin_url`.
     ///
     /// # Examples
@@ -180,23 +193,23 @@ impl DerefMut for IntegrationSettings {
 pub struct Synthetic {
     pub counter_store: String,
     pub opid_store: String,
-    #[validate(length(min = 1), custom(function = Synthetic::validate_secret_key))]
+    #[validate(length(min = 1))]
     pub secret_key: String,
     #[validate(length(min = 1))]
     pub template: String,
 }
 
 impl Synthetic {
-    /// Validates that the secret key is not the placeholder value.
-    ///
-    /// # Errors
-    ///
-    /// Returns a validation error if the secret key is `"secret_key"` (the placeholder).
-    pub fn validate_secret_key(secret_key: &str) -> Result<(), ValidationError> {
-        match secret_key {
-            "secret_key" => Err(ValidationError::new("Secret key is not valid")),
-            _ => Ok(()),
-        }
+    /// Known placeholder values that must not be used in production.
+    pub const SECRET_KEY_PLACEHOLDERS: &[&str] = &["secret-key", "secret_key", "trusted-server"];
+
+    /// Returns `true` if `secret_key` matches a known placeholder value
+    /// (case-insensitive).
+    #[must_use]
+    pub fn is_placeholder_secret_key(secret_key: &str) -> bool {
+        Self::SECRET_KEY_PLACEHOLDERS
+            .iter()
+            .any(|p| p.eq_ignore_ascii_case(secret_key))
     }
 }
 
@@ -381,6 +394,33 @@ impl Settings {
         Ok(settings)
     }
 
+    /// Checks all secret fields for known placeholder values and returns an
+    /// error listing every offending field. This centralises the placeholder
+    /// policy so callers don't need to know which fields are secrets.
+    ///
+    /// # Errors
+    ///
+    /// Returns [`TrustedServerError::InsecureDefault`] when one or more secret
+    /// fields still contain a placeholder value.
+    pub fn reject_placeholder_secrets(&self) -> Result<(), Report<TrustedServerError>> {
+        let mut insecure_fields: Vec<&str> = Vec::new();
+
+        if Synthetic::is_placeholder_secret_key(&self.synthetic.secret_key) {
+            insecure_fields.push("synthetic.secret_key");
+        }
+        if Publisher::is_placeholder_proxy_secret(&self.publisher.proxy_secret) {
+            insecure_fields.push("publisher.proxy_secret");
+        }
+
+        if insecure_fields.is_empty() {
+            Ok(())
+        } else {
+            Err(Report::new(TrustedServerError::InsecureDefault {
+                field: insecure_fields.join(", "),
+            }))
+        }
+    }
+
     #[must_use]
     pub fn handler_for_path(&self, path: &str) -> Option<&Handler> {
         self.handlers
@@ -661,6 +701,7 @@ mod tests {
     #[test]
     fn test_settings_missing_required_fields() {
         let re = Regex::new(r"origin_url = .*").expect("regex should compile");
+
         let toml_str = crate_test_settings_str();
         let toml_str = re.replace(&toml_str, "");
 
@@ -671,6 +712,62 @@ mod tests {
         );
     }
 
+    #[test]
+    fn is_placeholder_secret_key_rejects_all_known_placeholders() {
+        for placeholder in Synthetic::SECRET_KEY_PLACEHOLDERS {
+            assert!(
+                Synthetic::is_placeholder_secret_key(placeholder),
+                "should detect placeholder secret_key '{placeholder}'"
+            );
+        }
+    }
+
+    #[test]
+    fn is_placeholder_secret_key_is_case_insensitive() {
+        assert!(
+            Synthetic::is_placeholder_secret_key("SECRET-KEY"),
+            "should detect case-insensitive placeholder secret_key"
+        );
+        assert!(
+            Synthetic::is_placeholder_secret_key("Trusted-Server"),
+            "should detect mixed-case placeholder secret_key"
+        );
+    }
+
+    #[test]
+    fn is_placeholder_secret_key_accepts_non_placeholder() {
+        assert!(
+            !Synthetic::is_placeholder_secret_key("test-secret-key"),
+            "should accept non-placeholder secret_key"
+        );
+    }
+
+    #[test]
+    fn is_placeholder_proxy_secret_rejects_all_known_placeholders() {
+        for placeholder in Publisher::PROXY_SECRET_PLACEHOLDERS {
+            assert!(
+                Publisher::is_placeholder_proxy_secret(placeholder),
+                "should detect placeholder proxy_secret '{placeholder}'"
+            );
+        }
+    }
+
+    #[test]
+    fn is_placeholder_proxy_secret_is_case_insensitive() {
+        assert!(
+            Publisher::is_placeholder_proxy_secret("CHANGE-ME-PROXY-SECRET"),
+            "should detect case-insensitive placeholder proxy_secret"
+        );
+    }
+
+    #[test]
+    fn is_placeholder_proxy_secret_accepts_non_placeholder() {
+        assert!(
+            !Publisher::is_placeholder_proxy_secret("unit-test-proxy-secret"),
+            "should accept non-placeholder proxy_secret"
+        );
+    }
+
     #[test]
     fn test_settings_empty_toml() {
         let toml_str = "";

crates/common/src/settings_data.rs

@@ -34,9 +34,8 @@ pub fn get_settings() -> Result<Settings, Report<TrustedServerError>> {
             message: "Failed to validate configuration".to_string(),
         })?;
 
-    if settings.synthetic.secret_key == "secret-key" {
-        return Err(Report::new(TrustedServerError::InsecureSecretKey));
-    }
+    // Reject known placeholder values for secrets that feed into cryptographic operations.
+    settings.reject_placeholder_secrets()?;
 
     if !settings.proxy.certificate_check {
         log::warn!(
@@ -49,22 +48,110 @@ pub fn get_settings() -> Result<Settings, Report<TrustedServerError>> {
 
 #[cfg(test)]
 mod tests {
-    use super::*;
+    use crate::error::TrustedServerError;
+    use crate::settings::Settings;
+    use crate::test_support::tests::crate_test_settings_str;
+
+    /// Builds a TOML string with the given secret values swapped in.
+    ///
+    /// # Panics
+    ///
+    /// Panics if the replacement patterns no longer match the test TOML,
+    /// which would cause the substitution to silently no-op.
+    fn toml_with_secrets(secret_key: &str, proxy_secret: &str) -> String {
+        let original = crate_test_settings_str();
+        let after_secret_key = original.replace(
+            r#"secret_key = "test-secret-key""#,
+            &format!(r#"secret_key = "{secret_key}""#),
+        );
+        assert_ne!(
+            after_secret_key, original,
+            "should have replaced secret_key value"
+        );
+        let result = after_secret_key.replace(
+            r#"proxy_secret = "unit-test-proxy-secret""#,
+            &format!(r#"proxy_secret = "{proxy_secret}""#),
+        );
+        assert_ne!(
+            result, after_secret_key,
+            "should have replaced proxy_secret value"
+        );
+        result
+    }
+
+    #[test]
+    fn rejects_placeholder_secret_key() {
+        let toml = toml_with_secrets("secret-key", "real-proxy-secret");
+        let settings = Settings::from_toml(&toml).expect("should parse TOML");
+        let err = settings
+            .reject_placeholder_secrets()
+            .expect_err("should reject placeholder secret_key");
+        let root = err.current_context();
+        assert!(
+            matches!(root, TrustedServerError::InsecureDefault { field } if field.contains("synthetic.secret_key")),
+            "error should mention synthetic.secret_key, got: {root}"
+        );
+    }
 
     #[test]
-    fn test_get_settings() {
-        // Test that Settings::new() loads successfully
-        let settings = get_settings();
-        assert!(settings.is_ok(), "Settings should load from embedded TOML");
-
-        let settings = settings.expect("should load settings from embedded TOML");
-        // Verify basic structure is loaded
-        assert!(!settings.publisher.domain.is_empty());
-        assert!(!settings.publisher.cookie_domain.is_empty());
-        assert!(!settings.publisher.origin_url.is_empty());
-        assert!(!settings.synthetic.counter_store.is_empty());
-        assert!(!settings.synthetic.opid_store.is_empty());
-        assert!(!settings.synthetic.secret_key.is_empty());
-        assert!(!settings.synthetic.template.is_empty());
+    fn rejects_placeholder_proxy_secret() {
+        let toml = toml_with_secrets("real-secret-key", "change-me-proxy-secret");
+        let settings = Settings::from_toml(&toml).expect("should parse TOML");
+        let err = settings
+            .reject_placeholder_secrets()
+            .expect_err("should reject placeholder proxy_secret");
+        let root = err.current_context();
+        assert!(
+            matches!(root, TrustedServerError::InsecureDefault { field } if field.contains("publisher.proxy_secret")),
+            "error should mention publisher.proxy_secret, got: {root}"
+        );
+    }
+
+    #[test]
+    fn rejects_both_placeholders_in_single_error() {
+        let toml = toml_with_secrets("secret_key", "change-me-proxy-secret");
+        let settings = Settings::from_toml(&toml).expect("should parse TOML");
+        let err = settings
+            .reject_placeholder_secrets()
+            .expect_err("should reject both placeholder secrets");
+        let root = err.current_context();
+        match root {
+            TrustedServerError::InsecureDefault { field } => {
+                assert!(
+                    field.contains("synthetic.secret_key"),
+                    "error should mention synthetic.secret_key, got: {field}"
+                );
+                assert!(
+                    field.contains("publisher.proxy_secret"),
+                    "error should mention publisher.proxy_secret, got: {field}"
+                );
+            }
+            other => panic!("expected InsecureDefault, got: {other}"),
+        }
+    }
+
+    #[test]
+    fn accepts_non_placeholder_secrets() {
+        let toml = toml_with_secrets("production-secret-key", "production-proxy-secret");
+        let settings = Settings::from_toml(&toml).expect("should parse TOML");
+        settings
+            .reject_placeholder_secrets()
+            .expect("non-placeholder secrets should pass validation");
+    }
+
+    /// Smoke-test the full `get_settings()` pipeline (embedded bytes → UTF-8 →
+    /// parse → validate → placeholder check).  The build-time TOML ships with
+    /// placeholder secrets, so the expected outcome is an [`InsecureDefault`]
+    /// error — but reaching that error proves every earlier stage succeeded.
+    #[test]
+    fn get_settings_rejects_embedded_placeholder_secrets() {
+        let err = super::get_settings().expect_err("should reject embedded placeholder secrets");
+        assert!(
+            matches!(
+                err.current_context(),
+                TrustedServerError::InsecureDefault { .. }
+            ),
+            "should fail with InsecureDefault, got: {err}"
+        );
     }
 }

docs/guide/configuration.md

@@ -244,6 +244,7 @@ TRUSTED_SERVER__PUBLISHER__PROXY_SECRET=your-secret-here
 **Security**:
 
 - Keep confidential and secure
+- Cannot be the placeholder `"change-me-proxy-secret"` (case-insensitive) — startup will fail
 - Rotate periodically (90 days recommended)
 - Use cryptographically random values (32+ bytes)
 - Never commit to version control
@@ -359,7 +360,7 @@ fastly kv-store create --name=opid_store
 **Security**:
 
 - Must be non-empty
-- Cannot be `"secret_key"` or `"secret-key"` (reserved/invalid)
+- Cannot be a known placeholder: `"secret-key"`, `"secret_key"`, or `"trusted-server"` (case-insensitive)
 - Rotate periodically for security
 - Store securely (environment variable recommended)
 
@@ -373,7 +374,7 @@ openssl rand -hex 32
 **Validation**: Application startup fails if:
 
 - Empty string
-- Exactly `"secret_key"` or `"secret-key"` (default placeholders)
+- Exactly `"secret-key"`, `"secret_key"`, or `"trusted-server"` (known placeholders, case-insensitive)
 
 #### `template`
 
@@ -922,11 +923,12 @@ Configuration is validated at startup:
 
 - All fields non-empty
 - `origin_url` is valid URL
+- `proxy_secret` ≠ known placeholder (`"change-me-proxy-secret"` — case-insensitive)
 
 **Synthetic Validation**:
 
 - `secret_key` ≥ 1 character
-- `secret_key` ≠ `"secret-key"`
+- `secret_key` ≠ known placeholders (`"secret-key"`, `"secret_key"`, `"trusted-server"` — case-insensitive)
 - `template` non-empty
 
 **Handler Validation**:
@@ -1036,11 +1038,12 @@ trusted-server.dev.toml      # Development overrides
 - Verify all required fields present
 - Check environment variable format
 
-**"Secret key is not valid"**:
+**"Configuration field '...' is set to a known placeholder value"**:
 
-- Cannot use `"secret-key"` (placeholder)
+- `synthetic.secret_key` cannot be `"secret-key"`, `"secret_key"`, or `"trusted-server"` (case-insensitive)
+- `publisher.proxy_secret` cannot be `"change-me-proxy-secret"` (case-insensitive)
 - Must be non-empty
-- Change to secure random value
+- Change to a secure random value (see generation commands above)
 
 **"Invalid regex"**: