-
-
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 all commits
6e64a3c
95ba4d9
96bcbab
8c5ff4f
ca88a84
c7c2c28
a4d2f31
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 |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| --- | ||
| "@voltagent/core": patch | ||
| "@voltagent/cloudflare-d1": patch | ||
| "@voltagent/libsql": patch | ||
| "@voltagent/postgres": patch | ||
| "@voltagent/server-core": patch | ||
| "@voltagent/server-elysia": patch | ||
| "@voltagent/server-hono": patch | ||
| "@voltagent/supabase": patch | ||
| "@voltagent/voltagent-memory": patch | ||
| --- | ||
|
|
||
| Harden guarded memory conversation mutations by preserving ownership checks across vector cleanup, D1 deletes, and managed-memory remote mutations. Raise adapter peer dependency minimums to require the core release that exports ownership mismatch errors. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,5 @@ | ||
| import type { D1Database } from "@cloudflare/workers-types"; | ||
| import { ConversationOwnershipMismatchError } from "@voltagent/core"; | ||
| import { describe, expect, it, vi } from "vitest"; | ||
| import { D1MemoryAdapter } from "./memory-adapter"; | ||
|
|
||
|
|
@@ -81,3 +82,79 @@ describe("D1MemoryAdapter queryWorkflowRuns", () => { | |
| ]); | ||
| }); | ||
| }); | ||
|
|
||
| describe("D1MemoryAdapter conversation ownership guards", () => { | ||
| const row = { | ||
| id: "conv-1", | ||
| resource_id: "agent-1", | ||
| user_id: "user-1", | ||
| title: "Original", | ||
| metadata: "{}", | ||
| created_at: "2024-01-01T00:00:00.000Z", | ||
| updated_at: "2024-01-01T00:00:00.000Z", | ||
| }; | ||
|
|
||
| it("adds expectedUserId to updateConversation mutations", async () => { | ||
|
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. P3: The new adapter tests do not verify the security-critical failure path: a guarded update or delete must reject when the owner does not match or when the database affects zero rows. Adding negative tests for each adapter, including assertions that the conversation and child data remain unchanged, would protect the revalidation behavior claimed by this change. Prompt for AI agents |
||
| vi.spyOn(D1MemoryAdapter.prototype as any, "ensureInitialized").mockResolvedValue(undefined); | ||
| const adapter = new D1MemoryAdapter({ binding: createMockBinding(), tablePrefix: "test" }); | ||
| vi.spyOn(adapter as any, "all") | ||
| .mockResolvedValueOnce([row]) | ||
| .mockResolvedValueOnce([{ ...row, title: "Updated" }]); | ||
| const runSpy = vi.spyOn(adapter as any, "run").mockResolvedValue({ meta: { changes: 1 } }); | ||
|
|
||
| await (adapter as any).updateConversation( | ||
| "conv-1", | ||
| { title: "Updated" }, | ||
| { expectedUserId: "user-1" }, | ||
| ); | ||
|
|
||
| const [sql, args] = runSpy.mock.calls[0]; | ||
| expect(sql).toContain("WHERE id = ? AND user_id = ?"); | ||
| expect(args).toEqual([expect.any(String), "Updated", "conv-1", "user-1"]); | ||
| }); | ||
|
|
||
| it("rejects guarded updates before mutating when the existing owner does not match", async () => { | ||
| vi.spyOn(D1MemoryAdapter.prototype as any, "ensureInitialized").mockResolvedValue(undefined); | ||
| const adapter = new D1MemoryAdapter({ binding: createMockBinding(), tablePrefix: "test" }); | ||
| vi.spyOn(adapter as any, "all").mockResolvedValueOnce([{ ...row, user_id: "user-2" }]); | ||
| const runSpy = vi.spyOn(adapter as any, "run").mockResolvedValue({ meta: { changes: 1 } }); | ||
|
|
||
| await expect( | ||
| (adapter as any).updateConversation( | ||
| "conv-1", | ||
| { title: "Updated" }, | ||
| { expectedUserId: "user-1" }, | ||
| ), | ||
| ).rejects.toBeInstanceOf(ConversationOwnershipMismatchError); | ||
|
|
||
| expect(runSpy).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it("uses the guarded parent delete and relies on cascades for owned child rows", async () => { | ||
| vi.spyOn(D1MemoryAdapter.prototype as any, "ensureInitialized").mockResolvedValue(undefined); | ||
| const adapter = new D1MemoryAdapter({ binding: createMockBinding(), tablePrefix: "test" }); | ||
| const runSpy = vi.spyOn(adapter as any, "run").mockResolvedValue({ meta: { changes: 1 } }); | ||
|
|
||
| await (adapter as any).deleteConversation("conv-1", { expectedUserId: "user-1" }); | ||
|
|
||
| expect(runSpy).toHaveBeenCalledTimes(1); | ||
| const [sql, args] = runSpy.mock.calls[0]; | ||
| expect(sql).toContain("DELETE FROM test_conversations WHERE id = ? AND user_id = ?"); | ||
| expect(args).toEqual(["conv-1", "user-1"]); | ||
| }); | ||
|
|
||
| it("rejects guarded deletes without deleting child rows when no owned row is affected", async () => { | ||
| vi.spyOn(D1MemoryAdapter.prototype as any, "ensureInitialized").mockResolvedValue(undefined); | ||
| const adapter = new D1MemoryAdapter({ binding: createMockBinding(), tablePrefix: "test" }); | ||
| const runSpy = vi.spyOn(adapter as any, "run").mockResolvedValue({ meta: { changes: 0 } }); | ||
|
|
||
| await expect( | ||
| (adapter as any).deleteConversation("conv-1", { expectedUserId: "user-1" }), | ||
| ).rejects.toBeInstanceOf(ConversationOwnershipMismatchError); | ||
|
|
||
| expect(runSpy).toHaveBeenCalledTimes(1); | ||
| const [sql, args] = runSpy.mock.calls[0]; | ||
| expect(sql).toContain("DELETE FROM test_conversations WHERE id = ? AND user_id = ?"); | ||
| expect(args).toEqual(["conv-1", "user-1"]); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,84 @@ | ||
| import type { UIMessage } from "ai"; | ||
| import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; | ||
| import { InMemoryStorageAdapter } from "./adapters/storage/in-memory"; | ||
| import { InMemoryVectorAdapter } from "./adapters/vector/in-memory"; | ||
| import { Memory } from "./index"; | ||
|
|
||
| describe("Memory conversation mutation guards", () => { | ||
| let storage: InMemoryStorageAdapter; | ||
| let vector: InMemoryVectorAdapter; | ||
| let memory: Memory; | ||
|
|
||
| beforeEach(async () => { | ||
| storage = new InMemoryStorageAdapter(); | ||
| vector = new InMemoryVectorAdapter(); | ||
| memory = new Memory({ | ||
| storage, | ||
| vector, | ||
| }); | ||
|
|
||
| await memory.createConversation({ | ||
| id: "conv-1", | ||
| userId: "user-1", | ||
| resourceId: "agent-1", | ||
| title: "Conversation", | ||
| metadata: {}, | ||
| }); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| vi.restoreAllMocks(); | ||
| }); | ||
|
|
||
| it("continues unguarded deletes when vector cleanup cannot read the conversation", async () => { | ||
| const readError = new Error("read unavailable"); | ||
| const getSpy = vi.spyOn(storage, "getConversation").mockRejectedValueOnce(readError); | ||
| const deleteSpy = vi.spyOn(storage, "deleteConversation"); | ||
| const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); | ||
|
|
||
| await expect(memory.deleteConversation("conv-1")).resolves.toBeUndefined(); | ||
|
|
||
| expect(getSpy).toHaveBeenCalledWith("conv-1"); | ||
| expect(deleteSpy).toHaveBeenCalledWith("conv-1", undefined); | ||
| expect(warnSpy).toHaveBeenCalledWith( | ||
| "Failed to delete vectors for conversation conv-1:", | ||
| readError, | ||
| ); | ||
| await expect(storage.getConversation("conv-1")).resolves.toBeNull(); | ||
| }); | ||
|
|
||
| it("does not re-read vectors for guarded deletes when ownership lookup finds no conversation", async () => { | ||
| const replacementConversation = { | ||
| id: "conv-1", | ||
| userId: "user-2", | ||
| resourceId: "agent-1", | ||
| title: "Replacement", | ||
| metadata: {}, | ||
| createdAt: "2024-01-01T00:00:00.000Z", | ||
| updatedAt: "2024-01-01T00:00:00.000Z", | ||
| }; | ||
| const deleteError = new Error("missing conversation"); | ||
| const getSpy = vi | ||
| .spyOn(storage, "getConversation") | ||
| .mockResolvedValueOnce(null) | ||
| .mockResolvedValueOnce(replacementConversation); | ||
| const secretMessage: UIMessage<{ createdAt: Date }> = { | ||
| id: "msg-1", | ||
| role: "user", | ||
| parts: [{ type: "text", text: "secret" }], | ||
| metadata: { createdAt: new Date() }, | ||
| }; | ||
| const getMessagesSpy = vi.spyOn(storage, "getMessages").mockResolvedValueOnce([secretMessage]); | ||
| const deleteBatchSpy = vi.spyOn(vector, "deleteBatch"); | ||
| const deleteSpy = vi.spyOn(storage, "deleteConversation").mockRejectedValueOnce(deleteError); | ||
|
|
||
| await expect(memory.deleteConversation("conv-1", { expectedUserId: "user-1" })).rejects.toThrow( | ||
| deleteError, | ||
| ); | ||
|
|
||
| expect(getSpy).toHaveBeenCalledTimes(1); | ||
| expect(deleteSpy).toHaveBeenCalledWith("conv-1", { expectedUserId: "user-1" }); | ||
| expect(getMessagesSpy).not.toHaveBeenCalled(); | ||
| expect(deleteBatchSpy).not.toHaveBeenCalled(); | ||
| }); | ||
| }); |
Uh oh!
There was an error while loading. Please reload this page.