fix: multi-agent isolation + SQL safety at the data layer (#19) - #29
Conversation
Configurable isolation policy, SQL-safety fixes, by-id scoping, and server-derived caller identity. Stacked on #28 (fix/mcp-http-auth). SQL safety (unconditional): - increment_recall_counts: allowlist the interpolated table identifier ({memory_entries, conversations, delegations}); reject anything else. - replace()/remove(): escape LIKE metacharacters (\ % _) + ESCAPE '\', so remove(old_text="%") no longer wipes the whole (agent,target) scope. Isolation policy (HEXUS_MEMORY_ISOLATION, default "shared"): - shared: reads/recall/search span all agents (trusted-fleet default). - strict: reads scoped to the caller's identity. - Cross-agent MUTATIONS (confirm/reject/remove/forget/summarize by id) are always caller-scoped, in both modes. - Resolves the empty-agent_identity asymmetry (item C): empty means "all agents" in shared / "this agent" in strict, consistently. - list_entries now treats agent_identity=None as all-agents, matching search()/count(). By-id scoping (item B): - fetch_full/confirm_entry/reject_entry/summarize_session take an optional agent_identity and filter on it; fetch_full bypasses the id-keyed CCRCache when strict-scoped so it can't leak another agent's row. Threaded through the MCP tools and the hermes handlers. Server-derived identity (item A): - HTTP transport publishes the authenticated X-Hermes-Session-Key as the caller identity (tools.current_caller ContextVar); it overrides the client-supplied agent_identity for writes/mutations, so an authenticated client can no longer act as another agent. stdio / in-process callers keep the prior behavior. Tests: SQL-safety + isolation unit tests (no DB) in test_http_auth.py; DB-backed cross-agent scoping tests in test_mcp_server.py. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| return | ||
| headers = dict(scope.get("headers") or []) | ||
| raw = headers.get(b"x-hermes-session-key", b"").decode("latin-1").strip() | ||
| token = tools.current_caller.set(raw or None) |
There was a problem hiding this comment.
WARNING: The tools.current_caller ContextVar is set per-request here, but the server-derived identity depends on this ContextVar propagating into FastMCP's tool dispatch. If FastMCP internally spawns tool execution in a context-isolated task, the ContextVar value is lost and _write_identity/_scope_identity fall through to the client-supplied agent_identity arg — defeating the isolation model.
The PR description flags this as untested end-to-end. Consider adding a runtime assertion or log warning when current_caller.get() returns None inside tool invocations on the HTTP transport to catch this failure mode early.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| When `agent_identity` is supplied the session lookup is scoped to that | ||
| agent, so a caller cannot summarize another agent's session by id. | ||
| """ | ||
| scope = "" if agent_identity is None else " AND agent_identity = %s" |
There was a problem hiding this comment.
SUGGESTION: The scope variable is embedded into SQL via f-string interpolation. While currently safe (the variable is binary: "" or " AND agent_identity = %s"), this pattern is fragile — a future extension adding more dynamic clauses could introduce SQL injection. The rest of this class uses list-based clause construction (e.g. search() at line 1089, count() at line 1891). Consider building clauses as a list and joining them with " AND ".
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (6 files)
Resolved from Previous Review
Previous Review Summary (commit 9a51fa7)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 9a51fa7)Status: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (7 files)
Positive Observations
Reviewed by deepseek-v4-pro · Input: 20.5K · Output: 3.4K · Cached: 89.5K |
…gation warning, update CI packaging check, and bump version to 0.9.2
1 participant
Closes #19. Stacked on #28 (
fix/mcp-http-auth) — base isfix/mcp-http-auth; retarget tomainonce #28 merges.Framing
The #19 review assumed a zero-trust multi-tenant model. This deployment is a trusted fleet that wants shared memory (agy ↔ hermes cross-recall). So the fixes split into genuine bugs (fixed unconditionally) and a configurable isolation policy that defaults to sharing. Decided with @codenamekt: reads shared, mutations scoped, default
shared.SQL safety (unconditional — the original "critical" items)
increment_recall_counts: allowlist the interpolated table identifier ({memory_entries, conversations, delegations}); reject anything else withValueError. A table name is an identifier, not a bindable param, so an allowlist is the safe equivalent of parameterization.replace()/remove(): new_escape_like()helper (escapes\ % _) +ESCAPE '\'on all three LIKE sites.remove(old_text="%")no longer wipes the whole(agent, target)scope;_no longer matches any character.Isolation policy —
HEXUS_MEMORY_ISOLATION(defaultshared)shared: reads/recall/search span all agents — one shared knowledge base for a trusted fleet.strict: reads scoped to the caller's own identity.agent_identityasymmetry (item C): empty means "all agents" in shared / "this agent" in strict, consistently acrossrecall/hybrid_search/search/count.list_entriestreatedagent_identity=NoneasWHERE agent_identity = NULL(zero rows); now all-agents, matchingsearch()/count().By-id scoping (item B)
fetch_full/confirm_entry/reject_entry/summarize_sessiontake an optionalagent_identityand filter on it.fetch_fullbypasses the id-keyedCCRCachewhen strict-scoped so the cache can't leak a row the caller doesn't own. Threaded through the MCP tools and the hermes in-process handlers (which passself._agent_identity).Server-derived identity (item A)
The HTTP transport publishes the authenticated
X-Hermes-Session-Keyas the caller identity (tools.current_callerContextVar, set per-request inside the bearer-auth gate). It overrides a client-suppliedagent_identityfor writes/mutations, so an authenticated client can no longer act as another agent. stdio / in-process callers keep the pre-#19 behavior.Note: #28's auth is a single shared
HEXUS_API_TOKEN(an endpoint gate, not a per-agent credential), so there was no credential→identity mapping to derive from. TheX-Hermes-Session-Keyheader is the fleet's actual per-agent signal, so that's the authoritative source here. A token→identity map is the stronger-isolation upgrade if this ever goes multi-tenant.Tests
tests/test_http_auth.py: pure/no-DB tests for_escape_like,_resolve_isolation, the identity resolvers, and the_wrap_with_identityheader→ContextVar middleware.tests/test_mcp_server.py: DB-backed tests — table allowlist rejection, LIKE%/_escaping, cross-agent confirm/reject/summarize blocked, shared-mode cross-agent read allowed, strict-mode read confinement + CCRCache non-leak.Reviewer notes⚠️
py_compile+ a standalone run of the pure logic. Please run the full suite withPG_TEST_DSNset andmcpinstalled.mcpversion — worth a live check with a realX-Hermes-Session-Keyrequest. Fallback: FastMCPget_http_headers()/ request-context API.🤖 Generated with Claude Code