Fix #163: enforce agent-memory trust model (bind agent_id + visibility)#167
Merged
Conversation
#151 shipped agent-memory conventions + read-side scoping but left the trust model advisory-only (the 2.19.0 CHANGELOG documented agent_id as self-asserted and visibility as stored-but-not-enforced). This makes both real. Write-path identity binding (ArticleController.create): - An agent-role key writing agent-memory metadata (agent_id/memory_type/ visibility) has metadata.agent_id stamped from its verified key identity, overriding any spoofed value. An agent can no longer author a memory under another agent's identity. - An agent key with no agent identity gets 403 agent_identity_required for such a write. Higher roles (orchestrator/user) may still attribute on behalf of others (orchestration/backfill). Plain non-memory articles are untouched. Read-path visibility enforcement (Loopctl.Knowledge): - New maybe_filter_by_visibility/2 predicate: an article is visible to an agent when its visibility is absent/shared OR the caller owns it (metadata.agent_id matches). Wired into apply_article_filters (list), apply_search_filters (keyword/semantic/combined + context-via-search), list_index, and get_article (single fetch → 404 with no existence leak). - The caller's scope is computed by LoopctlWeb.Helpers.Visibility.scope_opts/1: agent role → [visibility_agent_id: key.agent_id]; higher roles → [] (see all). Threaded through the article/search/context/index read controllers. The key's agent_id (a UUID) is the canonical identity; metadata.agent_id is stamped/compared as its string form. Tests: new agent_memory_trust_test.exs — write binding (stamp/override, omitted, no-identity 403, higher-role passthrough, non-memory untouched) and read enforcement (private hidden from other agents on get/list/index/search/context, owner sees own, shared visible, higher role sees all). Verified deterministic across seeds (caught + fixed a String.to_existing_atom hazard and a list_index filter gap during the sweep). Full suite green (2511), mix precommit clean. Docs: mcp-server 2.21.0 CHANGELOG documents the enforcement and marks the 2.19.0 advisory note superseded.
…round 1) The enhanced-review team found the read enforcement was incomplete: the first pass only covered list/index/show/search/context, but agent memories are published, so every OTHER agent-reachable read path leaked private/owner memories. Fixed all of them so the trust barrier holds across the whole surface. Read paths now visibility-scoped (for agent callers): - distant_pairs (/knowledge/pairs) — candidate set filtered - random_walk (/knowledge/walk) — start node + every neighbor step - graph_traversal (/knowledge/graph) — start gate + the recursive CTE node set (visibility clause bound as a param, injection-safe) - suggest_links (/knowledge/articles/:id/suggested_links) — start + candidates - count_articles / tag_facets (/knowledge/count, /knowledge/facets) — via the apply_article_filters chokepoint (controllers now pass the scope) - stats (/knowledge/stats) — base query filtered - novelty_scores (/knowledge/novelty) — private priors excluded from the comparison set and the prior_count - get_article preloaded links + context batch_linked_refs — linked-article refs are filtered so a shared article never leaks a private memory's title (closes the "no existence leak" gap the BA flagged) All five remaining agent-facing controllers merge LoopctlWeb.Helpers.Visibility .scope_opts/1, mirroring the article/search/index/context wiring. Tests: agent_memory_trust_test gains a "cross-endpoint visibility isolation" suite asserting agent A's private memory never leaks to agent B through pairs, walk, graph, suggest_links, count, stats, facets, or novelty — plus owner- visibility and identity-less-agent read cases (engineer/architect test gaps). Verified deterministic across seeds. Full suite green (2518), precommit clean. Docs (BA findings): - mcp-server CHANGELOG 2.21.0 lists all covered read surfaces and adds a migration note: pre-#163 memories written with a non-UUID agent_id won't match their owner's registry-UUID key identity (backfill or treat as a clean break). - knowledge_create MCP tool gains a `metadata` property (the handler now forwards it) documenting agent-memory keys + the trust model (agent_id key-stamped, visibility enforced, 403 agent_identity_required). knowledge_context notes that private/owner peers' memories are excluded regardless of the agents= filter.
…nd 2) The 4-agent adversarial review found two more leak paths that surface article state INDIRECTLY (round 1 only covered the direct knowledge reads), plus an identity edge case. All fixed; one finding disproved. CRITICAL — change feed (GET /changes): - Audit `new_state`/`old_state` embed an article's body + metadata, and the change feed is agent-reachable. An agent could poll it and read every private memory another agent ever created/edited. Now, for agent callers, article change entries are dropped unless the underlying article is currently visible to the caller (new Knowledge.visible_article_ids/3 does one batched lookup — robust even for events whose snapshot omits metadata, e.g. article.created). HIGH — GET /articles/:id/links + POST /article_links: - list_links_for_article returned linked articles' id+title with no visibility filter (a sibling of the get_article/batch_linked_refs leak round 1 closed). It now takes the caller scope and drops links whose far side isn't visible (both-sides-visible also gates probing a hidden start article → empty). - create_link gained a visibility gate: an agent may not link to an article it can't see (→ 404, no existence leak), closing the "link-then-list to read a private title" attack. Self-links still fall through to the 422 changeset rejection (distinct-id count). MEDIUM — blank agent_id: - The changeset now rejects metadata.agent_id == "" (it was a valid string), so a higher role can't store an ""-owned "private" memory that an identity-less agent key (whose scope is agent_id = "") would then match and read. LOW — bridge middle node: - distant_pairs `bridge_path` now requires the 2-hop middle node to ALSO be visible to an agent caller, so a pair can't be reported bridgeable through a private memory. Disproved (documented, not a deferral): - "PATCH /articles/:id allows agent_id spoofing" — update/delete are role :user; agents cannot reach them. Higher-role attribution on update is by-design, consistent with create (only the agent role is key-bound). Perf (architect): the visibility predicate is intentionally not given a dedicated index — its OR returns the shared majority, so a partial index on private/owner rows can't serve the dominant read; the per-row JSONB extraction is cheap on the already-tenant-scoped scan (rationale documented in code). Tests: +7 — change-feed isolation, /links isolation, create-link rejection, graph + context owner-positive (over-filtering guard), blank-agent_id 422. Deterministic across seeds. Full suite green (2523), precommit clean, MCP 45/45. Docs: OpenAPI operation() descriptions on the 7 now-filtered endpoints + README rows note the visibility scoping; the CHANGELOG migration note is now an actionable procedure (identify SQL + PATCH backfill).
A focused final security verification confirmed the round-2 fixes are correct and the read surface is exhaustively covered, but found two LOW residual existence/id leaks (no content). Both are the exact existence-leak class this PR closes, so fixed here per NO-DEFERRALS. - Idempotency probe (A10-1): create with another agent's private memory's idempotency_key returned `200 deduplicated` echoing that private article's id. The dedup lookup (fast path AND the post-insert unique-violation recovery) is now visibility-scoped: an agent only dedups against a match it can see; a key colliding with an invisible memory falls through to the unique index, which rejects the write (422) without revealing the private id. - Change-feed article_link entries (A10-2): `article_link.created/deleted` entries carry both endpoint UUIDs and were unfiltered. They're now dropped for an agent unless BOTH endpoints are visible (folded into the same visible_article_ids lookup), mirroring the /links both-sides rule — a link can't leak a private memory's id or graph edge. Verifier verdict on the round-2 fixes: all PASS (change-feed id types match, no over-drop/leak; /links fail-closed; create_link distinct-id self-link → 422; blank agent_id rejected before length check on create+update; bridge 10 args/10 placeholders, vis bound). Non-leaks confirmed role-gated away from agents: webhooks (subscribe is :user), analytics (:orchestrator), exports (:user). Tests: +2 — article_link change-feed isolation, idempotency-probe non-disclosure. Deterministic across seeds. Full suite green (2525), precommit clean.
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.
Summary
Closes #163. #151 shipped agent-memory conventions + read-side scoping but left the trust model advisory-only (the 2.19.0 CHANGELOG documented
agent_idas self-asserted andvisibilityas stored-but-not-enforced). This PR makes both a real trust barrier.Write-path:
agent_idbinding (ArticleController.create)agent_id/memory_type/visibility) hasmetadata.agent_idstamped from its verified key identity, overriding any spoofed value — an agent can no longer author a memory under another agent's identity.agent_identity_requiredfor such a write. Higher roles (orchestrator/user) may still attribute on behalf of others. Plain non-memory articles are untouched.Read-path:
visibilityenforcement (Loopctl.Knowledge)maybe_filter_by_visibility/2: an article is visible to an agent when itsvisibilityis absent/shared, OR the caller owns it (metadata.agent_idmatches). Wired intoapply_article_filters(list),apply_search_filters(keyword/semantic/combined + context-via-search),list_index, andget_article(single fetch →404, no existence leak).LoopctlWeb.Helpers.Visibility.scope_opts/1: agent role →[visibility_agent_id: key.agent_id]; higher roles →[](see all). Threaded through the article/search/context/index read controllers.The key's
agent_id(UUID) is the canonical identity;metadata.agent_idis stamped/compared as its string form.Tests
New
agent_memory_trust_test.exs— write binding (stamp/override, omitted, no-identity 403, higher-role passthrough, non-memory untouched) and read enforcement (private hidden from another agent on get/list/index/search/context, owner sees own, shared visible, higher role sees all). Verified deterministic across seeds — the sweep caught and fixed aString.to_existing_atomflakiness hazard and alist_indexfilter gap. Full suite green (2511),mix precommitclean.Docs
mcp-server 2.21.0 CHANGELOG documents the enforcement and marks the 2.19.0 advisory note superseded.