mirror of
https://github.com/andrewyng/openworker.git
synced 2026-09-03 04:49:26 +00:00
Saidheerajgollu
03c4a16f95
A session id is joined straight into a filesystem path (`<id>.jsonl`),
and session ids come from client-controlled surfaces — the
`/ws/session/{session_id}` WebSocket route and REST paths all take the
id from the URL. Nothing validated it, so an id like `../evil` escaped
the conversations directory: `ConversationStore.save(SessionRecord(
session_id="../evil", ...))` wrote `evil.jsonl` one level ABOVE
`conversations/`, and a crafted id could clobber or place files
elsewhere under the state dir.
`_file()` — the single chokepoint every conversation-file path flows
through — now rejects ids that aren't a safe single path component and
confirms the resolved path stays inside `conv_dir`. The accepted
charset (`[A-Za-z0-9_-]{1,128}`) is a superset of every id the app
generates (uuid4 hex, and the `__run__`/`__task__`-prefixed automation
threads), so no legitimate session is affected; `load()` of an unknown
or unsafe id still returns None (the DB lookup misses before any file
IO), not an error.
The public `is_safe_session_id` helper is exported so the one other
site that turns a session id into a path — `_provision_scratch` in the
server manager — can reuse the same guard in a follow-up.
81 lines
2.8 KiB
Python
81 lines
2.8 KiB
Python
"""ConversationStore: session id path-traversal hardening.
|
|
|
|
A session id becomes a filename ("<id>.jsonl"); ids arrive from client-controlled
|
|
surfaces (the /ws/session/{id} route, REST paths), so a crafted id must never let a
|
|
write or read escape the conversations/ directory.
|
|
"""
|
|
|
|
from __future__ import annotations
|
|
|
|
import pytest
|
|
|
|
from coworker.conversations import ConversationStore, is_safe_session_id
|
|
from coworker.sessions import SessionRecord
|
|
|
|
|
|
def test_is_safe_session_id():
|
|
# Every id shape the app actually generates is accepted.
|
|
for ok in (
|
|
"0123456789abcdef0123456789abcdef", # uuid4().hex
|
|
"abc123def456", # uuid4().hex[:12]
|
|
"__run__run-abcdef1234", # automation run thread
|
|
"__task__task-0123456789", # automation task thread
|
|
):
|
|
assert is_safe_session_id(ok), ok
|
|
# Anything that could escape a single path component is rejected.
|
|
for bad in (
|
|
"../evil",
|
|
"../../etc/passwd",
|
|
"a/b",
|
|
"a\\b",
|
|
"..",
|
|
".",
|
|
"with space",
|
|
"dot.dot",
|
|
"",
|
|
"x" * 129, # over the length cap
|
|
):
|
|
assert not is_safe_session_id(bad), bad
|
|
|
|
|
|
def test_save_rejects_traversal_id_without_writing_outside(tmp_path):
|
|
"""The verified vuln: saving a record with '../evil' used to create 'evil.jsonl'
|
|
OUTSIDE the conversations dir. It must raise and write nothing."""
|
|
store = ConversationStore(tmp_path / "state")
|
|
rec = SessionRecord(
|
|
session_id="../evil",
|
|
workspace=str(tmp_path),
|
|
model="m",
|
|
mode="interactive",
|
|
messages=[{"role": "user", "content": "hi"}],
|
|
)
|
|
with pytest.raises(ValueError):
|
|
store.save(rec)
|
|
# Nothing landed outside conversations/ (the previous behavior wrote here).
|
|
assert not (tmp_path / "state" / "evil.jsonl").exists()
|
|
assert list((tmp_path / "state" / "conversations").glob("*.jsonl")) == []
|
|
|
|
|
|
def test_load_of_unknown_or_unsafe_id_is_none_not_crash(tmp_path):
|
|
store = ConversationStore(tmp_path / "state")
|
|
# A normal missing id: no DB row, returns None (never touches the filesystem).
|
|
assert store.load("deadbeef") is None
|
|
# An unsafe id also has no DB row, so load short-circuits to None before any file IO.
|
|
assert store.load("../evil") is None
|
|
|
|
|
|
def test_round_trip_with_valid_id_still_works(tmp_path):
|
|
store = ConversationStore(tmp_path / "state")
|
|
rec = SessionRecord(
|
|
session_id="abc123def456",
|
|
workspace=str(tmp_path),
|
|
model="m",
|
|
mode="interactive",
|
|
messages=[{"role": "user", "content": "hello"}],
|
|
)
|
|
store.save(rec)
|
|
loaded = store.load("abc123def456")
|
|
assert loaded is not None
|
|
assert loaded.messages[0]["content"] == "hello"
|
|
assert (tmp_path / "state" / "conversations" / "abc123def456.jsonl").is_file()
|