From 508c537debd1e35044b2f499560413b50d29034a Mon Sep 17 00:00:00 2001 From: claude Date: Thu, 25 Jun 2026 06:59:09 +0000 Subject: [PATCH] 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