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.
This commit is contained in:
rUv 2026-07-27 17:01:25 -07:00 committed by GitHub
parent 42d56fc1a5
commit 535043731c
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
10 changed files with 328 additions and 30 deletions

View File

@ -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,6 +604,23 @@ 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>,
@ -599,12 +628,7 @@ pub async fn fire_event(
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"));
}
}

View File

@ -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;
}

View File

@ -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);
}

View File

@ -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();

View File

@ -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)]

View File

@ -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> {

View File

@ -7,7 +7,7 @@ use homecore::DeviceEntry;
use serde::Deserialize;
use crate::{
storage::{read_envelope, write_json_atomic_noclobber},
storage::{read_envelope, write_json_atomic},
storage_format::v13,
MigrateError,
};
@ -114,6 +114,17 @@ pub fn read_device_registry(path: &Path) -> Result<Vec<DeviceEntry>, MigrateErro
pub fn write_device_registry(
storage_dir: &Path,
devices: &[DeviceEntry],
) -> Result<PathBuf, MigrateError> {
write_device_registry_with(storage_dir, devices, false)
}
/// As [`write_device_registry`], 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_device_registry_with(
storage_dir: &Path,
devices: &[DeviceEntry],
force: bool,
) -> Result<PathBuf, MigrateError> {
let target = storage_dir.join(FILE_KEY);
let payload = serde_json::json!({
@ -125,7 +136,7 @@ pub fn write_device_registry(
"deleted_devices": []
}
});
write_json_atomic_noclobber(&target, &payload)
write_json_atomic(&target, &payload, force)
}
#[cfg(test)]
@ -180,4 +191,23 @@ mod tests {
1
);
}
/// `force = true` is the operator escape hatch: re-running an import
/// after fixing a bad source row (or re-importing after further HA-side
/// changes) must not require manually deleting prior output first.
#[test]
fn force_overwrites_existing_destination() {
let mut source = NamedTempFile::new().unwrap();
source.write_all(FIXTURE.as_bytes()).unwrap();
let devices = read_device_registry(source.path()).unwrap();
let destination = tempfile::tempdir().unwrap();
write_device_registry(destination.path(), &devices).unwrap();
write_device_registry_with(destination.path(), &[], true).unwrap();
assert_eq!(
read_device_registry(&destination.path().join(FILE_KEY))
.unwrap()
.len(),
0
);
}
}

View File

@ -33,7 +33,7 @@ use serde::{Deserialize, Serialize};
use homecore::{registry::DisabledBy, EntityCategory, EntityEntry, EntityId};
use crate::{
storage::{read_envelope, write_json_atomic_noclobber},
storage::{read_envelope, write_json_atomic},
storage_format::v13,
MigrateError,
};
@ -174,6 +174,17 @@ pub fn read_entity_registry(path: &Path) -> Result<Vec<EntityEntry>, MigrateErro
pub fn write_entity_registry(
storage_dir: &Path,
entries: &[EntityEntry],
) -> Result<PathBuf, MigrateError> {
write_entity_registry_with(storage_dir, entries, false)
}
/// As [`write_entity_registry`], 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_entity_registry_with(
storage_dir: &Path,
entries: &[EntityEntry],
force: bool,
) -> Result<PathBuf, MigrateError> {
let target = storage_dir.join(FILE_KEY);
let payload = serde_json::json!({
@ -185,7 +196,7 @@ pub fn write_entity_registry(
"deleted_entities": []
}
});
write_json_atomic_noclobber(&target, &payload)
write_json_atomic(&target, &payload, force)
}
#[cfg(test)]

View File

@ -61,8 +61,11 @@ fn main() -> anyhow::Result<()> {
Command::ImportEntities(args) => {
let entity_path = args.storage.join("core.entity_registry");
let entries = homecore_migrate::entity_registry::read_entity_registry(&entity_path)?;
let destination =
homecore_migrate::entity_registry::write_entity_registry(&args.to, &entries)?;
let destination = homecore_migrate::entity_registry::write_entity_registry_with(
&args.to,
&entries,
args.force,
)?;
print_summary(&ImportSummary {
kind: "entity_registry",
imported: entries.len(),
@ -75,8 +78,11 @@ fn main() -> anyhow::Result<()> {
Command::ImportDevices(args) => {
let device_path = args.storage.join("core.device_registry");
let devices = homecore_migrate::device_registry::read_device_registry(&device_path)?;
let destination =
homecore_migrate::device_registry::write_device_registry(&args.to, &devices)?;
let destination = homecore_migrate::device_registry::write_device_registry_with(
&args.to,
&devices,
args.force,
)?;
print_summary(&ImportSummary {
kind: "device_registry",
imported: devices.len(),
@ -107,8 +113,11 @@ fn main() -> anyhow::Result<()> {
.iter()
.map(serde_json::to_value)
.collect::<Result<Vec<_>, _>>()?;
let destination =
homecore_migrate::config_entries::write_config_entries(&args.to, &converted)?;
let destination = homecore_migrate::config_entries::write_config_entries_with(
&args.to,
&converted,
args.force,
)?;
print_summary(&ImportSummary {
kind: "config_entries",
imported,

View File

@ -80,6 +80,21 @@ pub fn read_envelope(path: &Path) -> Result<HaStorageEnvelope, MigrateError> {
pub fn write_json_atomic_noclobber<T: Serialize>(
target: &Path,
value: &T,
) -> Result<PathBuf, MigrateError> {
write_json_atomic(target, value, false)
}
/// Same atomic write (temp file → `sync_all` → publish), but when `force` is
/// true an existing destination is atomically replaced via `rename` instead
/// of refusing via `hard_link`'s `AlreadyExists`. Without `force`, an
/// operator who re-runs an import after fixing a bad source row (or wants a
/// fresh pass) had no way to overwrite prior output short of deleting it by
/// hand first — this is the escape hatch for that, opt-in so the default
/// no-clobber safety is unchanged.
pub fn write_json_atomic<T: Serialize>(
target: &Path,
value: &T,
force: bool,
) -> Result<PathBuf, MigrateError> {
let parent = target.parent().ok_or_else(|| MigrateError::Io {
path: target.display().to_string(),
@ -112,6 +127,22 @@ pub fn write_json_atomic_noclobber<T: Serialize>(
file.write_all(&bytes)?;
file.write_all(b"\n")?;
file.sync_all()?;
if force {
// `rename`-over-existing hits real sharing-violation flakiness
// on Windows (ERROR_ACCESS_DENIED even with no other open
// handle in this process). Pre-clear the destination instead,
// then publish through the same hard_link step the no-clobber
// path uses. This briefly widens the crash window (a crash
// between remove and hard_link leaves no destination file
// rather than the old one), which is the accepted, opt-in
// tradeoff of explicitly requesting an overwrite — the default
// (non-force) path keeps its full no-clobber atomicity.
match fs::remove_file(target) {
Ok(()) => {}
Err(error) if error.kind() == std::io::ErrorKind::NotFound => {}
Err(error) => return Err(error),
}
}
fs::hard_link(&temp, target)?;
fs::remove_file(&temp)?;
Ok(())