From 2c0b60410264867e95a8da6dcf07d91269289511 Mon Sep 17 00:00:00 2001 From: coderdailyone Date: Wed, 26 Aug 2026 05:24:56 -0700 Subject: [PATCH] security: validate session_id so transcript paths can't escape conv_dir MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ConversationStore._file built transcript paths by raw join of the client-minted session_id: conv_dir / f"{sid}.jsonl". Starlette's {session_id} route param excludes "/" but not "\", and "\" is a path separator on Windows, so a percent-encoded "..%5C..%5C.." survived routing and escaped conv_dir. delete() then called path.unlink() unconditionally (it runs regardless of the DB rowcount), giving arbitrary deletion of any .jsonl-suffixed file; the same id fed _append/save as a write primitive. Fix: validate the id at the _file chokepoint (covers read/count/append/ save/delete for every REST + WS entry) against a strict charset that every minted id matches (uuid hex, worker hex, __task__…, __run__…), and confirm the resolved path's parent is conv_dir. delete() now resolves the path before any DB or filesystem mutation and returns False on an invalid id, so a hostile DELETE touches nothing. Fixes #523 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01CLxsdFGXjdztTRNHjNgPXP --- coworker/conversations.py | 8 +++- tests/test_conversation_store_paths.py | 57 ++++++++++++++++++++++++++ 2 files changed, 64 insertions(+), 1 deletion(-) create mode 100644 tests/test_conversation_store_paths.py 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