diff --git a/engine/cli.py b/engine/cli.py index 2211da0..b1f3412 100644 --- a/engine/cli.py +++ b/engine/cli.py @@ -157,7 +157,7 @@ def cmd_info(args: argparse.Namespace, engine: types.SimpleNamespace) -> None: def cmd_scrub(args: argparse.Namespace, engine: types.SimpleNamespace) -> None: """Scrub PII from a capture.""" from engine.review import ReviewStatus, transition_status - from engine.scrubber import Scrubber, ScrubLevel + from engine.scrubber import Scrubber, ScrubbingUnavailableError, ScrubLevel cap = engine.db.get_capture(args.capture_id) if not cap: @@ -165,7 +165,13 @@ def cmd_scrub(args: argparse.Namespace, engine: types.SimpleNamespace) -> None: sys.exit(1) scrubber = Scrubber(level=ScrubLevel(args.level)) - scrubbed_path = scrubber.scrub_capture(Path(cap["capture_path"])) + try: + scrubbed_path = scrubber.scrub_capture(Path(cap["capture_path"])) + except ScrubbingUnavailableError as exc: + # Exit non-zero and leave the capture in CAPTURED: an operator script + # must not read "scrubbed" from a scrub that never happened. + print(f"Scrub refused: {exc}") + sys.exit(1) transition_status( args.capture_id, diff --git a/engine/dispatch.py b/engine/dispatch.py index 94201f2..339730d 100644 --- a/engine/dispatch.py +++ b/engine/dispatch.py @@ -2299,7 +2299,7 @@ def get_capabilities(self, **params: Any) -> dict: def scrub_capture(self, **params: Any) -> dict: """Scrub PII from a capture and advance its review state.""" from engine.review import ReviewStatus, transition_status - from engine.scrubber import Scrubber, ScrubLevel + from engine.scrubber import Scrubber, ScrubbingUnavailableError, ScrubLevel capture_id = params.get("capture_id") capture = capture_id and self.services.db.get_capture(capture_id) @@ -2308,7 +2308,16 @@ def scrub_capture(self, **params: Any) -> dict: capture_id = str(capture_id) level = params.get("level", "basic") scrubber = Scrubber(level=ScrubLevel(level)) - scrubbed = scrubber.scrub_capture(Path(capture["capture_path"])) + try: + scrubbed = scrubber.scrub_capture(Path(capture["capture_path"])) + except ScrubbingUnavailableError as exc: + # The scrub could not run. The capture stays CAPTURED, which the + # review state machine already blocks from every egress path. + logger.warning("scrub_capture refused for {c}: {e}", c=capture_id, e=exc) + self.services.audit.log( + "scrub_refused", capture_id=capture_id, level=level, reason=str(exc) + ) + return {"ok": False, "error": str(exc)} transition_status( capture_id, ReviewStatus.CAPTURED, diff --git a/engine/scrubber.py b/engine/scrubber.py index 4b7a27a..eab878f 100644 --- a/engine/scrubber.py +++ b/engine/scrubber.py @@ -17,6 +17,12 @@ review_status.json User approvals/rejections The scrub_manifest.json enables the review UI to show exactly what changed. + +Scrubbing is fail-closed, like every other gate in this engine. When the +requested level cannot actually run, the scrubber raises +:class:`ScrubbingUnavailableError` rather than copying the input through and +reporting zero redactions. An empty redaction list means the scrubber ran and +found nothing; it never means the scrubber was absent. """ from __future__ import annotations @@ -47,6 +53,37 @@ class ScrubLevel(enum.Enum): ENHANCED = "enhanced" +# A probe string carrying one entity every Presidio configuration recognises. +# Scrubbing it proves the recognizer pipeline actually loads, which importing +# the provider class does NOT: `openadapt_privacy.providers.presidio` imports +# fine without Presidio/spaCy installed and only fails when asked to work. +_PROVIDER_PROBE_TEXT = "Contact probe@example.com" + + +class ScrubbingUnavailableError(RuntimeError): + """The requested scrub level could not be applied on this machine. + + Raised instead of copying the input through unredacted and returning an + empty redaction list. "The scrubber ran and found no PII" and "the + scrubber could not run" must never be the same observable result: the + first clears a capture for review and egress, the second must not. + + A capture whose scrub raises this never reaches ``SCRUBBED``, so it stays + blocked from every outbound path by the review state machine. + """ + + def __init__(self, level: ScrubLevel, cause: BaseException) -> None: + super().__init__( + f"{level.value} scrubbing is unavailable on this machine " + f"({type(cause).__name__}: {cause}). Nothing was scrubbed and no " + "scrubbed copy was produced. Install the openadapt-privacy " + "Presidio extras, or re-run at the 'basic' level and accept that " + "screenshots are copied without image redaction." + ) + self.level = level + self.cause = cause + + class Scrubber: """Orchestrates PII scrubbing of capture sessions. @@ -74,10 +111,18 @@ def scrub_capture(self, capture_path: Path) -> Path: Raises: FileNotFoundError: If the capture directory does not exist. + ScrubbingUnavailableError: If the requested level needs Presidio + and Presidio cannot run here. Raised BEFORE anything is + written, so a failed scrub never leaves a partial + ``.scrubbed/`` directory that reads as a completed one. """ if not capture_path.exists(): raise FileNotFoundError(f"Capture directory not found: {capture_path}") + # Preflight: prove the provider works before creating any output. + if self.level != ScrubLevel.BASIC: + self._require_provider() + scrubbed_path = capture_path.parent / (capture_path.name + ".scrubbed") scrubbed_path.mkdir(parents=True, exist_ok=True) @@ -127,6 +172,30 @@ def scrub_capture(self, capture_path: Path) -> Path: return scrubbed_path + def _require_provider(self): # noqa: ANN202 - provider type is optional dep + """Return a Presidio provider PROVEN able to run, or refuse. + + Importing the provider is not evidence that it works: the module + imports cleanly without Presidio/spaCy and only raises when asked to + scrub. So construct it and scrub a probe string before trusting it. + + Raises: + ScrubbingUnavailableError: If the provider cannot be imported, + constructed, or exercised. + """ + if self._provider is None: + try: + from openadapt_privacy.providers.presidio import ( + PresidioScrubbingProvider, + ) + + provider = PresidioScrubbingProvider() + provider.scrub_text(_PROVIDER_PROBE_TEXT) + except Exception as exc: + raise ScrubbingUnavailableError(self.level, exc) from exc + self._provider = provider + return self._provider + def scrub_text(self, text: str) -> tuple[str, list[dict]]: """Scrub PII from a text string. @@ -135,6 +204,10 @@ def scrub_text(self, text: str) -> tuple[str, list[dict]]: Returns: Tuple of (scrubbed text, list of redaction records). + + Raises: + ScrubbingUnavailableError: At standard/enhanced level, when + Presidio cannot run here. """ if self.level == ScrubLevel.BASIC: return self._scrub_text_regex(text) @@ -150,28 +223,30 @@ def scrub_image(self, image_path: Path, output_path: Path) -> list[dict]: Returns: List of redacted region records. + + Raises: + ScrubbingUnavailableError: At standard/enhanced level, when image + redaction could not run. ``output_path`` is left absent: an + unredacted screenshot must never appear inside a ``.scrubbed/`` + directory, and an empty redaction list must only ever mean + "redaction ran and changed nothing". """ if self.level == ScrubLevel.BASIC: - # Basic level: no image scrubbing, just copy + # Basic level: no image scrubbing, just copy. Declared by the + # level the caller chose and recorded in the scrub manifest. shutil.copy2(image_path, output_path) return [] - # Standard/Enhanced: try openadapt-privacy + provider = self._require_provider() try: - from openadapt_privacy.providers.presidio import PresidioScrubbingProvider - - if self._provider is None: - self._provider = PresidioScrubbingProvider() - from PIL import Image + img = Image.open(image_path) - scrubbed_img = self._provider.scrub_image(img) + scrubbed_img = provider.scrub_image(img) scrubbed_img.save(output_path) - return [{"type": "image_scrub", "path": str(output_path)}] - except ImportError: - # Fallback: copy without scrubbing - shutil.copy2(image_path, output_path) - return [] + except Exception as exc: + raise ScrubbingUnavailableError(self.level, exc) from exc + return [{"type": "image_scrub", "path": str(output_path)}] def _scrub_text_regex(self, text: str) -> tuple[str, list[dict]]: """Scrub PII using regex patterns only (basic level). @@ -212,23 +287,24 @@ def _scrub_text_presidio(self, text: str) -> tuple[str, list[dict]]: Returns: Tuple of (scrubbed text, list of redaction records). + + Raises: + ScrubbingUnavailableError: If Presidio cannot run here. It does + NOT silently fall back to the regex pass: the manifest records + the level the caller asked for, so a regex result labelled + ``enhanced`` would overstate what was checked. """ + provider = self._require_provider() try: - from openadapt_privacy.providers.presidio import PresidioScrubbingProvider - - if self._provider is None: - self._provider = PresidioScrubbingProvider() - - scrubbed = self._provider.scrub_text(text) - # Build redaction records by diffing original vs scrubbed - redactions: list[dict] = [] - if scrubbed != text: - redactions.append({ - "entity": "PRESIDIO_DETECTED", - "original_length": len(text), - "scrubbed_length": len(scrubbed), - }) - return scrubbed, redactions - except ImportError: - # Fallback to regex if presidio not available - return self._scrub_text_regex(text) + scrubbed = provider.scrub_text(text) + except Exception as exc: + raise ScrubbingUnavailableError(self.level, exc) from exc + # Build redaction records by diffing original vs scrubbed + redactions: list[dict] = [] + if scrubbed != text: + redactions.append({ + "entity": "PRESIDIO_DETECTED", + "original_length": len(text), + "scrubbed_length": len(scrubbed), + }) + return scrubbed, redactions diff --git a/tests/test_engine/test_dispatch.py b/tests/test_engine/test_dispatch.py index d94e878..67283b5 100644 --- a/tests/test_engine/test_dispatch.py +++ b/tests/test_engine/test_dispatch.py @@ -1415,3 +1415,38 @@ def test_all_frontend_commands_registered(self, deps) -> None: "get_pending_reviews", } assert expected.issubset(set(disp.commands)) + + +class TestScrubCaptureRefusal: + """A scrub that could not run must not advance the egress gate.""" + + def test_unavailable_scrubber_reports_failure_and_holds_state( + self, deps, tmp_path: Path, monkeypatch, + ) -> None: + """Missing Presidio -> ok False, capture stays CAPTURED, no scrubbed copy. + + Before this was fixed the scrubber copied every screenshot through + unredacted, reported zero redactions, and the capture advanced to + SCRUBBED -- one operator approval away from leaving the machine. + """ + import sys + + disp, db, _events = deps + capture_dir = tmp_path / "cap-enhanced" + (capture_dir / "screenshots").mkdir(parents=True) + from PIL import Image + + Image.new("RGB", (8, 8), "white").save(capture_dir / "screenshots" / "0001.png") + db.insert_capture("capE", str(capture_dir), "2026-07-27T09:00:00Z") + + # The shipped default: openadapt-privacy is installed, Presidio is not. + monkeypatch.setitem(sys.modules, "openadapt_privacy.providers.presidio", None) + + result = disp.dispatch("scrub_capture", {"capture_id": "capE", "level": "enhanced"}) + + assert result["ok"] is False + assert "unavailable" in result["error"] + assert "scrubbed_path" not in result + row = db.get_capture("capE") + assert row["review_status"] == "captured" + assert not row["scrubbed_path"] diff --git a/tests/test_engine/test_scrubber.py b/tests/test_engine/test_scrubber.py index d9be8eb..6041e5d 100644 --- a/tests/test_engine/test_scrubber.py +++ b/tests/test_engine/test_scrubber.py @@ -2,11 +2,128 @@ from __future__ import annotations +import json +import sys +import types from pathlib import Path import pytest -from engine.scrubber import Scrubber, ScrubLevel +from engine.scrubber import Scrubber, ScrubbingUnavailableError, ScrubLevel + +_PRESIDIO_MODULE = "openadapt_privacy.providers.presidio" + + +def _png(path: Path) -> Path: + """Write the smallest valid PNG PIL will open.""" + from PIL import Image + + path.parent.mkdir(parents=True, exist_ok=True) + Image.new("RGB", (8, 8), "white").save(path) + return path + + +def _capture_with_screenshot(root: Path) -> Path: + """A capture directory holding one screenshot.""" + capture = root / "2026-07-27_09-00-00_pii" + (capture / "screenshots").mkdir(parents=True) + _png(capture / "screenshots" / "0001.png") + (capture / "meta.json").write_text(json.dumps({"task_description": "a@b.com"})) + return capture + + +def _break_presidio(monkeypatch: pytest.MonkeyPatch) -> None: + """Make the Presidio provider unimportable, as on a stock install. + + ``openadapt-privacy`` is a hard dependency but Presidio and spaCy are not, + so this is the shipped default rather than an exotic environment. + """ + monkeypatch.setitem(sys.modules, _PRESIDIO_MODULE, None) + + +def _presidio_that_cannot_run(monkeypatch: pytest.MonkeyPatch) -> None: + """Provider that imports and constructs but fails when asked to work. + + This is the real observed shape: the module imports without spaCy and only + raises ``ModuleNotFoundError`` (an ``ImportError``) from ``scrub_*``. + """ + + class _Provider: + def scrub_text(self, text: str) -> str: + raise ModuleNotFoundError("No module named 'spacy'") + + def scrub_image(self, image: object) -> object: + raise ModuleNotFoundError("No module named 'spacy'") + + module = types.ModuleType(_PRESIDIO_MODULE) + module.PresidioScrubbingProvider = _Provider # type: ignore[attr-defined] + monkeypatch.setitem(sys.modules, _PRESIDIO_MODULE, module) + + +class TestScrubUnavailableIsNotCleanResult: + """"Could not scrub" must never look like "scrubbed and found nothing".""" + + @pytest.mark.parametrize("level", [ScrubLevel.STANDARD, ScrubLevel.ENHANCED]) + def test_scrub_image_refuses_and_writes_no_output( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch, level: ScrubLevel, + ) -> None: + """No provider -> refuse, and leave no unredacted file behind.""" + _break_presidio(monkeypatch) + source = _png(tmp_path / "in" / "shot.png") + output = tmp_path / "out" / "shot.png" + output.parent.mkdir() + + with pytest.raises(ScrubbingUnavailableError): + Scrubber(level=level).scrub_image(source, output) + + assert not output.exists(), ( + "an unredacted screenshot was copied into the scrubbed output" + ) + + def test_scrub_image_refuses_when_provider_cannot_run( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch, + ) -> None: + """Importable-but-broken Presidio refuses too, not just a missing one.""" + _presidio_that_cannot_run(monkeypatch) + source = _png(tmp_path / "in" / "shot.png") + output = tmp_path / "out" / "shot.png" + output.parent.mkdir() + + with pytest.raises(ScrubbingUnavailableError): + Scrubber(level=ScrubLevel.ENHANCED).scrub_image(source, output) + + assert not output.exists() + + def test_scrub_capture_refuses_before_writing_anything( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch, + ) -> None: + """A refused scrub leaves no directory that reads as a completed one.""" + _break_presidio(monkeypatch) + capture = _capture_with_screenshot(tmp_path) + + with pytest.raises(ScrubbingUnavailableError): + Scrubber(level=ScrubLevel.ENHANCED).scrub_capture(capture) + + scrubbed = capture.parent / (capture.name + ".scrubbed") + assert not (scrubbed / "scrub_manifest.json").exists(), ( + "a manifest reporting zero redactions was written for a scrub " + "that never ran" + ) + assert not (scrubbed / "review_status.json").exists() + assert not (scrubbed / "screenshots" / "0001.png").exists() + + def test_scrub_text_does_not_silently_downgrade_to_regex( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch, + ) -> None: + """Enhanced text scrubbing refuses rather than returning regex output. + + A regex result labelled ``enhanced`` in the manifest overstates what + was checked: it never looked for names, locations, or dates. + """ + _break_presidio(monkeypatch) + + with pytest.raises(ScrubbingUnavailableError): + Scrubber(level=ScrubLevel.ENHANCED).scrub_text("Jane Doe, a@b.com") class TestScrubber: