From 7b1b1d9db829d86e97d7db90c7b70847f90f1a4f Mon Sep 17 00:00:00 2001 From: atlas Date: Sun, 12 Jul 2026 02:21:33 +0200 Subject: [PATCH] =?UTF-8?q?fix(#2164):=20address=20argus=20review=20?= =?UTF-8?q?=E2=80=94=20Option=20secret=20+=20stale=20hook=20cleanu?= =?UTF-8?q?p?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two issues flagged by argus in PR #2388 review: 1. Empty-key fallback: when load_or_generate() failed, webhook_secret was String::new(). An attacker knowing this could forge deliveries with a valid HMAC of the empty key. Fix: change to Option; on None, skip hook registration entirely and return 503 from /webhook/* handlers (rather than 401 with a misleadingly-verifiable empty-key HMAC). 2. Stale hook cleanup: on upgrade from old code, old loopback hooks (http://127.0.0.1:.../webhook/knowledge, .../webhook/config-pr) were left alongside the new domain-URL hook. Fix: during ensure_webhook / ensure_config_pr_webhook, after listing hooks, delete any that end with our path suffix but point at a different base URL. clippy + nix fmt clean. --- hive-c0re/src/dashboard/mod.rs | 10 ++++++++-- hive-c0re/src/dashboard/webhook.rs | 24 +++++++++++++++++++----- hive-c0re/src/forge/mod.rs | 24 ++++++++++++++++++++++++ hive-c0re/src/main.rs | 18 +++++++++++++----- hive-c0re/src/workers/knowledge.rs | 20 ++++++++++++++++++++ 5 files changed, 84 insertions(+), 12 deletions(-) diff --git a/hive-c0re/src/dashboard/mod.rs b/hive-c0re/src/dashboard/mod.rs index 655c2145..c68e0336 100644 --- a/hive-c0re/src/dashboard/mod.rs +++ b/hive-c0re/src/dashboard/mod.rs @@ -58,7 +58,9 @@ struct AppState { coord: Arc, /// HMAC-SHA256 secret shared with Forgejo webhook registrations. /// Verified on every incoming `/webhook/*` POST. - webhook_secret: String, + /// `None` when the secret could not be loaded at startup — all + /// `/webhook/*` requests are rejected with 503 in that case. + webhook_secret: Option, } #[allow( @@ -68,7 +70,11 @@ struct AppState { handler; splitting that exhaustive list across helpers would \ obscure the route map for no readability gain" )] -pub async fn serve(port: u16, coord: Arc, webhook_secret: String) -> Result<()> { +pub async fn serve( + port: u16, + coord: Arc, + webhook_secret: Option, +) -> Result<()> { // API-only: the gateway static-serves the dashboard dist and proxies // non-static requests here (see hive-gateway.nix). Unmatched paths 404. let app = Router::new() diff --git a/hive-c0re/src/dashboard/webhook.rs b/hive-c0re/src/dashboard/webhook.rs index 863c1a52..76ae260f 100644 --- a/hive-c0re/src/dashboard/webhook.rs +++ b/hive-c0re/src/dashboard/webhook.rs @@ -25,8 +25,13 @@ use super::AppState; // ── HMAC helper ─────────────────────────────────────────────────────────────── /// Verify the `X-Hub-Signature-256` header on an incoming Forgejo webhook. -/// Returns `Err` (with a safe-to-log message) on mismatch or missing header. +/// Returns `Err` (with a safe-to-log message) on mismatch, missing header, +/// or when the HMAC secret is unavailable (load failure at startup). fn verify_hmac(state: &AppState, headers: &HeaderMap, body: &Bytes) -> Result<(), String> { + let secret = state + .webhook_secret + .as_deref() + .ok_or_else(|| "webhook HMAC secret unavailable; endpoint disabled".to_owned())?; let sig = headers .get("x-hub-signature-256") .and_then(|v| v.to_str().ok()) @@ -34,8 +39,7 @@ fn verify_hmac(state: &AppState, headers: &HeaderMap, body: &Bytes) -> Result<() if sig.is_empty() { return Err("missing X-Hub-Signature-256 header".to_owned()); } - crate::webhook_secret::verify_signature(&state.webhook_secret, body, sig) - .map_err(|e| e.to_string()) + crate::webhook_secret::verify_signature(secret, body, sig).map_err(|e| e.to_string()) } // ── knowledge webhook ────────────────────────────────────────────────────────── @@ -72,7 +76,12 @@ pub(super) async fn post_webhook_knowledge( ) -> Response { if let Err(e) = verify_hmac(&state, &headers, &body) { tracing::warn!("webhook/knowledge: HMAC verification failed: {e}"); - return (StatusCode::UNAUTHORIZED, "signature mismatch").into_response(); + let status = if e.contains("unavailable") { + StatusCode::SERVICE_UNAVAILABLE + } else { + StatusCode::UNAUTHORIZED + }; + return (status, e).into_response(); } let payload = match serde_json::from_slice::(&body) { @@ -174,7 +183,12 @@ pub(super) async fn post_webhook_config_pr( ) -> Response { if let Err(e) = verify_hmac(&state, &headers, &body) { tracing::warn!("webhook/config-pr: HMAC verification failed: {e}"); - return (StatusCode::UNAUTHORIZED, "signature mismatch").into_response(); + let status = if e.contains("unavailable") { + StatusCode::SERVICE_UNAVAILABLE + } else { + StatusCode::UNAUTHORIZED + }; + return (status, e).into_response(); } let payload = match serde_json::from_slice::(&body) { diff --git a/hive-c0re/src/forge/mod.rs b/hive-c0re/src/forge/mod.rs index e718652e..79cf118f 100644 --- a/hive-c0re/src/forge/mod.rs +++ b/hive-c0re/src/forge/mod.rs @@ -358,6 +358,30 @@ pub async fn ensure_config_pr_webhook( tracing::debug!(%target_url, "forge: config-pr webhook already configured"); return Ok(()); } + // Delete stale hooks that point at our path but a different base + // (e.g. old loopback hooks from before the SSRF-bypass migration). + for h in &hooks { + let hook_url = h + .config + .as_ref() + .and_then(|c| c.get("url")) + .map_or("", String::as_str); + if hook_url.ends_with("/webhook/config-pr") + && hook_url != target_url + && let Some(id) = h.id + { + tracing::info!( + hook_url, + org = CONFIG_ORG, + "forge: deleting stale config-pr webhook (wrong base)" + ); + let _ = tokio::time::timeout( + HTTP_TIMEOUT, + client.org_delete_hook(CONFIG_ORG, id).send(), + ) + .await; + } + } } Err(e) => { tracing::debug!(error = %e, "forge: listing config-pr hooks failed; attempting create"); diff --git a/hive-c0re/src/main.rs b/hive-c0re/src/main.rs index 311b44dd..27b6c61a 100644 --- a/hive-c0re/src/main.rs +++ b/hive-c0re/src/main.rs @@ -306,11 +306,14 @@ async fn cmd_serve( // Webhook HMAC secret: load from state dir or generate on first run. // Used by both the webhook handlers (verification) and the Forgejo // hook registrations (so Forgejo signs deliveries with the same key). - let webhook_secret = match hive_c0re::webhook_secret::load_or_generate() { - Ok(s) => s, + let webhook_secret: Option = match hive_c0re::webhook_secret::load_or_generate() { + Ok(s) => Some(s), Err(e) => { - tracing::warn!(error = ?e, "webhook secret load/generate failed; webhooks will not verify HMAC"); - String::new() + tracing::error!( + error = ?e, + "webhook secret load/generate failed; /webhook/* endpoints disabled and hooks not registered" + ); + None } }; // Webhook setup: ensure Forgejo webhooks are registered for both @@ -319,9 +322,14 @@ async fn cmd_serve( // forge::ensure_all so the core token + repos + org are present. // URLs use the public hive domain (HYPERHIVE_HIVE_DOMAIN) so Forgejo // delivers through the gateway, bypassing the SSRF loopback guard. - // No-op when the core token or domain are absent. + // No-op when the core token or domain are absent, or when the HMAC + // secret is unavailable (load failure). let webhook_secret_reg = webhook_secret.clone(); tokio::spawn(async move { + let Some(webhook_secret_reg) = webhook_secret_reg else { + tracing::debug!("webhook secret unavailable; skipping hook registration"); + return; + }; let Some(token) = forge::core_token() else { return; }; diff --git a/hive-c0re/src/workers/knowledge.rs b/hive-c0re/src/workers/knowledge.rs index f8cf2489..4cd348e4 100644 --- a/hive-c0re/src/workers/knowledge.rs +++ b/hive-c0re/src/workers/knowledge.rs @@ -190,6 +190,26 @@ pub async fn ensure_webhook( tracing::debug!(%target_url, "knowledge: push webhook already configured"); return Ok(()); } + // Delete stale hooks that point at our path but a different base + // (e.g. old loopback hooks from before the SSRF-bypass migration). + for h in &hooks { + let hook_url = h + .config + .as_ref() + .and_then(|c| c.get("url")) + .map_or("", String::as_str); + if hook_url.ends_with("/webhook/knowledge") + && hook_url != target_url + && let Some(id) = h.id + { + tracing::info!(hook_url, "knowledge: deleting stale webhook (wrong base)"); + let _ = tokio::time::timeout( + HTTP_TIMEOUT, + client.repo_delete_hook(ORG, REPO, id).send(), + ) + .await; + } + } } Err(e) => { tracing::debug!(error = %e, "knowledge: listing hooks failed; attempting create");