You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@allura.apache.org by gc...@apache.org on 2026/09/17 20:55:39 UTC
(allura) 02/03: [#8618] harden rev_parse() against git's relative-ref syntax
This is an automated email from the ASF dual-hosted git repository.
asf-gitbox-commits pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/allura.git
commit 17b40870a524a5738b52d0ccf69fe8fbe10d90eb
Author: Dave Brondsema <da...@brondsema.net>
AuthorDate: Wed Sep 9 14:08:50 2026 -0400
[#8618] harden rev_parse() against git's relative-ref syntax
Co-Authored-By: Claude Sonnet 5 <no...@anthropic.com>
---
ForgeGit/forgegit/model/git_repo.py | 40 +++++++++++++++++++-----
ForgeGit/forgegit/tests/model/test_repository.py | 35 ++++++++++++++++++++-
2 files changed, 67 insertions(+), 8 deletions(-)
diff --git a/ForgeGit/forgegit/model/git_repo.py b/ForgeGit/forgegit/model/git_repo.py
index ae24595a4..2cbadb070 100644
--- a/ForgeGit/forgegit/model/git_repo.py
+++ b/ForgeGit/forgegit/model/git_repo.py
@@ -18,6 +18,7 @@
import sys
import os
+import re
import shutil
import string
import logging
@@ -59,6 +60,13 @@
max_open_handles=128)
+# Characters gitrevisions(7) treats as syntax rather than literal ref/hash content
+# (~N, ^N, :path, a..b range, @{...}), plus the control/space/glob characters
+# git-check-ref-format(1) bans from ref names outright. Anything else -- e.g. the
+# punctuation @ $ ! # % & is legal in a git branch/tag name and must not be rejected here.
+_UNSAFE_REV_CHARS_RE = re.compile(r'[\x00-\x1f\x7f ~^:?*\[\\]')
+
+
class GitLibCmdWrapper:
def __init__(self, client):
@@ -270,7 +278,7 @@ def commit(self, rev):
if result is None:
# find the id by branch/tag name
try:
- impl = self._git.rev_parse(str(rev) + '^0')
+ impl = self.rev_parse(rev, require_commit=True)
result = cache.get(M.repository.Commit, dict(_id=impl.hexsha))
except Exception:
url = ''
@@ -279,7 +287,7 @@ def commit(self, rev):
url = ' at ' + request.url
except Exception:
pass
- log.info(f"couldn't run rev_parse({str(rev) + '^0'}){url}", exc_info=True)
+ log.info(f"couldn't run rev_parse({rev!r}){url}", exc_info=True)
if result:
result.set_context(self._repo)
return result
@@ -319,7 +327,7 @@ def refresh_commit_info(self, oid, seen, lazy=True):
ci_doc = CommitDoc.m.get(_id=oid)
if ci_doc and lazy:
return False
- ci = self._git.rev_parse(oid)
+ ci = self.rev_parse(oid)
args = dict(
tree_id=ci.tree.hexsha,
committed=Object(
@@ -560,8 +568,26 @@ def _object(self, oid):
binsha += bytes((int(e + o, 16),))
return git.Object.new_from_sha(self._git, binsha)
- def rev_parse(self, rev):
- return self._git.rev_parse(rev)
+ def rev_parse(self, rev, require_commit: bool = False):
+ '''Resolve a plain commit hash or branch/tag name to a git object.
+
+ Only accepts safe rev syntax -- not relative-ref forms like ~N, ^N, :path, a..b range, @{...}
+
+ :param require_commit: dereference a tag object down to the commit it points to (git's `<rev>^0` syntax).
+ '''
+ if not self._is_safe_rev(rev):
+ raise ValueError(f'Unsafe rev spec: {rev!r}')
+ resolved = f'{rev}^0' if require_commit else str(rev)
+ return self._git.rev_parse(resolved)
+
+ @staticmethod
+ def _is_safe_rev(rev) -> bool:
+ rev = str(rev)
+ if not rev or rev.startswith('-'):
+ return False # a leading dash risks being read as a git rev-parse option
+ if '..' in rev or '@{' in rev:
+ return False
+ return not _UNSAFE_REV_CHARS_RE.search(rev)
def symbolics_for_commit(self, commit):
try:
@@ -573,7 +599,7 @@ def symbolics_for_commit(self, commit):
return [], []
def compute_tree_new(self, commit, tree_path='/'):
- ci = self._git.rev_parse(commit._id)
+ ci = self.rev_parse(commit._id)
tree = self.refresh_tree_info(ci.tree, set())
return tree._id
@@ -602,7 +628,7 @@ def is_empty(self):
def is_file(self, path, rev=None):
path = path.strip('/')
- ci = self._git.rev_parse(rev)
+ ci = self.rev_parse(rev)
try:
node = ci.tree / path
return node.type == 'blob'
diff --git a/ForgeGit/forgegit/tests/model/test_repository.py b/ForgeGit/forgegit/tests/model/test_repository.py
index eed4c38cd..5bc0ff32c 100644
--- a/ForgeGit/forgegit/tests/model/test_repository.py
+++ b/ForgeGit/forgegit/tests/model/test_repository.py
@@ -599,7 +599,8 @@ def test_ls_with_prev(self):
c.lcid_cache = {} # else it'll be a mock
# build the previous last-commit-doc. This causes different behavior when generating the next one.
- self.repo.commit('HEAD^').tree.ls()
+ # 'HEAD^' would be more readable, but commit() no longer accepts relative-ref syntax
+ self.repo.commit('df30427c488aeab84b2352bdf88a3b19223f9d7a').tree.ls()
self.test_ls()
@@ -640,6 +641,38 @@ def test_blob_size(self):
assert self.repo._impl.blob_size(tree['README']) == 28
assert self.repo._impl.blob_size(tree['bad']) == 9
+ def test_rev_parse_rejects_unsafe_rev(self):
+ impl = self.repo._impl
+ for unsafe_rev in ('zz~1', 'zz^1', 'zz^0', 'HEAD@{0}', 'zz:README', 'zz..master', '-zz'):
+ with pytest.raises(ValueError):
+ impl.rev_parse(unsafe_rev)
+
+ def test_rev_parse_allows_punctuation_legal_in_ref_names(self):
+ impl = self.repo._impl
+ for safe_rev in ('feature@2024', 'release/$version', 'foo!bar', 'a+b', 'x,y', 'name#1', "it's-fine"):
+ assert impl._is_safe_rev(safe_rev)
+
+ def test_rev_parse_require_commit(self):
+ # require_commit appends git's `^0` dereference suffix internally --
+ # after the safety check, not as a caller-supplied suffix that could itself smuggle in unsafe syntax
+ impl = self.repo._impl
+ obj = impl.rev_parse('master', require_commit=True)
+ assert obj.hexsha == '1e146e67985dcd71c74de79613719bef7bddca4a'
+
+ def test_commit_rejects_relative_ref_syntax(self):
+ # commit()'s docstring says rev can be "_id or a branch/tag name" -- it must
+ # not also accept git's relative-ref forms (~N, ^N, :path, a..b, @{...}).
+ for unsafe_rev in ('zz~1', 'zz^1', 'zz^0', 'HEAD@{0}', 'zz:README', 'zz..master', '-zz'):
+ assert self.repo.commit(unsafe_rev) is None
+
+ def test_commit_resolves_safe_revs(self):
+ # plain hashes (short and long) and branch/tag names must still resolve
+ full_hexsha = '1e146e67985dcd71c74de79613719bef7bddca4a'
+ for safe_rev in (full_hexsha, full_hexsha[:7], 'master'):
+ result = self.repo.commit(safe_rev)
+ assert result is not None
+ assert result._id == full_hexsha
+
def test_tarball_status(self):
tmpdir = tg.config['scm.repos.tarball.root']
if os.path.isfile(os.path.join(tmpdir, "git/t/te/test/testgit.git/test-src-git-HEAD.zip")):