From 46d3d0520d8877b1979e6385bada9d9e1e0731ec Mon Sep 17 00:00:00 2001 From: Facundo Tuesca Date: Thu, 21 Sep 2023 12:00:59 +0200 Subject: [PATCH] Add more checks for the validity of refnames This change adds checks based on the rules described in [0] in order to more robustly check a refname's validity. [0]: https://git-scm.com/docs/git-check-ref-format --- git/refs/symbolic.py | 50 ++++++++++++++++++++++++++++++++++++++++++-- test/test_refs.py | 36 +++++++++++++++++++++++++++++++ 2 files changed, 84 insertions(+), 2 deletions(-) diff --git a/git/refs/symbolic.py b/git/refs/symbolic.py index 5c293aa7b..819615103 100644 --- a/git/refs/symbolic.py +++ b/git/refs/symbolic.py @@ -161,6 +161,51 @@ def dereference_recursive(cls, repo: "Repo", ref_path: Union[PathLike, None]) -> return hexsha # END recursive dereferencing + @staticmethod + def _check_ref_name_valid(ref_path: PathLike) -> None: + # Based on the rules described in https://git-scm.com/docs/git-check-ref-format/#_description + previous: Union[str, None] = None + one_before_previous: Union[str, None] = None + for c in str(ref_path): + if c in " ~^:?*[\\": + raise ValueError( + f"Invalid reference '{ref_path}': references cannot contain spaces, tildes (~), carets (^)," + f" colons (:), question marks (?), asterisks (*), open brackets ([) or backslashes (\\)" + ) + elif c == ".": + if previous is None or previous == "/": + raise ValueError( + f"Invalid reference '{ref_path}': references cannot start with a period (.) or contain '/.'" + ) + elif previous == ".": + raise ValueError(f"Invalid reference '{ref_path}': references cannot contain '..'") + elif c == "/": + if previous == "/": + raise ValueError(f"Invalid reference '{ref_path}': references cannot contain '//'") + elif previous is None: + raise ValueError( + f"Invalid reference '{ref_path}': references cannot start with forward slashes '/'" + ) + elif c == "{" and previous == "@": + raise ValueError(f"Invalid reference '{ref_path}': references cannot contain '@{{'") + elif ord(c) < 32 or ord(c) == 127: + raise ValueError(f"Invalid reference '{ref_path}': references cannot contain ASCII control characters") + + one_before_previous = previous + previous = c + + if previous == ".": + raise ValueError(f"Invalid reference '{ref_path}': references cannot end with a period (.)") + elif previous == "/": + raise ValueError(f"Invalid reference '{ref_path}': references cannot end with a forward slash (/)") + elif previous == "@" and one_before_previous is None: + raise ValueError(f"Invalid reference '{ref_path}': references cannot be '@'") + elif any([component.endswith(".lock") for component in str(ref_path).split("/")]): + raise ValueError( + f"Invalid reference '{ref_path}': references cannot have slash-separated components that end with" + f" '.lock'" + ) + @classmethod def _get_ref_info_helper( cls, repo: "Repo", ref_path: Union[PathLike, None] @@ -168,8 +213,9 @@ def _get_ref_info_helper( """Return: (str(sha), str(target_ref_path)) if available, the sha the file at rela_path points to, or None. target_ref_path is the reference we point to, or None""" - if ".." in str(ref_path): - raise ValueError(f"Invalid reference '{ref_path}'") + if ref_path: + cls._check_ref_name_valid(ref_path) + tokens: Union[None, List[str], Tuple[str, str]] = None repodir = _git_dir(repo, ref_path) try: diff --git a/test/test_refs.py b/test/test_refs.py index afd273df9..80166f651 100644 --- a/test/test_refs.py +++ b/test/test_refs.py @@ -631,3 +631,39 @@ def test_refs_outside_repo(self): ref_file.flush() ref_file_name = Path(ref_file.name).name self.assertRaises(BadName, self.rorepo.commit, f"../../{ref_file_name}") + + def test_validity_ref_names(self): + check_ref = SymbolicReference._check_ref_name_valid + # Based on the rules specified in https://git-scm.com/docs/git-check-ref-format/#_description + # Rule 1 + self.assertRaises(ValueError, check_ref, ".ref/begins/with/dot") + self.assertRaises(ValueError, check_ref, "ref/component/.begins/with/dot") + self.assertRaises(ValueError, check_ref, "ref/ends/with/a.lock") + self.assertRaises(ValueError, check_ref, "ref/component/ends.lock/with/period_lock") + # Rule 2 + check_ref("valid_one_level_refname") + # Rule 3 + self.assertRaises(ValueError, check_ref, "ref/contains/../double/period") + # Rule 4 + for c in " ~^:": + self.assertRaises(ValueError, check_ref, f"ref/contains/invalid{c}/character") + for code in range(0, 32): + self.assertRaises(ValueError, check_ref, f"ref/contains/invalid{chr(code)}/ASCII/control_character") + self.assertRaises(ValueError, check_ref, f"ref/contains/invalid{chr(127)}/ASCII/control_character") + # Rule 5 + for c in "*?[": + self.assertRaises(ValueError, check_ref, f"ref/contains/invalid{c}/character") + # Rule 6 + self.assertRaises(ValueError, check_ref, "/ref/begins/with/slash") + self.assertRaises(ValueError, check_ref, "ref/ends/with/slash/") + self.assertRaises(ValueError, check_ref, "ref/contains//double/slash/") + # Rule 7 + self.assertRaises(ValueError, check_ref, "ref/ends/with/dot.") + # Rule 8 + self.assertRaises(ValueError, check_ref, "ref/contains@{/at_brace") + # Rule 9 + self.assertRaises(ValueError, check_ref, "@") + # Rule 10 + self.assertRaises(ValueError, check_ref, "ref/contain\\s/backslash") + # Valid reference name should not raise + check_ref("valid/ref/name")