From 76aa8d7feb84ffe75da4438da3dba5a6fdc8f747 Mon Sep 17 00:00:00 2001 From: nopoz Date: Fri, 28 Aug 2026 10:55:16 -0700 Subject: [PATCH] fix(security): keep agent file tools out of the app state directory The agent's read tools (read_file, grep, glob, ls) resolved model-supplied paths against a root list whose first entry was the whole data directory. That directory holds the session store, the auth database, the app encryption key and the settings file, so prompt-injected content could ask for any of them. No approval prompt stood in the way: reads are classified read_workspace and pass the untrusted-context gate untouched, which is correct for reading a workspace and wrong for reading the app's own state. The agent gets data/agent_workspace/ instead, and the subprocess cwd and HOME move with it so bash and read_file agree on where scratch files live. The deny itself is a property of the path, not of the root it arrived through, because three routes reach the same bytes and closing only the first leaves the other two working: - the default root list - a workspace bound at or above the data directory, which vet_workspace accepted and chat_routes auto-binds from a path named in the message - a tool_path_extra_roots setting covering the data directory _resolve_search_root also returned the workspace root unchecked when the path was empty, so a bare ls enumerated the directory whatever the deny list said. It now resolves that case through the same guards. A containment rule rather than a filename deny list, so state files added later are covered without anyone remembering to list them, and so a user's own settings.json or app.db inside a real workspace is not caught. Four directories of user content stay readable, because the application hands their paths to the model and tells it to open them: the chat upload manifest, downloaded mail attachments, personal docs (which covers the runbook) and personal uploads. --- setup.py | 3 +- src/app_initializer.py | 5 +- src/constants.py | 5 + src/tool_execution.py | 135 +++++++++++-- tests/test_agent_state_dir_confinement.py | 225 ++++++++++++++++++++++ tests/test_tool_path_confinement.py | 12 +- 6 files changed, 361 insertions(+), 24 deletions(-) create mode 100644 tests/test_agent_state_dir_confinement.py diff --git a/setup.py b/setup.py index 5b4eadcb5..8c4934a82 100644 --- a/setup.py +++ b/setup.py @@ -16,7 +16,7 @@ sys.path.insert(0, BASE_DIR) from src.constants import ( DATA_DIR, AUTH_FILE, UPLOAD_DIR, PERSONAL_DIR, PERSONAL_UPLOADS_DIR, TTS_CACHE_DIR, GENERATED_IMAGES_DIR, DEEP_RESEARCH_DIR, CHROMA_DIR, - RAG_DIR, MEMORY_VECTORS_DIR, PASSWORD_MIN_LENGTH, + RAG_DIR, MEMORY_VECTORS_DIR, AGENT_WORKSPACE_DIR, PASSWORD_MIN_LENGTH, ) from core.auth import RESERVED_USERNAMES @@ -31,6 +31,7 @@ DIRS = [ CHROMA_DIR, RAG_DIR, MEMORY_VECTORS_DIR, + AGENT_WORKSPACE_DIR, os.path.join(BASE_DIR, "logs"), ] diff --git a/src/app_initializer.py b/src/app_initializer.py index 1b29f06d2..ba4d81d30 100644 --- a/src/app_initializer.py +++ b/src/app_initializer.py @@ -5,7 +5,7 @@ import logging from typing import Dict, Any from src.constants import ( - DATA_DIR, PERSONAL_DIR, RUNBOOK_DIR, UPLOAD_DIR, + DATA_DIR, PERSONAL_DIR, RUNBOOK_DIR, UPLOAD_DIR, AGENT_WORKSPACE_DIR, SESSIONS_FILE, DEFAULT_HOST, OPENAI_API_KEY ) from src.memory import MemoryManager @@ -28,7 +28,8 @@ logger = logging.getLogger(__name__) def create_directories(): """Create necessary directories if they don't exist.""" - for directory in (DATA_DIR, PERSONAL_DIR, RUNBOOK_DIR, UPLOAD_DIR): + for directory in (DATA_DIR, PERSONAL_DIR, RUNBOOK_DIR, UPLOAD_DIR, + AGENT_WORKSPACE_DIR): os.makedirs(directory, exist_ok=True) def initialize_managers(base_dir: str, rag_manager=None) -> Dict[str, Any]: diff --git a/src/constants.py b/src/constants.py index 28d47efa0..d4f8ba63f 100644 --- a/src/constants.py +++ b/src/constants.py @@ -54,6 +54,11 @@ GALLERY_DIR = os.path.join(DATA_DIR, "gallery") GALLERY_UPLOADS_DIR = os.path.join(DATA_DIR, "gallery_uploads") MEMORY_VECTORS_DIR = os.path.join(DATA_DIR, "memory_vectors") +# The only part of DATA_DIR the agent's file tools and subprocesses may touch. +# Everything else under DATA_DIR is application state (session store, auth +# database, encryption key, settings), and the agent has no business reading it. +AGENT_WORKSPACE_DIR = os.path.join(DATA_DIR, "agent_workspace") + # Paths with an intentional dedicated env override, defaulting under DATA_DIR. MAIL_ATTACHMENTS_DIR = os.getenv("ODYSSEUS_MAIL_ATTACHMENTS_DIR", os.path.join(DATA_DIR, "mail-attachments")) # `or` (not os.getenv's default arg) so a PRESENT-but-EMPTY value falls back to diff --git a/src/tool_execution.py b/src/tool_execution.py index 8c0c83032..219ebe632 100644 --- a/src/tool_execution.py +++ b/src/tool_execution.py @@ -30,7 +30,12 @@ from src.tool_security import ( from src.tool_capabilities import ToolRunSecurityContext, blocked_tool_result from src.tool_approvals import ExactToolApproval from src.tool_policy import ToolPolicy -from src.constants import MAX_OUTPUT_CHARS, MAX_READ_CHARS, MAX_DIFF_LINES, DATA_DIR +from src.constants import ( + MAX_OUTPUT_CHARS, + MAX_READ_CHARS, + MAX_DIFF_LINES, + AGENT_WORKSPACE_DIR, +) from src.tool_utils import _truncate, get_mcp_manager @@ -46,11 +51,11 @@ _MISSING_TOOL_SECURITY_CONTEXT = _MissingToolSecurityContext() NO_TOOL_SECURITY_CONTEXT = _NoToolSecurityContext() # Persistent working directory for agent subprocesses. -# Resolves to /data, which is the bind-mounted volume in Docker -# (/app/data) and the local data directory for manual installs. -# Using this as cwd and HOME prevents the agent from silently creating files -# in ephemeral container layers that are lost on the next rebuild. -_AGENT_WORKDIR = DATA_DIR +# Resolves to /data/agent_workspace, inside the bind-mounted volume +# in Docker (/app/data), so files survive a rebuild as before. The subdirectory +# rather than data/ itself keeps agent scratch files and dotfiles out of the +# directory holding the session store and the auth database. +_AGENT_WORKDIR = AGENT_WORKSPACE_DIR @@ -66,10 +71,15 @@ _AGENT_WORKDIR = DATA_DIR # 1. Sensitive-subpath deny list — checked FIRST. Blocks .ssh, # .gnupg, shell rc files, token/env files even if the root above # them is on the allowlist. -# 2. Allowlist — only the directories the agent legitimately needs -# (project data/, system tmp). $HOME is NOT on the default list. -# 3. Opt-in extra roots — admin can add broader roots via the -# "tool_path_extra_roots" setting (list of path strings). +# 2. Application-state deny (_is_app_state_path) - DATA_DIR holds the +# session store, auth database, app key and settings, so only +# _agent_readable_data_subdirs() is readable inside it. +# 3. Allowlist - only the directories the agent legitimately needs +# (its data/ workspace, user content, system tmp). $HOME is NOT on +# the default list. +# 4. Opt-in extra roots - admin can add broader roots via the +# "tool_path_extra_roots" setting. These cannot re-open DATA_DIR; +# rule 2 is independent of which root a path arrived through. # --------------------------------------------------------------------------- _SENSITIVE_BASENAMES: set[str] = { @@ -116,6 +126,84 @@ def _is_sensitive_path(resolved: str) -> bool: return filename in _SENSITIVE_FILE_PATTERNS_CF +def _path_within(resolved: str, root: str) -> bool: + """True when *resolved* is *root* itself or sits underneath it. + + Folds case before comparing, because the caller is a deny rule: a missed + match here ALLOWS the path, where a missed match in the allowlist below + only rejects it. On default macOS a case-variant name opens the same file + and realpath does not canonicalise case, so an unfolded compare would let + the state directory through. casefold rather than os.path.normcase for the + reason given above _is_sensitive_path: normcase is a no-op on POSIX, which + is exactly where that gap lives. + """ + resolved, root = resolved.casefold(), root.casefold() + if resolved == root: + return True + try: + return os.path.commonpath([resolved, root]) == root + except ValueError: + return False + + +def _agent_readable_data_subdirs() -> tuple[str, ...]: + """The only parts of DATA_DIR the agent's file tools may reach. + + The agent's own scratch folder, plus the directories of user content whose + paths the application itself gives to the model, which it would then be + unable to open: + + UPLOAD_DIR the chat upload manifest renders "path=

