From 74826d036985b8fc49f22daecbf00b8cf2c0fbee Mon Sep 17 00:00:00 2001 From: Antonio Maiolo Date: Mon, 27 Apr 2026 15:25:13 +0200 Subject: [PATCH 01/13] fix(ocr): refuse decompression-bomb images (NDS-014) Set Pillow MAX_IMAGE_PIXELS to ~50 MP and convert DecompressionBombWarning to an error so a malicious image-block can never trigger huge memory allocations. The dispatcher converts the raised ValueError into an extraction_error and skips the block. --- src/noirdoc/file_analysis/extractors/ocr.py | 21 +++++++++++++++-- tests/file_analysis/test_extractor.py | 26 +++++++++++++++++++++ 2 files changed, 45 insertions(+), 2 deletions(-) diff --git a/src/noirdoc/file_analysis/extractors/ocr.py b/src/noirdoc/file_analysis/extractors/ocr.py index bcf9016..0345512 100644 --- a/src/noirdoc/file_analysis/extractors/ocr.py +++ b/src/noirdoc/file_analysis/extractors/ocr.py @@ -3,12 +3,17 @@ from __future__ import annotations import io +import warnings from typing import TYPE_CHECKING if TYPE_CHECKING: from PIL.Image import Image as PILImage _MAX_DIM = 4096 +# Pillow emits DecompressionBombWarning above MAX_IMAGE_PIXELS and +# raises DecompressionBombError above 2x. We treat both as fatal so a +# malicious input cannot trigger huge memory allocations. +_MAX_IMAGE_PIXELS = 50_000_000 def ocr_image(img: PILImage, *, lang: str = "deu+eng") -> str: @@ -26,8 +31,20 @@ def ocr_image(img: PILImage, *, lang: str = "deu+eng") -> str: def extract_ocr(data: bytes, *, lang: str = "deu+eng") -> str: - """Run Tesseract OCR on an image byte-string.""" + """Run Tesseract OCR on an image byte-string. + + Raises ``ValueError`` if the image exceeds the decompression-bomb + threshold; the caller treats this as an extraction failure. + """ from PIL import Image - img = Image.open(io.BytesIO(data)) + Image.MAX_IMAGE_PIXELS = _MAX_IMAGE_PIXELS + try: + with warnings.catch_warnings(): + warnings.simplefilter("error", Image.DecompressionBombWarning) + img = Image.open(io.BytesIO(data)) + img.load() + except (Image.DecompressionBombError, Image.DecompressionBombWarning) as exc: + raise ValueError(f"image refused: decompression bomb suspected ({exc})") from exc + return ocr_image(img, lang=lang) diff --git a/tests/file_analysis/test_extractor.py b/tests/file_analysis/test_extractor.py index 3f3ad8b..22d634a 100644 --- a/tests/file_analysis/test_extractor.py +++ b/tests/file_analysis/test_extractor.py @@ -71,3 +71,29 @@ async def test_corrupt_data_sets_error(extractor): result = await extractor.extract_text(block) assert result is None assert block.extraction_error is not None + + +async def test_ocr_decompression_bomb_refused(monkeypatch): + """A PNG that decodes to >50 MP must be refused, not OOM the process.""" + import io + + from PIL import Image + + # Force the bomb threshold low so the test stays fast; the production + # constant is 50 MP. + monkeypatch.setattr( + "noirdoc.file_analysis.extractors.ocr._MAX_IMAGE_PIXELS", + 100, + ) + huge = Image.new("RGB", (50, 50), color="white") + buf = io.BytesIO() + huge.save(buf, format="PNG") + block = FileBlock( + content_bytes=buf.getvalue(), + mime_type="image/png", + source_path="test", + source_type="image", + ) + result = await FileTextExtractor(ocr_enabled=True).extract_text(block) + assert result is None + assert "decompression bomb" in (block.extraction_error or "").lower() From 9a7b2f2adbff65ca42e6bebc613ba83253c69bae Mon Sep 17 00:00:00 2001 From: Antonio Maiolo Date: Mon, 27 Apr 2026 15:26:34 +0200 Subject: [PATCH 02/13] fix(ensemble): surface detector failures via warning log (NDS-023) asyncio.gather(..., return_exceptions=True) silently dropped detector exceptions; a spaCy/Flair/GLiNER load error would degrade the ensemble to whatever still worked, with no signal. Replace with an explicit per-detector wrapper that logs each failure under detection.detector_failed so operators can detect the degraded state and downstream PII may not silently leak. --- src/noirdoc/detection/ensemble.py | 33 +++++++++++++++++++++++++++---- tests/test_ensemble.py | 12 +++++++++++ 2 files changed, 41 insertions(+), 4 deletions(-) diff --git a/src/noirdoc/detection/ensemble.py b/src/noirdoc/detection/ensemble.py index c71dff8..e30994e 100644 --- a/src/noirdoc/detection/ensemble.py +++ b/src/noirdoc/detection/ensemble.py @@ -3,8 +3,12 @@ import asyncio import re +import structlog + from noirdoc.detection.base import BaseDetector, DetectedEntity +log = structlog.get_logger(__name__) + # Strong indicators: if ANY of these appear in a multi-word PERSON entity, reject it. _PERSON_STRONG_REJECT: set[str] = { # verbs / participles commonly absorbed by spaCy NER @@ -103,14 +107,11 @@ async def detect(self, text: str, language: str = "de") -> list[DetectedEntity]: return [] results = await asyncio.gather( - *(d.detect(text, language) for d in self.detectors), - return_exceptions=True, + *(self._run_one(d, text, language) for d in self.detectors), ) all_entities: list[DetectedEntity] = [] for result in results: - if isinstance(result, BaseException): - continue all_entities.extend(result) filtered = [ @@ -122,6 +123,30 @@ async def detect(self, text: str, language: str = "de") -> list[DetectedEntity]: validated = [e for e in merged if _validate_person(e)] return sorted(validated, key=lambda e: e.start) + @staticmethod + async def _run_one( + detector: BaseDetector, + text: str, + language: str, + ) -> list[DetectedEntity]: + """Run one detector. On failure, log and degrade to empty results. + + A silent ``return_exceptions=True`` would bury detector failures + and cause silent leakage (e.g. PERSON detection going dark on a + spaCy load error). We log explicitly so operators can spot the + degraded state. + """ + try: + return await detector.detect(text, language) + except Exception as exc: + log.warning( + "detection.detector_failed", + detector=getattr(detector, "name", detector.__class__.__name__), + language=language, + error=str(exc), + ) + return [] + def _merge_entities(self, entities: list[DetectedEntity]) -> list[DetectedEntity]: """ Overlap Resolution: diff --git a/tests/test_ensemble.py b/tests/test_ensemble.py index a17e0a7..e352ef5 100644 --- a/tests/test_ensemble.py +++ b/tests/test_ensemble.py @@ -166,6 +166,18 @@ async def test_failing_detector_does_not_crash(): assert len(result) == 1 +async def test_failing_detector_logs_warning(capsys): + """A failing detector must log a warning so operators can detect degraded state.""" + ent = _ent("PERSON", "Max", 0, 3, 0.9, "presidio") + ensemble = EnsembleDetector( + [FailingDetector(), FakeDetector([ent])], + score_threshold=0.0, + ) + await ensemble.detect("dummy", "de") + captured = capsys.readouterr().out + capsys.readouterr().err + assert "detection.detector_failed" in captured + + # --- PERSON validation --- From c46eaaa0cc11fdcd21fdde1182479dfaa374d2eb Mon Sep 17 00:00:00 2001 From: Antonio Maiolo Date: Mon, 27 Apr 2026 15:28:32 +0200 Subject: [PATCH 03/13] fix(daemon): bound socket buffer + per-field length caps (NDS-027) A daemon peer could previously stream an unbounded line to wedge the asyncio StreamReader into eating memory until OOM. Cap the read buffer at 32 MB and reply with a structured bad_request when a peer exceeds it. Add Pydantic max_length on every protocol string field so a too-long path or text value is refused at the validation layer before any work is queued. --- src/noirdoc/daemon/client.py | 5 ++++- src/noirdoc/daemon/protocol.py | 27 +++++++++++++++++++-------- src/noirdoc/daemon/server.py | 28 +++++++++++++++++++++++++++- tests/daemon/test_protocol.py | 16 ++++++++++++++++ 4 files changed, 66 insertions(+), 10 deletions(-) diff --git a/src/noirdoc/daemon/client.py b/src/noirdoc/daemon/client.py index 56a2a1b..369a5d0 100644 --- a/src/noirdoc/daemon/client.py +++ b/src/noirdoc/daemon/client.py @@ -20,6 +20,9 @@ CONNECT_TIMEOUT = 2.0 RPC_TIMEOUT = 600.0 # generous; covers cold-spawn warmup + slow file redaction SHUTDOWN_DRAIN_TIMEOUT = 5.0 +# Match the server-side cap (server.SOCKET_READ_LIMIT). Bounds the +# memory the client will buffer if the daemon sends an oversize line. +SOCKET_READ_LIMIT = 32 * 1024 * 1024 class DaemonError(Exception): @@ -35,7 +38,7 @@ async def _try_connect( ) -> tuple[asyncio.StreamReader, asyncio.StreamWriter] | None: try: return await asyncio.wait_for( - asyncio.open_unix_connection(path=str(socket_path)), + asyncio.open_unix_connection(path=str(socket_path), limit=SOCKET_READ_LIMIT), timeout=CONNECT_TIMEOUT, ) except (FileNotFoundError, ConnectionRefusedError, TimeoutError, OSError): diff --git a/src/noirdoc/daemon/protocol.py b/src/noirdoc/daemon/protocol.py index 0768717..2798946 100644 --- a/src/noirdoc/daemon/protocol.py +++ b/src/noirdoc/daemon/protocol.py @@ -20,9 +20,17 @@ DetectorChoice = Literal["presidio", "gliner", "ensemble"] +# Per-field caps. Bound so a malicious or buggy client cannot wedge the +# daemon with a multi-gigabyte JSON payload. Tunable here; matched by +# the asyncio buffer limit in server.py / client.py. +MAX_TEXT_VALUE_LEN = 16 * 1024 * 1024 # 16 MB, covers very large texts +MAX_PATH_LEN = 4096 # POSIX PATH_MAX +MAX_NAMESPACE_LEN = 64 +MAX_DETECTOR_MODEL_LEN = 512 + class HelloParams(BaseModel): - client_version: str + client_version: str = Field(max_length=64) class HelloResult(BaseModel): @@ -33,12 +41,12 @@ class HelloResult(BaseModel): class RedactTextInput(BaseModel): type: Literal["text"] = "text" - value: str + value: str = Field(max_length=MAX_TEXT_VALUE_LEN) class RedactFileInput(BaseModel): type: Literal["file"] = "file" - path: str # absolute path on the daemon's filesystem (same user as CLI) + path: str = Field(max_length=MAX_PATH_LEN) RedactInput = Annotated[ @@ -48,14 +56,17 @@ class RedactFileInput(BaseModel): class RedactParams(BaseModel): - namespace: str | None = None - namespace_root: str | None = None - language: str = "de" + namespace: str | None = Field(default=None, max_length=MAX_NAMESPACE_LEN) + namespace_root: str | None = Field(default=None, max_length=MAX_PATH_LEN) + language: str = Field(default="de", max_length=8) detector: DetectorChoice = "ensemble" score_threshold: float = 0.5 - gliner_model: str = "knowledgator/gliner-pii-edge-v1.0" + gliner_model: str = Field( + default="knowledgator/gliner-pii-edge-v1.0", + max_length=MAX_DETECTOR_MODEL_LEN, + ) input: RedactInput - output_path: str | None = None # for file input; daemon writes here directly + output_path: str | None = Field(default=None, max_length=MAX_PATH_LEN) class RedactResult(BaseModel): diff --git a/src/noirdoc/daemon/server.py b/src/noirdoc/daemon/server.py index b2269f9..be04eb7 100644 --- a/src/noirdoc/daemon/server.py +++ b/src/noirdoc/daemon/server.py @@ -49,6 +49,12 @@ LOG_BACKUP_COUNT = 1 SUPPORTED_WARMUP_LANGUAGES = ("de", "en") +# Cap per-message buffer the asyncio StreamReader will accept. A line +# longer than this raises LimitOverrunError instead of growing memory +# unbounded. Matches the protocol-level Field(max_length=...) caps with +# enough headroom for JSON encoding overhead. +SOCKET_READ_LIMIT = 32 * 1024 * 1024 + log = logging.getLogger("noirdoc.daemon") @@ -368,7 +374,26 @@ async def _handle_connection( ) -> None: try: while not state.shutdown_event.is_set(): - line = await reader.readline() + try: + line = await reader.readline() + except asyncio.LimitOverrunError as exc: + # Drain the offending line and report the error so a + # malicious peer can't wedge the daemon by sending an + # endless line. + log.warning("daemon.line_too_long bytes=%d", exc.consumed) + writer.write( + _serialize( + Response( + id="", + error=ErrorPayload( + code=ERR_BAD_REQUEST, + message=f"request line exceeded {SOCKET_READ_LIMIT} bytes", + ), + ), + ), + ) + await writer.drain() + return if not line: return # client closed response = await _dispatch(state, line) @@ -432,6 +457,7 @@ async def _async_main() -> None: server = await asyncio.start_unix_server( lambda r, w: _handle_connection(r, w, state), path=str(sock_path), + limit=SOCKET_READ_LIMIT, ) try: os.chmod(sock_path, 0o600) diff --git a/tests/daemon/test_protocol.py b/tests/daemon/test_protocol.py index b54fdf7..97ef263 100644 --- a/tests/daemon/test_protocol.py +++ b/tests/daemon/test_protocol.py @@ -7,6 +7,8 @@ import pytest from noirdoc.daemon.protocol import ( + MAX_PATH_LEN, + MAX_TEXT_VALUE_LEN, ErrorPayload, HelloParams, HelloResult, @@ -108,3 +110,17 @@ def test_response_envelope_with_error(): out = _roundtrip(resp) assert out.error is not None assert out.error.code == "bad_request" + + +def test_oversized_text_value_rejected(): + """Text inputs above MAX_TEXT_VALUE_LEN are refused to bound DoS.""" + too_big = "a" * (MAX_TEXT_VALUE_LEN + 1) + with pytest.raises(Exception): + RedactTextInput(value=too_big) + + +def test_oversized_path_rejected(): + """Paths above MAX_PATH_LEN are refused to bound DoS.""" + too_long = "/" + ("a" * MAX_PATH_LEN) + with pytest.raises(Exception): + RedactFileInput(path=too_long) From e73a4dbcb3845fc8db3dc240db7a35303f03d602 Mon Sep 17 00:00:00 2001 From: Antonio Maiolo Date: Mon, 27 Apr 2026 15:29:32 +0200 Subject: [PATCH 04/13] fix(namespace): reject path-traversal namespace names (NDS-009) Namespace names came straight from user input and were joined into the namespace root with no validation. A name like '../../etc' or '/abs' would silently escape the configured root. Whitelist names to [A-Za-z0-9][A-Za-z0-9._-]{0,63} so traversal, separators, and shell metacharacters all raise ValueError before any filesystem I/O. --- src/noirdoc/namespace.py | 17 +++++++++++++++-- tests/test_namespace.py | 22 ++++++++++++++++++++++ 2 files changed, 37 insertions(+), 2 deletions(-) diff --git a/src/noirdoc/namespace.py b/src/noirdoc/namespace.py index 06944c6..08f692d 100644 --- a/src/noirdoc/namespace.py +++ b/src/noirdoc/namespace.py @@ -11,6 +11,7 @@ import json import os +import re from pathlib import Path from cryptography.fernet import Fernet, InvalidToken @@ -21,15 +22,27 @@ _KEY_FILE = "key" _DATA_FILE = "mapper.enc" +# Restrictive whitelist for namespace names. Rejects path traversal +# (``..``), absolute paths, separators, and shell metacharacters so a +# user-supplied namespace cannot escape the configured root. +_NAMESPACE_NAME_RE = re.compile(r"^[A-Za-z0-9][A-Za-z0-9._-]{0,63}$") + + +def _validate_namespace_name(name: str) -> str: + if not _NAMESPACE_NAME_RE.fullmatch(name): + raise ValueError( + f"invalid namespace name {name!r}: must match [A-Za-z0-9][A-Za-z0-9._-]{{0,63}}", + ) + return name class Namespace: """A persistent, Fernet-encrypted pseudonym mapping on disk.""" def __init__(self, name: str, root: Path | str | None = None) -> None: - self.name = name + self.name = _validate_namespace_name(name) self.root = Path(root).expanduser() if root else DEFAULT_NAMESPACE_ROOT - self.path = self.root / name + self.path = self.root / self.name @property def key_path(self) -> Path: diff --git a/tests/test_namespace.py b/tests/test_namespace.py index 22ef348..d12fdb9 100644 --- a/tests/test_namespace.py +++ b/tests/test_namespace.py @@ -84,6 +84,28 @@ def test_mapper_to_dict_roundtrip(): assert restored.get_or_create("Jane Doe", "PERSON") == "<>" +@pytest.mark.parametrize( + "bad_name", + [ + "../escape", + "/etc/passwd", + "..", + ".", + "foo/bar", + "foo\\bar", + "foo bar", + "", + "a" * 65, + ".hidden", + "-leading-dash", + ], +) +def test_namespace_rejects_unsafe_names(tmp_path: Path, bad_name: str): + """Path-traversal / shell-metacharacter names must raise before any I/O.""" + with pytest.raises(ValueError, match="invalid namespace name"): + Namespace(bad_name, root=tmp_path) + + def test_corrupt_key_raises(tmp_path: Path): ns = Namespace("demo", root=tmp_path) mapper = ns.load() From 1261908f3d5a4f3f02c20d1c489bbacf20ecdeff Mon Sep 17 00:00:00 2001 From: Antonio Maiolo Date: Mon, 27 Apr 2026 15:30:29 +0200 Subject: [PATCH 05/13] fix(namespace): close TOCTOU on Fernet key creation (NDS-004) The previous _ensure_key: - created the namespace dir with the user's umask, leaving intermediate dirs potentially world-readable - wrote the key with default mode then chmod'd 0600 (window where another UID could open the file) - check-then-write race could clobber a concurrently-created key Replace with mkdir(mode=0o700) + explicit chmod, then atomic os.open(O_CREAT|O_EXCL, 0o600). The key is born with the right mode and a concurrent writer is detected by FileExistsError and re-read. --- src/noirdoc/namespace.py | 28 +++++++++++++++++++++++++--- tests/test_namespace.py | 17 ++++++++++++++++- 2 files changed, 41 insertions(+), 4 deletions(-) diff --git a/src/noirdoc/namespace.py b/src/noirdoc/namespace.py index 08f692d..ede31bb 100644 --- a/src/noirdoc/namespace.py +++ b/src/noirdoc/namespace.py @@ -59,10 +59,32 @@ def _ensure_key(self) -> bytes: if self.key_path.is_file(): return self.key_path.read_bytes() - self.path.mkdir(parents=True, exist_ok=True) + # Create the namespace directory with 0700 *up front* so the key + # file is never momentarily readable by another user. mkdir's + # ``mode`` is honored only on creation; umask may have stripped + # bits, so chmod explicitly afterward. + self.path.mkdir(parents=True, mode=0o700, exist_ok=True) + try: + os.chmod(self.path, 0o700) + except OSError: + pass + key = Fernet.generate_key() - self.key_path.write_bytes(key) - os.chmod(self.key_path, 0o600) + # Atomically create the key file with mode 0600 and refuse to + # clobber. O_EXCL closes the TOCTOU window where the file might + # have appeared after the is_file() check above. + try: + fd = os.open( + str(self.key_path), + os.O_WRONLY | os.O_CREAT | os.O_EXCL, + 0o600, + ) + except FileExistsError: + return self.key_path.read_bytes() + try: + os.write(fd, key) + finally: + os.close(fd) return key def load(self) -> PseudonymMapper: diff --git a/tests/test_namespace.py b/tests/test_namespace.py index d12fdb9..85d69af 100644 --- a/tests/test_namespace.py +++ b/tests/test_namespace.py @@ -19,9 +19,24 @@ def test_create_namespace_generates_key(tmp_path: Path): assert mapper.entity_count == 0 assert ns.exists() assert ns.key_path.is_file() - # Key file should be 0600 + # Key file must be 0600 from the moment it is created. mode = ns.key_path.stat().st_mode & 0o777 assert mode == 0o600 + # Namespace directory must be 0700 so other users cannot list it. + dir_mode = ns.path.stat().st_mode & 0o777 + assert dir_mode == 0o700 + + +def test_existing_key_not_clobbered_on_concurrent_load(tmp_path: Path): + """A second `_ensure_key` must reuse the existing key, not overwrite it.""" + ns = Namespace("demo", root=tmp_path) + key1 = ns._ensure_key() + key2 = ns._ensure_key() + assert key1 == key2 + # Even after deleting the in-memory file handle and re-creating, the + # on-disk key remains stable. + ns2 = Namespace("demo", root=tmp_path) + assert ns2._ensure_key() == key1 def test_save_and_load_roundtrip(tmp_path: Path): From b734ddb068f498b46586a8949cb7ee04be319bce Mon Sep 17 00:00:00 2001 From: Antonio Maiolo Date: Mon, 27 Apr 2026 15:33:10 +0200 Subject: [PATCH 06/13] fix(cli): canonicalize output paths and refuse namespace-store writes (NDS-034) -o and --output-dir flowed straight into Path joins. Two failure modes: - Resolved output could land outside the requested --output-dir (Path semantics mostly cover this, but the contract was implicit) - Nothing prevented writing inside DEFAULT_NAMESPACE_ROOT, so a crafted -o could clobber a Fernet key or mapper.enc and silently destroy reversibility for that namespace. _guard_output_path now resolves both inputs, asserts is_relative_to the requested dir, and refuses anything inside DEFAULT_NAMESPACE_ROOT. The basename for derived names is taken from input_path.name only so a crafted relative input cannot route the output elsewhere. --- src/noirdoc/cli.py | 42 +++++++++++++++++++++++++++++++++++++----- tests/test_cli.py | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 71 insertions(+), 5 deletions(-) diff --git a/src/noirdoc/cli.py b/src/noirdoc/cli.py index 4962e23..df982eb 100644 --- a/src/noirdoc/cli.py +++ b/src/noirdoc/cli.py @@ -483,6 +483,31 @@ def _expand_inputs(inputs: tuple[Path, ...]) -> list[Path]: return files +def _guard_output_path(out_path: Path, *, output_dir: Path | None) -> Path: + """Canonicalize ``out_path`` and refuse paths that escape sane bounds. + + Two checks: + * If ``--output-dir`` was given, the resolved output must live under + the resolved output dir. Drops the bullet "malicious input + filename routes the redacted output outside the requested dir". + * Refuse anything inside the user's namespaces directory: clobbering + a key file would silently destroy reversibility. + """ + resolved = out_path.resolve() + if output_dir is not None: + resolved_dir = output_dir.resolve() + if not resolved.is_relative_to(resolved_dir): + raise click.ClickException( + f"refusing to write {resolved} outside --output-dir {resolved_dir}", + ) + namespaces_root = DEFAULT_NAMESPACE_ROOT.resolve() + if resolved.is_relative_to(namespaces_root): + raise click.ClickException( + f"refusing to write inside namespace store {namespaces_root}", + ) + return resolved + + def _choose_output_path( input_path: Path, *, @@ -490,12 +515,19 @@ def _choose_output_path( output_dir: Path | None, reconstructed: bool, ) -> Path: + # Always derive the leaf name from the input *basename* so a + # crafted input path like "/tmp/in/../etc/passwd" cannot route the + # output anywhere but next to the requested location. + safe_name = Path(input_path.name) if output: - return output - parent = output_dir or input_path.parent - if reconstructed: - return parent / f"{input_path.stem}_redacted{input_path.suffix}" - return parent / f"{input_path.stem}_redacted.txt" + chosen = output + else: + parent = output_dir or input_path.parent + if reconstructed: + chosen = parent / f"{safe_name.stem}_redacted{safe_name.suffix}" + else: + chosen = parent / f"{safe_name.stem}_redacted.txt" + return _guard_output_path(chosen, output_dir=output_dir) if __name__ == "__main__": diff --git a/tests/test_cli.py b/tests/test_cli.py index df0beb5..dfd622a 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -3,6 +3,8 @@ import json from pathlib import Path +import click +import pytest from click.testing import CliRunner from noirdoc import cli as cli_module @@ -46,3 +48,35 @@ def test_ns_summary_missing_namespace(monkeypatch, tmp_path: Path): result = CliRunner().invoke(main, ["ns", "summary", "nope"]) assert result.exit_code == 1 assert "Namespace 'nope' does not exist." in result.output + + +def test_choose_output_path_drops_input_directory_components(tmp_path: Path): + """Crafted input paths must not route output outside --output-dir.""" + out_dir = tmp_path / "out" + out_dir.mkdir() + crafted = tmp_path / "in" / ".." / "etc" / "passwd-fake" + crafted.parent.mkdir(parents=True, exist_ok=True) + crafted.touch() + chosen = cli_module._choose_output_path( + crafted, + output=None, + output_dir=out_dir, + reconstructed=True, + ) + assert chosen.is_relative_to(out_dir.resolve()) + + +def test_choose_output_path_refuses_namespace_store(monkeypatch, tmp_path: Path): + """Refuse to overwrite anything inside the namespaces directory.""" + namespaces_root = tmp_path / "namespaces" + namespaces_root.mkdir() + monkeypatch.setattr(cli_module, "DEFAULT_NAMESPACE_ROOT", namespaces_root) + + target = namespaces_root / "demo" / "key" + with pytest.raises(click.ClickException, match="namespace store"): + cli_module._choose_output_path( + tmp_path / "vertrag.txt", + output=target, + output_dir=None, + reconstructed=True, + ) From 4117b21e996987b6fb644bd6f47df9008d5ce909 Mon Sep 17 00:00:00 2001 From: Antonio Maiolo Date: Mon, 27 Apr 2026 15:33:58 +0200 Subject: [PATCH 07/13] fix(cli): require --unsafe to reveal full namespace mapping (NDS-033) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ns show printed the entire pseudonym→original map. The README's command table called it a "summary", and any wrapper, transcript, or log capture immediately defeated redaction for that namespace. Refuse to run without --unsafe and point users at ns summary for the safe counts-only view. Update the README table accordingly. --- README.md | 7 ++++--- src/noirdoc/cli.py | 23 +++++++++++++++++++++-- tests/test_cli.py | 28 ++++++++++++++++++++++++++++ 3 files changed, 53 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index 038b7a2..a94c035 100644 --- a/README.md +++ b/README.md @@ -75,9 +75,10 @@ Output: | `noirdoc redact ` | Redact one or more files (accepts directories; `-o FILE` or `--output-dir DIR`). | | `noirdoc reveal ` | Reverse pseudonyms back to originals (DOCX / XLSX / plain; `--namespace` required). | | `noirdoc lookup ` | Resolve a pseudonym like `<>` to its original value. | -| `noirdoc ns list` | List persistent namespaces under `~/.noirdoc/namespaces/`. | -| `noirdoc ns show ` | Print the mapping summary for a namespace as JSON. | -| `noirdoc ns delete ` | Delete a namespace (prompts for confirmation). | +| `noirdoc ns list` | List persistent namespaces. | +| `noirdoc ns summary ` | Counts-only summary (entity totals + per-type counts). Safe to log. | +| `noirdoc ns show --unsafe` | Print the full pseudonym↔original mapping as JSON. **Reveals every original value.** Requires `--unsafe`. | +| `noirdoc ns delete ` | Delete a namespace (prompts for confirmation). | | `noirdoc models pull` | Download spaCy models and (optionally) GLiNER weights up front. | Run `noirdoc --help` for the full flag list on any subcommand. diff --git a/src/noirdoc/cli.py b/src/noirdoc/cli.py index df982eb..fcddbf2 100644 --- a/src/noirdoc/cli.py +++ b/src/noirdoc/cli.py @@ -314,8 +314,27 @@ def ns_list() -> None: @ns.command("show") @click.argument("namespace") -def ns_show(namespace: str) -> None: - """Print the mapping summary for NAMESPACE as JSON.""" +@click.option( + "--unsafe", + is_flag=True, + help="Acknowledge that the full pseudonym→original mapping will be printed.", +) +def ns_show(namespace: str, unsafe: bool) -> None: + """Print the full pseudonym→original mapping as JSON. + + Reveals every original value in the namespace. Refuses to run + without --unsafe so the mapping cannot be captured by accident + (terminal scrollback, CI logs, copy-paste). For a non-revealing + overview, use ``noirdoc ns summary``. + """ + if not unsafe: + click.echo( + "ns show prints original values that defeat redaction. " + "Re-run with --unsafe to confirm, or use 'noirdoc ns summary' " + "for a counts-only view.", + err=True, + ) + sys.exit(2) ns_obj = Namespace(namespace) if not ns_obj.exists(): click.echo(f"Namespace {namespace!r} does not exist.", err=True) diff --git a/tests/test_cli.py b/tests/test_cli.py index dfd622a..b64b6f5 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -50,6 +50,34 @@ def test_ns_summary_missing_namespace(monkeypatch, tmp_path: Path): assert "Namespace 'nope' does not exist." in result.output +def test_ns_show_requires_unsafe_flag(monkeypatch, tmp_path: Path): + """ns show must not print original values without --unsafe.""" + _redirect_namespace_root(monkeypatch, tmp_path) + + ns = Namespace("demo") + mapper = ns.load() + mapper.get_or_create("Anna Müller", "PERSON") + ns.save(mapper) + + result = CliRunner().invoke(main, ["ns", "show", "demo"]) + assert result.exit_code == 2 + assert "Anna Müller" not in result.output + assert "--unsafe" in result.output + + +def test_ns_show_unsafe_prints_mapping(monkeypatch, tmp_path: Path): + _redirect_namespace_root(monkeypatch, tmp_path) + + ns = Namespace("demo") + mapper = ns.load() + mapper.get_or_create("Anna Müller", "PERSON") + ns.save(mapper) + + result = CliRunner().invoke(main, ["ns", "show", "demo", "--unsafe"]) + assert result.exit_code == 0, result.output + assert "Anna Müller" in result.output + + def test_choose_output_path_drops_input_directory_components(tmp_path: Path): """Crafted input paths must not route output outside --output-dir.""" out_dir = tmp_path / "out" From 67983b463fe7d19eb23fb4da188e0527adcfa7da Mon Sep 17 00:00:00 2001 From: Antonio Maiolo Date: Mon, 27 Apr 2026 15:36:20 +0200 Subject: [PATCH 08/13] fix(daemon): same-UID stat invariant on input/output paths (NDS-028) The redact handler accepted any input.path and output_path string from the socket peer. Cross-user is blocked by the 0o600 socket inside a 0o700 dir, but any same-UID process able to talk to the socket could turn the daemon into a confused-deputy primitive: read any user-readable file (and exfil through the redacted output), or overwrite any user-writable file with the redacted bytes of an arbitrary input. Resolve both paths and stat them against os.getuid() before doing any I/O. Refuse with ERR_BAD_REQUEST if the input file or the output parent dir is not owned by the daemon's UID. Add ValueError to the bad_request branch in _dispatch so a refused request gets a structured response instead of the generic internal error code. --- src/noirdoc/daemon/server.py | 31 ++++++++++++- tests/daemon/test_path_trust.py | 77 +++++++++++++++++++++++++++++++++ 2 files changed, 107 insertions(+), 1 deletion(-) create mode 100644 tests/daemon/test_path_trust.py diff --git a/src/noirdoc/daemon/server.py b/src/noirdoc/daemon/server.py index be04eb7..2ed4874 100644 --- a/src/noirdoc/daemon/server.py +++ b/src/noirdoc/daemon/server.py @@ -226,6 +226,24 @@ async def handle_shutdown( return ShutdownResult().model_dump() +def _check_same_uid(path: Path, label: str) -> None: + """Refuse a request if *path* is not owned by the daemon's UID. + + Closes the same-UID confused-deputy hole where any process able to + talk to the daemon socket could trick it into reading or writing + files the user did not intend. The daemon already runs as the user; + this assertion guards against symlink-swap and sloppy callers. + """ + try: + st = os.stat(path) + except FileNotFoundError as exc: + raise ValueError(f"{label} not found: {path}") from exc + if st.st_uid != os.getuid(): + raise ValueError( + f"{label} {path} is not owned by the current user (uid={os.getuid()})", + ) + + async def handle_redact( state: DaemonState, params: dict[str, Any], @@ -235,6 +253,17 @@ async def handle_redact( parsed = RedactParams.model_validate(params) + if isinstance(parsed.input, RedactFileInput): + in_path = Path(parsed.input.path).resolve() + _check_same_uid(in_path, "input.path") + parsed.input.path = str(in_path) + if parsed.output_path: + out_path = Path(parsed.output_path).resolve() + out_parent = out_path.parent + out_parent.mkdir(parents=True, exist_ok=True) + _check_same_uid(out_parent, "output_path parent") + parsed.output_path = str(out_path) + state.queue_depth += 1 try: async with state.redact_lock: @@ -352,7 +381,7 @@ async def _dispatch(state: DaemonState, raw_line: bytes) -> Response: try: result = await handler(state, request.params) - except ValidationError as exc: + except (ValidationError, ValueError) as exc: return Response( id=request.id, error=ErrorPayload(code=ERR_BAD_REQUEST, message=str(exc)), diff --git a/tests/daemon/test_path_trust.py b/tests/daemon/test_path_trust.py new file mode 100644 index 0000000..035ebf3 --- /dev/null +++ b/tests/daemon/test_path_trust.py @@ -0,0 +1,77 @@ +"""NDS-028: daemon must reject input/output paths owned by another UID.""" + +from __future__ import annotations + +import os + +import pytest + +from noirdoc.daemon import server + +pytestmark = pytest.mark.asyncio + + +async def test_redact_rejects_input_owned_by_another_uid(monkeypatch, tmp_path): + """A peer that asks the daemon to read a file not owned by its UID is refused.""" + target = tmp_path / "victim.txt" + target.write_bytes(b"some content") + + # Pretend our UID is something other than the file's owner. + monkeypatch.setattr(os, "getuid", lambda: os.stat(target).st_uid + 1) + + state = server.DaemonState() + params = { + "input": {"type": "file", "path": str(target)}, + "output_path": str(tmp_path / "out.txt"), + } + with pytest.raises(ValueError, match="not owned by the current user"): + await server.handle_redact(state, params) + + +async def test_redact_rejects_missing_input(tmp_path): + state = server.DaemonState() + params = { + "input": {"type": "file", "path": str(tmp_path / "nonexistent")}, + } + with pytest.raises(ValueError, match="not found"): + await server.handle_redact(state, params) + + +async def test_redact_rejects_output_parent_owned_by_another_uid(monkeypatch, tmp_path): + """Refuse to write into a directory owned by a different UID.""" + src = tmp_path / "in.txt" + src.write_bytes(b"content") + bad_dir = tmp_path / "owned-by-someone-else" + bad_dir.mkdir() + + real_stat = os.stat + real_uid = os.getuid() + + def fake_stat(path, *args, **kwargs): + st = real_stat(path, *args, **kwargs) + if str(path).endswith("owned-by-someone-else"): + return os.stat_result( + ( + st.st_mode, + st.st_ino, + st.st_dev, + st.st_nlink, + real_uid + 1, # foreign uid + st.st_gid, + st.st_size, + st.st_atime, + st.st_mtime, + st.st_ctime, + ), + ) + return st + + monkeypatch.setattr(server.os, "stat", fake_stat) + + state = server.DaemonState() + params = { + "input": {"type": "file", "path": str(src)}, + "output_path": str(bad_dir / "result.txt"), + } + with pytest.raises(ValueError, match="not owned by the current user"): + await server.handle_redact(state, params) From a574aef950d80acbfa885796a4d40f755b84fb88 Mon Sep 17 00:00:00 2001 From: Antonio Maiolo Date: Mon, 27 Apr 2026 15:41:26 +0200 Subject: [PATCH 09/13] fix: surface PDF /Info metadata so detectors scrub embedded PII (NDS-017) PDF documents commonly carry the same names, titles, and addresses in their Info dictionary (Author, Title, Subject, Creator, Producer, Keywords) that the body does. Without extraction those entries never reach the detector ensemble, so redacted PDFs leaked the originals while pretending the body was clean. extract_pdf and extract_pdf_with_ocr_fallback now read the Info dict via pypdfium2.PdfDocument.get_metadata_value and prepend non-empty fields to the page text. Detection treats them as ordinary text and the PDF output path (plain-text fallback) carries the pseudonymized versions instead of the originals. --- src/noirdoc/file_analysis/extractors/pdf.py | 38 ++++++++++++-- tests/file_analysis/test_pdf_extractor.py | 55 +++++++++++++++++++++ 2 files changed, 90 insertions(+), 3 deletions(-) diff --git a/src/noirdoc/file_analysis/extractors/pdf.py b/src/noirdoc/file_analysis/extractors/pdf.py index a84d646..247b5cb 100644 --- a/src/noirdoc/file_analysis/extractors/pdf.py +++ b/src/noirdoc/file_analysis/extractors/pdf.py @@ -7,16 +7,47 @@ logger = structlog.get_logger() +_METADATA_KEYS = ("Title", "Author", "Subject", "Keywords", "Creator", "Producer") + + +def _extract_pdf_metadata(pdf) -> str: + """Return PDF Info-dict metadata as text so detectors can see embedded PII. + + PDF /Info entries (Author, Title, …) routinely carry the same names and + addresses the body does. Without this, redacted PDFs round-trip the + metadata untouched and leak originals. + """ + lines: list[str] = [] + for key in _METADATA_KEYS: + try: + value = pdf.get_metadata_value(key) + except Exception: + continue + if value: + lines.append(f"{key}: {value}") + return "\n".join(lines) + + +def _prepend_metadata(pages_text: str, metadata_text: str) -> str: + if not metadata_text: + return pages_text + if not pages_text: + return metadata_text + return f"{metadata_text}\n\n{pages_text}" + + def extract_pdf(data: bytes, *, max_pages: int = 50) -> str: """Extract text from a PDF byte-string, page by page. - Returns concatenated text with double-newlines between pages. + Returns concatenated text with double-newlines between pages, prefixed + by any Info-dict metadata so detectors can scrub PII embedded there. """ import pypdfium2 as pdfium pdf = pdfium.PdfDocument(data) pages: list[str] = [] try: + metadata_text = _extract_pdf_metadata(pdf) for i, page in enumerate(pdf): if i >= max_pages: break @@ -26,7 +57,7 @@ def extract_pdf(data: bytes, *, max_pages: int = 50) -> str: page.close() finally: pdf.close() - return "\n\n".join(pages) + return _prepend_metadata("\n\n".join(pages), metadata_text) def extract_pdf_with_ocr_fallback( @@ -51,6 +82,7 @@ def extract_pdf_with_ocr_fallback( pages: list[str] = [] ocr_triggered = 0 try: + metadata_text = _extract_pdf_metadata(pdf) for i, page in enumerate(pdf): if i >= max_pages: break @@ -76,4 +108,4 @@ def extract_pdf_with_ocr_fallback( threshold=min_chars_per_page, ) - return "\n\n".join(pages) + return _prepend_metadata("\n\n".join(pages), metadata_text) diff --git a/tests/file_analysis/test_pdf_extractor.py b/tests/file_analysis/test_pdf_extractor.py index c36f097..e0d3aee 100644 --- a/tests/file_analysis/test_pdf_extractor.py +++ b/tests/file_analysis/test_pdf_extractor.py @@ -83,6 +83,40 @@ def _make_image_pdf(text: str) -> bytes: return buf.getvalue() +def _make_pdf_with_metadata( + body_text: str, + *, + author: str | None = None, + title: str | None = None, + subject: str | None = None, +) -> bytes: + """A single-page PDF carrying /Info metadata fields. + + Used to verify that PDF /Info entries are surfaced to the detector so PII + embedded there gets pseudonymized rather than passed through. + """ + from PIL import Image, ImageDraw, ImageFont + + img = Image.new("RGB", (1200, 1600), "white") + draw = ImageDraw.Draw(img) + try: + font = ImageFont.truetype("/System/Library/Fonts/Helvetica.ttc", 48) + except OSError: + font = ImageFont.load_default() + draw.text((60, 100), body_text, fill="black", font=font) + + buf = io.BytesIO() + save_kwargs: dict[str, str] = {} + if author is not None: + save_kwargs["author"] = author + if title is not None: + save_kwargs["title"] = title + if subject is not None: + save_kwargs["subject"] = subject + img.save(buf, format="PDF", resolution=150.0, **save_kwargs) + return buf.getvalue() + + @pytest.fixture def text_pdf_bytes() -> bytes: return _TEXT_PDF_BYTES @@ -107,6 +141,27 @@ def test_extract_pdf_returns_empty_for_image_only_pdf(image_pdf_bytes): assert text.strip() == "" +def test_extract_pdf_surfaces_info_dict_metadata(): + """PII in /Info fields must reach the extracted text so detection sees it.""" + pdf_bytes = _make_pdf_with_metadata( + "body text", + author="Anna Mueller", + title="Vertrag fuer Mueller", + subject="Confidential", + ) + text = extract_pdf(pdf_bytes) + assert "Author: Anna Mueller" in text + assert "Title: Vertrag fuer Mueller" in text + assert "Subject: Confidential" in text + + +def test_extract_pdf_handles_missing_metadata(text_pdf_bytes): + """A PDF without /Info entries must not crash or inject spurious labels.""" + text = extract_pdf(text_pdf_bytes) + assert "Author:" not in text + assert "Title:" not in text + + # ── extract_pdf_with_ocr_fallback (new) ────────────────────────────────────── From 38312798625f761748aed6255099ba2247ab9acf Mon Sep 17 00:00:00 2001 From: Antonio Maiolo Date: Mon, 27 Apr 2026 15:46:14 +0200 Subject: [PATCH 10/13] fix: harden DOCX/XLSX input parsing against XML and zip bombs (NDS-015) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit DOCX and XLSX are zip envelopes around XML. python-docx already pins its lxml parser with resolve_entities=False, but openpyxl only swaps in defusedxml when the package is importable. We now declare defusedxml as a baseline dependency so openpyxl's iterparse path becomes entity-safe by default. The zip envelope itself was unguarded. A new check_ooxml_zip_safe helper runs before python-docx / openpyxl touches the bytes and refuses archives whose central directory advertises more than 200 MB uncompressed or a compression ratio above 100x — the standard zip bomb shapes. Wired into extract_docx, extract_xlsx, and the smart xlsx inference path. --- pyproject.toml | 1 + .../file_analysis/extractors/_zip_safety.py | 52 +++++++++++++++++++ .../file_analysis/extractors/docx_ext.py | 3 ++ src/noirdoc/file_analysis/extractors/xlsx.py | 13 ++++- src/noirdoc/file_analysis/xlsx_inference.py | 3 ++ tests/file_analysis/test_zip_safety.py | 46 ++++++++++++++++ 6 files changed, 117 insertions(+), 1 deletion(-) create mode 100644 src/noirdoc/file_analysis/extractors/_zip_safety.py create mode 100644 tests/file_analysis/test_zip_safety.py diff --git a/pyproject.toml b/pyproject.toml index 66318cf..c96c0c9 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -50,6 +50,7 @@ dependencies = [ "pypdfium2>=4.0.0", "python-docx>=1.1.0", "openpyxl>=3.1.0", + "defusedxml>=0.7.1", "pytesseract>=0.3.10", "Pillow>=10.0.0", "python-magic>=0.4.27", diff --git a/src/noirdoc/file_analysis/extractors/_zip_safety.py b/src/noirdoc/file_analysis/extractors/_zip_safety.py new file mode 100644 index 0000000..441d2d4 --- /dev/null +++ b/src/noirdoc/file_analysis/extractors/_zip_safety.py @@ -0,0 +1,52 @@ +"""Zip-bomb defenses for OOXML container formats (DOCX, XLSX). + +DOCX and XLSX are zip archives. python-docx / openpyxl will happily +decompress whatever the central directory advertises, which lets a tiny +file balloon into gigabytes during extraction. We pre-flight every input +through this guard so the extractor never touches a payload we can spot +as malicious before unzip. +""" + +from __future__ import annotations + +import io +import zipfile + +# Sized for "real, large business documents are fine" — a 200 MB +# uncompressed corpus is generous; legitimate DOCX/XLSX rarely exceed +# tens of MB even with embedded media. +_MAX_UNCOMPRESSED_BYTES = 200 * 1024 * 1024 + +# Real OOXML files compress well, but ratios above ~30× are unusual. +# Cap at 100× so we block obvious bombs without false-flagging the +# occasional CSV-like sheet of repetitive text. +_MAX_COMPRESSION_RATIO = 100 + + +def check_ooxml_zip_safe(data: bytes, *, label: str) -> None: + """Raise ``ValueError`` if ``data`` looks like a zip bomb. + + Validates the central directory's declared sizes before any extractor + starts decompressing entries. + """ + try: + with zipfile.ZipFile(io.BytesIO(data)) as zf: + entries = zf.infolist() + except zipfile.BadZipFile as exc: + raise ValueError(f"{label}: not a valid zip archive") from exc + + total_uncompressed = sum(zi.file_size for zi in entries) + total_compressed = sum(zi.compress_size for zi in entries) + + if total_uncompressed > _MAX_UNCOMPRESSED_BYTES: + raise ValueError( + f"{label}: archive declares {total_uncompressed} bytes uncompressed " + f"(cap is {_MAX_UNCOMPRESSED_BYTES})", + ) + + if total_compressed and total_uncompressed / total_compressed > _MAX_COMPRESSION_RATIO: + ratio = total_uncompressed / total_compressed + raise ValueError( + f"{label}: compression ratio {ratio:.0f}x exceeds {_MAX_COMPRESSION_RATIO}x — " + "refusing as zip bomb", + ) diff --git a/src/noirdoc/file_analysis/extractors/docx_ext.py b/src/noirdoc/file_analysis/extractors/docx_ext.py index 597c51f..a159168 100644 --- a/src/noirdoc/file_analysis/extractors/docx_ext.py +++ b/src/noirdoc/file_analysis/extractors/docx_ext.py @@ -4,11 +4,14 @@ import io +from noirdoc.file_analysis.extractors._zip_safety import check_ooxml_zip_safe + def extract_docx(data: bytes) -> str: """Extract text from a DOCX byte-string (paragraphs + table cells).""" from docx import Document + check_ooxml_zip_safe(data, label="docx") doc = Document(io.BytesIO(data)) parts: list[str] = [] diff --git a/src/noirdoc/file_analysis/extractors/xlsx.py b/src/noirdoc/file_analysis/extractors/xlsx.py index 8d84996..7e886ac 100644 --- a/src/noirdoc/file_analysis/extractors/xlsx.py +++ b/src/noirdoc/file_analysis/extractors/xlsx.py @@ -1,14 +1,25 @@ -"""XLSX text extraction using openpyxl.""" +"""XLSX text extraction using openpyxl. + +This helper flattens an entire workbook into a single string and is intended +for the "ship XLSX as text to a non-Excel-aware LLM" path +(``file_analysis.pipeline.convert_unsupported_files``). It is **not** suitable +for redaction — the joined text destroys cell context. Redaction must go +through :func:`noirdoc.file_analysis.xlsx_inference.pseudonymize_xlsx_smart`, +which preserves columns and writes per-cell pseudonyms. +""" from __future__ import annotations import io +from noirdoc.file_analysis.extractors._zip_safety import check_ooxml_zip_safe + def extract_xlsx(data: bytes) -> str: """Extract cell values from all sheets of an XLSX byte-string.""" from openpyxl import load_workbook + check_ooxml_zip_safe(data, label="xlsx") wb = load_workbook(io.BytesIO(data), read_only=True, data_only=True) lines: list[str] = [] try: diff --git a/src/noirdoc/file_analysis/xlsx_inference.py b/src/noirdoc/file_analysis/xlsx_inference.py index 759ed12..40f1d2d 100644 --- a/src/noirdoc/file_analysis/xlsx_inference.py +++ b/src/noirdoc/file_analysis/xlsx_inference.py @@ -136,9 +136,12 @@ async def pseudonymize_xlsx_smart( """ from openpyxl import load_workbook + from noirdoc.file_analysis.extractors._zip_safety import check_ooxml_zip_safe + result = XlsxResult() try: + check_ooxml_zip_safe(data, label="xlsx") wb = load_workbook(io.BytesIO(data)) except Exception as exc: logger.warning("xlsx_inference.load_failed", error=str(exc)) diff --git a/tests/file_analysis/test_zip_safety.py b/tests/file_analysis/test_zip_safety.py new file mode 100644 index 0000000..f7a2d56 --- /dev/null +++ b/tests/file_analysis/test_zip_safety.py @@ -0,0 +1,46 @@ +"""NDS-015: refuse OOXML inputs whose zip envelope looks like a bomb.""" + +from __future__ import annotations + +import io +import zipfile + +import pytest + +from noirdoc.file_analysis.extractors._zip_safety import check_ooxml_zip_safe + + +def _build_zip(entries: list[tuple[str, bytes]], *, compress: bool = True) -> bytes: + """Build a zip archive with the given (name, payload) entries.""" + buf = io.BytesIO() + method = zipfile.ZIP_DEFLATED if compress else zipfile.ZIP_STORED + with zipfile.ZipFile(buf, mode="w", compression=method) as zf: + for name, payload in entries: + zf.writestr(name, payload) + return buf.getvalue() + + +def test_check_ooxml_passes_for_normal_archive(): + payload = _build_zip([("word/document.xml", b"hello")]) + check_ooxml_zip_safe(payload, label="docx") + + +def test_check_ooxml_rejects_non_zip_input(): + with pytest.raises(ValueError, match="not a valid zip archive"): + check_ooxml_zip_safe(b"not a zip", label="docx") + + +def test_check_ooxml_rejects_oversized_uncompressed(): + """A single highly-compressible 300 MB blob must be refused.""" + bomb = b"A" * (300 * 1024 * 1024) + payload = _build_zip([("payload.bin", bomb)]) + with pytest.raises(ValueError, match="bytes uncompressed"): + check_ooxml_zip_safe(payload, label="xlsx") + + +def test_check_ooxml_rejects_extreme_compression_ratio(): + """A 10 MB blob of zeros compresses to a tiny file — ratio guard catches it.""" + bomb = b"\x00" * (10 * 1024 * 1024) + payload = _build_zip([("payload.bin", bomb)]) + with pytest.raises(ValueError, match="compression ratio"): + check_ooxml_zip_safe(payload, label="docx") From 14422cf5e44ec3d4343e5fcdeb368e6a8b39c9a3 Mon Sep 17 00:00:00 2001 From: Antonio Maiolo Date: Mon, 27 Apr 2026 15:46:23 +0200 Subject: [PATCH 11/13] fix: scope Image.MAX_IMAGE_PIXELS change to extract_ocr (follow-up to NDS-014) The decompression-bomb guard mutated PIL's module-global MAX_IMAGE_PIXELS without restoring it, leaking the lowered cap into later code paths (notably PDF rendering). Wrap the assignment in a try/finally so each call restores the previous value. --- src/noirdoc/file_analysis/extractors/ocr.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/noirdoc/file_analysis/extractors/ocr.py b/src/noirdoc/file_analysis/extractors/ocr.py index 0345512..0e9fa19 100644 --- a/src/noirdoc/file_analysis/extractors/ocr.py +++ b/src/noirdoc/file_analysis/extractors/ocr.py @@ -38,6 +38,7 @@ def extract_ocr(data: bytes, *, lang: str = "deu+eng") -> str: """ from PIL import Image + previous_max = Image.MAX_IMAGE_PIXELS Image.MAX_IMAGE_PIXELS = _MAX_IMAGE_PIXELS try: with warnings.catch_warnings(): @@ -46,5 +47,7 @@ def extract_ocr(data: bytes, *, lang: str = "deu+eng") -> str: img.load() except (Image.DecompressionBombError, Image.DecompressionBombWarning) as exc: raise ValueError(f"image refused: decompression bomb suspected ({exc})") from exc + finally: + Image.MAX_IMAGE_PIXELS = previous_max return ocr_image(img, lang=lang) From b88939183354094b9634b169f10e16a3ae629d04 Mon Sep 17 00:00:00 2001 From: Antonio Maiolo Date: Mon, 27 Apr 2026 15:49:59 +0200 Subject: [PATCH 12/13] fix: walk DOCX headers, footers, and comments during redact (NDS-016) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit extract_docx previously only saw paragraphs and table cells in the document body. Section headers, footers (default + first-page + even-page variants), and review comments routinely carry the same PII the body does — the detector ensemble never saw it, so it survived into the redacted output untouched. Both extract_docx and _reconstruct_docx now walk those surfaces. PII embedded in headers/footers/comments now reaches detection on the way in and gets pseudonymized in the output bytes on the way out. Footnotes, endnotes, and tracked-change deletions remain known gaps; the standard mitigation ("Accept all changes" before redacting) is documented in the README caveats. --- .../file_analysis/extractors/docx_ext.py | 52 +++++++--- src/noirdoc/file_analysis/reconstruction.py | 40 ++++++-- tests/file_analysis/test_docx_extractor.py | 99 +++++++++++++++++++ 3 files changed, 172 insertions(+), 19 deletions(-) create mode 100644 tests/file_analysis/test_docx_extractor.py diff --git a/src/noirdoc/file_analysis/extractors/docx_ext.py b/src/noirdoc/file_analysis/extractors/docx_ext.py index a159168..a79a6f7 100644 --- a/src/noirdoc/file_analysis/extractors/docx_ext.py +++ b/src/noirdoc/file_analysis/extractors/docx_ext.py @@ -7,24 +7,54 @@ from noirdoc.file_analysis.extractors._zip_safety import check_ooxml_zip_safe +def _walk_block_container(container, parts: list[str]) -> None: + """Append non-empty text from every paragraph and table cell in *container*. + + Used for the document body, headers, footers, and comments — all of + which are BlockItemContainers in python-docx and routinely carry the + same PII (author lines, "Confidential — Anna Müller" stamps, etc.) + the body does. + """ + for para in container.paragraphs: + text = para.text.strip() + if text: + parts.append(text) + for table in container.tables: + for row in table.rows: + for cell in row.cells: + for cell_para in cell.paragraphs: + cell_text = cell_para.text.strip() + if cell_text: + parts.append(cell_text) + + def extract_docx(data: bytes) -> str: - """Extract text from a DOCX byte-string (paragraphs + table cells).""" + """Extract text from a DOCX byte-string. + + Walks the document body, all section headers and footers (default, + first-page, even-page), and review comments. Headers, footers, and + comments are common PII surfaces that the detector pipeline must see + before the output is reconstructed. + """ from docx import Document check_ooxml_zip_safe(data, label="docx") doc = Document(io.BytesIO(data)) parts: list[str] = [] - for para in doc.paragraphs: - text = para.text.strip() - if text: - parts.append(text) + _walk_block_container(doc, parts) - for table in doc.tables: - for row in table.rows: - for cell in row.cells: - text = cell.text.strip() - if text: - parts.append(text) + for section in doc.sections: + for header in (section.header, section.first_page_header, section.even_page_header): + _walk_block_container(header, parts) + for footer in (section.footer, section.first_page_footer, section.even_page_footer): + _walk_block_container(footer, parts) + + try: + comments = list(doc.comments) + except Exception: + comments = [] + for comment in comments: + _walk_block_container(comment, parts) return "\n".join(parts) diff --git a/src/noirdoc/file_analysis/reconstruction.py b/src/noirdoc/file_analysis/reconstruction.py index ce46e07..d9bc1bb 100644 --- a/src/noirdoc/file_analysis/reconstruction.py +++ b/src/noirdoc/file_analysis/reconstruction.py @@ -70,8 +70,25 @@ def _reconstruct_plain(block: FileBlock) -> bytes: # --------------------------------------------------------------------------- +def _replace_in_block_container(container, replacements: dict[str, str]) -> None: + """Apply *replacements* to every paragraph in *container* and its tables.""" + for para in container.paragraphs: + _replace_in_paragraph(para, replacements) + for table in container.tables: + for row in table.rows: + for cell in row.cells: + for para in cell.paragraphs: + _replace_in_paragraph(para, replacements) + + def _reconstruct_docx(block: FileBlock) -> bytes | None: - """Find-and-replace detected entities in DOCX paragraph runs.""" + """Find-and-replace detected entities in DOCX paragraph runs. + + Covers the document body, every section's headers and footers + (default, first-page, even-page variants), and review comments — + matching the surfaces walked by :func:`extract_docx` so PII the + detector saw is also stripped from the output bytes. + """ from docx import Document try: @@ -83,13 +100,20 @@ def _reconstruct_docx(block: FileBlock) -> bytes | None: if not replacements: return block.content_bytes # nothing to replace - for para in doc.paragraphs: - _replace_in_paragraph(para, replacements) - for table in doc.tables: - for row in table.rows: - for cell in row.cells: - for para in cell.paragraphs: - _replace_in_paragraph(para, replacements) + _replace_in_block_container(doc, replacements) + + for section in doc.sections: + for header in (section.header, section.first_page_header, section.even_page_header): + _replace_in_block_container(header, replacements) + for footer in (section.footer, section.first_page_footer, section.even_page_footer): + _replace_in_block_container(footer, replacements) + + try: + comments = list(doc.comments) + except Exception: + comments = [] + for comment in comments: + _replace_in_block_container(comment, replacements) buf = io.BytesIO() doc.save(buf) diff --git a/tests/file_analysis/test_docx_extractor.py b/tests/file_analysis/test_docx_extractor.py new file mode 100644 index 0000000..4a4ff58 --- /dev/null +++ b/tests/file_analysis/test_docx_extractor.py @@ -0,0 +1,99 @@ +"""NDS-016: DOCX extraction must cover header/footer/comment surfaces.""" + +from __future__ import annotations + +import io + +from noirdoc.detection.base import DetectedEntity +from noirdoc.file_analysis.extractors.docx_ext import extract_docx +from noirdoc.file_analysis.models import FileBlock +from noirdoc.file_analysis.reconstruction import _reconstruct_docx + + +def _entity(text: str, in_text: str) -> DetectedEntity: + start = in_text.index(text) + return DetectedEntity( + entity_type="PERSON", + text=text, + start=start, + end=start + len(text), + score=0.9, + source="test", + ) + + +def _docx_with_headers_footers_and_body() -> bytes: + """Build a DOCX whose header, footer, and body each contain distinct PII.""" + from docx import Document + + doc = Document() + section = doc.sections[0] + section.header.paragraphs[0].text = "Header: Anna Mueller" + section.footer.paragraphs[0].text = "Footer: Bernd Schmidt" + doc.add_paragraph("Body: Carla Weber") + + buf = io.BytesIO() + doc.save(buf) + return buf.getvalue() + + +def test_extract_docx_walks_headers_and_footers(): + """PII embedded in section headers and footers must reach the detector.""" + text = extract_docx(_docx_with_headers_footers_and_body()) + assert "Anna Mueller" in text + assert "Bernd Schmidt" in text + assert "Carla Weber" in text + + +def test_extract_docx_walks_comments(): + """Review comments are a routine PII surface — they must be extracted.""" + from docx import Document + + doc = Document() + para = doc.add_paragraph("Body text") + doc.add_comment(runs=[para.runs[0]] if para.runs else [], text="Reviewer: Dora Klein") + buf = io.BytesIO() + doc.save(buf) + + text = extract_docx(buf.getvalue()) + assert "Dora Klein" in text + + +def test_reconstruct_docx_replaces_text_in_headers_and_footers(): + """Reconstruction must scrub header/footer text so the output bytes are clean.""" + docx_bytes = _docx_with_headers_footers_and_body() + extracted = extract_docx(docx_bytes) + + # Build a synthetic pseudonymized result: replace each name with a token + pseudonymized = ( + extracted.replace("Anna Mueller", "<>") + .replace("Bernd Schmidt", "<>") + .replace("Carla Weber", "<>") + ) + + entities = [ + _entity("Anna Mueller", extracted), + _entity("Bernd Schmidt", extracted), + _entity("Carla Weber", extracted), + ] + + block = FileBlock( + content_bytes=docx_bytes, + mime_type="application/vnd.openxmlformats-officedocument.wordprocessingml.document", + source_path="test.docx", + source_type="file", + extracted_text=extracted, + pseudonymized_text=pseudonymized, + entities=entities, + ) + + new_bytes = _reconstruct_docx(block) + assert new_bytes is not None + + rewritten = extract_docx(new_bytes) + assert "Anna Mueller" not in rewritten + assert "Bernd Schmidt" not in rewritten + assert "Carla Weber" not in rewritten + assert "<>" in rewritten + assert "<>" in rewritten + assert "<>" in rewritten From 26858884bfd422b536b98407fd652971123ffa12 Mon Sep 17 00:00:00 2001 From: Antonio Maiolo Date: Mon, 27 Apr 2026 15:53:36 +0200 Subject: [PATCH 13/13] =?UTF-8?q?docs(changelog):=20cut=200.1.2=20?= =?UTF-8?q?=E2=80=94=20security=20patch=20for=20all=20High=20findings?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 11 High-severity findings from the internal 0.1.1 security review are addressed in this release: PDF metadata leak (NDS-017), DOCX header/footer/comment coverage (NDS-016), OOXML zip-bomb defense and defusedxml (NDS-015), OCR decompression-bomb guard (NDS-014), detector failure surfacing (NDS-023), daemon protocol size limits (NDS-027), daemon path-trust check (NDS-028), namespace name validation (NDS-009), Fernet key TOCTOU + 0700 dir mode (NDS-004), CLI output-path traversal guard (NDS-034), and ns-show --unsafe gate (NDS-033). --- CHANGELOG.md | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 56 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3fa2b2f..b28905a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,60 @@ and [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ## [Unreleased] +## [0.1.2] — 2026-04-27 + +Security patch covering all High-severity findings from the 0.1.1 +internal security review. Recommended upgrade for anyone running +0.1.x. + +### Security +- **PDF metadata leak.** PII embedded in a PDF's `/Info` dictionary + (Author, Title, Subject, Creator, Producer, Keywords) was passed + through unchanged. Metadata fields are now extracted with the page + text so the detector ensemble pseudonymizes them before output. +- **DOCX header / footer / comment leak.** `extract_docx` and + `_reconstruct_docx` only walked paragraphs and table cells — section + headers, footers (default + first-page + even-page), and review + comments survived untouched. All three surfaces are now extracted + on input and rewritten on output. +- **OOXML zip-bomb defense.** DOCX and XLSX inputs are now pre-flighted + through a zip-envelope check that refuses archives declaring more + than 200 MB uncompressed or a compression ratio above 100×. +- **XML entity expansion.** `defusedxml` is now a baseline dependency + so openpyxl's `iterparse` path is entity-safe by default. python-docx + was already pinned to a `resolve_entities=False` parser. +- **Image decompression-bomb DoS.** OCR extraction now caps + `Image.MAX_IMAGE_PIXELS` at 50 megapixels and converts + `DecompressionBombWarning` into a hard refusal. The cap is + scoped per-call so it cannot leak into other PIL consumers. +- **Detector ensemble silent failure.** A failing detector inside + `EnsembleDetector` no longer produces an empty result silently; + the failure is logged with the detector name so observability + catches partial-coverage regressions. +- **Daemon protocol size limits.** `asyncio.start_unix_server` and + the matching client now cap line buffers at 32 MB, and the + protocol enforces per-field length caps (16 MB text, 4 KB paths, + 64 chars for namespace names). +- **Daemon path-trust check.** `handle_redact` now refuses input + files and output directories that are not owned by the current + UID. A peer cannot ask the daemon to read or overwrite another + user's files. +- **Namespace name validation.** Namespace names are restricted to + `^[A-Za-z0-9][A-Za-z0-9._-]{0,63}$`, blocking path traversal + (`../`, `/`, leading dot) into or out of the namespace store. +- **Fernet key TOCTOU.** Per-namespace key creation now uses + `O_CREAT | O_EXCL` with `0600`, the namespace directory is + created with `0700`, and concurrent first-load races no longer + clobber an existing key. +- **CLI output-path traversal.** `noirdoc redact -o / --output-dir` + is now guarded against crafted input paths that resolved outside + the chosen output directory, and refuses to write into the + namespace store. +- **`ns show` requires `--unsafe`.** Printing a namespace's full + pseudonym ↔ original mapping reveals every original value. The + command now exits with an error and points at `noirdoc ns + summary` unless `--unsafe` is passed. + ## [0.1.1] — 2026-04-27 ### Added @@ -58,6 +112,7 @@ First public alpha on PyPI. a `UserWarning`, and keeps working. Explicit `--detector gliner` still fails loudly when the `[full]` extra isn't installed. -[Unreleased]: https://github.com/nextaim-de/noirdoc/compare/v0.1.1...HEAD +[Unreleased]: https://github.com/nextaim-de/noirdoc/compare/v0.1.2...HEAD +[0.1.2]: https://github.com/nextaim-de/noirdoc/releases/tag/v0.1.2 [0.1.1]: https://github.com/nextaim-de/noirdoc/releases/tag/v0.1.1 [0.1.0]: https://github.com/nextaim-de/noirdoc/releases/tag/v0.1.0