fix(manifest): lexical pre-check for record_existing + add error-case tests

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).
This commit is contained in:
eldar702
2026-05-15 19:47:51 +03:00
parent ba9b252722
commit 5a6e25bbaf
2 changed files with 60 additions and 0 deletions

View File

@@ -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).

View File

@@ -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)