From 5c9e66108debed72d4bb5ea5e40d4de98d5df9e9 Mon Sep 17 00:00:00 2001 From: Sam Valladares Date: Sun, 28 Jun 2026 13:47:10 -0500 Subject: [PATCH] fix(security): close SSRF/token-exfil + credential leaks (swarm audit) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Multi-model audit (deepseek-v4/minimax/kimi/qwen) surfaced these; verified against the real code and fixed the confirmed ones: - github connector: host-pin failed OPEN when api_root was unparseable/hostless — the bearer token would ride a Link `next` url to an attacker host. Now fail-closed: no pinned host => drop the url. (CRITICAL: SSRF / token exfil) - GithubConfig/RedmineConfig derived Debug leaked the token/api_key into any {:?} log line or panic message. Replaced with manual redacting Debug impls. - cross_project priority calc used `as u32 - i` which underflows/panics (debug) or wraps + corrupts the sort (release). Use saturating_sub. Verified false-positive (no change): path-traversal in get_file_context — it only inspects the path string, never reads the file. core: 535 passed / 0 failed, clippy clean. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../src/advanced/cross_project.rs | 7 ++- crates/vestige-core/src/connectors/github.rs | 51 +++++++++++++------ crates/vestige-core/src/connectors/redmine.rs | 15 +++++- 3 files changed, 56 insertions(+), 17 deletions(-) diff --git a/crates/vestige-core/src/advanced/cross_project.rs b/crates/vestige-core/src/advanced/cross_project.rs index ffda53a..706b46b 100644 --- a/crates/vestige-core/src/advanced/cross_project.rs +++ b/crates/vestige-core/src/advanced/cross_project.rs @@ -411,7 +411,12 @@ impl CrossProjectLearner { based_on: pattern.pattern.name.clone(), confidence: applicable.applicability_confidence, evidence: applicable.supporting_memories.clone(), - priority: (10.0 * applicable.applicability_confidence) as u32 - i as u32, + // saturating_sub: when confidence is low (small base) and + // the suggestion index i exceeds it, a plain `-` underflows + // — panicking in debug, wrapping to a huge value in release + // (which corrupts the Reverse-priority sort). Saturate to 0. + priority: ((10.0 * applicable.applicability_confidence) as u32) + .saturating_sub(i as u32), }); } } diff --git a/crates/vestige-core/src/connectors/github.rs b/crates/vestige-core/src/connectors/github.rs index 24bd60e..3665b3f 100644 --- a/crates/vestige-core/src/connectors/github.rs +++ b/crates/vestige-core/src/connectors/github.rs @@ -35,7 +35,7 @@ const USER_AGENT: &str = concat!("vestige-connector/", env!("CARGO_PKG_VERSION") const PER_PAGE: u32 = 100; /// Configuration for a GitHub Issues connector instance. -#[derive(Debug, Clone)] +#[derive(Clone)] pub struct GithubConfig { /// Repository owner (user or org). pub owner: String, @@ -50,6 +50,20 @@ pub struct GithubConfig { pub max_comments: usize, } +// Manual Debug that NEVER prints the token — a derived Debug would leak the +// bearer credential into any `{:?}` log line or panic message. +impl std::fmt::Debug for GithubConfig { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("GithubConfig") + .field("owner", &self.owner) + .field("repo", &self.repo) + .field("token", &self.token.as_ref().map(|_| "")) + .field("api_root", &self.api_root) + .field("max_comments", &self.max_comments) + .finish() + } +} + impl GithubConfig { pub fn new(owner: impl Into, repo: impl Into) -> Self { Self { @@ -162,22 +176,29 @@ impl GithubConnector { { let url = &part[start + 1..end]; // Host-pin: only follow a next-url on the same host as the API - // root we were configured with. - if let Some(expected) = expected_host { - match reqwest::Url::parse(url) { - Ok(parsed) if parsed.host_str() == Some(expected) => { - return Some(url.to_string()); - } - _ => { - tracing::warn!( - next_url = url, - "dropping cross-host Link next url (host pin)" - ); - return None; - } + // root we were configured with. FAIL-CLOSED: if we could not + // determine the expected host (unparseable/hostless api_root), we + // must NOT follow the url — the bearer token would otherwise ride + // along to an attacker-influenced host (SSRF / token exfiltration). + let Some(expected) = expected_host else { + tracing::warn!( + next_url = url, + "dropping Link next url: no pinned host (fail-closed)" + ); + return None; + }; + match reqwest::Url::parse(url) { + Ok(parsed) if parsed.host_str() == Some(expected) => { + return Some(url.to_string()); + } + _ => { + tracing::warn!( + next_url = url, + "dropping cross-host Link next url (host pin)" + ); + return None; } } - return Some(url.to_string()); } } None diff --git a/crates/vestige-core/src/connectors/redmine.rs b/crates/vestige-core/src/connectors/redmine.rs index 1b2a316..de4464f 100644 --- a/crates/vestige-core/src/connectors/redmine.rs +++ b/crates/vestige-core/src/connectors/redmine.rs @@ -40,7 +40,7 @@ const USER_AGENT: &str = concat!("vestige-connector/", env!("CARGO_PKG_VERSION") const PAGE_LIMIT: u32 = 100; /// Configuration for a Redmine connector instance bound to one project. -#[derive(Debug, Clone)] +#[derive(Clone)] pub struct RedmineConfig { /// Base URL of the Redmine instance, e.g. `https://redmine.example.com`. pub base_url: String, @@ -55,6 +55,19 @@ pub struct RedmineConfig { pub max_journals: usize, } +// Manual Debug that NEVER prints the api_key — a derived Debug would leak the +// credential into any `{:?}` log line or panic message. +impl std::fmt::Debug for RedmineConfig { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("RedmineConfig") + .field("base_url", &self.base_url) + .field("project", &self.project) + .field("api_key", &self.api_key.as_ref().map(|_| "")) + .field("max_journals", &self.max_journals) + .finish() + } +} + impl RedmineConfig { pub fn new(base_url: impl Into, project: impl Into) -> Self { Self {