From e4bb821da885eb685c1d84b3cf617d8dc0c59969 Mon Sep 17 00:00:00 2001 From: glatinone <93207632+glatinone@users.noreply.github.com> Date: Sun, 4 Oct 2026 16:00:05 +0800 Subject: [PATCH] fix(mcp): let the MCP binding act as a real agent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every access rule is decided from the caller's identity, and every MCP tool hard-coded `mcp_client`. So every MCP client pointed at one store *was* the same agent, with three consequences, one of them a security hole: - **A cell one MCP client created was readable by all of them.** `created_by` grants access to its creator, and the creator was the same synthetic id for everybody - so the per-cell access model was inert in this binding, which is the cross-agent leakage RFC-AMP-001 §5 lists as a threat. - **`readable_by` naming a real agent id matched nothing.** A caller passing it to `amp_remember` - the documented parameter - stored a cell its own server could not recall. That is the same root cause, and it makes the multi-agent use case the RFC cites for this binding unusable. - **`created_by` recorded a name no agent uses**, which is exactly the attribution §5 leans on to make a poisoned memory traceable. `AMP_MCP_AGENT_ID` now supplies the identity, read per call rather than cached at import, because the MCP client sets the environment when it spawns the process. Unset, it falls back to the old name so nothing breaks - and the fallback is documented as what it is: a shared namespace, not an identity. Both places a user copies from now set it: the getting-started MCP snippet and `examples/mcp-claude-desktop/mcp_config.json`. The release notes gained the caveat next to the other honest limits. Four tests, all of which fail on the old code: the configured id is what lands in `created_by`, the default is used when nothing is configured, a cell restricted to an agent is recallable when the server *is* that agent and invisible when it is another, and `amp_forget` is gated on the same identity. --- CHANGELOG.md | 12 ++++ docs/getting-started.md | 10 ++- docs/release-notes-v0.1.0.md | 4 ++ examples/mcp-claude-desktop/mcp_config.json | 3 +- server/amp_server/mcp_server.py | 31 +++++++-- server/tests/test_mcp_tools.py | 73 +++++++++++++++++++++ 6 files changed, 127 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1d43c8c..e91fe75 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -227,6 +227,18 @@ This project follows [Semantic Versioning](https://semver.org/spec/v2.0.0.html). `app_state` / `app_client` fixtures, and CI runs the server suite file by file so the whole class fails loudly instead of being averaged away by test ordering. +- **The MCP binding can act as a real agent** (`AMP_MCP_AGENT_ID`). Every access + rule is decided from the caller's identity, and this binding hard-coded + `mcp_client` - so every MCP client pointed at one store was the same agent: a + cell one of them created was readable by the others, `readable_by` patterns + naming real agent ids matched nothing, and `identity.created_by` recorded a name + no agent uses, which is the attribution RFC-AMP-001 §5 relies on to make a + poisoned memory traceable. A caller passing `readable_by` to `amp_remember` was + in the worst spot: it stored a cell its own server could not recall. The identity + is now read from the environment, one value per MCP server, defaulting to the old + shared name so nothing breaks - and the getting-started config and the example + `mcp_config.json` both set it. + ### Changed - **Every endpoint returns one error shape.** `PATCH /memories/{id}` answered a conflict with `{"detail": ...}` while `DELETE` answered with diff --git a/docs/getting-started.md b/docs/getting-started.md index 2037259..6f8a053 100644 --- a/docs/getting-started.md +++ b/docs/getting-started.md @@ -152,13 +152,21 @@ Add the following JSON snippet to your `claude_desktop_config.json` (typically l "command": "python", "args": ["-m", "amp_server.mcp_server"], "env": { - "AMP_PERSIST_DIR": "C:\\path\\to\\your\\persistent\\dir" + "AMP_PERSIST_DIR": "C:\\path\\to\\your\\persistent\\dir", + "AMP_MCP_AGENT_ID": "claude-desktop" } } } } ``` +`AMP_MCP_AGENT_ID` is the agent this MCP server acts as. Set it to something that +names the client, and set a different value for each one you run: every access rule +is decided from this identity, so a shared value means every MCP client pointed at +the same store is the same agent - cells one of them created are readable by the +others, and `readable_by` patterns naming real agent ids never match. Unset, it +falls back to `mcp_client`, which is a shared namespace rather than an identity. + > [!IMPORTANT] > The command must be run in an environment where the `amp-server` package (containing the `amp_server` module) is installed. Ensure your python environment path or active virtual environment is correctly accessible to the command. diff --git a/docs/release-notes-v0.1.0.md b/docs/release-notes-v0.1.0.md index c5836f5..eb44094 100644 --- a/docs/release-notes-v0.1.0.md +++ b/docs/release-notes-v0.1.0.md @@ -97,6 +97,10 @@ Stated plainly rather than left for you to discover: was developed on has no PostgreSQL, so the adapter's proof is the CI service container. Run `pytest tests/test_adapter_contract.py` with `AMP_TEST_POSTGRES_DSN` set to check it against yours. +- **The MCP binding's identity is shared by default.** One MCP server acts as one + agent (`AMP_MCP_AGENT_ID`); unset, every MCP client pointed at the same store is + the same agent, so per-agent `readable_by` is only meaningful once you set it - + and run one server per agent. - **Some state is per process.** The scoring-edit budget lives in the server process, so two processes over one database keep two budgets. The retention purge is likewise an in-process scheduled pass. diff --git a/examples/mcp-claude-desktop/mcp_config.json b/examples/mcp-claude-desktop/mcp_config.json index 7f3b8b7..461e524 100644 --- a/examples/mcp-claude-desktop/mcp_config.json +++ b/examples/mcp-claude-desktop/mcp_config.json @@ -4,7 +4,8 @@ "command": "python", "args": ["-m", "amp_server.mcp_server"], "env": { - "AMP_PERSIST_DIR": "/absolute/path/to/a/persistent/data/directory" + "AMP_PERSIST_DIR": "/absolute/path/to/a/persistent/data/directory", + "AMP_MCP_AGENT_ID": "claude-desktop" } } } diff --git a/server/amp_server/mcp_server.py b/server/amp_server/mcp_server.py index 42ccbf4..9e6cdc9 100644 --- a/server/amp_server/mcp_server.py +++ b/server/amp_server/mcp_server.py @@ -26,6 +26,29 @@ # Create the FastMCP server instance mcp = FastMCP("AMP") +#: The identity this MCP server acts as when `AMP_MCP_AGENT_ID` is unset. +DEFAULT_AGENT_ID = "mcp_client" + + +def agent_id() -> str: + """Which agent this MCP server acts as. + + Every access rule is decided from this identity, so one shared default means + every MCP client pointed at the same store is the same agent: a cell one of + them created is readable by all of them, `readable_by` patterns naming real + agent ids never match anything a caller wrote, and `identity.created_by` - + the attribution RFC-AMP-001 §5 leans on to make a poisoned memory traceable - + records a name no agent uses. A caller who passes `readable_by` to + `amp_remember` is in the worst spot: the cell it just stored is one its own + server may not be able to recall. + + Run one MCP server per agent and set `AMP_MCP_AGENT_ID` in its `mcp_config.json` + `env` block. Read per call rather than cached at import, because the client + sets the environment when it spawns this process. + """ + return os.environ.get("AMP_MCP_AGENT_ID") or DEFAULT_AGENT_ID + + # Storage injection holder for testing _storage: ChromaAdapter | None = None @@ -61,7 +84,7 @@ async def amp_remember( identity=MemoryIdentity( owner_id=owner_id, owner_type=OwnerType.USER, - created_by="mcp_client", + created_by=agent_id(), ), lifecycle=MemoryLifecycle( created_at=datetime.now(UTC), @@ -89,7 +112,7 @@ async def amp_recall( limit=limit, include_stale=include_stale, ) - results = await storage.search(request, agent_id="mcp_client") + results = await storage.search(request, agent_id=agent_id()) if not results: return "No memories found." @@ -114,7 +137,7 @@ async def amp_forget(memory_id: str, owner_id: str) -> str: if cell.identity.owner_id != owner_id: return "Memory not found." - if not check_write_access(cell, "mcp_client"): + if not check_write_access(cell, agent_id()): return "Memory not found." try: @@ -146,7 +169,7 @@ async def amp_list_memories( limit=limit, ) - allowed_cells = [c for c in cells if check_read_access(c, "mcp_client")] + allowed_cells = [c for c in cells if check_read_access(c, agent_id())] if not allowed_cells: return "No memories found." diff --git a/server/tests/test_mcp_tools.py b/server/tests/test_mcp_tools.py index 30e4e31..509b552 100644 --- a/server/tests/test_mcp_tools.py +++ b/server/tests/test_mcp_tools.py @@ -6,6 +6,7 @@ from conftest import make_cell from amp_server.mcp_server import ( + DEFAULT_AGENT_ID, amp_forget, amp_list_memories, amp_recall, @@ -168,3 +169,75 @@ async def test_amp_list_memories_success(storage): # List with no matches res_empty = await amp_list_memories(owner_id="user-456") assert res_empty == "No memories found." + + +# --------------------------------------------------------------------------- +# The identity the binding acts as +# --------------------------------------------------------------------------- + + +@pytest.mark.asyncio +async def test_the_configured_identity_is_recorded_as_created_by(storage, monkeypatch): + """`created_by` is what makes a memory attributable; a shared name is not that.""" + monkeypatch.setenv("AMP_MCP_AGENT_ID", "agent-research") + + result = await amp_remember(content="a fact", owner_id="user-123") + cell = await storage._get_raw(result.split(": ")[1]) + + assert cell.identity.created_by == "agent-research" + + +@pytest.mark.asyncio +async def test_the_default_identity_is_used_when_none_is_configured( + storage, monkeypatch +): + monkeypatch.delenv("AMP_MCP_AGENT_ID", raising=False) + + result = await amp_remember(content="a fact", owner_id="user-123") + cell = await storage._get_raw(result.split(": ")[1]) + + assert cell.identity.created_by == DEFAULT_AGENT_ID + + +@pytest.mark.asyncio +async def test_a_cell_restricted_to_an_agent_is_reachable_when_it_is_that_agent( + storage, monkeypatch +): + """The point of the variable: per-agent policy has to work through this binding. + + `amp_remember` takes `readable_by`. With one shared identity, a caller that + restricts a cell to a specific agent stores something its own server cannot + recall - the restriction names an agent that, from this binding, never calls. + """ + monkeypatch.setenv("AMP_MCP_AGENT_ID", "agent-research") + stored = await amp_remember( + content="restricted fact", owner_id="user-123", readable_by=["agent-research"] + ) + + recalled = await amp_recall(query="restricted fact", owner_id="user-123") + + assert "restricted fact" in recalled + + # And another identity cannot read it. + monkeypatch.setenv("AMP_MCP_AGENT_ID", "agent-somebody-else") + assert await amp_recall(query="restricted fact", owner_id="user-123") == ( + "No memories found." + ) + assert stored.startswith("Memory stored: mem_") + + +@pytest.mark.asyncio +async def test_forget_is_gated_on_the_configured_identity(storage, monkeypatch): + monkeypatch.setenv("AMP_MCP_AGENT_ID", "agent-research") + cell = make_cell( + owner_id="user-123", + created_by="agent-research", + writable_by=["agent-research"], + ) + await storage.save(cell) + + monkeypatch.setenv("AMP_MCP_AGENT_ID", "agent-intruder") + assert await amp_forget(cell.id, owner_id="user-123") == "Memory not found." + + monkeypatch.setenv("AMP_MCP_AGENT_ID", "agent-research") + assert (await amp_forget(cell.id, owner_id="user-123")).startswith("Memory ")