Files
ruvnet--RuView/v2/crates/homecore/src/service.rs
T
rUv bf1dfe79fd fix(homecore core): TOCTOU race dropped/reordered state_changed events under concurrent writers (~93k→0) + 2 fail-closed hardenings (#1087)
* fix(homecore): atomic state set — close TOCTOU lost/reordered state_changed events

StateMachine::set did get() (release shard lock) → compute next + no-op
decision → insert() (re-acquire lock) → send(). The read-modify-write was
not atomic w.r.t. a concurrent writer on the same entity: a writer that
read a stale `old` could mis-classify a real transition as a no-op and drop
its state_changed event (a missed automation trigger) or fire an event whose
new_state duplicated the previously delivered one (a spurious trigger for any
automation keyed on old_state != new_state). ADR-127 §2.1 promises "writer
atomically replaces the map entry"; the implementation did not.

Fix: hold the DashMap shard write-lock across the whole read→decide→insert→
fire sequence via entry()/insert_entry(). tx.send is non-blocking, non-async,
and never re-enters the map, so firing under the shard lock cannot deadlock
and keeps global event order in lock-step with global commit order.

Pinned by concurrent_set_fires_no_duplicate_adjacent_events: 4 writers
toggling one entity A/B; asserts no two consecutive fired events carry the
same new_state (impossible under correct serialisation). Fails reliably on
the old code (~365-476 duplicate-adjacent events on the first trial), passes
on the fix across repeated runs.

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

* harden(homecore): bound entity_id length — close memory-DoS at the REST boundary

homecore-api/src/rest.rs parses untrusted path segments straight through
EntityId::parse (get/delete/set_state). With no length cap, an otherwise-valid
id like "a." + many MB of [a-z0-9_] was accepted; a POST /api/states/<giant>
would persist it into the DashMap state store, permanently growing memory
(amplification across distinct ids).

Fix: reject ids longer than MAX_ENTITY_ID_LEN (255, HA-compatible) up front in
parse(), before any per-char scan, with a new EntityIdError::TooLong. Fails
closed at the boundary type so every caller (REST, registry deserialize,
automation) is protected.

Pinned by entity_id_length_boundary: exactly-MAX accepted, MAX+1 rejected,
4 MiB id rejected as TooLong. Fails on old code (oversized parses Ok).

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

* harden(homecore): isolate panicking service handlers (catch_unwind)

ServiceRegistry::call already ran handlers outside the registry lock (the
Arc<dyn ServiceHandler> is cloned out of the read guard first), so a panic
could never poison the RwLock or block other callers — good. But a panicking
handler unwound through call() into the caller's task; the task driving the
engine (e.g. an axum request handler invoking a service) could be aborted by
one buggy integration.

Fix: wrap the handler future in AssertUnwindSafe + FutureExt::catch_unwind and
convert a panic into ServiceError::HandlerPanicked. Mirrors HA isolating
service-handler exceptions. The registry stays fully usable afterwards.

Pinned by panicking_handler_is_isolated_and_registry_survives: the panicking
call returns HandlerPanicked (not an unwind), a sibling healthy service still
returns its value, and the bad service remains registered. Fails on old code
(the await point panics instead of returning Err).

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

* test(homecore): pin event-bus lag safety (bounded broadcast, no DoS)

Documents-with-evidence that the core EventBus does NOT have the homecore-api
WS broadcast-lag failure: with EVENT_CHANNEL_CAPACITY=4096, firing 3x capacity
while a subscriber never drains keeps fire_* non-blocking (publisher never
waits on slow receivers), gives the slow receiver a recoverable Lagged(n)
(drop-oldest + re-sync) rather than a closed channel, and leaves the bus live
for a fresh fast subscriber. No code change — pins the clean dimension.

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

* docs(homecore): record ADR-127 §9 security+concurrency review + CHANGELOG

Documents the three pinned fixes (HC-RACE-01 state-set TOCTOU, HC-EID-LEN-01
entity_id memory-DoS, HC-SVC-PANIC-01 service-handler isolation) and the
clean dimensions (bounded event-bus lag handling, lock discipline / no
lock-across-await, no panic-on-input) with their evidence.

Co-Authored-By: claude-flow <ruv@ruv.net>
2026-06-14 22:28:05 -04:00

254 lines
8.3 KiB
Rust

//! Service registry stub.
//!
//! Mirrors `homeassistant.core.ServiceRegistry`. P1 ships the public
//! surface + a simple direct-dispatch `call` so downstream ADRs can
//! depend on it; ADR-127 P2 replaces direct dispatch with the
//! mpsc-router pattern described in §2.3.
use std::collections::HashMap;
use std::future::Future;
use std::pin::Pin;
use std::sync::Arc;
use async_trait::async_trait;
use serde::{Deserialize, Serialize};
use thiserror::Error;
use tokio::sync::RwLock;
use crate::event::Context;
/// Service name within a domain. e.g. `light.turn_on` → domain
/// `"light"`, service `"turn_on"`.
#[derive(Clone, Debug, Eq, PartialEq, Hash, Serialize, Deserialize)]
pub struct ServiceName {
pub domain: String,
pub service: String,
}
impl ServiceName {
pub fn new(domain: impl Into<String>, service: impl Into<String>) -> Self {
Self {
domain: domain.into(),
service: service.into(),
}
}
}
/// Inbound service-call payload. Mirrors HA's `service_data` dict
/// plus the originating `Context`.
#[derive(Clone, Debug)]
pub struct ServiceCall {
pub name: ServiceName,
pub data: serde_json::Value,
pub context: Context,
}
#[derive(Error, Debug)]
pub enum ServiceError {
#[error("service not registered: {domain}.{service}")]
NotRegistered { domain: String, service: String },
#[error("service handler returned error: {0}")]
HandlerFailed(String),
#[error("service handler panicked: {0}")]
HandlerPanicked(String),
}
/// Handler trait. Integration code implements this and registers via
/// [`ServiceRegistry::register`]. P2 will add schema validation via
/// `serde` `Deserialize<'_>`.
#[async_trait]
pub trait ServiceHandler: Send + Sync + 'static {
async fn call(&self, call: ServiceCall) -> Result<serde_json::Value, ServiceError>;
}
/// Direct closure adapter so simple handlers don't need a struct.
pub struct FnHandler<F>(pub F);
#[async_trait]
impl<F, Fut> ServiceHandler for FnHandler<F>
where
F: Fn(ServiceCall) -> Fut + Send + Sync + 'static,
Fut: Future<Output = Result<serde_json::Value, ServiceError>> + Send + 'static,
{
async fn call(&self, call: ServiceCall) -> Result<serde_json::Value, ServiceError> {
(self.0)(call).await
}
}
#[derive(Clone)]
pub struct ServiceRegistry {
handlers: Arc<RwLock<HashMap<ServiceName, Arc<dyn ServiceHandler>>>>,
}
impl ServiceRegistry {
pub fn new() -> Self {
Self {
handlers: Arc::new(RwLock::new(HashMap::new())),
}
}
pub async fn register<H: ServiceHandler>(&self, name: ServiceName, handler: H) {
self.handlers.write().await.insert(name, Arc::new(handler));
}
pub async fn remove(&self, name: &ServiceName) {
self.handlers.write().await.remove(name);
}
pub async fn has(&self, name: &ServiceName) -> bool {
self.handlers.read().await.contains_key(name)
}
/// Call a service. P1 direct dispatch; P2 routes through the
/// event bus per ADR-127 §2.3.
///
/// The handler runs **outside** the registry lock (we clone the
/// `Arc<dyn ServiceHandler>` out of the read guard first), so a slow or
/// panicking handler can never poison the `RwLock` or block other
/// callers. A panic inside the handler is additionally caught and
/// converted to [`ServiceError::HandlerPanicked`] rather than unwinding
/// into the caller's task — one buggy integration cannot abort the task
/// that drives the engine. Mirrors HA isolating service-handler
/// exceptions.
pub async fn call(&self, call: ServiceCall) -> Result<serde_json::Value, ServiceError> {
let handler = {
let guard = self.handlers.read().await;
guard.get(&call.name).cloned()
};
match handler {
Some(h) => {
use futures::FutureExt;
let fut = std::panic::AssertUnwindSafe(h.call(call));
match fut.catch_unwind().await {
Ok(result) => result,
Err(panic) => Err(ServiceError::HandlerPanicked(panic_message(panic))),
}
}
None => Err(ServiceError::NotRegistered {
domain: call.name.domain.clone(),
service: call.name.service.clone(),
}),
}
}
pub async fn registered_services(&self) -> Vec<ServiceName> {
self.handlers.read().await.keys().cloned().collect()
}
}
impl Default for ServiceRegistry {
fn default() -> Self {
Self::new()
}
}
/// Best-effort extraction of a panic payload's message for
/// [`ServiceError::HandlerPanicked`]. Panic payloads are usually `&str`
/// or `String`; anything else collapses to a generic label.
fn panic_message(payload: Box<dyn std::any::Any + Send>) -> String {
if let Some(s) = payload.downcast_ref::<&str>() {
(*s).to_string()
} else if let Some(s) = payload.downcast_ref::<String>() {
s.clone()
} else {
"<non-string panic payload>".to_string()
}
}
// Suppress unused-import warning when no consumer of Pin/Box uses them yet
#[allow(dead_code)]
type _UnusedFutureType = Pin<Box<dyn Future<Output = ()> + Send>>;
#[cfg(test)]
mod tests {
use super::*;
#[tokio::test]
async fn register_and_call_returns_handler_value() {
let reg = ServiceRegistry::new();
reg.register(
ServiceName::new("light", "turn_on"),
FnHandler(|call: ServiceCall| async move {
Ok(serde_json::json!({"called_with": call.data}))
}),
)
.await;
let resp = reg
.call(ServiceCall {
name: ServiceName::new("light", "turn_on"),
data: serde_json::json!({"brightness": 200}),
context: Context::new(),
})
.await
.unwrap();
assert_eq!(resp["called_with"]["brightness"], 200);
}
#[tokio::test]
async fn unregistered_service_returns_error() {
let reg = ServiceRegistry::new();
let err = reg
.call(ServiceCall {
name: ServiceName::new("light", "turn_on"),
data: serde_json::json!({}),
context: Context::new(),
})
.await
.unwrap_err();
assert!(matches!(err, ServiceError::NotRegistered { .. }));
}
/// Service isolation: a panicking handler must be contained — converted
/// to `HandlerPanicked` rather than unwinding into the caller's task —
/// and the registry must remain fully usable afterwards (no poisoned
/// lock, other services still callable). On the pre-fix code the panic
/// unwinds through `call`, so the `catch_unwind`-based assertion below
/// fails (the await point panics instead of returning an `Err`).
#[tokio::test]
async fn panicking_handler_is_isolated_and_registry_survives() {
let reg = ServiceRegistry::new();
reg.register(
ServiceName::new("bad", "boom"),
FnHandler(|_call: ServiceCall| async move {
panic!("handler exploded");
#[allow(unreachable_code)]
Ok(serde_json::json!(null))
}),
)
.await;
reg.register(
ServiceName::new("good", "ping"),
FnHandler(|_call: ServiceCall| async move { Ok(serde_json::json!("pong")) }),
)
.await;
// The panicking call returns an error, not an unwind.
let err = reg
.call(ServiceCall {
name: ServiceName::new("bad", "boom"),
data: serde_json::json!({}),
context: Context::new(),
})
.await
.unwrap_err();
assert!(
matches!(err, ServiceError::HandlerPanicked(ref m) if m.contains("handler exploded")),
"expected HandlerPanicked, got {err:?}",
);
// The registry is not poisoned: a healthy service still works, and
// the bad service is still registered (call path, not lock, failed).
let ok = reg
.call(ServiceCall {
name: ServiceName::new("good", "ping"),
data: serde_json::json!({}),
context: Context::new(),
})
.await
.unwrap();
assert_eq!(ok, serde_json::json!("pong"));
assert!(reg.has(&ServiceName::new("bad", "boom")).await);
}
}