diff --git a/coworker/conversations.py b/coworker/conversations.py index 300ca9cc9..2ecb96add 100644 --- a/coworker/conversations.py +++ b/coworker/conversations.py @@ -533,12 +533,18 @@ def canonicalize_workspaces(self) -> None: self._conn.commit() def delete(self, session_id: str) -> bool: + # Resolve (and validate) the path BEFORE any DB or filesystem mutation so a hostile + # id can't unlink a .jsonl outside conv_dir — path.unlink() below runs regardless of + # the DB rowcount, which made it an arbitrary-delete primitive. + try: + path = self._file(session_id) + except ValueError: + return False with self._lock: cur = self._conn.execute( "DELETE FROM sessions WHERE session_id = ?", (session_id,) ) self._conn.commit() - path = self._file(session_id) if path.exists(): path.unlink() return cur.rowcount > 0 diff --git a/tests/test_conversation_store_paths.py b/tests/test_conversation_store_paths.py new file mode 100644 index 000000000..9c6d9b802 --- /dev/null +++ b/tests/test_conversation_store_paths.py @@ -0,0 +1,57 @@ +"""ConversationStore transcript paths must stay inside conv_dir. + +A client-minted session_id flows straight into ConversationStore._file() from the REST/WS +routes. Starlette's {session_id} param excludes "/" but not "\\" — a path separator on Windows +— so "..%5C..%5C.." would escape conv_dir and delete()'s unconditional unlink becomes an +arbitrary-delete primitive. These tests pin the store-level guard (platform-independent). +""" + +from __future__ import annotations + +import pytest + +from coworker.conversations import ConversationStore +from coworker.sessions import SessionRecord + +HOSTILE_IDS = [ + "../secret", # posix separator escapes on Linux/macOS + "..\\..\\secret", # backslash escapes on Windows + "sub/evil", # any embedded separator + "a.b", # "." is disallowed — keeps f"{sid}.jsonl" a single filename + "", + "x" * 129, # over the length bound +] + + +@pytest.mark.parametrize("sid", HOSTILE_IDS) +def test_file_rejects_ids_that_could_escape_conv_dir(tmp_path, sid): + store = ConversationStore(tmp_path) + with pytest.raises(ValueError): + store._file(sid) + + +def test_delete_with_traversal_id_touches_nothing_and_returns_false(tmp_path): + store = ConversationStore(tmp_path) + # A transcript-looking file one level above conv_dir (tmp_path/secret.jsonl). + secret = tmp_path / "secret.jsonl" + secret.write_text("keep me", encoding="utf-8") + + # conv_dir is tmp_path/conversations, so "../secret" resolves to the file above — the + # exact arbitrary-delete the unconditional unlink allowed before the fix. + assert store.delete("../secret") is False + assert secret.exists() # not unlinked + + +def test_valid_session_id_still_round_trips(tmp_path): + store = ConversationStore(tmp_path) + store.save( + SessionRecord( + session_id="__task__task-abc123", + workspace=str(tmp_path), + model="m", + mode="interactive", + ) + ) + assert store.load("__task__task-abc123") is not None + assert store.delete("__task__task-abc123") is True + assert store.load("__task__task-abc123") is None