Add explicit timeouts to subprocess and HTTP calls in git-gate paths #271

Merged
didericis merged 3 commits from add-subprocess-http-timeouts into main 2026-06-25 04:23:03 -04:00
5 changed files with 105 additions and 8 deletions
@@ -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:
+6 -6
View File
@@ -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 \\",
+4
View File
@@ -16,6 +16,8 @@ from http.server import BaseHTTPRequestHandler, ThreadingHTTPServer
from pathlib import Path
from urllib.parse import urlsplit
from .git_gate import GIT_GATE_TIMEOUT_SECS
Outdated
Review

Given we're using this for two different purposes now, probably makes sense to rename it to GIT_GATE_TIMEOUT_SECS. Can add a comment about where it's currently used/fact that it's used in a multiple places.

Given we're using this for two different purposes now, probably makes sense to rename it to `GIT_GATE_TIMEOUT_SECS`. Can add a comment about where it's currently used/fact that it's used in a multiple places.
DEFAULT_PORT = 9420
1
@@ -47,6 +49,7 @@ class GitHttpHandler(BaseHTTPRequestHandler):
[hook_path, "upload-pack", str(repo_dir), peer, peer],
capture_output=True,
check=False,
timeout=GIT_GATE_TIMEOUT_SECS,
)
if hook.returncode != 0:
detail = (hook.stderr or hook.stdout).decode(
@@ -110,6 +113,7 @@ class GitHttpHandler(BaseHTTPRequestHandler):
env=env,
capture_output=True,
check=False,
timeout=GIT_GATE_TIMEOUT_SECS,
)
self._write_cgi_response(proc.stdout)
@@ -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(
+56
View File
@@ -9,6 +9,7 @@ import urllib.request
from pathlib import Path
from unittest import mock
from bot_bottle.git_gate import GIT_GATE_TIMEOUT_SECS
from bot_bottle.git_http_backend import GitHttpHandler, MAX_BODY_BYTES
@@ -150,6 +151,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_GATE_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