Validate VCS urls in hash-checking mode using their commit hashes

[sbidoul]

closes #6469

[sbidoul]

Note to self: when req.is_wheel_from_cache is True, link is the cached wheel and not the link to the artifact that was downloaded to build the wheel. So this function may not necessarily do what one expects when reasoning on req.link.

[uranusjr]

I wonder if this would break anything. How can this branch be reached?

[sbidoul]

This would raise in require-hashes mode only, if you have a dependency as a git direct URL that is obtained from cache.

I think these should not be considered hash-valid unless the commit hash has been provided by the user, either as a requirement file or a constraint.

This PR still needs work. Next step is adding a lot of test coverage. Then possibly some refactoring.

[uranusjr]

If an immutable VCS URL is both user-supplied and a dependency, would this cause the latter specifier break installation?

[sbidoul]

It should not, because when the dependency is examined by the resolver it is "merged" with the user-supplied one? That's definitely a test case to consider.

[uranusjr]

Suggested change

	pass

[sbidoul]

This is still on my list but unfortunately I won't have time to finish it in time for 23.2.

[pfmoore]

I removed #6469 from the 23.2 milestone for consistency.

[bcail]

This PR seemed to work for me, for installing a package from a git repo.

[adamzap]

Is there anything we can do to move this forward? It seems to work. Thanks!

[sbidoul]

Tests to do

• test git url + --hash errors out (with and without --require-hashes, with and without cache)
• test git url + not commit hash errors out with --require-hashes
• test git url + not commit hash don't error out without --require-hashes
• test that an indirect dependency specified as a git url fails in hash-checking mode

[bcail]

@sbidoul Here's a test for the second and third points you mentioned. Is that helpful? Should I work on expanding it more?

diff --git a/tests/unit/test_req.py b/tests/unit/test_req.py
index d70fa9da5..22cc7256c 100644
--- a/tests/unit/test_req.py
+++ b/tests/unit/test_req.py
@@ -241,6 +241,35 @@ class TestRequirementSet:
             ):
                 resolver.resolve(reqset.all_requirements, True)
 
+    def test_vcs_url_without_commit_hash(self, data: TestData) -> None:
+        """VCS URLs should raise errors when --require-hashes is on, if
+        they don't have a commit hash.
+        """
+        reqset = RequirementSet()
+        reqset.add_unnamed_requirement(
+            get_processed_req_from_line(
+                "git+https://github.com/pypa/pip-test-package",
+                lineno=1,
+            )
+        )
+
+        finder = make_test_finder(find_links=[data.find_links])
+
+        with self._basic_resolver(finder, require_hashes=True) as resolver:
+            with pytest.raises(
+                HashErrors,
+                match=(
+                    r"Can't verify hashes for these VCS requirements because their ref is "
+                    r"not immutable:\n"
+                    r"    git\+https://github\.com/pypa/pip-test-package \(from -r "
+                    r"file \(line 1\)\)"
+                ),
+            ):
+                resolver.resolve(reqset.all_requirements, True)
+
+        with self._basic_resolver(finder, require_hashes=False) as resolver:
+            resolver.resolve(reqset.all_requirements, True)
+

[FrancoisPerez]

Any way we can help this PR move forward @sbidoul ?