Merge pull request #2126 from MemPalace/fix/2103-onto-develop
fix(mcp): refuse config and ack writes in read-only mode (#2103)
This commit is contained in:
Igor Lins e Silva
GitHub
commit
6ec20305a4
|
|
@ -2189,7 +2189,7 @@ def main():
|
|||
p_serve.add_argument(
|
||||
"--read-only",
|
||||
action="store_true",
|
||||
help="Expose recall only: mutating tools are hidden and refused",
|
||||
help="Expose recall only: tools that change state are hidden and refused",
|
||||
)
|
||||
p_serve.add_argument(
|
||||
"--allow-insecure",
|
||||
|
|
|
|||
|
|
@ -290,8 +290,8 @@ def _parse_args():
|
|||
parser.add_argument(
|
||||
"--read-only",
|
||||
action="store_true",
|
||||
help="Serve a read-only tool surface: the mutating tools are hidden from "
|
||||
"tools/list and refused at dispatch (env MEMPALACE_MCP_READ_ONLY)",
|
||||
help="Serve a read-only tool surface: the tools that change state are hidden "
|
||||
"from tools/list and refused at dispatch (env MEMPALACE_MCP_READ_ONLY)",
|
||||
)
|
||||
args, unknown = parser.parse_known_args()
|
||||
if unknown:
|
||||
|
|
@ -313,10 +313,12 @@ if _args.backend:
|
|||
|
||||
_config = MempalaceConfig()
|
||||
|
||||
# Read-only server mode: when on, the mutating tools are hidden from tools/list
|
||||
# and refused at dispatch (-32003). Resolved once at startup from --read-only or
|
||||
# MEMPALACE_MCP_READ_ONLY. Computed inline (not via _truthy_env, defined below)
|
||||
# so it is available to the request path regardless of import order.
|
||||
# Read-only server mode: when on, the tools in _READ_ONLY_REFUSED_TOOLS (defined
|
||||
# below) are hidden from tools/list and refused at dispatch (-32003). That is a
|
||||
# wider set than the _MUTATING_TOOLS the peer-writer guard uses. Resolved once at
|
||||
# startup from --read-only or MEMPALACE_MCP_READ_ONLY. Computed inline (not via
|
||||
# _truthy_env, defined below) so it is available to the request path regardless
|
||||
# of import order.
|
||||
_READ_ONLY = bool(getattr(_args, "read_only", False)) or os.environ.get(
|
||||
"MEMPALACE_MCP_READ_ONLY", ""
|
||||
).strip().lower() in {"1", "true", "yes", "on"}
|
||||
|
|
@ -403,6 +405,42 @@ _MUTATING_TOOLS = frozenset(
|
|||
}
|
||||
)
|
||||
|
||||
# Read-only mode (#1877) refuses a wider set than the peer-writer guard above.
|
||||
#
|
||||
# _MUTATING_TOOLS is the *palace-write* set: _mcp_peer_writer_refusal consults it
|
||||
# to decide which calls need this process to hold the palace mine lock. A tool
|
||||
# that never touches Chroma or the knowledge graph has to stay out of that set,
|
||||
# or a server that lost the lease to a peer would start refusing calls the lease
|
||||
# has no say over.
|
||||
#
|
||||
# Two tools are exactly that shape, and read-only has to name both because it is
|
||||
# a capability boundary rather than a lock: it exists so a shared server can
|
||||
# serve recall to a client that must not change server state.
|
||||
#
|
||||
# mempalace_hook_settings, given an argument, writes the server's
|
||||
# ~/.mempalace/config.json through MempalaceConfig.set_hook_setting.
|
||||
# service.WRITE_TOOLS already classifies it as a write, which the daemon uses
|
||||
# as an allowlist, so read-only was the odd one out.
|
||||
#
|
||||
# mempalace_memories_filed_away unlinks ~/.mempalace/hook_state/last_checkpoint
|
||||
# on both of its branches. Consuming the file is the contract of the tool, but
|
||||
# it is still a delete of state that outlives the process, on behalf of a
|
||||
# client with no write access. (service.classify_tool calls this one "read",
|
||||
# which is wrong for the same reason.)
|
||||
#
|
||||
# mempalace_reconnect is deliberately NOT here even though it is not write-free:
|
||||
# it clears ChromaBackend._quarantined_paths, so the reopen that follows can let
|
||||
# quarantine_stale_hnsw rename a segment directory. It is the only way to pick up
|
||||
# an external writer's changes, and _SQLITE_INTEGRITY_ALLOWED_TOOLS already keeps
|
||||
# it reachable for recovery, so gating it would strand a read-only server on a
|
||||
# stale index. This set means "refuse what a client asked to change", not
|
||||
# "nothing past here touches the disk" -- opening the palace or the knowledge
|
||||
# graph materialises files on its own, which no name-based gate can express.
|
||||
_READ_ONLY_REFUSED_TOOLS = _MUTATING_TOOLS | {
|
||||
"mempalace_hook_settings",
|
||||
"mempalace_memories_filed_away",
|
||||
}
|
||||
|
||||
|
||||
def _truthy_env(name: str) -> bool:
|
||||
return os.environ.get(name, "").strip().lower() in {"1", "true", "yes", "on"}
|
||||
|
|
@ -4816,15 +4854,19 @@ def _internal_tool_error(req_id, tool_name: str, exc: BaseException = None) -> d
|
|||
|
||||
|
||||
def _mcp_read_only_refusal(req_id, tool_name: str):
|
||||
"""Refuse mutating tools when the server runs in read-only mode (#1877).
|
||||
"""Refuse state-changing tools when the server runs in read-only mode (#1877).
|
||||
|
||||
Read-only is an operator-set server mode (``--read-only`` /
|
||||
``MEMPALACE_MCP_READ_ONLY``), distinct from the dynamic peer-writer lock:
|
||||
it is an unconditional gate so a shared team server can expose recall
|
||||
without write access. Enforced at dispatch, not merely hidden from
|
||||
tools/list, so a client that calls a mutating tool by name is still refused.
|
||||
|
||||
Gates on ``_READ_ONLY_REFUSED_TOOLS``, not ``_MUTATING_TOOLS``: a tool can
|
||||
write outside the palace database, which the peer-writer lease has no reason
|
||||
to arbitrate but read-only still has to refuse.
|
||||
"""
|
||||
if not _READ_ONLY or tool_name not in _MUTATING_TOOLS:
|
||||
if not _READ_ONLY or tool_name not in _READ_ONLY_REFUSED_TOOLS:
|
||||
return None
|
||||
|
||||
return {
|
||||
|
|
@ -4896,8 +4938,9 @@ def handle_request(request):
|
|||
# Notifications (no id) never get a response per JSON-RPC spec
|
||||
return None
|
||||
elif method == "tools/list":
|
||||
# In read-only mode, hide the mutating tools so clients don't advertise
|
||||
# In read-only mode, hide the refused tools so clients don't advertise
|
||||
# write capabilities they can't use (dispatch also refuses them, #1877).
|
||||
# Same set on both sides, or a tool would be listed and then rejected.
|
||||
return {
|
||||
"jsonrpc": "2.0",
|
||||
"id": req_id,
|
||||
|
|
@ -4905,7 +4948,7 @@ def handle_request(request):
|
|||
"tools": [
|
||||
{"name": n, "description": t["description"], "inputSchema": t["input_schema"]}
|
||||
for n, t in TOOLS.items()
|
||||
if not (_READ_ONLY and n in _MUTATING_TOOLS)
|
||||
if not (_READ_ONLY and n in _READ_ONLY_REFUSED_TOOLS)
|
||||
]
|
||||
},
|
||||
}
|
||||
|
|
|
|||
|
|
@ -20,6 +20,7 @@ Design constraints
|
|||
import http.client
|
||||
import json
|
||||
import logging
|
||||
import os
|
||||
import socketserver
|
||||
import ssl
|
||||
import threading
|
||||
|
|
@ -201,7 +202,7 @@ def test_bearer_token_enforced_when_configured(monkeypatch):
|
|||
|
||||
|
||||
def test_read_only_hides_and_refuses_mutating_tools(http_server, monkeypatch):
|
||||
"""Read-only mode (#1877): mutating tools are hidden from tools/list AND
|
||||
"""Read-only mode (#1877): the refused tools are hidden from tools/list AND
|
||||
refused at dispatch with -32003, while read tools still work."""
|
||||
monkeypatch.setattr(mcp, "_READ_ONLY", True)
|
||||
port, _ = http_server
|
||||
|
|
@ -211,7 +212,7 @@ def test_read_only_hides_and_refuses_mutating_tools(http_server, monkeypatch):
|
|||
names = {t["name"] for t in json.loads(body)["result"]["tools"]}
|
||||
assert "mempalace_search" in names # read tool stays
|
||||
assert "mempalace_add_drawer" not in names # mutating tool hidden
|
||||
assert names.isdisjoint(mcp._MUTATING_TOOLS)
|
||||
assert names.isdisjoint(mcp._READ_ONLY_REFUSED_TOOLS)
|
||||
|
||||
status, body = _post(
|
||||
port,
|
||||
|
|
@ -332,6 +333,117 @@ def test_writable_http_releases_writer_lease_after_bind_failure(monkeypatch):
|
|||
assert events == ["bind-failed", "discard", "lease-exit"]
|
||||
assert mcp._MCP_WRITER_LOCK_CM is None
|
||||
|
||||
def _hook_settings_call(req_id):
|
||||
return {
|
||||
"jsonrpc": "2.0",
|
||||
"id": req_id,
|
||||
"method": "tools/call",
|
||||
"params": {
|
||||
"name": "mempalace_hook_settings",
|
||||
"arguments": {"silent_save": False, "desktop_toast": True},
|
||||
},
|
||||
}
|
||||
|
||||
|
||||
def test_read_only_refuses_the_hook_settings_config_write(http_server, monkeypatch, tmp_path):
|
||||
"""mempalace_hook_settings writes the server's ~/.mempalace/config.json.
|
||||
|
||||
It touches no palace state, so it is correctly absent from _MUTATING_TOOLS,
|
||||
the palace-write set the peer-writer lease arbitrates. Read-only gated on
|
||||
that set, which let a read-only server persist a config change on behalf of
|
||||
a client that is supposed to have no write access at all.
|
||||
|
||||
The first half is the control: it proves the write really does land here, so
|
||||
the "unchanged" assertion in the second half cannot pass vacuously.
|
||||
"""
|
||||
home = tmp_path / "home"
|
||||
(home / ".mempalace").mkdir(parents=True)
|
||||
cfg_file = home / ".mempalace" / "config.json"
|
||||
cfg_file.write_text(
|
||||
json.dumps({"hooks": {"silent_save": True, "desktop_toast": False}}), encoding="utf-8"
|
||||
)
|
||||
monkeypatch.setenv("HOME", str(home))
|
||||
monkeypatch.setenv("USERPROFILE", str(home))
|
||||
monkeypatch.setenv("HOMEDRIVE", os.path.splitdrive(str(home))[0] or "C:")
|
||||
monkeypatch.setenv("HOMEPATH", os.path.splitdrive(str(home))[1] or str(home))
|
||||
pristine = cfg_file.read_bytes()
|
||||
|
||||
port, _ = http_server
|
||||
|
||||
# Control: the gate is off, so the very same call rewrites config.json.
|
||||
# _READ_ONLY is resolved at import from the environment, so pin it rather
|
||||
# than inherit whatever the suite was started with.
|
||||
monkeypatch.setattr(mcp, "_READ_ONLY", False)
|
||||
status, body = _post(port, "/mcp", _hook_settings_call(1))
|
||||
assert status == 200
|
||||
# The handler reports its own failures inside `result` as {"success": false},
|
||||
# not as a JSON-RPC error, so check the payload rather than just the envelope.
|
||||
payload = json.loads(body)
|
||||
assert "error" not in payload
|
||||
assert json.loads(payload["result"]["content"][0]["text"])["success"] is True
|
||||
assert cfg_file.read_bytes() != pristine
|
||||
cfg_file.write_bytes(pristine)
|
||||
|
||||
# Gate on: hidden from tools/list, refused at dispatch, file left alone.
|
||||
monkeypatch.setattr(mcp, "_READ_ONLY", True)
|
||||
|
||||
status, body = _post(port, "/mcp", {"jsonrpc": "2.0", "id": 2, "method": "tools/list"})
|
||||
names = {t["name"] for t in json.loads(body)["result"]["tools"]}
|
||||
assert "mempalace_hook_settings" not in names
|
||||
|
||||
status, body = _post(port, "/mcp", _hook_settings_call(3))
|
||||
assert status == 200
|
||||
assert json.loads(body)["error"]["code"] == -32003
|
||||
assert cfg_file.read_bytes() == pristine
|
||||
|
||||
|
||||
def test_read_only_refuses_the_checkpoint_ack_delete(http_server, monkeypatch, tmp_path):
|
||||
"""mempalace_memories_filed_away unlinks the Stop hook's checkpoint ack file.
|
||||
|
||||
Consuming that file is the contract of the tool, but it is still a delete of
|
||||
state that outlives the process, done for a client with no write access. Same
|
||||
two-phase shape as the config test: the control proves the delete lands, so
|
||||
the survival assertion afterwards cannot pass vacuously.
|
||||
"""
|
||||
home = tmp_path / "home"
|
||||
state_dir = home / ".mempalace" / "hook_state"
|
||||
state_dir.mkdir(parents=True)
|
||||
ack = state_dir / "last_checkpoint"
|
||||
ack.write_text(json.dumps({"msgs": 7, "ts": "2026-01-01T00:00:00"}), encoding="utf-8")
|
||||
monkeypatch.setenv("HOME", str(home))
|
||||
monkeypatch.setenv("USERPROFILE", str(home))
|
||||
monkeypatch.setenv("HOMEDRIVE", os.path.splitdrive(str(home))[0] or "C:")
|
||||
monkeypatch.setenv("HOMEPATH", os.path.splitdrive(str(home))[1] or str(home))
|
||||
|
||||
port, _ = http_server
|
||||
call = {
|
||||
"jsonrpc": "2.0",
|
||||
"id": 1,
|
||||
"method": "tools/call",
|
||||
"params": {"name": "mempalace_memories_filed_away", "arguments": {}},
|
||||
}
|
||||
|
||||
# Control: the gate is off, so the call consumes the ack file.
|
||||
monkeypatch.setattr(mcp, "_READ_ONLY", False)
|
||||
status, body = _post(port, "/mcp", call)
|
||||
assert status == 200
|
||||
assert json.loads(json.loads(body)["result"]["content"][0]["text"])["count"] == 7
|
||||
assert not ack.exists()
|
||||
|
||||
# Gate on: refused, and a fresh ack file survives untouched.
|
||||
ack.write_text(json.dumps({"msgs": 7, "ts": "2026-01-01T00:00:00"}), encoding="utf-8")
|
||||
pristine = ack.read_bytes()
|
||||
monkeypatch.setattr(mcp, "_READ_ONLY", True)
|
||||
|
||||
status, body = _post(port, "/mcp", {"jsonrpc": "2.0", "id": 2, "method": "tools/list"})
|
||||
names = {t["name"] for t in json.loads(body)["result"]["tools"]}
|
||||
assert "mempalace_memories_filed_away" not in names
|
||||
|
||||
status, body = _post(port, "/mcp", dict(call, id=3))
|
||||
assert status == 200
|
||||
assert json.loads(body)["error"]["code"] == -32003
|
||||
assert ack.read_bytes() == pristine
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"disconnect_exc",
|
||||
|
|
|
|||
|
|
@ -5300,6 +5300,85 @@ def test_peer_writer_guard_does_not_gate_read_tool(monkeypatch):
|
|||
assert '"ok": true' in response["result"]["content"][0]["text"]
|
||||
|
||||
|
||||
def test_read_only_refuses_exactly_the_refused_set(monkeypatch):
|
||||
"""Ask the gate which tools it refuses instead of restating the set.
|
||||
|
||||
Comparing against the whole TOOLS registry also catches a stale name: a tool
|
||||
renamed or removed while the set still lists it would gate nothing, and the
|
||||
two sides would stop matching.
|
||||
"""
|
||||
from mempalace import mcp_server
|
||||
|
||||
monkeypatch.setattr(mcp_server, "_READ_ONLY", True)
|
||||
|
||||
refused = {
|
||||
name for name in mcp_server.TOOLS if mcp_server._mcp_read_only_refusal(1, name) is not None
|
||||
}
|
||||
assert refused == set(mcp_server._READ_ONLY_REFUSED_TOOLS)
|
||||
assert "mempalace_hook_settings" in refused
|
||||
assert "mempalace_memories_filed_away" in refused
|
||||
# Reconnect stays reachable on purpose: it is the only way a read-only
|
||||
# server picks up an external writer's changes.
|
||||
assert "mempalace_reconnect" not in refused
|
||||
|
||||
# The palace-write set the peer-writer lease arbitrates stays the narrower
|
||||
# of the two; see test_peer_writer_guard_does_not_gate_hook_settings.
|
||||
assert mcp_server._MUTATING_TOOLS < mcp_server._READ_ONLY_REFUSED_TOOLS
|
||||
assert "mempalace_hook_settings" not in mcp_server._MUTATING_TOOLS
|
||||
|
||||
|
||||
def test_read_only_refuses_every_daemon_write_tool():
|
||||
"""Read-only must not be laxer than the daemon's own write classification.
|
||||
|
||||
service.WRITE_TOOLS is a security allowlist: execute_job lets the generic
|
||||
mcp_tool escape hatch run write-classified tools only. A tool the daemon
|
||||
calls a write while read-only serves it is the exact gap this fixes, and
|
||||
mempalace_hook_settings was that tool.
|
||||
"""
|
||||
from mempalace import mcp_server, service
|
||||
|
||||
assert service.WRITE_TOOLS <= mcp_server._READ_ONLY_REFUSED_TOOLS
|
||||
assert "mempalace_hook_settings" in service.WRITE_TOOLS
|
||||
|
||||
|
||||
def test_peer_writer_guard_does_not_gate_hook_settings(monkeypatch):
|
||||
"""The read-only widening must not leak into the peer-writer path.
|
||||
|
||||
mempalace_hook_settings writes the config file and never the palace, so it
|
||||
stays out of _MUTATING_TOOLS and the lease has no say over it. Read-only
|
||||
refuses it through _READ_ONLY_REFUSED_TOOLS instead. Were it moved into
|
||||
_MUTATING_TOOLS, a peer holding the lease would refuse it with -32001,
|
||||
including the no-argument form that only reads the current settings.
|
||||
"""
|
||||
from mempalace import mcp_server
|
||||
|
||||
def forbidden_lock():
|
||||
raise AssertionError("hook_settings should not acquire the peer-writer lock")
|
||||
|
||||
monkeypatch.setitem(
|
||||
mcp_server.TOOLS,
|
||||
"mempalace_hook_settings",
|
||||
{
|
||||
"description": "test config tool",
|
||||
"input_schema": {"type": "object", "properties": {}},
|
||||
"handler": lambda: {"ok": True},
|
||||
},
|
||||
)
|
||||
monkeypatch.setattr(mcp_server, "_acquire_mcp_writer_lock", forbidden_lock)
|
||||
|
||||
response = mcp_server.handle_request(
|
||||
{
|
||||
"jsonrpc": "2.0",
|
||||
"id": 9,
|
||||
"method": "tools/call",
|
||||
"params": {"name": "mempalace_hook_settings", "arguments": {}},
|
||||
}
|
||||
)
|
||||
|
||||
assert '"ok": true' in response["result"]["content"][0]["text"]
|
||||
assert "mempalace_hook_settings" not in mcp_server._MUTATING_TOOLS
|
||||
|
||||
|
||||
def test_status_tool_does_not_acquire_peer_writer_lock(monkeypatch):
|
||||
from mempalace import mcp_server
|
||||
|
||||
|
|
|
|||
|
|
@ -132,7 +132,7 @@ Output includes the token and the exact client command. Useful flags:
|
|||
| `--port` | `8765` | Listen port |
|
||||
| `--backend` | config/env | Storage backend (e.g. `qdrant`) |
|
||||
| `--tls-cert` / `--tls-key` | _(none)_ | PEM cert + key to terminate **TLS natively** (server speaks `https`) |
|
||||
| `--read-only` | off | Expose recall only — the mutating tools are hidden and refused |
|
||||
| `--read-only` | off | Expose recall only — the tools that change state are hidden and refused |
|
||||
| `--token` | auto | Use a specific bearer token instead of the generated one |
|
||||
| `--allow-insecure` | off | Permit a non-loopback bind with no token (only behind a trusted proxy) |
|
||||
|
||||
|
|
|
|||
Loading…
Reference in New Issue