diff --git a/src/specify_cli/integrations/manifest.py b/src/specify_cli/integrations/manifest.py index c05b85a99..6a9eee668 100644 --- a/src/specify_cli/integrations/manifest.py +++ b/src/specify_cli/integrations/manifest.py @@ -157,6 +157,15 @@ class IntegrationManifest: treat the entry as permanently broken. """ rel = Path(rel_path) + # Cheap lexical pre-check first so absolute / parent-traversal paths + # don't trigger a filesystem stat outside the project root before + # ``_validate_rel_path`` raises. ``_validate_rel_path`` produces the + # canonical error messages used elsewhere. + if rel.is_absolute() or ".." in rel.parts: + _validate_rel_path(rel, self.project_root) + # Defensive: _validate_rel_path always raises on these inputs, + # but make the contract explicit if it is ever loosened. + raise ValueError(f"Manifest path escapes project root: {rel}") # Check ``is_symlink()`` on the un-resolved path because # ``_validate_rel_path`` resolves the path (which would follow # the symlink and silently record the target instead). diff --git a/tests/integrations/test_manifest.py b/tests/integrations/test_manifest.py index 596397d4f..a3fb14e3d 100644 --- a/tests/integrations/test_manifest.py +++ b/tests/integrations/test_manifest.py @@ -34,6 +34,57 @@ class TestManifestRecordFile: assert m.files["existing.txt"] == _sha256(f) +class TestManifestRecordExistingErrors: + """Error-case coverage for ``record_existing`` symlink + non-file guards. + + Added in #2483 — Copilot review flagged these as un-tested regressions + after the ``is_symlink``/``is_file`` guards were introduced. + """ + + def test_rejects_symlink_target(self, tmp_path): + target = tmp_path / "target.txt" + target.write_text("target content", encoding="utf-8") + link = tmp_path / "link.txt" + link.symlink_to(target) + m = IntegrationManifest("test", tmp_path) + with pytest.raises(ValueError, match="symlinked"): + m.record_existing("link.txt") + + def test_rejects_dangling_symlink(self, tmp_path): + # A symlink pointing nowhere should still be rejected before the + # ``is_file()`` check (which would itself be False on a dangler). + link = tmp_path / "dangler.txt" + link.symlink_to(tmp_path / "no-such-target.txt") + m = IntegrationManifest("test", tmp_path) + with pytest.raises(ValueError, match="symlinked"): + m.record_existing("dangler.txt") + + def test_rejects_directory_path(self, tmp_path): + (tmp_path / "a_dir").mkdir() + m = IntegrationManifest("test", tmp_path) + with pytest.raises(ValueError, match="not a regular file"): + m.record_existing("a_dir") + + def test_rejects_missing_path(self, tmp_path): + # ``is_file()`` is False for non-existent paths too; the same error + # surface keeps callers from having to distinguish "missing" from + # "wrong kind" — both mean "cannot hash this". + m = IntegrationManifest("test", tmp_path) + with pytest.raises(ValueError, match="not a regular file"): + m.record_existing("never-existed.txt") + + def test_lexical_prevalidation_for_absolute_path(self, tmp_path): + # ``record_existing`` must reject absolute paths via the lexical + # pre-check, NOT via the filesystem-touching ``is_symlink()`` call. + # Verified by passing an absolute path that points to a directory + # outside the project root — the canonical "Absolute paths" error + # must surface before any stat on the absolute path. + m = IntegrationManifest("test", tmp_path) + abs_path = "C:\\tmp\\escape.txt" if sys.platform == "win32" else "/tmp/escape.txt" + with pytest.raises(ValueError, match="Absolute paths"): + m.record_existing(abs_path) + + class TestManifestPathTraversal: def test_record_file_rejects_parent_traversal(self, tmp_path): m = IntegrationManifest("test", tmp_path)