Merge pull request #1 from ashutoshcipher/codex/find-and-fix-bugs-in-code

Fix ignored path handling in hashing configuration

Signed-off-by: Ashutosh Gupta <[email protected]>

Author: Ashutosh Gupta

scripts/tests/verify_test.py

@@ -41,7 +41,7 @@ def test_verify_key_v0_3_1(self, base_path: pathlib.Path):
             public_key=public_key
         ).set_hashing_config(
             model_signing.hashing.Config().set_ignored_paths(
-                paths=list(ignore_paths) + [signature],
+                paths=list(ignore_paths) + [signature.name],
                 ignore_git_paths=ignore_git_paths,
             )
         ).verify(model_path, signature)
@@ -61,7 +61,7 @@ def test_verify_certificate_v0_3_1(self, base_path: pathlib.Path):
             log_fingerprints=log_fingerprints,
         ).set_hashing_config(
             model_signing.hashing.Config().set_ignored_paths(
-                paths=list(ignore_paths) + [signature],
+                paths=list(ignore_paths) + [signature.name],
                 ignore_git_paths=ignore_git_paths,
             )
         ).verify(model_path, signature)
@@ -83,7 +83,7 @@ def test_verify_sigstore_v0_3_1(self, base_path: pathlib.Path):
             use_staging=use_staging,
         ).set_hashing_config(
             model_signing.hashing.Config().set_ignored_paths(
-                paths=list(ignore_paths) + [signature],
+                paths=list(ignore_paths) + [signature.name],
                 ignore_git_paths=ignore_git_paths,
             )
         ).verify(model_path, signature)
@@ -101,7 +101,7 @@ def test_verify_key_v1_0_0(self, base_path: pathlib.Path):
             public_key=public_key
         ).set_hashing_config(
             model_signing.hashing.Config().set_ignored_paths(
-                paths=list(ignore_paths) + [signature],
+                paths=list(ignore_paths) + [signature.name],
                 ignore_git_paths=ignore_git_paths,
             )
         ).verify(model_path, signature)
@@ -121,7 +121,7 @@ def test_verify_certificate_v1_0_0(self, base_path: pathlib.Path):
             log_fingerprints=log_fingerprints,
         ).set_hashing_config(
             model_signing.hashing.Config().set_ignored_paths(
-                paths=list(ignore_paths) + [signature],
+                paths=list(ignore_paths) + [signature.name],
                 ignore_git_paths=ignore_git_paths,
             )
         ).verify(model_path, signature)
@@ -143,7 +143,7 @@ def test_verify_sigstore_v1_0_0(self, base_path: pathlib.Path):
             use_staging=use_staging,
         ).set_hashing_config(
             model_signing.hashing.Config().set_ignored_paths(
-                paths=list(ignore_paths) + [signature],
+                paths=list(ignore_paths) + [signature.name],
                 ignore_git_paths=ignore_git_paths,
             )
         ).verify(model_path, signature)

src/model_signing/hashing.py

@@ -148,18 +148,23 @@ def hash(
         files_to_hash: Optional[Iterable[PathLike]] = None,
     ) -> manifest.Manifest:
         """Hashes a model using the current configuration."""
-        # All paths in ignored_paths must have model_path as prefix
+        # All paths in ``_ignored_paths`` are expected to be relative to the
+        # model directory. Join them to ``model_path`` and ensure they do not
+        # escape it.
+        model_path = pathlib.Path(model_path)
         ignored_paths = []
         for p in self._ignored_paths:
-            rp = os.path.relpath(p, model_path)
-            # rp may start with "../" if it is not relative to model_path
-            if not rp.startswith("../"):
-                ignored_paths.append(pathlib.Path(os.path.join(model_path, rp)))
+            full = model_path / p
+            try:
+                full.relative_to(model_path)
+            except ValueError:
+                continue
+            ignored_paths.append(full)
 
         if self._ignore_git_paths:
             ignored_paths.extend(
                 [
-                    os.path.join(model_path, p)
+                    model_path / p
                     for p in [
                         ".git/",
                         ".gitattributes",
@@ -357,11 +362,9 @@ def set_ignored_paths(
         Returns:
             The new hashing configuration with a new set of ignored paths.
         """
-        # Use relpath to possibly fix weird paths like '../a/b' -> 'b'
-        # when '../a/' is a no-op
-        self._ignored_paths = frozenset(
-            {pathlib.Path(p).resolve() for p in paths}
-        )
+        # Preserve the user-provided relative paths; they are resolved against
+        # the model directory later when hashing.
+        self._ignored_paths = frozenset(pathlib.Path(p) for p in paths)
         self._ignore_git_paths = ignore_git_paths
         return self
 
@@ -376,7 +379,15 @@ def add_ignored_paths(
                    the model directory.
         """
         newset = set(self._ignored_paths)
-        newset.update([os.path.join(model_path, p) for p in paths])
+        model_path = pathlib.Path(model_path)
+        for p in paths:
+            candidate = pathlib.Path(p)
+            full = model_path / candidate
+            try:
+                full.relative_to(model_path)
+            except ValueError:
+                continue
+            newset.add(candidate)
         self._ignored_paths = newset
 
     def set_allow_symlinks(self, allow_symlinks: bool) -> Self:

tests/hashing_config_test.py

@@ -0,0 +1,18 @@
+from model_signing import hashing
+
+
+def test_set_ignored_paths_relative_to_model(tmp_path, monkeypatch):
+    model = tmp_path / "model"
+    model.mkdir()
+    (model / "ignored.txt").write_text("skip")
+    (model / "keep.txt").write_text("keep")
+
+    cfg = hashing.Config().set_ignored_paths(paths=["ignored.txt"])
+    # Change working directory to ensure ignored paths aren't resolved against
+    # the current directory used at hashing time.
+    monkeypatch.chdir(tmp_path)
+
+    manifest = cfg.hash(model)
+    identifiers = {rd.identifier for rd in manifest.resource_descriptors()}
+    assert "ignored.txt" not in identifiers
+    assert "keep.txt" in identifiers