diff --git a/bot_bottle/git_gate.py b/bot_bottle/git_gate.py index 59b29d1..274d121 100644 --- a/bot_bottle/git_gate.py +++ b/bot_bottle/git_gate.py @@ -29,6 +29,7 @@ backend-specific and lives on concrete subclasses (see from __future__ import annotations +import shlex from abc import ABC, abstractmethod from dataclasses import dataclass from pathlib import Path @@ -207,10 +208,7 @@ def git_gate_render_entrypoint(upstreams: tuple[GitGateUpstream, ...]) -> str: "mkdir -p /git", ] for u in upstreams: - # Single-quote args so URL/path content (containing : and /) - # passes through ash unmangled. Names came through the manifest - # validator so they don't contain a single quote. - lines.append(f"init_repo '{u.name}' '{u.upstream_url}'") + lines.append(f"init_repo {shlex.quote(u.name)} {shlex.quote(u.upstream_url)}") lines.extend([ "", "exec git daemon \\", diff --git a/bot_bottle/manifest_git.py b/bot_bottle/manifest_git.py index 08ab734..cf24a83 100644 --- a/bot_bottle/manifest_git.py +++ b/bot_bottle/manifest_git.py @@ -2,10 +2,16 @@ from __future__ import annotations +import re from dataclasses import dataclass from .manifest_util import ManifestError, as_json_object +# Shell-safe characters for git-gate repo names. Names are embedded in +# the generated entrypoint shell script (shlex.quote is the primary +# defence; this regex is belt-and-suspenders and documents intent). +_GIT_NAME_RE = re.compile(r"^[A-Za-z0-9._-]+$") + def _opt_str(value: object, label: str) -> str: if value is None: @@ -94,6 +100,11 @@ class GitEntry: raise ManifestError( f"bottle '{bottle_name}' git-gate.repos has an empty key" ) + if not _GIT_NAME_RE.match(repo_name): + raise ManifestError( + f"bottle '{bottle_name}' git-gate.repos name {repo_name!r} is invalid; " + f"allowed characters: A-Z a-z 0-9 . _ -" + ) label = f"git-gate.repos[{repo_name!r}]" d = as_json_object(raw, f"bottle '{bottle_name}' {label}") for k in d: diff --git a/tests/unit/test_git_gate.py b/tests/unit/test_git_gate.py index c93462c..f3f2759 100644 --- a/tests/unit/test_git_gate.py +++ b/tests/unit/test_git_gate.py @@ -76,14 +76,18 @@ class TestEntrypointRender(unittest.TestCase): ) script = git_gate_render_entrypoint(ups) self.assertIn("#!/bin/sh", script) - self.assertIn( - "init_repo 'bot-bottle' " - "'ssh://git@gitea.dideric.is:30009/didericis/bot-bottle.git'", - script, + # shlex.quote leaves safe strings unquoted; verify via token parse. + import shlex as _shlex + lines_with_init = [l for l in script.splitlines() if l.startswith("init_repo ")] + self.assertEqual(2, len(lines_with_init)) + self.assertEqual( + ["init_repo", "bot-bottle", + "ssh://git@gitea.dideric.is:30009/didericis/bot-bottle.git"], + _shlex.split(lines_with_init[0]), ) - self.assertIn( - "init_repo 'foo' 'ssh://git@github.com/didericis/foo.git'", - script, + self.assertEqual( + ["init_repo", "foo", "ssh://git@github.com/didericis/foo.git"], + _shlex.split(lines_with_init[1]), ) # Daemon line is what keeps PID 1 alive. self.assertIn("exec git daemon", script) @@ -108,6 +112,41 @@ class TestEntrypointRender(unittest.TestCase): self.assertNotIn("init_repo '", script) self.assertIn("exec git daemon", script) + def test_single_quote_in_upstream_url_is_escaped(self): + ups = (GitGateUpstream( + name="myrepo", + upstream_url="ssh://git@host/path'with'quotes.git", + upstream_host="host", + upstream_port="22", + identity_file="/key", + known_host_key="", + ),) + script = git_gate_render_entrypoint(ups) + self.assertNotIn( + "init_repo 'myrepo' 'ssh://git@host/path'with'quotes.git'", + script, + ) + self.assertIn("init_repo", script) + self.assertIn("path", script) + + def test_space_and_semicolon_in_upstream_url_are_escaped(self): + import shlex as _shlex + raw_url = "ssh://git@host/path with spaces;evil.git" + ups = (GitGateUpstream( + name="myrepo", + upstream_url=raw_url, + upstream_host="host", + upstream_port="22", + identity_file="/key", + known_host_key="", + ),) + script = git_gate_render_entrypoint(ups) + line = next(l for l in script.splitlines() if l.startswith("init_repo ")) + tokens = _shlex.split(line) + self.assertEqual(3, len(tokens)) + self.assertEqual("myrepo", tokens[1]) + self.assertEqual(raw_url, tokens[2]) + class TestHookRender(unittest.TestCase): def test_pre_receive_hook_has_two_phases(self): diff --git a/tests/unit/test_manifest_git.py b/tests/unit/test_manifest_git.py index 5422497..8fcc22e 100644 --- a/tests/unit/test_manifest_git.py +++ b/tests/unit/test_manifest_git.py @@ -185,6 +185,51 @@ class TestGitEntryCrossValidation(unittest.TestCase): "agents": {"demo": {"skills": [], "prompt": "", "bottle": "dev"}}, }) + def test_name_with_single_quote_dies(self): + with self.assertRaises(ManifestError): + Manifest.from_json_obj(_manifest({ + "o'reilly": { + "url": "ssh://git@github.com/foo.git", + "identity": "/dev/null", + }, + })) + + def test_name_with_space_dies(self): + with self.assertRaises(ManifestError): + Manifest.from_json_obj(_manifest({ + "my repo": { + "url": "ssh://git@github.com/foo.git", + "identity": "/dev/null", + }, + })) + + def test_name_with_semicolon_dies(self): + with self.assertRaises(ManifestError): + Manifest.from_json_obj(_manifest({ + "foo;bar": { + "url": "ssh://git@github.com/foo.git", + "identity": "/dev/null", + }, + })) + + def test_name_with_dollar_dies(self): + with self.assertRaises(ManifestError): + Manifest.from_json_obj(_manifest({ + "foo$bar": { + "url": "ssh://git@github.com/foo.git", + "identity": "/dev/null", + }, + })) + + def test_valid_name_with_dots_and_hyphens_accepted(self): + m = Manifest.from_json_obj(_manifest({ + "my.repo-name_1": { + "url": "ssh://git@github.com/foo.git", + "identity": "/dev/null", + }, + })) + self.assertEqual("my.repo-name_1", m.bottles["dev"].git[0].Name) + def test_legacy_git_key_dies_with_hint(self): msg = "" try: