Repository navigation
fix(memory): implement the memory/episode/raw-turn scope assertions - #611
Open
akushonkamen wants to merge 1 commit into
Open
akushonkamen wants to merge 1 commit into
akushonkamen wants to merge 1 commit into
Conversation
assertMemoryInScope, assertEpisodeInScope and assertRawTurnInScope discarded both parameters (void memory; void namespace), so the get/delete/archive/retrieval call sites that pass a namespace had no boundary at all (MemTensor#593). The three assertions now reject a request whose namespace explicitly carries a userId or projectId that contradicts the row's owner (MemoryServiceError forbidden), mirroring the existing user-memory and L3 World Model checks in deleteMemory. A request without a namespace keeps the historical whole-local-store semantics, so single-namespace local installs see no change, and sparse namespaces (no userId/projectId) are not newly rejected. Single-memory and skill-detail GETs stay reachable across namespaces on purpose: the REST contract requires cross-key shared resource reads and the evolution pipeline merges policies across user ids, while the skills list endpoint is already namespace-unfiltered. The misleading assertMemoryInScope call is therefore removed from those two shared read paths; enforcement lives on the mutation paths (archive, delete, redact, panel task delete) and the session/turn/feedback/trial/import pipelines. Negative tests cover cross-namespace archive/delete, cross-project delete, no-namespace whole-store behavior, and cross-namespace episode delete plus raw-turn redaction.
This branch has not been deployed
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
1 participant
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.
Fixes #593
Root cause
assertMemoryInScope,assertEpisodeInScopeandassertRawTurnInScopeinMemory/src/service/memory-service.ts(pre-fix :2847/:2852/:2857) all had the bodyvoid memory; void namespace;— they discarded both parameters and enforced nothing. Yet the archive/delete/panel/redact paths, the retrieval pipeline, the read models, and the feedback/trial/import pipelines all inject these methods as the scope boundary, so every namespace-scoped call went through with no actual boundary.Fix (issue option 1, semantics-conservative)
The three assertions now perform real checks (
Memory/src/service/memory-service.ts:2851-2875):userId/projectIdcontradicting the row's owner →MemoryServiceError("forbidden"). This mirrors the existing in-function paradigm indeleteMemory(user-memory branch :1821, L3 World Model branches :1878-1881). Sparse namespaces (no userId/projectId) are not newly rejected.RawTurnRecordhas noprojectIdfield, so its assertion only comparesuserId.Contract alignment. Two existing tests prove single-item detail GETs are shared-resource reads, not authorization boundaries: the REST contract test ("enforces scoped API permissions … shared resource access") requires a cross-key
GET /memory/:idto return 200, and the evolution test requires cross-usergetMemorypolicy merging. The misleadingassertMemoryInScopecalls are therefore removed from the two shared read paths —getMemory(Memory/src/service/read-model/episode.ts:230) andgetSkill(Memory/src/service/read-model/skill.ts:248, whose list endpointGET /api/v1/skillswas already namespace-unfiltered) — each replaced by a comment explaining why. Enforcement stays on all mutation paths (archive/delete/redact/deletePanelTask) and the session/turn/feedback/trial/import pipelines — exactly the issue's request to "distinguish a relevance/project filter from an authorization boundary".Left untouched on purpose: the L3 v2 checks;
assertSessionInScope(:2824, same-shaped no-op, outside this issue's three-assertion scope);scopeBundleTables(:3119, whole-store export path).Test evidence
New negative test
Memory/tests/service/memory-scope-assertions.test.ts(the two cross-namespace negative tests named by the issue author, plus a no-namespace whole-store control):npx vitest run tests/service/memory-scope-assertions.test.ts→ 3 failed / 1 passed (cross-user archive/delete, cross-project delete, and episode delete + raw-turn redact all completed without throwing — the no-op confirmed).npx vitest run tests/service/memory-scope-assertions.test.ts→ 4 passed / 4.npx vitest run tests/service tests/contract→ 60 files / 568 tests all passed (includes the REST shared-read-200 contract case and the evolution cross-user policy-merge case).npx vitest run --dir tests→ 117 files passed / 1 skipped; 1080 tests passed / 1 skipped / 0 failed.npx tsc -p Memory/tsconfig.json --noEmit→ exit 0.AI disclosure
This fix was prepared with AI assistance (Claude Code) and submitted by a human contributor, as disclosed in the issue thread.