-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
fix(server-core): enforce memory conversation ownership #1388
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 2 commits
6e64a3c
95ba4d9
96bcbab
8c5ff4f
ca88a84
c7c2c28
a4d2f31
2cf761b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -58,6 +58,10 @@ export type ConversationQueryOptions = { | |
| orderDirection?: "ASC" | "DESC"; | ||
| }; | ||
|
|
||
| export type ConversationMutationOptions = { | ||
| expectedUserId?: string; | ||
| }; | ||
|
|
||
| /** | ||
| * Options for getting messages | ||
| */ | ||
|
|
@@ -443,8 +447,9 @@ export interface StorageAdapter { | |
| updateConversation( | ||
| id: string, | ||
| updates: Partial<Omit<Conversation, "id" | "createdAt" | "updatedAt">>, | ||
| options?: ConversationMutationOptions, | ||
| ): Promise<Conversation>; | ||
| deleteConversation(id: string): Promise<void>; | ||
| deleteConversation(id: string, options?: ConversationMutationOptions): Promise<void>; | ||
|
coderabbitai[bot] marked this conversation as resolved.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P1: Persistent storage adapters ignore the new Prompt for AI agents |
||
|
|
||
| saveConversationSteps?(steps: ConversationStepRecord[]): Promise<void>; | ||
| getConversationSteps?( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,239 @@ | ||
| import { | ||
| ConversationOwnershipMismatchError, | ||
| InMemoryStorageAdapter, | ||
| Memory, | ||
| } from "@voltagent/core"; | ||
| import type { Agent, Logger, ServerProviderDeps, VoltOpsClient } from "@voltagent/core"; | ||
| import { beforeEach, describe, expect, it, vi } from "vitest"; | ||
| import { | ||
| handleDeleteMemoryConversation, | ||
| handleGetMemoryConversation, | ||
| handleListMemoryConversationMessages, | ||
| handleListMemoryConversations, | ||
| handleUpdateMemoryConversation, | ||
| } from "./memory.handlers"; | ||
|
|
||
| function createAgentWithMemory(agentId: string, agentName: string, memory: Memory): Agent { | ||
| return { | ||
| getFullState: () => ({ | ||
| id: agentId, | ||
| name: agentName, | ||
| instructions: "", | ||
| status: "idle", | ||
| model: "test-model", | ||
| tools: [], | ||
| subAgents: [], | ||
| memory: {}, | ||
| }), | ||
| getMemory: () => memory, | ||
| } as unknown as Agent; | ||
| } | ||
|
|
||
| function createDepsWithAgents(agents: Agent[]): ServerProviderDeps { | ||
| const logger: Logger = { | ||
| trace: vi.fn(), | ||
| debug: vi.fn(), | ||
| info: vi.fn(), | ||
| warn: vi.fn(), | ||
| error: vi.fn(), | ||
| fatal: vi.fn(), | ||
| child: vi.fn().mockReturnThis(), | ||
| level: "info", | ||
| silent: vi.fn(), | ||
| } as unknown as Logger; | ||
|
|
||
| return { | ||
| agentRegistry: { | ||
| getAgent: vi.fn((agentId: string) => | ||
| agents.find((agent) => agent.getFullState().id === agentId), | ||
| ), | ||
| getAllAgents: vi.fn().mockReturnValue(agents), | ||
| getAgentCount: vi.fn().mockReturnValue(agents.length), | ||
| removeAgent: vi.fn(), | ||
| registerAgent: vi.fn(), | ||
| getGlobalVoltOpsClient: vi.fn().mockReturnValue(undefined as unknown as VoltOpsClient), | ||
| getGlobalLogger: vi.fn().mockReturnValue(logger), | ||
| }, | ||
| workflowRegistry: { | ||
| getWorkflow: vi.fn(), | ||
| getWorkflowsForApi: vi.fn().mockReturnValue([]), | ||
| getWorkflowDetailForApi: vi.fn(), | ||
| getWorkflowCount: vi.fn().mockReturnValue(0), | ||
| on: vi.fn(), | ||
| off: vi.fn(), | ||
| activeExecutions: new Map(), | ||
| resumeSuspendedWorkflow: vi.fn(), | ||
| }, | ||
| triggerRegistry: { | ||
| list: vi.fn().mockReturnValue([]), | ||
| register: vi.fn(), | ||
| registerMany: vi.fn(), | ||
| get: vi.fn(), | ||
| getByPath: vi.fn(), | ||
| unregister: vi.fn(), | ||
| clear: vi.fn(), | ||
| } as any, | ||
| logger, | ||
| } as unknown as ServerProviderDeps; | ||
| } | ||
|
|
||
| describe("memory handlers ownership checks", () => { | ||
| let memory: Memory; | ||
| let deps: ServerProviderDeps; | ||
| const agentId = "agent-1"; | ||
| const ownerUserId = "user-alice"; | ||
| const otherUserId = "user-bob"; | ||
| const conversationId = "conv-private"; | ||
| const otherConversationId = "conv-bob"; | ||
|
|
||
| beforeEach(async () => { | ||
| memory = new Memory({ | ||
| storage: new InMemoryStorageAdapter(), | ||
| }); | ||
|
|
||
| await memory.createConversation({ | ||
| id: conversationId, | ||
| resourceId: agentId, | ||
| userId: ownerUserId, | ||
| title: "Alice Private", | ||
| metadata: {}, | ||
| }); | ||
|
|
||
| await memory.createConversation({ | ||
| id: otherConversationId, | ||
| resourceId: agentId, | ||
| userId: otherUserId, | ||
| title: "Bob Private", | ||
| metadata: {}, | ||
| }); | ||
|
|
||
| await memory.addMessage( | ||
| { | ||
| id: "msg-1", | ||
| role: "user", | ||
| parts: [{ type: "text", text: "Confidential" }], | ||
| }, | ||
| ownerUserId, | ||
| conversationId, | ||
| ); | ||
|
|
||
| deps = createDepsWithAgents([createAgentWithMemory(agentId, "Agent One", memory)]); | ||
| }); | ||
|
|
||
| it("rejects reading a conversation owned by a different authenticated user", async () => { | ||
| const result = await handleGetMemoryConversation(deps, conversationId, { | ||
| agentId, | ||
| requestingUserId: otherUserId, | ||
| }); | ||
|
|
||
| expect(result.success).toBe(false); | ||
| expect(result.httpStatus).toBe(403); | ||
| }); | ||
|
|
||
| it("rejects reading a conversation when the authenticated identity is empty", async () => { | ||
| const result = await handleGetMemoryConversation(deps, conversationId, { | ||
| agentId, | ||
| requestingUserId: "", | ||
| }); | ||
|
|
||
| expect(result.success).toBe(false); | ||
| expect(result.httpStatus).toBe(403); | ||
| }); | ||
|
|
||
| it("lists conversations for the authenticated user instead of a client-supplied userId", async () => { | ||
| const result = await handleListMemoryConversations(deps, { | ||
| agentId, | ||
| userId: ownerUserId, | ||
| requestingUserId: otherUserId, | ||
| }); | ||
|
|
||
| expect(result.success).toBe(true); | ||
| if (!result.success) return; | ||
| expect(result.data.total).toBe(1); | ||
| expect(result.data.conversations).toHaveLength(1); | ||
| expect(result.data.conversations[0]?.id).toBe(otherConversationId); | ||
| expect(result.data.conversations[0]?.userId).toBe(otherUserId); | ||
| }); | ||
|
|
||
| it("rejects listing messages for a conversation owned by a different authenticated user", async () => { | ||
|
cubic-dev-ai[bot] marked this conversation as resolved.
|
||
| const result = await handleListMemoryConversationMessages(deps, conversationId, { | ||
| agentId, | ||
| requestingUserId: otherUserId, | ||
| }); | ||
|
|
||
| expect(result.success).toBe(false); | ||
| expect(result.httpStatus).toBe(403); | ||
| }); | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
|
|
||
| it("rejects updating a conversation owned by a different authenticated user", async () => { | ||
| const result = await handleUpdateMemoryConversation(deps, conversationId, { | ||
| agentId, | ||
| requestingUserId: otherUserId, | ||
| title: "Bob title", | ||
| } as any); | ||
|
|
||
| expect(result.success).toBe(false); | ||
| expect(result.httpStatus).toBe(403); | ||
|
|
||
| const conversation = await memory.getConversation(conversationId); | ||
| expect(conversation?.title).toBe("Alice Private"); | ||
| }); | ||
|
|
||
| it("returns forbidden when a guarded storage update no longer affects the owner", async () => { | ||
| const updateSpy = vi | ||
| .spyOn(memory, "updateConversation") | ||
| .mockRejectedValueOnce(new ConversationOwnershipMismatchError(conversationId)); | ||
|
|
||
| const result = await handleUpdateMemoryConversation(deps, conversationId, { | ||
| agentId, | ||
| requestingUserId: ownerUserId, | ||
| title: "Updated title", | ||
| }); | ||
|
|
||
| expect(result.success).toBe(false); | ||
| expect(result.httpStatus).toBe(403); | ||
| expect(updateSpy).toHaveBeenCalledWith( | ||
| conversationId, | ||
| { title: "Updated title" }, | ||
| { expectedUserId: ownerUserId }, | ||
| ); | ||
| }); | ||
|
|
||
| it("rejects deleting a conversation owned by a different authenticated user", async () => { | ||
| const result = await handleDeleteMemoryConversation(deps, conversationId, { | ||
| agentId, | ||
| requestingUserId: otherUserId, | ||
| }); | ||
|
|
||
| expect(result.success).toBe(false); | ||
| expect(result.httpStatus).toBe(403); | ||
|
|
||
| await expect(memory.getConversation(conversationId)).resolves.not.toBeNull(); | ||
| }); | ||
|
|
||
| it("returns forbidden when a guarded storage delete no longer affects the owner", async () => { | ||
| const deleteSpy = vi | ||
| .spyOn(memory, "deleteConversation") | ||
| .mockRejectedValueOnce(new ConversationOwnershipMismatchError(conversationId)); | ||
|
|
||
| const result = await handleDeleteMemoryConversation(deps, conversationId, { | ||
| agentId, | ||
| requestingUserId: ownerUserId, | ||
| }); | ||
|
|
||
| expect(result.success).toBe(false); | ||
| expect(result.httpStatus).toBe(403); | ||
| expect(deleteSpy).toHaveBeenCalledWith(conversationId, { expectedUserId: ownerUserId }); | ||
| }); | ||
|
|
||
| it("allows the owning authenticated user to manage the conversation", async () => { | ||
| const result = await handleGetMemoryConversation(deps, conversationId, { | ||
| agentId, | ||
| requestingUserId: ownerUserId, | ||
| }); | ||
|
|
||
| expect(result.success).toBe(true); | ||
| if (!result.success) return; | ||
| expect(result.data.conversation.userId).toBe(ownerUserId); | ||
| }); | ||
| }); | ||
Uh oh!
There was an error while loading. Please reload this page.