From cfac917b4d0bd7c4c6ed00a0e44bdd69bf7f768e Mon Sep 17 00:00:00 2001 From: damocles Date: Sun, 19 Jul 2026 17:46:04 +0200 Subject: [PATCH] feat(#2302): parse agent names into AgentName newtype at dashboard boundary --- hive-c0re/src/dashboard/build_logs.rs | 17 ++-- hive-c0re/src/dashboard/idents.rs | 103 +++++++++++++++++++++++++ hive-c0re/src/dashboard/journal.rs | 15 ++-- hive-c0re/src/dashboard/misc_api.rs | 13 ++-- hive-c0re/src/dashboard/mod.rs | 74 ++---------------- hive-c0re/src/dashboard/permissions.rs | 18 +++-- hive-c0re/src/dashboard/tombstones.rs | 19 +++-- hive-c0re/src/socket_server/mod.rs | 6 +- 8 files changed, 162 insertions(+), 103 deletions(-) create mode 100644 hive-c0re/src/dashboard/idents.rs diff --git a/hive-c0re/src/dashboard/build_logs.rs b/hive-c0re/src/dashboard/build_logs.rs index c469caf0..4dbf4071 100644 --- a/hive-c0re/src/dashboard/build_logs.rs +++ b/hive-c0re/src/dashboard/build_logs.rs @@ -18,7 +18,7 @@ use serde::{Deserialize, Serialize}; use tokio_stream::Stream; use tokio_stream::wrappers::ReceiverStream; -use super::{AppState, error_response, validate_agent_name}; +use super::{AppState, error_response, idents::AgentName}; #[derive(Deserialize)] pub(super) struct BuildLogsAllQuery { @@ -58,11 +58,18 @@ pub(super) async fn get_build_logs_agent( AxumPath(name): AxumPath, axum::extract::Query(q): axum::extract::Query, ) -> Response { - if let Some(reason) = validate_agent_name(&name) { - return (StatusCode::BAD_REQUEST, format!("bad agent name: {reason}")).into_response(); - } + let name = match AgentName::parse(&name) { + Ok(n) => n, + Err(reason) => { + return (StatusCode::BAD_REQUEST, format!("bad agent name: {reason}")).into_response(); + } + }; let limit = q.limit.unwrap_or(10); - match state.coord.build_logs.list_recent_for_agent(&name, limit) { + match state + .coord + .build_logs + .list_recent_for_agent(name.as_str(), limit) + { Ok(rows) => axum::Json(rows).into_response(), Err(e) => error_response(&format!("build-logs {name}: {e:#}")), } diff --git a/hive-c0re/src/dashboard/idents.rs b/hive-c0re/src/dashboard/idents.rs new file mode 100644 index 00000000..6516573f --- /dev/null +++ b/hive-c0re/src/dashboard/idents.rs @@ -0,0 +1,103 @@ +//! Validated identifier newtypes for dashboard path-params — "parse, don't +//! validate" (#2302). +//! +//! [`AgentName`] can only be constructed through a validating parser, so +//! "this string passed the naming whitelist" becomes a compile-time fact the +//! type carries, instead of a convention every handler re-checks against the +//! raw `String`. This is **format** validation only — +//! whether the name refers to a *live* agent is a separate, stateful runtime +//! concern kept at the lookup sites (see `guard_agent_name`), deliberately not +//! folded into the constructor. + +use std::fmt; + +/// A validated agent name: 1-63 chars of `[a-z0-9_-]`. +/// +/// Constructed only via [`AgentName::parse`]. The invariant matches the +/// historical `validate_agent_name` whitelist (conservative, tracking +/// `nixos-container` basename rules and the agent-name convention across the +/// codebase): rejects empty, over-long, uppercase, slashes, dots, and any +/// non-ASCII (incl. unicode homoglyphs of dash / underscore). +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) struct AgentName(String); + +impl AgentName { + /// Parse + validate a path-param agent name (format only). + /// + /// # Errors + /// Returns `Err(reason)` — a caller-ready 400-body string — when `name` + /// is empty, longer than 63 chars, or contains any byte outside + /// `[a-z0-9_-]`. + pub(crate) fn parse(name: &str) -> Result { + if name.is_empty() { + return Err("agent name must not be empty"); + } + if name.len() > 63 { + return Err("agent name must be 63 characters or fewer"); + } + if !name + .bytes() + .all(|b| b.is_ascii_lowercase() || b.is_ascii_digit() || b == b'-' || b == b'_') + { + return Err("agent name must contain only [a-z0-9_-]"); + } + Ok(Self(name.to_owned())) + } + + /// The validated name as a string slice. + #[must_use] + pub(crate) fn as_str(&self) -> &str { + &self.0 + } +} + +impl fmt::Display for AgentName { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.write_str(&self.0) + } +} + +#[cfg(test)] +mod tests { + use super::AgentName; + + #[test] + fn agent_name_accepts_canonical_shapes() { + for ok in [ + "damocles", + "hm1nd", + "agent-with-dashes", + "snake_case", + "mixed_2-3", + ] { + assert!(AgentName::parse(ok).is_ok(), "should accept {ok:?}"); + } + let max = "a".repeat(63); + assert!(AgentName::parse(&max).is_ok(), "63 chars is the boundary"); + } + + #[test] + fn agent_name_rejects_bad_input() { + let too_long = "a".repeat(64); + for bad in [ + "", + &too_long, + "../etc/passwd", + "alice/bob", + "Alice", + "alice bob", + "alice.bob", + "alice;DROP TABLE messages", + // Non-ASCII, incl. unicode homoglyphs of ASCII dash. + "damóclès", + "alice\u{2013}bob", // en-dash + ] { + assert!(AgentName::parse(bad).is_err(), "should reject {bad:?}"); + } + } + + #[test] + fn agent_name_round_trips_as_str() { + assert_eq!(AgentName::parse("damocles").unwrap().as_str(), "damocles"); + } +} diff --git a/hive-c0re/src/dashboard/journal.rs b/hive-c0re/src/dashboard/journal.rs index 5eb739f3..8851d9ac 100644 --- a/hive-c0re/src/dashboard/journal.rs +++ b/hive-c0re/src/dashboard/journal.rs @@ -18,7 +18,7 @@ use serde::Deserialize; use problem_details::ProblemDetails; -use super::{error_problem, strip_container_prefix, validate_agent_name}; +use super::{error_problem, idents::AgentName, strip_container_prefix}; use crate::lifecycle; #[derive(Deserialize)] @@ -57,13 +57,16 @@ pub(super) async fn get_journal( // shellout below — the `lifecycle::list()` existence check would // catch them anyway, but rejecting at the boundary keeps the // failure mode crisp. - if let Some(reason) = validate_agent_name(&name) { - return Err(ProblemDetails::from_status_code(StatusCode::BAD_REQUEST) - .with_detail(format!("bad agent name: {reason}"))); - } + let name = match AgentName::parse(&name) { + Ok(n) => n, + Err(reason) => { + return Err(ProblemDetails::from_status_code(StatusCode::BAD_REQUEST) + .with_detail(format!("bad agent name: {reason}"))); + } + }; // Validate the container name against the list of managed // containers so we don't shell out with arbitrary input. - let container = strip_container_prefix(&name); + let container = strip_container_prefix(name.as_str()); let prefixed = format!("{}{container}", lifecycle::AGENT_PREFIX); let live = lifecycle::list().await.unwrap_or_default(); if !live.iter().any(|c| c == &prefixed) { diff --git a/hive-c0re/src/dashboard/misc_api.rs b/hive-c0re/src/dashboard/misc_api.rs index 8f5fde99..dbd099fc 100644 --- a/hive-c0re/src/dashboard/misc_api.rs +++ b/hive-c0re/src/dashboard/misc_api.rs @@ -10,7 +10,7 @@ use axum::{ }; use serde::Deserialize; -use super::{AppState, error_response, scan_validated_paths, validate_agent_name}; +use super::{AppState, error_response, idents::AgentName, scan_validated_paths}; /// Unread operator-directed messages for the dashboard's Y3R C4LL inbox. /// Returns messages addressed to `"operator"` that haven't been @@ -113,10 +113,13 @@ pub(super) async fn post_mark_all_read( State(state): State, AxumPath(name): AxumPath, ) -> Response { - if let Some(reason) = validate_agent_name(&name) { - return (StatusCode::BAD_REQUEST, format!("bad agent name: {reason}")).into_response(); - } - match state.coord.broker.mark_all_read(&name) { + let name = match AgentName::parse(&name) { + Ok(n) => n, + Err(reason) => { + return (StatusCode::BAD_REQUEST, format!("bad agent name: {reason}")).into_response(); + } + }; + match state.coord.broker.mark_all_read(name.as_str()) { Ok(n) => { tracing::info!(%name, marked = n, "operator marked all messages read"); axum::Json(serde_json::json!({ "marked": n })).into_response() diff --git a/hive-c0re/src/dashboard/mod.rs b/hive-c0re/src/dashboard/mod.rs index 85fb0aa2..cd559acd 100644 --- a/hive-c0re/src/dashboard/mod.rs +++ b/hive-c0re/src/dashboard/mod.rs @@ -19,6 +19,8 @@ use crate::lifecycle; mod approvals; mod build_logs; mod extra_forges; +mod idents; +pub(crate) use idents::AgentName; mod infra_containers; mod journal; mod lifecycle_ops; @@ -292,33 +294,10 @@ fn try_bind(addr: SocketAddr) -> std::io::Result { sock.listen(1024) } -/// Validate that a path-param agent name conforms to the hyperhive -/// naming whitelist: 1-63 chars of `[a-z0-9_-]`. Rejects empty, -/// uppercase, slashes, dots, and any non-ASCII (incl. unicode -/// homoglyphs of dash/underscore). Returns `None` on accept, `Some(reason)` -/// on reject — caller wraps the reason in a 400 response. Conservative -/// whitelist matching `nixos-container` basename rules and the existing -/// agent-name convention across the codebase. -pub(crate) fn validate_agent_name(name: &str) -> Option<&'static str> { - if name.is_empty() { - return Some("agent name must not be empty"); - } - if name.len() > 63 { - return Some("agent name must be 63 characters or fewer"); - } - if !name - .bytes() - .all(|b| b.is_ascii_lowercase() || b.is_ascii_digit() || b == b'-' || b == b'_') - { - return Some("agent name must contain only [a-z0-9_-]"); - } - None -} - /// Two-axis path-param guard for write routes. Combines: /// -/// 1. **format validation** (`validate_agent_name`) — rejects path -/// traversal / unicode homoglyphs / empty + too-long names with +/// 1. **format validation** ([`idents::AgentName::parse`]) — rejects +/// path traversal / unicode homoglyphs / empty + too-long names with /// HTTP 400. /// 2. **existence check** — looks up `name` in the coordinator's /// container snapshot; unknown name → HTTP 404 with a clear @@ -332,9 +311,9 @@ pub(crate) fn validate_agent_name(name: &str) -> Option<&'static str> { /// handler taking a name path-param. Read-only GET handlers and /// handlers that legitimately operate on tombstoned agents (e.g. /// `mark-all-read` on broker rows for a destroyed agent) call -/// `validate_agent_name` directly and skip the existence check. +/// [`idents::AgentName::parse`] directly and skip the existence check. async fn guard_agent_name(state: &AppState, name: &str) -> Option { - if let Some(reason) = validate_agent_name(name) { + if let Err(reason) = idents::AgentName::parse(name) { return Some( (StatusCode::BAD_REQUEST, format!("bad agent name: {reason}")).into_response(), ); @@ -396,45 +375,4 @@ mod tests { let fv = serde_json::to_value(&five).expect("problem details serialise"); assert_eq!(fv["status"], 500); } - - #[test] - fn validate_agent_name_accepts_canonical_shapes() { - assert!(validate_agent_name("damocles").is_none()); - assert!(validate_agent_name("hm1nd").is_none()); - assert!(validate_agent_name("agent-with-dashes").is_none()); - assert!(validate_agent_name("snake_case").is_none()); - assert!(validate_agent_name("mixed_2-3").is_none()); - let max = "a".repeat(63); - assert!( - validate_agent_name(&max).is_none(), - "63-char name should pass" - ); - } - - // The two-axis guard (`guard_agent_name`) wires `validate_agent_name` - // + an async coordinator lookup. The lookup needs a populated - // `Coordinator`, which needs sqlite + tokio runtime; rather than - // build that scaffolding for an integration-flavoured test we cover - // the format axis here (the existence axis is enforced by the - // shared `containers_snapshot` API, tested in `coordinator.rs`'s - // own suite). 9 cases below cover the boundary-length case and - // other expected rejects to make the contract explicit. - #[test] - fn validate_agent_name_rejects_bad_input() { - assert!(validate_agent_name("").is_some()); - let too_long = "a".repeat(64); - assert!(validate_agent_name(&too_long).is_some()); - // Path-traversal attempts. - assert!(validate_agent_name("../etc/passwd").is_some()); - assert!(validate_agent_name("alice/bob").is_some()); - // Uppercase rejected — canonical lowercase convention. - assert!(validate_agent_name("Alice").is_some()); - // No spaces, dots, special chars. - assert!(validate_agent_name("alice bob").is_some()); - assert!(validate_agent_name("alice.bob").is_some()); - assert!(validate_agent_name("alice;DROP TABLE messages").is_some()); - // Non-ASCII (incl. unicode homoglyphs of ASCII dash). - assert!(validate_agent_name("damóclès").is_some()); - assert!(validate_agent_name("alice\u{2013}bob").is_some()); // en-dash - } } diff --git a/hive-c0re/src/dashboard/permissions.rs b/hive-c0re/src/dashboard/permissions.rs index 1f133354..b3fa368b 100644 --- a/hive-c0re/src/dashboard/permissions.rs +++ b/hive-c0re/src/dashboard/permissions.rs @@ -14,7 +14,7 @@ use serde::{Deserialize, Serialize}; use problem_details::ProblemDetails; -use super::{AppState, guard_agent_name, strip_container_prefix, validate_agent_name}; +use super::{AppState, guard_agent_name, idents::AgentName, strip_container_prefix}; #[derive(Serialize)] pub(super) struct ToolGroupsSnapshot { @@ -371,24 +371,26 @@ pub(super) async fn get_stale_permissions( /// /// Bypasses `guard_agent_name`'s live-roster check intentionally — /// the whole point is to remove entries for non-roster agents. Only -/// the format check (`validate_agent_name`) is applied. No rebuild is +/// the format check ([`AgentName::parse`]) is applied. No rebuild is /// enqueued (the agent doesn't exist to rebuild); the SSE snapshots /// update the P3RM1SS10NS tab live. pub(super) async fn delete_agent_permissions( State(state): State, AxumPath(name): AxumPath, ) -> Response { - let logical = strip_container_prefix(&name); - if let Some(reason) = validate_agent_name(&logical) { - return (StatusCode::BAD_REQUEST, format!("bad agent name: {reason}")).into_response(); - } + let logical = match AgentName::parse(&strip_container_prefix(&name)) { + Ok(n) => n, + Err(reason) => { + return (StatusCode::BAD_REQUEST, format!("bad agent name: {reason}")).into_response(); + } + }; // Run both removals regardless so we clean up as much as possible // even on partial I/O errors. Collect errors to surface below. - let tg_err = crate::tool_groups::remove_agent(&logical).err(); + let tg_err = crate::tool_groups::remove_agent(logical.as_str()).err(); if let Some(ref e) = tg_err { tracing::warn!(agent = %logical, error = ?e, "failed to remove tool-groups entry"); } - let cap_err = crate::capabilities::remove_agent(&logical).err(); + let cap_err = crate::capabilities::remove_agent(logical.as_str()).err(); if let Some(ref e) = cap_err { tracing::warn!(agent = %logical, error = ?e, "failed to remove capabilities entry"); } diff --git a/hive-c0re/src/dashboard/tombstones.rs b/hive-c0re/src/dashboard/tombstones.rs index 885f9676..4844ef64 100644 --- a/hive-c0re/src/dashboard/tombstones.rs +++ b/hive-c0re/src/dashboard/tombstones.rs @@ -17,7 +17,7 @@ use crate::container_view::{ContainerView, claude_has_session}; use crate::coordinator::Coordinator; use crate::lifecycle; -use super::{AppState, error_response, validate_agent_name}; +use super::{AppState, error_response, idents::AgentName}; #[derive(Serialize, Clone, Debug)] pub struct TombstoneView { @@ -116,16 +116,19 @@ pub(super) async fn post_purge_tombstone( // `containers_snapshot()` is deliberately NOT used here: // tombstoned agents are gone from the snapshot by design; that's // the whole point of this endpoint. - if let Some(reason) = validate_agent_name(&name) { - return (StatusCode::BAD_REQUEST, format!("bad agent name: {reason}")).into_response(); - } + let name = match AgentName::parse(&name) { + Ok(n) => n, + Err(reason) => { + return (StatusCode::BAD_REQUEST, format!("bad agent name: {reason}")).into_response(); + } + }; // Sanity: refuse to purge if a live container still exists with this // name. The dashboard already filters tombstones to non-live names, // but the operator could send a stale POST. let live = lifecycle::list().await.unwrap_or_default(); if live .iter() - .any(|c| c == &format!("{}{name}", lifecycle::AGENT_PREFIX) || c == &name) + .any(|c| c == &format!("{}{name}", lifecycle::AGENT_PREFIX) || c.as_str() == name.as_str()) { return error_response(&format!( "refusing to purge {name}: container still exists — use DESTR0Y first" @@ -133,8 +136,8 @@ pub(super) async fn post_purge_tombstone( } let mut errors = Vec::new(); for dir in [ - crate::paths::agent_state_dir(&name), - crate::paths::applied_dir(&name), + crate::paths::agent_state_dir(name.as_str()), + crate::paths::applied_dir(name.as_str()), ] { if dir.exists() && let Err(e) = std::fs::remove_dir_all(&dir) @@ -145,7 +148,7 @@ pub(super) async fn post_purge_tombstone( let _ = state .coord .approvals - .fail_pending_for_agent(&name, "agent state purged"); + .fail_pending_for_agent(name.as_str(), "agent state purged"); if errors.is_empty() { tracing::info!(%name, "tombstone purged"); // Fire the post-purge tombstones snapshot so dashboards diff --git a/hive-c0re/src/socket_server/mod.rs b/hive-c0re/src/socket_server/mod.rs index 7b04daf3..8efd8440 100644 --- a/hive-c0re/src/socket_server/mod.rs +++ b/hive-c0re/src/socket_server/mod.rs @@ -425,7 +425,7 @@ async fn handle_get_agent_meta( // the OS level. Validate it before any path is built. The `None` default // (`target == agent`) is the caller's own authenticated name, already // valid — but validating unconditionally is simplest and harmless. - if let Some(reason) = crate::dashboard::validate_agent_name(target) { + if let Err(reason) = crate::dashboard::AgentName::parse(target) { return hive_agent_sock::Response::Err { message: format!("get_agent_meta: invalid agent name {target:?}: {reason}"), }; @@ -445,7 +445,7 @@ async fn handle_get_agent_meta( // `@user:server` / `homeserver`) — the access token lives separately // in the agent's `matrix-token` and is never part of this response. // Peer visibility is intentional: it lets an agent verify/contact - // another on a public matrix instance. The `validate_agent_name` gate + // another on a public matrix instance. The `AgentName::parse` gate // above is what closes the real vector here (path traversal via `../` // in an agent-supplied name). matrix_accounts: read_agent_matrix_identities(target), @@ -748,7 +748,7 @@ fn require_group(agent: &str, group: &str, action: &str) -> Option { /// `submit_init_config`, which builds filesystem paths from it, so validate /// before that. fn require_new_child(agent: &str, target: &str, action: &str) -> Option { - if let Some(reason) = crate::dashboard::validate_agent_name(target) { + if let Err(reason) = crate::dashboard::AgentName::parse(target) { return Some(Response::Err { message: format!("agent `{agent}` cannot {action} `{target}`: {reason}"), });