Skip to content
Open
56 changes: 56 additions & 0 deletions packages/cloudflare-d1/src/memory-adapter.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -81,3 +81,59 @@ 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 () => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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
Check if this issue is valid — if so, understand the root cause and fix it. At packages/cloudflare-d1/src/memory-adapter.spec.ts, line 96:

<comment>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.</comment>

<file context>
@@ -81,3 +81,59 @@ describe("D1MemoryAdapter queryWorkflowRuns", () => {
+    updated_at: "2024-01-01T00:00:00.000Z",
+  };
+
+  it("adds expectedUserId to updateConversation mutations", async () => {
+    vi.spyOn(D1MemoryAdapter.prototype as any, "ensureInitialized").mockResolvedValue(undefined);
+    const adapter = new D1MemoryAdapter({ binding: createMockBinding(), tablePrefix: "test" });
</file context>

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("adds expectedUserId to deleteConversation mutations", 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" });

const messageDelete = runSpy.mock.calls.find(([sql]) =>
String(sql).includes("DELETE FROM test_messages"),
);
const stepsDelete = runSpy.mock.calls.find(([sql]) =>
String(sql).includes("DELETE FROM test_steps"),
);
const conversationDelete = runSpy.mock.calls.find(([sql]) =>
String(sql).includes("DELETE FROM test_conversations"),
);

expect(messageDelete?.[0]).toContain("EXISTS");
expect(messageDelete?.[1]).toEqual(["conv-1", "conv-1", "user-1"]);
expect(stepsDelete?.[0]).toContain("EXISTS");
expect(stepsDelete?.[1]).toEqual(["conv-1", "conv-1", "user-1"]);
expect(conversationDelete?.[0]).toContain("WHERE id = ? AND user_id = ?");
expect(conversationDelete?.[1]).toEqual(["conv-1", "user-1"]);
});
});
45 changes: 40 additions & 5 deletions packages/cloudflare-d1/src/memory-adapter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,11 @@ import {
AgentRegistry,
ConversationAlreadyExistsError,
ConversationNotFoundError,
ConversationOwnershipMismatchError,
} from "@voltagent/core";
import type {
Conversation,
ConversationMutationOptions,
ConversationQueryOptions,
ConversationStepRecord,
CreateConversationInput,
Expand Down Expand Up @@ -102,8 +104,8 @@ export class D1MemoryAdapter implements StorageAdapter {
return args.length > 0 ? statement.bind(...args) : statement;
}

private async run(sql: string, args: unknown[] = []): Promise<void> {
await this.buildStatement(sql, args).run();
private async run(sql: string, args: unknown[] = []): Promise<{ meta?: { changes?: number } }> {
return (await this.buildStatement(sql, args).run()) as { meta?: { changes?: number } };
}

private async all<T extends D1Row = D1Row>(sql: string, args: unknown[] = []): Promise<T[]> {
Expand Down Expand Up @@ -1037,6 +1039,7 @@ export class D1MemoryAdapter implements StorageAdapter {
async updateConversation(
id: string,
updates: Partial<Omit<Conversation, "id" | "createdAt" | "updatedAt">>,
options?: ConversationMutationOptions,
): Promise<Conversation> {
await this.ensureInitialized();

Expand All @@ -1046,6 +1049,10 @@ export class D1MemoryAdapter implements StorageAdapter {
throw new ConversationNotFoundError(id);
}

if (options?.expectedUserId !== undefined && conversation.userId !== options.expectedUserId) {
throw new ConversationOwnershipMismatchError(id);
}

const now = new Date().toISOString();
const fieldsToUpdate: string[] = ["updated_at = ?"];
const args: unknown[] = [now];
Expand All @@ -1066,26 +1073,54 @@ export class D1MemoryAdapter implements StorageAdapter {
}

args.push(id);
let whereClause = "WHERE id = ?";
if (options?.expectedUserId !== undefined) {
whereClause += " AND user_id = ?";
args.push(options.expectedUserId);
}

await this.run(
`UPDATE ${conversationsTable} SET ${fieldsToUpdate.join(", ")} WHERE id = ?`,
const result = await this.run(
`UPDATE ${conversationsTable} SET ${fieldsToUpdate.join(", ")} ${whereClause}`,
args,
);

if (options?.expectedUserId !== undefined && result.meta?.changes === 0) {
throw new ConversationOwnershipMismatchError(id);
}

const updated = await this.getConversation(id);
if (!updated) {
throw new Error(`Conversation not found after update: ${id}`);
}
return updated;
}

async deleteConversation(id: string): Promise<void> {
async deleteConversation(id: string, options?: ConversationMutationOptions): Promise<void> {
await this.ensureInitialized();

const conversationsTable = `${this.tablePrefix}_conversations`;
const messagesTable = `${this.tablePrefix}_messages`;
const stepsTable = `${this.tablePrefix}_steps`;

if (options?.expectedUserId !== undefined) {
await this.run(
Comment thread
cubic-dev-ai[bot] marked this conversation as resolved.
Outdated
`DELETE FROM ${messagesTable} WHERE conversation_id = ? AND EXISTS (SELECT 1 FROM ${conversationsTable} WHERE id = ? AND user_id = ?)`,
[id, id, options.expectedUserId],
);
await this.run(
`DELETE FROM ${stepsTable} WHERE conversation_id = ? AND EXISTS (SELECT 1 FROM ${conversationsTable} WHERE id = ? AND user_id = ?)`,
[id, id, options.expectedUserId],
);
const result = await this.run(
`DELETE FROM ${conversationsTable} WHERE id = ? AND user_id = ?`,
[id, options.expectedUserId],
);
if (result.meta?.changes === 0) {
throw new ConversationOwnershipMismatchError(id);
}
return;
}

await this.run(`DELETE FROM ${messagesTable} WHERE conversation_id = ?`, [id]);
await this.run(`DELETE FROM ${stepsTable} WHERE conversation_id = ?`, [id]);
await this.run(`DELETE FROM ${conversationsTable} WHERE id = ?`, [id]);
Expand Down
18 changes: 16 additions & 2 deletions packages/core/src/memory/adapters/storage/in-memory.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,14 @@
import { deepClone } from "@voltagent/internal/utils";
import type { UIMessage } from "ai";
import type { OperationContext } from "../../../agent/types";
import { ConversationAlreadyExistsError, ConversationNotFoundError } from "../../errors";
import {
ConversationAlreadyExistsError,
ConversationNotFoundError,
ConversationOwnershipMismatchError,
} from "../../errors";
import type {
Conversation,
ConversationMutationOptions,
ConversationQueryOptions,
ConversationStepRecord,
CreateConversationInput,
Expand Down Expand Up @@ -424,12 +429,17 @@ export class InMemoryStorageAdapter implements StorageAdapter {
async updateConversation(
id: string,
updates: Partial<Omit<Conversation, "id" | "createdAt" | "updatedAt">>,
options?: ConversationMutationOptions,
): Promise<Conversation> {
const conversation = this.conversations.get(id);
if (!conversation) {
throw new ConversationNotFoundError(id);
}

if (options?.expectedUserId !== undefined && conversation.userId !== options.expectedUserId) {
throw new ConversationOwnershipMismatchError(id);
}

const updatedConversation: Conversation = {
...conversation,
...updates,
Expand All @@ -443,12 +453,16 @@ export class InMemoryStorageAdapter implements StorageAdapter {
/**
* Delete a conversation
*/
async deleteConversation(id: string): Promise<void> {
async deleteConversation(id: string, options?: ConversationMutationOptions): Promise<void> {
const conversation = this.conversations.get(id);
if (!conversation) {
throw new ConversationNotFoundError(id);
}

if (options?.expectedUserId !== undefined && conversation.userId !== options.expectedUserId) {
throw new ConversationOwnershipMismatchError(id);
}

// Delete conversation
this.conversations.delete(id);

Expand Down
13 changes: 13 additions & 0 deletions packages/core/src/memory/errors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,19 @@ export class ConversationNotFoundError extends MemoryV2Error {
}
}

/**
* Error thrown when a guarded conversation mutation no longer matches the expected owner
*/
export class ConversationOwnershipMismatchError extends MemoryV2Error {
constructor(conversationId: string) {
super(`Conversation ownership mismatch: ${conversationId}`, "CONVERSATION_OWNERSHIP_MISMATCH", {
conversationId,
});
this.name = "ConversationOwnershipMismatchError";
Object.setPrototypeOf(this, ConversationOwnershipMismatchError.prototype);
}
}

/**
* Error thrown when trying to create a conversation that already exists
*/
Expand Down
44 changes: 44 additions & 0 deletions packages/core/src/memory/index.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
import { 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 memory: Memory;

beforeEach(async () => {
storage = new InMemoryStorageAdapter();
memory = new Memory({
storage,
vector: new InMemoryVectorAdapter(),
});

await memory.createConversation({
id: "conv-1",
userId: "user-1",
resourceId: "agent-1",
title: "Conversation",
metadata: {},
});
});

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

warnSpy.mockRestore();
Comment thread
cubic-dev-ai[bot] marked this conversation as resolved.
Outdated
});
});
28 changes: 22 additions & 6 deletions packages/core/src/memory/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,14 @@ import type { UIMessage } from "ai";
import type { z } from "zod";
import type { OperationContext } from "../agent/types";
import { AiSdkEmbeddingAdapter } from "./adapters/embedding/ai-sdk";
import { EmbeddingAdapterNotConfiguredError, VectorAdapterNotConfiguredError } from "./errors";
import {
ConversationOwnershipMismatchError,
EmbeddingAdapterNotConfiguredError,
VectorAdapterNotConfiguredError,
} from "./errors";
import type {
Conversation,
ConversationMutationOptions,
ConversationQueryOptions,
ConversationStepRecord,
CreateConversationInput,
Expand Down Expand Up @@ -280,19 +285,30 @@ export class Memory {
async updateConversation(
id: string,
updates: Partial<Omit<Conversation, "id" | "createdAt" | "updatedAt">>,
options?: ConversationMutationOptions,
): Promise<Conversation> {
return this.storage.updateConversation(id, updates);
return this.storage.updateConversation(id, updates, options);
}

/**
* Delete a conversation
*/
async deleteConversation(id: string): Promise<void> {
async deleteConversation(id: string, options?: ConversationMutationOptions): Promise<void> {
let conversation: Conversation | null = null;

if (options?.expectedUserId !== undefined) {
conversation = await this.storage.getConversation(id);
Comment thread
cubic-dev-ai[bot] marked this conversation as resolved.

if (conversation && conversation.userId !== options.expectedUserId) {
throw new ConversationOwnershipMismatchError(id);
}
}

// If vector adapter is configured, delete associated vectors
if (this.vector) {
try {
// Try to get the conversation first to get userId
const conversation = await this.storage.getConversation(id);
conversation ??= await this.storage.getConversation(id);
Comment thread
cubic-dev-ai[bot] marked this conversation as resolved.

if (conversation) {
// Get all messages to find vector IDs
const messages = await this.storage.getMessages(conversation.userId, id);
Expand All @@ -307,7 +323,7 @@ export class Memory {
}
}

return this.storage.deleteConversation(id);
return this.storage.deleteConversation(id, options);
}

// ============================================================================
Expand Down
7 changes: 6 additions & 1 deletion packages/core/src/memory/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,10 @@ export type ConversationQueryOptions = {
orderDirection?: "ASC" | "DESC";
};

export type ConversationMutationOptions = {
expectedUserId?: string;
};

/**
* Options for getting messages
*/
Expand Down Expand Up @@ -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>;
Comment thread
coderabbitai[bot] marked this conversation as resolved.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: Persistent storage adapters ignore the new expectedUserId option, so the ownership check is not enforced at the storage boundary for update/delete operations. Updating every adapter to apply the user ID atomically, or enforcing the check centrally before delegation, would prevent a race or direct Memory caller from mutating another user’s conversation.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/src/memory/types.ts, line 452:

<comment>Persistent storage adapters ignore the new `expectedUserId` option, so the ownership check is not enforced at the storage boundary for update/delete operations. Updating every adapter to apply the user ID atomically, or enforcing the check centrally before delegation, would prevent a race or direct `Memory` caller from mutating another user’s conversation.</comment>

<file context>
@@ -443,8 +447,9 @@ export interface StorageAdapter {
+    options?: ConversationMutationOptions,
   ): Promise<Conversation>;
-  deleteConversation(id: string): Promise<void>;
+  deleteConversation(id: string, options?: ConversationMutationOptions): Promise<void>;
 
   saveConversationSteps?(steps: ConversationStepRecord[]): Promise<void>;
</file context>


saveConversationSteps?(steps: ConversationStepRecord[]): Promise<void>;
getConversationSteps?(
Expand Down
Loading