diff --git a/mempalace/cli.py b/mempalace/cli.py index e4a96ac..6f8edda 100644 --- a/mempalace/cli.py +++ b/mempalace/cli.py @@ -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", diff --git a/mempalace/mcp_server.py b/mempalace/mcp_server.py index b8de02e..7755c33 100644 --- a/mempalace/mcp_server.py +++ b/mempalace/mcp_server.py @@ -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) ] }, } diff --git a/tests/test_mcp_http_transport.py b/tests/test_mcp_http_transport.py index a6b6139..ce67ae9 100644 --- a/tests/test_mcp_http_transport.py +++ b/tests/test_mcp_http_transport.py @@ -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", diff --git a/tests/test_mcp_server.py b/tests/test_mcp_server.py index 0f61bfa..131854e 100644 --- a/tests/test_mcp_server.py +++ b/tests/test_mcp_server.py @@ -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 diff --git a/website/guide/remote-server.md b/website/guide/remote-server.md index 2e8d498..6e2daa4 100644 --- a/website/guide/remote-server.md +++ b/website/guide/remote-server.md @@ -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) |