fix(mcp): refuse config and ack writes in read-only mode (#2101)
Read-only gated on _MUTATING_TOOLS, which is the palace-write set the peer-writer lease consults to decide which calls need the palace mine lock. Two tools change state without touching the palace, so they are correctly absent from that set and were served on a --read-only server anyway: mempalace_hook_settings rewrites ~/.mempalace/config.json via MempalaceConfig.set_hook_setting, and mempalace_memories_filed_away unlinks ~/.mempalace/hook_state/last_checkpoint on both branches. Add _READ_ONLY_REFUSED_TOOLS and point the dispatch gate and the tools/list filter at it. _MUTATING_TOOLS and the peer-writer path are unchanged: adding the two names there instead would put a config-only tool under the palace lease, so a server that lost the lease to a peer would answer -32001 for a call the lease has no say over. mempalace_reconnect stays reachable on purpose and the comment records why, since clearing ChromaBackend._quarantined_paths lets the reopen rename a segment directory. The two --read-only help strings and the matching row in the remote-server guide said "the mutating tools", which now names the narrower set.
This commit is contained in:
mvalentsev
parent
aa89bd8227
commit
bd439ae54f
|
|
@ -2154,7 +2154,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"}
|
||||
|
|
@ -402,6 +404,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"}
|
||||
|
|
@ -4672,15 +4710,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 {
|
||||
|
|
@ -4752,8 +4794,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,
|
||||
|
|
@ -4761,7 +4804,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,
|
||||
|
|
@ -235,6 +236,118 @@ def test_read_only_off_exposes_mutating_tools(http_server):
|
|||
assert "mempalace_add_drawer" in names
|
||||
|
||||
|
||||
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",
|
||||
[
|
||||
|
|
|
|||
|
|
@ -5031,6 +5031,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