From 1765a621bd03770eef2367bbfbeca45026942c95 Mon Sep 17 00:00:00 2001 From: Oleksii Dolhov Date: Wed, 5 Aug 2026 11:01:30 +0300 Subject: [PATCH 1/2] fix(deploy): validate an archive .mcp.json before it is persisted, not after (#2006) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ent#213 added the archive `.mcp.json` guard, but it ran 32 lines after `shutil.copytree(extract_root, dest_path)` and the `finally` removed only `temp_dir`. The 400 aborted the deploy while the refused config stayed under `/data/deployed-templates//` — a live member of `_LOCAL_TEMPLATE_ROOTS` — so it remained reachable through `POST /api/agents {"template": "local:"}`, by the same caller who had just been told the deploy failed. A guard that runs after the persist is not a gate. The guard moves to step 6a, on the extracted copy. That also puts it ahead of the quota lookup and the stop-previous-version step, so a deploy that will be refused no longer stops a running agent on its way to the refusal. The post-copy call site becomes bookkeeping only. `_remove_partial_deploy` is a second layer for the rest of the class: any failure between the copy and creation (`_prepopulate_workspace_from_template`'s 500, a docker outage) leaves the same addressable residue. The handle is cleared just before `create_agent_fn`, so a directory that a container may already reference is never removed on a late failure — creation-path rollback stays crud.py's job. Regression coverage drives the real `deploy_local_agent_logic` and asserts on the filesystem. ent#213's suite tests the guard in isolation ("the full deploy flow needs Docker"), which is why the ordering was never covered; no Docker is needed here because the rejection path now reaches nothing external. The ordering is pinned twice — behaviourally (every post-guard step explodes) and statically (source order) — because once cleanup exists, a residue assertion alone can no longer distinguish the two candidate fixes. Closes #2006 Co-Authored-By: Claude Opus 5 (1M context) --- src/backend/services/agent_service/deploy.py | 68 +++- tests/registry.json | 12 + tests/unit/test_2006_deploy_residue.py | 349 +++++++++++++++++++ 3 files changed, 425 insertions(+), 4 deletions(-) create mode 100644 tests/unit/test_2006_deploy_residue.py diff --git a/src/backend/services/agent_service/deploy.py b/src/backend/services/agent_service/deploy.py index b53bdcc45..04d33abe0 100644 --- a/src/backend/services/agent_service/deploy.py +++ b/src/backend/services/agent_service/deploy.py @@ -83,6 +83,32 @@ def _validate_archive_mcp_config(mcp_file: Path, version_name: str, ) +def _remove_partial_deploy(dest_created: Path | None) -> None: + """Remove a deployed-templates directory a failed deploy left behind (#2006). + + Second layer under the moved `.mcp.json` gate: that gate now runs before + the copy, but every OTHER failure between `copytree` and `create_agent_fn` + (`_prepopulate_workspace_from_template`'s 500, a docker outage) leaves the + same addressable residue, because `/data/deployed-templates` is a member of + `_LOCAL_TEMPLATE_ROOTS` and the directory name is exactly the + `local:` id the caller can pass to `POST /api/agents`. + + Deliberately NOT a blanket cleanup in `finally`: the caller clears its + handle before creation starts, so a directory a container may already + reference is never removed. Never raises — this runs on an error path and + must not replace the real failure with a cleanup failure. + """ + if dest_created is None: + return + try: + shutil.rmtree(dest_created) + logger.info("Removed partial deploy directory: %s", dest_created) + except Exception as e: # noqa: BLE001 — the original error is what matters + logger.warning( + "could not remove partial deploy directory %s: %s", dest_created, e + ) + + def _prepopulate_workspace_from_template(version_name: str, template_dir: Path) -> None: """Pre-populate `agent-{version_name}-workspace` with the template files (#950). @@ -359,6 +385,12 @@ async def deploy_local_agent_logic( DeployLocalResponse with deployment details """ temp_dir = None + # #2006: the deployed-templates dir this call created, while it is still + # this call's to remove. Cleared just before `create_agent_fn` — once + # creation starts, the directory may be referenced by a container mount + # spec, so removing it on a late failure would break a half-created agent + # rather than clean up after one. + dest_created = None try: # 1. Validate archive size @@ -441,6 +473,26 @@ async def deploy_local_agent_logic( base_name = sanitize_agent_name(base_name) + # 6a. Structure-validate an archive-supplied `.mcp.json` (ent#213) — + # on the EXTRACTED copy, BEFORE anything is persisted or stopped + # (#2006). The guard used to run 32 lines after `copytree`, so a 400 + # aborted the deploy while leaving the rejected config on disk under + # `/data/deployed-templates//` — a live member of + # `_LOCAL_TEMPLATE_ROOTS`, therefore reachable by a subsequent + # `POST /api/agents {"template": "local:"}`. A guard that + # runs after the persist is not a gate. + # + # `temp_dir` is the only thing written before this point and the + # `finally` already removes it, so a rejection now leaves nothing + # behind. Running here also precedes the quota check and the + # stop-previous-version step, so a deploy that will be refused no + # longer stops a running agent on its way to the refusal. + archive_mcp_file = extract_root / ".mcp.json" + if archive_mcp_file.exists(): + _validate_archive_mcp_config( + archive_mcp_file, base_name, getattr(current_user, "email", None) + ) + # 6b. Agent quota enforcement: per-role limits (QUOTA-001) # Skip for redeploys of existing agents owned by this user existing_versions = get_agents_by_prefix(base_name) @@ -532,6 +584,7 @@ async def deploy_local_agent_logic( shutil.rmtree(dest_path) shutil.copytree(extract_root, dest_path) + dest_created = dest_path logger.info(f"Copied agent template to: {dest_path}") # 10. Create agent @@ -565,10 +618,11 @@ async def deploy_local_agent_logic( mcp_file = dest_path / ".mcp.json" if mcp_file.exists(): - # ent#213: structure-validate an archive-supplied `.mcp.json` before - # it is pre-populated into the workspace, matching the inject path. - _validate_archive_mcp_config(mcp_file, version_name, - getattr(current_user, "email", None)) + # ent#213's validation moved to step 6a (#2006) — it now runs on + # `extract_root` before this copy exists. This is a copy of the + # bytes that already passed, so re-validating here would only be + # able to fail on something written between the two points, and + # nothing writes `.mcp.json` in that window. Bookkeeping only. credentials_imported[".mcp.json"] = "from_archive" # Write credentials from request to template directory @@ -608,6 +662,10 @@ async def deploy_local_agent_logic( # run anyway since no /template bind is set up — see crud.py). _prepopulate_workspace_from_template(version_name, dest_path) + # Hand the directory over: from here it can be referenced by the + # container's mount spec, so it is no longer ours to delete (#2006). + dest_created = None + agent_status = await create_agent_fn( agent_config, current_user, @@ -636,8 +694,10 @@ async def deploy_local_agent_logic( ) except HTTPException: + _remove_partial_deploy(dest_created) raise except Exception as e: + _remove_partial_deploy(dest_created) raise HTTPException( status_code=500, detail=f"Failed to deploy local agent: {str(e)}" diff --git a/tests/registry.json b/tests/registry.json index eb177c29f..b84094ec7 100644 --- a/tests/registry.json +++ b/tests/registry.json @@ -1553,6 +1553,18 @@ "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_2006_deploy_residue.py", + "feature": "#2006", + "added": "2026-08-05", + "categories": [ + "backend", + "unit", + "security", + "deploy" + ], + "description": "deploy-local validated an archive-supplied .mcp.json 32 lines AFTER shutil.copytree(extract_root, dest_path), and the finally block removed only temp_dir - so a config the ent#213 validator refused stayed on disk under /data/deployed-templates//, a live member of crud._LOCAL_TEMPLATE_ROOTS, addressable by the same caller via POST /api/agents {template: local:}. A guard that runs after the persist is not a gate. Fix: the guard moved to step 6a, running on extract_root before the copy AND before the quota check and the stop-previous-version step (a doomed deploy no longer stops a running agent on its way to a 400), plus _remove_partial_deploy as a second layer for failures between copytree and create_agent_fn (_prepopulate_workspace_from_template's 500) - deliberately handed over before creation starts, since a directory a container may already reference must not be removed on a late failure. ent#213's suite tests _validate_archive_mcp_config in isolation ('the full deploy flow needs Docker'), which is exactly why the ordering was never covered; these drive the REAL deploy_local_agent_logic and assert on the filesystem, needing no Docker because the rejection path now reaches nothing external. Covers: the issue's reproduction, local: resolving to nothing, the ordering pinned twice (behaviourally by exploding every post-guard step, and statically by source order - the behavioural residue assertion alone cannot distinguish the two candidate fixes once cleanup exists), the valid path reaching creation with the template in place, an archive with no .mcp.json, and both sides of the cleanup boundary." } ] } diff --git a/tests/unit/test_2006_deploy_residue.py b/tests/unit/test_2006_deploy_residue.py new file mode 100644 index 000000000..3c440c6a1 --- /dev/null +++ b/tests/unit/test_2006_deploy_residue.py @@ -0,0 +1,349 @@ +"""#2006 — a rejected `.mcp.json` must leave nothing on disk. + +ent#213 added the archive `.mcp.json` guard, but it ran **32 lines after** +`shutil.copytree(extract_root, dest_path)`. The 400 aborted the deploy and +nothing removed the directory, so the refused config stayed under +`/data/deployed-templates//` — a live member of +`crud._LOCAL_TEMPLATE_ROOTS` — and remained reachable via a subsequent +`POST /api/agents {"template": "local:"}` by the same caller who +had just been told the deploy failed. A guard that runs after the persist is +not a gate. + +ent#213's own suite exercises `_validate_archive_mcp_config` in isolation ("the +full deploy flow needs Docker" — `test_213_deploy_mcp_validation.py:17-19`), +which is exactly why the ordering was never covered: the guard was always +correct, its POSITION was not. So these tests drive the **real** +`deploy_local_agent_logic` and assert on the filesystem. + +No Docker is needed, and that is a property of the fix rather than of the +harness: the guard now runs before the quota lookup, before the +stop-previous-version step, and before the copy, so nothing external is reached +on the rejection path. The valid-archive test mocks the db/docker lookups that +follow, and stops at `create_agent_fn`. +""" + +from __future__ import annotations + +import base64 +import io +import json +import tarfile +from datetime import datetime, timezone +from pathlib import Path +from types import SimpleNamespace + +import pytest +from fastapi import HTTPException + +from models import AgentStatus +from services.agent_service import deploy as deploy_mod +from services.agent_service.deploy import deploy_local_agent_logic + +pytestmark = pytest.mark.unit + + +# The `${LD_PRELOAD}` reserved env-ref from the issue's reproduction: the +# validator refuses it, and #1929's `${VAR:-default}` blanking on the CREATE +# path would later launder it to `""` — which is why "validate the generated +# output" is not a fix for this and the residue must simply not exist. +REJECTED_MCP = { + "mcpServers": { + "ebook-mcp": { + "command": "python3", + "args": ["--directory", "${LD_PRELOAD}", "run", "ebook-mcp"], + "env": {"LD_PRELOAD_REF": "${LD_PRELOAD}"}, + } + } +} + +VALID_MCP = {"mcpServers": {"ok": {"command": "python3", "args": ["-m", "server"]}}} + + +def _archive(name: str, mcp: dict | None) -> str: + """A minimal Trinity-compatible archive, base64 tar.gz, as the API takes.""" + buf = io.BytesIO() + with tarfile.open(fileobj=buf, mode="w:gz") as tar: + def add(arcname: str, content: str): + data = content.encode() + info = tarfile.TarInfo(arcname) + info.size = len(data) + tar.addfile(info, io.BytesIO(data)) + + add( + f"{name}/template.yaml", + f"name: {name}\ntype: business-assistant\n" + "resources:\n cpu: '2'\n memory: 4g\n", + ) + add(f"{name}/CLAUDE.md", "# agent\n") + if mcp is not None: + add(f"{name}/.mcp.json", json.dumps(mcp)) + return base64.b64encode(buf.getvalue()).decode() + + +@pytest.fixture +def templates_dir(tmp_path, monkeypatch): + """Point the deployed-templates root at tmp_path. + + Patched as a module attribute because the function reads the constant at + call time (`Path(DEPLOYED_TEMPLATES_DIR_IN_BACKEND)`), which is what makes + the real flow testable without touching /data. + """ + d = tmp_path / "deployed-templates" + monkeypatch.setattr( + deploy_mod, "DEPLOYED_TEMPLATES_DIR_IN_BACKEND", str(d), raising=True + ) + return d + + +@pytest.fixture +def user(): + return SimpleNamespace( + username="creator", email="creator@example.com", role="creator", id=7 + ) + + +def _agent_status(name: str) -> AgentStatus: + """A minimal real AgentStatus — DeployLocalResponse validates the field, so + a stand-in namespace would fail for reasons unrelated to what is tested.""" + return AgentStatus( + name=name, type="business-assistant", status="running", port=2222, + created=datetime(2026, 8, 5, tzinfo=timezone.utc), + resources={"cpu": "2", "memory": "4g"}, + ) + + +def _body(archive: str, name: str): + return SimpleNamespace(archive=archive, name=name, credentials=None) + + +async def _deploy(body, user, create_fn=None): + async def _fail_if_called(*a, **kw): # pragma: no cover — asserted not reached + raise AssertionError("create_agent_fn must not be reached") + + return await deploy_local_agent_logic( + body, user, SimpleNamespace(), create_fn or _fail_if_called + ) + + +# --------------------------------------------------------------------------- +# The reported bug +# --------------------------------------------------------------------------- + +class TestRejectedDeployLeavesNothing: + + @pytest.mark.asyncio + async def test_rejected_mcp_json_leaves_no_deployed_template_dir( + self, templates_dir, user + ): + """AC #1 — the exact reproduction from the issue.""" + body = _body(_archive("resid-probe", REJECTED_MCP), "resid-probe") + + with pytest.raises(HTTPException) as ei: + await _deploy(body, user) + + assert ei.value.status_code == 400 + assert "Invalid .mcp.json in archive" in str(ei.value.detail) + # The residue check, not the guard check — the guard was already right. + assert not templates_dir.exists() or list(templates_dir.iterdir()) == [] + + @pytest.mark.asyncio + async def test_local_id_for_a_rejected_deploy_resolves_to_nothing( + self, templates_dir, user + ): + """AC #2 — `local:` must not resolve. + + Resolution is a directory lookup under the roots, so "no directory" is + the whole of it; asserted against the id the deploy path itself would + have used (`local:{version_name}`, `deploy.py`'s own create call). + """ + body = _body(_archive("ghost-tpl", REJECTED_MCP), "ghost-tpl") + + with pytest.raises(HTTPException): + await _deploy(body, user) + + assert not (templates_dir / "ghost-tpl").exists() + assert not (templates_dir / "ghost-tpl-v2").exists() + + @pytest.mark.asyncio + async def test_rejection_happens_before_any_external_side_effect( + self, templates_dir, user, monkeypatch + ): + """The ordering property, stated directly. + + The quota lookup and the stop-previous-version step both sat between + the copy and the old guard, so a doomed deploy could stop a running + agent on its way to a 400. Anything reached after the guard is made to + explode; a clean 400 proves the guard ran first. + """ + def boom(*a, **kw): + raise AssertionError("reached a post-guard step on a rejected deploy") + + for name in ("get_agents_by_prefix", "get_next_version_name", + "get_latest_version", "get_agent_container"): + monkeypatch.setattr(deploy_mod, name, boom, raising=True) + + body = _body(_archive("early-gate", REJECTED_MCP), "early-gate") + + with pytest.raises(HTTPException) as ei: + await _deploy(body, user) + assert ei.value.status_code == 400 + + +# --------------------------------------------------------------------------- +# The valid path is unchanged (AC #3) +# --------------------------------------------------------------------------- + +class TestValidDeployUnaffected: + + @pytest.mark.asyncio + async def test_valid_archive_still_reaches_creation_with_the_template_on_disk( + self, templates_dir, user, monkeypatch + ): + """A valid `.mcp.json` must deploy exactly as before: the directory is + persisted, and the handover to `create_agent_fn` happens with it in + place — the fix must not turn the cleanup into a regression.""" + seen = {} + + monkeypatch.setattr(deploy_mod, "get_agents_by_prefix", lambda *a: [], raising=True) + monkeypatch.setattr(deploy_mod, "get_latest_version", lambda *a: None, raising=True) + monkeypatch.setattr(deploy_mod, "get_next_version_name", lambda n: n, raising=True) + monkeypatch.setattr(deploy_mod, "get_agent_quota_for_role", lambda r: 0, raising=True) + monkeypatch.setattr(deploy_mod.db, "get_agents_by_owner", lambda u: [], raising=True) + monkeypatch.setattr( + deploy_mod, "collect_mcp_credential_warnings", lambda p: [], raising=True + ) + monkeypatch.setattr( + deploy_mod, "_prepopulate_workspace_from_template", + lambda v, d: None, raising=True, + ) + + async def create_fn(config, *a, **kw): + seen["template"] = config.template + seen["dir_present"] = (templates_dir / "good-tpl").is_dir() + seen["mcp_present"] = (templates_dir / "good-tpl" / ".mcp.json").is_file() + return _agent_status(config.name) + + body = _body(_archive("good-tpl", VALID_MCP), "good-tpl") + result = await _deploy(body, user, create_fn) + + assert result.status == "success" + assert seen == { + "template": "local:good-tpl", + "dir_present": True, + "mcp_present": True, + } + # Still there after a successful deploy — cleanup is failure-only. + assert (templates_dir / "good-tpl").is_dir() + + @pytest.mark.asyncio + async def test_archive_without_an_mcp_json_deploys( + self, templates_dir, user, monkeypatch + ): + monkeypatch.setattr(deploy_mod, "get_agents_by_prefix", lambda *a: [], raising=True) + monkeypatch.setattr(deploy_mod, "get_latest_version", lambda *a: None, raising=True) + monkeypatch.setattr(deploy_mod, "get_next_version_name", lambda n: n, raising=True) + monkeypatch.setattr(deploy_mod, "get_agent_quota_for_role", lambda r: 0, raising=True) + monkeypatch.setattr(deploy_mod.db, "get_agents_by_owner", lambda u: [], raising=True) + monkeypatch.setattr( + deploy_mod, "collect_mcp_credential_warnings", lambda p: [], raising=True + ) + monkeypatch.setattr( + deploy_mod, "_prepopulate_workspace_from_template", + lambda v, d: None, raising=True, + ) + + async def create_fn(config, *a, **kw): + return _agent_status(config.name) + + body = _body(_archive("no-mcp", None), "no-mcp") + result = await _deploy(body, user, create_fn) + assert result.status == "success" + + +# --------------------------------------------------------------------------- +# Second layer: a failure AFTER the copy also leaves nothing +# --------------------------------------------------------------------------- + +class TestPartialDeployCleanup: + + @pytest.mark.asyncio + async def test_a_failure_between_copy_and_creation_removes_the_directory( + self, templates_dir, user, monkeypatch + ): + """Moving the `.mcp.json` gate fixes the reported route; it does not + fix the class. `_prepopulate_workspace_from_template` raising (a docker + outage) leaves the same addressable `local:` residue.""" + monkeypatch.setattr(deploy_mod, "get_agents_by_prefix", lambda *a: [], raising=True) + monkeypatch.setattr(deploy_mod, "get_latest_version", lambda *a: None, raising=True) + monkeypatch.setattr(deploy_mod, "get_next_version_name", lambda n: n, raising=True) + monkeypatch.setattr(deploy_mod, "get_agent_quota_for_role", lambda r: 0, raising=True) + monkeypatch.setattr(deploy_mod.db, "get_agents_by_owner", lambda u: [], raising=True) + monkeypatch.setattr( + deploy_mod, "collect_mcp_credential_warnings", lambda p: [], raising=True + ) + + def explode(version_name, template_dir): + raise HTTPException(status_code=500, detail="docker unavailable") + + monkeypatch.setattr( + deploy_mod, "_prepopulate_workspace_from_template", explode, raising=True + ) + + body = _body(_archive("half-deploy", VALID_MCP), "half-deploy") + + with pytest.raises(HTTPException) as ei: + await _deploy(body, user) + + assert ei.value.status_code == 500 + assert not (templates_dir / "half-deploy").exists() + + @pytest.mark.asyncio + async def test_a_failure_during_creation_keeps_the_directory( + self, templates_dir, user, monkeypatch + ): + """The deliberate limit of the cleanup. Once creation starts, the + directory can be referenced by a container mount spec, so removing it + on a late failure would break a half-created agent instead of tidying + after one. Creation-path rollback is `crud.py`'s job.""" + monkeypatch.setattr(deploy_mod, "get_agents_by_prefix", lambda *a: [], raising=True) + monkeypatch.setattr(deploy_mod, "get_latest_version", lambda *a: None, raising=True) + monkeypatch.setattr(deploy_mod, "get_next_version_name", lambda n: n, raising=True) + monkeypatch.setattr(deploy_mod, "get_agent_quota_for_role", lambda r: 0, raising=True) + monkeypatch.setattr(deploy_mod.db, "get_agents_by_owner", lambda u: [], raising=True) + monkeypatch.setattr( + deploy_mod, "collect_mcp_credential_warnings", lambda p: [], raising=True + ) + monkeypatch.setattr( + deploy_mod, "_prepopulate_workspace_from_template", + lambda v, d: None, raising=True, + ) + + async def create_fn(config, *a, **kw): + raise HTTPException(status_code=500, detail="container create failed") + + body = _body(_archive("late-fail", VALID_MCP), "late-fail") + + with pytest.raises(HTTPException): + await _deploy(body, user, create_fn) + + assert (templates_dir / "late-fail").is_dir() + + +# --------------------------------------------------------------------------- +# Static: the gate must stay in front of the persist +# --------------------------------------------------------------------------- + +def test_the_guard_is_called_before_the_copy_in_source_order(): + """Pins the ORDERING itself, so a refactor that moves the guard back below + `copytree` fails here even if every behavioural test still passes on some + other path (it was the position, not the guard, that was wrong).""" + src = Path(deploy_mod.__file__).read_text() + body = src.split("async def deploy_local_agent_logic", 1)[1] + + guard = body.index("_validate_archive_mcp_config(") + copy = body.index("shutil.copytree(extract_root") + + assert guard < copy, ( + "the archive .mcp.json guard runs after the template is persisted — " + "that is #2006" + ) From 4c6d28b892f3d4adb3d3c07fb32fa5b114068f5f Mon Sep 17 00:00:00 2001 From: Oleksii Dolhov Date: Wed, 5 Aug 2026 11:29:19 +0300 Subject: [PATCH 2/2] fix(deploy): confine the partial-deploy cleanup at the sink (#2006) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeQL flagged the new `shutil.rmtree` as py/path-injection (high): the path descends from the caller's own `base_name`, and the #950 containment guard that confines it lives in the caller. Fixed rather than dismissed. On a destructive sink, "the caller already validated it" is a property that survives exactly until someone adds a second call site — and this helper is now reachable from two error paths. It re-normalizes and prefix-checks against the deployed-templates root before removing anything, refusing the root itself and any traversal escape, which is also the barrier shape CodeQL recognizes. Five tests, mutation-verified: neutering the check fails exactly the three refusal cases. Co-Authored-By: Claude Opus 5 (1M context) --- src/backend/services/agent_service/deploy.py | 22 ++++++++- tests/unit/test_2006_deploy_residue.py | 50 ++++++++++++++++++++ 2 files changed, 70 insertions(+), 2 deletions(-) diff --git a/src/backend/services/agent_service/deploy.py b/src/backend/services/agent_service/deploy.py index 04d33abe0..957e934ee 100644 --- a/src/backend/services/agent_service/deploy.py +++ b/src/backend/services/agent_service/deploy.py @@ -97,12 +97,30 @@ def _remove_partial_deploy(dest_created: Path | None) -> None: handle before creation starts, so a directory a container may already reference is never removed. Never raises — this runs on an error path and must not replace the real failure with a cleanup failure. + + The containment check is repeated HERE rather than inherited from the + caller's #950 guard. `rmtree` is a destructive sink whose path descends + from a caller-supplied name, and "the caller already validated it" is a + property that survives exactly until someone adds a second call site. It is + also the barrier CodeQL recognizes (normalize → prefix-check → use the + normalized value), which is why the flagged alert was fixed rather than + dismissed: on a `rmtree`, "probably confined" is not the standard. """ if dest_created is None: return + + base = os.path.normpath(DEPLOYED_TEMPLATES_DIR_IN_BACKEND) + target = os.path.normpath(str(dest_created)) + if not target.startswith(base + os.sep) or target == base: + logger.error( + "refusing to remove a partial deploy outside the deployed-templates " + "directory: %s", target, + ) + return + try: - shutil.rmtree(dest_created) - logger.info("Removed partial deploy directory: %s", dest_created) + shutil.rmtree(target) + logger.info("Removed partial deploy directory: %s", target) except Exception as e: # noqa: BLE001 — the original error is what matters logger.warning( "could not remove partial deploy directory %s: %s", dest_created, e diff --git a/tests/unit/test_2006_deploy_residue.py b/tests/unit/test_2006_deploy_residue.py index 3c440c6a1..a805f77ac 100644 --- a/tests/unit/test_2006_deploy_residue.py +++ b/tests/unit/test_2006_deploy_residue.py @@ -329,6 +329,56 @@ async def create_fn(config, *a, **kw): assert (templates_dir / "late-fail").is_dir() +class TestCleanupIsConfined: + """`_remove_partial_deploy` re-checks containment at the sink. + + Raised by CodeQL (py/path-injection, high) against the `rmtree`: the path + descends from a caller-supplied name, and the #950 guard that confines it + lives in the CALLER. Fixed rather than dismissed — on a destructive sink, + "the caller already validated it" holds only until there is a second call + site. + """ + + def test_a_path_outside_the_templates_dir_is_refused( + self, templates_dir, user, monkeypatch, tmp_path + ): + outside = tmp_path / "not-templates" / "precious" + outside.mkdir(parents=True) + (outside / "data.txt").write_text("do not delete me") + + deploy_mod._remove_partial_deploy(outside) + + assert (outside / "data.txt").exists() + + def test_the_templates_root_itself_is_refused(self, templates_dir, monkeypatch): + templates_dir.mkdir(parents=True) + (templates_dir / "other-agent").mkdir() + + deploy_mod._remove_partial_deploy(templates_dir) + + assert (templates_dir / "other-agent").exists() + + def test_a_traversal_escape_is_refused(self, templates_dir, tmp_path): + templates_dir.mkdir(parents=True) + sibling = tmp_path / "sibling" + sibling.mkdir() + + deploy_mod._remove_partial_deploy(templates_dir / ".." / "sibling") + + assert sibling.exists() + + def test_a_legitimate_child_is_still_removed(self, templates_dir): + child = templates_dir / "agent-v1" + child.mkdir(parents=True) + + deploy_mod._remove_partial_deploy(child) + + assert not child.exists() + + def test_none_is_a_no_op(self): + deploy_mod._remove_partial_deploy(None) # must not raise + + # --------------------------------------------------------------------------- # Static: the gate must stay in front of the persist # ---------------------------------------------------------------------------