security(homecore-migrate): redact secret value from malformed secrets.yaml error (#1089)

* fix(homecore-migrate): redact secret value from malformed secrets.yaml error (secret-leak)

`read_secrets` wrapped serde_yaml's parse error into `MigrateError::YamlParse {
source }`. serde_yaml's message for a typed-tag coercion failure embeds the
offending scalar verbatim, e.g. `invalid value: string "<the-secret-value>"`.
That error propagates out of `read_secrets`, is `?`-returned by the
`InspectSecrets` CLI path in main.rs, and printed to stderr by anyhow — leaking
a secret value despite the CLI's deliberate `<redacted>` design.

Fix: secrets.yaml parse failures now map to a new redacting variant
`MigrateError::SecretsParse { path, line, column }` that carries only the file
path and a coarse location (from `serde_yaml::Error::location()`), never the
scalar content. Other (non-secret) YAML files keep `YamlParse`.

Pinned by `secrets::tests::malformed_secrets_error_never_contains_secret_value`
(asserts the rendered error AND its full #[source] chain never contain the
secret value; fails on the old `YamlParse` path) plus
`malformed_secrets_error_reports_location` (still fail-closed + locatable).

ADR-165 secret-handling rule: a secret value must never appear in output.

Co-Authored-By: claude-flow <ruv@ruv.net>

* docs(homecore-migrate): record secret-leak fix in ADR-165 + CHANGELOG

Note the secrets.yaml error-redaction fix and the review's clean dimensions
(read-only source / no traversal / no panic / fail-closed versioning / no
injection) in ADR-165 §2.4, bump the test-evidence count 19→21 in §2.6, and add
an [Unreleased] Security entry to CHANGELOG.

Co-Authored-By: claude-flow <ruv@ruv.net>

docs/adr/ADR-165-homecore-migrate-from-home-assistant.md

@@ -78,6 +78,23 @@ converts the entity registry; full conversion of the remaining artifacts is defe
 
 - `MigrateError` carries context (`path`, line/field) for I/O, JSON, YAML, missing-field,
   unsupported-schema-version, and entity-id parse failures (`src/lib.rs`).
+- **Secret-leak hardening (security review, 2026-06).** `secrets.yaml` parse failures must
+  NOT use the generic `MigrateError::YamlParse { source }` variant: `serde_yaml`'s message
+  for a typed-tag coercion error (e.g. `port: !!int <value>`) embeds the offending scalar
+  verbatim (`invalid value: string "<the-secret-value>"`), and that error propagates through
+  the `InspectSecrets` CLI path to stderr — leaking a secret value despite the CLI's
+  deliberate `<redacted>` design. `read_secrets` now maps such failures to a dedicated
+  redacting variant `MigrateError::SecretsParse { path, line, column }` that carries only the
+  file path and a coarse location (`serde_yaml::Error::location()`), never the scalar content.
+  Pinned by `secrets::tests::malformed_secrets_error_never_contains_secret_value` (asserts the
+  rendered error **and its full `#[source]` chain** never contain the secret value).
+  **Review dimensions confirmed clean with evidence:** source is never mutated (no
+  `fs::write`/`remove`/`create` anywhere — P1 reads source, writes nothing); paths are
+  user-supplied dirs joined with fixed filenames (no `..`/absolute traversal beyond the
+  user's own privileges); malformed/typed/truncated `.storage` JSON and YAML **error, never
+  panic** (every production `unwrap`/`expect` is test-only); unknown schema `minor_version`
+  hard-errors fail-closed; no SQL/shell/path injection surface (the tool emits diagnostics
+  only, persists nothing in P1).
 
 ### 2.5 Deferred to P2+ (NOT built — honestly labelled)
 
@@ -89,7 +106,9 @@ converts the entity registry; full conversion of the remaining artifacts is defe
 
 ### 2.6 Test evidence (as shipped)
 
-- 19 tests (`cargo test -p homecore-migrate`), per the crate README badge.
+- 21 tests (`cargo test -p homecore-migrate`) — 19 as originally shipped plus 2 added by the
+  2026-06 security review (`secrets::tests::malformed_secrets_error_never_contains_secret_value`,
+  `malformed_secrets_error_reports_location`).
 
 ## 3. Consequences
 

v2/crates/homecore-migrate/src/lib.rs

@@ -55,6 +55,25 @@ pub enum MigrateError {
         source: serde_yaml::Error,
     },
 
+    /// Parse failure in a SECRET-bearing file (`secrets.yaml`).
+    ///
+    /// Unlike [`MigrateError::YamlParse`], this variant deliberately does NOT
+    /// embed the underlying `serde_yaml::Error` message — that message can quote
+    /// the offending scalar verbatim (e.g. a typed-tag coercion error renders
+    /// `invalid value: string "<the-secret-value>"`), which would leak a secret
+    /// into stderr/logs. We carry only the file path plus a coarse line/column
+    /// so the user can locate the problem without the value being printed.
+    /// (ADR-165 secret-handling rule: a secret value must never appear in output.)
+    #[error(
+        "secrets.yaml parse error in {path} (line {line}, column {column}): \
+         malformed YAML (value content redacted)"
+    )]
+    SecretsParse {
+        path: String,
+        line: usize,
+        column: usize,
+    },
+
     /// Fired when the outer `{version, minor_version}` envelope version is
     /// known but the `minor_version` is not supported by any compiled parser.
     /// Per ADR-165 §6 Q5: hard error on unknown minor_version.