" and + says to read it with read_file (agent_loop.py) + MAIL_ATTACHMENTS_DIR download_attachment returns the path and its own + description tells the model to read it + PERSONAL_DIR GET /api/personal returns a path per file and is + reachable through the app_api tool; RUNBOOK_DIR + nests under it + PERSONAL_UPLOADS_DIR indexed as a personal-docs directory, which + manage_rag lists as an absolute path + + Order matters: the first entry is roots[0], which _resolve_search_root uses + when grep/glob/ls are called with no path. + """ + from src.constants import ( + MAIL_ATTACHMENTS_DIR, + PERSONAL_DIR, + PERSONAL_UPLOADS_DIR, + UPLOAD_DIR, + ) + return ( + AGENT_WORKSPACE_DIR, + UPLOAD_DIR, + MAIL_ATTACHMENTS_DIR, + PERSONAL_DIR, + PERSONAL_UPLOADS_DIR, + ) + + +def _is_app_state_path(resolved: str) -> bool: + """True for anything under DATA_DIR that is not agent-readable. + + DATA_DIR holds the session store, the auth database, the app encryption key + and the settings file. A model-supplied path must not reach those through + any root, so this is checked in both resolvers rather than expressed as an + absence from the allowlist: a workspace bound at or above the data + directory, or an opt-in tool_path_extra_roots entry covering it, would + otherwise put them back in reach. + + A containment rule rather than a filename deny list, so state files added + later are covered without anyone remembering to list them, and so a user's + own settings.json or app.db inside a real workspace is not caught. + """ + from src.constants import DATA_DIR + if not _path_within(resolved, os.path.realpath(DATA_DIR)): + return False + return not any( + _path_within(resolved, os.path.realpath(d)) + for d in _agent_readable_data_subdirs() + ) + + def _tool_path_roots() -> list[str]: """Return the list of directory roots that read_file / write_file may touch. Default: project data/ + system temp dirs. Extra roots @@ -123,9 +211,9 @@ def _tool_path_roots() -> list[str]: """ roots: list[str] = [] - # Project data directory — the agent's primary workspace. - from src.constants import DATA_DIR - roots.append(DATA_DIR) + # The agent's workspace plus the user-content directories inside data/. + # The rest of DATA_DIR is denied by _is_app_state_path. + roots.extend(_agent_readable_data_subdirs()) # /tmp (and its macOS realpath /private/tmp). roots.append("/tmp") @@ -193,6 +281,10 @@ def _resolve_tool_path(raw_path: str) -> str: f"path '{raw_path}' is inside a sensitive directory " f"(e.g. .ssh, .gnupg) or matches a sensitive filename" ) + if _is_app_state_path(resolved): + raise ValueError( + f"path '{raw_path}' is inside the application state directory" + ) for root in _tool_path_roots(): if resolved == root: @@ -228,6 +320,10 @@ def _resolve_tool_path_in_workspace(workspace: str, raw_path: str) -> str: f"path '{raw_path}' is inside a sensitive directory " f"(e.g. .ssh, .gnupg) or matches a sensitive filename" ) + if _is_app_state_path(resolved): + raise ValueError( + f"path '{raw_path}' is inside the application state directory" + ) if resolved != base: # normcase so containment holds on case-insensitive filesystems # (Windows, default macOS): it lowercases on Windows and is a no-op on @@ -277,6 +373,10 @@ def vet_workspace(raw: str) -> Optional[str]: resolved = os.path.realpath(os.path.expanduser(raw)) if not os.path.isdir(resolved) or _is_sensitive_path(resolved): return None + # Refuse the bind rather than binding a workspace where every subsequent + # tool call would fail on the same deny list. + if _is_app_state_path(resolved): + return None # Reject filesystem roots: binding / (or a Windows drive/UNC root) as the # workspace would make every absolute path "inside" it, collapsing the # confinement into host-wide file access. A root is its own dirname, which @@ -304,13 +404,16 @@ def _resolve_search_root(raw_path: str) -> str: With a workspace active, the workspace folder is the root and a supplied path is confined inside it. Otherwise an empty path defaults to the agent's - primary root (project data dir) and a supplied path is confined by the - global allowlist + sensitive-file policy. + primary root (its workspace under the project data dir) and a supplied path + is confined by the global allowlist + sensitive-file policy. """ raw = (raw_path or "").strip() ws = get_active_workspace() if ws: - return os.path.realpath(ws) if not raw else _resolve_tool_path_in_workspace(ws, raw) + # Resolve the empty case as the workspace path rather than returning + # it directly: returned unchecked it skipped both deny lists, so a + # bare ls listed whatever the workspace was bound to. + return _resolve_tool_path_in_workspace(ws, raw or ws) if not raw: roots = _tool_path_roots() return roots[0] if roots else os.path.realpath(".") diff --git a/tests/test_agent_state_dir_confinement.py b/tests/test_agent_state_dir_confinement.py new file mode 100644 index 000000000..5bf093779 --- /dev/null +++ b/tests/test_agent_state_dir_confinement.py @@ -0,0 +1,225 @@ +"""The agent's file tools must not reach the application's own state. + +read_file / grep / glob / ls resolve model-supplied paths against +_tool_path_roots(), and the data directory holds the session store, the +credential database, the encryption key and the settings file. A read tool +pointed at those is a credential disclosure, and no approval prompt stands in +the way because reads are classified read_workspace and pass the untrusted- +context gate untouched. + +The agent gets its own subdirectory instead. Three routes have to close +together, because closing only the first leaves the other two working: + + - the default roots, which put DATA_DIR first + - an active workspace bound at (or above) the data directory + - a tool_path_extra_roots setting that covers the data directory + +so the guard is a property of the path, not of the root it arrived through. +""" + +import os +from contextlib import contextmanager + +import pytest + +from src.constants import ( + AGENT_WORKSPACE_DIR, + DATA_DIR, + MAIL_ATTACHMENTS_DIR, + PERSONAL_DIR, + PERSONAL_UPLOADS_DIR, + RUNBOOK_DIR, + UPLOAD_DIR, +) +from src.tool_execution import ( + _active_workspace, + _resolve_search_root, + _resolve_tool_path, + agent_cwd, + vet_workspace, +) + +APP_STATE_FILES = [ + "sessions.json", # session token -> username, cleartext + "auth.json", # bcrypt hashes, admin flags, privileges + "app.db", # every user's notes, documents, mail rows + ".app_key", # Fernet key for secret_storage + "settings.json", # provider API keys +] + + +@contextmanager +def workspace_at(path): + """Bind an active workspace for the body of a test. + + Set and reset in the same context; a ContextVar token cannot be reset from + fixture teardown, which runs in a different one. + """ + token = _active_workspace.set(os.path.realpath(path)) + try: + yield + finally: + _active_workspace.reset(token) + + +# ── The default roots ──────────────────────────────────────────────── + +@pytest.mark.parametrize("name", APP_STATE_FILES) +def test_blocks_app_state_file(name): + with pytest.raises(ValueError, match="application state"): + _resolve_tool_path(os.path.join(DATA_DIR, name)) + + +def test_blocks_listing_the_data_directory_itself(): + """`ls data` enumerated the state files, which is how an attacker who + does not know the install path finds them.""" + with pytest.raises(ValueError, match="application state"): + _resolve_tool_path(DATA_DIR) + + +def test_blocks_app_state_reached_by_relative_path(monkeypatch): + """The data directory is a relative hop from the checkout root, so + confinement cannot depend on the model supplying an absolute path.""" + monkeypatch.chdir(os.path.dirname(DATA_DIR)) + with pytest.raises(ValueError, match="application state"): + _resolve_tool_path(os.path.join(os.path.basename(DATA_DIR), "sessions.json")) + + +def test_blocks_app_state_reached_through_a_symlink(tmp_path): + """/tmp is an allowed root and the agent can create links there in an + un-armed turn, so containment has to survive one.""" + link = tmp_path / "shortcut" + try: + link.symlink_to(DATA_DIR) + except OSError: + pytest.skip("cannot create symlink") + with pytest.raises(ValueError, match="application state"): + _resolve_tool_path(str(link / "sessions.json")) + + +def test_blocks_app_state_on_a_case_insensitive_filesystem(): + """On default macOS a case-variant path opens the same file, and realpath + does not canonicalise case there the way it does on Windows. + + This deny rule fails OPEN when containment misses, unlike the allowlist + beside it, which fails closed. So it folds case, for the same reason + _is_sensitive_path does and not with normcase, which is a no-op on POSIX. + """ + shouty = os.path.join(DATA_DIR.upper(), "SESSIONS.JSON") + with pytest.raises(ValueError, match="application state"): + _resolve_tool_path(shouty) + + +def test_default_search_root_is_the_agent_workspace(): + """grep/glob/ls with no path fall back to roots[0]. That was DATA_DIR.""" + assert _resolve_search_root("") == os.path.realpath(AGENT_WORKSPACE_DIR) + + +def test_agent_workspace_is_inside_the_data_directory(): + """It has to stay under data/ to be covered by the Docker bind mount, + so the guard cannot simply be 'anything under DATA_DIR is denied'.""" + assert os.path.realpath(AGENT_WORKSPACE_DIR).startswith( + os.path.realpath(DATA_DIR) + os.sep + ) + + +# ── An active workspace ────────────────────────────────────────────── + +def test_workspace_bound_at_the_data_directory_still_blocks_app_state(): + """vet_workspace() accepts the data directory, and chat_routes auto-binds + a workspace from a path named in the message, so this is reachable.""" + with workspace_at(DATA_DIR): + with pytest.raises(ValueError, match="application state"): + _resolve_tool_path("sessions.json") + + +def test_workspace_bound_above_the_data_directory_still_blocks_app_state(): + with workspace_at(os.path.dirname(DATA_DIR)): + with pytest.raises(ValueError, match="application state"): + _resolve_tool_path(os.path.join(DATA_DIR, "sessions.json")) + + +def test_workspace_bound_at_the_data_directory_refuses_the_empty_search_root(): + """grep/glob/ls with no path take the workspace itself as the root, which + skipped the in-workspace resolver and enumerated the state directory.""" + with workspace_at(DATA_DIR): + with pytest.raises(ValueError, match="application state"): + _resolve_search_root("") + + +def test_vet_workspace_refuses_the_data_directory(): + """Rejecting the bind is the cleaner failure: the client is told the + workspace was refused instead of every tool call erroring separately.""" + assert vet_workspace(DATA_DIR) is None + + +def test_vet_workspace_accepts_the_agent_workspace(): + os.makedirs(AGENT_WORKSPACE_DIR, exist_ok=True) + assert vet_workspace(AGENT_WORKSPACE_DIR) == os.path.realpath(AGENT_WORKSPACE_DIR) + + +def test_workspace_bound_at_the_data_directory_still_allows_the_agent_workspace(): + with workspace_at(DATA_DIR): + resolved = _resolve_tool_path(os.path.join("agent_workspace", "notes.txt")) + assert resolved == os.path.realpath(os.path.join(AGENT_WORKSPACE_DIR, "notes.txt")) + + +# ── An opt-in extra root ───────────────────────────────────────────── + +def test_extra_root_covering_the_data_directory_still_blocks_app_state(monkeypatch): + monkeypatch.setattr( + "src.settings.get_setting", lambda *_a, **_k: [os.path.dirname(DATA_DIR)] + ) + with pytest.raises(ValueError, match="application state"): + _resolve_tool_path(os.path.join(DATA_DIR, "sessions.json")) + + +# ── What the agent keeps ───────────────────────────────────────────── + +def test_allows_files_in_the_agent_workspace(): + resolved = _resolve_tool_path(os.path.join(AGENT_WORKSPACE_DIR, "scratch.txt")) + assert resolved == os.path.realpath(os.path.join(AGENT_WORKSPACE_DIR, "scratch.txt")) + + +@pytest.mark.parametrize("directory, why", [ + (UPLOAD_DIR, + "_uploaded_files_context_message emits path= and tells the model to " + "read it with read_file (src/agent_loop.py)"), + (MAIL_ATTACHMENTS_DIR, + "download_attachment returns the path and its description says to read " + "it with read_file (mcp_servers/email_server.py)"), + (PERSONAL_DIR, + "GET /api/personal returns a path per file and is reachable through the " + "app_api tool, which does not block that prefix"), + (PERSONAL_UPLOADS_DIR, + "indexed into personal docs by routes/personal_routes.py, and listed as " + "an absolute path by manage_rag"), +]) +def test_allows_user_content_the_app_hands_to_the_model(directory, why): + """Carving these out is not convenience. The app gives the model these + paths and tells it to read them, so denying them breaks the feature.""" + target = os.path.join(directory, "example.txt") + assert _resolve_tool_path(target) == os.path.realpath(target), why + + +def test_runbook_is_covered_by_the_personal_docs_carve_out(): + """RUNBOOK_DIR nests under PERSONAL_DIR, so it needs no entry of its own.""" + target = os.path.join(RUNBOOK_DIR, "notes.md") + assert _resolve_tool_path(target) == os.path.realpath(target) + + +def test_allows_tmp(): + """Unchanged: /tmp is still a root and holds no application state.""" + assert _resolve_tool_path("/tmp/scratch.txt") == os.path.realpath("/tmp/scratch.txt") + + +def test_subprocess_cwd_is_the_agent_workspace(): + """bash/python cwd has to move with the file root, or the agent writes + where read_file can no longer look.""" + assert agent_cwd() == os.path.realpath(AGENT_WORKSPACE_DIR) + + +def test_sensitive_deny_list_still_fires_inside_the_agent_workspace(): + """The new guard is layered on the existing one, not a replacement.""" + with pytest.raises(ValueError, match="sensitive directory"): + _resolve_tool_path(os.path.join(AGENT_WORKSPACE_DIR, "id_rsa")) diff --git a/tests/test_tool_path_confinement.py b/tests/test_tool_path_confinement.py index c23c99750..410081fbf 100644 --- a/tests/test_tool_path_confinement.py +++ b/tests/test_tool_path_confinement.py @@ -161,12 +161,14 @@ def test_blocks_netrc(): _resolve_tool_path("~/.netrc") -def test_allows_project_data(tmp_path): - """Paths under project data/ must resolve cleanly.""" +def test_allows_agent_workspace(tmp_path): + """Paths under the agent's workspace in project data/ must resolve + cleanly. The rest of data/ is application state and is rejected; + tests/test_agent_state_dir_confinement.py covers that side.""" from src.tool_execution import _resolve_tool_path - from src.constants import DATA_DIR - target = os.path.join(DATA_DIR, "test-confinement-ok.txt") - os.makedirs(DATA_DIR, exist_ok=True) + from src.constants import AGENT_WORKSPACE_DIR + target = os.path.join(AGENT_WORKSPACE_DIR, "test-confinement-ok.txt") + os.makedirs(AGENT_WORKSPACE_DIR, exist_ok=True) with open(target, "w") as f: f.write("ok") try: