From 6f41fe7490ce92a86157891cfae0306d647659bf Mon Sep 17 00:00:00 2001 From: nesquena-hermes Date: Thu, 11 Jun 2026 21:41:51 +0000 Subject: [PATCH] fix(#3994): reject read-only + messaging sessions on BOTH happy-path and materialize fallback (Codex CORE x2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex caught two data-integrity gaps in the contributor's guard: (1) the get_session() happy path returned a stored session without checking read_only/ messaging, so an already-imported read-only session could be mutated via rename/update/move; (2) the fallback only checked cli_meta.read_only, but agent rows normalize messaging sources WITHOUT setting read_only — materializing a writable sidecar for a state.db-owned messaging session forks its title/state. Now reject getattr(s,'read_only') OR _is_messaging_session_record on the happy path, and cli_meta.read_only OR _is_messaging_session_record(cli_meta) in the fallback. Replaced the messaging-stub test with 3 regression tests (stored read-only, stored messaging, messaging cli_meta without read_only flag). --- api/routes.py | 13 ++++- tests/test_issue3994_materialize_session.py | 54 ++++++++++++++------- 2 files changed, 47 insertions(+), 20 deletions(-) diff --git a/api/routes.py b/api/routes.py index 88d49776..36f58c21 100644 --- a/api/routes.py +++ b/api/routes.py @@ -1579,6 +1579,11 @@ def _get_or_materialize_session(sid: str): try: s = get_session(sid) s = _ensure_full_session_before_mutation(sid, s) + # Read-only guard on the happy path too: an already-stored read-only / + # imported session (messaging or Claude Code) must not be mutated via + # rename/update/move. Session.save() does not enforce this itself. + if getattr(s, "read_only", False) or _is_messaging_session_record(s): + raise PermissionError("read-only imported session") return s except KeyError: pass @@ -1588,8 +1593,12 @@ def _get_or_materialize_session(sid: str): if not cli_meta: raise KeyError(sid) - # Read-only guard: messaging sessions and Claude Code imports cannot be mutated - if cli_meta.get("read_only"): + # Read-only guard: messaging sessions and Claude Code imports cannot be + # mutated. Reject BOTH an explicit read_only flag AND any messaging-source + # record — agent rows normalize messaging sources without setting read_only, + # and state.db (not a WebUI sidecar) is the source of truth for them, so + # materializing a writable sidecar would fork the title/state. + if cli_meta.get("read_only") or _is_messaging_session_record(cli_meta): raise PermissionError("read-only imported session") # Preserve source metadata fields diff --git a/tests/test_issue3994_materialize_session.py b/tests/test_issue3994_materialize_session.py index 79ee2a4f..678544de 100644 --- a/tests/test_issue3994_materialize_session.py +++ b/tests/test_issue3994_materialize_session.py @@ -71,26 +71,44 @@ def test_materialize_cli_session_imports_full_history(): assert getattr(out, "is_cli_session", None) is True -def test_materialize_messaging_session_builds_lightweight_stub(): - """A messaging session is materialized as a lightweight Session (state.db is the - source of truth) rather than importing a full message history.""" +def test_materialize_rejects_stored_readonly_session(): + """An already-STORED read-only session must be refused on the happy path too — + get_session() succeeding doesn't make it mutable (Codex CORE #1).""" import api.routes as routes - cli_meta = { - "read_only": False, - "title": "tg chat", - "model": "gpt-test", - "source_tag": "telegram", - } + ro = SimpleNamespace(session_id="ro_stored", profile="default", messages=[], read_only=True) + with patch("api.routes.get_session", return_value=ro), \ + patch("api.routes._ensure_full_session_before_mutation", return_value=ro): + with pytest.raises(PermissionError): + routes._get_or_materialize_session("ro_stored") + + +def test_materialize_rejects_stored_messaging_session(): + """A stored messaging session (state.db-owned) must be refused even with no + read_only flag (Codex CORE #1).""" + import api.routes as routes + + msg = SimpleNamespace(session_id="msg_stored", profile="default", messages=[], + session_source="messaging", source_tag="telegram") + with patch("api.routes.get_session", return_value=msg), \ + patch("api.routes._ensure_full_session_before_mutation", return_value=msg): + with pytest.raises(PermissionError): + routes._get_or_materialize_session("msg_stored") + + +def test_materialize_rejects_messaging_cli_meta_without_readonly_flag(): + """Messaging cli_meta lacking an explicit read_only flag must still be refused + — agent rows normalize messaging sources without setting read_only, and + state.db is the source of truth, so a writable sidecar would fork it (Codex CORE #2).""" + import api.routes as routes + + cli_meta = {"title": "tg chat", "model": "gpt-test", "source_tag": "telegram", "session_source": "messaging"} with patch("api.routes.get_session", side_effect=KeyError("msg1")), \ patch("api.routes._lookup_cli_session_metadata", return_value=cli_meta), \ - patch("api.routes._is_messaging_session_record", return_value=True), \ - patch("api.routes.get_cli_session_messages", return_value=[]), \ - patch("api.routes.title_from", return_value="tg chat"), \ - patch("api.routes.get_last_workspace", return_value="/tmp/ws"), \ + patch("api.routes.import_cli_session") as mock_import, \ patch("api.routes.Session") as MockSession: - stub = MockSession.return_value - stub.save = lambda **_kw: None - out = routes._get_or_materialize_session("msg1") - assert out is MockSession.return_value - assert MockSession.called + with pytest.raises(PermissionError): + routes._get_or_materialize_session("msg1") + assert not mock_import.called, "must not import a messaging session" + assert not MockSession.called, "must not build a writable messaging stub" +