fix: harden three scope and ownership leak paths in memory and reflection storage#923
fix: harden three scope and ownership leak paths in memory and reflection storage#923gorkem2020 wants to merge 2 commits into
Conversation
792f959 to
62a9897
Compare
|
I completed the review on head Since #920 merged, GitHub now reports this branch as Please rebase onto or merge the latest |
… reflection storage Three separate leak paths let data cross agent boundaries that should never have been shared: 1. MemoryStore.list()'s scope filter included "OR scope IS NULL", so any pre-scoping legacy row with no scope was visible to every agent regardless of the requested scope filter. Drop the NULL-scope pass; a real scope filter now only matches rows actually in that scope. 2. isOwnedByAgent() let a blank owner pass for any requesting agent, and separately let owner === "main" match every requesting agent (not just "main" itself), for legacy/invariant/mapped reflection rows. Both were unconditional passes regardless of the requester's real identity. Blank/legacy rows without a real owner are now never inheritable, and "main" ownership only matches the literal "main" agent. 3. When a reflection's sessionKey failed to parse to a real agent, the plugin silently minted "main" as the owner and persisted the reflection under that identity. Combined with (2)'s old universal main match, an unparseable session's reflection was visible to every agent; even after (2), it would still be wrongly attributed to the literal "main" agent. Track the parsed agentId separately from the operational fallback used for scope/filenames/logging, and persist reflection ownership metadata with a blank (non-inheritable) owner instead of "main" when parsing fails. No data migration is included. Pre-existing NULL-scope rows and blank/legacy-main-owned reflection rows become invisible/non-inheritable after this change; operators who want them visible to a specific agent can re-scope or re-attribute them explicitly. Also updated two stale comments in isOwnedByAgent() that still described the removed "main fallback" behavior. Note: the same "OR scope IS NULL" pattern still exists in vectorSearch, bm25Search, lexicalFallbackSearch, stats, and fetchForCompaction in src/store.ts. Only the list() method named in this fix was in scope here; the others are flagged for a follow-up decision rather than changed silently. Wire the new test file into the local npm test chain and the core-regression CI group.
…/stats/fetchForCompaction
MemoryStore.list() was hardened against NULL-scope rows leaking through
a scope filter, but vectorSearch, bm25Search (and its lexicalFallbackSearch
helper), stats, and fetchForCompaction all built the identical
`(${scopeConditions}) OR scope IS NULL` WHERE clause and were left
unpatched. Two of these (stats, fetchForCompaction) have no app-layer
re-check at all, so a NULL-scope row was returned outright. The other
two (vectorSearch, bm25Search) do re-check scope in the application
layer, but only after defaulting a NULL scope to "global" — so the
leak still reaches any caller whose scope filter includes "global",
which is a common agent-scope-plus-shared-scope configuration.
Applies the same fix as list(): drop the `OR scope IS NULL` escape
hatch from all five WHERE clauses so NULL-scope legacy rows are only
ever visible to callers that pass no scope filter at all.
62a9897 to
f6526cb
Compare
|
Fixed the sibling read paths: vectorSearch, bm25Search/lexicalFallbackSearch, stats, and fetchForCompaction all carried the same unpatched WHERE clause. stats and fetchForCompaction had no app-layer re-check at all, and the vectorSearch/bm25Search re-check was defeated whenever the caller's scope filter included "global", since NULL scope defaults to "global" before that check runs. Applied the same one-line fix as list() at all five sites, with four new tests confirmed red against the pre-fix code. On the main-fallback question: removing that fallback is the fix itself, not a side effect. It is covered by the existing regression suite (main keeps access to its own rows; other agents lose only the previously leaked access). Rebased onto latest master. |
app3apps
left a comment
There was a problem hiding this comment.
Reviewed head f6526cb. The previous NULL-scope completeness finding is addressed across list, vector, BM25/lexical fallback, stats, and compaction paths, with regression coverage.
One blocking compatibility issue remains: the repository's existing test/isOwnedByAgent.test.mjs contract explicitly expects a main-owned invariant to be visible to a sub-agent and preserves the same fallback for legacy rows. This PR removes that behavior, but the contract is neither updated nor run by the active test manifest. That makes the change a concrete cross-agent inheritance regression, not only leak hardening.
Please either preserve intentional main/global invariant inheritance while closing unintended ownership leaks, or explicitly change the compatibility contract with active regression tests and a migration/release plan for existing rows. Also add the invariant ownership test to the normal CI manifest.
What Problem This Solves
Three separate paths let stored memory and reflection data cross agent boundaries that should never have been shared.
First, the general memory listing query included a clause that matched any row with no scope set at all, in addition to the requested scope. Any older row stored before scoping existed, or any row that ended up with a missing scope for another reason, was returned no matter which agent asked for which scope.
Second, the function that checks whether a reflection belongs to the requesting agent had two unconditional passes: a row with a blank owner was treated as belonging to everyone, and a row owned by "main" was treated as belonging to every requesting agent, not just the agent actually named "main".
Third, when the code that generates a reflection could not work out which agent a session belonged to from its session key, it silently treated the reflection as if it belonged to "main" and stored it that way. Combined with the second issue, this meant a reflection from a session that could not even be identified ended up visible to every agent, and even on its own it wrongly attributed content to the literal "main" agent.
Why This Change Was Made
Each of these was an unconditional bypass of the ownership and scope checks that are supposed to keep one agent's memory and reflections separate from another's. None of them required a broken or malicious input, they happened on ordinary legacy or unidentified data. Closing them means scope and ownership checks now mean what they say: a row is visible only to the scope it was stored under, and a reflection is inheritable only by the agent that actually owns it.
User Impact
After this change, rows with no scope set, and reflections with a blank or literal "main" owner, are no longer visible to other agents. If any such rows already exist from before these checks were tightened, they simply stop being returned, they are not deleted or altered. No migration is included as part of this change. If an operator wants a specific one of these older rows to remain visible or inheritable to a particular agent, it needs to be re-scoped or re-attributed to that agent explicitly. A parse failure on a session key that previously silently became a "main"-owned reflection is now stored without a usable owner (not editorializing the content, just no longer treating it as belonging to "main"), so it does not leak to any agent and is not wrongly attributed either.
One related item was intentionally left out of scope, the same missing-scope pattern also appears in a few other read paths that back the retrieval and recall pipeline (vector search, keyword search, a lexical fallback, stats, and a compaction read). Only the specific listing query described above was changed here, so the fix stays narrow and reviewable. The rest are worth a follow-up decision, since some of them may have relied on that fallback for a different reason and deserve their own look.
Evidence
Added a new test file covering all three fixes independently, plus regression tests confirming normal same-scope reads and exact-owner matches still work as before.
Verified each of the five new failing assertions is red against the code before this change and green after it.
Ran the full local test suite; all tests pass (one unrelated test that binds to a fixed local port was skipped due to a pre-existing environment conflict with an already-running local service, unrelated to this change).
tsc build completes with no errors, and the compiled dist files reflect the same change.
Production verification on a live deployment: after deploying this change, reflection inheritance remained fully intact for all agents (every stored reflection row carries a real agentId, so the stricter exact-match ownership changes nothing for correctly attributed data), confirming the hardening only affects the leak shapes it targets.