fix: prevent memory contamination between agents in different sessions#17
Merged
Conversation
#7) This commit fixes a critical security vulnerability where agents in different sessions could access each other's memory due to shared default namespace, lack of session-based isolation, and no access control enforcement. Changes: - Add MemoryAccessControl class for session-based memory isolation - Require sessionId in AgentContext for all memory operations - Add deleteByNamespace() to SQLite store for session cleanup - Enforce namespace isolation in MemoryManager operations (store, get, list, search, delete) - Update spawnAgent to generate memoryNamespace for each agent - Add session cleanup in sessionEndHook (delete memory + stop agents) - Secure MCP memory tools with required sessionId parameter - Add optional sessionId filtering to REST memory endpoints - Add memoryNamespace field to SpawnedAgent type Tests: - 23 unit tests for MemoryAccessControl - 11 integration tests for end-to-end session isolation - Updated existing MCP memory tools tests Closes #7 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughImplements session-scoped memory isolation and access control: new MemoryAccessControl, per-session namespaces for agents, MemoryManager and MCP tools require/derive sessionId, SQLite persistence and active_agents updated with memory_namespace, and session-end cleanup that deletes session memory and stops session agents. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Spawner as Agent Spawner
participant Access as MemoryAccessControl
participant Memory as Memory Manager
participant Store as SQLite Store
Client->>Spawner: spawnAgent(options{sessionId})
Spawner->>Access: getSessionNamespace(sessionId)
Access-->>Spawner: namespace = "session:<id>"
Spawner->>Store: saveActiveAgent(..., memory_namespace)
Store-->>Spawner: persisted
Spawner-->>Client: SpawnedAgent { memoryNamespace }
Client->>Memory: store(key, content, sessionId)
Memory->>Access: validateAccess(sessionId, write)
Access-->>Memory: allowed
Memory->>Access: deriveNamespace(sessionId)
Access-->>Memory: namespace = "session:<id>"
Memory->>Store: insert(key, content, namespace)
Store-->>Memory: stored
Memory-->>Client: success
sequenceDiagram
participant SessionHook as Session Manager
participant Access as MemoryAccessControl
participant Store as SQLite Store
participant Spawner as Agent Spawner
participant Memory as Memory Manager
SessionHook->>Access: getSessionNamespace(sessionId)
Access-->>SessionHook: namespace = "session:<id>"
SessionHook->>Store: deleteByNamespace(namespace)
Store-->>SessionHook: rows_deleted
SessionHook->>Spawner: listAgents(sessionId)
Spawner-->>SessionHook: [agents...]
loop for each agent
SessionHook->>Spawner: stopAgent(agentId)
Spawner-->>SessionHook: stopped
end
SessionHook->>Memory: clearAgentContext()
Memory-->>SessionHook: context cleared
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/memory/sqlite-store.ts (1)
279-287: Add a migration foractive_agents.memory_namespace.
CREATE TABLE IF NOT EXISTSwon’t add the new column on existing DBs, so the new INSERT/UPDATE statements will fail at runtime. Add a lightweight migration (e.g.,ALTER TABLE) during initialization.🛠️ Proposed fix
private initSchema(): void { this.db.exec(SCHEMA); + this.ensureSchemaMigrations(); } + + private ensureSchemaMigrations(): void { + const columns = this.db + .prepare('PRAGMA table_info(active_agents)') + .all() as Array<{ name: string }>; + if (!columns.some(c => c.name === 'memory_namespace')) { + this.db.exec('ALTER TABLE active_agents ADD COLUMN memory_namespace TEXT'); + } + }Also applies to: 1732-1760
src/mcp/tools/memory-tools.ts (1)
186-324: Always clear agent context on handler errors.
memory_get,memory_list, andmemory_deletedon’t clear context when an exception is thrown (e.g., access denied). That risks stale context leaking into later operations. Usetry/finally.🛠️ Proposed fix
- // Set agent context for this operation - memory.setAgentContext({ - sessionId: input.sessionId, - }); - - const entry = memory.get(input.key, namespace); - - // Clear context after operation - memory.clearAgentContext(); - - if (!entry) { - return { - found: false, - message: 'Entry not found', - }; - } - - return { - found: true, - entry: { - id: entry.id, - key: entry.key, - content: entry.content, - namespace: entry.namespace, - metadata: entry.metadata, - createdAt: entry.createdAt.toISOString(), - updatedAt: entry.updatedAt.toISOString(), - }, - }; + // Set agent context for this operation + memory.setAgentContext({ + sessionId: input.sessionId, + }); + + try { + const entry = memory.get(input.key, namespace); + + if (!entry) { + return { + found: false, + message: 'Entry not found', + }; + } + + return { + found: true, + entry: { + id: entry.id, + key: entry.key, + content: entry.content, + namespace: entry.namespace, + metadata: entry.metadata, + createdAt: entry.createdAt.toISOString(), + updatedAt: entry.updatedAt.toISOString(), + }, + }; + } finally { + memory.clearAgentContext(); + }- // Set agent context for this operation - // Only set agentId if explicitly provided for filtering - memory.setAgentContext({ - agentId: input.agentId, // undefined if not provided - sessionId: input.sessionId, - includeShared: input.includeShared ?? true, // Default to including shared - }); - - const entries = memory.list(namespace, input.limit, input.offset, { - agentId: input.agentId, - includeShared: input.includeShared ?? true, // Default to including shared - }); - const total = memory.count(namespace); - - // Clear context after operation - memory.clearAgentContext(); - - return { - total, - count: entries.length, - offset: input.offset ?? 0, - entries: entries.map(e => ({ - id: e.id, - key: e.key, - namespace: e.namespace, - agentId: e.agentId, - contentPreview: e.content.slice(0, 200) + (e.content.length > 200 ? '...' : ''), - metadata: e.metadata, - createdAt: e.createdAt.toISOString(), - updatedAt: e.updatedAt.toISOString(), - })), - }; + // Set agent context for this operation + // Only set agentId if explicitly provided for filtering + memory.setAgentContext({ + agentId: input.agentId, // undefined if not provided + sessionId: input.sessionId, + includeShared: input.includeShared ?? true, // Default to including shared + }); + + try { + const entries = memory.list(namespace, input.limit, input.offset, { + agentId: input.agentId, + includeShared: input.includeShared ?? true, // Default to including shared + }); + const total = memory.count(namespace); + + return { + total, + count: entries.length, + offset: input.offset ?? 0, + entries: entries.map(e => ({ + id: e.id, + key: e.key, + namespace: e.namespace, + agentId: e.agentId, + contentPreview: e.content.slice(0, 200) + (e.content.length > 200 ? '...' : ''), + metadata: e.metadata, + createdAt: e.createdAt.toISOString(), + updatedAt: e.updatedAt.toISOString(), + })), + }; + } finally { + memory.clearAgentContext(); + }- // Set agent context for this operation - memory.setAgentContext({ - sessionId: input.sessionId, - }); - - const deleted = memory.delete(input.key, namespace); - - // Clear context after operation - memory.clearAgentContext(); - - return { - success: deleted, - message: deleted ? 'Entry deleted' : 'Entry not found', - }; + // Set agent context for this operation + memory.setAgentContext({ + sessionId: input.sessionId, + }); + + try { + const deleted = memory.delete(input.key, namespace); + return { + success: deleted, + message: deleted ? 'Entry deleted' : 'Entry not found', + }; + } finally { + memory.clearAgentContext(); + }
🤖 Fix all issues with AI agents
In `@src/memory/access-control.ts`:
- Around line 61-71: In validateAccess, the local variable sessionNamespace
(assigned via this.getSessionNamespace(context.sessionId)) is declared but never
used, triggering the no-unused-vars lint error; either remove the unused
declaration or use it where intended—open the validateAccess method and delete
the line declaring sessionNamespace, or if session-specific namespace checks
were intended, replace the unused variable with actual logic that uses
this.getSessionNamespace(context.sessionId) to enforce namespace rules; ensure
references to getSessionNamespace and validateAccess remain consistent.
- Around line 126-132: The canAccessEntry agent scoping is too permissive:
update the check in canAccessEntry to explicitly consider entry.shared and
require matching agentId when includeShared is false, and when includeShared is
true allow either a matching agentId or a shared entry. Specifically, inside the
context.agentId branch (where entryAgentId is available) change the condition so
that if context.includeShared === false you return false unless entryAgentId ===
context.agentId AND entry.shared !== true; if context.includeShared === true
only allow access when entryAgentId === context.agentId OR entry.shared ===
true; also handle cases where entryAgentId is missing by denying access unless
entry.shared === true and context.includeShared === true.
In `@src/web/routes/memory.ts`:
- Around line 31-37: deriveNamespace currently allows an explicit namespace like
"session:*" even when sessionId is missing, bypassing setSessionContext and
access checks; update deriveNamespace to reject session-scoped explicit
namespaces when sessionId is absent (i.e., if explicitNamespace startsWith
"session:" and sessionId is falsy, return undefined), and ensure callers still
call setSessionContext(sessionId) only when sessionId is present so
accessControl.getSessionNamespace(sessionId) is used for derived session
namespaces.
- Around line 79-93: The route sets session context with
setSessionContext(manager, sessionId, body.agentId) but calls
manager.clearAgentContext() only on the happy path, risking context leakage if
manager.store (or other manager methods) throws; change the flow to call
setSessionContext before invoking manager.store/search/list/get/delete and
ensure manager.clearAgentContext() is always invoked in a finally block (i.e.,
wrap the manager.* call(s) in try { ... } finally { manager.clearAgentContext();
}) so the agent context is cleared regardless of errors—apply the same pattern
to all routes that use setSessionContext.
- Around line 51-53: The pagination total is wrong because manager.count()
doesn't accept the same filtering options as manager.list(); update the count
method signature to count(namespace: string, options?: { agentId?: string;
includeShared?: boolean }) and implement the same filtering logic used by list
(apply agentId and includeShared in the query/where clause), then change the
call in memory route (where you call manager.count(namespace)) to pass the same
options object you pass to manager.list (e.g., { agentId }). Ensure the unique
symbols manager.count, manager.list, and the options shape are used consistently
so pagination totals match the filtered list.
🧹 Nitpick comments (1)
src/memory/sqlite-store.ts (1)
538-577: Consider relying on FK cascades indeleteByNamespace.The schema already uses
ON DELETE CASCADEformemory_tags,memory_versions, andmemory_relationships, so manual deletes can be removed for a simpler and cheaper implementation.♻️ Proposed simplification
return this.transaction((db) => { - // Get all memory IDs in this namespace - const memoryIds = db - .prepare('SELECT id FROM memory WHERE namespace = ?') - .all(namespace) as Array<{ id: string }>; - - if (memoryIds.length === 0) { - return 0; - } - - const ids = memoryIds.map(m => m.id); - - // Delete memory_tags (junction table) - const placeholders = ids.map(() => '?').join(','); - db.prepare(`DELETE FROM memory_tags WHERE memory_id IN (${placeholders})`).run(...ids); - - // Delete memory_versions - db.prepare(`DELETE FROM memory_versions WHERE memory_id IN (${placeholders})`).run(...ids); - - // Delete memory_relationships (both directions) - db.prepare(`DELETE FROM memory_relationships WHERE from_id IN (${placeholders}) OR to_id IN (${placeholders})`).run(...ids, ...ids); - - // Delete the memory entries themselves const result = db .prepare('DELETE FROM memory WHERE namespace = ?') .run(namespace); log.info('Deleted memory by namespace', { namespace, count: result.changes }); return result.changes; });
Comment on lines
+126
to
+132
| // If agentId filter is set on context and entry has an agentId | ||
| if (context.agentId && entryAgentId) { | ||
| // If includeShared is false, only allow matching agentId | ||
| if (context.includeShared === false && entryAgentId !== context.agentId) { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
Tighten agent scoping in canAccessEntry.
Current logic allows access to other agents’ private entries when includeShared is true, and doesn’t exclude shared entries when includeShared is false. This breaks agent-level isolation inside a session.
🛠️ Proposed fix
- if (context.agentId && entryAgentId) {
- // If includeShared is false, only allow matching agentId
- if (context.includeShared === false && entryAgentId !== context.agentId) {
- return false;
- }
- }
+ if (context.agentId) {
+ // Never allow access to another agent's private entry
+ if (entryAgentId && entryAgentId !== context.agentId) {
+ return false;
+ }
+ // If includeShared is false, exclude shared (agent_id = NULL) entries
+ if (context.includeShared === false && !entryAgentId) {
+ return false;
+ }
+ }🤖 Prompt for AI Agents
In `@src/memory/access-control.ts` around lines 126 - 132, The canAccessEntry
agent scoping is too permissive: update the check in canAccessEntry to
explicitly consider entry.shared and require matching agentId when includeShared
is false, and when includeShared is true allow either a matching agentId or a
shared entry. Specifically, inside the context.agentId branch (where
entryAgentId is available) change the condition so that if context.includeShared
=== false you return false unless entryAgentId === context.agentId AND
entry.shared !== true; if context.includeShared === true only allow access when
entryAgentId === context.agentId OR entry.shared === true; also handle cases
where entryAgentId is missing by denying access unless entry.shared === true and
context.includeShared === true.
Comment on lines
+51
to
53
| const entries = manager.list(namespace, limit, offset, { agentId }); | ||
| const total = manager.count(namespace); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether MemoryManager.count supports agentId/filters.
rg -n "count\s*\(" src/memory/index.ts src/memory/sqlite-store.ts -C2Repository: blackms/aistack
Length of output: 613
🏁 Script executed:
#!/bin/bash
# Check the list() method signature and implementation
rg -n "^\s*list\s*\(" src/memory/index.ts src/memory/sqlite-store.ts -A5 -B2Repository: blackms/aistack
Length of output: 853
🏁 Script executed:
#!/bin/bash
# Check the actual implementation details of list() and count() in sqlite-store
rg -n "list\s*\(" src/memory/sqlite-store.ts -A25 | head -50Repository: blackms/aistack
Length of output: 907
🏁 Script executed:
#!/bin/bash
# Check the full count() implementation in sqlite-store
sed -n '617,635p' src/memory/sqlite-store.tsRepository: blackms/aistack
Length of output: 685
🏁 Script executed:
#!/bin/bash
# Verify the usage context in memory.ts to confirm the issue
sed -n '48,55p' src/web/routes/memory.tsRepository: blackms/aistack
Length of output: 297
Update count() to respect agentId filtering for accurate pagination totals.
list() filters by agentId (via options parameter), but count() does not accept this parameter, causing the total to include entries from other agents and mislead pagination.
The count() method signature should accept the same options?: { agentId?: string; includeShared?: boolean } parameter as list() and apply the corresponding filters in the query.
🤖 Prompt for AI Agents
In `@src/web/routes/memory.ts` around lines 51 - 53, The pagination total is wrong
because manager.count() doesn't accept the same filtering options as
manager.list(); update the count method signature to count(namespace: string,
options?: { agentId?: string; includeShared?: boolean }) and implement the same
filtering logic used by list (apply agentId and includeShared in the query/where
clause), then change the call in memory route (where you call
manager.count(namespace)) to pass the same options object you pass to
manager.list (e.g., { agentId }). Ensure the unique symbols manager.count,
manager.list, and the options shape are used consistently so pagination totals
match the filtered list.
Comment on lines
+79
to
+93
| const manager = getManager(); | ||
| const sessionId = body.sessionId ?? params.query.sessionId; | ||
| const namespace = deriveNamespace(sessionId, body.namespace); | ||
|
|
||
| setSessionContext(manager, sessionId, body.agentId); | ||
|
|
||
| const entry = await manager.store(body.key, body.content, { | ||
| namespace: body.namespace, | ||
| namespace, | ||
| metadata: body.metadata, | ||
| generateEmbedding: body.generateEmbedding, | ||
| agentId: body.agentId, | ||
| }); | ||
|
|
||
| manager.clearAgentContext(); | ||
|
|
There was a problem hiding this comment.
Always clear agent context in a finally block.
If store/search/list/get/delete throws, the context remains set and can leak into subsequent requests, undermining isolation.
♻️ Suggested fix (apply similarly to other routes using setSessionContext)
- setSessionContext(manager, sessionId, body.agentId);
-
- const entry = await manager.store(body.key, body.content, {
- namespace,
- metadata: body.metadata,
- generateEmbedding: body.generateEmbedding,
- agentId: body.agentId,
- });
-
- manager.clearAgentContext();
+ setSessionContext(manager, sessionId, body.agentId);
+ let entry;
+ try {
+ entry = await manager.store(body.key, body.content, {
+ namespace,
+ metadata: body.metadata,
+ generateEmbedding: body.generateEmbedding,
+ agentId: body.agentId,
+ });
+ } finally {
+ manager.clearAgentContext();
+ }🤖 Prompt for AI Agents
In `@src/web/routes/memory.ts` around lines 79 - 93, The route sets session
context with setSessionContext(manager, sessionId, body.agentId) but calls
manager.clearAgentContext() only on the happy path, risking context leakage if
manager.store (or other manager methods) throws; change the flow to call
setSessionContext before invoking manager.store/search/list/get/delete and
ensure manager.clearAgentContext() is always invoked in a finally block (i.e.,
wrap the manager.* call(s) in try { ... } finally { manager.clearAgentContext();
}) so the agent context is cleared regardless of errors—apply the same pattern
to all routes that use setSessionContext.
- Add error handling tests for memory_store and memory_search - Add test for session end with active agents (covers agent cleanup) - Add test for session memory cleanup on session end These tests cover previously uncovered error paths: - memory-tools.ts: lines 110-115, 176-182 (catch blocks) - session.ts: lines 94-95, 97-98 (agent cleanup code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add withContext() helper to reduce context management duplication in memory-tools.ts - Add comprehensive JSDoc documentation to access-control.ts - Improve logging in session.ts: use debug level when no entries deleted - Require sessionId for write/delete operations in REST memory endpoints - Update routes tests to include sessionId for write operations Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/web/routes/memory.ts`:
- Around line 22-28: The setSessionContext function currently defaults agentId
to sessionId which alters access semantics; change setSessionContext (the
function that calls manager.setAgentContext) so it only includes agentId in the
object when agentId is explicitly provided (i.e., set { sessionId } when agentId
is undefined, and { agentId, sessionId } only when agentId is truthy),
preserving agentId as an optional filter rather than forcing it to sessionId.
Comment on lines
+22
to
+28
| function setSessionContext(manager: ReturnType<typeof getManager>, sessionId?: string, agentId?: string) { | ||
| if (sessionId) { | ||
| manager.setAgentContext({ | ||
| agentId: agentId ?? sessionId, | ||
| sessionId, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Don’t default agentId to sessionId.
agentId is an optional filter; forcing it to sessionId changes access semantics and diverges from MCP tools (which only set agentId when provided). This can inadvertently hide agent-owned entries once stricter agent scoping is enforced.
✅ Suggested fix
function setSessionContext(manager: ReturnType<typeof getManager>, sessionId?: string, agentId?: string) {
if (sessionId) {
manager.setAgentContext({
- agentId: agentId ?? sessionId,
- sessionId,
+ sessionId,
+ ...(agentId ? { agentId } : {}),
});
}
}🤖 Prompt for AI Agents
In `@src/web/routes/memory.ts` around lines 22 - 28, The setSessionContext
function currently defaults agentId to sessionId which alters access semantics;
change setSessionContext (the function that calls manager.setAgentContext) so it
only includes agentId in the object when agentId is explicitly provided (i.e.,
set { sessionId } when agentId is undefined, and { agentId, sessionId } only
when agentId is truthy), preserving agentId as an optional filter rather than
forcing it to sessionId.
Fixes lint error: 'sessionNamespace' is assigned a value but never used Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes a critical security vulnerability where agents in different sessions could access each other's memory due to:
getById()having no access filteringChanges
Core Access Control (
src/memory/access-control.ts- NEW)MemoryAccessContextinterface with requiredsessionIdMemoryAccessControlclass for validating session-based accessgetSessionNamespace(),validateAccess(),canAccessEntry(),deriveNamespace()Memory Manager (
src/memory/index.ts)AgentContextto requiresessionIdSQLite Store (
src/memory/sqlite-store.ts)deleteByNamespace()for session cleanupactive_agentsschema withmemory_namespacecolumnAgent Spawner (
src/agents/spawner.ts)memoryNamespaceusingsession:${sessionId}orsession:${agentId}as fallbackSession Hooks (
src/hooks/session.ts)MCP Tools (
src/mcp/tools/memory-tools.ts)sessionIdrequired for all memory operationsREST Endpoints (
src/web/routes/memory.ts)sessionIdquery parameter for session-scoped operationsTypes (
src/types.ts)memoryNamespace?: stringtoSpawnedAgentTest plan
npm test- 1,874 tests passingtests/unit/memory-access-control.test.tstests/integration/memory-isolation.test.tsgetById()denies access to other session's entriesCloses #7
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.