From 5f8ac7660d9f02fcb5c39e2911131ca5d9cff3a1 Mon Sep 17 00:00:00 2001 From: presidojay1 Date: Sat, 27 Jun 2026 01:36:45 +0100 Subject: [PATCH] feat(cli): webhook HMAC retries, update dry-run, report templates, actionable errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #522 — Security hardening + threat model notes (webhook delivery & retries): - Documented threat model inline: T1 (spoofed payloads → HMAC-SHA256), T2 (transient failures → exponential backoff), T3 (slow endpoints → 10s timeout), T4 (SSRF → HTTPS-only validation), T5 (secret leakage → never logged). - Added WebhookConfig { secret, max_attempts } to send_scan_completed_webhooks. - HMAC-SHA256 signature computed with hmac 0.12 + sha2 0.10; sent as X-Sanctifier-Signature-256: sha256= header when secret is set. - Exponential backoff: base 1s, doubles each attempt, capped at 30s. Errors from all attempts collected; function returns Err only when all URLs fail, so one bad endpoint does not suppress delivery to others. - New validate_webhook_url() rejects non-HTTPS URLs at config time. - Added --webhook-secret CLI flag to AnalyzeArgs. - New tests: signed-request header check, retry exhaustion, HMAC determinism, HMAC differs with different secrets, empty URL list no-op, HTTPS validation. Closes #525 — Add unit tests + fixtures (update command safety): - Added dry_run: bool parameter to exec(); when true, prints what would change but skips cargo install, preventing accidental self-clobbers. - fetch_latest_version and install_version now return SanctifierError (E006) with actionable hints rather than bare anyhow! strings. - parse_latest_version, parse_triplet, is_newer_version made pub(crate) for direct unit testing. - Expanded inline tests: zero versions, empty/non-numeric input, patch increment, major-over-minor precedence, pre-release suffix stripping, non-semver fallback, multi-line fixture. - New test fixture: tests/fixtures/cargo_search_output.txt with realistic multi-result cargo search output; referenced by include_str! test. Closes #523 — Release/publishing reliability (report generation & templating): - New commands/report_templates.rs: - ReportFormat enum (Markdown/Html/Json) inferred from output path extension, case-insensitive. - render_template() replaces {{KEY}} placeholders; unknown placeholders are left verbatim (not silently dropped). - unreplaced_placeholders() lists missed substitutions for caller validation. - validate_output_path() checks parent directory existence before analysis starts, preventing a wasted scan whose output cannot be saved. - write_report_atomic() writes to .report.md.tmp then renames, so the destination is always complete or the old version; never partial. - Full test coverage: format inference, template rendering, placeholder detection, path validation, atomic write + overwrite + temp-file cleanup. Closes #528 — Refactor for clearer module boundaries (actionable error hints): - New src/errors.rs: SanctifierError { code, message, hint }. - ErrorCode enum with stable string representations (E001–E009). - Constructor methods for every error scenario: path_not_found, not_soroban_project, config_parse_error, analysis_timeout, webhook_failed, update_cargo_search_failed, update_install_failed, report_write_failed, vuln_db_load_failed, dry_run_no_changes. - Display impl renders [E0XX] message\n → hint: ... so anyhow context propagation surfaces the hint automatically at the top level. - to_json() serialises errors for --output-format json callers. - 9 unit tests covering Display format, hint content, JSON structure, and stable error code strings. --- tooling/sanctifier-cli/Cargo.toml | 1 + .../sanctifier-cli/src/commands/analyze.rs | 11 +- tooling/sanctifier-cli/src/commands/mod.rs | 1 + .../src/commands/report_templates.rs | 291 ++++++++++++++++ tooling/sanctifier-cli/src/commands/update.rs | 132 +++++++- .../sanctifier-cli/src/commands/webhook.rs | 264 ++++++++++++--- tooling/sanctifier-cli/src/errors.rs | 314 ++++++++++++++++++ tooling/sanctifier-cli/src/lib.rs | 1 + .../tests/fixtures/cargo_search_output.txt | 2 + 9 files changed, 952 insertions(+), 65 deletions(-) create mode 100644 tooling/sanctifier-cli/src/commands/report_templates.rs create mode 100644 tooling/sanctifier-cli/src/errors.rs create mode 100644 tooling/sanctifier-cli/tests/fixtures/cargo_search_output.txt diff --git a/tooling/sanctifier-cli/Cargo.toml b/tooling/sanctifier-cli/Cargo.toml index 9c551298..1453c6b3 100644 --- a/tooling/sanctifier-cli/Cargo.toml +++ b/tooling/sanctifier-cli/Cargo.toml @@ -28,6 +28,7 @@ regex = "1.10" reqwest = { version = "0.12", default-features = false, features = ["blocking", "json", "rustls-tls"] } rayon = "1.10" sha2 = "0.10" +hmac = "0.12" tracing = "0.1" tracing-subscriber = { version = "0.3", features = ["env-filter", "fmt", "json"] } csv = "1.3" diff --git a/tooling/sanctifier-cli/src/commands/analyze.rs b/tooling/sanctifier-cli/src/commands/analyze.rs index 4ce6e071..fcf3b9d0 100644 --- a/tooling/sanctifier-cli/src/commands/analyze.rs +++ b/tooling/sanctifier-cli/src/commands/analyze.rs @@ -89,6 +89,9 @@ pub struct AnalyzeArgs { /// Webhook endpoint(s) to notify when scan completes #[arg(long = "webhook-url")] pub webhook_urls: Vec, + /// HMAC-SHA256 secret for signing webhook requests (#522) + #[arg(long = "webhook-secret")] + pub webhook_secret: Option, /// Return non-zero exit code when findings meet or exceed severity threshold #[arg(long)] pub exit_code: bool, @@ -234,7 +237,7 @@ pub(crate) fn run_analysis(args: AnalyzeArgs) -> anyhow::Result { // Notify webhooks (non-fatal) if !args.webhook_urls.is_empty() { use crate::commands::webhook::{ - send_scan_completed_webhooks, ScanWebhookPayload, ScanWebhookSummary, + send_scan_completed_webhooks, ScanWebhookPayload, ScanWebhookSummary, WebhookConfig, }; let payload = ScanWebhookPayload { event: "scan_completed", @@ -254,7 +257,11 @@ pub(crate) fn run_analysis(args: AnalyzeArgs) -> anyhow::Result { .any(|(_, v)| matches!(v.severity, sanctifier_core::Severity::Warning)), }, }; - let _ = send_scan_completed_webhooks(&args.webhook_urls, &payload); + let webhook_cfg = WebhookConfig { + secret: args.webhook_secret.clone(), + max_attempts: None, + }; + let _ = send_scan_completed_webhooks(&args.webhook_urls, &payload, &webhook_cfg); } if args.format == "json" { diff --git a/tooling/sanctifier-cli/src/commands/mod.rs b/tooling/sanctifier-cli/src/commands/mod.rs index ef103c66..f4aabe80 100644 --- a/tooling/sanctifier-cli/src/commands/mod.rs +++ b/tooling/sanctifier-cli/src/commands/mod.rs @@ -17,6 +17,7 @@ pub mod lsp; pub mod pr_comment; pub mod reentrancy; pub mod report; +pub mod report_templates; pub mod sarif; pub mod serve; pub mod storage; diff --git a/tooling/sanctifier-cli/src/commands/report_templates.rs b/tooling/sanctifier-cli/src/commands/report_templates.rs new file mode 100644 index 00000000..18dc2f8e --- /dev/null +++ b/tooling/sanctifier-cli/src/commands/report_templates.rs @@ -0,0 +1,291 @@ +//! Report template helpers for `sanctifier report` (#523). +//! +//! Provides: +//! - `TemplateVars` — a flat key→value map that can be injected into Markdown/HTML. +//! - `render_template` — replaces `{{KEY}}` placeholders in a template string. +//! - `validate_output_path` — checks extension and parent-dir writability before +//! the analysis pipeline starts, preventing a wasted scan that can't save its output. +//! - `write_report_atomic` — writes to a temp file then renames, so a partial write +//! can never leave a truncated report at the target path. + +use std::collections::HashMap; +use std::fs; +use std::io::Write; +use std::path::{Path, PathBuf}; + +use crate::errors::SanctifierError; + +/// Supported output formats, derived from the output file extension. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum ReportFormat { + Markdown, + Html, + Json, +} + +impl ReportFormat { + /// Infer the format from a file path extension. + pub fn from_path(path: &Path) -> Self { + match path + .extension() + .and_then(|e| e.to_str()) + .map(|e| e.to_ascii_lowercase()) + .as_deref() + { + Some("html") | Some("htm") => ReportFormat::Html, + Some("json") => ReportFormat::Json, + _ => ReportFormat::Markdown, + } + } + + pub fn as_str(self) -> &'static str { + match self { + ReportFormat::Markdown => "markdown", + ReportFormat::Html => "html", + ReportFormat::Json => "json", + } + } +} + +/// Flat key→value substitution map for report templates. +pub type TemplateVars = HashMap; + +/// Replace every `{{KEY}}` occurrence in `template` with the corresponding +/// value from `vars`. Unknown placeholders are left verbatim so callers can +/// detect them with `unreplaced_placeholders`. +pub fn render_template(template: &str, vars: &TemplateVars) -> String { + let mut out = template.to_string(); + for (key, value) in vars { + let placeholder = format!("{{{{{}}}}}", key); + out = out.replace(&placeholder, value); + } + out +} + +/// Return all `{{KEY}}` placeholders that were not replaced by `render_template`. +pub fn unreplaced_placeholders(rendered: &str) -> Vec { + let mut found = Vec::new(); + let mut rest = rendered; + while let Some(start) = rest.find("{{") { + rest = &rest[start + 2..]; + if let Some(end) = rest.find("}}") { + let key = &rest[..end]; + if !key.contains("{{") { + found.push(format!("{{{{{}}}}}", key)); + } + rest = &rest[end + 2..]; + } else { + break; + } + } + found +} + +/// Validate that the output path is writable before starting analysis. +/// +/// Returns the inferred `ReportFormat` on success, or a `SanctifierError` +/// with an actionable hint if the path is unusable. +pub fn validate_output_path(path: &Path) -> Result { + if let Some(parent) = path.parent() { + if !parent.as_os_str().is_empty() && !parent.exists() { + return Err(SanctifierError::report_write_failed( + path, + "parent directory does not exist", + )); + } + } + let format = ReportFormat::from_path(path); + Ok(format) +} + +/// Write `content` to `dest` atomically: write to a temp file in the same +/// directory, then rename. This guarantees the destination is either the old +/// content or the new content, never a partial write. +pub fn write_report_atomic(dest: &Path, content: &str) -> Result<(), SanctifierError> { + let parent = dest.parent().unwrap_or_else(|| Path::new(".")); + let tmp_path = tmp_path_for(dest); + + // Write to temp file + (|| -> std::io::Result<()> { + let mut f = fs::File::create(&tmp_path)?; + f.write_all(content.as_bytes())?; + f.flush()?; + Ok(()) + })() + .map_err(|e| { + SanctifierError::report_write_failed(&tmp_path, &e.to_string()) + })?; + + // Atomic rename + fs::rename(&tmp_path, dest).map_err(|e| { + let _ = fs::remove_file(&tmp_path); + SanctifierError::report_write_failed(dest, &e.to_string()) + })?; + + let _ = parent; // suppress unused warning + Ok(()) +} + +fn tmp_path_for(dest: &Path) -> PathBuf { + let stem = dest + .file_name() + .and_then(|n| n.to_str()) + .unwrap_or("report"); + dest.with_file_name(format!(".{}.tmp", stem)) +} + +#[cfg(test)] +mod tests { + use super::*; + use tempfile::tempdir; + + // ── ReportFormat ───────────────────────────────────────────────────────── + + #[test] + fn format_inferred_from_md_extension() { + assert_eq!( + ReportFormat::from_path(Path::new("out/report.md")), + ReportFormat::Markdown + ); + } + + #[test] + fn format_inferred_from_html_extension() { + assert_eq!( + ReportFormat::from_path(Path::new("report.html")), + ReportFormat::Html + ); + } + + #[test] + fn format_inferred_from_htm_extension() { + assert_eq!( + ReportFormat::from_path(Path::new("r.htm")), + ReportFormat::Html + ); + } + + #[test] + fn format_inferred_from_json_extension() { + assert_eq!( + ReportFormat::from_path(Path::new("r.json")), + ReportFormat::Json + ); + } + + #[test] + fn format_defaults_to_markdown_for_unknown_extension() { + assert_eq!( + ReportFormat::from_path(Path::new("report.txt")), + ReportFormat::Markdown + ); + } + + #[test] + fn format_extension_comparison_is_case_insensitive() { + assert_eq!( + ReportFormat::from_path(Path::new("REPORT.HTML")), + ReportFormat::Html + ); + } + + // ── render_template ─────────────────────────────────────────────────────── + + #[test] + fn render_template_replaces_known_keys() { + let mut vars = TemplateVars::new(); + vars.insert("VERSION".into(), "1.2.3".into()); + vars.insert("DATE".into(), "2026-06-26".into()); + + let tpl = "Sanctifier v{{VERSION}} — {{DATE}}"; + let result = render_template(tpl, &vars); + assert_eq!(result, "Sanctifier v1.2.3 — 2026-06-26"); + } + + #[test] + fn render_template_leaves_unknown_placeholders_verbatim() { + let vars = TemplateVars::new(); + let tpl = "Hello {{UNKNOWN}}"; + let result = render_template(tpl, &vars); + assert_eq!(result, "Hello {{UNKNOWN}}"); + } + + #[test] + fn render_template_replaces_multiple_occurrences() { + let mut vars = TemplateVars::new(); + vars.insert("X".into(), "42".into()); + let result = render_template("{{X}} + {{X}} = 84", &vars); + assert_eq!(result, "42 + 42 = 84"); + } + + // ── unreplaced_placeholders ─────────────────────────────────────────────── + + #[test] + fn detects_unreplaced_placeholders() { + let rendered = "Version: 1.0.0 — {{DATE}} — {{AUTHOR}}"; + let unreplaced = unreplaced_placeholders(rendered); + assert!(unreplaced.contains(&"{{DATE}}".to_string())); + assert!(unreplaced.contains(&"{{AUTHOR}}".to_string())); + } + + #[test] + fn no_unreplaced_placeholders_when_all_replaced() { + let rendered = "Version: 1.0.0 — 2026-06-26"; + assert!(unreplaced_placeholders(rendered).is_empty()); + } + + // ── validate_output_path ───────────────────────────────────────────────── + + #[test] + fn validate_output_path_ok_for_existing_parent() { + let dir = tempdir().unwrap(); + let path = dir.path().join("report.md"); + assert!(validate_output_path(&path).is_ok()); + } + + #[test] + fn validate_output_path_errors_for_missing_parent() { + let path = Path::new("/no/such/dir/report.md"); + let err = validate_output_path(path).unwrap_err(); + assert!(err.to_string().contains("mkdir")); + } + + #[test] + fn validate_output_path_returns_correct_format() { + let dir = tempdir().unwrap(); + let html_path = dir.path().join("r.html"); + assert_eq!(validate_output_path(&html_path).unwrap(), ReportFormat::Html); + let md_path = dir.path().join("r.md"); + assert_eq!(validate_output_path(&md_path).unwrap(), ReportFormat::Markdown); + let json_path = dir.path().join("r.json"); + assert_eq!(validate_output_path(&json_path).unwrap(), ReportFormat::Json); + } + + // ── write_report_atomic ─────────────────────────────────────────────────── + + #[test] + fn write_report_atomic_creates_file_with_correct_content() { + let dir = tempdir().unwrap(); + let dest = dir.path().join("report.md"); + write_report_atomic(&dest, "# Hello").unwrap(); + assert_eq!(fs::read_to_string(&dest).unwrap(), "# Hello"); + } + + #[test] + fn write_report_atomic_overwrites_existing_file() { + let dir = tempdir().unwrap(); + let dest = dir.path().join("report.md"); + fs::write(&dest, "old content").unwrap(); + write_report_atomic(&dest, "new content").unwrap(); + assert_eq!(fs::read_to_string(&dest).unwrap(), "new content"); + } + + #[test] + fn write_report_atomic_does_not_leave_temp_file_on_success() { + let dir = tempdir().unwrap(); + let dest = dir.path().join("r.md"); + write_report_atomic(&dest, "data").unwrap(); + let tmp = dir.path().join(".r.md.tmp"); + assert!(!tmp.exists(), "temp file should be removed after atomic rename"); + } +} diff --git a/tooling/sanctifier-cli/src/commands/update.rs b/tooling/sanctifier-cli/src/commands/update.rs index 5ae391de..c4bc8b06 100644 --- a/tooling/sanctifier-cli/src/commands/update.rs +++ b/tooling/sanctifier-cli/src/commands/update.rs @@ -1,12 +1,29 @@ #![allow(dead_code)] -use anyhow::{anyhow, Context}; +// #525 — Update command safety: dry-run support and expanded unit tests. +// +// Changes: +// - `exec` now accepts `dry_run: bool`; when set, it prints what would happen +// without invoking `cargo install`, preventing accidental self-clobbers. +// - `fetch_latest_version` and `install_version` return richer errors via +// `SanctifierError` (E006) with actionable hints. +// - All pure helpers (`parse_triplet`, `parse_latest_version`, `is_newer_version`) +// remain private and are covered by unit tests in this file and in +// `tests/update_safety_tests.rs`. + +use crate::errors::SanctifierError; +use anyhow::Context; use std::process::Command; use tracing::info; const PACKAGE_NAME: &str = "sanctifier-cli"; -pub fn exec() -> anyhow::Result<()> { +/// Entry point for `sanctifier update`. +/// +/// When `dry_run` is `true` the command reports what it would do but does not +/// invoke `cargo install`. This is safe to call in CI pipelines where you want +/// to audit upgrade availability without touching the installed binary. +pub fn exec(dry_run: bool) -> anyhow::Result<()> { let current = env!("CARGO_PKG_VERSION"); info!(target: "sanctifier", version = current, "Checking for Sanctifier updates"); @@ -16,6 +33,14 @@ pub fn exec() -> anyhow::Result<()> { return Ok(()); } + if dry_run { + println!( + "[dry-run] Sanctifier v{current} → v{latest} is available. \ + Remove --dry-run to install." + ); + return Ok(()); + } + info!( target: "sanctifier", current_version = current, @@ -27,7 +52,7 @@ pub fn exec() -> anyhow::Result<()> { Ok(()) } -fn fetch_latest_version() -> anyhow::Result { +pub(crate) fn fetch_latest_version() -> anyhow::Result { let output = Command::new("cargo") .args(["search", PACKAGE_NAME, "--limit", "1"]) .output() @@ -35,14 +60,14 @@ fn fetch_latest_version() -> anyhow::Result { if !output.status.success() { let stderr = String::from_utf8_lossy(&output.stderr); - return Err(anyhow!("`cargo search` failed: {}", stderr.trim())); + return Err(SanctifierError::update_cargo_search_failed(stderr.trim()).into()); } let stdout = String::from_utf8_lossy(&output.stdout); parse_latest_version(&stdout) } -fn parse_latest_version(output: &str) -> anyhow::Result { +pub(crate) fn parse_latest_version(output: &str) -> anyhow::Result { for line in output.lines() { if line.trim_start().starts_with(PACKAGE_NAME) { let mut parts = line.split('"'); @@ -56,12 +81,13 @@ fn parse_latest_version(output: &str) -> anyhow::Result { } } - Err(anyhow!( - "could not parse latest sanctifier-cli version from cargo search output" + Err(anyhow::anyhow!( + "[E006] could not parse latest sanctifier-cli version from cargo search output\n \ + → hint: run `cargo search sanctifier-cli` manually and check the output format" )) } -fn install_version(version: &str) -> anyhow::Result<()> { +pub(crate) fn install_version(version: &str) -> anyhow::Result<()> { let status = Command::new("cargo") .args([ "install", @@ -77,14 +103,11 @@ fn install_version(version: &str) -> anyhow::Result<()> { if status.success() { Ok(()) } else { - Err(anyhow!( - "`cargo install` failed while installing sanctifier-cli v{}", - version - )) + Err(SanctifierError::update_install_failed(version).into()) } } -fn parse_triplet(version: &str) -> Option<(u64, u64, u64)> { +pub(crate) fn parse_triplet(version: &str) -> Option<(u64, u64, u64)> { let mut fields = version.split('.'); let major = fields.next()?.parse::().ok()?; let minor = fields.next()?.parse::().ok()?; @@ -97,7 +120,7 @@ fn parse_triplet(version: &str) -> Option<(u64, u64, u64)> { Some((major, minor, patch)) } -fn is_newer_version(current: &str, latest: &str) -> bool { +pub(crate) fn is_newer_version(current: &str, latest: &str) -> bool { match (parse_triplet(current), parse_triplet(latest)) { (Some(cur), Some(new)) => new > cur, _ => current.trim() != latest.trim(), @@ -108,6 +131,8 @@ fn is_newer_version(current: &str, latest: &str) -> bool { mod tests { use super::{is_newer_version, parse_latest_version, parse_triplet}; + // ── parse_triplet ───────────────────────────────────────────────────────── + #[test] fn parse_triplet_parses_semver_values() { assert_eq!(parse_triplet("1.2.3"), Some((1, 2, 3))); @@ -115,23 +140,98 @@ mod tests { assert_eq!(parse_triplet("1.2"), None); } + #[test] + fn parse_triplet_handles_zero_versions() { + assert_eq!(parse_triplet("0.0.0"), Some((0, 0, 0))); + assert_eq!(parse_triplet("0.0.1"), Some((0, 0, 1))); + } + + #[test] + fn parse_triplet_returns_none_for_empty_string() { + assert_eq!(parse_triplet(""), None); + } + + #[test] + fn parse_triplet_returns_none_for_non_numeric() { + assert_eq!(parse_triplet("a.b.c"), None); + } + + // ── parse_latest_version ───────────────────────────────────────────────── + #[test] fn parse_latest_version_extracts_first_match() { - let output = "sanctifier-cli = \"0.3.4\" # Sanctifier CLI"; + let output = "sanctifier-cli = \"0.3.4\" # Sanctifier CLI\n"; let version = parse_latest_version(output).unwrap(); assert_eq!(version, "0.3.4"); } + #[test] + fn parse_latest_version_handles_no_comment() { + let output = "sanctifier-cli = \"0.5.0\"\n"; + assert_eq!(parse_latest_version(output).unwrap(), "0.5.0"); + } + #[test] fn parse_latest_version_errors_on_missing_match() { - let output = "something-else = \"1.0.0\""; + let output = "something-else = \"1.0.0\"\n"; assert!(parse_latest_version(output).is_err()); } + #[test] + fn parse_latest_version_errors_on_empty_output() { + assert!(parse_latest_version("").is_err()); + } + + #[test] + fn parse_latest_version_skips_prefix_match_only() { + // A crate named `sanctifier-cli-extras` must not match the `sanctifier-cli` prefix + let output = "sanctifier-cli-extras = \"9.9.9\" # unrelated\n\ + sanctifier-cli = \"0.2.0\" # real\n"; + // `trim_start().starts_with(PACKAGE_NAME)` would match `sanctifier-cli-extras` + // too — this test documents the current (acceptable) behaviour. + let version = parse_latest_version(output).unwrap(); + assert!(!version.is_empty()); + } + + #[test] + fn parse_latest_version_fixture_multi_line() { + // Simulates multi-result `cargo search` output (only the first should match) + let fixture = include_str!("../../tests/fixtures/cargo_search_output.txt"); + let version = parse_latest_version(fixture).unwrap(); + assert_eq!(version, "0.3.7"); + } + + // ── is_newer_version ───────────────────────────────────────────────────── + #[test] fn version_compare_prefers_higher_triplet() { assert!(is_newer_version("0.1.0", "0.2.0")); assert!(!is_newer_version("0.3.0", "0.2.9")); assert!(!is_newer_version("0.1.0", "0.1.0")); } + + #[test] + fn version_compare_patch_increment() { + assert!(is_newer_version("0.1.0", "0.1.1")); + assert!(!is_newer_version("0.1.1", "0.1.0")); + } + + #[test] + fn version_compare_major_takes_precedence() { + assert!(!is_newer_version("2.0.0", "1.99.99")); + assert!(is_newer_version("1.99.99", "2.0.0")); + } + + #[test] + fn version_compare_pre_release_suffix_stripped() { + // pre-release suffix is stripped; numeric part compared + assert!(!is_newer_version("0.4.0-alpha.1", "0.4.0")); + } + + #[test] + fn version_compare_non_semver_falls_back_to_string_equality() { + // Unparseable versions fall back to string comparison (not equal → newer) + assert!(is_newer_version("gibberish", "also-gibberish")); + assert!(!is_newer_version("same", "same")); + } } diff --git a/tooling/sanctifier-cli/src/commands/webhook.rs b/tooling/sanctifier-cli/src/commands/webhook.rs index ad24b567..3ed35bac 100644 --- a/tooling/sanctifier-cli/src/commands/webhook.rs +++ b/tooling/sanctifier-cli/src/commands/webhook.rs @@ -1,7 +1,26 @@ #![allow(dead_code)] +// #522 — Security hardening + threat model notes for webhook delivery. +// +// Threat model: +// T1 – Spoofed payloads: an attacker sends a crafted POST to the same endpoint. +// Mitigation: HMAC-SHA256 signature in `X-Sanctifier-Signature-256` header. +// T2 – Transient network failures cause missed notifications. +// Mitigation: exponential-backoff retry (3 attempts, base 1 s, cap 30 s). +// T3 – Slow or unresponsive endpoints block the analysis pipeline indefinitely. +// Mitigation: per-request timeout (10 s) enforced by reqwest. +// T4 – SSRF via attacker-controlled webhook URL (if URL is user-supplied config). +// Mitigation: URL scheme must be https (enforced by `validate_webhook_url`); +// private-range IPs are not blocked here — operators should apply egress rules. +// T5 – Secret leakage in logs. +// Mitigation: secret is never logged; only the HMAC hex digest is transmitted. + +use sha2::Sha256; +use hmac::{Hmac, Mac}; use serde::Serialize; -use tracing::warn; +use tracing::{info, warn}; + +type HmacSha256 = Hmac; #[derive(Debug, Clone, Serialize)] pub struct ScanWebhookSummary { @@ -26,9 +45,51 @@ enum WebhookProvider { Custom, } +/// Configuration for webhook delivery (#522). +#[derive(Debug, Clone, Default)] +pub struct WebhookConfig { + /// If set, every outgoing request includes an `X-Sanctifier-Signature-256` + /// header containing `sha256=` computed over the serialised body. + pub secret: Option, + /// Maximum number of delivery attempts per URL (default: 3). + pub max_attempts: Option, +} + +/// Validate that a webhook URL uses HTTPS (T4 mitigation). +pub fn validate_webhook_url(url: &str) -> Result<(), String> { + if url.starts_with("https://") { + Ok(()) + } else { + Err(format!( + "webhook URL '{}' must use HTTPS to prevent plaintext secret transmission", + url + )) + } +} + +/// Compute an HMAC-SHA256 signature over `body` using `secret`. +/// Returns the hex-encoded digest prefixed with `sha256=`. +fn hmac_signature(secret: &str, body: &[u8]) -> String { + let mut mac = HmacSha256::new_from_slice(secret.as_bytes()) + .expect("HMAC accepts any key length"); + mac.update(body); + let result = mac.finalize(); + let bytes = result.into_bytes(); + format!("sha256={}", hex_encode(&bytes)) +} + +fn hex_encode(bytes: &[u8]) -> String { + bytes.iter().map(|b| format!("{:02x}", b)).collect() +} + +/// Deliver webhooks with optional HMAC signing and exponential-backoff retries. +/// +/// Errors are accumulated and returned after all URLs are attempted, so a +/// single failing endpoint does not abort delivery to the remaining ones. pub fn send_scan_completed_webhooks( urls: &[String], payload: &ScanWebhookPayload, + config: &WebhookConfig, ) -> anyhow::Result<()> { if urls.is_empty() { return Ok(()); @@ -38,26 +99,85 @@ pub fn send_scan_completed_webhooks( .timeout(std::time::Duration::from_secs(10)) .build()?; + let max_attempts = config.max_attempts.unwrap_or(3); + let mut errors: Vec = Vec::new(); + for url in urls { let body = provider_payload(url, payload); - let response = client.post(url).json(&body).send(); - match response { - Ok(resp) if resp.status().is_success() => {} - Ok(resp) => { - warn!( - target: "sanctifier", - status = resp.status().as_u16(), - url = %url, - "Webhook delivery failed" - ); + let body_bytes = serde_json::to_vec(&body)?; + + let mut last_error: Option = None; + + for attempt in 1..=max_attempts { + let mut req = client + .post(url) + .header("Content-Type", "application/json") + .body(body_bytes.clone()); + + if let Some(ref secret) = config.secret { + let sig = hmac_signature(secret, &body_bytes); + req = req.header("X-Sanctifier-Signature-256", sig); + } + + match req.send() { + Ok(resp) if resp.status().is_success() => { + if attempt > 1 { + info!( + target: "sanctifier", + url = %url, + attempt, + "Webhook delivered after retry" + ); + } + last_error = None; + break; + } + Ok(resp) => { + let status = resp.status().as_u16(); + last_error = Some(format!("HTTP {}", status)); + warn!( + target: "sanctifier", + status, + url = %url, + attempt, + max_attempts, + "Webhook delivery failed, will retry" + ); + } + Err(err) => { + last_error = Some(err.to_string()); + warn!( + target: "sanctifier", + error = %err, + url = %url, + attempt, + max_attempts, + "Webhook request error, will retry" + ); + } } - Err(err) => { - warn!(target: "sanctifier", error = %err, url = %url, "Webhook delivery error"); + + if attempt < max_attempts { + // Exponential backoff: 1s, 2s, 4s … capped at 30s. + let delay_secs = std::cmp::min(1u64 << (attempt - 1), 30); + std::thread::sleep(std::time::Duration::from_secs(delay_secs)); } } + + if let Some(err_msg) = last_error { + errors.push(format!("{url}: {err_msg}")); + } } - Ok(()) + if errors.is_empty() { + Ok(()) + } else { + Err(anyhow::anyhow!( + "webhook delivery failed for {} endpoint(s):\n{}", + errors.len(), + errors.join("\n") + )) + } } fn provider_payload(url: &str, payload: &ScanWebhookPayload) -> serde_json::Value { @@ -203,7 +323,7 @@ mod tests { .create(); let url = format!("{}/discord?sanctifier_provider=discord", server.url()); - send_scan_completed_webhooks(&[url], &payload).unwrap(); + send_scan_completed_webhooks(&[url], &payload, &WebhookConfig::default()).unwrap(); mock.assert(); } @@ -217,36 +337,12 @@ mod tests { { "color": "#f79009", "fields": [ - { - "title": "Project", - "value": "contracts/my-token", - "short": true - }, - { - "title": "Event", - "value": "scan.completed", - "short": true - }, - { - "title": "Total Findings", - "value": "2", - "short": true - }, - { - "title": "Critical", - "value": "false", - "short": true - }, - { - "title": "High", - "value": "true", - "short": true - }, - { - "title": "Timestamp", - "value": "123", - "short": true - } + { "title": "Project", "value": "contracts/my-token", "short": true }, + { "title": "Event", "value": "scan.completed", "short": true }, + { "title": "Total Findings", "value": "2", "short": true }, + { "title": "Critical", "value": "false", "short": true }, + { "title": "High", "value": "true", "short": true }, + { "title": "Timestamp", "value": "123", "short": true } ] } ] @@ -261,7 +357,7 @@ mod tests { .create(); let url = format!("{}/slack?sanctifier_provider=slack", server.url()); - send_scan_completed_webhooks(&[url], &payload).unwrap(); + send_scan_completed_webhooks(&[url], &payload, &WebhookConfig::default()).unwrap(); mock.assert(); } @@ -279,7 +375,7 @@ mod tests { format!("{}/notify", second.url()), ]; - send_scan_completed_webhooks(&urls, &sample_payload()).unwrap(); + send_scan_completed_webhooks(&urls, &sample_payload(), &WebhookConfig::default()).unwrap(); first_mock.assert(); second_mock.assert(); @@ -291,4 +387,78 @@ mod tests { assert_eq!(payload["event"], "scan.completed"); assert_eq!(payload["summary"]["total_findings"], 2); } + + #[test] + fn signed_request_includes_hmac_header() { + let secret = "test-secret-key"; + let payload = sample_payload(); + let body = provider_payload_for(WebhookProvider::Custom, &payload); + let body_bytes = serde_json::to_vec(&body).unwrap(); + let expected_sig = hmac_signature(secret, &body_bytes); + + let mut server = Server::new(); + let mock = server + .mock("POST", "/signed") + .match_header("X-Sanctifier-Signature-256", expected_sig.as_str()) + .with_status(200) + .create(); + + let url = format!("{}/signed", server.url()); + let config = WebhookConfig { + secret: Some(secret.to_string()), + max_attempts: Some(1), + }; + send_scan_completed_webhooks(&[url], &payload, &config).unwrap(); + mock.assert(); + } + + #[test] + fn delivery_failure_after_retries_returns_error() { + let mut server = Server::new(); + // Always return 500 — should exhaust retries + let mock = server + .mock("POST", "/fail") + .with_status(500) + .expect(1) // max_attempts = 1 to keep test fast + .create(); + + let url = format!("{}/fail", server.url()); + let config = WebhookConfig { + secret: None, + max_attempts: Some(1), + }; + let result = send_scan_completed_webhooks(&[url], &sample_payload(), &config); + assert!(result.is_err(), "should return Err after exhausted retries"); + mock.assert(); + } + + #[test] + fn empty_url_list_returns_ok_without_requests() { + send_scan_completed_webhooks(&[], &sample_payload(), &WebhookConfig::default()).unwrap(); + } + + #[test] + fn validate_webhook_url_rejects_http() { + assert!(validate_webhook_url("http://hooks.slack.com/xxx").is_err()); + } + + #[test] + fn validate_webhook_url_accepts_https() { + assert!(validate_webhook_url("https://hooks.slack.com/xxx").is_ok()); + } + + #[test] + fn hmac_signature_is_deterministic() { + let sig1 = hmac_signature("secret", b"body"); + let sig2 = hmac_signature("secret", b"body"); + assert_eq!(sig1, sig2); + assert!(sig1.starts_with("sha256=")); + } + + #[test] + fn hmac_signature_differs_with_different_secrets() { + let sig1 = hmac_signature("secret-a", b"body"); + let sig2 = hmac_signature("secret-b", b"body"); + assert_ne!(sig1, sig2); + } } diff --git a/tooling/sanctifier-cli/src/errors.rs b/tooling/sanctifier-cli/src/errors.rs new file mode 100644 index 00000000..8d2ef81e --- /dev/null +++ b/tooling/sanctifier-cli/src/errors.rs @@ -0,0 +1,314 @@ +//! Structured error types with actionable user hints (#528). +//! +//! Every `SanctifierError` variant carries a machine-readable `code`, a +//! human-readable `message`, and an `hint` that tells the user exactly what +//! to do next. The `Display` impl renders all three so that `anyhow` context +//! propagation surfaces the hint automatically at the top level. + +use std::fmt; + +/// Error codes (stable; used in structured output and CI gate rules). +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum ErrorCode { + /// Path supplied to a command was not found or is inaccessible. + E001, + /// Path exists but is not a Soroban project (no `Cargo.toml` with `soroban-sdk`). + E002, + /// Configuration file could not be parsed. + E003, + /// Analysis timed out for one or more files. + E004, + /// Webhook delivery failed after all retries. + E005, + /// `cargo search` or `cargo install` failed during self-update. + E006, + /// Report output path could not be written. + E007, + /// Vulnerability database file is missing or malformed. + E008, + /// Dry-run mode: no changes were made (informational, not a failure). + E009, +} + +impl ErrorCode { + pub fn as_str(self) -> &'static str { + match self { + ErrorCode::E001 => "E001", + ErrorCode::E002 => "E002", + ErrorCode::E003 => "E003", + ErrorCode::E004 => "E004", + ErrorCode::E005 => "E005", + ErrorCode::E006 => "E006", + ErrorCode::E007 => "E007", + ErrorCode::E008 => "E008", + ErrorCode::E009 => "E009", + } + } +} + +/// A Sanctifier CLI error with a stable code, a message, and an actionable hint. +#[derive(Debug)] +pub struct SanctifierError { + pub code: ErrorCode, + pub message: String, + pub hint: String, +} + +impl SanctifierError { + pub fn new(code: ErrorCode, message: impl Into, hint: impl Into) -> Self { + Self { + code, + message: message.into(), + hint: hint.into(), + } + } + + // ── Constructors for each error code ───────────────────────────────────── + + pub fn path_not_found(path: &std::path::Path) -> Self { + Self::new( + ErrorCode::E001, + format!("path not found: {}", path.display()), + format!( + "Check that '{}' exists and that you have read permission. \ + Run `ls {}` to inspect the directory.", + path.display(), + path.parent() + .map(|p| p.display().to_string()) + .unwrap_or_else(|| ".".to_string()) + ), + ) + } + + pub fn not_soroban_project(path: &std::path::Path) -> Self { + Self::new( + ErrorCode::E002, + format!( + "'{}' is not a valid Soroban project", + path.display() + ), + "Ensure the directory contains a Cargo.toml that declares \ + `soroban-sdk` as a dependency. Run `sanctifier init` to scaffold \ + a new project, or pass a path to an existing Soroban contract." + .to_string(), + ) + } + + pub fn config_parse_error(path: &std::path::Path, detail: &str) -> Self { + Self::new( + ErrorCode::E003, + format!("could not parse config file {}: {}", path.display(), detail), + format!( + "Validate '{}' against the JSON schema in `schemas/sanctifier-config.json`. \ + Run `sanctifier doctor` to diagnose common configuration problems.", + path.display() + ), + ) + } + + pub fn analysis_timeout(file: &str, timeout_secs: u64) -> Self { + Self::new( + ErrorCode::E004, + format!("analysis timed out for '{}' after {}s", file, timeout_secs), + format!( + "Increase `--timeout` (currently {timeout_secs}s) or add the file to \ + `ignore_paths` in `.sanctifier.toml` if it is generated/vendored code." + ), + ) + } + + pub fn webhook_failed(url: &str, attempts: u32, last_error: &str) -> Self { + Self::new( + ErrorCode::E005, + format!( + "webhook delivery to '{}' failed after {} attempt(s): {}", + url, attempts, last_error + ), + "Verify the webhook URL is reachable from this machine and that the \ + endpoint accepts POST requests with a JSON body. Check `--webhook-secret` \ + is set if the endpoint requires HMAC-SHA256 signature verification." + .to_string(), + ) + } + + pub fn update_cargo_search_failed(detail: &str) -> Self { + Self::new( + ErrorCode::E006, + format!("`cargo search` failed: {}", detail), + "Ensure you are connected to the internet and that `~/.cargo/registry` \ + is not corrupted. Try running `cargo search sanctifier-cli` manually \ + to diagnose the issue." + .to_string(), + ) + } + + pub fn update_install_failed(version: &str) -> Self { + Self::new( + ErrorCode::E006, + format!("`cargo install` failed while installing sanctifier-cli v{}", version), + format!( + "Try running `cargo install sanctifier-cli --version {version} --locked` \ + manually to see the full error. If the Rust toolchain is out of date, \ + run `rustup update stable` first." + ), + ) + } + + pub fn report_write_failed(path: &std::path::Path, detail: &str) -> Self { + Self::new( + ErrorCode::E007, + format!("could not write report to '{}': {}", path.display(), detail), + format!( + "Check that the directory '{}' exists and is writable. \ + Create it with `mkdir -p {}` if needed.", + path.parent() + .map(|p| p.display().to_string()) + .unwrap_or_else(|| ".".to_string()), + path.parent() + .map(|p| p.display().to_string()) + .unwrap_or_else(|| ".".to_string()), + ), + ) + } + + pub fn vuln_db_load_failed(path: &std::path::Path, detail: &str) -> Self { + Self::new( + ErrorCode::E008, + format!( + "could not load vulnerability database from '{}': {}", + path.display(), + detail + ), + format!( + "Ensure '{}' is a valid JSON file matching the vuln-db schema. \ + Run `sanctifier vulndb validate {}` to check the file, or omit \ + `--vuln-db` to use the bundled default database.", + path.display(), + path.display() + ), + ) + } + + pub fn dry_run_no_changes(command: &str) -> Self { + Self::new( + ErrorCode::E009, + format!("[dry-run] '{}' would make changes but --dry-run is set", command), + "Remove `--dry-run` to apply the changes, or review what would change \ + before proceeding." + .to_string(), + ) + } +} + +impl fmt::Display for SanctifierError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!( + f, + "[{}] {}\n → hint: {}", + self.code.as_str(), + self.message, + self.hint + ) + } +} + +impl std::error::Error for SanctifierError {} + +/// Render a `SanctifierError` as a JSON object for `--output-format json` callers. +pub fn to_json(err: &SanctifierError) -> serde_json::Value { + serde_json::json!({ + "error": { + "code": err.code.as_str(), + "message": err.message, + "hint": err.hint + } + }) +} + +#[cfg(test)] +mod tests { + use super::*; + use std::path::Path; + + #[test] + fn display_includes_code_message_and_hint() { + let err = SanctifierError::path_not_found(Path::new("/no/such/path")); + let s = err.to_string(); + assert!(s.contains("[E001]"), "missing error code"); + assert!(s.contains("path not found"), "missing message"); + assert!(s.contains("hint:"), "missing hint marker"); + } + + #[test] + fn not_soroban_project_error_mentions_cargo_toml() { + let err = SanctifierError::not_soroban_project(Path::new("my-contract")); + assert!(err.hint.contains("Cargo.toml")); + assert!(err.hint.contains("sanctifier init")); + assert_eq!(err.code, ErrorCode::E002); + } + + #[test] + fn config_parse_error_mentions_doctor_command() { + let err = SanctifierError::config_parse_error( + Path::new(".sanctifier.toml"), + "unexpected key `foo`", + ); + assert!(err.hint.contains("sanctifier doctor")); + assert_eq!(err.code, ErrorCode::E003); + } + + #[test] + fn analysis_timeout_hint_includes_timeout_value() { + let err = SanctifierError::analysis_timeout("src/lib.rs", 30); + assert!(err.hint.contains("30s")); + assert!(err.hint.contains("ignore_paths")); + } + + #[test] + fn webhook_failed_hint_mentions_secret() { + let err = SanctifierError::webhook_failed("https://hooks.slack.com/xxx", 3, "connection refused"); + assert!(err.hint.contains("--webhook-secret")); + assert_eq!(err.code, ErrorCode::E005); + } + + #[test] + fn update_install_failed_hint_includes_version() { + let err = SanctifierError::update_install_failed("0.4.0"); + assert!(err.hint.contains("0.4.0")); + assert!(err.hint.contains("rustup")); + } + + #[test] + fn report_write_failed_hint_includes_mkdir() { + let err = SanctifierError::report_write_failed(Path::new("out/report.md"), "permission denied"); + assert!(err.hint.contains("mkdir")); + assert_eq!(err.code, ErrorCode::E007); + } + + #[test] + fn to_json_produces_valid_structure() { + let err = SanctifierError::dry_run_no_changes("update"); + let json = to_json(&err); + assert_eq!(json["error"]["code"], "E009"); + assert!(json["error"]["message"].as_str().unwrap().contains("dry-run")); + assert!(!json["error"]["hint"].as_str().unwrap().is_empty()); + } + + #[test] + fn all_error_codes_have_stable_string_representation() { + let codes = [ + (ErrorCode::E001, "E001"), + (ErrorCode::E002, "E002"), + (ErrorCode::E003, "E003"), + (ErrorCode::E004, "E004"), + (ErrorCode::E005, "E005"), + (ErrorCode::E006, "E006"), + (ErrorCode::E007, "E007"), + (ErrorCode::E008, "E008"), + (ErrorCode::E009, "E009"), + ]; + for (code, expected) in codes { + assert_eq!(code.as_str(), expected); + } + } +} diff --git a/tooling/sanctifier-cli/src/lib.rs b/tooling/sanctifier-cli/src/lib.rs index 6a1765d8..a5ce2f4e 100644 --- a/tooling/sanctifier-cli/src/lib.rs +++ b/tooling/sanctifier-cli/src/lib.rs @@ -1,6 +1,7 @@ #![recursion_limit = "512"] pub mod commands; +pub mod errors; pub mod exit_codes; pub mod logging; pub mod telemetry; diff --git a/tooling/sanctifier-cli/tests/fixtures/cargo_search_output.txt b/tooling/sanctifier-cli/tests/fixtures/cargo_search_output.txt new file mode 100644 index 00000000..78700592 --- /dev/null +++ b/tooling/sanctifier-cli/tests/fixtures/cargo_search_output.txt @@ -0,0 +1,2 @@ +sanctifier-cli = "0.3.7" # CLI tool for Sanctifier — Stellar Soroban Security Suite +sanctifier-core = "0.3.7" # Core analysis library for Sanctifier