diff --git a/nerve/agent/backends/base.py b/nerve/agent/backends/base.py index bad960a0..5d0d2112 100644 --- a/nerve/agent/backends/base.py +++ b/nerve/agent/backends/base.py @@ -53,7 +53,19 @@ class TurnInput: @dataclass class SessionSpec: - """Everything a backend needs to build a client for one session.""" + """Everything a backend needs to build a client for one session. + + **``system_prompt`` never goes on a command line.** It carries the + instance's private context — the operator's identity and memory files + plus TOOLS.md, which indexes where the host's credentials live — and + argv is world-readable: one unprivileged ``ps`` reads it out of every + running session at once. Backends must hand it to their runtime out + of band. Claude writes it to a 0600 file and passes the path + (``--system-prompt-file``); Codex sends it as ``developerInstructions`` + inside the app-server JSON payload. A new backend that puts it in an + argument is a data leak, not a style choice — + ``tests/test_system_prompt_transport.py`` guards the invariant. + """ session_id: str source: str diff --git a/nerve/agent/backends/claude.py b/nerve/agent/backends/claude.py index e8578dc4..cee8eebf 100644 --- a/nerve/agent/backends/claude.py +++ b/nerve/agent/backends/claude.py @@ -25,6 +25,9 @@ import logging import os import re +import stat +import time +from pathlib import Path from typing import Any, AsyncIterator from claude_agent_sdk import ( @@ -63,6 +66,7 @@ InteractiveToolHandler, _read_file_safe, ) +from nerve.utils.fs import atomic_write_text logger = logging.getLogger(__name__) @@ -71,21 +75,36 @@ except ImportError: # pragma: no cover - depends on SDK version ThinkingBlock = None -# Linux execve() limits a single argv element to MAX_ARG_STRLEN = PAGE_SIZE * 32 -# = 131,072 bytes on common configurations. The Claude Agent SDK passes the -# system prompt inline as `--system-prompt `, which makes the string a -# single argv element. When SOUL.md + TASK.md + AGENTS.md + TOOLS.md + -# MEMORY.md + recalled memU summaries cross that boundary, execve() returns -# E2BIG ("Argument list too long") and Claude Code fails to start. +# The system prompt is confidential, so it never travels in argv. # -# We sidestep the limit by writing the prompt to a file and passing -# `SystemPromptFile = {"type": "file", "path": ...}` (which the SDK converts -# to `--system-prompt-file ` — the path string is short). +# What Nerve assembles into it is the instance's whole private context: +# SOUL.md / IDENTITY.md / USER.md (the operator's name, addresses, health +# notes), AGENTS.md and TOOLS.md (which is an index of where every +# credential on the host lives), MEMORY.md, and the recalled-memory block. +# The SDK's default transport is `--system-prompt `, and argv is +# world-readable on every OS Nerve runs on: any process under any uid can +# `ps -ww` the lot out of a running session, for every session at once, +# unprivileged and untraced. Every MCP server Nerve spawns and every +# command an agent shells out to is such a process. # -# Threshold below which we keep passing inline (preserves prompt-cache hit -# behavior for small, stable prompts). Set conservatively well under the -# kernel limit to leave room for env/argv overhead. -SYSTEM_PROMPT_INLINE_MAX = 100_000 # bytes +# So `_build_options` always passes `SystemPromptFile = {"type": "file", +# "path": ...}` — the SDK turns that into `--system-prompt-file `, +# and only the short path reaches the process table. The file itself is +# written 0600 in a 0700 directory (see ``_write_system_prompt_file``). +# There is no size threshold and no inline branch: a small prompt is not +# less secret than a large one, and prompt-cache behavior is keyed on the +# prompt's *content*, not on how it reached the CLI. +# +# Two consequences worth naming, both of which used to be the whole +# rationale for this path: +# +# * execve() caps a single argv element at MAX_ARG_STRLEN (PAGE_SIZE * 32 +# = 131,072 bytes on common Linux configs). A full bundle crosses that +# and the CLI fails to start with E2BIG. Off argv, it cannot happen. +# * Workspace-file text in argv makes every string in SOUL/TOOLS/MEMORY a +# `pkill -f` pattern that matches *every* concurrent session on the +# box. Off argv, an ordinary cleanup command stops being a fleet-wide +# kill switch. # CLI env vars that remap model *aliases* (the short names accepted by the # Agent/Workflow tools' model option, skill frontmatter, `--model opus`, @@ -441,20 +460,14 @@ def _build_options(self, spec: SessionSpec) -> ClaudeAgentOptions: config = self.config session_id = spec.session_id - # Pass the system prompt as a file when it's large enough to risk - # hitting Linux's MAX_ARG_STRLEN argv-element limit (see the - # SYSTEM_PROMPT_INLINE_MAX comment above). - system_prompt: str | dict[str, Any] - if len(spec.system_prompt) > SYSTEM_PROMPT_INLINE_MAX: - sp_path = self._write_system_prompt_file(session_id, spec.system_prompt) - system_prompt = {"type": "file", "path": sp_path} - logger.info( - "Session %s: system prompt %d bytes (> %d), passing via file %s", - session_id[:8], len(spec.system_prompt), - SYSTEM_PROMPT_INLINE_MAX, sp_path, - ) - else: - system_prompt = spec.system_prompt + # Always via file, never inline — argv is world-readable and this + # prompt is the instance's private context (see the module comment). + sp_path = self._write_system_prompt_file(session_id, spec.system_prompt) + system_prompt: dict[str, Any] = {"type": "file", "path": sp_path} + logger.debug( + "Session %s: system prompt %d bytes passed via file %s", + session_id[:8], len(spec.system_prompt), sp_path, + ) # Local Ollama models are reached through the proxy and speak the # OpenAI-translated API — Anthropic-only knobs (extended thinking, @@ -565,39 +578,76 @@ def _cli_stderr(line: str) -> None: plugins=self._deps.claude_plugins(), ) - def _system_prompt_dir(self) -> "os.PathLike[str]": - """Directory where oversized system prompts are spilled to disk.""" - from pathlib import Path + def _system_prompt_dir(self) -> Path: + """Owner-only directory holding the per-session system prompts. + + ``mkdir(exist_ok=True)`` does not touch the mode of a directory + that already exists, and the pre-hardening code created this one + at the umask default (0755 on most hosts) — so the chmod is + unconditional, not just part of the create. That is the upgrade + path for every install that ran the old code; without it the dir + stays world-traversable forever. + """ d = Path(self.config.workspace) / ".nerve" / "cache" / "system_prompts" - d.mkdir(parents=True, exist_ok=True) + d.mkdir(parents=True, exist_ok=True, mode=0o700) + try: + os.chmod(d, 0o700) + except OSError as e: # read-only FS, or someone else owns it + logger.warning("Could not restrict %s to 0700: %s", d, e) return d def _write_system_prompt_file(self, session_id: str, content: str) -> str: - """Write the system prompt to disk and return its absolute path. + """Write the system prompt to disk 0600 and return its path. Deterministic filename so a session that reconnects (resume) gets - the same prompt without re-writing. Lazy GC of stale files (>7d). + the same prompt without re-writing. The write is atomic and the + mode is in place *before* the content is, so the bundle is never + briefly readable at the process umask — same discipline the Codex + backend uses for the files it generates. """ - import time - from pathlib import Path + dir_path = self._system_prompt_dir() + self._sweep_system_prompt_dir(dir_path) - dir_path = Path(self._system_prompt_dir()) + safe_id = re.sub(r"[^A-Za-z0-9_.-]", "_", session_id)[:120] + path = dir_path / f"{safe_id}.md" + atomic_write_text(path, content, mode=0o600) + return str(path) + @staticmethod + def _sweep_system_prompt_dir(dir_path: Path) -> None: + """Drop spills older than 7d; clamp whatever is left to 0600. + + Two jobs, one pass over the directory. The GC is what keeps a + long-lived workspace from accumulating a prompt file per session + forever. The clamp is the upgrade path for files the + pre-hardening code already wrote at 0644: they hold the full + identity bundle and would otherwise sit there world-readable + until they aged out. + + Best-effort throughout — a file that cannot be removed or + chmod'd (foreign owner, read-only mount) must not stop a session + from starting. + + ``lstat``, not ``stat``: a symlink here is skipped rather than + followed, so neither the unlink nor the chmod can be aimed at a + file outside this directory by planting one. + """ cutoff = time.time() - 7 * 24 * 3600 try: - for old in dir_path.iterdir(): - try: - if old.is_file() and old.stat().st_mtime < cutoff: - old.unlink() - except OSError: - pass + entries = list(dir_path.iterdir()) except OSError: - pass - - safe_id = re.sub(r"[^A-Za-z0-9_.-]", "_", session_id)[:120] - path = dir_path / f"{safe_id}.md" - path.write_text(content, encoding="utf-8") - return str(path) + return + for old in entries: + try: + st = old.lstat() + if not stat.S_ISREG(st.st_mode): + continue + if st.st_mtime < cutoff: + old.unlink() + elif st.st_mode & 0o077: + os.chmod(old, 0o600) + except OSError: + pass def _build_env(self, cache_ttl: str = "5m") -> dict[str, str]: """Build environment variables for the SDK subprocess.""" diff --git a/tests/test_engine.py b/tests/test_engine.py index af135399..8d0e49e4 100644 --- a/tests/test_engine.py +++ b/tests/test_engine.py @@ -4,6 +4,7 @@ import asyncio import json import os +from pathlib import Path from types import SimpleNamespace from unittest.mock import AsyncMock, patch @@ -135,9 +136,12 @@ def test_claude_system_prompt_excludes_codex_runbook_policy(tmp_path): patch.object(backend, "_build_hooks", return_value={}): options = backend._build_options(spec) - assert options.system_prompt == marker - assert "Nerve runbooks" not in str(options.system_prompt) - assert "Codex-native skills" not in str(options.system_prompt) + # The prompt travels by file (never argv), so the exactness check is on + # what landed on disk. + rendered = Path(options.system_prompt["path"]).read_text(encoding="utf-8") + assert rendered == marker + assert "Nerve runbooks" not in rendered + assert "Codex-native skills" not in rendered # --------------------------------------------------------------------------- diff --git a/tests/test_system_prompt_transport.py b/tests/test_system_prompt_transport.py new file mode 100644 index 00000000..6ccc0775 --- /dev/null +++ b/tests/test_system_prompt_transport.py @@ -0,0 +1,280 @@ +"""The system prompt must never reach a command line, at any size. + +Nerve assembles the operator's identity and memory files plus TOOLS.md — +an index of where the host's credentials live — into every session's +system prompt. argv is world-readable: a single unprivileged ``ps -ww`` +reads it out of every running session at once. So the prompt travels out +of band, and the file it travels in is a secret's file: 0600, in a 0700 +directory, written atomically so the mode is in place before the bytes +are. + +These tests pin the transport itself (through the SDK's own argv builder, +not a shape assertion on our options object), the permissions, and the +upgrade path for installs that already spilled prompts at 0644. +""" + +import os +import time +from pathlib import Path +from types import SimpleNamespace +from unittest.mock import patch + +import pytest + +from nerve.agent.backends.base import SessionSpec +from nerve.agent.backends.claude import ClaudeBackend +from nerve.config import NerveConfig + +# A string that appears nowhere else, standing in for the real bundle. +MARKER = "SENTINEL-a-credential-index-and-a-home-address" + + +def _backend(tmp_path: Path) -> ClaudeBackend: + cfg = NerveConfig.from_dict({"workspace": str(tmp_path)}) + return ClaudeBackend(SimpleNamespace( + config=lambda: cfg, + claude_plugins=lambda: [], + )) + + +def _spec(tmp_path: Path, prompt: str, session_id: str = "sess-1234") -> SessionSpec: + return SessionSpec( + session_id=session_id, + source="web", + model="claude-opus-5", + effort="high", + system_prompt=prompt, + cwd=str(tmp_path), + ) + + +def _build(backend: ClaudeBackend, spec: SessionSpec): + with patch.object(backend, "_build_mcp_servers", return_value={}), \ + patch.object(backend, "_build_hooks", return_value={}): + return backend._build_options(spec) + + +def _prompt_dir(tmp_path: Path) -> Path: + return tmp_path / ".nerve" / "cache" / "system_prompts" + + +# --------------------------------------------------------------------- # +# Transport # +# --------------------------------------------------------------------- # + + +@pytest.mark.parametrize("size", [0, 1, 500, 100_000, 150_000]) +def test_prompt_never_inline_at_any_size(tmp_path, size): + """No threshold, no inline branch — every size takes the file path. + + The old code inlined anything under 100 KB, which put the whole + bundle in argv for most sessions and made which-leak-you-get a + function of incidental prompt drift. + """ + prompt = MARKER + "x" * size + options = _build(_backend(tmp_path), _spec(tmp_path, prompt)) + + assert isinstance(options.system_prompt, dict) + assert options.system_prompt["type"] == "file" + assert Path(options.system_prompt["path"]).read_text(encoding="utf-8") == prompt + + +def test_sdk_argv_carries_the_path_not_the_prompt(tmp_path): + """End-to-end through the SDK's own argv builder. + + Asserting the shape of our options dict only proves we asked for the + file transport. This proves the SDK honors it — that the bytes the + kernel sees are a path, and that nothing else in the options assembly + smuggles the prompt onto the command line by another route. + """ + from claude_agent_sdk._internal.transport.subprocess_cli import ( + SubprocessCLITransport, + ) + + prompt = MARKER + "\n" + "line of private context\n" * 5000 + backend = _backend(tmp_path) + options = _build(backend, _spec(tmp_path, prompt)) + options.cli_path = "/nonexistent/claude" # skip the CLI discovery walk + + argv = SubprocessCLITransport(prompt="hi", options=options)._build_command() + + assert "--system-prompt-file" in argv + assert "--system-prompt" not in argv + # The prompt — or any fragment of it — must appear in no argument. + joined = "\x00".join(argv) + assert MARKER not in joined + assert "line of private context" not in joined + # And the whole command line stays small: a path, not a bundle. + assert len(joined) < 4096 < len(prompt) + + +def test_codex_backend_keeps_the_prompt_out_of_argv(tmp_path): + """The other backend satisfies the same invariant, differently. + + Codex sends the prompt as ``developerInstructions`` inside the + app-server JSON payload. Pinned here so the two backends cannot + silently drift apart on whether the prompt is a secret. + """ + from nerve.agent.backends.codex.backend import CodexBackend + + cfg = NerveConfig.from_dict({"workspace": str(tmp_path)}) + backend = CodexBackend(SimpleNamespace(config=lambda: cfg)) + params = backend.thread_params(_spec(tmp_path, MARKER)) + + assert MARKER in params["developerInstructions"] + # Everything else in the payload is a scalar knob; none of it is argv, + # and none of it repeats the prompt. + assert not any( + MARKER in str(v) for k, v in params.items() if k != "developerInstructions" + ) + + +# --------------------------------------------------------------------- # +# Permissions # +# --------------------------------------------------------------------- # + + +def test_spill_file_and_dir_are_owner_only(tmp_path): + backend = _backend(tmp_path) + options = _build(backend, _spec(tmp_path, MARKER)) + + path = Path(options.system_prompt["path"]) + assert path.stat().st_mode & 0o777 == 0o600 + assert path.parent.stat().st_mode & 0o777 == 0o700 + + +def test_mode_survives_a_permissive_umask(tmp_path): + """0600 by construction, not by inheriting a strict umask. + + ``open(path, "w")`` under ``umask 000`` yields 0666. If the mode came + from the umask this test would fail on exactly the hosts where it + matters most. + """ + backend = _backend(tmp_path) + old = os.umask(0o000) + try: + options = _build(backend, _spec(tmp_path, MARKER)) + finally: + os.umask(old) + + assert Path(options.system_prompt["path"]).stat().st_mode & 0o777 == 0o600 + + +def test_write_leaves_no_readable_debris(tmp_path): + """The atomic write must not leave its temp file behind. + + A stray temp file is the same leak under a different name. + """ + backend = _backend(tmp_path) + _build(backend, _spec(tmp_path, MARKER)) + _build(backend, _spec(tmp_path, MARKER + " turn two")) + + files = list(_prompt_dir(tmp_path).iterdir()) + assert len(files) == 1 + assert files[0].suffix == ".md" + assert all(f.stat().st_mode & 0o077 == 0 for f in files) + + +# --------------------------------------------------------------------- # +# Upgrade path — what earlier versions left on disk # +# --------------------------------------------------------------------- # + + +def test_existing_world_readable_dir_is_clamped(tmp_path): + """``mkdir(exist_ok=True)`` does not fix the mode of an existing dir. + + Every install that ran the pre-hardening code has this directory at + 0755. Re-creating it is a no-op; the chmod has to be unconditional. + """ + d = _prompt_dir(tmp_path) + d.mkdir(parents=True) + os.chmod(d, 0o755) + + _build(_backend(tmp_path), _spec(tmp_path, MARKER)) + + assert d.stat().st_mode & 0o777 == 0o700 + + +def test_existing_0644_spills_are_clamped(tmp_path): + """Old spills hold the full bundle and must not wait for GC to age out.""" + d = _prompt_dir(tmp_path) + d.mkdir(parents=True) + stale = d / "old-session.md" + stale.write_text("previously world-readable bundle", encoding="utf-8") + os.chmod(stale, 0o644) + + _build(_backend(tmp_path), _spec(tmp_path, MARKER)) + + assert stale.stat().st_mode & 0o777 == 0o600 + # Untouched otherwise — this is a permission fix, not a purge. + assert stale.read_text(encoding="utf-8") == "previously world-readable bundle" + + +def test_unclampable_file_does_not_block_a_session(tmp_path): + """Best-effort: a file we cannot chmod must not stop the CLI starting.""" + d = _prompt_dir(tmp_path) + d.mkdir(parents=True) + (d / "foreign.md").write_text("not ours", encoding="utf-8") + + with patch("nerve.agent.backends.claude.os.chmod", side_effect=OSError("EPERM")): + options = _build(_backend(tmp_path), _spec(tmp_path, MARKER)) + + assert Path(options.system_prompt["path"]).exists() + + +# --------------------------------------------------------------------- # +# Filename discipline and GC (unchanged behavior, still load-bearing) # +# --------------------------------------------------------------------- # + + +def test_filename_is_deterministic_across_reconnects(tmp_path): + """Resume re-uses the same path, and the content is refreshed.""" + backend = _backend(tmp_path) + first = _build(backend, _spec(tmp_path, MARKER, "sess-resume")) + second = _build(backend, _spec(tmp_path, MARKER + " v2", "sess-resume")) + + assert first.system_prompt["path"] == second.system_prompt["path"] + assert Path(second.system_prompt["path"]).read_text(encoding="utf-8").endswith("v2") + + +def test_session_id_cannot_escape_the_directory(tmp_path): + """A path-shaped session id is sanitized, not honored.""" + backend = _backend(tmp_path) + options = _build(backend, _spec(tmp_path, MARKER, "../../../../tmp/evil")) + + path = Path(options.system_prompt["path"]) + assert path.parent == _prompt_dir(tmp_path) + assert not Path("/tmp/evil.md").exists() + + +def test_sweep_does_not_follow_symlinks(tmp_path): + """A symlink in the dir must not aim the chmod (or the unlink) elsewhere.""" + d = _prompt_dir(tmp_path) + d.mkdir(parents=True) + outside = tmp_path / "someone-elses-file" + outside.write_text("not a system prompt", encoding="utf-8") + os.chmod(outside, 0o644) + link = d / "link.md" + link.symlink_to(outside) + os.utime(link, (time.time() - 8 * 24 * 3600,) * 2, follow_symlinks=False) + + _build(_backend(tmp_path), _spec(tmp_path, MARKER)) + + assert outside.exists() + assert outside.stat().st_mode & 0o777 == 0o644 + assert link.is_symlink() + + +def test_gc_prunes_stale_spills_and_keeps_fresh_ones(tmp_path): + d = _prompt_dir(tmp_path) + d.mkdir(parents=True) + old, fresh = d / "old.md", d / "fresh.md" + for f in (old, fresh): + f.write_text("x", encoding="utf-8") + eight_days = time.time() - 8 * 24 * 3600 + os.utime(old, (eight_days, eight_days)) + + _build(_backend(tmp_path), _spec(tmp_path, MARKER)) + + assert not old.exists() + assert fresh.exists()