diff --git a/CLAUDE.md b/CLAUDE.md index 6b2e53cfc..4facfb55f 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -322,8 +322,8 @@ curl -X POST http://localhost:8000/api/agents \ ### Credential Pattern ``` .env # Source of truth (KEY=VALUE) -.mcp.json.template # Template with ${VAR} placeholders -.mcp.json # Generated at runtime +.mcp.json.template # Template with ${VAR} placeholders (rendering is #2007, not yet live) +.mcp.json # Written by credential injection ``` --- diff --git a/docker/base-image/agent_server/models.py b/docker/base-image/agent_server/models.py index 13f264143..68cc21668 100644 --- a/docker/base-image/agent_server/models.py +++ b/docker/base-image/agent_server/models.py @@ -40,13 +40,6 @@ class ModelRequest(BaseModel): # Credential Models # ============================================================================ -class CredentialUpdateRequest(BaseModel): - credentials: dict # {"VAR_NAME": "value", ...} - mcp_config: Optional[str] = None # Pre-generated .mcp.json content (if provided) - files: Optional[Dict[str, str]] = None # File-type credentials: {"path": "content", ...} - files_b64: Optional[Dict[str, str]] = None # Binary file creds: {"path": base64(content)} (#11) - - # ============================================================================ # Agent Info Models # ============================================================================ diff --git a/docker/base-image/agent_server/routers/credentials.py b/docker/base-image/agent_server/routers/credentials.py index 9f41eb750..785ab6f6b 100644 --- a/docker/base-image/agent_server/routers/credentials.py +++ b/docker/base-image/agent_server/routers/credentials.py @@ -11,7 +11,6 @@ from fastapi import APIRouter, HTTPException, Query from ..models import ( - CredentialUpdateRequest, CredentialReadRequest, CredentialReadResponse, CredentialInjectRequest, @@ -76,105 +75,6 @@ def _write_credential_file(rel_path: str, *, text: str = None, b64: str = None) router = APIRouter() -@router.post("/api/credentials/update") -async def update_credentials(request: CredentialUpdateRequest): - """ - Update agent credentials by writing .env and regenerating .mcp.json. - - This endpoint is called by the Trinity backend when credentials are updated. - It writes the new credentials to files that MCP servers read at startup/runtime. - - Flow: - 1. Write credentials to /home/developer/.env - 2. If mcp_config provided, write to /home/developer/.mcp.json - 3. If .mcp.json.template exists, generate .mcp.json from it using envsubst - """ - home_dir = Path("/home/developer") - env_file = home_dir / ".env" - mcp_file = home_dir / ".mcp.json" - mcp_template = home_dir / ".mcp.json.template" - - updated_files = [] - - try: - # 1. Write .env file - env_lines = ["# Generated by Trinity - Agent credentials", ""] - for var_name, value in request.credentials.items(): - # Escape special characters in values - escaped_value = str(value).replace('"', '\\"') - env_lines.append(f'{var_name}="{escaped_value}"') - - env_content = "\n".join(env_lines) + "\n" - env_file.write_text(env_content) - updated_files.append(str(env_file)) - logger.info(f"Updated .env with {len(request.credentials)} credentials") - - # 2. Handle .mcp.json generation - if request.mcp_config: - # If backend provides pre-generated .mcp.json, use it - mcp_file.write_text(request.mcp_config) - updated_files.append(str(mcp_file)) - logger.info("Updated .mcp.json from provided config") - - # Re-inject Trinity MCP after updating .mcp.json - if inject_trinity_mcp_if_configured(): - logger.info("Re-injected Trinity MCP after credential reload") - - elif mcp_template.exists(): - # Generate .mcp.json from template using envsubst-style substitution - template_content = mcp_template.read_text() - - # Perform variable substitution (${VAR_NAME} -> value) - generated_content = template_content - for var_name, value in request.credentials.items(): - placeholder = f"${{{var_name}}}" - generated_content = generated_content.replace(placeholder, str(value)) - - mcp_file.write_text(generated_content) - updated_files.append(str(mcp_file)) - logger.info("Generated .mcp.json from template") - - # Re-inject Trinity MCP after regenerating .mcp.json - # This uses the same injection logic as agent startup - if inject_trinity_mcp_if_configured(): - logger.info("Re-injected Trinity MCP after credential reload") - - # 3. Re-point this process's env at the .env just written (#1999). - # Spawned executions no longer read this — `build_execution_env` parses - # the file per spawn — but in-process readers (error classifier, - # AGENT_RUNTIME, the sanitizer's redaction set) still do. Unlike the - # loop this replaces, it also REMOVES keys the new file dropped, so a - # re-push of a cleaned .env clears a deleted key instead of leaving it - # applied to every future execution. - sync_process_env(env_file) - - # SECURITY: Refresh credential sanitizer cache after updating credentials - refresh_credential_values() - - # 4. Write file-type credentials (e.g., service account JSON files). - # Policy-checked + traversal-guarded via the shared helper (#11) — the - # original loop wrote arbitrary paths with no allowlist or `..` guard. - files_written = [] - if request.files: - for file_path, content in request.files.items(): - files_written.append(_write_credential_file(file_path, text=content)) - if request.files_b64: - for file_path, b64 in request.files_b64.items(): - files_written.append(_write_credential_file(file_path, b64=b64)) - - return { - "status": "success", - "updated_files": updated_files + files_written, - "credential_count": len(request.credentials), - "files_written": files_written, - "note": "MCP servers may need to be restarted to pick up new credentials" - } - - except Exception as e: - logger.error(f"Failed to update credentials: {e}") - raise HTTPException(status_code=500, detail=f"Credential update failed: {str(e)}") - - # Writable-layer override path (#1089). Deliberately NOT under /home/developer — # that path is the persistent agent-{name}-workspace volume which # `recreate_container_with_updated_config` preserves, so a token written there diff --git a/docker/base-image/agent_server/routers/files.py b/docker/base-image/agent_server/routers/files.py index 0673e0824..dbd8f90ab 100644 --- a/docker/base-image/agent_server/routers/files.py +++ b/docker/base-image/agent_server/routers/files.py @@ -179,10 +179,22 @@ async def download_file(path: str): # # .mcp.json and .mcp.json.template were historically editable here because # "users need to modify them" — but raw editing of either is RCE-by-config: -# tool `command:` fields run as the agent process. Owners modify MCP servers -# at agent-creation time via the template, or via the platform-internal -# /api/credentials/update flow which regenerates .mcp.json from the template -# with envsubst (no arbitrary content). See #590 (AISEC-C2). +# tool `command:` fields run as the agent process. See #590 (AISEC-C2). +# +# The sanctioned path (#2008 — this comment previously named +# `/api/credentials/update`, which had no callers anywhere and was removed; +# both blocks below were justified by a route that did not run): +# * `POST /api/agents/{name}/credentials/inject`, gated by +# `validate_mcp_config` — Layer 2 of the #590 closure (#598). Real today. +# +# PLANNED, not yet available: rendering `.mcp.json.template` into `.mcp.json` +# at container startup (`${VAR}` substituted inside `env` only, each server +# validated) — #2007 / PR #2013. Deleting `update_credentials` removed the only +# code that ever rendered the template, `startup.sh` performs no substitution, +# and `credential_paths.py` denies the template on the inject path — so until +# #2013 lands, declaring a server in the template alone never reaches +# `.mcp.json`. Written as planned rather than present so this comment does not +# replace one forward reference with another. # # CLAUDE.md is intentionally NOT here — owners do edit their agent's # instructions directly. diff --git a/docs/diagrams/03-agent-container.md b/docs/diagrams/03-agent-container.md index 7bb468d88..c08b78bc9 100644 --- a/docs/diagrams/03-agent-container.md +++ b/docs/diagrams/03-agent-container.md @@ -75,7 +75,7 @@ This document details the internal architecture of a Trinity agent container - t | | DELETE /api/files Delete file/directory | | | | | | | | # Credentials (routers/credentials.py) | | -| | POST /api/credentials/update Hot-reload credentials | | +| | POST /api/credentials/inject Write credential files (validated) | | | | GET /api/credentials/status Get credential file status | | | | | | | | # Trinity Injection (routers/trinity.py) | | diff --git a/docs/diagrams/09-agent-lifecycle-states.md b/docs/diagrams/09-agent-lifecycle-states.md index 32890e513..25b77d332 100644 --- a/docs/diagrams/09-agent-lifecycle-states.md +++ b/docs/diagrams/09-agent-lifecycle-states.md @@ -201,7 +201,8 @@ async def inject_trinity_meta_prompt(agent_name: str, max_retries: int = 5, retr # Source: src/backend/services/agent_service/lifecycle.py:93-171 async def inject_assigned_credentials(agent_name: str, max_retries: int = 3, retry_delay: float = 2.0): # Get owner, fetch assigned credentials, push to agent HTTP endpoint - # Endpoint: http://agent-{name}:8000/api/credentials/update + # Endpoint: http://agent-{name}:8000/api/credentials/inject (#2008 — this + # diagram named /credentials/update, which had no callers and was removed) ``` --- diff --git a/docs/memory/architecture.md b/docs/memory/architecture.md index fb8edf2b6..d0af7ce86 100644 --- a/docs/memory/architecture.md +++ b/docs/memory/architecture.md @@ -338,7 +338,6 @@ Vector 0.43.1 (`timberio/vector:0.43.1-alpine`). Captures all container stdout/s **Internal server** `agent-server.py` (FastAPI, port 8000): - `/api/chat` - Claude Code execution (messages persisted to database) - `/health` - Health check. Returns `{status}` plus `active_tasks` (concurrent executions across `/api/chat` + `/api/task`), `last_task_at`, `consecutive_failures` (reset on success — consumed by the dispatch breaker #526 and fleet health #307), the #333 `diagnostics` gauges (#1020), and `clone_status` (`ok`|`failed`, #1439) — a coarse, server-computed identity-clone signal read defensively from the untrusted `.git-clone-status` marker (enum only, never the agent-supplied repo/branch/error strings, since `/health` is unauthenticated) that lets `monitoring_service` mark a silently-failed GitHub-template clone **unhealthy** instead of reporting a running-but-empty agent healthy. `mailbox_depth` intentionally NOT emitted — no agent-side mailbox until the actor model (#945); the backend derives queue depth from `CapacityManager`. Counters live in `agent_server/state.py`; backend reads them in `monitoring_service.py` with graceful defaults for older images. -- `/api/credentials/update` - Hot-reload credentials (rewrites `.env`/`.mcp.json`) - `/api/credentials/reload-token` - Surgical subscription-token hot-reload (#1089): mutates the agent-server process `os.environ["CLAUDE_CODE_OAUTH_TOKEN"]` so the NEXT claude subprocess uses the rotated token while in-flight subprocesses keep theirs; persists to the writable-layer override `/var/lib/trinity/oauth-token` (0600). Does NOT touch `.env`/`.mcp.json`. See [Subscription Token Rotation](#subscription-token-rotation-via-hot-reload-1089) - `/api/chat/session` - Context window stats - `/api/files`, `/api/files/download` (100MB limit), `/api/files/mkdir` (workspace-confined, #37) diff --git a/docs/planning/TARGET_ARCHITECTURE.md b/docs/planning/TARGET_ARCHITECTURE.md index a5cbcde7f..2f9391202 100644 --- a/docs/planning/TARGET_ARCHITECTURE.md +++ b/docs/planning/TARGET_ARCHITECTURE.md @@ -294,7 +294,7 @@ When a single container's `max_parallel_tasks` ceiling — bounded by container - Docker container per agent — correct isolation model, keep - `agent-server.py` (FastAPI on port 8000 inside container) — keep - Claude Code as the execution runtime — keep -- `/api/chat`, `/health`, `/api/credentials/update`, `/api/files` endpoints — keep +- `/api/chat`, `/health`, `/api/credentials/inject`, `/api/files` endpoints — keep (`/api/credentials/update` was listed here until #2008 removed it: it had no callers anywhere) - Pre-check hook (`~/.trinity/pre-check`) — keep - Credential injection via `.env` + `.credentials.enc` — keep diff --git a/docs/testing/API_TEST_REQUIREMENTS.md b/docs/testing/API_TEST_REQUIREMENTS.md index eea416a46..2f75bb30b 100644 --- a/docs/testing/API_TEST_REQUIREMENTS.md +++ b/docs/testing/API_TEST_REQUIREMENTS.md @@ -615,14 +615,6 @@ These tests run directly against an agent container's internal API. They require ### 17. Agent Server Credentials Tests (`test_agent_credentials_direct.py`) -#### REQ-AS-CRED-001: Update Credentials -- **Endpoint**: `POST /api/credentials/update` -- **Tests**: - - Writes .env file - - Generates .mcp.json from template - - Returns updated files list -- **Priority**: HIGH - #### REQ-AS-CRED-002: Credential Status - **Endpoint**: `GET /api/credentials/status` - **Tests**: diff --git a/tests/registry.json b/tests/registry.json index 2864004e4..a1b6cef63 100644 --- a/tests/registry.json +++ b/tests/registry.json @@ -2000,6 +2000,19 @@ "credentials" ], "description": "Credential injects mirrored every .env key into the long-lived agent-server process env, and all five runtime spawn sites (claude_code, headless_executor, codex_runtime, gemini_runtime x2) passed env={**os.environ, ...} - a one-way sync with NO delete phase. A key removed from .env by any non-mirroring write path (SSH, docker exec, or an agent editing its own .env) kept reaching every subsequently-spawned execution until container restart, and was invisible to /proc//environ (an exec-time snapshot - runtime os.environ mutations never appear there) AND to docker exec (which shows the container baseline, not this process's mutated env), so credential revocation silently failed with no inspection route that could reveal it. Fix: services/execution_env.py rebuilds the execution env per spawn as INITIAL_ENV (boot baseline = what docker exec shows) + .env (parsed fresh, authoritative for credentials) + RUNTIME_OVERRIDES (#1089 token rotation - applied AFTER the file so a stale token in .env cannot beat an explicit rotation; None = force-unset, which a dict merge cannot express) + extra (EXECUTION_TAG_NAME last, #407). The process mirror gains a delete phase that restores the container baseline rather than popping blind, and only ever undoes keys it itself wrote. Covers: the issue's exact reproduction (inject, delete out-of-band, assert the next execution is clean), re-injecting a cleaned .env, baseline restore vs delete, the #1089 no-regression cases, EXECUTION_TAG_NAME non-displacement, tolerant .env parsing incl. the writer's own backslash escaping (.strip('\"') eats a legitimate trailing quote), unreadable/oversized files degrading to the baseline rather than raising on the spawn path, protected keys (.env is agent-writable and now read at every spawn, so PATH/LD_PRELOAD/BASH_ENV redirection is refused), the name-only drift report added to /api/credentials/status, and a static guard that fails if a sixth spawn site reintroduces the {**os.environ} form." + }, + { + "file": "unit/test_2008_dead_credentials_update.py", + "feature": "#2008", + "added": "2026-08-05", + "categories": [ + "agent-server", + "unit", + "security", + "credentials", + "docs" + ], + "description": "POST /api/credentials/update had no callers anywhere - not the backend, MCP server, startup.sh, frontend, or enterprise submodule - while its own docstring said 'called by the Trinity backend', agent_server/routers/files.py named it as the SANCTIONED alternative to raw .mcp.json editing (both direct-edit blocks rested on that sentence), and architecture.md listed it as live. So the documented escape hatch for configuring MCP servers did not exist. It also mattered if re-wired: its renderer was a whole-text str.replace over .mcp.json.template, so unlike every other path it DID substitute into `command`, with no validate_mcp_config anywhere on the path - the RCE-by-config class #590 closed, inert only because nothing called it. Resolved by deletion (the decision the issue asks for), because #2007 put the .mcp.json.template renderer where the files actually are: in-container at startup, env-only, validated per server. Covers: the route is gone (AST, not a substring scan - the corrected files.py comment and this test both still mention the string), the request model is gone (a dangling model is how a deleted route comes back), the whole-text replace renderer is gone, and no LIVE doc sends anyone back to it (files.py, architecture.md, API_TEST_REQUIREMENTS, the container diagram). Historical records - docs/archive/**, docs/security-reports/**, the 2026-06 meta-analysis - are deliberately out of scope: they were true when written." } ] } diff --git a/tests/unit/test_2008_dead_credentials_update.py b/tests/unit/test_2008_dead_credentials_update.py new file mode 100644 index 000000000..4e55a2000 --- /dev/null +++ b/tests/unit/test_2008_dead_credentials_update.py @@ -0,0 +1,133 @@ +"""#2008 — `POST /api/credentials/update` was documented but unreachable. + +The endpoint had **no callers**: not the backend, not the MCP server, not +`startup.sh`, not the frontend, not the enterprise submodule. Two live places +treated it as the sanctioned path anyway: + + * `agent_server/routers/files.py` blocked BOTH direct-edit paths for + `.mcp.json` on the strength of a sentence pointing at it — so the + documented escape hatch for configuring MCP servers did not exist; + * `docs/memory/architecture.md` listed it as a live agent endpoint. + +It also mattered if anyone re-wired it: the renderer was a whole-text +`str.replace` over `.mcp.json.template`, so unlike every other path it **did** +substitute into `command`, and nothing on that path ran `validate_mcp_config`. +A credential value becoming the executed command is the RCE-by-config class +#590 closed. It was inert only because nothing called it — while the security +comment above actively invited someone to. + +Resolved by deletion, because #2007 put the `.mcp.json.template` renderer where +the files actually are (in-container at startup, `env`-only, validated +per-server). The AC's "a test that fails if a `${VAR}` can reach `command` +through this path" is therefore expressed as: the path is gone, and no live +document sends anyone back to it. +""" + +from __future__ import annotations + +import ast +from pathlib import Path + +import pytest + +_ROOT = Path(__file__).resolve().parents[2] +_AGENT_SERVER = _ROOT / "docker" / "base-image" / "agent_server" +_CREDENTIALS = _AGENT_SERVER / "routers" / "credentials.py" +_FILES = _AGENT_SERVER / "routers" / "files.py" +_MODELS = _AGENT_SERVER / "models.py" + +pytestmark = pytest.mark.unit + + +class TestTheEndpointIsGone: + + def test_no_route_registers_credentials_update(self): + """AST, not a substring scan: a `@router.post("/api/credentials/update")` + is what makes it reachable, and a mention of the string in a comment is + not (this file, and the corrected `files.py` comment, both contain it).""" + tree = ast.parse(_CREDENTIALS.read_text()) + routes = [] + for node in ast.walk(tree): + if not isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)): + continue + for dec in node.decorator_list: + if not isinstance(dec, ast.Call): + continue + for arg in dec.args: + if isinstance(arg, ast.Constant) and arg.value == "/api/credentials/update": + routes.append(node.name) + assert not routes, ( + f"/api/credentials/update is registered again by {routes} (#2008). " + "It substituted ${VAR} into `command` with no validate_mcp_config — " + "the #590 RCE-by-config class. If it must come back, it needs the " + "validator and a refusal for placeholders in `command`." + ) + + def test_its_request_model_is_gone_too(self): + """A dangling model is how a deleted route quietly comes back.""" + assert "class CredentialUpdateRequest" not in _MODELS.read_text() + + def test_the_whole_text_replace_renderer_is_gone(self): + """The specific mechanism: a whole-text `str.replace` over the template + substitutes anywhere, including `command`.""" + src = _CREDENTIALS.read_text() + assert "mcp_template.read_text()" not in src + assert "generated_content.replace(placeholder" not in src + + +class TestNoLiveDocSendsYouThere: + """AC #2 and #3: the two places that named a path which did not exist.""" + + def test_files_py_names_a_path_that_exists(self): + """The rationale must point at a route that runs today. + + The first draft was `"/api/credentials/inject" in block or + ".mcp.json.template" in block`. `PROTECTED_PATHS` contains the literal + `.mcp.json.template` and sits well inside the 1500-character window, so + the `or` was satisfied by the path list no matter what the comment + said — deleting the whole rationale, or rewording it to point back at + the deleted endpoint, both left this green. It now asserts what the AC + actually claims: the sanctioned path is named, and the deleted route is + not offered as a destination. + """ + src = _FILES.read_text() + # Anchored on the rationale block itself, not a fixed byte window: a + # 1500-character lookbehind both reaches into `PROTECTED_PATHS` (which + # is what made the old `or` branch always true) and silently drops the + # top of the comment as soon as it grows. + start = src.index("# Paths that cannot be edited via the file-write endpoint.") + block = src[start:src.index("EDIT_PROTECTED_PATHS = [")] + # The backend route is `/api/agents/{name}/credentials/inject`; the + # first draft asserted `/api/credentials/inject`, which is not a path + # that exists anywhere — and it went unnoticed because the `or` branch + # carried the test. + assert "credentials/inject" in block, ( + "the EDIT_PROTECTED_PATHS rationale must name a real path — both " + "direct-edit blocks rest on it" + ) + # The one permitted mention is the historical note saying it was + # removed; anything else is the comment sending an owner to a route + # that does not exist. + without_history = block.replace( + "`/api/credentials/update`, which had no callers anywhere and was removed", "" + ) + assert "/api/credentials/update" not in without_history, ( + "the rationale points at /api/credentials/update as a live path " + "again — it was deleted in #2008" + ) + assert "platform-internal\n# /api/credentials/update flow" not in src + + def test_architecture_md_does_not_list_it_as_live(self): + arch = (_ROOT / "docs" / "memory" / "architecture.md").read_text() + assert "`/api/credentials/update` - Hot-reload credentials" not in arch + + @pytest.mark.parametrize("doc", [ + "docs/testing/API_TEST_REQUIREMENTS.md", + "docs/diagrams/03-agent-container.md", + ]) + def test_no_live_doc_advertises_the_endpoint(self, doc): + """Historical records (`docs/archive/**`, `docs/security-reports/**`, + the 2026-06 meta-analysis) are deliberately NOT in scope: they are + point-in-time and were true when written.""" + text = (_ROOT / doc).read_text() + assert "POST /api/credentials/update" not in text diff --git a/tests/unit/test_reload_token_endpoint.py b/tests/unit/test_reload_token_endpoint.py index 22fa83689..c4b8a1982 100644 --- a/tests/unit/test_reload_token_endpoint.py +++ b/tests/unit/test_reload_token_endpoint.py @@ -7,7 +7,8 @@ to the writable-layer override (``/var/lib/trinity/oauth-token``, 0600) so it survives a plain stop+start (F2 durability). It must NOT rewrite ``.env`` / ``.mcp.json`` or re-inject Trinity MCP — those are the destructive whole-file -flows owned by ``/api/credentials/update`` and ``/api/credentials/inject``. +flows owned by ``/api/credentials/inject`` (``/api/credentials/update`` was +the other such flow and was deleted in #2008 — it had no callers). Module: docker/base-image/agent_server/routers/credentials.py