fix(http,security): close 5 shipping blockers

* fix(http): correct web::Data<Arc<T>> handler signatures (6 sites)
  Six handlers in fusion.rs / audit.rs / ml.rs / model_upload.rs declared
  web::Data<Arc<T>> but http_server.rs registers them via
  web::Data::from(arc), which produces web::Data<T>. Every call to those
  endpoints would 500 with "Application data is not configured".

* fix(security): reject path-traversal in model upload manifest
  models.model and preprocessing.scaler_sidecar were joined into staging_dir
  and models_dir verbatim — a YAML value like "../../../etc/cron.d/evil"
  would let users:admin write outside the models tree. Added
  validate_manifest_basename() that rejects /, \, .., absolute paths.

* fix(security): redact bot token from Telegram error logs
  reqwest::Error's Display includes the request URL by default, and the
  Telegram URL embeds the bot token in its path. Call .without_url() on
  the error before wrapping it in NotificationError::TelegramRequestFailed
  so the token never reaches journal/stderr.

* fix(http,security): require users:admin + emit audit on DELETE /api/ml/models/current
  The endpoint reverted the active ML detector with no permission check
  and no audit trail, while the symmetric upload path required users:admin
  and emitted model_swap to the WORM chain. Added the same permission
  gate and a model_dormant audit entry capturing the pre-revert state.

* fix(http): return 200 + chain_intact:false on tampered audit chain
  verify_audit_log_chain returning AuditPrevHashMismatch / AuditRowHashMismatch
  is a successful detection, not a server failure — clients should treat
  tamper detection differently from transient DB outages. Reserve 500 for
  real connectivity errors.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
DaLaw2 2026-04-19 14:01:32 +08:00
parent 10505d88d1
commit a39dc01d65
5 changed files with 130 additions and 17 deletions

View File

