libfetchers/git-utils: Be more correct about validating refnames

Turns out there's a much better API for this that doesn't have the
footguns of the previous method.

isLegalRefName is somewhat of a misnomer, since it's mainly used to
validate user inputs that can be either references, branch names,
psedorefs or tags.

Author: xokdvium

src/libfetchers-tests/git-utils.cc

@@ -175,6 +175,12 @@ TEST_F(GitUtilsTest, peel_reference)
 
 TEST(GitUtils, isLegalRefName)
 {
+    ASSERT_TRUE(isLegalRefName("A/b"));
+    ASSERT_TRUE(isLegalRefName("AaA/b"));
+    ASSERT_TRUE(isLegalRefName("FOO/BAR/BAZ"));
+    ASSERT_TRUE(isLegalRefName("HEAD"));
+    ASSERT_TRUE(isLegalRefName("refs/tags/1.2.3"));
+    ASSERT_TRUE(isLegalRefName("refs/heads/master"));
     ASSERT_TRUE(isLegalRefName("foox"));
     ASSERT_TRUE(isLegalRefName("1337"));
     ASSERT_TRUE(isLegalRefName("foo.baz"));

src/libfetchers/git-utils.cc

@@ -12,6 +12,7 @@
 
 #include <git2/attr.h>
 #include <git2/blob.h>
+#include <git2/branch.h>
 #include <git2/commit.h>
 #include <git2/config.h>
 #include <git2/describe.h>
@@ -28,6 +29,7 @@
 #include <git2/submodule.h>
 #include <git2/sys/odb_backend.h>
 #include <git2/sys/mempack.h>
+#include <git2/tag.h>
 #include <git2/tree.h>
 
 #include <boost/unordered/unordered_flat_map.hpp>
@@ -1323,63 +1325,33 @@ GitRepo::WorkdirInfo GitRepo::getCachedWorkdirInfo(const std::filesystem::path &
     return workdirInfo;
 }
 
-/**
- * Checks that the git reference is valid and normalizes slash '/' sequences.
- *
- * Accepts shorthand references (one-level refnames are allowed).
- */
-bool isValidRefNameAllowNormalizations(const std::string & refName)
-{
-    /* Unfortunately libgit2 doesn't expose the limit in headers, but its internal
-       limit is also 1024. */
-    std::array<char, 1024> normalizedRefBuffer;
-
-    /* It would be nice to have a better API like git_reference_name_is_valid, but
-     * with GIT_REFERENCE_FORMAT_REFSPEC_SHORTHAND flag. libgit2 uses it internally
-     * but doesn't expose it in public headers [1].
-     * [1]:
-     * https://github.com/libgit2/libgit2/blob/9d5f1bacc23594c2ba324c8f0d41b88bf0e9ef04/src/libgit2/refs.c#L1362-L1365
-     */
-
-    auto res = git_reference_normalize_name(
-        normalizedRefBuffer.data(),
-        normalizedRefBuffer.size(),
-        refName.c_str(),
-        GIT_REFERENCE_FORMAT_ALLOW_ONELEVEL | GIT_REFERENCE_FORMAT_REFSPEC_SHORTHAND);
-
-    return res == 0;
-}
-
 bool isLegalRefName(const std::string & refName)
 {
     initLibGit2();
 
-    /* Since `git_reference_normalize_name` is the best API libgit2 has for verifying
-     * reference names with shorthands (see comment in normalizeRefName), we need to
-     * ensure that exceptions to the validity checks imposed by normalization [1] are checked
-     * explicitly.
-     * [1]: https://git-scm.com/docs/git-check-ref-format#Documentation/git-check-ref-format.txt---normalize
-     */
-
     /* Check for cases that don't get rejected by libgit2.
      * FIXME: libgit2 should reject this. */
     if (refName == "@")
         return false;
 
-    /* Leading slashes and consecutive slashes are stripped during normalizatiton. */
-    if (refName.starts_with('/') || refName.find("//") != refName.npos)
-        return false;
-
-    /* Refer to libgit2. */
-    if (!isValidRefNameAllowNormalizations(refName))
-        return false;
-
     /* libgit2 doesn't barf on DEL symbol.
      * FIXME: libgit2 should reject this. */
     if (refName.find('\177') != refName.npos)
         return false;
 
-    return true;
+    for (auto * func : {
+             git_reference_name_is_valid,
+             git_branch_name_is_valid,
+             git_tag_name_is_valid,
+         }) {
+        int valid = 0;
+        if (func(&valid, refName.c_str()))
+            throw Error("checking git reference '%s': %s", refName, git_error_last()->message);
+        if (valid)
+            return true;
+    }
+
+    return false;
 }
 
 } // namespace nix

src/libfetchers/include/nix/fetchers/git-utils.hh

@@ -158,9 +158,12 @@ struct Setter
 };
 
 /**
- * Checks that the git reference is valid and normalized.
+ * Checks that the string can be a valid git reference, branch or tag name.
+ * Accepts shorthand references (one-level refnames are allowed), pseudorefs
+ * like `HEAD`.
  *
- * Accepts shorthand references (one-level refnames are allowed).
+ * @note This is a coarse test to make sure that the refname is at least something
+ * that Git can make sense of.
  */
 bool isLegalRefName(const std::string & refName);
 

tests/functional/fetchGitRefs.sh

@@ -59,6 +59,9 @@ invalid_ref() {
 }
 
 
+valid_ref 'A/b'
+valid_ref 'AaA/b'
+valid_ref 'FOO/BAR/BAZ'
 valid_ref 'foox'
 valid_ref '1337'
 valid_ref 'foo.baz'