From 91b5a852afabcb775aeaf12c3fba946409e7a853 Mon Sep 17 00:00:00 2001 From: Itecz Solution Date: Wed, 29 Jul 2026 23:36:27 +0530 Subject: [PATCH] raise instead of skipping model directories that cannot be listed check_file_or_directory accepts any path that stats as a directory, but glob("**/*") drops a subtree it cannot list without surfacing the error. CPython swallows the failure inside the per-directory generator (PermissionError on 3.10-3.12, OSError in glob._Globber on 3.13+), and the manifest records only files, so an unreadable directory contributes zero entries and nothing raises. That leaves a model tree that verifies clean while part of it was never hashed: mkdir model/evil; cp backdoor.py model/evil/; chmod 0111 model/evil keeps backdoor.py out of the manifest and verify still reports success. Mode 0111 still grants search, so whatever loads the model opens the planted file normally. The helper now scandirs any directory it is handed and raises when the listing fails, which its own docstring already claimed it did for permission errors. Both the file and shard serializers call it, so one check covers both walk loops. Signed-off-by: Itecz Solution --- CHANGELOG.md | 1 + .../_serialization/serialization.py | 19 +++++++++++-- tests/_serialization/file_test.py | 28 +++++++++++++++++++ tests/test_support.py | 18 ++++++++++++ 4 files changed, 64 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7ff870d0..c430f381 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,7 @@ All versions prior to 1.0.0 are untracked. - Standardized CLI flags to use hyphens (e.g., `--trust-config` instead of `--trust_config`). Underscore variants are still accepted for backwards compatibility via token normalization. ### Fixed +- Fixed a bug where a directory that could not be listed was skipped during serialization instead of raising. Every file under it was left out of the manifest, so a model carrying such a directory verified successfully even though its contents were never hashed. ([#652](https://github.com/sigstore/model-transparency/pull/652)) - Fixed a bug where reusing a single `verifying.Config` across models let the ignore paths and guessed hashing configuration from one verification carry over into later ones. A file that a later model's signature never excluded could be silently skipped instead of reported as unsigned. ([#650](https://github.com/sigstore/model-transparency/pull/650)) - Fixed a bug where installing from the sdist produced an empty wheel with zero Python modules. The hatch `packages` directive was scoped to all build targets instead of the wheel target only, causing the sdist's flattened layout to not match the expected `src/` path. ([#636](https://github.com/sigstore/model-transparency/issues/636)) - Fixed a bug where ignored symlinks could raise `ValueError`s if allow_symlinks was unset, even though they were skipped during serialization. ([#550](https://github.com/sigstore/model-transparency/pull/550)) diff --git a/src/model_signing/_serialization/serialization.py b/src/model_signing/_serialization/serialization.py index ba47bb43..1ca65cbc 100644 --- a/src/model_signing/_serialization/serialization.py +++ b/src/model_signing/_serialization/serialization.py @@ -16,6 +16,7 @@ import abc from collections.abc import Iterable +import os import pathlib from model_signing import manifest @@ -39,8 +40,9 @@ def check_file_or_directory( serialization would raise an error. Raises: - ValueError: The path is neither a file or a directory, or the path - is a symlink and `allow_symlinks` is false. + ValueError: The path is neither a file or a directory, the path is a + symlink and `allow_symlinks` is false, or the path is a directory + whose entries cannot be listed. """ if not allow_symlinks and path.is_symlink(): raise ValueError( @@ -53,6 +55,19 @@ def check_file_or_directory( " special file, it could be missing, or there might be a" " permission issue." ) + if path.is_dir(): + # Listing a directory needs read permission, while `is_dir` only needs + # search permission on the parent. `glob` drops a subtree it cannot + # list without reporting it, so check here instead of silently leaving + # the files it holds out of the manifest. + try: + with os.scandir(path): + pass + except OSError as err: + raise ValueError( + f"Cannot list the contents of '{path}'. Any file under it" + " would be missing from the manifest." + ) from err def should_ignore( diff --git a/tests/_serialization/file_test.py b/tests/_serialization/file_test.py index 9659e8e9..63d74308 100644 --- a/tests/_serialization/file_test.py +++ b/tests/_serialization/file_test.py @@ -326,6 +326,20 @@ def test_ignored_symlinks_dont_raise_error(self, symlink_model_folder): symlink_model_folder, ignore_paths=[symlink_model_folder] ) + def test_unlistable_directory_raises(self, sample_model_folder): + hidden_dir = sample_model_folder / "hidden" + hidden_dir.mkdir() + (hidden_dir / "extra_file").write_bytes(b"not in the manifest") + if not test_support.make_unlistable(hidden_dir): + return # trivially pass where permissions cannot restrict listing + + serializer = file.Serializer(self._hasher_factory) + try: + with pytest.raises(ValueError, match="Cannot list the contents of"): + _ = serializer.serialize(sample_model_folder) + finally: + hidden_dir.chmod(0o755) + class TestUtilities: def test_check_file_or_directory_raises_on_pipes(self, sample_model_file): @@ -341,3 +355,17 @@ def test_check_file_or_directory_raises_on_pipes(self, sample_model_file): ValueError, match="Cannot use .* as file or directory" ): serialization.check_file_or_directory(pipe) + + def test_check_file_or_directory_raises_on_unlistable_directory( + self, sample_model_folder + ): + directory = sample_model_folder / "unlistable" + directory.mkdir() + if not test_support.make_unlistable(directory): + return # trivially pass where permissions cannot restrict listing + + try: + with pytest.raises(ValueError, match="Cannot list the contents of"): + serialization.check_file_or_directory(directory) + finally: + directory.chmod(0o755) diff --git a/tests/test_support.py b/tests/test_support.py index 7943610a..f88f4a33 100644 --- a/tests/test_support.py +++ b/tests/test_support.py @@ -15,6 +15,7 @@ """Helpers and constants used in fixtures and tests. Not in the public API.""" import itertools +import os import pathlib from model_signing import manifest @@ -113,3 +114,20 @@ def count_files(path: pathlib.Path) -> int: if child_path.is_file(): count += 1 return count + + +def make_unlistable(path: pathlib.Path) -> bool: + """Drops read permission on a directory, keeping it searchable. + + Returns whether the directory actually became unlistable. Windows does not + map this onto file modes, and a user with CAP_DAC_READ_SEARCH (such as + root) bypasses the check, so callers must skip when this returns False. + """ + path.chmod(0o111) + try: + with os.scandir(path): + pass + except OSError: + return True + path.chmod(0o755) + return False