fix(server-core): enforce memory conversation ownership - #1388
Open
zcxGGmu wants to merge 7 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.
🦋 Changeset detectedLatest commit: a4d2f31 The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughMemory routes pass authenticated user identifiers to handlers. Handlers enforce ownership for conversation listing, reads, messages, updates, and deletes. Memory services and storage adapters guard mutations with expected owner IDs. Tests cover unauthorized access and ownership propagation. 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: Apply 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 💡 1⚔️ Resolve merge conflicts 💡
🧪 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.
2 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/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/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
| 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>
| 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>
Author
|
Thanks for the review — I pushed Changes made:
Validation:
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/core/src/memory/index.spec.ts (2)
64-70: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the
as anycast from the message fixture.Use the concrete message type or a shared typed fixture. The cast disables TypeScript checks and can hide an invalid stored-message shape in this regression test.
As per coding guidelines,
**/*.tsfiles must maintain type safety in this TypeScript-first codebase.🤖 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/core/src/memory/index.spec.ts` around lines 64 - 70, Remove the `as any` cast from the message fixture passed to `storage.getMessages` in the `getMessagesSpy` setup. Type the fixture using the concrete stored-message type or an existing shared typed fixture, preserving the same test data while retaining TypeScript validation.Source: Coding guidelines
73-82: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert the guarded storage call.
Capture the
storage.deleteConversationspy and assert that it receives{ expectedUserId: "user-1" }. The current test only verifies error propagation. A regression that dropsexpectedUserIdwould still pass because the mock rejects for every argument.Proposed test assertion
- vi.spyOn(storage, "deleteConversation").mockRejectedValueOnce(deleteError); + const deleteSpy = vi + .spyOn(storage, "deleteConversation") + .mockRejectedValueOnce(deleteError); ... + expect(deleteSpy).toHaveBeenCalledWith("conv-1", { + expectedUserId: "user-1", + });The storage adapter contract checks
expectedUserIdbefore deletion inpackages/core/src/memory/adapters/storage/in-memory.ts:456-475.🤖 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/core/src/memory/index.spec.ts` around lines 73 - 82, Update the deleteConversation test to retain the spy returned by vi.spyOn(storage, "deleteConversation"), then assert it was called with { expectedUserId: "user-1" } after verifying the rejection. Keep the existing error-propagation and call-count assertions unchanged.
🤖 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.
Nitpick comments:
In `@packages/core/src/memory/index.spec.ts`:
- Around line 64-70: Remove the `as any` cast from the message fixture passed to
`storage.getMessages` in the `getMessagesSpy` setup. Type the fixture using the
concrete stored-message type or an existing shared typed fixture, preserving the
same test data while retaining TypeScript validation.
- Around line 73-82: Update the deleteConversation test to retain the spy
returned by vi.spyOn(storage, "deleteConversation"), then assert it was called
with { expectedUserId: "user-1" } after verifying the rejection. Keep the
existing error-propagation and call-count assertions unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7fbc0494-6c42-43eb-9dc2-df2111fbe731
📒 Files selected for processing (16)
.changeset/tender-memory-ownership.mdpackages/cloudflare-d1/package.jsonpackages/cloudflare-d1/src/memory-adapter.spec.tspackages/cloudflare-d1/src/memory-adapter.tspackages/core/src/memory/index.spec.tspackages/core/src/memory/index.tspackages/core/src/voltops/client.tspackages/core/src/voltops/types.tspackages/libsql/package.jsonpackages/libsql/src/memory-v2-adapter.spec.tspackages/postgres/package.jsonpackages/server-core/package.jsonpackages/supabase/package.jsonpackages/voltagent-memory/package.jsonpackages/voltagent-memory/src/index.spec.tspackages/voltagent-memory/src/index.ts
💤 Files with no reviewable changes (1)
- packages/cloudflare-d1/src/memory-adapter.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/libsql/src/memory-v2-adapter.spec.ts
- packages/core/src/memory/index.ts
- packages/voltagent-memory/src/index.ts
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 16 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Author
|
Thanks for the review — I pushed Changes made:
Validation:
|
Author
|
Follow-up: I pushed Changes made:
Validation:
|
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/server-core/package.json`:
- Line 7: Regenerate pnpm-lock.yaml after the dependency update in
packages/server-core/package.json so the server-core importer records
`@voltagent/core` at ^2.9.1 instead of the stale ^2.8.1 range, then commit the
synchronized lockfile.
🪄 Autofix
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: 02c8dea7-6095-4eae-9001-265b5254ee21
📒 Files selected for processing (3)
.changeset/tender-memory-ownership.mdpackages/core/src/memory/index.spec.tspackages/server-core/package.json
🚧 Files skipped from review as they are similar to previous changes (2)
- .changeset/tender-memory-ownership.md
- packages/core/src/memory/index.spec.ts
Author
|
Follow-up: I pushed Changes made:
Validation:
|
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 across memory APIs, handlers, and adapters to prevent cross-user reads and mutations (IDOR). Authenticated identity is used end-to-end; guarded updates/deletes verify the owner and return 403. Fixes #1371.
Bug Fixes
@voltagent/server-core; mapConversationOwnershipMismatchErrorto 403; reject empty authenticated identities; prefer the authenticated user over clientuserIdwhen listing.@voltagent/server-honoand@voltagent/server-elysiato handlers asrequestingUserId(id/sub) for list/get/messages/update/delete.ConversationMutationOptions.expectedUserIdand enforce ownership inMemory.updateConversation/deleteConversation; pre-check owner, skip vector cleanup when the pre-check finds no conversation, and keep vector cleanup best-effort for unguarded deletes or read failures.@voltagent/libsql,@voltagent/postgres,@voltagent/supabase,@voltagent/cloudflare-d1, and in-memory adduser_idguards to update/delete; D1 uses a guarded parent delete (cascades for child rows); throwConversationOwnershipMismatchErrorwhen no owned rows are affected.@voltagent/voltagent-memoryvalidates expected owner before delegating updates/deletes and rejects empty expected owners; the VoltOps client forwardsexpectedUserIdon managed-memory update/delete calls.Dependencies
@voltagent/core^2.9.1across adapters, server packages, and@voltagent/voltagent-memory.Written for commit a4d2f31. Summary will update on new commits.
Summary by CodeRabbit
Security
Tests