From 0006c0635e868f18e2a3f524dd51179441780b11 Mon Sep 17 00:00:00 2001 From: damocles Date: Fri, 29 May 2026 18:40:30 +0200 Subject: [PATCH] dashboard: validate agent-name on every write route (closes #572) --- hive-c0re/src/dashboard.rs | 81 ++++++++++++++++++++++++++++++++++++-- 1 file changed, 78 insertions(+), 3 deletions(-) diff --git a/hive-c0re/src/dashboard.rs b/hive-c0re/src/dashboard.rs index eda66f71..86fd64d2 100644 --- a/hive-c0re/src/dashboard.rs +++ b/hive-c0re/src/dashboard.rs @@ -1034,6 +1034,13 @@ async fn get_journal( AxumPath(name): AxumPath, axum::extract::Query(q): axum::extract::Query, ) -> Response { + // Defense-in-depth format check (#572) so weird chars never reach + // the 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 (StatusCode::BAD_REQUEST, format!("bad agent name: {reason}")).into_response(); + } // 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); @@ -1274,6 +1281,14 @@ mod tests { 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 match what shipped in #581 + the new + // boundary-length case to make the contract explicit. #[test] fn validate_agent_name_rejects_bad_input() { assert!(validate_agent_name("").is_some()); @@ -1708,6 +1723,12 @@ async fn post_schedule_cancel( /// Failure modes (agent down, slow response, malformed JSON) all /// degrade to an empty list so the dashboard still renders. async fn get_agent_links(AxumPath(name): AxumPath) -> Response { + // Format-only guard (#572). GET routes return empty gracefully + // on unknown names per argus's spec; bad format we reject early + // so the downstream port-hash + HTTP fetch never sees garbage. + if validate_agent_name(&name).is_some() { + return axum::Json(serde_json::json!([])).into_response(); + } let port = lifecycle::agent_web_port(&name); let url = format!("http://127.0.0.1:{port}/api/state"); let client = match reqwest::Client::builder() @@ -1784,9 +1805,8 @@ async fn post_retry_reminder( /// 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. (mara nag on #566 — this -/// is the local fix for `mark-all-read`; general extractor + rollout -/// across other endpoints tracked at #572.) +/// agent-name convention across the codebase. (mara nag on #566 → +/// shipped in #581; rolled out across every write route in #572.) fn validate_agent_name(name: &str) -> Option<&'static str> { if name.is_empty() { return Some("agent name must not be empty"); @@ -1803,6 +1823,43 @@ fn validate_agent_name(name: &str) -> Option<&'static str> { None } +/// Two-axis path-param guard for write routes (#572). Combines: +/// +/// 1. **format validation** (`validate_agent_name`) — 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 +/// "no such agent" message. catches the operator-typo case where +/// a destructive POST would otherwise hit silently (mark-all-read +/// returning 0) or hit downstream lifecycle code that fails with +/// a confusing nspawn error. +/// +/// Returns `None` when both checks pass (caller proceeds), `Some(Response)` +/// when the request should be rejected. Use at the top of every write +/// 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. +async fn guard_agent_name(state: &AppState, name: &str) -> Option { + if let Some(reason) = validate_agent_name(name) { + return Some( + (StatusCode::BAD_REQUEST, format!("bad agent name: {reason}")).into_response(), + ); + } + let snapshot = state.coord.containers_snapshot().await; + if !snapshot.iter().any(|c| c.name == name) { + return Some( + ( + StatusCode::NOT_FOUND, + format!("no such agent: {name}"), + ) + .into_response(), + ); + } + None +} + /// Operator-driven "clear this agent's inbox" — backs the side-panel /// "mark all read" button (#559). Marks every message addressed to the /// agent as acked (backfilling `delivered_at` for any still-pending @@ -2044,6 +2101,9 @@ async fn post_set_parent( async fn post_rebuild(State(state): State, AxumPath(name): AxumPath) -> Response { let logical = strip_container_prefix(&name); + if let Some(reject) = guard_agent_name(&state, &logical).await { + return reject; + } state.coord.rebuild_queue.enqueue( crate::rebuild_queue::QueueKind::Rebuild, logical, @@ -2094,6 +2154,10 @@ where } async fn post_kill(State(state): State, AxumPath(name): AxumPath) -> Response { + let logical = strip_container_prefix(&name); + if let Some(reject) = guard_agent_name(&state, &logical).await { + return reject; + } // #443: manager is stoppable from the dashboard like any other // agent. The host's dashboard server keeps running (it's // hive-c0re, not the manager container), per-agent approvals @@ -2121,6 +2185,10 @@ async fn post_kill(State(state): State, AxumPath(name): AxumPath, AxumPath(name): AxumPath) -> Response { + let logical = strip_container_prefix(&name); + if let Some(reject) = guard_agent_name(&state, &logical).await { + return reject; + } lifecycle_action( &state, &name, @@ -2133,6 +2201,10 @@ async fn post_restart(State(state): State, AxumPath(name): AxumPath, AxumPath(name): AxumPath) -> Response { + let logical = strip_container_prefix(&name); + if let Some(reject) = guard_agent_name(&state, &logical).await { + return reject; + } lifecycle_action( &state, &name, @@ -2199,6 +2271,9 @@ async fn post_destroy( AxumPath(name): AxumPath, Form(form): Form, ) -> Response { + if let Some(reject) = guard_agent_name(&state, &name).await { + return reject; + } // Checkbox semantics: any non-empty value (axum sends "on") = purge. let purge = form.purge.as_deref().is_some_and(|v| !v.is_empty()); // `actions::destroy` rescans the container list on success, so the