Compare commits

..

4 Commits

Author SHA1 Message Date
didericis-claude cc0c952d0b fix(security): harden git_gate.py shell rendering with shlex.quote and name validation
test / unit (pull_request) Successful in 35s
test / integration (pull_request) Successful in 44s
test / unit (push) Successful in 32s
test / integration (push) Successful in 41s
Use shlex.quote() on name and upstream_url in git_gate_render_entrypoint()
so special characters (single quotes, spaces, semicolons) cannot break or
inject into the generated sh script.

Add _GIT_NAME_RE validation in GitEntry.from_repos_entry() to restrict
repo names to [A-Za-z0-9._-]+, making the manifest the first line of
defence and shlex.quote() the belt-and-suspenders backstop.

Closes #155
2026-06-03 04:40:21 +00:00
didericis-claude 8c9d4fbc46 refactor: address PR review feedback — de-privatize helpers and rename modules
test / unit (pull_request) Successful in 34s
test / integration (pull_request) Successful in 43s
test / unit (push) Successful in 34s
test / integration (push) Successful in 43s
- Rename _manifest_util.py → manifest_util.py (module isn't private)
- Rename _as_json_object → as_json_object, _parse_git_upstream → parse_git_upstream,
  _parse_git_gate_config → parse_git_gate_config,
  _validate_unique_git_names → validate_unique_git_names,
  _validate_egress_routes → validate_egress_routes (none are private at
  module boundary — underscore prefix was a carry-over from the old
  monolithic manifest.py where everything lived in one namespace)
- Move _is_ip_literal → util.is_ip_literal (generic, belongs in the
  top-level util module)
- Update all import sites across manifest_*.py, manifest_extends.py,
  manifest_schema.py; existing callers of manifest.py are unaffected

All 867 unit tests pass.
2026-06-03 00:33:02 -04:00
didericis-claude b9ab1263c2 refactor: split manifest.py into domain-specific modules
Closes #157. Distributes the 1,026-line manifest.py across four
focused modules:

- _manifest_util.py: ManifestError + _as_json_object (shared base)
- manifest_git.py: GitEntry, GitUser, git-gate config helpers
- manifest_egress.py: EgressRoute, EgressConfig, PipelockRoutePolicy
- manifest_agent.py: AgentProvider, Agent

manifest.py is now the residual orchestration layer: Bottle, Manifest,
and re-exports of all public names so existing callers are unaffected.
All 867 unit tests pass.
2026-06-03 00:33:02 -04:00
didericis-claude 9282bceaf8 fix: emit WARNING when Docker teardown ExitStack raises (issue #156)
test / unit (pull_request) Successful in 37s
test / integration (pull_request) Successful in 40s
test / unit (push) Successful in 32s
test / integration (push) Successful in 43s
Replace the bare `except BaseException: pass` in the `teardown` closure
with a `warn()` call that includes the container name and operation type
("compose-down"), so cleanup failures are visible in the log rather than
silently discarded.  Non-blocking: the exception is consumed and teardown
continues, preserving the original error-propagation contract.

Add test_docker_launch_teardown.py to lock the new behaviour: it injects
a RuntimeError via a mocked `compose_down` callback and asserts the
WARNING message contains the container name and operation label.
2026-06-03 04:13:53 +00:00
6 changed files with 255 additions and 16 deletions
+6 -5
View File
@@ -43,7 +43,7 @@ from pathlib import Path
from typing import Callable, Generator from typing import Callable, Generator
from ...egress import egress_resolve_token_values from ...egress import egress_resolve_token_values
from ...log import info from ...log import info, warn
from . import network as network_mod from . import network as network_mod
from . import util as docker_mod from . import util as docker_mod
from .bottle import DockerBottle from .bottle import DockerBottle
@@ -87,10 +87,11 @@ def launch(
def teardown() -> None: def teardown() -> None:
try: try:
stack.close() stack.close()
except BaseException: except BaseException as exc:
# Teardown must not raise; swallow so the caller's warn(
# __exit__ path can still propagate the original error. f"teardown failed for container {plan.container_name}"
pass f" (compose-down): {exc!r}"
)
try: try:
# Step 1: agent image build. Sidecar images get built lazily by # Step 1: agent image build. Sidecar images get built lazily by
+2 -4
View File
@@ -29,6 +29,7 @@ backend-specific and lives on concrete subclasses (see
from __future__ import annotations from __future__ import annotations
import shlex
from abc import ABC, abstractmethod from abc import ABC, abstractmethod
from dataclasses import dataclass from dataclasses import dataclass
from pathlib import Path from pathlib import Path
@@ -207,10 +208,7 @@ def git_gate_render_entrypoint(upstreams: tuple[GitGateUpstream, ...]) -> str:
"mkdir -p /git", "mkdir -p /git",
] ]
for u in upstreams: for u in upstreams:
# Single-quote args so URL/path content (containing : and /) lines.append(f"init_repo {shlex.quote(u.name)} {shlex.quote(u.upstream_url)}")
# 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.extend([ lines.extend([
"", "",
"exec git daemon \\", "exec git daemon \\",
+11
View File
@@ -2,10 +2,16 @@
from __future__ import annotations from __future__ import annotations
import re
from dataclasses import dataclass from dataclasses import dataclass
from .manifest_util import ManifestError, as_json_object 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: def _opt_str(value: object, label: str) -> str:
if value is None: if value is None:
@@ -94,6 +100,11 @@ class GitEntry:
raise ManifestError( raise ManifestError(
f"bottle '{bottle_name}' git-gate.repos has an empty key" 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}]" label = f"git-gate.repos[{repo_name!r}]"
d = as_json_object(raw, f"bottle '{bottle_name}' {label}") d = as_json_object(raw, f"bottle '{bottle_name}' {label}")
for k in d: for k in d:
+145
View File
@@ -0,0 +1,145 @@
"""Unit: Docker launch teardown warning on ExitStack failure (issue #156).
When a callback registered in the ExitStack raises during teardown,
the teardown function must emit a WARNING-level message that includes
the container name and operation type, rather than silently discarding
the exception.
"""
from __future__ import annotations
import contextlib
import io
import tempfile
import unittest
from pathlib import Path
from unittest import mock
from bot_bottle.agent_provider import AgentProvisionPlan
from bot_bottle.backend import BottleSpec
from bot_bottle.backend.docker import launch as launch_mod
from bot_bottle.backend.docker.bottle_plan import DockerBottlePlan
from bot_bottle.egress import EgressPlan
from bot_bottle.git_gate import GitGatePlan
from bot_bottle.manifest import Manifest
from bot_bottle.pipelock import PipelockProxyPlan
from bot_bottle.workspace import workspace_plan
def _manifest() -> Manifest:
return Manifest.from_json_obj({
"bottles": {"dev": {}},
"agents": {"demo": {"skills": [], "prompt": "", "bottle": "dev"}},
})
def _plan(tmp: str) -> DockerBottlePlan:
stage = Path(tmp)
manifest = _manifest()
spec = BottleSpec(
manifest=manifest,
agent_name="demo",
copy_cwd=False,
user_cwd=tmp,
identity="test-teardown-00001",
)
return DockerBottlePlan(
spec=spec,
stage_dir=stage,
git_gate_plan=GitGatePlan(
slug="test-teardown-00001",
entrypoint_script=stage / "entrypoint.sh",
hook_script=stage / "hook.sh",
access_hook_script=stage / "access-hook.sh",
upstreams=(),
),
egress_plan=EgressPlan(
slug="test-teardown-00001",
routes_path=stage / "egress.yaml",
routes=(),
token_env_map={},
),
supervise_plan=None,
agent_provision=AgentProvisionPlan(
template="claude",
command="claude",
prompt_mode="append_file",
image="",
dockerfile="",
guest_env={},
),
workspace_plan=workspace_plan(spec, guest_home="/home/node"),
slug="test-teardown-00001",
container_name="bot-bottle-test-teardown-abc",
container_name_pinned=False,
image="bot-bottle-claude:latest",
derived_image="",
runtime_image="bot-bottle-claude:latest",
dockerfile_path="",
env_file=stage / "env",
forwarded_env={},
prompt_file=stage / "prompt.txt",
proxy_plan=PipelockProxyPlan(
yaml_path=stage / "pipelock.yaml",
slug="test-teardown-00001",
),
use_runsc=False,
)
class TestTeardownWarning(unittest.TestCase):
def setUp(self) -> None:
self._tmp = tempfile.mkdtemp(prefix="docker-launch-teardown-test.")
def tearDown(self) -> None:
import shutil
shutil.rmtree(self._tmp, ignore_errors=True)
def test_teardown_failure_emits_warning_with_container_and_operation(self):
plan = _plan(self._tmp)
buf = io.StringIO()
with mock.patch.object(launch_mod.docker_mod, "build_image"), \
mock.patch.object(
launch_mod, "pipelock_tls_init",
return_value=(Path("/ca.crt"), Path("/ca.key")),
), \
mock.patch.object(
launch_mod, "egress_tls_init",
return_value=(Path("/egress_ca"), Path("/egress_cert")),
), \
mock.patch.object(
launch_mod.network_mod, "network_name_for_slug",
return_value="bb-internal-test",
), \
mock.patch.object(
launch_mod.network_mod, "network_egress_name_for_slug",
return_value="bb-egress-test",
), \
mock.patch.object(
launch_mod, "bottle_plan_to_compose",
return_value={"services": {"agent": {}}},
), \
mock.patch.object(
launch_mod, "write_compose_file",
return_value=Path("/tmp/compose.yml"),
), \
mock.patch.object(launch_mod, "compose_up"), \
mock.patch.object(launch_mod, "compose_dump_logs"), \
mock.patch.object(
launch_mod, "compose_down",
side_effect=RuntimeError("network remove failed"),
), \
contextlib.redirect_stderr(buf):
provision = mock.Mock(return_value=None)
with launch_mod.launch(plan, provision=provision):
pass
output = buf.getvalue()
self.assertIn("bot-bottle: warning:", output)
self.assertIn("bot-bottle-test-teardown-abc", output)
self.assertIn("compose-down", output)
if __name__ == "__main__":
unittest.main()
+46 -7
View File
@@ -76,14 +76,18 @@ class TestEntrypointRender(unittest.TestCase):
) )
script = git_gate_render_entrypoint(ups) script = git_gate_render_entrypoint(ups)
self.assertIn("#!/bin/sh", script) self.assertIn("#!/bin/sh", script)
self.assertIn( # shlex.quote leaves safe strings unquoted; verify via token parse.
"init_repo 'bot-bottle' " import shlex as _shlex
"'ssh://git@gitea.dideric.is:30009/didericis/bot-bottle.git'", lines_with_init = [l for l in script.splitlines() if l.startswith("init_repo ")]
script, 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( self.assertEqual(
"init_repo 'foo' 'ssh://git@github.com/didericis/foo.git'", ["init_repo", "foo", "ssh://git@github.com/didericis/foo.git"],
script, _shlex.split(lines_with_init[1]),
) )
# Daemon line is what keeps PID 1 alive. # Daemon line is what keeps PID 1 alive.
self.assertIn("exec git daemon", script) self.assertIn("exec git daemon", script)
@@ -108,6 +112,41 @@ class TestEntrypointRender(unittest.TestCase):
self.assertNotIn("init_repo '", script) self.assertNotIn("init_repo '", script)
self.assertIn("exec git daemon", 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): class TestHookRender(unittest.TestCase):
def test_pre_receive_hook_has_two_phases(self): def test_pre_receive_hook_has_two_phases(self):
+45
View File
@@ -185,6 +185,51 @@ class TestGitEntryCrossValidation(unittest.TestCase):
"agents": {"demo": {"skills": [], "prompt": "", "bottle": "dev"}}, "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): def test_legacy_git_key_dies_with_hint(self):
msg = "" msg = ""
try: try: