fix(server-core): enforce memory conversation ownership - #1388
Open
zcxGGmu wants to merge 3 commits into
Open
Conversation
Reject authenticated memory conversation reads, message listing, updates, and deletes when the conversation belongs to another user.\n\nAdd regression tests for the IDOR paths reported in VoltAgent#1371 and pass authenticated user context from Hono and Elysia memory routes.
|
Contributor
📝 WalkthroughWalkthroughMemory routes now pass authenticated user identifiers to handlers. Handlers enforce ownership for listing, reads, message listing, updates, and deletes. Memory and storage adapters guard mutations with expected owner IDs. Tests cover unauthorized access, route propagation, storage predicates, and owner access. ChangesMemory ownership authorization
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant MemoryRoutes
participant MemoryHandlers
participant Memory
participant StorageAdapters
Client->>MemoryRoutes: Request conversation operation
MemoryRoutes->>MemoryRoutes: Extract authenticated user ID
MemoryRoutes->>MemoryHandlers: Pass requestingUserId
MemoryHandlers->>Memory: Fetch or mutate conversation
Memory->>StorageAdapters: Validate expectedUserId
StorageAdapters-->>Memory: Return result or ownership mismatch
Memory-->>MemoryHandlers: Return result or ownership error
MemoryHandlers-->>MemoryRoutes: Return result or 403 Forbidden
MemoryRoutes-->>Client: HTTP response
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/server-core/src/handlers/memory.handlers.spec.ts`:
- Around line 120-128: Add a test for the conversation-list handler covering an
authenticated Bob requesting Alice’s userId through /api/memory/conversations,
and assert Alice’s conversation is excluded from the response before the
existing ownership tests. Reuse the existing test fixtures, dependencies,
handler symbol, and response shape in memory.handlers.spec.ts; keep the current
known-conversation message-listing test unchanged.
In `@packages/server-core/src/handlers/memory.handlers.ts`:
- Around line 568-581: Make updateConversation and deleteConversation use
storage-level mutations guarded by the verified owner via an expectedUserId
condition, rather than relying only on the preceding
getConversation/assertConversationOwner check. In
packages/server-core/src/handlers/memory.handlers.ts lines 568-581 and 614-627,
return 403 when the conditional update or delete affects no owned record, while
preserving the existing 404 response for a missing conversation.
In `@packages/server-elysia/src/routes/memory.routes.ts`:
- Around line 44-53: Update the list-conversations handlers in
packages/server-elysia/src/routes/memory.routes.ts (lines 44-53) and
packages/server-hono/src/routes/memory.routes.ts (lines 44-53) to derive the
authenticated identity via getAuthenticatedUserId and pass it as
requestingUserId to handleListMemoryConversations; remove use of the
client-controlled query.userId while preserving the existing route behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 60ab5139-c2e8-4847-9ff8-4578c2c97f9a
📒 Files selected for processing (4)
packages/server-core/src/handlers/memory.handlers.spec.tspackages/server-core/src/handlers/memory.handlers.tspackages/server-elysia/src/routes/memory.routes.tspackages/server-hono/src/routes/memory.routes.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 4 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Author
|
Thanks for the review — I pushed Changes made:
Validation:
Note: package-wide |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/server-elysia/src/routes/memory.routes.ts (1)
117-130: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winEnforce conversation ownership for working-memory, clone, update-working-memory, delete-messages, and search routes.
These routes still pass only client-supplied IDs/
userId. The handlers only reject another user whenuserIdis explicitly provided; they do not accept or reject based on the authenticatedrequestingUserId, so a non-owner can fetch, update, clone, delete, or search that conversation’s memory data by supplying the conversation ID. AddrequestingUserIdguards before the operation, withuserIdchecked against the conversation owner where required.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/server-elysia/src/routes/memory.routes.ts` around lines 117 - 130, Enforce authenticated ownership across the working-memory, clone, update-working-memory, delete-messages, and search route handlers by obtaining the authenticated requestingUserId and passing it into the corresponding memory operations before they execute. Validate that requestingUserId owns the conversation, and where a client-supplied userId is accepted, require it to match the conversation owner; reject unauthorized requests consistently while preserving existing success and HTTP status handling.
🧹 Nitpick comments (2)
packages/server-core/src/handlers/memory.handlers.spec.ts (1)
143-156: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a list-handler test for an explicit empty
requestingUserId.
handleListMemoryConversationsusesquery.requestingUserId ?? query.userId, a different code path thanassertConversationOwnerused byhandleGetMemoryConversation. The empty-string case tested at lines 133-141 for reads doesn't cover this path. Add a test that passesrequestingUserId: ""tohandleListMemoryConversationsand asserts it returns no conversations, to lock in the "explicitly empty identity" contract across all ownership-sensitive handlers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/server-core/src/handlers/memory.handlers.spec.ts` around lines 143 - 156, Add a test alongside the existing handleListMemoryConversations tests that passes requestingUserId as an explicit empty string while retaining the authenticated user setup. Assert the handler succeeds with zero total conversations and an empty conversations list, covering the query.requestingUserId ?? query.userId path without altering the existing non-empty identity test.packages/server-elysia/src/routes/memory.routes.ts (1)
45-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated
getAuthenticatedUserIdhelper.This function is duplicated verbatim in
packages/server-hono/src/routes/memory.routes.ts. Since it implements the exact claim-extraction logic that this PR's ownership enforcement depends on, keeping one shared implementation in@voltagent/server-corereduces the risk of the two copies drifting and silently weakening authorization in one framework but not the other.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/server-elysia/src/routes/memory.routes.ts` around lines 45 - 58, Move the duplicated getAuthenticatedUserId helper into the shared `@voltagent/server-core` package, export it, and replace the local implementations in the Elysia and Hono memory routes with that shared import. Preserve the existing id-then-sub claim extraction and undefined fallback behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/memory/types.ts`:
- Around line 447-452: Update updateConversation and deleteConversation across
D1MemoryAdapter, PostgreSQLMemoryAdapter, LibSQLMemoryCore,
SupabaseMemoryAdapter, and ManagedMemoryAdapter to accept and enforce
ConversationMutationOptions.expectedUserId. Use a single atomic
ownership-filtered UPDATE or DELETE statement keyed by both conversation id and
expectedUserId, and preserve the existing behavior for calls without an expected
user ID.
---
Outside diff comments:
In `@packages/server-elysia/src/routes/memory.routes.ts`:
- Around line 117-130: Enforce authenticated ownership across the
working-memory, clone, update-working-memory, delete-messages, and search route
handlers by obtaining the authenticated requestingUserId and passing it into the
corresponding memory operations before they execute. Validate that
requestingUserId owns the conversation, and where a client-supplied userId is
accepted, require it to match the conversation owner; reject unauthorized
requests consistently while preserving existing success and HTTP status
handling.
---
Nitpick comments:
In `@packages/server-core/src/handlers/memory.handlers.spec.ts`:
- Around line 143-156: Add a test alongside the existing
handleListMemoryConversations tests that passes requestingUserId as an explicit
empty string while retaining the authenticated user setup. Assert the handler
succeeds with zero total conversations and an empty conversations list, covering
the query.requestingUserId ?? query.userId path without altering the existing
non-empty identity test.
In `@packages/server-elysia/src/routes/memory.routes.ts`:
- Around line 45-58: Move the duplicated getAuthenticatedUserId helper into the
shared `@voltagent/server-core` package, export it, and replace the local
implementations in the Elysia and Hono memory routes with that shared import.
Preserve the existing id-then-sub claim extraction and undefined fallback
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 510b6c75-7d80-4689-bf86-c0025a60f667
📒 Files selected for processing (11)
packages/core/src/memory/adapters/storage/in-memory.tspackages/core/src/memory/errors.tspackages/core/src/memory/index.tspackages/core/src/memory/types.tspackages/server-core/src/handlers/memory.handlers.spec.tspackages/server-core/src/handlers/memory.handlers.tspackages/server-elysia/src/auth/middleware.tspackages/server-elysia/src/routes/memory.routes.spec.tspackages/server-elysia/src/routes/memory.routes.tspackages/server-hono/src/routes/memory.routes.spec.tspackages/server-hono/src/routes/memory.routes.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/server-hono/src/routes/memory.routes.ts
Contributor
There was a problem hiding this comment.
1 issue found across 11 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/core/src/memory/types.ts">
<violation number="1" location="packages/core/src/memory/types.ts:452">
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.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| options?: ConversationMutationOptions, | ||
| ): Promise<Conversation>; | ||
| deleteConversation(id: string): Promise<void>; | ||
| deleteConversation(id: string, options?: ConversationMutationOptions): Promise<void>; |
Contributor
There was a problem hiding this comment.
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>
Apply expectedUserId checks to persistent conversation mutations, preserve best-effort vector cleanup for unguarded deletes, and reject empty authenticated identities when listing conversations.
Author
|
Thanks for the review — pushed
Validation passed:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/voltagent-memory/src/index.spec.ts`:
- Around line 68-110: Expand the mutation ownership tests around
ManagedMemoryAdapter.updateConversation and deleteConversation to cover both
outcomes: add an owner-matched update case asserting conversations.update is
delegated with the expected arguments, and add a mismatched-owner delete case
asserting ConversationOwnershipMismatchError and that conversations.delete is
not called.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bc96dae9-7ee0-48b5-8b55-39f964b86a35
📒 Files selected for processing (14)
packages/cloudflare-d1/src/memory-adapter.spec.tspackages/cloudflare-d1/src/memory-adapter.tspackages/core/src/memory/index.spec.tspackages/core/src/memory/index.tspackages/libsql/src/memory-core.tspackages/libsql/src/memory-v2-adapter.spec.tspackages/postgres/src/memory-adapter.spec.tspackages/postgres/src/memory-adapter.tspackages/server-core/src/handlers/memory.handlers.spec.tspackages/server-core/src/handlers/memory.handlers.tspackages/supabase/src/memory-adapter.spec.tspackages/supabase/src/memory-adapter.tspackages/voltagent-memory/src/index.spec.tspackages/voltagent-memory/src/index.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/server-core/src/handlers/memory.handlers.spec.ts
- packages/core/src/memory/index.ts
- packages/server-core/src/handlers/memory.handlers.ts
Comment on lines
+68
to
+110
| it("rejects guarded updates before delegating when the owner does not match", async () => { | ||
| const { client, conversations } = createVoltOpsClient(); | ||
| conversations.get.mockResolvedValue({ | ||
| id: "conv-1", | ||
| userId: "user-2", | ||
| resourceId: "agent-1", | ||
| title: "Private", | ||
| metadata: {}, | ||
| createdAt: "2024-01-01T00:00:00.000Z", | ||
| updatedAt: "2024-01-01T00:00:00.000Z", | ||
| }); | ||
|
|
||
| const adapter = new ManagedMemoryAdapter({ databaseId: "db-1", voltOpsClient: client }); | ||
|
|
||
| await expect( | ||
| adapter.updateConversation("conv-1", { title: "Updated" }, { expectedUserId: "user-1" }), | ||
| ).rejects.toBeInstanceOf(ConversationOwnershipMismatchError); | ||
|
|
||
| expect(conversations.update).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it("checks ownership before delegated deletes", async () => { | ||
| const { client, conversations } = createVoltOpsClient(); | ||
| conversations.get.mockResolvedValue({ | ||
| id: "conv-1", | ||
| userId: "user-1", | ||
| resourceId: "agent-1", | ||
| title: "Private", | ||
| metadata: {}, | ||
| createdAt: "2024-01-01T00:00:00.000Z", | ||
| updatedAt: "2024-01-01T00:00:00.000Z", | ||
| }); | ||
| conversations.delete.mockResolvedValue(undefined); | ||
|
|
||
| const adapter = new ManagedMemoryAdapter({ databaseId: "db-1", voltOpsClient: client }); | ||
|
|
||
| await expect( | ||
| adapter.deleteConversation("conv-1", { expectedUserId: "user-1" }), | ||
| ).resolves.toBeUndefined(); | ||
|
|
||
| expect(conversations.get).toHaveBeenCalledWith("db-1", "conv-1"); | ||
| expect(conversations.delete).toHaveBeenCalledWith("db-1", "conv-1"); | ||
| }); |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Cover both ownership outcomes for each mutation.
Lines 68-87 test only a denied update. Add an owner-matched update test that verifies conversations.update is called.
Lines 89-110 test only an allowed delete. Add a mismatched-owner delete test that verifies ConversationOwnershipMismatchError and that conversations.delete is not called.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/voltagent-memory/src/index.spec.ts` around lines 68 - 110, Expand
the mutation ownership tests around ManagedMemoryAdapter.updateConversation and
deleteConversation to cover both outcomes: add an owner-matched update case
asserting conversations.update is delegated with the expected arguments, and add
a mismatched-owner delete case asserting ConversationOwnershipMismatchError and
that conversations.delete is not called.
Contributor
There was a problem hiding this comment.
8 issues found across 14 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/core/src/memory/index.ts">
<violation number="1" location="packages/core/src/memory/index.ts:310">
P1: A guarded delete can delete another owner’s message vectors even though the conversation delete is later rejected: when the initial ownership read returns null, this re-fetch may find a newly created same-ID conversation owned by someone else. Skip the vector-cleanup lookup when `expectedUserId` is set and the ownership lookup returned no conversation, so cleanup cannot run before the guarded mutation establishes ownership.</violation>
</file>
<file name="packages/libsql/src/memory-v2-adapter.spec.ts">
<violation number="1" location="packages/libsql/src/memory-v2-adapter.spec.ts:155">
P2: Cross-user mutation rejection is not covered here: these tests can pass even if ownership checks stop throwing. Adding cases with a different existing owner for update and `rowsAffected: 0` for delete, asserting `ConversationOwnershipMismatchError`, would protect the security behavior this PR introduces.</violation>
</file>
<file name="packages/cloudflare-d1/src/memory-adapter.ts">
<violation number="1" location="packages/cloudflare-d1/src/memory-adapter.ts:1106">
P2: Authenticated conversation deletion can permanently remove messages or steps while leaving the conversation behind if a later D1 write fails. Group the child and ownership-filtered parent deletes in one D1 batch/transaction, or rely on the existing parent cascades, so the operation cannot commit partially.</violation>
</file>
<file name="packages/postgres/src/memory-adapter.ts">
<violation number="1" location="packages/postgres/src/memory-adapter.ts:11">
P1: Installing this adapter with an older `@voltagent/core` still allowed by its `^2.0.0` peer range can now fail at module load because the new value import is missing. The package peer minimum should be raised to the first core release exporting `ConversationOwnershipMismatchError` (and the corresponding release metadata updated).</violation>
</file>
<file name="packages/core/src/memory/index.spec.ts">
<violation number="1" location="packages/core/src/memory/index.spec.ts:42">
P3: A failed assertion leaves `console.warn` mocked, which can suppress diagnostics and affect later tests. Restoring the spy in an `afterEach` hook or a `finally` block would keep this regression test isolated.</violation>
</file>
<file name="packages/voltagent-memory/src/index.ts">
<violation number="1" location="packages/voltagent-memory/src/index.ts:184">
P2: An explicitly empty authenticated identity is accepted when the conversation also has an empty `userId`, bypassing the intended empty-identity ownership rejection. Treat `expectedUserId === ""` as a mismatch before comparing owners.</violation>
<violation number="2" location="packages/voltagent-memory/src/index.ts:380">
P1: An ownership check can become stale before the managed-memory mutation executes: the adapter reads the conversation owner and then performs an unscoped remote update/delete. If ownership changes between those calls, the request can modify or delete the conversation after it belongs to another user. Passing the expected owner through to an atomic conditional update/delete (or rechecking ownership in the managed-memory service) would close this TOCTOU gap.</violation>
</file>
<file name="packages/cloudflare-d1/src/memory-adapter.spec.ts">
<violation number="1" location="packages/cloudflare-d1/src/memory-adapter.spec.ts:96">
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.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| try { | ||
| // Try to get the conversation first to get userId | ||
| const conversation = await this.storage.getConversation(id); | ||
| conversation ??= await this.storage.getConversation(id); |
Contributor
There was a problem hiding this comment.
P1: A guarded delete can delete another owner’s message vectors even though the conversation delete is later rejected: when the initial ownership read returns null, this re-fetch may find a newly created same-ID conversation owned by someone else. Skip the vector-cleanup lookup when expectedUserId is set and the ownership lookup returned no conversation, so cleanup cannot run before the guarded mutation establishes ownership.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/src/memory/index.ts, line 310:
<comment>A guarded delete can delete another owner’s message vectors even though the conversation delete is later rejected: when the initial ownership read returns null, this re-fetch may find a newly created same-ID conversation owned by someone else. Skip the vector-cleanup lookup when `expectedUserId` is set and the ownership lookup returned no conversation, so cleanup cannot run before the guarded mutation establishes ownership.</comment>
<file context>
@@ -296,21 +296,19 @@ export class Memory {
// If vector adapter is configured, delete associated vectors
if (this.vector) {
try {
+ conversation ??= await this.storage.getConversation(id);
+
if (conversation) {
</file context>
Suggested change
| conversation ??= await this.storage.getConversation(id); | |
| if (options?.expectedUserId === undefined) { | |
| conversation = await this.storage.getConversation(id); | |
| } |
| import { | ||
| ConversationAlreadyExistsError, | ||
| ConversationNotFoundError, | ||
| ConversationOwnershipMismatchError, |
Contributor
There was a problem hiding this comment.
P1: Installing this adapter with an older @voltagent/core still allowed by its ^2.0.0 peer range can now fail at module load because the new value import is missing. The package peer minimum should be raised to the first core release exporting ConversationOwnershipMismatchError (and the corresponding release metadata updated).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/postgres/src/memory-adapter.ts, line 11:
<comment>Installing this adapter with an older `@voltagent/core` still allowed by its `^2.0.0` peer range can now fail at module load because the new value import is missing. The package peer minimum should be raised to the first core release exporting `ConversationOwnershipMismatchError` (and the corresponding release metadata updated).</comment>
<file context>
@@ -5,9 +5,14 @@
+import {
+ ConversationAlreadyExistsError,
+ ConversationNotFoundError,
+ ConversationOwnershipMismatchError,
+} from "@voltagent/core";
import type {
</file context>
| updates: Partial<Omit<Conversation, "id" | "createdAt" | "updatedAt">>, | ||
| options?: ConversationMutationOptions, | ||
| ): Promise<Conversation> { | ||
| await this.assertExpectedConversationOwner(id, options); |
Contributor
There was a problem hiding this comment.
P1: An ownership check can become stale before the managed-memory mutation executes: the adapter reads the conversation owner and then performs an unscoped remote update/delete. If ownership changes between those calls, the request can modify or delete the conversation after it belongs to another user. Passing the expected owner through to an atomic conditional update/delete (or rechecking ownership in the managed-memory service) would close this TOCTOU gap.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/voltagent-memory/src/index.ts, line 380:
<comment>An ownership check can become stale before the managed-memory mutation executes: the adapter reads the conversation owner and then performs an unscoped remote update/delete. If ownership changes between those calls, the request can modify or delete the conversation after it belongs to another user. Passing the expected owner through to an atomic conditional update/delete (or rechecking ownership in the managed-memory service) would close this TOCTOU gap.</comment>
<file context>
@@ -351,10 +372,13 @@ export class ManagedMemoryAdapter implements StorageAdapter {
updates: Partial<Omit<Conversation, "id" | "createdAt" | "updatedAt">>,
+ options?: ConversationMutationOptions,
): Promise<Conversation> {
+ await this.assertExpectedConversationOwner(id, options);
+
return this.withClientContext(({ client, database }) => {
</file context>
| .mockResolvedValueOnce({ rows: [], rowsAffected: 1 }) | ||
| .mockResolvedValueOnce({ rows: [{ ...existing, title: "Updated" }] }); | ||
|
|
||
| await (adapter as any).updateConversation( |
Contributor
There was a problem hiding this comment.
P2: Cross-user mutation rejection is not covered here: these tests can pass even if ownership checks stop throwing. Adding cases with a different existing owner for update and rowsAffected: 0 for delete, asserting ConversationOwnershipMismatchError, would protect the security behavior this PR introduces.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/libsql/src/memory-v2-adapter.spec.ts, line 155:
<comment>Cross-user mutation rejection is not covered here: these tests can pass even if ownership checks stop throwing. Adding cases with a different existing owner for update and `rowsAffected: 0` for delete, asserting `ConversationOwnershipMismatchError`, would protect the security behavior this PR introduces.</comment>
<file context>
@@ -135,4 +135,43 @@ describe.sequential("LibSQLMemoryAdapter - Advanced Behavior", () => {
+ .mockResolvedValueOnce({ rows: [], rowsAffected: 1 })
+ .mockResolvedValueOnce({ rows: [{ ...existing, title: "Updated" }] });
+
+ await (adapter as any).updateConversation(
+ "conv-1",
+ { title: "Updated" },
</file context>
| const stepsTable = `${this.tablePrefix}_steps`; | ||
|
|
||
| if (options?.expectedUserId !== undefined) { | ||
| await this.run( |
Contributor
There was a problem hiding this comment.
P2: Authenticated conversation deletion can permanently remove messages or steps while leaving the conversation behind if a later D1 write fails. Group the child and ownership-filtered parent deletes in one D1 batch/transaction, or rely on the existing parent cascades, so the operation cannot commit partially.
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.ts, line 1106:
<comment>Authenticated conversation deletion can permanently remove messages or steps while leaving the conversation behind if a later D1 write fails. Group the child and ownership-filtered parent deletes in one D1 batch/transaction, or rely on the existing parent cascades, so the operation cannot commit partially.</comment>
<file context>
@@ -1066,26 +1073,54 @@ export class D1MemoryAdapter implements StorageAdapter {
const stepsTable = `${this.tablePrefix}_steps`;
+ if (options?.expectedUserId !== undefined) {
+ await this.run(
+ `DELETE FROM ${messagesTable} WHERE conversation_id = ? AND EXISTS (SELECT 1 FROM ${conversationsTable} WHERE id = ? AND user_id = ?)`,
+ [id, id, options.expectedUserId],
</file context>
| throw new ConversationNotFoundError(id); | ||
| } | ||
|
|
||
| if (conversation.userId !== options.expectedUserId) { |
Contributor
There was a problem hiding this comment.
P2: An explicitly empty authenticated identity is accepted when the conversation also has an empty userId, bypassing the intended empty-identity ownership rejection. Treat expectedUserId === "" as a mismatch before comparing owners.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/voltagent-memory/src/index.ts, line 184:
<comment>An explicitly empty authenticated identity is accepted when the conversation also has an empty `userId`, bypassing the intended empty-identity ownership rejection. Treat `expectedUserId === ""` as a mismatch before comparing owners.</comment>
<file context>
@@ -165,6 +168,24 @@ export class ManagedMemoryAdapter implements StorageAdapter {
+ throw new ConversationNotFoundError(id);
+ }
+
+ if (conversation.userId !== options.expectedUserId) {
+ throw new ConversationOwnershipMismatchError(id);
+ }
</file context>
Suggested change
| if (conversation.userId !== options.expectedUserId) { | |
| if (options.expectedUserId === "" || conversation.userId !== options.expectedUserId) { |
| ); | ||
| await expect(storage.getConversation("conv-1")).resolves.toBeNull(); | ||
|
|
||
| warnSpy.mockRestore(); |
Contributor
There was a problem hiding this comment.
P3: A failed assertion leaves console.warn mocked, which can suppress diagnostics and affect later tests. Restoring the spy in an afterEach hook or a finally block would keep this regression test isolated.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/src/memory/index.spec.ts, line 42:
<comment>A failed assertion leaves `console.warn` mocked, which can suppress diagnostics and affect later tests. Restoring the spy in an `afterEach` hook or a `finally` block would keep this regression test isolated.</comment>
<file context>
@@ -0,0 +1,44 @@
+ );
+ await expect(storage.getConversation("conv-1")).resolves.toBeNull();
+
+ warnSpy.mockRestore();
+ });
+});
</file context>
| updated_at: "2024-01-01T00:00:00.000Z", | ||
| }; | ||
|
|
||
| it("adds expectedUserId to updateConversation mutations", async () => { |
Contributor
There was a problem hiding this comment.
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>
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
2 participants
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
Test Plan
pnpm --filter @voltagent/server-core test -- --runpnpm --filter @voltagent/server-core typecheckpnpm --filter @voltagent/server-core --filter @voltagent/server-hono --filter @voltagent/server-elysia buildbiome check packages/server-core/src/handlers/memory.handlers.ts packages/server-core/src/handlers/memory.handlers.spec.ts packages/server-hono/src/routes/memory.routes.ts packages/server-elysia/src/routes/memory.routes.tsFixes #1371
Summary by cubic
Enforces conversation ownership checks across memory APIs and storage adapters to block cross-user access (IDOR). Routes and handlers now pass and enforce
requestingUserId, and guarded mutations return 403 on ownership mismatch. Fixes #1371.@voltagent/server-core; mapConversationOwnershipMismatchErrorto 403 and forbid empty authenticated identities when listing.@voltagent/server-honoand@voltagent/server-elysiato handlers asrequestingUserId(extracted fromid/sub), and prefer it over clientuserIdwhen listing.ConversationMutationOptions.expectedUserIdand enforce ownership onupdateConversation/deleteConversationin@voltagent/core; pre-check owner inMemory.deleteConversationand keep vector cleanup best-effort for unguarded deletes.@voltagent/libsql,@voltagent/postgres,@voltagent/supabase,@voltagent/cloudflare-d1, and in-memory adduser_idpredicates and throwConversationOwnershipMismatchErrorwhen no rows are affected.@voltagent/voltagent-memorychecks expected owner before delegating updates/deletes to the managed service.Written for commit 96bcbab. Summary will update on new commits.
Summary by CodeRabbit
Security
Tests