SECURITY: validate context IDs before chat persistence/removal to prevent path traversal outside `usr/chats` #43

New issue

Open

opened 2026-08-24 01:04:37 +00:00 by lost-rob0t · 0 comments

lost-rob0t commented

2026-08-24 01:04:37 +00:00

(Migrated from github.com)

Finding

Context IDs cross a filesystem trust boundary without canonical validation.

helpers/context_utils.py::use_context() accepts a caller-provided ctxid and, if it does not already exist, constructs an AgentContext(id=ctxid) verbatim.

Chat persistence then builds paths with:

files.get_abs_path("usr/chats", ctxid, "chat.json")

helpers.files.get_abs_path() is only os.path.join(_base_dir, *relative_paths) for relative inputs; it does not normalize/reject .., separators or absolute path components in a multi-part join.

More critically, api/chat_remove.py accepts input["context"], calls persist_chat.remove_chat(ctxid) even when no context exists, and remove_chat() calls:

path = get_chat_folder_path(ctxid)
files.delete_dir(path)

files.delete_dir() resolves the supplied joined path through the OS and recursively calls shutil.rmtree(..., ignore_errors=True) with no containment check.

A context ID containing traversal components can therefore escape usr/chats. Depending on process permissions, chat removal can recursively delete a directory outside the chat store, and persistence can create/write chat.json outside the intended store.

Example shape to cover in regression tests (do not use a real important path):

../../tmp/a0-context-traversal-fixture

Required fix direction

Treat context IDs as opaque identifiers with one canonical validator used at every external boundary and persistence entry point.

generated IDs should remain the normal format
reject path separators, ./.., absolute paths, NUL/control characters and invalid length/charset before lookup/create/remove
resolve chat paths and then independently assert Path(...).resolve().is_relative_to(CHATS_FOLDER.resolve())
destructive filesystem helpers used with externally-derived paths need containment guards at their owning boundary; never rely only on string joins
existing persisted IDs need an explicit compatibility/migration check rather than silently accepting unsafe names

Acceptance

chat_create cannot create a context whose ID escapes or creates nested filesystem paths.
chat_remove cannot delete anything outside the canonical usr/chats/<validated-id> directory.
save_tmp_chat, remove_chat, message-file helpers and chat-file path helpers defensively enforce containment even if called internally with an invalid ID.
WebSocket/context helper entry points use the same validator where client-controlled context IDs can create/select contexts.
Tests cover .., slash/backslash variants, absolute paths, URL-ish values, empty/whitespace, Unicode/control characters and overlong IDs.
A test uses a temporary sentinel directory outside the chat root and proves create/save/remove cannot touch it.
Existing valid chats continue loading; unsafe legacy names are reported/migrated safely rather than acted on as paths.
Audit other identifier-to-filesystem joins for the same containment assumption.

## Finding Context IDs cross a filesystem trust boundary without canonical validation. `helpers/context_utils.py::use_context()` accepts a caller-provided `ctxid` and, if it does not already exist, constructs an `AgentContext(id=ctxid)` verbatim. Chat persistence then builds paths with: ```python files.get_abs_path("usr/chats", ctxid, "chat.json") ``` `helpers.files.get_abs_path()` is only `os.path.join(_base_dir, *relative_paths)` for relative inputs; it does not normalize/reject `..`, separators or absolute path components in a multi-part join. More critically, `api/chat_remove.py` accepts `input["context"]`, calls `persist_chat.remove_chat(ctxid)` even when no context exists, and `remove_chat()` calls: ```python path = get_chat_folder_path(ctxid) files.delete_dir(path) ``` `files.delete_dir()` resolves the supplied joined path through the OS and recursively calls `shutil.rmtree(..., ignore_errors=True)` with no containment check. A context ID containing traversal components can therefore escape `usr/chats`. Depending on process permissions, chat removal can recursively delete a directory outside the chat store, and persistence can create/write `chat.json` outside the intended store. Example shape to cover in regression tests (do not use a real important path): ```text ../../tmp/a0-context-traversal-fixture ``` ## Required fix direction Treat context IDs as opaque identifiers with one canonical validator used at every external boundary and persistence entry point. - generated IDs should remain the normal format - reject path separators, `.`/`..`, absolute paths, NUL/control characters and invalid length/charset before lookup/create/remove - resolve chat paths and then independently assert `Path(...).resolve().is_relative_to(CHATS_FOLDER.resolve())` - destructive filesystem helpers used with externally-derived paths need containment guards at their owning boundary; never rely only on string joins - existing persisted IDs need an explicit compatibility/migration check rather than silently accepting unsafe names ## Acceptance - [ ] `chat_create` cannot create a context whose ID escapes or creates nested filesystem paths. - [ ] `chat_remove` cannot delete anything outside the canonical `usr/chats/<validated-id>` directory. - [ ] `save_tmp_chat`, `remove_chat`, message-file helpers and chat-file path helpers defensively enforce containment even if called internally with an invalid ID. - [ ] WebSocket/context helper entry points use the same validator where client-controlled context IDs can create/select contexts. - [ ] Tests cover `..`, slash/backslash variants, absolute paths, URL-ish values, empty/whitespace, Unicode/control characters and overlong IDs. - [ ] A test uses a temporary sentinel directory outside the chat root and proves create/save/remove cannot touch it. - [ ] Existing valid chats continue loading; unsafe legacy names are reported/migrated safely rather than acted on as paths. - [ ] Audit other identifier-to-filesystem joins for the same containment assumption.