v2/crates/homecore-migrate/src/secrets.rs

@@ -33,11 +33,19 @@ pub fn read_secrets(path: &Path) -> Result<HashMap<String, String>, MigrateError
         return Ok(HashMap::new());
     }
 
-    let parsed: serde_yaml::Value =
-        serde_yaml::from_str(&raw).map_err(|e| MigrateError::YamlParse {
+    // SECURITY: do NOT use `MigrateError::YamlParse` here. serde_yaml error
+    // messages can quote the offending scalar verbatim (a typed-tag coercion
+    // error renders `invalid value: string "<the-secret-value>"`), and that
+    // message would be printed to stderr by the CLI — leaking a secret value.
+    // `MigrateError::SecretsParse` carries only the path + line/column.
+    let parsed: serde_yaml::Value = serde_yaml::from_str(&raw).map_err(|e| {
+        let loc = e.location();
+        MigrateError::SecretsParse {
             path: path.display().to_string(),
-            source: e,
-        })?;
+            line: loc.as_ref().map_or(0, |l| l.line()),
+            column: loc.as_ref().map_or(0, |l| l.column()),
+        }
+    })?;
 
     let map = match parsed {
         serde_yaml::Value::Mapping(m) => m,
@@ -94,6 +102,59 @@ mod tests {
         assert!(secrets.is_empty());
     }
 
+    /// SECURITY regression (fails on the pre-fix `YamlParse` path): a malformed
+    /// `secrets.yaml` whose offending scalar is a secret value must NOT have that
+    /// value rendered in the returned error. serde_yaml's own error message for a
+    /// typed-tag coercion failure embeds the scalar verbatim
+    /// (`invalid value: string "<secret>"`); the old code wrapped that message
+    /// into `MigrateError::YamlParse { source }`, so `Display` leaked the secret.
+    #[test]
+    fn malformed_secrets_error_never_contains_secret_value() {
+        // `!!int` forces integer coercion of a string scalar; serde_yaml reports
+        // the scalar text in its message. The scalar here is a stand-in secret.
+        let yaml = "api_port: !!int s3cr3t_TOKEN_VALUE\n";
+        let mut f = NamedTempFile::new().unwrap();
+        f.write_all(yaml.as_bytes()).unwrap();
+
+        let err = read_secrets(f.path()).unwrap_err();
+        let rendered = err.to_string();
+
+        // The secret VALUE must never appear in the error output...
+        assert!(
+            !rendered.contains("s3cr3t_TOKEN_VALUE"),
+            "secret value leaked into error: {rendered}"
+        );
+        // ...and the full chain (with #[source]) must also be clean, since the
+        // CLI/anyhow prints the source chain too.
+        let mut source = std::error::Error::source(&err);
+        while let Some(s) = source {
+            assert!(
+                !s.to_string().contains("s3cr3t_TOKEN_VALUE"),
+                "secret value leaked into error source chain: {s}"
+            );
+            source = s.source();
+        }
+
+        // It should still be a structured, locatable error (fail-closed).
+        assert!(
+            matches!(err, MigrateError::SecretsParse { .. }),
+            "expected SecretsParse, got: {err:?}"
+        );
+    }
+
+    /// A secret KEY name is non-sensitive context and is fine to surface, but the
+    /// redacting error must still help the user locate the problem (line/column).
+    #[test]
+    fn malformed_secrets_error_reports_location() {
+        let yaml = "api_port: !!int notanumber\n";
+        let mut f = NamedTempFile::new().unwrap();
+        f.write_all(yaml.as_bytes()).unwrap();
+        let err = read_secrets(f.path()).unwrap_err();
+        let rendered = err.to_string();
+        assert!(rendered.contains("line"), "should report a line: {rendered}");
+        assert!(rendered.contains("redacted"), "should signal redaction: {rendered}");
+    }
+
     #[test]
     fn secret_count_is_correct() {
         let yaml = "a: 1\nb: 2\nc: 3\n";