From e88ae61072712831a688f66f56a4154a489368c3 Mon Sep 17 00:00:00 2001 From: Oleksii Dolhov Date: Wed, 5 Aug 2026 11:33:25 +0300 Subject: [PATCH 1/2] fix(agent-server): remove the unreachable /api/credentials/update endpoint (#2008) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The endpoint had no callers anywhere — backend, MCP server, startup.sh, frontend and the enterprise submodule all verified. Its docstring claimed the backend called it; `routers/files.py` named it as the *sanctioned* alternative to raw `.mcp.json` editing, with both direct-edit blocks resting on that one sentence; and architecture.md listed it as a live agent endpoint. The documented escape hatch for configuring MCP servers did not exist. It was also the one path that would substitute a credential value into `command` — a whole-text `str.replace` over `.mcp.json.template` with no `validate_mcp_config` anywhere on it. That is the RCE-by-config class #590 closed, inert only because nothing called it, while the security comment above invited someone to. Deleted rather than revived, which is the branch the issue anticipated: #2007 put the `.mcp.json.template` renderer where the files actually are — in the container at startup, `env`-only, validated per server. Reviving this would have meant a second renderer with weaker rules. The references it left behind are corrected to name paths that exist: `files.py` now points at the template and at the validated `POST /api/agents/{name}/credentials/inject`; architecture.md, the API test requirements, and both diagrams drop or repoint it. Historical records (`docs/archive/**`, `docs/security-reports/**`, the 2026-06 meta-analysis) are left alone — they were true when written. Closes #2008 Co-Authored-By: Claude Opus 5 (1M context) --- docker/base-image/agent_server/models.py | 7 -- .../agent_server/routers/credentials.py | 96 ---------------- .../base-image/agent_server/routers/files.py | 14 ++- docs/diagrams/03-agent-container.md | 2 +- docs/diagrams/09-agent-lifecycle-states.md | 3 +- docs/memory/architecture.md | 1 - docs/planning/TARGET_ARCHITECTURE.md | 2 +- docs/testing/API_TEST_REQUIREMENTS.md | 8 -- tests/registry.json | 13 +++ .../unit/test_2008_dead_credentials_update.py | 103 ++++++++++++++++++ 10 files changed, 130 insertions(+), 119 deletions(-) create mode 100644 tests/unit/test_2008_dead_credentials_update.py 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 1ee7128ba..86b3fb8bb 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, @@ -71,101 +70,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. Also export credentials to environment (for current process) - # Note: This won't affect already-running subprocesses, but helps for new ones - for var_name, value in request.credentials.items(): - os.environ[var_name] = str(value) - - # 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..4c876376b 100644 --- a/docker/base-image/agent_server/routers/files.py +++ b/docker/base-image/agent_server/routers/files.py @@ -179,10 +179,16 @@ 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 paths (#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): +# * declare servers in the template's `.mcp.json.template` — rendered into +# `.mcp.json` at container startup, `${VAR}` substituted inside `env` only, +# each server validated (#2007); +# * post-deploy, `POST /api/agents/{name}/credentials/inject` — the same +# `validate_mcp_config` gate, which is Layer 2 of the #590 closure (#598). # # 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 ff72b7e57..a2be8091b 100644 --- a/docs/memory/architecture.md +++ b/docs/memory/architecture.md @@ -328,7 +328,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 eb177c29f..c0b02609c 100644 --- a/tests/registry.json +++ b/tests/registry.json @@ -1553,6 +1553,19 @@ "parity" ], "description": "The agent server was outside ent#314's YAML sweep (#1965). utils/safe_yaml.py (PR #1961) put every author-controlled YAML reader in the backend behind one hardened loader, and its AST guard walks the whole backend with an EMPTY allowlist - but it walked _BACKEND.rglob only, so docker/base-image/agent_server/ kept six bare yaml.safe_load calls on documents the backend itself assigns REJECT: template.yaml (x2, credential_requirements_service), skill frontmatter (skill_packaging), dashboard.yaml (compatibility/static_checks) and .trinity/persistent-state.yaml. The vector is amplification at SERIALIZATION, not parse - a 416 B level-6 anchor bomb resolves in ~0.001 s and blows up to ~110 MB when something walks the graph - and the backend proxies /info and /dashboard, so the walk happens in-container then again across the wire. Covers: byte-parity of the vendored loader (the credential_paths.py shape, Invariant #5) plus proof the vendored COPY actually behaves - refuses a level-6 bomb under BUDGET, any alias under REJECT, duplicate keys, and still parses an honest document (byte parity is not behaviour parity if the file never imports); each of the four agent-authored sites on the shared loader with its backend counterpart's kind AND policy; /config/agent-config.yaml deliberately BUDGET not REJECT, stated as an exception because the platform writes it and bind-mounts it mode:'ro' so the agent cannot author it, and yaml.dump emits an anchor for any shared object reference - REJECT there would be a self-inflicted outage for no security; no bare safe_load left anywhere in the tree; and HardenedYamlError named in the except arms that previously caught only yaml.YAMLError (it is a ValueError, so without its own arm a refused bomb escapes to the generic handler and surfaces as the unnamed 500 the AC rules out - the trap static_checks._parse_yaml records backend-side). AC #4 end-to-end: a level-6 bomb in a container's template.yaml is refused by BOTH template.yaml readers with the expanded graph never reaching the response, an honest template still serves, and the metrics assertion checks the NAMED refusal rather than has_metrics:False - a bomb parses fine under bare safe_load and yields no metrics: key, so the flag alone passes against the very tree this issue reports. The AST-guard widening itself lives in test_ent314_hardened_yaml.py (both trees, still empty allowlist) rather than here, because splitting a guard across two files is how the second copy stops being run." + }, + { + "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..4d1a2134e --- /dev/null +++ b/tests/unit/test_2008_dead_credentials_update.py @@ -0,0 +1,103 @@ +"""#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): + src = _FILES.read_text() + block = src[src.index("EDIT_PROTECTED_PATHS") - 1500:src.index("EDIT_PROTECTED_PATHS")] + assert "/api/credentials/inject" in block or ".mcp.json.template" in block, ( + "the EDIT_PROTECTED_PATHS rationale must name a real path — both " + "direct-edit blocks rest on it" + ) + 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 From b6e437e0a97f1099452211e065a877487750f25b Mon Sep 17 00:00:00 2001 From: Oleksii Dolhov Date: Mon, 10 Aug 2026 11:09:27 +0300 Subject: [PATCH 2/2] docs(2008): point files.py at the path that exists, and make its guard fail when it doesn't MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit AC #2 said 'files.py describes a path that exists' and neither half held. The comment replaced one forward reference with another: it told owners to declare servers in `.mcp.json.template`, 'rendered into .mcp.json at container startup (#2007)'. That renderer does not exist on dev — 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. #2013 is still open. It now leads with `POST /api/agents/{name}/credentials/inject`, which is real today, and marks the template rendering as PLANNED with the reason it isn't reachable yet. The guard was inert. `assert "/api/credentials/inject" in block or ".mcp.json.template" in block` — `PROTECTED_PATHS` contains that literal and sits inside the 1500-character lookbehind, so the `or` was true no matter what the comment said. Deleting the entire rationale, or rewording it to point back at the deleted endpoint, both left it green. Three changes, each of which the mutations proved necessary: * drop the `or` branch and assert the deleted route is not offered as a destination (the historical 'was removed' note is the one allowed mention); * anchor on the rationale block instead of a 1500-byte window — the window both reached into PROTECTED_PATHS and silently dropped the top of the comment as soon as it grew (it did, here); * assert `credentials/inject`, not `/api/credentials/inject`. The backend route is `/api/agents/{name}/credentials/inject`, so the literal in the first draft matched nothing anywhere — invisible because the `or` carried it. Mutation-verified: rationale deleted -> FAILED reworded to /credentials/update -> FAILED as shipped -> 7 passed Also the two sweep misses: test_reload_token_endpoint.py's docstring no longer lists the deleted endpoint as a live owner, and CLAUDE.md no longer says .mcp.json is 'Generated at runtime' (nothing generates it; injection writes it). Related to #2008 Co-Authored-By: Claude Opus 5 (1M context) --- CLAUDE.md | 4 +-- .../base-image/agent_server/routers/files.py | 18 ++++++---- .../unit/test_2008_dead_credentials_update.py | 34 +++++++++++++++++-- tests/unit/test_reload_token_endpoint.py | 3 +- 4 files changed, 48 insertions(+), 11 deletions(-) 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/routers/files.py b/docker/base-image/agent_server/routers/files.py index 4c876376b..dbd8f90ab 100644 --- a/docker/base-image/agent_server/routers/files.py +++ b/docker/base-image/agent_server/routers/files.py @@ -181,14 +181,20 @@ async def download_file(path: str): # "users need to modify them" — but raw editing of either is RCE-by-config: # tool `command:` fields run as the agent process. See #590 (AISEC-C2). # -# The sanctioned paths (#2008 — this comment previously named +# 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): -# * declare servers in the template's `.mcp.json.template` — rendered into -# `.mcp.json` at container startup, `${VAR}` substituted inside `env` only, -# each server validated (#2007); -# * post-deploy, `POST /api/agents/{name}/credentials/inject` — the same -# `validate_mcp_config` gate, which is Layer 2 of the #590 closure (#598). +# * `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/tests/unit/test_2008_dead_credentials_update.py b/tests/unit/test_2008_dead_credentials_update.py index 4d1a2134e..4e55a2000 100644 --- a/tests/unit/test_2008_dead_credentials_update.py +++ b/tests/unit/test_2008_dead_credentials_update.py @@ -79,12 +79,42 @@ 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() - block = src[src.index("EDIT_PROTECTED_PATHS") - 1500:src.index("EDIT_PROTECTED_PATHS")] - assert "/api/credentials/inject" in block or ".mcp.json.template" in block, ( + # 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): 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