fix(homecore): review findings from PR #1451 — HAP secret redaction, REST cap, event_type, migration --force (#1452)

HAP accessory signing seed no longer reachable via derived Debug. /api/history/period and /api/logbook no longer break the default (unfiltered) call shape above 32 entities. fire_event's event_type validation relaxed to match real HA's contract. homecore-migrate gained a --force flag for re-running imports. Public v2051 release notes corrected. 110 tests across the 3 touched crates, 0 failed, clippy clean.

Author: ruvnet

v2/crates/homecore-api/src/rest.rs

@@ -149,6 +149,7 @@ async fn history_response(
         ));
     }
 
+    let explicit_filter = query.filter_entity_id.is_some();
     let entity_ids = match query.filter_entity_id.as_deref() {
         Some(raw) => raw
             .split(',')
@@ -167,9 +168,15 @@ async fn history_response(
             .map(|snapshot| snapshot.entity_id.clone())
             .collect(),
     };
-    if entity_ids.len() > MAX_HISTORY_ENTITIES {
+    // Only reject an explicit, unusually-large `filter_entity_id` list. The
+    // real HA frontend's history page calls this endpoint with NO filter by
+    // design (meaning "all entities") — a real install routinely has 50-500+
+    // entities, so applying this cap there rejected the single most common
+    // call shape outright. The `MAX_API_HISTORY_ROWS` total-row budget below
+    // already bounds the actual work regardless of entity count.
+    if explicit_filter && entity_ids.len() > MAX_HISTORY_ENTITIES {
         return Err(ApiError::BadRequest(format!(
-            "history queries are limited to {MAX_HISTORY_ENTITIES} entities"
+            "history queries are limited to {MAX_HISTORY_ENTITIES} explicitly filtered entities"
         )));
     }
 
@@ -277,6 +284,7 @@ async fn logbook_response(
             "end_time must not precede start_time".into(),
         ));
     }
+    let explicit_filter = query.entity.is_some();
     let entity_ids = match query.entity.as_deref() {
         Some(raw) => raw
             .split(',')
@@ -295,9 +303,13 @@ async fn logbook_response(
             .map(|snapshot| snapshot.entity_id.clone())
             .collect(),
     };
-    if entity_ids.len() > MAX_HISTORY_ENTITIES {
+    // See the matching comment in `history_response`: only reject an
+    // explicit, unusually-large filter — the default (no filter, "all
+    // entities") is the real HA frontend's normal call shape, and the
+    // `MAX_API_HISTORY_ROWS` row budget below already bounds the work.
+    if explicit_filter && entity_ids.len() > MAX_HISTORY_ENTITIES {
         return Err(ApiError::BadRequest(format!(
-            "logbook queries are limited to {MAX_HISTORY_ENTITIES} entities"
+            "logbook queries are limited to {MAX_HISTORY_ENTITIES} explicitly filtered entities"
         )));
     }
     let mut entries = Vec::new();
@@ -592,19 +604,31 @@ pub async fn get_events(
     ))
 }
 
+/// Whether `event_type` is acceptable to fire on the domain bus.
+///
+/// Real Home Assistant places essentially no format restriction on event
+/// types beyond "non-empty string" — integrations commonly fire types with
+/// mixed case, dots, or hyphens (e.g. `mobile_app.notification_action`,
+/// `ios.action_fired`). The original check here only accepted
+/// `[a-z0-9_]+`, silently rejecting any of those — a real behavioral gap
+/// versus the documented contract, not a security boundary (this endpoint is
+/// already bearer-authenticated). We keep only the bounds that protect the
+/// server itself: non-empty, a sane length cap, and no control characters
+/// (which could otherwise corrupt log lines or downstream storage).
+pub(crate) fn is_valid_event_type(event_type: &str) -> bool {
+    !event_type.is_empty()
+        && event_type.len() <= 255
+        && event_type.chars().all(|ch| !ch.is_control())
+}
+
 pub async fn fire_event(
     headers: HeaderMap,
     State(s): State<SharedState>,
     Path(event_type): Path<String>,
     Json(body): Json<serde_json::Value>,
 ) -> ApiResult<Json<serde_json::Value>> {
     let _ = BearerAuth::from_headers(&headers, s.tokens()).await?;
-    if event_type.is_empty()
-        || event_type.len() > 255
-        || !event_type
-            .chars()
-            .all(|ch| ch.is_ascii_lowercase() || ch.is_ascii_digit() || ch == '_')
-    {
+    if !is_valid_event_type(&event_type) {
         return Err(ApiError::BadRequest("invalid event_type".into()));
     }
     if !body.is_object() && !body.is_null() {
@@ -705,3 +729,28 @@ pub async fn compatibility(
         }
     })))
 }
