From dcb5f7ddef8549dadca630b7e774736f37cbe3f1 Mon Sep 17 00:00:00 2001 From: tegwick Date: Sat, 3 Oct 2026 14:57:31 +0200 Subject: [PATCH] Keep backup subprocess output out of workflow evidence Sanitize failures and returned summaries at the adapter boundary. Allowlist backup status values and preserve failure exit codes. Verify synthetic sensitive output cannot survive Python traceback or Temporal failure serialization. Decision-Class: strengthen-existing-evidence-boundary Delegation: explicit-user-task; Emergent-Security-A6 Evidence: full-suite-676-passed-1-skipped; final-focused-48-passed; no-new-lint-findings Assistant: codex Assistant-Model: gpt-6-astra Assistant-Session: 01a100de-2e1c-7e23-9ae9-2da0e0499411 --- src/activity_core/bounded_operations.py | 12 ++- .../context_resolvers/cnpg_backup.py | 56 +++++++------- tests/test_bounded_operations.py | 12 +++ tests/test_cnpg_option_a_backup_resolver.py | 77 ++++++++++++++++++- ...form-backup-restore-evidence-scheduling.md | 31 ++++++++ 5 files changed, 158 insertions(+), 30 deletions(-) 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.