diff --git a/src/activity_core/bounded_operations.py b/src/activity_core/bounded_operations.py index ba9c9b1..32a97b2 100644 --- a/src/activity_core/bounded_operations.py +++ b/src/activity_core/bounded_operations.py @@ -119,14 +119,22 @@ def normalize_operation_result(operation_id: str, raw: Any) -> dict[str, Any]: else 0, } if operation_id == "cnpg_option_a_backup": + overall = result.get("overall") + if not isinstance(overall, str) or overall not in { + "ok", "degraded", "error", "pass", "fail", "unknown" + }: + overall = "unknown" + exit_code = result.get("script_exit_code", 0) + if isinstance(exit_code, bool) or not isinstance(exit_code, int): + exit_code = 1 return { "kind": operation_id, - "overall": str(result.get("overall") or "unknown")[:40], + "overall": overall, "dry_run": bool(result.get("dry_run")), "dumped": _safe_count(result.get("dumped")), "uploaded": _safe_count(result.get("uploaded")), "failed": _safe_count(result.get("failed")), - "script_exit_code": _safe_count(result.get("script_exit_code")), + "script_exit_code": exit_code, } raise ValueError(f"no result projection for bounded operation {operation_id!r}") diff --git a/src/activity_core/context_resolvers/cnpg_backup.py b/src/activity_core/context_resolvers/cnpg_backup.py index e1e39a2..51e91f2 100644 --- a/src/activity_core/context_resolvers/cnpg_backup.py +++ b/src/activity_core/context_resolvers/cnpg_backup.py @@ -3,13 +3,12 @@ from __future__ import annotations import json -import logging import os import subprocess from pathlib import Path from typing import Any -logger = logging.getLogger(__name__) +from activity_core.bounded_operations import normalize_operation_result _DEFAULT_SCRIPT = Path("/opt/railiance-platform/tools/cmd/cnpg-option-a-backup") _DEFAULT_TIMEOUT_SECONDS = 7200 @@ -19,7 +18,7 @@ def cnpg_option_a_backup(params: dict[str, Any]) -> dict[str, Any]: """Run multi-host Option A backup CLI and return its JSON summary.""" script = Path(str(params.get("backup_script", _DEFAULT_SCRIPT))).expanduser() if not script.is_file(): - raise FileNotFoundError(f"cnpg_option_a_backup script not found: {script}") + raise FileNotFoundError("cnpg_option_a_backup script not found") dry_run = bool(params.get("dry_run", False)) timeout = float(params.get("timeout_seconds", _DEFAULT_TIMEOUT_SECONDS)) @@ -63,34 +62,37 @@ def cnpg_option_a_backup(params: dict[str, Any]) -> dict[str, Any]: env["CNPG_BACKUP_TARGETS"] = str(targets) cmd = [str(script)] - completed = subprocess.run( - cmd, - capture_output=True, - text=True, - timeout=timeout, - check=False, - env=env, - ) - stdout = (completed.stdout or "").strip() - stderr = (completed.stderr or "").strip() - if completed.returncode not in (0, 2) and not stdout: - raise RuntimeError( - f"cnpg_option_a_backup failed (exit {completed.returncode}): {stderr[:500]}" + try: + completed = subprocess.run( + cmd, + capture_output=True, + text=True, + timeout=timeout, + check=False, + env=env, ) + except subprocess.TimeoutExpired: + raise RuntimeError("cnpg_option_a_backup timed out") from None + except OSError: + raise RuntimeError("cnpg_option_a_backup could not start") from None + except UnicodeError: + raise RuntimeError("cnpg_option_a_backup returned invalid output encoding") from None + + stdout = (completed.stdout or "").strip() + if completed.returncode not in (0, 2) and not stdout: + raise RuntimeError(f"cnpg_option_a_backup failed (exit {completed.returncode})") try: summary = json.loads(stdout) - except json.JSONDecodeError as exc: - raise RuntimeError( - f"cnpg_option_a_backup returned invalid JSON: {exc}; stderr={stderr[:300]}" - ) from exc + except json.JSONDecodeError: + raise RuntimeError("cnpg_option_a_backup returned invalid JSON") from None + if not isinstance(summary, dict): + raise TypeError("cnpg_option_a_backup returned a non-object summary") if completed.returncode not in (0, 2): - summary.setdefault("errors", []).append( - f"script_exit_code:{completed.returncode}" - ) - summary["kind"] = "cnpg_option_a_backup" + # A failed process cannot report a successful backup, even with valid JSON. + summary["overall"] = "error" summary["script_exit_code"] = completed.returncode - if stderr: - summary["log_tail"] = stderr[-1500:] - return summary + # Sanitize at the adapter boundary, before any caller can log or serialize it. + # Raw targets, errors, stderr and provider payloads never leave this function. + return normalize_operation_result("cnpg_option_a_backup", summary) diff --git a/tests/test_bounded_operations.py b/tests/test_bounded_operations.py index 9f4d36e..bbdf003 100644 --- a/tests/test_bounded_operations.py +++ b/tests/test_bounded_operations.py @@ -165,3 +165,15 @@ def test_definition_cannot_combine_two_bounded_operations() -> None: with pytest.raises(ValueError, match="at most one bounded operation"): validate_bounded_operations([prune, backup], []) + + +@pytest.mark.parametrize("overall", ["sensitive-value", {"secret": "value"}, ["value"], None]) +def test_backup_projection_does_not_copy_untrusted_status(overall): + result = normalize_operation_result("cnpg_option_a_backup", {"overall": overall}) + assert result["overall"] == "unknown" + + +@pytest.mark.parametrize("overall", ["ok", "degraded", "error", "pass", "fail", "unknown"]) +def test_backup_projection_preserves_supported_status(overall): + result = normalize_operation_result("cnpg_option_a_backup", {"overall": overall}) + assert result["overall"] == overall diff --git a/tests/test_cnpg_option_a_backup_resolver.py b/tests/test_cnpg_option_a_backup_resolver.py index e0d3fd3..7fa3569 100644 --- a/tests/test_cnpg_option_a_backup_resolver.py +++ b/tests/test_cnpg_option_a_backup_resolver.py @@ -3,11 +3,12 @@ from __future__ import annotations import json +import subprocess +import traceback from pathlib import Path from unittest.mock import MagicMock, patch import pytest - from activity_core.context_resolvers.cnpg_backup import cnpg_option_a_backup @@ -60,3 +61,77 @@ def test_degraded_exit_2_still_parses(tmp_path: Path) -> None: result = cnpg_option_a_backup({"backup_script": str(script)}) assert result["overall"] == "degraded" assert result["script_exit_code"] == 2 + + +CANARY = "synthetic-sensitive-subprocess-value-83917" + + +def assert_safe_failure(error): + from temporalio.api.failure.v1 import Failure + from temporalio.converter import DefaultFailureConverter, DefaultPayloadConverter + + assert CANARY not in "".join(traceback.format_exception(error)) + failure = Failure() + DefaultFailureConverter().to_failure(error, DefaultPayloadConverter(), failure) + assert CANARY.encode() not in failure.SerializeToString() + + +@pytest.mark.parametrize("failure,expected", [ + (subprocess.TimeoutExpired([CANARY], 5, output=CANARY, stderr=CANARY), "timed out"), + (OSError(CANARY), "could not start"), + (UnicodeDecodeError("utf-8", CANARY.encode(), 0, 1, CANARY), "invalid output encoding"), +]) +def test_subprocess_exceptions_are_safe_in_temporal(tmp_path, failure, expected): + script = tmp_path / "backup" + script.touch() + with ( + patch("activity_core.context_resolvers.cnpg_backup.subprocess.run", side_effect=failure), + pytest.raises(RuntimeError, match=expected) as exc, + ): + cnpg_option_a_backup({"backup_script": str(script)}) + assert_safe_failure(exc.value) + + +@pytest.mark.parametrize("stdout,exit_code,expected,error_type", [ + ("", 1, "failed ", RuntimeError), + (CANARY, 0, "invalid JSON", RuntimeError), + (json.dumps([CANARY]), 0, "non-object summary", TypeError), + ("null", 0, "non-object summary", TypeError), + (json.dumps(CANARY), 0, "non-object summary", TypeError), +]) +def test_bad_output_never_reaches_exception_evidence(tmp_path, stdout, exit_code, expected, error_type): + script = tmp_path / "backup" + script.touch() + completed = MagicMock(returncode=exit_code, stdout=stdout, stderr=CANARY) + with ( + patch("activity_core.context_resolvers.cnpg_backup.subprocess.run", return_value=completed), + pytest.raises(error_type, match=expected) as exc, + ): + cnpg_option_a_backup({"backup_script": str(script)}) + assert_safe_failure(exc.value) + + +@pytest.mark.parametrize("overall", [CANARY, {"secret": CANARY}, [CANARY]]) +def test_successful_json_is_allowlisted_before_return(tmp_path, overall): + script = tmp_path / "backup" + script.touch() + payload = {"overall": overall, "dry_run": False, "dumped": 2, "uploaded": 2, + "failed": 0, "targets": [CANARY], "errors": [CANARY], "log_tail": CANARY} + completed = MagicMock(returncode=0, stdout=json.dumps(payload), stderr=CANARY) + with patch("activity_core.context_resolvers.cnpg_backup.subprocess.run", return_value=completed): + result = cnpg_option_a_backup({"backup_script": str(script)}) + assert result == {"kind": "cnpg_option_a_backup", "overall": "unknown", "dry_run": False, + "dumped": 2, "uploaded": 2, "failed": 0, "script_exit_code": 0} + assert CANARY not in json.dumps(result) + + +@pytest.mark.parametrize("exit_code", [1, -9]) +def test_process_failure_cannot_claim_success(tmp_path, exit_code): + script = tmp_path / "backup" + script.touch() + completed = MagicMock(returncode=exit_code, stdout=json.dumps({"overall": "ok"}), stderr=CANARY) + with patch("activity_core.context_resolvers.cnpg_backup.subprocess.run", return_value=completed): + result = cnpg_option_a_backup({"backup_script": str(script)}) + assert result["overall"] == "error" + assert result["script_exit_code"] == exit_code + assert CANARY not in json.dumps(result) diff --git a/workplans/ACTIVITY-WP-0042-platform-backup-restore-evidence-scheduling.md b/workplans/ACTIVITY-WP-0042-platform-backup-restore-evidence-scheduling.md index 0a9df19..5f70593 100644 --- a/workplans/ACTIVITY-WP-0042-platform-backup-restore-evidence-scheduling.md +++ b/workplans/ACTIVITY-WP-0042-platform-backup-restore-evidence-scheduling.md @@ -178,3 +178,34 @@ focused recovery-contract/bounded-operation suite **43 passed**; Ruff and `git diff --check` clean. No runtime wiring, new operation registration, credential fetch, or live restore was performed. Decision receipt: `5ec92a02-cb0b-466d-9aaf-9da5ffc6c92d` (codex scope clarification; D2 unchanged). + + +## Harden the existing backup evidence boundary + +```task +id: ACTIVITY-WP-0042-T06 +status: done +priority: high +``` + +User requested the follow-up backup error-handling fix. Enforce ACT-ADR-007's +existing non-secret evidence constraint and Emergent Security C2/C6/C9 without +changing execution scope: discard raw subprocess stderr/targets/errors at the +adapter boundary, suppress unsafe exception causes, and allowlist status values. +Exercise malformed output, timeout/launch/decode failures, failure exit codes, +and Temporal failure serialization with synthetic secret canaries. Preserve +normal/degraded backup summaries and surface negative signal exit codes. + +Decision class: task-scoped code/tests; delegation: explicit user task and +Emergent Security A6; no credentials, live effects, or policy widening. + + +T06 result: the resolver returns the existing allowlisted evidence shape directly; +raw output never escapes in returned results or serialized Temporal failures. +Known status values are preserved; unknown status content becomes `unknown`. +Unexpected exit codes cannot claim success, and negative signal codes survive. +Validation: **676 passed, 1 skipped** in the full suite; final focused backup, +bounded-operation and evidence-sink checks **48 passed**. Ruff baseline comparison +shows no introduced findings (five pre-existing findings remain across the +changed files); `git diff --check` passes. This is source hardening only, not a +worker rollout or execution-authority change.