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 2c8e26f46a7b0845b54a59102dce8cf9ffc1227e Author: Dave Brondsema <[email protected]> AuthorDate: Sat May 30 12:27:30 2026 -0400 [#8607] harden git operations --- Allura/allura/controllers/repository.py | 10 +++- Allura/allura/model/__init__.py | 2 +- Allura/allura/model/repository.py | 17 ++++++ ForgeGit/forgegit/model/git_repo.py | 13 +++-- .../forgegit/tests/functional/test_controllers.py | 22 +++++++ ForgeGit/forgegit/tests/model/test_repository.py | 68 +++++++++++++++++++++- 6 files changed, 124 insertions(+), 8 deletions(-) diff --git a/Allura/allura/controllers/repository.py b/Allura/allura/controllers/repository.py index 3cd059f91..6be33c3fb 100644 --- a/Allura/allura/controllers/repository.py +++ b/Allura/allura/controllers/repository.py @@ -253,7 +253,10 @@ def commit_browser_data(self, start=None, limit=None, **kw): log.debug('Got %s heads', len(head_ids)) # recent commits from any head - heads_log = list(c.app.repo.log(head_ids, id_only=True, limit=int(limit))) + try: + heads_log = list(c.app.repo.log(head_ids, id_only=True, limit=int(limit))) + except ValueError: + raise exc.HTTPNotFound log.debug('Did log lookup') commit_ids = [c.app.repo.rev_to_commit_id(r) for r in heads_log] @@ -341,7 +344,10 @@ def commits(self, rev=None, limit=25, **kw): Return 120 latest commits : /rest/p/code/logs/?limit=120 ''' - revisions = c.app.repo.log(rev, id_only=False, limit=int(limit)) + try: + revisions = c.app.repo.log(rev, id_only=False, limit=int(limit)) + except ValueError: + raise exc.HTTPNotFound return { 'commits': [ diff --git a/Allura/allura/model/__init__.py b/Allura/allura/model/__init__.py index abbf624a4..b7814ab6b 100644 --- a/Allura/allura/model/__init__.py +++ b/Allura/allura/model/__init__.py @@ -28,7 +28,7 @@ from .auth import AuditLog, AlluraUserProperty, UserLoginDetails from .filesystem import File from .notification import Notification, Mailbox, SiteNotification -from .repository import Repository, RepositoryImplementation, CommitStatus +from .repository import Repository, RepositoryImplementation, CommitStatus, validate_scm_refs from .repository import MergeRequest, GitLikeTree from .stats import Stats from .oauth import OAuthToken, OAuthConsumerToken, OAuthRequestToken, OAuthAccessToken, OAuth2ClientApp, OAuth2AuthorizationCode, OAuth2AccessToken diff --git a/Allura/allura/model/repository.py b/Allura/allura/model/repository.py index d61828755..9f321c841 100644 --- a/Allura/allura/model/repository.py +++ b/Allura/allura/model/repository.py @@ -349,6 +349,19 @@ def merge_request_commits(self, mr): raise NotImplementedError('merge_request_commits') +def validate_scm_refs(values: Iterable[str]): + # Reject any value the scm layer would parse as a CLI option (argument injection, + # e.g. `git log --output=...`); no legal revision/ref begins with a dash + + if isinstance(values, str): + # avoid accidentally iterating over chars in a str + raise ValueError(f'Expected an iterable of revisions or refs, got a string: {values!r}') + + for value in values: + if not isinstance(value, (str, int)) or str(value).lstrip().startswith('-'): + raise ValueError(f'Invalid revision or ref: {value!r}') + + class Repository(Artifact, ActivityObject): BATCH_SIZE = 100 @@ -585,6 +598,10 @@ def log(self, revs=None, path=None, exclude=None, id_only=True, limit=None, **kw revs = [revs] if exclude is not None and not isinstance(exclude, (list, tuple)): exclude = [exclude] + if revs: + validate_scm_refs(revs) + if exclude: + validate_scm_refs(exclude) log_iter = self._impl.log(revs, path, exclude=exclude, id_only=id_only, limit=limit, **kw) return islice(log_iter, limit) diff --git a/ForgeGit/forgegit/model/git_repo.py b/ForgeGit/forgegit/model/git_repo.py index 88446677c..3355bbee8 100644 --- a/ForgeGit/forgegit/model/git_repo.py +++ b/ForgeGit/forgegit/model/git_repo.py @@ -118,6 +118,7 @@ def can_merge(self, mr): """ Given merge request `mr` determine if it can be merged w/o conflicts. """ + M.validate_scm_refs([mr.source_branch, mr.target_branch, mr.downstream.commit_id]) g = self._impl._git.git # http://stackoverflow.com/a/6283843 # fetch source branch @@ -130,6 +131,7 @@ def can_merge(self, mr): return '+<<<<<<<' not in merge_tree def merge(self, mr): + M.validate_scm_refs([mr.source_branch, mr.target_branch, mr.downstream.commit_id]) g = self._impl._git.git # can't merge in bare repo, so need to clone tmp_path = tempfile.mkdtemp() @@ -373,7 +375,8 @@ def log(self, revs=None, path=None, exclude=None, id_only=True, limit=None, **kw path = path.strip('/') if path else None if exclude is not None: revs.extend(['^%s' % e for e in exclude]) - args = ['--follow', '--name-status', revs, '--', path or '.'] + # --end-of-options stops git parsing a dash-prefixed rev as an option (arg injection). + args = ['--follow', '--name-status', '--end-of-options', revs, '--', path or '.'] kwargs = {} if limit: kwargs['n'] = limit @@ -451,6 +454,7 @@ def _iter_commits_with_refs(self, *args, **kwargs): D\t<some path> # other cases etc """ + assert '--end-of-options' in args proc = self._git.git.log(*args, format='%H%x00%d', as_process=True, **kwargs) stream = proc.stdout @@ -652,7 +656,7 @@ def _get_last_commit(self, commit_id, paths): skip = 0 while commit_id and not files: output = self._git.git.log( - commit_id, '--', *[p for p in paths], + '--end-of-options', commit_id, '--', *[p for p in paths], pretty='format:%H', name_only=True, max_count=1, @@ -670,7 +674,7 @@ def _get_last_commit(self, commit_id, paths): def get_changes(self, commit_id): return self._git.git.log( - commit_id, + '--end-of-options', commit_id, name_only=True, pretty='format:%H', max_count=1).splitlines()[1:] @@ -690,7 +694,7 @@ def paged_diffs(self, commit_id, start=0, end=None, onlyChangedFiles=False): if asbool(tg.config.get('scm.commit.git.detect_copies', True)): cmd_args += ['-M', '-C'] - cmd_output = self._git.git.diff_tree(commit_id, *cmd_args).split('\x00')[:-1] # don't escape filenames and use \x00 as fields delimiter + cmd_output = self._git.git.diff_tree(*cmd_args, '--end-of-options', commit_id).split('\x00')[:-1] # don't escape filenames and use \x00 as fields delimiter ''' cmd_output will be like: [ @@ -755,6 +759,7 @@ def _shared_clone(self, from_path): shutil.rmtree(tmp_path, ignore_errors=True) def merge_base(self, mr): + M.validate_scm_refs([mr.target_branch, mr.downstream.commit_id]) g = self._git.git g.fetch(mr.app.repo.full_fs_path, mr.target_branch) return g.merge_base(mr.downstream.commit_id, 'FETCH_HEAD') diff --git a/ForgeGit/forgegit/tests/functional/test_controllers.py b/ForgeGit/forgegit/tests/functional/test_controllers.py index 4b83848aa..e6cfa1eee 100644 --- a/ForgeGit/forgegit/tests/functional/test_controllers.py +++ b/ForgeGit/forgegit/tests/functional/test_controllers.py @@ -161,6 +161,28 @@ def test_commit_browser_data(self): 'parents': ['6a45885ae7347f1cac5103b0050cc1be6a1496c8'], 'message': 'Add README', 'row': 2}) + def test_commit_browser_data_arg_injection(self, tmp_path): + # End-to-end: `git log --output=<path>` argument injection via the `start` + # rev is rejected and writes no attacker-controlled file. + target = tmp_path / 'pwned' + sha = '1e146e67985dcd71c74de79613719bef7bddca4a' + self.app.get('/src-git/commit_browser_data', + params={'start': f'--output={target},{sha}'}, + status=404) + assert not target.exists() + + def test_rest_commits_arg_injection(self, tmp_path): + sha = '1e146e67985dcd71c74de79613719bef7bddca4a' + # baseline: the endpoint is reachable and returns commits + ok = self.app.get('/rest/p/test/src-git/commits/', params={'rev': sha}) + assert ok.json['commits'] + # an option-like rev is rejected (404) and writes no file + target = tmp_path / 'pwned' + self.app.get('/rest/p/test/src-git/commits/', + params={'rev': f'--output={target}'}, + status=404) + assert not target.exists() + def test_commit_browser_basic_view(self): resp = self.app.get('/src-git/ci/1e146e67985dcd71c74de79613719bef7bddca4a/basic') resp.mustcontain('Rick') diff --git a/ForgeGit/forgegit/tests/model/test_repository.py b/ForgeGit/forgegit/tests/model/test_repository.py index 7d981d3fb..bb6ac5e40 100644 --- a/ForgeGit/forgegit/tests/model/test_repository.py +++ b/ForgeGit/forgegit/tests/model/test_repository.py @@ -22,6 +22,7 @@ import email.iterators import pytest import mock +import git from tg import tmpl_context as c, app_globals as g import tg @@ -313,6 +314,68 @@ def test_log_unicode(self): entries = list(self.repo.log(path='völundr', id_only=False)) assert entries == [] + def test_log_rejects_optionlike_revs(self): + # option-like revs are rejected before reaching git + sha = '1e146e67985dcd71c74de79613719bef7bddca4a' + with pytest.raises(ValueError): + self.repo.log('--output=/tmp/pwned') + with pytest.raises(ValueError): + self.repo.log(' -f-o-o') + with pytest.raises(ValueError): + self.repo.log([sha, '--upload-pack=x']) + with pytest.raises(ValueError): + self.repo.log('HEAD', exclude='--output=/tmp/pwned') + with pytest.raises(ValueError): + self.repo.log('HEAD', exclude=['--output=/tmp/pwned']) + + self.repo.log(['HEAD'], exclude=['HEAD']) # ok normal usage + + # non-str / nested values must not slip past the check (the scm layer str()s them) + class _OptionLike: + def __str__(self): + return '--output=/tmp/pwned' + with pytest.raises(ValueError): + self.repo.log(_OptionLike()) + with pytest.raises(ValueError): + self.repo.log([['--output=/tmp/pwned']]) + + def test_log_arg_injection_neutralized_in_git_impl(self, tmp_path): + # calling into the GitImplementation directly bypasses the model-layer check, + # but --end-of-options still stops git honoring an injected option + target = tmp_path / 'pwned' + list(self.repo._impl.log( + [f'--output={target}', '1e146e67985dcd71c74de79613719bef7bddca4a'], + id_only=True)) + assert not target.exists() + + def test_git_impl_sinks_neutralize_arg_injection(self, tmp_path): + # get_changes / paged_diffs / _get_last_commit feed commit_id into git argv; + # --end-of-options stops an option-like value from being honored as an option. + impl = self.repo._impl + for fname, call in ( + ('a', lambda t: impl.get_changes(f'--output={t}')), + ('b', lambda t: impl.paged_diffs(f'--output={t}')), + ('c', lambda t: impl._get_last_commit(f'--output={t}', ['README'])), + ): + target = tmp_path / fname + with pytest.raises(git.GitCommandError, match=r'(bad revision|must come before non-option arguments)'): + call(target) + assert not target.exists() + + def test_merge_methods_reject_option_like_refs(self): + # can_merge / merge / merge_base build git fetch/checkout/merge argv from the MR's + # branch and commit refs; option-like values are rejected before any git runs. + mr = mock.Mock() + mr.source_branch = 'master' + mr.target_branch = '--upload-pack=touch /tmp/pwned' + mr.downstream.commit_id = '1e146e67985dcd71c74de79613719bef7bddca4a' + with pytest.raises(ValueError, match=r"Invalid revision or ref: '--upload-pack"): + self.repo.can_merge(mr) + with pytest.raises(ValueError, match=r"Invalid revision or ref: '--upload-pack"): + self.repo.merge(mr) + with pytest.raises(ValueError, match=r"Invalid revision or ref: '--upload-pack"): + self.repo._impl.merge_base(mr) + def test_log_file(self): entries = list(self.repo.log(path='README', id_only=False)) assert entries == [ @@ -787,7 +850,10 @@ def test_merge_raise_exception(self, git, shutil, tempfile): self.repo._impl._git.git = mock.Mock() git.Repo.clone_from.side_effect = ConnectionError with pytest.raises(ConnectionError): - self.repo.merge(mock.Mock()) + self.repo.merge(mock.Mock(source_branch='test-src-branch', + target_branch='test-target-branch', + downstream=mock.Mock(commit_id='test-commit-id') + )) assert shutil.rmtree.called @mock.patch.dict('allura.lib.app_globals.config', {'scm.commit.git.detect_copies': 'false'})