+
+#[cfg(test)]
+mod tests {
+    use super::is_valid_event_type;
+
+    /// Real HA integrations commonly fire event types with mixed case, dots,
+    /// or hyphens (e.g. `mobile_app.notification_action`). The original
+    /// `[a-z0-9_]+`-only check rejected all of these; only non-empty,
+    /// length, and control-character bounds should remain.
+    #[test]
+    fn realistic_ha_event_types_are_accepted() {
+        assert!(is_valid_event_type("mobile_app.notification_action"));
+        assert!(is_valid_event_type("ios.action_fired"));
+        assert!(is_valid_event_type("Custom-Event.2"));
+        assert!(is_valid_event_type("state_changed"));
+    }
+
+    #[test]
+    fn empty_oversized_or_control_char_event_types_are_rejected() {
+        assert!(!is_valid_event_type(""));
+        assert!(!is_valid_event_type(&"a".repeat(256)));
+        assert!(!is_valid_event_type("bad\nevent"));
+        assert!(!is_valid_event_type("bad\tevent"));
+    }
+}

v2/crates/homecore-api/src/ws.rs

@@ -328,12 +328,7 @@ impl Connection {
                     self.err(tx, cmd.id, "invalid_format", "event_type is required");
                     return;
                 };
-                if event_type.is_empty()
-                    || event_type.len() > 255
-                    || !event_type
-                        .chars()
-                        .all(|ch| ch.is_ascii_lowercase() || ch.is_ascii_digit() || ch == '_')
-                {
+                if !crate::rest::is_valid_event_type(&event_type) {
                     self.err(tx, cmd.id, "invalid_format", "invalid event_type");
                     return;
                 }

v2/crates/homecore-api/tests/compatibility_surface.rs

@@ -102,3 +102,92 @@ async fn history_reads_real_recorder_rows() {
     assert_eq!(body[0][0]["state"], "on");
     assert_eq!(body[0][0]["attributes"]["brightness"], 123);
 }
+
+/// The real HA frontend's history/logbook pages call these endpoints with NO
+/// entity filter by design (meaning "all entities") — a real install
+/// routinely has 50-500+ entities. The 32-entity cap must only reject an
+/// explicit, unusually-large `filter_entity_id`/`entity` list, never the
+/// default unfiltered "all entities" shape.
+#[tokio::test]
+async fn history_and_logbook_unfiltered_are_not_capped_by_entity_count() {
+    use homecore::{Context, EntityId};
+    use homecore_recorder::Recorder;
+
+    let homecore = HomeCore::new();
+    let recorder = Recorder::open("sqlite::memory:").await.unwrap();
+    for i in 0..40 {
+        homecore.states().set(
+            EntityId::parse(&format!("sensor.probe_{i}")).unwrap(),
+            "on",
+            serde_json::json!({}),
+            Context::new(),
+        );
+    }
+    assert_eq!(homecore.states().all().len(), 40, "sanity: more than MAX_HISTORY_ENTITIES");
+
+    let tokens = LongLivedTokenStore::empty();
+    tokens.register("test-token").await;
+    let state =
+        SharedState::with_tokens(homecore, "Test", "test", tokens).with_recorder(Some(recorder));
+    let app = router(state);
+
+    let history_response = app
+        .clone()
+        .oneshot(
+            Request::builder()
+                .uri("/api/history/period")
+                .header("authorization", "Bearer test-token")
+                .body(Body::empty())
+                .unwrap(),
+        )
+        .await
+        .unwrap();
+    assert_eq!(
+        history_response.status(),
+        StatusCode::OK,
+        "unfiltered history with >32 known entities must not be rejected"
+    );
+
+    let logbook_response = app
+        .oneshot(
+            Request::builder()
+                .uri("/api/logbook")
+                .header("authorization", "Bearer test-token")
+                .body(Body::empty())
+                .unwrap(),
+        )
+        .await
+        .unwrap();
+    assert_eq!(
+        logbook_response.status(),
+        StatusCode::OK,
+        "unfiltered logbook with >32 known entities must not be rejected"
+    );
+}
+
+/// An explicit, unusually-large `filter_entity_id` list is still rejected —
+/// only the default "no filter" shape is exempt from the cap.
+#[tokio::test]
+async fn history_explicit_oversized_filter_is_still_rejected() {
+    use homecore_recorder::Recorder;
+
+    let homecore = HomeCore::new();
+    let recorder = Recorder::open("sqlite::memory:").await.unwrap();
+    let tokens = LongLivedTokenStore::empty();
+    tokens.register("test-token").await;
+    let state =
+        SharedState::with_tokens(homecore, "Test", "test", tokens).with_recorder(Some(recorder));
+
+    let filter: String = (0..40).map(|i| format!("sensor.probe_{i}")).collect::<Vec<_>>().join(",");
+    let response = router(state)
+        .oneshot(
+            Request::builder()
+                .uri(format!("/api/history/period?filter_entity_id={filter}"))
+                .header("authorization", "Bearer test-token")
+                .body(Body::empty())
+                .unwrap(),
+        )
+        .await
+        .unwrap();
+    assert_eq!(response.status(), StatusCode::BAD_REQUEST);
+}

v2/crates/homecore-hap/src/pairing.rs

@@ -144,20 +144,44 @@ pub struct PairingStoreProvisioning {
     pub setup_code: Option<SetupCode>,
 }
 
-#[derive(Debug, Clone, Serialize, Deserialize)]
+#[derive(Clone, Serialize, Deserialize)]
 #[serde(deny_unknown_fields)]
 struct StoredAccessory {
     device_id: String,
     signing_seed: [u8; 32],
 }
 
-#[derive(Debug, Clone, Serialize, Deserialize)]
+// Manual, redacted impl: `signing_seed` is the accessory's permanent Ed25519
+// identity key, used to sign every Pair-Setup/Pair-Verify transcript for the
+// device's whole lifetime with no rotation mechanism. A derived `Debug` would
+// print it in plaintext the first time anything formats this struct (a log
+// line, a panic message) — same rationale as `SetupCode`'s manual impl below.
+impl std::fmt::Debug for StoredAccessory {
+    fn fmt(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        formatter
+            .debug_struct("StoredAccessory")
+            .field("device_id", &self.device_id)
+            .field("signing_seed", &"[REDACTED]")
+            .finish()
+    }
+}
+
+#[derive(Clone, Serialize, Deserialize)]
 #[serde(deny_unknown_fields)]
 struct StoredSetup {
     salt: [u8; 16],
     verifier: Vec<u8>,
 }
 
+// Manual, redacted impl: `salt`/`verifier` are the SRP-6a material derived
+// from the setup code. Printing them would hand an attacker exactly what an
+// offline dictionary attack against the (8-digit) setup code needs.
+impl std::fmt::Debug for StoredSetup {
+    fn fmt(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        formatter.write_str("StoredSetup([REDACTED])")
+    }
+}
+
 impl Drop for StoredAccessory {
     fn drop(&mut self) {
         self.signing_seed.zeroize();
@@ -679,6 +703,40 @@ mod tests {
         assert_eq!(format!("{code:?}"), "SetupCode([REDACTED])");
     }
 
+    /// A stray `format!("{store:?}")` (a future debug log line, a panic
+    /// message) must never print the accessory's permanent Ed25519 signing
+    /// seed or the SRP salt/verifier — both are compromise-forever secrets
+    /// with no rotation mechanism.
+    #[test]
+    fn stored_accessory_and_setup_debug_never_print_secret_material() {
+        let accessory = StoredAccessory {
+            device_id: "AA:BB:CC:DD:EE:FF".into(),
+            signing_seed: [0x42; 32],
+        };
+        let rendered = format!("{accessory:?}");
+        assert!(rendered.contains("device_id"));
+        assert!(rendered.contains("AA:BB:CC:DD:EE:FF"));
+        assert!(!rendered.contains("66"), "hex of 0x42 must not leak: {rendered}");
+        assert_eq!(
+            rendered,
+            "StoredAccessory { device_id: \"AA:BB:CC:DD:EE:FF\", signing_seed: \"[REDACTED]\" }"
+        );
+
+        let setup = StoredSetup { salt: [0x7a; 16], verifier: vec![0x13; 8] };
+        assert_eq!(format!("{setup:?}"), "StoredSetup([REDACTED])");
+
+        // The redaction must propagate through every derived-Debug container
+        // that embeds these structs, with no further code changes needed.
+        let state = StoreState {
+            accessory,
+            setup,
+            controllers: std::collections::BTreeMap::new(),
+        };
+        let rendered_state = format!("{state:?}");
+        assert!(!rendered_state.contains("0x42"));
+        assert!(rendered_state.contains("[REDACTED]"));
+    }
+
     #[test]
     fn removing_last_admin_clears_all_pairings() {
         let directory = tempfile::tempdir().unwrap();

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

@@ -48,6 +48,11 @@ pub struct ImportEntitiesArgs {
     /// Path to the HOMECORE storage directory (destination).
     #[arg(long)]
     pub to: PathBuf,
+    /// Overwrite an existing destination file instead of refusing. Use this
+    /// to re-run an import after fixing a bad source row, or to re-import
+    /// after further changes on the HA side.
+    #[arg(long)]
+    pub force: bool,
 }
 
 #[derive(Debug, clap::Args)]
@@ -58,6 +63,11 @@ pub struct ImportDevicesArgs {
     /// Path to the HOMECORE storage directory (destination).
     #[arg(long)]
     pub to: PathBuf,
+    /// Overwrite an existing destination file instead of refusing. Use this
+    /// to re-run an import after fixing a bad source row, or to re-import
+    /// after further changes on the HA side.
+    #[arg(long)]
+    pub force: bool,
 }
 
 #[derive(Debug, clap::Args)]
@@ -68,6 +78,11 @@ pub struct ImportConfigEntriesArgs {
     /// Path to the HOMECORE storage directory (destination).
     #[arg(long)]
     pub to: PathBuf,
+    /// Overwrite an existing destination file instead of refusing. Use this
+    /// to re-run an import after fixing a bad source row, or to re-import
+    /// after further changes on the HA side.
+    #[arg(long)]
+    pub force: bool,
 }
 
 #[derive(Debug, clap::Args)]

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

@@ -11,7 +11,7 @@ use std::path::{Path, PathBuf};
 use serde::{Deserialize, Serialize};
 
 use crate::{
-    storage::{read_envelope, write_json_atomic_noclobber, HaStorageEnvelope},
+    storage::{read_envelope, write_json_atomic, HaStorageEnvelope},
     MigrateError,
 };
 
@@ -183,9 +183,20 @@ pub fn convert_config_entries(path: &Path) -> Result<HomeCoreConfigEnvelope, Mig
 pub fn write_config_entries(
     storage_dir: &Path,
     envelope: &HomeCoreConfigEnvelope,
+) -> Result<PathBuf, MigrateError> {
+    write_config_entries_with(storage_dir, envelope, false)
+}
+
+/// As [`write_config_entries`], but `force = true` atomically replaces an
+/// existing destination instead of refusing — the escape hatch for
+/// re-running an import after fixing a bad source row.
+pub fn write_config_entries_with(
+    storage_dir: &Path,
+    envelope: &HomeCoreConfigEnvelope,
+    force: bool,
 ) -> Result<PathBuf, MigrateError> {
     let target = storage_dir.join(DESTINATION_KEY);
-    write_json_atomic_noclobber(&target, envelope)
+    write_json_atomic(&target, envelope, force)
 }
 
 pub fn read_homecore_config_entries(path: &Path) -> Result<HomeCoreConfigEnvelope, MigrateError> {