From 5787ca2271887bc7f1b1ac7ef87ceacf93053476 Mon Sep 17 00:00:00 2001 From: abrichr Date: Fri, 9 Oct 2026 23:58:55 -0400 Subject: [PATCH 1/3] fix: fail the PHI scan when the root is missing, empty, or unreadable openadapt-privacy-scan printed "PHI scan clean (0 files)" and exited 0 when --root did not exist, pointed at a file, or pointed at an empty directory. It also skipped directories and files it could not read and still reported clean. A mistyped path in CI turned the gate into a silent pass. The scan now exits 2 when it cannot run: the root is missing or is not a directory, a directory under it cannot be listed (from os.walk or from git ls-files, which warns and exits 0), or it has no files to scan. --allow-empty opts in to an empty tree. A file that cannot be opened is reported as an unreadable finding (exit 1) instead of being skipped as binary. scan_tree() raises OSError for a missing or non-directory root. The clean message counts the files that were scanned, and the README lists the exit codes. Ledger: B123 Co-Authored-By: Claude Opus 5.5 --- README.md | 8 ++++ openadapt_privacy/scan.py | 68 ++++++++++++++++++++++++++---- tests/test_scan.py | 87 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 156 insertions(+), 7 deletions(-) diff --git a/README.md b/README.md index bea464e..db1981d 100644 --- a/README.md +++ b/README.md @@ -45,6 +45,14 @@ openadapt-privacy-scan --self-test openadapt-privacy-scan --root /path/to/repo ``` +The command exits with: + +- `0` when it read every file and no rule fired. +- `1` when a rule fired or a file couldn't be read. +- `2` when the scan couldn't run: the root is missing or isn't a directory, a + directory under it can't be listed, or it has no files. Pass `--allow-empty` + if an empty tree is expected. + `[scan]` adds no packages. The scanner is stdlib, so `import openadapt_privacy.scan` works on a bare `pip install openadapt-privacy` and does not load Presidio, spaCy, or Pillow. `python -m openadapt_privacy.scan` is the same CLI. diff --git a/openadapt_privacy/scan.py b/openadapt_privacy/scan.py index 6e6e962..cb0d3bf 100644 --- a/openadapt_privacy/scan.py +++ b/openadapt_privacy/scan.py @@ -101,16 +101,28 @@ def _git_files(root: Path) -> list[Path] | None: ], check=True, capture_output=True, + # English messages, so the check below can read them. + env={**os.environ, "LC_ALL": "C"}, ) except (subprocess.CalledProcessError, FileNotFoundError): return None + # git skips a directory it can't read, warns, and still exits 0. + err = out.stderr.decode("utf-8", "replace") + if "could not open directory" in err or "Permission denied" in err: + raise PermissionError(err.strip()) names = [n for n in out.stdout.split(b"\0") if n] return [root / n.decode("utf-8", "surrogateescape") for n in names] +def _raise(exc: OSError) -> None: + raise exc + + def _walk_files(root: Path) -> list[Path]: files: list[Path] = [] - for dirpath, dirnames, filenames in os.walk(root): + # Raise on an unreadable directory. Skipping it would report a clean scan + # of files that were never read. + for dirpath, dirnames, filenames in os.walk(root, onerror=_raise): dirnames[:] = [d for d in dirnames if d not in SKIP_DIR_NAMES] for name in filenames: files.append(Path(dirpath) / name) @@ -118,6 +130,11 @@ def _walk_files(root: Path) -> list[Path]: def iter_scan_files(root: Path) -> list[Path]: + """List the files to scan. Raise ``OSError`` if ``root`` can't be listed.""" + if not root.exists(): + raise FileNotFoundError(f"{root} does not exist") + if not root.is_dir(): + raise NotADirectoryError(f"{root} is not a directory") tracked = _git_files(root) if tracked is not None: return [p for p in tracked if p.is_file()] @@ -129,7 +146,9 @@ def _is_binary(path: Path) -> bool: with path.open("rb") as fh: chunk = fh.read(8192) except OSError: - return True + # Not binary as far as we know. scan_contents then reports it as + # unreadable instead of skipping it. + return False return b"\0" in chunk @@ -172,9 +191,14 @@ def scan_contents(root: Path, files: list[Path]) -> list[str]: return hits -def scan_tree(root: Path) -> list[str]: - """Return the same hit lines the clinic git scanner printed.""" - files = iter_scan_files(root) +def scan_tree(root: Path, files: list[Path] | None = None) -> list[str]: + """Return the same hit lines the clinic git scanner printed. + + Raise ``OSError`` if ``root`` is missing, is not a directory, or has a + directory that can't be listed. + """ + if files is None: + files = iter_scan_files(root) return scan_forbidden_paths(root, files) + scan_contents(root, files) @@ -257,19 +281,49 @@ def main(argv: list[str] | None = None) -> int: default=None, help="tree to scan (default: current working directory)", ) + parser.add_argument( + "--allow-empty", + action="store_true", + help="exit 0 when the tree has no files to scan (default: exit 2)", + ) args = parser.parse_args(argv) if args.self_test: return self_test() + # Exit 2 when the scan could not run, so CI can tell it apart from a + # finding (exit 1). A scan of nothing must never report clean. root = (args.root if args.root is not None else Path.cwd()).resolve() - hits = scan_tree(root) + if not root.is_dir(): + print( + f"PHI scan error: root {root} does not exist or is not a directory. " + "Nothing was scanned.", + file=sys.stderr, + ) + return 2 + try: + files = iter_scan_files(root) + except OSError as exc: + print( + f"PHI scan error: could not list files under {root}: {exc}. " + "Nothing was scanned.", + file=sys.stderr, + ) + return 2 + if not files and not args.allow_empty: + print( + f"PHI scan error: no files to scan under {root}. " + "Pass --allow-empty if an empty tree is expected.", + file=sys.stderr, + ) + return 2 + hits = scan_tree(root, files) if hits: print("PHI / clinic-path scan failed:", file=sys.stderr) for hit in hits: print(hit, file=sys.stderr) return 1 - print(f"PHI scan clean ({len(iter_scan_files(root))} files)") + print(f"PHI scan clean ({len(files)} files)") return 0 diff --git a/tests/test_scan.py b/tests/test_scan.py index 2a46b09..2eb7e79 100644 --- a/tests/test_scan.py +++ b/tests/test_scan.py @@ -3,6 +3,7 @@ from __future__ import annotations import ast +import os import re import subprocess import sys @@ -210,6 +211,91 @@ def test_cli_root_exits_zero_when_clean(tmp_path: Path) -> None: assert main(["--root", str(tmp_path)]) == 0 +def test_cli_missing_root_fails_closed( + tmp_path: Path, capsys: pytest.CaptureFixture[str] +) -> None: + assert main(["--root", str(tmp_path / "nope")]) == 2 + captured = capsys.readouterr() + assert "does not exist" in captured.err + assert "clean" not in captured.out + captured.err + + +def test_cli_file_root_fails_closed( + tmp_path: Path, capsys: pytest.CaptureFixture[str] +) -> None: + target = tmp_path / "file.txt" + target.write_text("hello\n", encoding="utf-8") + assert main(["--root", str(target)]) == 2 + captured = capsys.readouterr() + assert "not a directory" in captured.err + assert "clean" not in captured.out + captured.err + + +@pytest.mark.parametrize("use_git", [False, True], ids=["plain-dir", "git-repo"]) +def test_cli_empty_root_fails_closed( + tmp_path: Path, capsys: pytest.CaptureFixture[str], use_git: bool +) -> None: + if use_git: + _git_init(tmp_path) + assert main(["--root", str(tmp_path)]) == 2 + captured = capsys.readouterr() + assert "no files" in captured.err + assert "clean" not in captured.out + captured.err + + +def test_cli_empty_root_passes_with_allow_empty(tmp_path: Path) -> None: + assert main(["--root", str(tmp_path), "--allow-empty"]) == 0 + + +def test_scan_tree_missing_root_raises(tmp_path: Path) -> None: + with pytest.raises(OSError): + scan_tree(tmp_path / "nope") + + +def _skip_if_root_user() -> None: + if hasattr(os, "geteuid") and os.geteuid() == 0: + pytest.skip("root can read files whatever their permissions") + + +@pytest.mark.parametrize("use_git", [False, True], ids=["plain-dir", "git-repo"]) +def test_cli_unreadable_subdirectory_fails_closed( + tmp_path: Path, capsys: pytest.CaptureFixture[str], use_git: bool +) -> None: + _skip_if_root_user() + if use_git: + _git_init(tmp_path) + (tmp_path / "ok.txt").write_text("hello\n", encoding="utf-8") + locked = tmp_path / "locked" + locked.mkdir() + (locked / "note.txt").write_text("hello\n", encoding="utf-8") + locked.chmod(0) + try: + code = main(["--root", str(tmp_path)]) + finally: + locked.chmod(0o700) + assert code == 2 + captured = capsys.readouterr() + assert "could not list files" in captured.err + assert "clean" not in captured.out + captured.err + + +def test_cli_unreadable_file_is_a_finding( + tmp_path: Path, capsys: pytest.CaptureFixture[str] +) -> None: + _skip_if_root_user() + (tmp_path / "ok.txt").write_text("hello\n", encoding="utf-8") + locked = tmp_path / "locked.txt" + locked.write_text("hello\n", encoding="utf-8") + locked.chmod(0) + try: + code = main(["--root", str(tmp_path)]) + finally: + locked.chmod(0o600) + assert code == 1 + captured = capsys.readouterr() + assert "unreadable\tlocked.txt" in captured.err + + def test_module_cli_self_test() -> None: completed = subprocess.run( [sys.executable, "-m", "openadapt_privacy.scan", "--self-test"], @@ -248,3 +334,4 @@ def test_readme_keeps_presidio_caveat_and_has_scan_section() -> None: assert "## Git / CI scan (`[scan]`)" in readme assert "synthetic, not clinical" in readme assert "does not claim clinical validation" in readme + assert "--allow-empty" in readme From aa8cb190d27b2e21a6b339b57391d26a7ee10df6 Mon Sep 17 00:00:00 2001 From: abrichr Date: Sat, 10 Oct 2026 00:00:08 -0400 Subject: [PATCH 2/3] fix: stop requiring pip at install time pyproject.toml listed "pip>=25.3" as a runtime dependency. The package never imports or runs pip, but installing openadapt-privacy (or any package that depends on it) into an environment with an older pip upgraded the user's pip, and it conflicted with environments that pin pip on purpose. Remove pip from the runtime dependencies. Keep the pip>=25.3 floor in the one CI step that installs pip for the spaCy model download. A new test checks that the runtime dependencies and user extras do not list pip, setuptools, wheel, or uv. Ledger: B124 Co-Authored-By: Claude Opus 5.5 --- .github/workflows/test.yml | 2 +- pyproject.toml | 1 - tests/test_packaging.py | 31 +++++++++++++++++++++++++++++++ 3 files changed, 32 insertions(+), 2 deletions(-) create mode 100644 tests/test_packaging.py diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 366e09b..ce95126 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -53,7 +53,7 @@ jobs: - name: Install dependencies with Presidio run: | uv sync --extra dev --extra presidio - uv pip install pip + uv pip install "pip>=25.3" uv run python -m spacy download en_core_web_sm - name: Run all tests diff --git a/pyproject.toml b/pyproject.toml index 329fb5a..450e74c 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -23,7 +23,6 @@ classifiers = [ dependencies = [ "pillow>=10.0.0", - "pip>=25.3", "pydantic>=2.0.0", ] diff --git a/tests/test_packaging.py b/tests/test_packaging.py new file mode 100644 index 0000000..4aecc1a --- /dev/null +++ b/tests/test_packaging.py @@ -0,0 +1,31 @@ +"""Installing the library must not change the user's install tools.""" + +from __future__ import annotations + +from pathlib import Path + +from packaging.requirements import Requirement +from packaging.utils import canonicalize_name + +try: + import tomllib +except ModuleNotFoundError: # Python 3.10 + import tomli as tomllib + +ROOT = Path(__file__).resolve().parents[1] +INSTALL_TOOLS = {"pip", "setuptools", "wheel", "uv"} + + +def test_runtime_and_user_extras_do_not_require_install_tools() -> None: + project = tomllib.loads((ROOT / "pyproject.toml").read_text(encoding="utf-8"))["project"] + groups = {"dependencies": project.get("dependencies", [])} + for extra, requirements in project.get("optional-dependencies", {}).items(): + if extra != "dev": + groups[f"[{extra}]"] = requirements + offenders = [ + f"{group}: {spec}" + for group, requirements in groups.items() + for spec in requirements + if canonicalize_name(Requirement(spec).name) in INSTALL_TOOLS + ] + assert offenders == [] From 08f193712873f3898415f51de1cd9ce429cd3a55 Mon Sep 17 00:00:00 2001 From: abrichr Date: Sat, 10 Oct 2026 00:18:35 -0400 Subject: [PATCH 3/3] fix: keep the scan's fail-closed checks precise across Python versions - Treat a git permission warning as a failed listing only when it names a path in the tree. An unreadable global ignore file under $HOME no longer turns a complete scan into "Nothing was scanned" (exit 2). - Check each listed path with os.stat and raise on a permission error. On Python 3.14, Path.is_file() returns False instead, so files under a directory without search permission dropped out and the scan reported clean. - Exit 2, not a traceback, when the root's parent can't be searched. Co-Authored-By: Claude Opus 5.5 --- README.md | 10 ++++-- openadapt_privacy/scan.py | 41 ++++++++++++++++++----- tests/test_scan.py | 69 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 109 insertions(+), 11 deletions(-) diff --git a/README.md b/README.md index db1981d..ec46f6c 100644 --- a/README.md +++ b/README.md @@ -49,9 +49,9 @@ The command exits with: - `0` when it read every file and no rule fired. - `1` when a rule fired or a file couldn't be read. -- `2` when the scan couldn't run: the root is missing or isn't a directory, a - directory under it can't be listed, or it has no files. Pass `--allow-empty` - if an empty tree is expected. +- `2` when the scan couldn't run: the root is missing, isn't a directory, or + can't be accessed; a directory under it can't be listed; or it has no files. + Pass `--allow-empty` if an empty tree is expected. `[scan]` adds no packages. The scanner is stdlib, so `import openadapt_privacy.scan` works on a bare `pip install openadapt-privacy` and does not load Presidio, @@ -65,6 +65,10 @@ hits = scan_tree(Path(".")) # ["ohip-dashed\tfile.txt:3", ...] self_test() # 0 ok, 1 a rule stayed silent ``` +`scan_tree` raises `OSError` when the root is missing, isn't a directory, or has +a directory that can't be listed. It returns `[]` for a tree with no files, so +check `iter_scan_files(root)` first if an empty tree should fail. + `--self-test` plants fixtures under `/tmp` and exits 1 if a rule does not fire. Matching OHIP examples are not stored in the library; they're built at runtime. diff --git a/openadapt_privacy/scan.py b/openadapt_privacy/scan.py index cb0d3bf..f961b10 100644 --- a/openadapt_privacy/scan.py +++ b/openadapt_privacy/scan.py @@ -13,6 +13,7 @@ from __future__ import annotations import argparse +import errno import os import re import stat @@ -106,10 +107,19 @@ def _git_files(root: Path) -> list[Path] | None: ) except (subprocess.CalledProcessError, FileNotFoundError): return None - # git skips a directory it can't read, warns, and still exits 0. + # git skips a directory it can't read, warns, and still exits 0, so treat a + # permission warning as a failed listing. git names paths in the tree + # relative to it. The exception is an absolute path, such as a global ignore + # file under an unreadable $HOME in a CI container: an ignore file git can't + # read hides no files. err = out.stderr.decode("utf-8", "replace") - if "could not open directory" in err or "Permission denied" in err: - raise PermissionError(err.strip()) + for line in err.splitlines(): + quoted = re.search(r"'([^']*)'", line) + outside = quoted is not None and os.path.isabs(quoted.group(1)) + if "could not open directory" in line or ( + "Permission denied" in line and not outside + ): + raise PermissionError(err.strip()) names = [n for n in out.stdout.split(b"\0") if n] return [root / n.decode("utf-8", "surrogateescape") for n in names] @@ -129,6 +139,20 @@ def _walk_files(root: Path) -> list[Path]: return files +def _is_file(path: Path) -> bool: + """Return whether ``path`` is a regular file. Raise if it can't be checked. + + ``Path.is_file()`` returns False on a permission error from Python 3.14, so + a file under a directory that can't be searched would drop out of the scan. + """ + try: + return stat.S_ISREG(path.stat().st_mode) + except OSError as exc: + if exc.errno in (errno.ENOENT, errno.ENOTDIR, errno.EBADF, errno.ELOOP): + return False + raise + + def iter_scan_files(root: Path) -> list[Path]: """List the files to scan. Raise ``OSError`` if ``root`` can't be listed.""" if not root.exists(): @@ -137,8 +161,8 @@ def iter_scan_files(root: Path) -> list[Path]: raise NotADirectoryError(f"{root} is not a directory") tracked = _git_files(root) if tracked is not None: - return [p for p in tracked if p.is_file()] - return [p for p in _walk_files(root) if p.is_file()] + return [p for p in tracked if _is_file(p)] + return [p for p in _walk_files(root) if _is_file(p)] def _is_binary(path: Path) -> bool: @@ -294,10 +318,11 @@ def main(argv: list[str] | None = None) -> int: # Exit 2 when the scan could not run, so CI can tell it apart from a # finding (exit 1). A scan of nothing must never report clean. root = (args.root if args.root is not None else Path.cwd()).resolve() - if not root.is_dir(): + # os.path.isdir is False, not an exception, when a parent can't be searched. + if not os.path.isdir(root): print( - f"PHI scan error: root {root} does not exist or is not a directory. " - "Nothing was scanned.", + f"PHI scan error: root {root} does not exist, is not a directory, " + "or can't be accessed. Nothing was scanned.", file=sys.stderr, ) return 2 diff --git a/tests/test_scan.py b/tests/test_scan.py index 2eb7e79..0aa0b9a 100644 --- a/tests/test_scan.py +++ b/tests/test_scan.py @@ -279,6 +279,75 @@ def test_cli_unreadable_subdirectory_fails_closed( assert "clean" not in captured.out + captured.err +@pytest.mark.parametrize("use_git", [False, True], ids=["plain-dir", "git-repo"]) +def test_cli_unsearchable_subdirectory_fails_closed( + tmp_path: Path, capsys: pytest.CaptureFixture[str], use_git: bool +) -> None: + # Read but no execute permission: the names list, but the files can't be + # opened. Python 3.14's Path.is_file() returns False here instead of raising. + _skip_if_root_user() + if use_git: + _git_init(tmp_path) + (tmp_path / "ok.txt").write_text("hello\n", encoding="utf-8") + half = tmp_path / "half" + half.mkdir() + (half / "note.txt").write_text("hello\n", encoding="utf-8") + half.chmod(0o444) + try: + code = main(["--root", str(tmp_path)]) + finally: + half.chmod(0o700) + assert code == 2 + captured = capsys.readouterr() + assert "could not list files" in captured.err + assert "clean" not in captured.out + captured.err + + +def test_cli_root_under_unsearchable_parent_fails_closed( + tmp_path: Path, capsys: pytest.CaptureFixture[str] +) -> None: + _skip_if_root_user() + locked = tmp_path / "locked" + root = locked / "sub" + root.mkdir(parents=True) + (root / "ok.txt").write_text("hello\n", encoding="utf-8") + locked.chmod(0) + try: + code = main(["--root", str(root)]) + finally: + locked.chmod(0o700) + assert code == 2 + captured = capsys.readouterr() + assert "can't be accessed" in captured.err + assert "clean" not in captured.out + captured.err + + +def test_cli_unreadable_global_git_ignore_still_scans( + tmp_path: Path, capsys: pytest.CaptureFixture[str], monkeypatch: pytest.MonkeyPatch +) -> None: + # git warns "unable to access ...: Permission denied" for an unreadable + # global ignore file, but it still lists every file. That's not a failed scan. + _skip_if_root_user() + repo = tmp_path / "repo" + repo.mkdir() + _git_init(repo) + (repo / "ok.txt").write_text("hello\n", encoding="utf-8") + home = tmp_path / "home" + ignore = home / ".config" / "git" / "ignore" + ignore.parent.mkdir(parents=True) + ignore.write_text("*.txt\n", encoding="utf-8") + ignore.chmod(0) + monkeypatch.setenv("HOME", str(home)) + monkeypatch.delenv("XDG_CONFIG_HOME", raising=False) + try: + code = main(["--root", str(repo)]) + finally: + ignore.chmod(0o600) + captured = capsys.readouterr() + assert code == 0, captured.err + assert "PHI scan clean (1 files)" in captured.out + + def test_cli_unreadable_file_is_a_finding( tmp_path: Path, capsys: pytest.CaptureFixture[str] ) -> None: