fix(backend): resolve 10 bugs from the final full-codebase audit (#141)

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) <noreply@anthropic.com>
This commit is contained in:
Sam Valladares 2026-07-02 16:05:40 -05:00 committed by GitHub
parent b7a021417b
commit 0e5c71b4ca
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
10 changed files with 152 additions and 44 deletions

View file

@ -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 });
}

View file

@ -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<Utc>, DateTime<Utc>)>, |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

View file

@ -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);

View file

@ -28,26 +28,18 @@ pub fn sanitize_fts5_terms(query: &str) -> Option<String> {
})
.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);
}
}

View file

@ -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<Utc>) -> DateTime<Utc> {
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::<Utc>::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<Utc>) -> DateTime<Utc> {
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::<Utc>::MAX_UTC)
}
/// Check if a time is within the capture window

View file

@ -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

View file

@ -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<String>>(1)?)),
)
.optional()?

View file

@ -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,

View file

@ -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 {

View file

@ -69,14 +69,17 @@ pub async fn execute(storage: &Arc<Storage>, args: Option<Value>) -> Result<Valu
let time_weight = args["time_weight"].as_f64().unwrap_or(0.3);
let topic_weight = args["topic_weight"].as_f64().unwrap_or(0.4);
let limit = args["limit"].as_i64().unwrap_or(10) as i32;
// Clamp caller-supplied limit to a sane range. Without this a negative value
// reaches Vec::with_capacity as a huge usize (capacity-overflow panic ->
// 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,