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 ...egress import egress_resolve_token_values
from ...log import info
from ...log import info, warn
from . import network as network_mod
from . import util as docker_mod
from .bottle import DockerBottle
@@ -87,10 +87,11 @@ def launch(
def teardown() -> None:
try:
stack.close()
except BaseException:
# Teardown must not raise; swallow so the caller's
# __exit__ path can still propagate the original error.
pass
except BaseException as exc:
warn(
f"teardown failed for container {plan.container_name}"
f" (compose-down): {exc!r}"
)
try:
# 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
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 \\",
+11
View File
@@ -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:
+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)
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):
+45
View File
@@ -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: