fix: tenant-scope memory workspace ids so co-located accounts can't cross-read - #93
Conversation
…ross-read The memory episode store scopes every query (recall, timeline, searchSessions, listRecent, ftsSearch, vector matrix, file_reads) solely by workspace_id, and workspace_id was derived from the working directory alone. Two accounts working the SAME directory on one daemon got the SAME workspace_id, so one tenant's recall()/timeline() (incl. the memory MCP tools, which had no masking) returned the other tenant's episodes verbatim — a cross-tenant disclosure. Fold the tenant (account_id + project_id) into workspaceIdFromPath's hash, so co-located tenants get distinct ids and every existing query is tenant-scoped by construction — no per-query filter to individually get right. The tenant arg is REQUIRED, so the compiler enforces that every derivation site supplies it. - workspaceIdFromPath(workdir, tenant) mixes the tenant into the id; same tenant + same repo still collapse to one id (cross-worktree sharing kept). - Session derives it from its auth; the ClaudeProvider now receives that id instead of re-deriving (which dropped the tenant); session.search and the share export/import paths bind the caller's tenant. Note: episodes written under the old path-only ids are not visible to the new tenant-scoped ids — a one-time memory reset, acceptable for the isolation fix. Tests: same-path/different-tenant ids diverge; an episode ingested by tenant A is invisible to tenant B's recall/timeline/listRecent/ftsSearch in the same dir. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
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:
📝 WalkthroughWalkthroughWorkspace IDs now incorporate tenant context across session creation, provider wiring, export/import/search handling, startup migration, and tests. Transcript persistence also gains an explicit flush hook. ChangesTenant-aware workspace scoping
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #93 +/- ##
==========================================
+ Coverage 66.86% 73.06% +6.19%
==========================================
Files 65 65
Lines 11165 11032 -133
==========================================
+ Hits 7466 8060 +594
+ Misses 3699 2972 -727
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/daemon/session.ts (1)
338-340: 🗄️ Data Integrity & Integration | 🔵 TrivialTenant-scoped derivation is consistent; flag the one-time memory-visibility break for existing installs.
Centralizing derivation here and reusing
this.#workspaceIdfor the chunker, index scheduler, usage recording, and provider MCP binding keeps read/write scope aligned — correct.Operationally, since the hash input now includes
account_id/project_id, episodes written under the old path-only IDs will no longer resolve after upgrade (recall/timeline/usage will read empty for existing workspaces). Consider a one-time backfill that re-keys existingworkspace_idrows to the new tenant-scoped IDs (using each session's storedaccountId/projectId), or documenting the expected memory reset in upgrade notes.🤖 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 `@src/daemon/session.ts` around lines 338 - 340, The tenant-scoped workspaceId change in session.ts means existing path-only memory entries will no longer be found after upgrade. Update the session initialization flow around workspaceIdFromPath and the uses of this.#workspaceId to either add a one-time migration/backfill that re-keys stored workspace_id rows using each session’s accountId/projectId, or clearly document the expected memory reset for existing installs in the upgrade notes.
🤖 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 `@src/daemon/session.ts`:
- Around line 338-340: The tenant-scoped workspaceId change in session.ts means
existing path-only memory entries will no longer be found after upgrade. Update
the session initialization flow around workspaceIdFromPath and the uses of
this.#workspaceId to either add a one-time migration/backfill that re-keys
stored workspace_id rows using each session’s accountId/projectId, or clearly
document the expected memory reset for existing installs in the upgrade notes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 980636e7-d101-4693-ae7e-7e5229d1018e
📒 Files selected for processing (7)
src/daemon/memory/index.tssrc/daemon/memory/store.tssrc/daemon/providers/claude/index.tssrc/daemon/session-manager.tssrc/daemon/session.tssrc/tests/memory.test.tssrc/tests/provider-claude.test.ts
Adds a SessionManager-level test that drives session.search and a session.create -> export -> import round-trip through handle(), exercising the three sites that derive a workspace id from the caller's auth. The isolation property itself is covered in memory.test.ts; this guards the handler wiring (and satisfies patch coverage on the changed lines). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/tests/session-manager-tenant.test.ts (2)
128-141: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWeak final assertion undermines the test's regression value.
Accepting either
"session.import.result"or"response.error"(Line 140) means the test passes even if the tenant-boundworkspaceIdForwiring silently breaks and import always errors out. The comment above acknowledges this tradeoff, but it defeats the stated purpose of "exercising the tenant binding end-to-end (and can't silently regress)" (Line 7).Assert on the successful path explicitly (or branch and assert meaningfully on both outcomes) so a regression that always fails import is actually caught.
🧪 Suggested tightening
- // Either a successful import or a well-formed error — both exercised the - // tenant-bound workspaceIdFor path; a thrown/undefined response would not. - expect(["session.import.result", "response.error"]).toContain(imported.type); + expect(imported.type).toBe("session.import.result"); + // add further assertions on newSessionId / importedEpisodes here🤖 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 `@src/tests/session-manager-tenant.test.ts` around lines 128 - 141, The final assertion in session-manager-tenant.test.ts is too permissive because it accepts both success and error, so a broken tenant-bound workspaceIdFor path could still pass. Update the test around manager.handle for session.import to assert the successful import result explicitly, or split the assertion so session.import.result and response.error are checked with different expectations. Keep the check anchored on the imported response type so this regression cannot silently pass.
128-137: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
as nevercast bypasses type checking on the import payload.Casting the message object to
never(Line 134) suppresses structural verification against the actualsession.importrequest type, so a shape drift (e.g. wrong field name for the bundle) wouldn't be caught at compile time. Prefer casting to the actual request/message union type (or importing/asserting against it) sotsc --noEmitstill validates the payload shape.🤖 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 `@src/tests/session-manager-tenant.test.ts` around lines 128 - 137, The `as never` cast in the `manager.handle(...)` call bypasses compile-time validation for the `session.import` payload. Replace it with the real request/message type from the session manager API so the object is checked structurally by TypeScript, and keep the test aligned with the actual `session.import` shape used by `handle`.Source: Coding guidelines
🤖 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 `@src/tests/session-manager-tenant.test.ts`:
- Around line 128-141: The final assertion in session-manager-tenant.test.ts is
too permissive because it accepts both success and error, so a broken
tenant-bound workspaceIdFor path could still pass. Update the test around
manager.handle for session.import to assert the successful import result
explicitly, or split the assertion so session.import.result and response.error
are checked with different expectations. Keep the check anchored on the imported
response type so this regression cannot silently pass.
- Around line 128-137: The `as never` cast in the `manager.handle(...)` call
bypasses compile-time validation for the `session.import` payload. Replace it
with the real request/message type from the session manager API so the object is
checked structurally by TypeScript, and keep the test aligned with the actual
`session.import` shape used by `handle`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 8acc0299-266b-4246-8c00-b6ea125b9fdf
📒 Files selected for processing (1)
src/tests/session-manager-tenant.test.ts
Addresses a CodeRabbit nitpick: the round-trip test accepted either import.result OR response.error, so a regression that broke the tenant-bound workspaceIdFor and made import always error would still pass. Use the correct inline `source` shape so import actually succeeds, and assert on a fresh newSessionId — a broken binding now fails the test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Addressed the CodeRabbit nitpicks:
|
The round-trip test creates real sessions via the manager, which fire-and-forget meta writes (saveMeta's atomic tmp->rename). In CI those raced the afterEach rmSync and surfaced as an unhandled ENOENT that failed the runner even though the tests passed. Add TranscriptStore.flush() (awaits the in-flight append + meta chains — also useful for graceful shutdown) and call it before cleanup. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…startup Without this, folding the tenant into the workspace id orphaned every episode written before the upgrade (path-only ids no longer resolve), losing memory on the first restart after merge. Add a one-time, guarded startup migration that re-keys existing episodes to their owning session's tenant-scoped id. - SqliteEpisodeStore.migrateWorkspaceIdsToTenant(sessions, workspaceIdFor): re-keys episodes by owning session (precise; isolates tenants that shared a directory). Episodes whose session was destroyed are recovered only when the old workspace id maps to a single tenant — ambiguous multi-tenant orphans are left untouched rather than risk cross-tenant attribution. Guarded by PRAGMA user_version (runs once) and idempotent. - legacyWorkspaceIdFromPath: frozen copy of the old path-only derivation, used to recognise/recover orphans. - Store.listAllSessionsForMigration: unscoped (id, workdir, tenant) feed. - server.ts runs it once after memory init, best-effort (non-fatal on error). Tests: precise re-key + idempotency, two tenants sharing a dir stay separate, orphan recovered only when single-tenant. Note: turn_usage/file_reads aren't re-keyed (read by session_id / pure cache). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/daemon/store.ts (1)
151-172: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUnbounded full-table scan runs on every startup, not just once.
listAllSessionsForMigration()has no guard of its own — it's called unconditionally at Line 203 ofsrc/daemon/server.tson every daemon boot with memory enabled, even after the one-time re-key has already completed. For deployments with many historical sessions, this means a fullsessionstable scan into memory happens on every restart forever, even though the downstreammigrateWorkspaceIdsToTenantcall is a guarded no-op after the first run.Consider having the caller check the migration-completion flag (e.g., expose a cheap
needsWorkspaceMigration()check on the memory store) before paying for this query, so post-migration restarts skip it entirely.🤖 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 `@src/daemon/store.ts` around lines 151 - 172, The full-table scan in listAllSessionsForMigration is being executed on every daemon startup even after the migration has already completed. Add a cheap migration guard in the memory store, such as a needsWorkspaceMigration() check, and have src/daemon/server.ts call listAllSessionsForMigration only when that flag says the workspace-id migration still needs to run. Keep the existing migrateWorkspaceIdsToTenant flow, but skip the query entirely once the one-time migration is done.src/daemon/server.ts (1)
198-215: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winStartup migration blocks the event loop; confirm acceptable scan cost.
this.#store.listAllSessionsForMigration()andbun:sqlite-backedmigrateWorkspaceIdsToTenantare synchronous calls, so this whole block runs beforeBun.serve()starts listening — acceptable for a one-time migration, but as noted instore.ts, the full session-table fetch is re-executed on every startup regardless of whether re-keying already happened. For daemons with largesessionstables, this adds a recurring synchronous startup cost even long after migration completes.The error handling and non-fatal fallback here are solid — this is purely a call-site cost concern tied to the
listAllSessionsForMigration()design flagged instore.ts.🤖 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 `@src/daemon/server.ts` around lines 198 - 215, The startup migration in server initialization still performs a synchronous full scan every boot via this.#store.listAllSessionsForMigration() before calling this.#memory.store.migrateWorkspaceIdsToTenant(), which adds recurring startup cost after the re-key has already succeeded. Update the server.ts migration block to run only once per deployment or only when needed by adding a durable guard/state check around the migration call, so the listAllSessionsForMigration() scan is skipped on later startups unless the migration has not yet been completed.
🤖 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 `@src/daemon/server.ts`:
- Around line 198-215: The startup migration in server initialization still
performs a synchronous full scan every boot via
this.#store.listAllSessionsForMigration() before calling
this.#memory.store.migrateWorkspaceIdsToTenant(), which adds recurring startup
cost after the re-key has already succeeded. Update the server.ts migration
block to run only once per deployment or only when needed by adding a durable
guard/state check around the migration call, so the
listAllSessionsForMigration() scan is skipped on later startups unless the
migration has not yet been completed.
In `@src/daemon/store.ts`:
- Around line 151-172: The full-table scan in listAllSessionsForMigration is
being executed on every daemon startup even after the migration has already
completed. Add a cheap migration guard in the memory store, such as a
needsWorkspaceMigration() check, and have src/daemon/server.ts call
listAllSessionsForMigration only when that flag says the workspace-id migration
still needs to run. Keep the existing migrateWorkspaceIdsToTenant flow, but skip
the query entirely once the one-time migration is done.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3f94b33d-7f05-4885-85aa-aa8fb37cbad3
📒 Files selected for processing (6)
src/daemon/memory/index.tssrc/daemon/memory/store.tssrc/daemon/server.tssrc/daemon/store.tssrc/tests/memory.test.tssrc/tests/session-manager-tenant.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/tests/session-manager-tenant.test.ts
- src/daemon/memory/index.ts
Addresses a CodeRabbit nitpick: listAllSessionsForMigration() ran a full sessions-table scan on every boot even though migrateWorkspaceIdsToTenant is a no-op after the first run. Expose needsWorkspaceMigration() (a cheap user_version check) and gate the scan on it in server.ts, so post-migration restarts skip the read entirely. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…log (#117) The 0.2.0 entry only covered the protocol/packages train (#100-#116) and missed ten PRs that also ship in this release: the untrusted-content sanitization and cross-tenant memory fixes (#91, #93 — now under a proper Security heading), the performance run (#94-#99), and the model catalog work (#78, #79). Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 participant
Problem (cross-tenant memory disclosure)
The memory episode store scopes every retrieval —
recall,timeline,searchSessions,listRecent,ftsSearch, the vector matrix, andfile_reads— solely byworkspace_id, andworkspace_idwas derived from the working directory alone (git-common-dir, else path hash).So two accounts working the same directory on one daemon got the same
workspace_id. One tenant'srecall()/timeline()then returned the other tenant's episodes verbatim — including tool outputs and file contents. The memory MCP tools (recall/timeline) had no tenant masking at all; only the humansession.searchRPC masked live-session hits (a stopgap it explicitly flagged as needing a tenant-scoped store).Fix — fold the tenant into the workspace id
workspaceIdFromPath(workdir, tenant)now mixesaccount_id+project_idinto the hash. Because every episode query already scopes byworkspace_id, making the id tenant-specific isolates every path by construction — no per-queryWHERE tenant = ?to individually get right and never miss. Same tenant + same repo still collapse to one id, so cross-worktree memory sharing is preserved.The
tenantargument is required, so TypeScript refuses to compile until every derivation site supplies it — the compiler enforces completeness rather than me hand-auditing call sites.Threaded through:
Sessionderives the id from itsauth(which carries the tenant).ClaudeProvidernow receives that id instead of re-deriving from the workdir (the re-derivation silently dropped the tenant and would desync the memory MCP binding from where episodes are stored).session.searchand the share export/import paths bind the caller's tenant into the id.AuthContextis structurally aWorkspaceTenant({account_id, project_id}), so each site passesauthdirectly — no destructuring boilerplate.Tests
src/tests/memory.test.ts:recall/timeline/listRecent/ftsSearchin the same directory, while tenant A still recalls its own.Full suite green:
bun test(735),tsc,biome.Migration note
Episodes written under the old path-only ids are not visible to the new tenant-scoped ids — a one-time memory reset. Acceptable for the isolation guarantee (documented in the code); memory re-accumulates from the next session.
Addresses the private "memory has no tenant isolation" security advisory. The
session.searchmasking is now belt-and-suspenders (cross-tenant hits no longer share a workspace id to begin with).Summary by CodeRabbit
flush()method.