@ -1,10 +1,10 @@
use std::sync::Arc;
use actix_web::{HttpResponse, Scope, web};
use crate::adapter::persistence::Database;
use crate::core::auth::extractor::AuthClaims;
use crate::interface::port::audit::AuditRepo;
use crate::model::error::Error;
use crate::model::error::database::DatabaseError;
pub fn initialize() -> Scope {
web::scope("/audit")
@ -39,14 +39,40 @@ async fn list_audit_logs(_auth: AuthClaims, db: web::Data<Database>) -> HttpResp
/// `--verify-audit-log` flag performs, so auditors can check chain
/// integrity without shell access. Any mismatch returns the offending
/// row id inside `error` so the dashboard can link straight to it.
async fn verify_chain(_auth: AuthClaims, audit: web::Data<Arc<dyn AuditRepo>>) -> HttpResponse {
async fn verify_chain(_auth: AuthClaims, audit: web::Data<dyn AuditRepo>) -> HttpResponse {
match audit.verify_audit_log_chain() {
Ok(count) => HttpResponse::Ok().json(serde_json::json!({
"chain_intact": true,
"verified": count,
})),
// Tamper detection is a successful verify outcome, not a server
// failure — return 200 with `chain_intact: false` so frontend
// retry/error handling treats real chain corruption as a
// distinct condition from transient DB connectivity issues.
Err(Error::Database(DatabaseError::AuditPrevHashMismatch { id, expected, found })) => {
HttpResponse::Ok().json(serde_json::json!({
"chain_intact": false,
"verified": 0,
"kind": "prev_hash_mismatch",
"id": id,
"expected": expected,
"found": found,
}))
}
Err(Error::Database(DatabaseError::AuditRowHashMismatch { id, computed, stored })) => {
HttpResponse::Ok().json(serde_json::json!({
"chain_intact": false,
"verified": 0,
"kind": "row_hash_mismatch",
"id": id,
"computed": computed,
"stored": stored,
}))
}
// Any other error is a real server-side failure (DB unreachable,
// query failed, IO) — keep 500 so monitoring / alerts fire.
Err(e) => HttpResponse::InternalServerError().json(serde_json::json!({
"chain_intact": false,
"chain_intact": null,
"verified": 0,
"error": e.to_string(),
})),

View File

@ -7,8 +7,6 @@
//! analysts can answer "why was this IP blocked?" without parsing
//! logs by hand.
use std::sync::Arc;
use actix_web::{HttpRequest, HttpResponse, Responder, Scope, web};
use crate::core::detection::metrics::FusionMetrics;
@ -38,7 +36,7 @@ pub fn initialize() -> Scope {
/// `GET /api/fusion/metrics` — lock-free snapshot of fusion counters and
/// derived rates. Drives the operator dashboard's "how well is fusion
/// working on my network?" view.
async fn get_metrics(metrics: web::Data<Arc<FusionMetrics>>) -> impl Responder {
async fn get_metrics(metrics: web::Data<FusionMetrics>) -> impl Responder {
HttpResponse::Ok().json(metrics.snapshot())
}
@ -46,7 +44,7 @@ async fn get_metrics(metrics: web::Data<Arc<FusionMetrics>>) -> impl Responder {
/// Scans the WORM audit chain for `fused_threat_emitted` entries that
/// match `src_ip`, returning them oldest-first so the UI can render a
/// chronological "why was this IP blocked" view.
async fn explain_ip(req: HttpRequest, audit: web::Data<Arc<dyn AuditRepo>>) -> impl Responder {
async fn explain_ip(req: HttpRequest, audit: web::Data<dyn AuditRepo>) -> impl Responder {
let src_ip = match req.match_info().get("src_ip") {
Some(ip) => ip.to_string(),
None => {

View File

@ -1,10 +1,27 @@
use std::sync::Arc;
use actix_web::{HttpResponse, Responder, Scope, web};
use crate::core::auth::extractor::AuthClaims;
use crate::core::ml::adapter::ModelSourceState;
use crate::core::ml::engine::Engine;
use crate::core::ml::inference::Inference;
use crate::infrastructure::communication_manager::CommunicationManager;
use crate::model::event::AuditEvent;
/// Permission required to forcibly revert the active ML source to dormant.
/// Mirrors the upload handler's gate so swap-out and revert are symmetric:
/// without this, anyone holding `ai_detection:write` could disable the
/// detector silently while the upload path required `users:admin`.
const DORMANT_REQUIRED_PERMISSION: &str = "users:admin";
/// Actor prefix recorded on the WORM chain when an admin reverts the ML
/// source. Matches the prefix used by `model_swap` so downstream filters
/// see both events in the same admin-action stream.
const AUDIT_ACTOR_SECURITY_ADMIN_PREFIX: &str = "SecurityAdmin";
/// Action recorded on the WORM chain when the ML source is forced
/// dormant via this endpoint. Stable wire string — UI/audit tooling
/// filters on it, paired with `model_swap` from the upload path.
const AUDIT_ACTION_MODEL_DORMANT: &str = "model_dormant";
pub fn initialize() -> Scope {
web::scope("/ml")
@ -30,7 +47,7 @@ async fn get_status(engine: web::Data<Engine>) -> impl Responder {
/// `GET /api/ml/models/current` — wire-format snapshot of the ML source
/// state the dashboard's ML Status panel renders.
async fn get_current_model(inference: web::Data<Arc<Inference>>) -> impl Responder {
async fn get_current_model(inference: web::Data<Inference>) -> impl Responder {
let status = inference.current_status();
let label = if status.is_active() {
"active"
@ -47,13 +64,41 @@ async fn get_current_model(inference: web::Data<Arc<Inference>>) -> impl Respond
/// `DELETE /api/ml/models/current` — admin action: force the ML source back
/// to dormant. No-op when already dormant so the client can retry idempotently.
async fn delete_current_model(inference: web::Data<Arc<Inference>>) -> impl Responder {
if inference.current_status().is_dormant() {
/// Requires `users:admin` (see `DORMANT_REQUIRED_PERMISSION`) and emits a
/// WORM `model_dormant` audit entry capturing the pre-revert state, mirroring
/// the upload path's `model_swap` so both swap-in and revert are auditable.
async fn delete_current_model(
inference: web::Data<Inference>,
comm: web::Data<CommunicationManager>,
claims: AuthClaims,
) -> impl Responder {
if !claims.permissions.iter().any(|p| p == DORMANT_REQUIRED_PERMISSION) {
return HttpResponse::Forbidden().json(serde_json::json!({
"error": format!("model dormant requires the {DORMANT_REQUIRED_PERMISSION} permission"),
}));
}
let before_status = inference.current_status();
if before_status.is_dormant() {
return HttpResponse::Ok().json(serde_json::json!({
"already_dormant": true,
}));
}
inference.swap_state(ModelSourceState::Dormant);
let audit_detail = serde_json::json!({
"before": serde_json::to_value(&before_status).unwrap_or(serde_json::Value::Null),
})
.to_string();
let _ = comm
.publish_event(AuditEvent {
actor: format!("{AUDIT_ACTOR_SECURITY_ADMIN_PREFIX}@{}", claims.username),
action: AUDIT_ACTION_MODEL_DORMANT.to_string(),
detail: audit_detail,
})
.await;
HttpResponse::Ok().json(serde_json::json!({
"already_dormant": false,
}))

View File

@ -19,7 +19,6 @@ use std::fs::File as StdFile;
use std::io;
use std::io::Read;
use std::path::{Path, PathBuf};
use std::sync::Arc;
use std::sync::atomic::{AtomicBool, Ordering};
use std::time::{Duration, SystemTime};
@ -132,7 +131,7 @@ pub fn initialize() -> Scope {
/// pre-swap ML source state. The staging directory is always torn
/// down on the way out, even on success (post-promote it's empty).
async fn upload(
app_config: web::Data<Arc<AppConfig>>,
app_config: web::Data<AppConfig>,
inference: web::Data<Inference>,
comm: web::Data<CommunicationManager>,
promote_lock: web::Data<PromoteGate>,
@ -406,6 +405,42 @@ pub fn sanitize_filename(raw: impl AsRef<str>) -> String {
}
}
/// Reject manifest-declared filenames that aren't single-segment basenames.
/// The multipart layer sanitizes client-sent part filenames (silent rewrite),
/// but manifest fields like `models.model` and `preprocessing.scaler_sidecar`
/// are user-controlled YAML — a value such as `../../../etc/cron.d/evil` would
/// otherwise flow into `staging_dir.join(..)` / `models_dir.join(..)` and let
/// `users:admin` write outside the models tree. Reject loudly rather than
/// silently rewriting so an operator who fat-fingered a path sees the failure.
fn validate_manifest_basename(field: &str, value: &str) -> Result<(), PromoteError> {
if value.is_empty() {
return Err(PromoteError::ManifestInvalid(format!(
"manifest field {field} is empty"
)));
}
if value.contains('/') || value.contains('\\') {
return Err(PromoteError::ManifestInvalid(format!(
"manifest field {field} must be a basename, not a path: {value:?}"
)));
}
if value == "." || value == ".." || value.contains("..") {
return Err(PromoteError::ManifestInvalid(format!(
"manifest field {field} must not contain path-traversal segments: {value:?}"
)));
}
if Path::new(value).is_absolute() {
return Err(PromoteError::ManifestInvalid(format!(
"manifest field {field} must be relative, not absolute: {value:?}"
)));
}
if Path::new(value).file_name().and_then(|s| s.to_str()) != Some(value) {
return Err(PromoteError::ManifestInvalid(format!(
"manifest field {field} must be a plain basename: {value:?}"
)));
}
Ok(())
}
/// Loose first-chunk heuristic. An ONNX protobuf starts with a varint
/// tag byte — the field=1 wire=varint (`0x08` for `ir_version`) and
/// field=1 wire=length-delimited (`0x0a`) patterns both occur in real
@ -501,6 +536,10 @@ async fn validate_and_promote(
.model
.clone()
.ok_or_else(|| PromoteError::ManifestInvalid("single-onnx adapters require models.model".to_string()))?;
validate_manifest_basename("models.model", &declared_onnx)?;
if let Some(ref pp) = manifest_preview.preprocessing {
validate_manifest_basename("preprocessing.scaler_sidecar", &pp.scaler_sidecar)?;
}
let uploaded_onnx = staging_dir.join(&summary.onnx_filename);
let staged_onnx = staging_dir.join(&declared_onnx);
@ -534,6 +573,7 @@ async fn validate_and_promote(
.map_err(|e| PromoteError::PromoteIo(format!("rename onnx into models/: {e}")))?;
if let Some(ref pp) = manifest.preprocessing {
validate_manifest_basename("preprocessing.scaler_sidecar", &pp.scaler_sidecar)?;
let src = staging_dir.join(&pp.scaler_sidecar);
let dst = models_dir.join(&pp.scaler_sidecar);
fs::rename(&src, &dst)

View File

@ -37,7 +37,7 @@ impl TelegramAdapter {
let client = Client::builder()
.timeout(Duration::from_secs(10))
.build()
.map_err(NotificationError::TelegramRequestFailed)?;
.map_err(|e| NotificationError::TelegramRequestFailed(e.without_url()))?;
Ok(Self {
client,
@ -150,7 +150,11 @@ impl TelegramAdapter {
if e.is_timeout() {
NotificationError::Timeout
} else {
NotificationError::TelegramRequestFailed(e)
// Strip URL — it embeds the bot token in the path
// (`/bot<TOKEN>/sendMessage`) and reqwest::Error's
// Display includes the full URL by default, which
// would leak the token into journal/error logs.
NotificationError::TelegramRequestFailed(e.without_url())
}
})?;