Merge pull request #1325 from MemPalace/security/mcp-omit-absolute-paths
fix(mcp): omit absolute filesystem paths from MCP tool responses
This commit is contained in:
+12
-5
@@ -454,7 +454,6 @@ def _tool_status_via_sqlite() -> dict:
|
|||||||
"total_drawers": total,
|
"total_drawers": total,
|
||||||
"wings": wings,
|
"wings": wings,
|
||||||
"rooms": rooms,
|
"rooms": rooms,
|
||||||
"palace_path": _config.palace_path,
|
|
||||||
"protocol": PALACE_PROTOCOL,
|
"protocol": PALACE_PROTOCOL,
|
||||||
"aaak_dialect": AAAK_SPEC,
|
"aaak_dialect": AAAK_SPEC,
|
||||||
"vector_disabled": True,
|
"vector_disabled": True,
|
||||||
@@ -493,7 +492,6 @@ def tool_status():
|
|||||||
"total_drawers": count,
|
"total_drawers": count,
|
||||||
"wings": wings,
|
"wings": wings,
|
||||||
"rooms": rooms,
|
"rooms": rooms,
|
||||||
"palace_path": _config.palace_path,
|
|
||||||
"protocol": PALACE_PROTOCOL,
|
"protocol": PALACE_PROTOCOL,
|
||||||
"aaak_dialect": AAAK_SPEC,
|
"aaak_dialect": AAAK_SPEC,
|
||||||
}
|
}
|
||||||
@@ -914,12 +912,21 @@ def tool_get_drawer(drawer_id: str):
|
|||||||
return {"error": f"Drawer not found: {drawer_id}"}
|
return {"error": f"Drawer not found: {drawer_id}"}
|
||||||
meta = result["metadatas"][0]
|
meta = result["metadatas"][0]
|
||||||
doc = result["documents"][0]
|
doc = result["documents"][0]
|
||||||
|
# source_file is the absolute filesystem path written by the
|
||||||
|
# miners. Reduce to its basename before handing it to the MCP
|
||||||
|
# client — same threat model as the palace_path leak fix:
|
||||||
|
# nested-agent / multi-server topologies treat the client as a
|
||||||
|
# separate trust domain. Basename preserves citation utility.
|
||||||
|
# Mirrors the searcher.search_memories() return shape.
|
||||||
|
safe_meta = dict(meta) if meta else {}
|
||||||
|
if safe_meta.get("source_file"):
|
||||||
|
safe_meta["source_file"] = Path(safe_meta["source_file"]).name
|
||||||
return {
|
return {
|
||||||
"drawer_id": drawer_id,
|
"drawer_id": drawer_id,
|
||||||
"content": doc,
|
"content": doc,
|
||||||
"wing": meta.get("wing", ""),
|
"wing": safe_meta.get("wing", ""),
|
||||||
"room": meta.get("room", ""),
|
"room": safe_meta.get("room", ""),
|
||||||
"metadata": meta,
|
"metadata": safe_meta,
|
||||||
}
|
}
|
||||||
except Exception as e:
|
except Exception as e:
|
||||||
return {"error": str(e)}
|
return {"error": str(e)}
|
||||||
|
|||||||
@@ -531,6 +531,45 @@ class TestWriteTools:
|
|||||||
result = tool_get_drawer("nonexistent_drawer")
|
result = tool_get_drawer("nonexistent_drawer")
|
||||||
assert "error" in result
|
assert "error" in result
|
||||||
|
|
||||||
|
def test_get_drawer_does_not_leak_absolute_source_file_path(
|
||||||
|
self, monkeypatch, config, palace_path, collection, kg
|
||||||
|
):
|
||||||
|
"""tool_get_drawer must not expose the absolute filesystem path
|
||||||
|
that the miners write into ``source_file``. Same threat class as
|
||||||
|
the palace_path leak in mempalace_status: in nested-agent or
|
||||||
|
multi-server MCP topologies the client is a separate trust
|
||||||
|
domain, and the directory layout of the host has no documented
|
||||||
|
client-side use. Basename is enough for citation."""
|
||||||
|
_patch_mcp_server(monkeypatch, config, kg)
|
||||||
|
|
||||||
|
secret_dir = "/private/home/alice/secret-research/2026"
|
||||||
|
absolute_source = f"{secret_dir}/notes.md"
|
||||||
|
collection.add(
|
||||||
|
ids=["drawer_leak_probe"],
|
||||||
|
documents=["verbatim drawer body for leak probe"],
|
||||||
|
metadatas=[
|
||||||
|
{
|
||||||
|
"wing": "research",
|
||||||
|
"room": "notes",
|
||||||
|
"source_file": absolute_source,
|
||||||
|
"chunk_index": 0,
|
||||||
|
"added_by": "miner",
|
||||||
|
"filed_at": "2026-05-03T00:00:00",
|
||||||
|
}
|
||||||
|
],
|
||||||
|
)
|
||||||
|
|
||||||
|
from mempalace.mcp_server import tool_get_drawer
|
||||||
|
|
||||||
|
result = tool_get_drawer("drawer_leak_probe")
|
||||||
|
assert result["drawer_id"] == "drawer_leak_probe"
|
||||||
|
assert result["metadata"]["source_file"] == "notes.md"
|
||||||
|
# Defense-in-depth: no field anywhere in the response should
|
||||||
|
# contain the absolute path or its parent directory.
|
||||||
|
serialized = json.dumps(result)
|
||||||
|
assert absolute_source not in serialized
|
||||||
|
assert secret_dir not in serialized
|
||||||
|
|
||||||
def test_list_drawers(self, monkeypatch, config, palace_path, seeded_collection, kg):
|
def test_list_drawers(self, monkeypatch, config, palace_path, seeded_collection, kg):
|
||||||
_patch_mcp_server(monkeypatch, config, kg)
|
_patch_mcp_server(monkeypatch, config, kg)
|
||||||
from mempalace.mcp_server import tool_list_drawers
|
from mempalace.mcp_server import tool_list_drawers
|
||||||
|
|||||||
@@ -10,7 +10,7 @@ Palace overview: total drawers, wing and room counts, AAAK spec, and memory prot
|
|||||||
|
|
||||||
**Parameters:** None
|
**Parameters:** None
|
||||||
|
|
||||||
**Returns:** `{ total_drawers, wings, rooms, palace_path, protocol, aaak_dialect }`
|
**Returns:** `{ total_drawers, wings, rooms, protocol, aaak_dialect }`
|
||||||
|
|
||||||
---
|
---
|
||||||
|
|
||||||
@@ -122,7 +122,7 @@ Fetch a single drawer by ID — returns full content and metadata.
|
|||||||
|-----------|------|----------|-------------|
|
|-----------|------|----------|-------------|
|
||||||
| `drawer_id` | string | **Yes** | ID of the drawer to fetch |
|
| `drawer_id` | string | **Yes** | ID of the drawer to fetch |
|
||||||
|
|
||||||
**Returns:** `{ drawer: { id, wing, room, content, ... } }`
|
**Returns:** `{ drawer_id, content, wing, room, metadata }` where `metadata.source_file`, when present, is the basename only — the absolute path written by the miners is reduced before the dict is returned to MCP clients.
|
||||||
|
|
||||||
---
|
---
|
||||||
|
|
||||||
@@ -378,4 +378,4 @@ Force a reconnect to the palace database. Use this after external scripts or CLI
|
|||||||
|
|
||||||
**Parameters:** None
|
**Parameters:** None
|
||||||
|
|
||||||
**Returns:** `{ success, palace_path }`
|
**Returns:** `{ success, message, drawers, vector_disabled[, vector_disabled_reason] }` (on no-palace: `{ success: false, message, drawers, vector_disabled }`; on exception: `{ success: false, error }`)
|
||||||
|
|||||||
Reference in New Issue
Block a user