From 508c537debd1e35044b2f499560413b50d29034a Mon Sep 17 00:00:00 2001 From: claude Date: Thu, 25 Jun 2026 06:59:09 +0000 Subject: [PATCH 1/3] fix: add explicit timeouts to subprocess and HTTP calls in git-gate paths Closes #255. Without timeouts, a hung upstream during the access-hook or git http-backend CGI call (git_http_backend.py) and a stalled Gitea API during deploy-key provisioning (contrib/gitea/deploy_key_provisioner.py) could wedge a sidecar indefinitely. Adds GIT_HTTP_BACKEND_TIMEOUT_SECS (30s) to both subprocess.run calls in the HTTP backend, mirroring the existing GIT_GATE_DAEMON_TIMEOUT_SECS on the daemon path. Adds _API_TIMEOUT_SECS (30s) and _KEYGEN_TIMEOUT_SECS (10s) to the Gitea provisioner's urlopen and ssh-keygen calls. Tests verify the timeout values are forwarded in all four call sites. --- .../contrib/gitea/deploy_key_provisioner.py | 10 ++- bot_bottle/git_http_backend.py | 8 +++ tests/unit/test_contrib_gitea_deploy_key.py | 31 ++++++++++ tests/unit/test_git_http_backend.py | 61 ++++++++++++++++++- 4 files changed, 107 insertions(+), 3 deletions(-) diff --git a/bot_bottle/contrib/gitea/deploy_key_provisioner.py b/bot_bottle/contrib/gitea/deploy_key_provisioner.py index 9a4a9dc..1545c4e 100644 --- a/bot_bottle/contrib/gitea/deploy_key_provisioner.py +++ b/bot_bottle/contrib/gitea/deploy_key_provisioner.py @@ -21,6 +21,11 @@ from pathlib import Path from ...deploy_key_provisioner import DeployKeyCollisionError, DeployKeyProvisioner +# Timeout for ssh-keygen and Gitea API HTTP calls. A hung Gitea instance at +# prepare time would stall bottle launch indefinitely without this bound. +_API_TIMEOUT_SECS = 30 +_KEYGEN_TIMEOUT_SECS = 10 + class GiteaDeployKeyProvisioner(DeployKeyProvisioner): """Manages deploy keys on a Gitea instance.""" @@ -46,6 +51,7 @@ class GiteaDeployKeyProvisioner(DeployKeyProvisioner): check=True, stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL, + timeout=_KEYGEN_TIMEOUT_SECS, ) private_key = key_path.read_bytes() public_key = key_path.with_suffix(".pub").read_text().strip() @@ -67,7 +73,7 @@ class GiteaDeployKeyProvisioner(DeployKeyProvisioner): method="POST", ) try: - with urllib.request.urlopen(req) as resp: + with urllib.request.urlopen(req, timeout=_API_TIMEOUT_SECS) as resp: body = json.loads(resp.read()) except urllib.error.HTTPError as exc: _body = _read_error_body(exc) @@ -98,7 +104,7 @@ class GiteaDeployKeyProvisioner(DeployKeyProvisioner): method="DELETE", ) try: - with urllib.request.urlopen(req): + with urllib.request.urlopen(req, timeout=_API_TIMEOUT_SECS): pass except urllib.error.HTTPError as exc: if exc.code == 404: diff --git a/bot_bottle/git_http_backend.py b/bot_bottle/git_http_backend.py index 9032d9e..973f384 100644 --- a/bot_bottle/git_http_backend.py +++ b/bot_bottle/git_http_backend.py @@ -22,6 +22,12 @@ DEFAULT_PORT = 9420 # Bound memory use while still allowing ordinary git push packfiles. MAX_BODY_BYTES = 100 * 1024 * 1024 +# Timeout for the access-hook subprocess and git http-backend CGI subprocess. +# Mirrors GIT_GATE_DAEMON_TIMEOUT_SECS so both HTTP and daemon paths share the +# same bound: a hung upstream fetch in the access-hook or a stalled CGI child +# cannot wedge the sidecar indefinitely. +GIT_HTTP_BACKEND_TIMEOUT_SECS = 30 + class GitHttpHandler(BaseHTTPRequestHandler): server_version = "bot-bottle-git-http/1" @@ -47,6 +53,7 @@ class GitHttpHandler(BaseHTTPRequestHandler): [hook_path, "upload-pack", str(repo_dir), peer, peer], capture_output=True, check=False, + timeout=GIT_HTTP_BACKEND_TIMEOUT_SECS, ) if hook.returncode != 0: detail = (hook.stderr or hook.stdout).decode( @@ -110,6 +117,7 @@ class GitHttpHandler(BaseHTTPRequestHandler): env=env, capture_output=True, check=False, + timeout=GIT_HTTP_BACKEND_TIMEOUT_SECS, ) self._write_cgi_response(proc.stdout) diff --git a/tests/unit/test_contrib_gitea_deploy_key.py b/tests/unit/test_contrib_gitea_deploy_key.py index 469f757..c3b206a 100644 --- a/tests/unit/test_contrib_gitea_deploy_key.py +++ b/tests/unit/test_contrib_gitea_deploy_key.py @@ -10,6 +10,8 @@ from unittest.mock import MagicMock, patch from bot_bottle.contrib.gitea.deploy_key_provisioner import ( GiteaDeployKeyProvisioner, + _API_TIMEOUT_SECS, + _KEYGEN_TIMEOUT_SECS, _split_owner_repo, ) from bot_bottle.deploy_key_provisioner import DeployKeyCollisionError @@ -83,6 +85,25 @@ class TestCreate(unittest.TestCase): self.assertEqual(str(fake_key_id), key_id) self.assertEqual(fake_private, private_bytes) + def test_create_passes_timeout_to_ssh_keygen_and_urlopen(self): + provisioner = _provisioner() + with patch( + "bot_bottle.contrib.gitea.deploy_key_provisioner.subprocess.run" + ) as mock_run, patch( + "bot_bottle.contrib.gitea.deploy_key_provisioner.urllib.request.urlopen" + ) as mock_urlopen, patch( + "bot_bottle.contrib.gitea.deploy_key_provisioner.Path.read_bytes", + return_value=b"PRIVATE", + ), patch( + "bot_bottle.contrib.gitea.deploy_key_provisioner.Path.read_text", + return_value="ssh-ed25519 AAAA\n", + ): + mock_urlopen.return_value = _urlopen_response({"id": 1}) + provisioner.create("owner/repo", "title") + + self.assertEqual(_KEYGEN_TIMEOUT_SECS, mock_run.call_args.kwargs.get("timeout")) + self.assertEqual(_API_TIMEOUT_SECS, mock_urlopen.call_args.kwargs.get("timeout")) + def test_create_raises_on_http_error(self): provisioner = _provisioner() with patch( @@ -139,6 +160,16 @@ class TestDelete(unittest.TestCase): self.assertIn("/api/v1/repos/didericis/bot-bottle/keys/99", req.full_url) self.assertEqual("DELETE", req.get_method()) + def test_delete_passes_timeout_to_urlopen(self): + provisioner = _provisioner() + with patch( + "bot_bottle.contrib.gitea.deploy_key_provisioner.urllib.request.urlopen" + ) as mock_urlopen: + mock_urlopen.return_value = _urlopen_response({}) + provisioner.delete("owner/repo", "7") + + self.assertEqual(_API_TIMEOUT_SECS, mock_urlopen.call_args.kwargs.get("timeout")) + def test_delete_tolerates_404(self): provisioner = _provisioner() with patch( diff --git a/tests/unit/test_git_http_backend.py b/tests/unit/test_git_http_backend.py index 0890b53..fac8ea5 100644 --- a/tests/unit/test_git_http_backend.py +++ b/tests/unit/test_git_http_backend.py @@ -9,7 +9,11 @@ import urllib.request from pathlib import Path from unittest import mock -from bot_bottle.git_http_backend import GitHttpHandler, MAX_BODY_BYTES +from bot_bottle.git_http_backend import ( + GIT_HTTP_BACKEND_TIMEOUT_SECS, + GitHttpHandler, + MAX_BODY_BYTES, +) class TestGitHttpBackend(unittest.TestCase): @@ -150,6 +154,61 @@ class TestGitHttpBackend(unittest.TestCase): ) self.assertEqual("git/test", env["HTTP_USER_AGENT"]) + def test_subprocess_calls_include_timeout(self): + """Both subprocess.run calls (access-hook and git http-backend) must + pass timeout= so a hung upstream cannot wedge the sidecar.""" + from http.server import ThreadingHTTPServer + + with tempfile.TemporaryDirectory() as tmp: + root = Path(tmp) + (root / "repo.git").mkdir() + + old_root = os.environ.get("GIT_PROJECT_ROOT") + os.environ["GIT_PROJECT_ROOT"] = str(root) + self.addCleanup(self._restore_env, old_root) + old_hook = os.environ.get("GIT_GATE_ACCESS_HOOK") + hook = root / "access-hook" + hook.write_text("#!/bin/sh\nexit 0\n") + hook.chmod(0o700) + os.environ["GIT_GATE_ACCESS_HOOK"] = str(hook) + self.addCleanup(self._restore_hook, old_hook) + + server = ThreadingHTTPServer(("127.0.0.1", 0), GitHttpHandler) + thread = threading.Thread(target=server.serve_forever, daemon=True) + thread.start() + self.addCleanup(server.shutdown) + self.addCleanup(server.server_close) + + backend_response = ( + b"Status: 200 OK\r\n" + b"Content-Type: application/x-git-upload-pack-result\r\n" + b"\r\n" + b"0000" + ) + calls = [ + subprocess.CompletedProcess(["hook"], 0, b"", b""), + subprocess.CompletedProcess(["git"], 0, backend_response, b""), + ] + with mock.patch( + "bot_bottle.git_http_backend.subprocess.run", + side_effect=calls, + ) as run: + req = urllib.request.Request( + f"http://127.0.0.1:{server.server_port}" + "/repo.git/git-upload-pack", + data=b"", + method="POST", + ) + with urllib.request.urlopen(req, timeout=5): + pass + + for call in run.call_args_list: + self.assertEqual( + GIT_HTTP_BACKEND_TIMEOUT_SECS, + call.kwargs.get("timeout"), + f"subprocess.run call missing timeout: {call}", + ) + def test_access_hook_denial_is_logged_to_stdout(self): """When the access-hook exits non-zero we still return 403 to the client, but the hook's stderr must also appear on the handler's -- 2.52.0 From c0d3f165199621186bed49b3f285a6027038f1fc Mon Sep 17 00:00:00 2001 From: claude Date: Thu, 25 Jun 2026 07:03:18 +0000 Subject: [PATCH 2/3] refactor: import GIT_GATE_DAEMON_TIMEOUT_SECS instead of duplicating the value --- bot_bottle/git_http_backend.py | 12 ++++-------- tests/unit/test_git_http_backend.py | 9 +++------ 2 files changed, 7 insertions(+), 14 deletions(-) diff --git a/bot_bottle/git_http_backend.py b/bot_bottle/git_http_backend.py index 973f384..add4b3b 100644 --- a/bot_bottle/git_http_backend.py +++ b/bot_bottle/git_http_backend.py @@ -16,18 +16,14 @@ from http.server import BaseHTTPRequestHandler, ThreadingHTTPServer from pathlib import Path from urllib.parse import urlsplit +from .git_gate import GIT_GATE_DAEMON_TIMEOUT_SECS + DEFAULT_PORT = 9420 # Bound memory use while still allowing ordinary git push packfiles. MAX_BODY_BYTES = 100 * 1024 * 1024 -# Timeout for the access-hook subprocess and git http-backend CGI subprocess. -# Mirrors GIT_GATE_DAEMON_TIMEOUT_SECS so both HTTP and daemon paths share the -# same bound: a hung upstream fetch in the access-hook or a stalled CGI child -# cannot wedge the sidecar indefinitely. -GIT_HTTP_BACKEND_TIMEOUT_SECS = 30 - class GitHttpHandler(BaseHTTPRequestHandler): server_version = "bot-bottle-git-http/1" @@ -53,7 +49,7 @@ class GitHttpHandler(BaseHTTPRequestHandler): [hook_path, "upload-pack", str(repo_dir), peer, peer], capture_output=True, check=False, - timeout=GIT_HTTP_BACKEND_TIMEOUT_SECS, + timeout=GIT_GATE_DAEMON_TIMEOUT_SECS, ) if hook.returncode != 0: detail = (hook.stderr or hook.stdout).decode( @@ -117,7 +113,7 @@ class GitHttpHandler(BaseHTTPRequestHandler): env=env, capture_output=True, check=False, - timeout=GIT_HTTP_BACKEND_TIMEOUT_SECS, + timeout=GIT_GATE_DAEMON_TIMEOUT_SECS, ) self._write_cgi_response(proc.stdout) diff --git a/tests/unit/test_git_http_backend.py b/tests/unit/test_git_http_backend.py index fac8ea5..ef70628 100644 --- a/tests/unit/test_git_http_backend.py +++ b/tests/unit/test_git_http_backend.py @@ -9,11 +9,8 @@ import urllib.request from pathlib import Path from unittest import mock -from bot_bottle.git_http_backend import ( - GIT_HTTP_BACKEND_TIMEOUT_SECS, - GitHttpHandler, - MAX_BODY_BYTES, -) +from bot_bottle.git_gate import GIT_GATE_DAEMON_TIMEOUT_SECS +from bot_bottle.git_http_backend import GitHttpHandler, MAX_BODY_BYTES class TestGitHttpBackend(unittest.TestCase): @@ -204,7 +201,7 @@ class TestGitHttpBackend(unittest.TestCase): for call in run.call_args_list: self.assertEqual( - GIT_HTTP_BACKEND_TIMEOUT_SECS, + GIT_GATE_DAEMON_TIMEOUT_SECS, call.kwargs.get("timeout"), f"subprocess.run call missing timeout: {call}", ) -- 2.52.0 From 0bace7615aaef802f83f10a017af0cb057c12612 Mon Sep 17 00:00:00 2001 From: claude Date: Thu, 25 Jun 2026 07:07:44 +0000 Subject: [PATCH 3/3] refactor: rename GIT_GATE_DAEMON_TIMEOUT_SECS to GIT_GATE_TIMEOUT_SECS The constant now covers the daemon path, the HTTP backend access-hook, and the git http-backend CGI subprocess, so 'daemon' in the name was too narrow. Updated the comment to list all three current uses. --- bot_bottle/git_gate.py | 12 ++++++------ bot_bottle/git_http_backend.py | 6 +++--- tests/unit/test_git_http_backend.py | 4 ++-- 3 files changed, 11 insertions(+), 11 deletions(-) diff --git a/bot_bottle/git_gate.py b/bot_bottle/git_gate.py index 239499c..acbf4d4 100644 --- a/bot_bottle/git_gate.py +++ b/bot_bottle/git_gate.py @@ -43,10 +43,10 @@ from .manifest import ManifestBottle, ManifestGitEntry # Short network alias for git-gate inside the sidecar bundle. The # agent's `.gitconfig` insteadOf rewrites resolve through this name. GIT_GATE_HOSTNAME = "git-gate" -# Bound half-open git client sessions. If an agent/tool runner is -# interrupted during push, git daemon should reap the receive-pack -# child instead of keeping the gate wedged indefinitely. -GIT_GATE_DAEMON_TIMEOUT_SECS = 15 +# Shared timeout (seconds) for all git-gate subprocess and CGI calls: +# git daemon (--timeout/--init-timeout), the access-hook subprocess in +# git_http_backend, and the git http-backend CGI subprocess. +GIT_GATE_TIMEOUT_SECS = 15 @dataclass(frozen=True) @@ -217,8 +217,8 @@ def git_gate_render_entrypoint(upstreams: tuple[GitGateUpstream, ...]) -> str: "", "exec git daemon \\", " --reuseaddr \\", - f" --timeout={GIT_GATE_DAEMON_TIMEOUT_SECS} \\", - f" --init-timeout={GIT_GATE_DAEMON_TIMEOUT_SECS} \\", + f" --timeout={GIT_GATE_TIMEOUT_SECS} \\", + f" --init-timeout={GIT_GATE_TIMEOUT_SECS} \\", " --base-path=/git \\", " --export-all \\", " --enable=receive-pack \\", diff --git a/bot_bottle/git_http_backend.py b/bot_bottle/git_http_backend.py index add4b3b..1b45a38 100644 --- a/bot_bottle/git_http_backend.py +++ b/bot_bottle/git_http_backend.py @@ -16,7 +16,7 @@ from http.server import BaseHTTPRequestHandler, ThreadingHTTPServer from pathlib import Path from urllib.parse import urlsplit -from .git_gate import GIT_GATE_DAEMON_TIMEOUT_SECS +from .git_gate import GIT_GATE_TIMEOUT_SECS DEFAULT_PORT = 9420 @@ -49,7 +49,7 @@ class GitHttpHandler(BaseHTTPRequestHandler): [hook_path, "upload-pack", str(repo_dir), peer, peer], capture_output=True, check=False, - timeout=GIT_GATE_DAEMON_TIMEOUT_SECS, + timeout=GIT_GATE_TIMEOUT_SECS, ) if hook.returncode != 0: detail = (hook.stderr or hook.stdout).decode( @@ -113,7 +113,7 @@ class GitHttpHandler(BaseHTTPRequestHandler): env=env, capture_output=True, check=False, - timeout=GIT_GATE_DAEMON_TIMEOUT_SECS, + timeout=GIT_GATE_TIMEOUT_SECS, ) self._write_cgi_response(proc.stdout) diff --git a/tests/unit/test_git_http_backend.py b/tests/unit/test_git_http_backend.py index ef70628..c605eb0 100644 --- a/tests/unit/test_git_http_backend.py +++ b/tests/unit/test_git_http_backend.py @@ -9,7 +9,7 @@ import urllib.request from pathlib import Path from unittest import mock -from bot_bottle.git_gate import GIT_GATE_DAEMON_TIMEOUT_SECS +from bot_bottle.git_gate import GIT_GATE_TIMEOUT_SECS from bot_bottle.git_http_backend import GitHttpHandler, MAX_BODY_BYTES @@ -201,7 +201,7 @@ class TestGitHttpBackend(unittest.TestCase): for call in run.call_args_list: self.assertEqual( - GIT_GATE_DAEMON_TIMEOUT_SECS, + GIT_GATE_TIMEOUT_SECS, call.kwargs.get("timeout"), f"subprocess.run call missing timeout: {call}", ) -- 2.52.0