From 0e5c71b4cabe26927811a33948932e60cd7b9019 Mon Sep 17 00:00:00 2001 From: Sam Valladares <143034159+samvallad33@users.noreply.github.com> Date: Thu, 2 Jul 2026 16:05:40 -0500 Subject: [PATCH] fix(backend): resolve 10 bugs from the final full-codebase audit (#141) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A second adversarial audit of shipped main (post the 29+3 earlier fixes) found 10 more real bugs in areas the first pass under-covered (FTS sanitizer, protocol/HTTP, connectors, frontend). None regressions — all pre-existing, and several are "twin sites" of a class fixed elsewhere but missed here. Majors: - FTS5: sanitize_fts5_terms leaked bare operators on doubled input ("foo AND AND AND"), aborting MATCH with a syntax error or silently flipping AND->OR. Now filters operator tokens (repetition-proof). - Server crash (DoS): unbounded MCP `hours_back`/`hours_forward` panicked via Duration::minutes in CaptureWindow; unclamped `limit` in match_context panicked via negative Vec::with_capacity / limit*2 overflow. Both now bounded/checked. - Connectors: the source idempotency key omitted source_project, so two repos (or two Redmine instances) with overlapping ids clobbered each other's memory. Key + unique index (migration V19) now include source_project. Minors/nits: - list_memories now honors node_type/tag on the search path (were dropped). - HTTP Accept honors */* and type wildcards (was hard 406 for curl et al.). - Bearer scheme matched case-insensitively (RFC 7235). - Dream insight time-span no longer seeds from now/-365d sentinels (fabricated a TemporalTrend for old clusters). - WebSocket disconnect() detaches onclose before close() so it can't resurrect the socket via scheduleReconnect. - Redmine thread truncation keeps the NEWEST journals (was keeping oldest, so new activity never re-indexed). Tests: FTS operator-leak regression added. Full workspace green (1559 tests, clippy -D warnings clean); dashboard check + 937 tests green. Co-authored-by: Claude Opus 4.8 (1M context) --- apps/dashboard/src/lib/stores/websocket.ts | 15 +++++- crates/vestige-core/src/advanced/dreams.rs | 19 ++++--- crates/vestige-core/src/connectors/redmine.rs | 9 +++- crates/vestige-core/src/fts.rs | 54 ++++++++++++------- .../src/neuroscience/synaptic_tagging.rs | 19 +++++-- crates/vestige-core/src/storage/migrations.rs | 22 ++++++++ crates/vestige-core/src/storage/sqlite.rs | 12 ++++- crates/vestige-mcp/src/dashboard/handlers.rs | 16 ++++++ crates/vestige-mcp/src/protocol/http.rs | 23 +++++--- crates/vestige-mcp/src/tools/context.rs | 7 ++- 10 files changed, 152 insertions(+), 44 deletions(-) diff --git a/apps/dashboard/src/lib/stores/websocket.ts b/apps/dashboard/src/lib/stores/websocket.ts index 2e591ac..578c43b 100644 --- a/apps/dashboard/src/lib/stores/websocket.ts +++ b/apps/dashboard/src/lib/stores/websocket.ts @@ -74,8 +74,19 @@ function createWebSocketStore() { } function disconnect() { - if (reconnectTimer) clearTimeout(reconnectTimer); - ws?.close(); + if (reconnectTimer) { + clearTimeout(reconnectTimer); + reconnectTimer = null; + } + if (ws) { + // Detach onclose BEFORE closing: otherwise close() fires the handler, + // which calls scheduleReconnect and resurrects the socket we just + // asked to tear down. + ws.onclose = null; + ws.onerror = null; + ws.onmessage = null; + ws.close(); + } ws = null; set({ connected: false, reconnecting: false, events: [], lastHeartbeat: null, error: null }); } diff --git a/crates/vestige-core/src/advanced/dreams.rs b/crates/vestige-core/src/advanced/dreams.rs index 81d9039..7e32547 100644 --- a/crates/vestige-core/src/advanced/dreams.rs +++ b/crates/vestige-core/src/advanced/dreams.rs @@ -1600,15 +1600,20 @@ impl MemoryDreamer { memories: &[&DreamMemory], common_tags: &[String], ) -> (String, InsightType) { - // Determine insight type based on memory characteristics - let time_range = memories + // Determine insight type based on memory characteristics. Seed the + // (min, max) fold from the ACTUAL first timestamp, not (now, now-365d) + // sentinels: those sentinels leaked into the span for clusters of old + // memories (all created > 365d ago), where min stayed pinned near `now` + // and inflated time_span_days past 30, fabricating a TemporalTrend. + let time_span_days = memories .iter() .map(|m| m.created_at) - .fold((Utc::now(), Utc::now() - Duration::days(365)), |acc, t| { - (acc.0.min(t), acc.1.max(t)) - }); - - let time_span_days = (time_range.1 - time_range.0).num_days(); + .fold(None::<(DateTime, DateTime)>, |acc, t| match acc { + None => Some((t, t)), + Some((lo, hi)) => Some((lo.min(t), hi.max(t))), + }) + .map(|(lo, hi)| (hi - lo).num_days()) + .unwrap_or(0); if time_span_days > 30 { // Temporal trend diff --git a/crates/vestige-core/src/connectors/redmine.rs b/crates/vestige-core/src/connectors/redmine.rs index f17cdd2..9ae9d18 100644 --- a/crates/vestige-core/src/connectors/redmine.rs +++ b/crates/vestige-core/src/connectors/redmine.rs @@ -243,7 +243,14 @@ impl RedmineConnector { }) .collect(); journals.sort_by_key(|j| j.id); - journals.truncate(self.config.max_journals); + // Keep the NEWEST max_journals, not the oldest: on a capped thread the + // latest activity is what signals the record changed. Truncating from the + // front (keeping oldest) meant new comments were dropped, the content_hash + // never moved, and the memory never re-indexed with fresh activity. + if journals.len() > self.config.max_journals { + let drop = journals.len() - self.config.max_journals; + journals.drain(0..drop); + } // Human-readable content. let mut content = format!("[{}#{}] {}\n", self.scope, issue.id, issue.subject); diff --git a/crates/vestige-core/src/fts.rs b/crates/vestige-core/src/fts.rs index 66e2299..e3de7bc 100644 --- a/crates/vestige-core/src/fts.rs +++ b/crates/vestige-core/src/fts.rs @@ -28,26 +28,18 @@ pub fn sanitize_fts5_terms(query: &str) -> Option { }) .collect(); - for op in FTS5_OPERATORS { - let pattern = format!(" {} ", op); - sanitized = sanitized.replace(&pattern, " "); - sanitized = sanitized.replace(&pattern.to_lowercase(), " "); - let upper = sanitized.to_uppercase(); - let start_pattern = format!("{} ", op); - if upper.starts_with(&start_pattern) { - sanitized = sanitized.chars().skip(op.len()).collect(); - } - let end_pattern = format!(" {}", op); - if upper.ends_with(&end_pattern) { - let char_count = sanitized.chars().count(); - sanitized = sanitized - .chars() - .take(char_count.saturating_sub(op.len())) - .collect(); - } - } - - let terms: Vec<&str> = sanitized.split_whitespace().collect(); + // Drop any token that IS a bare FTS5 boolean operator (AND/OR/NOT/NEAR), + // case-insensitively. Token-level filtering is complete and repetition-proof. + // The previous string-replace approach was single-pass and non-overlapping, + // so a doubled operator ("foo AND AND AND") left a bare operator at the edge + // or interior that leaked into the raw MATCH — either aborting with an FTS5 + // syntax error, or silently flipping implicit-AND to boolean-OR + // ("foo OR bar"). Filtering tokens cannot leak an operator regardless of how + // many are chained. + let terms: Vec<&str> = sanitized + .split_whitespace() + .filter(|t| !FTS5_OPERATORS.iter().any(|op| t.eq_ignore_ascii_case(op))) + .collect(); if terms.is_empty() { return None; } @@ -237,4 +229,26 @@ mod tests { let q2 = sanitize_fts5_or_query(&longtok).unwrap(); assert!(q2.len() <= 66, "single token capped at 64 + quotes, got {}", q2.len()); } + + #[test] + fn terms_strips_all_bare_operators_even_when_doubled() { + // Regression: doubled/chained operators must never leak a bare operator + // into the raw MATCH (which aborts with a syntax error or silently flips + // implicit-AND to boolean-OR). + for op in FTS5_OPERATORS { + let doubled = format!("foo {op} {op} {op} bar"); + let out = sanitize_fts5_terms(&doubled).unwrap(); + for tok in out.split_whitespace() { + assert!( + !FTS5_OPERATORS.iter().any(|o| tok.eq_ignore_ascii_case(o)), + "operator {op} leaked into output {out:?}" + ); + } + assert_eq!(out, "foo bar", "only real terms should survive: {out:?}"); + } + // Lowercase + leading/trailing operators are stripped too. + assert_eq!(sanitize_fts5_terms("or foo and bar or").unwrap(), "foo bar"); + // Operator-only input yields nothing. + assert_eq!(sanitize_fts5_terms("AND OR NOT").map(|_| ()), None); + } } diff --git a/crates/vestige-core/src/neuroscience/synaptic_tagging.rs b/crates/vestige-core/src/neuroscience/synaptic_tagging.rs index c5e9ca1..0bd3339 100644 --- a/crates/vestige-core/src/neuroscience/synaptic_tagging.rs +++ b/crates/vestige-core/src/neuroscience/synaptic_tagging.rs @@ -369,14 +369,25 @@ impl CaptureWindow { } } - /// Get the start of the capture window + /// Get the start of the capture window. + /// + /// Uses checked Duration + subtraction: `Duration::minutes` panics for an + /// out-of-range magnitude and the subtraction can overflow the DateTime + /// range, so an unbounded caller-supplied `backward_hours` (via the + /// `trigger_importance` MCP tool) would otherwise abort the server. On + /// overflow we saturate to the minimum representable time. pub fn window_start(&self, event_time: DateTime) -> DateTime { - event_time - Duration::minutes((self.backward_hours * 60.0) as i64) + Duration::try_minutes((self.backward_hours * 60.0) as i64) + .and_then(|d| event_time.checked_sub_signed(d)) + .unwrap_or(DateTime::::MIN_UTC) } - /// Get the end of the capture window + /// Get the end of the capture window. See `window_start` for the overflow + /// rationale; on overflow we saturate to the maximum representable time. pub fn window_end(&self, event_time: DateTime) -> DateTime { - event_time + Duration::minutes((self.forward_hours * 60.0) as i64) + Duration::try_minutes((self.forward_hours * 60.0) as i64) + .and_then(|d| event_time.checked_add_signed(d)) + .unwrap_or(DateTime::::MAX_UTC) } /// Check if a time is within the capture window diff --git a/crates/vestige-core/src/storage/migrations.rs b/crates/vestige-core/src/storage/migrations.rs index 50a1127..3440f61 100644 --- a/crates/vestige-core/src/storage/migrations.rs +++ b/crates/vestige-core/src/storage/migrations.rs @@ -94,6 +94,11 @@ pub const MIGRATIONS: &[Migration] = &[ description: "Agent Black Box + Memory Receipts + Memory PRs: replayable run traces, retrieval receipts, risk-gated brain-change review queue", up: MIGRATION_V18_UP, }, + Migration { + version: 19, + description: "Scope the source idempotency key by source_project so same-system sources with overlapping ids no longer clobber each other", + up: MIGRATION_V19_UP, + }, ]; /// A database migration @@ -1133,6 +1138,23 @@ CREATE INDEX IF NOT EXISTS idx_memory_prs_created_at ON memory_prs(created_at DE UPDATE schema_version SET version = 18, applied_at = datetime('now'); "#; +const MIGRATION_V19_UP: &str = r#" +-- Scope the source idempotency key by source_project. The V17 index keyed only +-- on (source_system, source_id), so two sources of the same system (e.g. github +-- repos octocat/repoA + octocat/repoB, or two Redmine instances) with the same +-- bare per-project id ("5") collided and overwrote each other's memory in place. +-- Include source_project so each (system, project, id) gets its own row. +-- COALESCE keeps legacy rows (source_project NULL) sharing a single bucket, +-- matching the upsert lookup's `source_project IS ?` semantics. +DROP INDEX IF EXISTS idx_nodes_source_key; + +CREATE UNIQUE INDEX IF NOT EXISTS idx_nodes_source_key + ON knowledge_nodes(source_system, COALESCE(source_project, ''), source_id) + WHERE source_system IS NOT NULL AND source_id IS NOT NULL; + +UPDATE schema_version SET version = 19, applied_at = datetime('now'); +"#; + /// Apply pending migrations /// /// Each migration is applied inside an explicit transaction so its schema diff --git a/crates/vestige-core/src/storage/sqlite.rs b/crates/vestige-core/src/storage/sqlite.rs index 0174009..5fa1108 100644 --- a/crates/vestige-core/src/storage/sqlite.rs +++ b/crates/vestige-core/src/storage/sqlite.rs @@ -9939,6 +9939,13 @@ impl SqliteMemoryStore { let source_system = env.source_system.clone().unwrap_or_default(); let source_id = env.source_id.clone().unwrap_or_default(); + // Scope the idempotency key by source_project too: two sources of the + // same system (e.g. github repos octocat/repoA and octocat/repoB, or two + // Redmine instances) reuse bare per-project ids ("5"), so keying on + // (source_system, source_id) alone made repoB's issue #5 overwrite + // repoA's row in place. `IS NOT DISTINCT FROM` matches NULL==NULL so + // legacy rows without a project still resolve. + let source_project = env.source_project.clone(); let now = Utc::now(); // Look up the existing memory for this external record, if any. @@ -9950,8 +9957,9 @@ impl SqliteMemoryStore { reader .query_row( "SELECT id, content_hash FROM knowledge_nodes \ - WHERE source_system = ?1 AND source_id = ?2 LIMIT 1", - params![source_system, source_id], + WHERE source_system = ?1 AND source_id = ?2 \ + AND source_project IS ?3 LIMIT 1", + params![source_system, source_id, source_project], |row| Ok((row.get::<_, String>(0)?, row.get::<_, Option>(1)?)), ) .optional()? diff --git a/crates/vestige-mcp/src/dashboard/handlers.rs b/crates/vestige-mcp/src/dashboard/handlers.rs index 4057738..aad6372 100644 --- a/crates/vestige-mcp/src/dashboard/handlers.rs +++ b/crates/vestige-mcp/src/dashboard/handlers.rs @@ -58,6 +58,22 @@ pub async fn list_memories( true } }) + // Honor node_type/tag filters on the search path too. Previously + // these were applied only in the no-query branch, so a filtered + // search (?q=foo&node_type=decision&tag=x) silently returned + // non-matching rows. + .filter(|r| { + params + .node_type + .as_ref() + .is_none_or(|nt| &r.node.node_type == nt) + }) + .filter(|r| { + params + .tag + .as_ref() + .is_none_or(|tag| r.node.tags.iter().any(|t| t == tag)) + }) .map(|r| { serde_json::json!({ "id": r.node.id, diff --git a/crates/vestige-mcp/src/protocol/http.rs b/crates/vestige-mcp/src/protocol/http.rs index 8bb788c..ac5b325 100644 --- a/crates/vestige-mcp/src/protocol/http.rs +++ b/crates/vestige-mcp/src/protocol/http.rs @@ -174,10 +174,17 @@ fn validate_auth(headers: &HeaderMap, expected: &str) -> Result<(), (StatusCode, .and_then(|v| v.to_str().ok()) .ok_or((StatusCode::UNAUTHORIZED, "Missing Authorization header"))?; - let token = header.strip_prefix("Bearer ").ok_or(( - StatusCode::UNAUTHORIZED, - "Invalid Authorization scheme (expected Bearer)", - ))?; + // The auth-scheme token is case-insensitive per RFC 7235/6750, so match + // "Bearer" case-insensitively (accepting "bearer", "BEARER", ...) while + // preserving the token's original case. + let token = header + .split_once(' ') + .filter(|(scheme, _)| scheme.eq_ignore_ascii_case("Bearer")) + .map(|(_, token)| token.trim_start()) + .ok_or(( + StatusCode::UNAUTHORIZED, + "Invalid Authorization scheme (expected Bearer)", + ))?; // Constant-time comparison: prevents timing side-channel attacks. // We first check lengths match (length itself is not secret since UUIDs @@ -242,8 +249,12 @@ fn validate_accept(headers: &HeaderMap) -> Result<(), (StatusCode, &'static str) .split(',') .map(|part| part.trim().split(';').next().unwrap_or("").trim()) { - accepts_json |= mime == "application/json"; - accepts_sse |= mime == "text/event-stream"; + // `*/*` accepts anything; `application/*` / `text/*` accept a whole type. + // Honoring these lets generic HTTP clients (curl's default Accept: */*) + // reach /mcp instead of getting a hard 406. + accepts_json |= + mime == "application/json" || mime == "application/*" || mime == "*/*"; + accepts_sse |= mime == "text/event-stream" || mime == "text/*" || mime == "*/*"; } if accepts_json && accepts_sse { diff --git a/crates/vestige-mcp/src/tools/context.rs b/crates/vestige-mcp/src/tools/context.rs index 10c70d1..2df73c0 100644 --- a/crates/vestige-mcp/src/tools/context.rs +++ b/crates/vestige-mcp/src/tools/context.rs @@ -69,14 +69,17 @@ pub async fn execute(storage: &Arc, args: Option) -> Result + // process abort under panic=abort) and a huge value overflows `limit * 2`. + let limit = args["limit"].as_i64().unwrap_or(10).clamp(1, 200) as i32; let now = Utc::now(); // Get candidate memories let recall_input = RecallInput { query: query.to_string(), - limit: limit * 2, // Get more, then filter + limit: limit.saturating_mul(2), // Get more, then filter min_retention: 0.0, search_mode: SearchMode::Hybrid, valid_at: None,