From 5d579c4cca0068cca0262284eb51ca7c0b6c1d83 Mon Sep 17 00:00:00 2001 From: Vasili Pascal Date: Wed, 9 Sep 2026 21:47:15 +0300 Subject: [PATCH 1/2] fix: send tool_call_id back to the AI, add IP ban API MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The tool-use loop never completed a single round: ToolRegistry::execute returned results labelled with the tool name instead of the id of the call they answered, so every request came back 400 ("'tool_call_id' of 'check_ip_status' not found in 'tool_calls' of previous message") and the analyzer silently fell back to the pattern analyzer. The id is now stamped in execute(), keeping all six executors unchanged. Bans were only inspectable over SQL. Adds GET /api/security/bans (optional status filter, capped limit) and DELETE /api/security/bans/{ip}, backed by list_offenses() and a new IpBanEngine::unban_ip() that removes the firewall rule, marks the offense released, and notifies — returning false for an address with no active block so the endpoint can answer 404. Also installs sqlite3 in the prod image, since inspecting the database required a package the image did not ship. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01PaYiQzdsosNr2C8L34ZJg1 --- docker/prod/Dockerfile | 2 +- src/api/security.rs | 187 +++++++++++++++++++++++++- src/database/repositories/offenses.rs | 35 +++++ src/ip_ban/engine.rs | 33 +++++ src/tools/mod.rs | 30 +++++ website/app/docs/page.tsx | 2 + 6 files changed, 287 insertions(+), 2 deletions(-) diff --git a/docker/prod/Dockerfile b/docker/prod/Dockerfile index a847df6..be29c2d 100644 --- a/docker/prod/Dockerfile +++ b/docker/prod/Dockerfile @@ -7,7 +7,7 @@ WORKDIR /app # The host's binaries are not reachable from here: network_mode host shares the # network namespace, not the filesystem, so the CLI must live in this image. RUN apt-get update && \ - apt-get install --no-install-recommends -y ca-certificates nftables iptables && \ + apt-get install --no-install-recommends -y ca-certificates sqlite3 nftables iptables && \ rm -rf /var/lib/apt/lists/* # copy binary and configuration files diff --git a/src/api/security.rs b/src/api/security.rs index 44a3945..7f676d5 100644 --- a/src/api/security.rs +++ b/src/api/security.rs @@ -1,8 +1,22 @@ //! Security API endpoints +use crate::database::repositories::offenses::{list_offenses, OffenseStatus}; use crate::database::{get_security_status_snapshot, DbPool, SecurityStatusSnapshot}; +use crate::ip_ban::{IpBanConfig, IpBanEngine}; use crate::models::api::security::SecurityStatusResponse; use actix_web::{web, HttpResponse, Responder}; +use serde::Deserialize; + +/// Upper bound on `?limit=`, so one request cannot pull the whole table. +const MAX_BAN_LIMIT: usize = 500; +const DEFAULT_BAN_LIMIT: usize = 100; + +#[derive(Debug, Deserialize)] +pub struct BanQuery { + /// active, blocked, or released. Case-insensitive; omit for all. + status: Option, + limit: Option, +} /// Get overall security status /// @@ -19,9 +33,76 @@ pub async fn get_security_status(pool: web::Data) -> impl Responder { } } +/// List IP ban offenses +/// +/// GET /api/security/bans?status=blocked&limit=100 +pub async fn list_bans(pool: web::Data, query: web::Query) -> impl Responder { + let status = match query.status.as_deref() { + None => None, + Some(raw) => match parse_offense_status(raw) { + Some(status) => Some(status), + None => { + return HttpResponse::BadRequest().json(serde_json::json!({ + "error": "Invalid status. Expected one of: active, blocked, released" + })) + } + }, + }; + let limit = query.limit.unwrap_or(DEFAULT_BAN_LIMIT).min(MAX_BAN_LIMIT); + + match list_offenses(pool.get_ref(), status, limit) { + Ok(offenses) => HttpResponse::Ok().json(offenses), + Err(err) => { + log::error!("Failed to list IP bans: {}", err); + HttpResponse::InternalServerError().json(serde_json::json!({ + "error": "Failed to list IP bans" + })) + } + } +} + +/// Release an IP ban ahead of its expiry +/// +/// DELETE /api/security/bans/{ip} +pub async fn delete_ban(pool: web::Data, path: web::Path) -> impl Responder { + let ip_address = path.into_inner(); + let engine = IpBanEngine::new(pool.get_ref().clone(), IpBanConfig::from_env()); + + match engine.unban_ip(&ip_address).await { + Ok(true) => HttpResponse::Ok().json(serde_json::json!({ + "ip_address": ip_address, + "status": "Released" + })), + Ok(false) => HttpResponse::NotFound().json(serde_json::json!({ + "error": format!("No active block for {}", ip_address) + })), + Err(err) => { + log::error!("Failed to release ban for {}: {}", ip_address, err); + HttpResponse::InternalServerError().json(serde_json::json!({ + "error": "Failed to release ban" + })) + } + } +} + +/// Accept the lowercase spellings a URL query would realistically use. +fn parse_offense_status(raw: &str) -> Option { + match raw.trim().to_ascii_lowercase().as_str() { + "active" => Some(OffenseStatus::Active), + "blocked" => Some(OffenseStatus::Blocked), + "released" => Some(OffenseStatus::Released), + _ => None, + } +} + /// Configure security routes pub fn configure_routes(cfg: &mut web::ServiceConfig) { - cfg.service(web::scope("/api/security").route("/status", web::get().to(get_security_status))); + cfg.service( + web::scope("/api/security") + .route("/status", web::get().to(get_security_status)) + .route("/bans", web::get().to(list_bans)) + .route("/bans/{ip}", web::delete().to(delete_ban)), + ); } pub(crate) fn build_security_status(pool: &DbPool) -> anyhow::Result { @@ -51,6 +132,110 @@ mod tests { use actix_web::{test, App}; use chrono::Utc; + fn insert_blocked_offense(pool: &DbPool, ip: &str) { + use crate::database::repositories::offenses::{insert_offense, mark_blocked, NewIpOffense}; + + insert_offense( + pool, + &NewIpOffense { + id: format!("offense-{ip}"), + ip_address: ip.to_string(), + source_type: "sniff".into(), + container_id: None, + first_seen: Utc::now(), + reason: "repeated offenses".into(), + metadata: None, + }, + ) + .unwrap(); + mark_blocked( + pool, + ip, + "sniff", + Utc::now() + chrono::Duration::minutes(30), + ) + .unwrap(); + } + + #[actix_rt::test] + async fn test_list_bans_returns_offenses() { + let pool = create_pool(":memory:").unwrap(); + init_database(&pool).unwrap(); + insert_blocked_offense(&pool, "46.224.127.228"); + + let app = test::init_service( + App::new() + .app_data(web::Data::new(pool)) + .configure(configure_routes), + ) + .await; + + let req = test::TestRequest::get() + .uri("/api/security/bans?status=blocked") + .to_request(); + let body: serde_json::Value = test::call_and_read_body_json(&app, req).await; + + assert_eq!(body.as_array().unwrap().len(), 1); + assert_eq!(body[0]["ip_address"], "46.224.127.228"); + assert_eq!(body[0]["status"], "Blocked"); + } + + #[actix_rt::test] + async fn test_list_bans_rejects_unknown_status() { + let pool = create_pool(":memory:").unwrap(); + init_database(&pool).unwrap(); + let app = test::init_service( + App::new() + .app_data(web::Data::new(pool)) + .configure(configure_routes), + ) + .await; + + let req = test::TestRequest::get() + .uri("/api/security/bans?status=banned") + .to_request(); + let resp = test::call_service(&app, req).await; + + assert_eq!(resp.status(), actix_web::http::StatusCode::BAD_REQUEST); + } + + #[actix_rt::test] + async fn test_delete_ban_returns_404_for_unknown_ip() { + let pool = create_pool(":memory:").unwrap(); + init_database(&pool).unwrap(); + let app = test::init_service( + App::new() + .app_data(web::Data::new(pool)) + .configure(configure_routes), + ) + .await; + + let req = test::TestRequest::delete() + .uri("/api/security/bans/203.0.113.99") + .to_request(); + let resp = test::call_service(&app, req).await; + + assert_eq!(resp.status(), actix_web::http::StatusCode::NOT_FOUND); + } + + // `#[test]` resolves to actix_web::test here, which requires async. + #[actix_rt::test] + async fn test_parse_offense_status_is_case_insensitive() { + assert_eq!( + parse_offense_status("Blocked"), + Some(OffenseStatus::Blocked) + ); + assert_eq!( + parse_offense_status(" active "), + Some(OffenseStatus::Active) + ); + assert_eq!( + parse_offense_status("released"), + Some(OffenseStatus::Released) + ); + assert_eq!(parse_offense_status("nonsense"), None); + } + #[actix_rt::test] async fn test_get_security_status() { let pool = create_pool(":memory:").unwrap(); diff --git a/src/database/repositories/offenses.rs b/src/database/repositories/offenses.rs index 143470d..2933940 100644 --- a/src/database/repositories/offenses.rs +++ b/src/database/repositories/offenses.rs @@ -212,6 +212,41 @@ pub fn expired_blocks(pool: &DbPool, now: DateTime) -> Result, + limit: usize, +) -> Result> { + let conn = pool.get()?; + let base = "SELECT + id, ip_address, source_type, container_id, offense_count, + first_seen, last_seen, blocked_until, status, reason, metadata + FROM ip_offenses"; + + let mut offenses = Vec::new(); + match status { + Some(status) => { + let mut stmt = conn.prepare(&format!( + "{base} WHERE status = ?1 ORDER BY last_seen DESC LIMIT ?2" + ))?; + let rows = stmt.query_map(params![status.to_string(), limit as i64], map_row)?; + for row in rows { + offenses.push(row?); + } + } + None => { + let mut stmt = conn.prepare(&format!("{base} ORDER BY last_seen DESC LIMIT ?1"))?; + let rows = stmt.query_map(params![limit as i64], map_row)?; + for row in rows { + offenses.push(row?); + } + } + } + + Ok(offenses) +} + pub fn mark_released(pool: &DbPool, offense_id: &str) -> Result<()> { let conn = pool.get()?; conn.execute( diff --git a/src/ip_ban/engine.rs b/src/ip_ban/engine.rs index e0986e1..faee635 100644 --- a/src/ip_ban/engine.rs +++ b/src/ip_ban/engine.rs @@ -76,6 +76,39 @@ impl IpBanEngine { Ok(false) } + /// Release a ban ahead of its expiry. + /// + /// Returns `false` when the address has no active block, so callers can + /// answer 404 rather than pretending something was undone. + pub async fn unban_ip(&self, ip_address: &str) -> Result { + let Some(offense) = active_block_for_ip(&self.pool, ip_address)? else { + return Ok(false); + }; + + #[cfg(target_os = "linux")] + self.with_firewall_backend(|backend| backend.unblock_ip(&offense.ip_address))?; + + mark_released(&self.pool, &offense.id)?; + let alert = create_alert( + &self.pool, + Alert::new( + AlertType::SystemEvent, + AlertSeverity::Info, + format!("Released IP ban for {}", offense.ip_address), + ) + .with_metadata( + AlertMetadata::default() + .with_source("ip_ban") + .with_reason(format!("Manually released ban for {}", offense.ip_address)), + ), + ) + .await?; + self.notify_action_alert(&alert, "STACKDOG_NOTIFY_IP_BAN_ACTIONS", "ip ban release") + .await; + + Ok(true) + } + pub async fn unban_expired(&self) -> Result { let now = Utc::now(); let expired = expired_blocks(&self.pool, now)?; diff --git a/src/tools/mod.rs b/src/tools/mod.rs index 395fb10..2b18f68 100644 --- a/src/tools/mod.rs +++ b/src/tools/mod.rs @@ -53,7 +53,18 @@ impl ToolRegistry { } /// Execute a tool call and return the result + /// + /// The individual executors label their results with the tool name, which is + /// convenient for logging but is not what the API wants back: a `tool` + /// message must carry the `id` of the originating call, or the provider + /// rejects the whole request. Stamping it here keeps every executor honest. pub async fn execute(&self, call: &ToolCall) -> ToolResult { + let mut result = self.dispatch(call).await; + result.tool_call_id = call.id.clone(); + result + } + + async fn dispatch(&self, call: &ToolCall) -> ToolResult { let args = &call.function.arguments; match call.function.name.as_str() { "check_ip_status" => ip_ban::execute_check_ip_status(&self.pool, args), @@ -110,6 +121,25 @@ mod tests { }; let result = registry.execute(&call).await; assert!(result.content.contains("Unknown tool")); + assert_eq!(result.tool_call_id, "call_1"); + } + + #[actix_rt::test] + async fn test_execute_returns_call_id_not_tool_name() { + let registry = make_registry(); + let call = ToolCall { + id: "call_abc123".into(), + call_type: "function".into(), + function: types::FunctionCall { + name: "check_ip_status".into(), + arguments: r#"{"ip_address":"203.0.113.10"}"#.into(), + }, + }; + + // The API rejects the request when a tool message references anything + // other than the id of the call it answers. + let result = registry.execute(&call).await; + assert_eq!(result.tool_call_id, "call_abc123"); } #[actix_rt::test] diff --git a/website/app/docs/page.tsx b/website/app/docs/page.tsx index 4ce7e70..a60b3f0 100644 --- a/website/app/docs/page.tsx +++ b/website/app/docs/page.tsx @@ -54,6 +54,8 @@ const apiRows = [ ['GET', '/api/containers', 'Container inventory and runtime state.'], ['GET', '/api/logs/sources', 'Registered log sources for sniffing.'], ['GET', '/api/logs/summaries', 'AI-generated log summaries and findings.'], + ['GET', '/api/security/bans', 'IP ban offenses. Filter with ?status=active|blocked|released and ?limit=.'], + ['DELETE', '/api/security/bans/{ip}', 'Release a ban ahead of its expiry.'], ['WS', '/ws', 'Real-time event stream over WebSocket.'] ] as const; From 051f2dc5940f49f72012a279d65971e4a5a48870 Mon Sep 17 00:00:00 2001 From: Vasili Pascal Date: Thu, 10 Sep 2026 08:48:19 +0300 Subject: [PATCH 2/2] fix: name containers by their name, not their hostname get_container_info built ContainerInfo::name from Config.Hostname, which is never what `docker ps` shows: under network_mode host it is the host's hostname (alerts read "docker:try", the server's name), and otherwise Docker defaults it to the short container ID (alerts read "container 2ef2d927f45a [2ef2d927f45a]", the ID twice). Uses the inspect name instead, trimming Docker's leading slash, and keeps hostname then short ID as fallbacks so there is always something to print. This also repairs mail guard targeting: its patterns ("wordpress", "php", "apache") are matched against ContainerInfo::name, and could not match a hostname or a short ID. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01PaYiQzdsosNr2C8L34ZJg1 --- src/docker/client.rs | 59 +++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 56 insertions(+), 3 deletions(-) diff --git a/src/docker/client.rs b/src/docker/client.rs index 2a7476c..67cda0d 100644 --- a/src/docker/client.rs +++ b/src/docker/client.rs @@ -55,6 +55,11 @@ impl DockerClient { } /// Get container info by ID + /// + /// The name comes from the container's own name, not its hostname: under + /// `network_mode: host` the hostname is the host's, and otherwise Docker + /// defaults it to the short ID — so hostname never yields what `docker ps` + /// shows. pub async fn get_container_info(&self, container_id: &str) -> Result { let inspect = self .client @@ -62,14 +67,17 @@ impl DockerClient { .await .context("Failed to inspect container")?; + let name = container_display_name( + inspect.name.as_deref(), + inspect.config.as_ref().and_then(|c| c.hostname.as_deref()), + container_id, + ); let config = inspect.config.unwrap_or_default(); let state = inspect.state.unwrap_or_default(); Ok(ContainerInfo { id: container_id.to_string(), - name: config - .hostname - .unwrap_or_else(|| container_id[..12].to_string()), + name, image: config.image.unwrap_or_else(|| "unknown".to_string()), status: if state.running.unwrap_or(false) { "Running" @@ -272,6 +280,24 @@ pub struct ContainerInfo { pub labels: HashMap, } +/// Pick the name to show for a container. +/// +/// Docker returns the inspect name with a leading slash ("/redis"). Falls back +/// to the hostname, then to the short ID, so there is always something to print. +fn container_display_name( + inspect_name: Option<&str>, + hostname: Option<&str>, + container_id: &str, +) -> String { + inspect_name + .map(|name| name.trim_start_matches('/')) + .filter(|name| !name.is_empty()) + .or(hostname) + .filter(|name| !name.is_empty()) + .map(str::to_string) + .unwrap_or_else(|| container_id.chars().take(12).collect()) +} + /// Container statistics #[derive(Debug, Clone, Default)] pub struct ContainerStats { @@ -288,6 +314,33 @@ pub struct ContainerStats { mod tests { use super::*; + #[test] + fn test_container_display_name_prefers_real_name() { + assert_eq!( + container_display_name(Some("/redis"), Some("0f3b46ca0c16"), "0f3b46ca0c16aaaa"), + "redis" + ); + } + + #[test] + fn test_container_display_name_falls_back_to_hostname_then_id() { + // No name from inspect: hostname is the next best thing. + assert_eq!( + container_display_name(None, Some("web-01"), "0f3b46ca0c16aaaa"), + "web-01" + ); + + // Neither available: short ID keeps the output usable. + assert_eq!( + container_display_name(None, None, "0f3b46ca0c16aaaa"), + "0f3b46ca0c16" + ); + assert_eq!( + container_display_name(Some("/"), Some(""), "0f3b46ca0c16aaaa"), + "0f3b46ca0c16" + ); + } + #[actix_rt::test] async fn test_docker_client_creation() { // This test requires Docker daemon running