From 5a6e25bbafad5df91562738e4c6b14e83f5ee6b6 Mon Sep 17 00:00:00 2001 From: eldar702 Date: Fri, 15 May 2026 19:47:51 +0300 Subject: [PATCH] fix(manifest): lexical pre-check for record_existing + add error-case tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address Copilot review (2026-05-11, review id 4266902103): 1. `record_existing` was calling `(self.project_root / rel).is_symlink()` BEFORE validating containment. For absolute paths or paths containing `..`, this performed a filesystem stat outside the project root before `_validate_rel_path()` raised. Add a cheap lexical pre-check that delegates to `_validate_rel_path()` for the canonical error messages, so the symlink stat only ever runs on paths that are already lexically inside the project root. 2. Add focused unit tests in `tests/integrations/test_manifest.py` for the symlink and non-regular-file error paths, including: - symlink target rejection - dangling symlink rejection (caught by the symlink guard before the is_file check) - directory path rejection (is_file == False) - missing-path rejection (is_file == False) - absolute-path lexical pre-check The Copilot reviewer noted these guards had no focused coverage in `test_manifest.py`, only via the `test_integration_claude.py` regression test. 3. The third Copilot finding (repeated `dict(self._files)` copies via `manifest.files` in the skip branches) is already resolved on this branch by using `prior_hashes` — the function-scope snapshot taken at the top of `install_shared_infra` — for the membership check, instead of `manifest.files`. AI disclosure: drafted with assistance from Claude (Opus 4.7). --- src/specify_cli/integrations/manifest.py | 9 +++++ tests/integrations/test_manifest.py | 51 ++++++++++++++++++++++++ 2 files changed, 60 insertions(+) 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)