From 598ad8fc96a3ee4e92a12b97b57d6f0b999a814d Mon Sep 17 00:00:00 2001 From: Tom Softreck Date: Sat, 31 Oct 2026 14:51:17 +0100 Subject: [PATCH] fix(mcp, ticket-112): validate receipt IDs before cleanup effects --- fixos/mcp_server.py | 25 +++++++++-- project/ticket-112/README.md | 34 +++++++++++++++ project/ticket-112/intent.json | 79 ++++++++++++++++++++++++++++++++++ tests/unit/test_mcp_server.py | 74 +++++++++++++++++++++++++++++++ 4 files changed, 209 insertions(+), 3 deletions(-) create mode 100644 project/ticket-112/README.md create mode 100644 project/ticket-112/intent.json diff --git a/fixos/mcp_server.py b/fixos/mcp_server.py index 7fc71e0..fcd9a56 100644 --- a/fixos/mcp_server.py +++ b/fixos/mcp_server.py @@ -13,6 +13,7 @@ import hashlib import json import os +import re import sys import uuid from collections.abc import Mapping @@ -26,6 +27,7 @@ CATALOG_SCHEMA = "wellmanifest.mcp/operation-catalog/v1" RECEIPT_SCHEMA = "wellmanifest.mcp/task-receipt/v1" PLAN_SCHEMA = "wellmanifest.nl-dsl-llm/task-plan/v1" +_REQUEST_ID_RE = re.compile(r"[A-Za-z0-9][A-Za-z0-9._-]{0,127}") OPERATIONS_CATALOG: tuple[dict[str, Any], ...] = ( { @@ -100,7 +102,12 @@ "dry_run": {"type": "boolean", "default": True}, "min_age_days": {"type": "number", "default": 7.0}, "include_projects": {"type": "boolean", "default": True}, - "request_id": {"type": "string", "default": ""}, + "request_id": { + "type": "string", + "default": "", + "maxLength": 128, + "description": "Opaque ID starting with a letter/digit, using letters, digits, dots, underscores or hyphens; no '..'. Empty generates an ID.", + }, }, "additionalProperties": False, }, @@ -390,10 +397,20 @@ def run_fixos_scan_projects(min_age_days: float = 7.0) -> dict[str, Any]: } +def _validated_request_id(request_id: str) -> str: + """Return a bounded portable receipt name, never a filesystem path.""" + clean_id = (request_id or "").strip() + if not _REQUEST_ID_RE.fullmatch(clean_id) or ".." in clean_id: + raise ValueError("invalid_request_id") + return clean_id + + def task_status(request_id: str, *, receipts_dir: str | Path | None = None) -> dict[str, Any]: """Read back a durable execution receipt without re-running any effect (wellmanifest/mcp).""" clean_id = (request_id or "").strip() - if not clean_id or "/" in clean_id or ".." in clean_id: + try: + clean_id = _validated_request_id(clean_id) + except ValueError: return { "schema": RECEIPT_SCHEMA, "found": False, @@ -430,7 +447,9 @@ def run_fixos_auto_cleanup( _cleanup_user_cache, ) - req_id = (request_id or "").strip() or f"fixos-mcp-{uuid.uuid4().hex[:12]}" + req_id = _validated_request_id( + (request_id or "").strip() or f"fixos-mcp-{uuid.uuid4().hex[:12]}" + ) existing = task_status(req_id, receipts_dir=receipts_dir) if existing.get("found"): return existing diff --git a/project/ticket-112/README.md b/project/ticket-112/README.md new file mode 100644 index 0000000..51e0b02 --- /dev/null +++ b/project/ticket-112/README.md @@ -0,0 +1,34 @@ +# Ticket 112: Reject unsafe MCP receipt request identifiers before cleanup + +- **ID**: ticket-112 +- **Owner**: agent:codex +- **Status**: IN_PROGRESS +- **Workflow state**: PUBLICATION +- **Created**: 2026-10-31 + +## Goal and scope + +A controlled dry-run with request_id `../escaped` created a receipt outside the +configured receipt directory. Reject traversal, absolute paths, unsafe portable +filenames and overlong identifiers before receipt lookup, scanning or cleanup. +Retain normal receipt replay and automatically generated identifiers. + +SESSION_EXECUTION_AUTHORIZATION: the user requested continuing FixOS repairs +and protected publication on 2026-10-31 after the earlier MCP repair. This is a +bounded correction to the same MCP cleanup interface. Prior publication actor +mismatches remain historical evidence and do not grant approval authority. + +## Acceptance criteria + +- [x] AC-01: Invalid request IDs fail before receipt directory access or cleanup, + for both dry-run and real execution; JSON-RPC reports an error. +- [x] AC-02: Valid IDs and generated IDs preserve receipt replay; checks pass. +- [ ] AC-03: Request independent validation and protected publication of the exact head. + +## Validation + +- A controlled dry-run reproduced receipt traversal outside the receipt root. +- New regression suite initially failed in 21 cases before the correction. +- Focused MCP checks: 36 passed; existing receipt replay remains valid. +- Ruff (including import ordering), diff whitespace and managed governance passed. +- Full governed tests and independent exact-head publication remain in the delivery process. diff --git a/project/ticket-112/intent.json b/project/ticket-112/intent.json new file mode 100644 index 0000000..c31852b --- /dev/null +++ b/project/ticket-112/intent.json @@ -0,0 +1,79 @@ +{ + "schema": "new-project.intent/v3", + "ticket": "ticket-112", + "summary": "Reject unsafe MCP receipt request identifiers before cleanup", + "workstream": "application", + "classification": { + "kind": "BUG", + "priority": "P1", + "origin": "requested" + }, + "allowedPaths": [ + "project/ticket-112/**", + "fixos/mcp_server.py", + "tests/unit/test_mcp_server.py" + ], + "forbiddenPaths": [ + "project/ticket-*/user-*.md" + ], + "stacks": [ + "python" + ], + "dependsOn": [], + "conflictsWith": [], + "integrationTicket": null, + "delivery": { + "acceptedBaseSha": "da980fac5189756b29d6c987c293c8e793014ad9", + "targetBranch": "main", + "outcome": "Publish strict receipt request-id validation before MCP cleanup scans or effects, preventing filesystem traversal and unsafe receipt filenames.", + "nonGoals": [ + "No cleanup policy changes", + "No changes to existing receipt contents or publication authority", + "No runtime dependencies or new MCP operations" + ], + "complexity": "S", + "estimatedMinutes": 30, + "budgets": { + "maxImplementationFiles": 2, + "maxAffectedComponents": 1, + "maxPublicInterfaceChanges": 1, + "maxRuntimeDependencies": 0 + }, + "architecture": { + "status": "accepted", + "decision": "Share one portable request-id validator between receipt readback and cleanup execution, validating before directory access or scans.", + "components": [ + { + "name": "mcp-receipts", + "paths": [ + "fixos/mcp_server.py", + "tests/unit/test_mcp_server.py" + ] + } + ], + "responsibilityChanges": false, + "interfaceChanges": [ + "Reject malformed request identifiers before any cleanup operation; keep normal and automatically generated identifiers valid." + ], + "dataChanges": [], + "ui": { + "impact": "none", + "states": [], + "evidence": [] + }, + "rollback": "Revert through protected publication; preserve receipt history." + }, + "runtimeDependencies": [], + "validation": [ + { + "criterion": "AC-01", + "commands": [ + "python -m pytest tests/unit/test_mcp_server.py -q", + "python -m ruff check --select I fixos/mcp_server.py tests/unit/test_mcp_server.py", + "./project/governance-check.sh" + ], + "evidence": "project/ticket-112/README.md" + } + ] + } +} diff --git a/tests/unit/test_mcp_server.py b/tests/unit/test_mcp_server.py index 305f8a0..6f50802 100644 --- a/tests/unit/test_mcp_server.py +++ b/tests/unit/test_mcp_server.py @@ -110,6 +110,80 @@ def test_task_execute_rejects_mismatched_digest(): task_execute("fixos://diagnostics/quick", {}, registry_digest="0" * 64) +@pytest.mark.parametrize("dry_run", [True, False]) +@pytest.mark.parametrize( + "request_id", + ["../escaped", "/absolute", r"..\escaped", "nested/id", "C:escaped", "bad\x00id", "..", ".hidden", "bad id", "x" * 129], +) +def test_cleanup_rejects_unsafe_request_ids_before_any_effect( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, request_id: str, dry_run: bool +): + def unexpected_access(*args, **kwargs): + pytest.fail("Invalid request ID reached receipt access or cleanup scanning") + + for name in ("_receipts_dir", "scan_user_cache_dirs", "scan_additional_safe_caches", "scan_build_and_package_caches"): + monkeypatch.setattr(f"fixos.mcp_server.{name}", unexpected_access) + + with pytest.raises(ValueError, match="invalid_request_id"): + task_execute( + "fixos://cleanup/auto", + {"dry_run": dry_run, "include_projects": False}, + request_id=request_id, + receipts_dir=tmp_path / "receipts", + ) + assert list(tmp_path.iterdir()) == [] + + +def test_jsonrpc_reports_unsafe_receipt_id_without_creating_files( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +): + monkeypatch.setattr("fixos.mcp_server.scan_user_cache_dirs", list) + monkeypatch.setattr("fixos.mcp_server.scan_additional_safe_caches", list) + monkeypatch.setattr("fixos.mcp_server.scan_build_and_package_caches", list) + response = handle_jsonrpc_message( + { + "jsonrpc": "2.0", + "id": 1, + "method": "tools/call", + "params": { + "name": "fixos_auto_cleanup", + "arguments": {"request_id": "../escaped", "include_projects": False}, + }, + }, + receipts_dir=tmp_path / "receipts", + ) + assert response["result"]["isError"] is True + assert response["result"]["structuredContent"]["error"] == "invalid_request_id" + assert list(tmp_path.iterdir()) == [] + + +@pytest.mark.parametrize("request_id", ["", "../escaped", r"nested\id", "C:escaped", "bad\x00id", "x" * 129]) +def test_status_rejects_unsafe_ids_without_accessing_receipts( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, request_id: str +): + def unexpected_access(*args, **kwargs): + pytest.fail("Invalid status request reached receipt directory access") + + monkeypatch.setattr("fixos.mcp_server._receipts_dir", unexpected_access) + result = task_status(request_id, receipts_dir=tmp_path / "receipts") + assert result["found"] is False + assert result["error"] == "invalid_request_id" + assert list(tmp_path.iterdir()) == [] + + +def test_generated_receipt_id_remains_replayable(tmp_path: Path, monkeypatch: pytest.MonkeyPatch): + monkeypatch.setattr("fixos.mcp_server.scan_user_cache_dirs", list) + monkeypatch.setattr("fixos.mcp_server.scan_additional_safe_caches", list) + monkeypatch.setattr("fixos.mcp_server.scan_build_and_package_caches", list) + result = task_execute( + "fixos://cleanup/auto", + {"dry_run": True, "include_projects": False}, + receipts_dir=tmp_path, + ) + assert result["request_id"].startswith("fixos-mcp-") + assert task_status(result["request_id"], receipts_dir=tmp_path)["receipt_hash"] == result["receipt_hash"] + + def test_jsonrpc_protocol_roundtrip(tmp_path: Path, monkeypatch: pytest.MonkeyPatch): monkeypatch.setattr( "fixos.diagnostics.quick_snapshot.collect_quick_snapshot",