From 1719d181812245326866046cc1589039393e3f9a Mon Sep 17 00:00:00 2001 From: Lukas Geiger Date: Mon, 2 Nov 2026 01:48:06 +0100 Subject: [PATCH 1/2] fix(git): reject ambient client certificates without empty TLS paths --- docs/git-tls-transport.md | 16 ++++++++++ src/githubbot_core/typed_actions.py | 31 ++++++++++++++++---- tests/test_git_tls_policy.py | 45 +++++++++++++++++++++++++++++ tests/test_typed_actions.py | 19 +++++++++++- 4 files changed, 104 insertions(+), 7 deletions(-) create mode 100644 docs/git-tls-transport.md create mode 100644 tests/test_git_tls_policy.py diff --git a/docs/git-tls-transport.md b/docs/git-tls-transport.md new file mode 100644 index 0000000..95e5f47 --- /dev/null +++ b/docs/git-tls-transport.md @@ -0,0 +1,16 @@ +# Git client certificates in bot transport + +The bot transport uses one process-local bot token, an isolated AskPass helper, +exact origin/ref checks and verified TLS. System/global Git configuration and +certificate environment overrides are excluded from that process. + +Empty `http.sslCert=` / `http.sslKey=` path overrides can make Git reject even +an unauthenticated public read with `credential missing host field`. The +transport therefore rejects local client-certificate configuration (including +URL scopes and includes) before push instead of overriding those paths with +empty strings. It reads key names only and fails closed on probe errors. + +All other credential/header/cookie overrides, HTTPS verification, identity +checks and exact remote readback remain in place. A host configured for a TLS +client certificate needs a separate reviewed deployment policy; this change +does not activate certificate authentication or alter host configuration. diff --git a/src/githubbot_core/typed_actions.py b/src/githubbot_core/typed_actions.py index fdaa865..7ef49f3 100644 --- a/src/githubbot_core/typed_actions.py +++ b/src/githubbot_core/typed_actions.py @@ -491,10 +491,6 @@ def _git_auth_config(repo: str) -> tuple[str, ...]: "-c", "http.followRedirects=false", "-c", - "http.sslCert=", - "-c", - "http.sslKey=", - "-c", "http.cookieFile=", ] for scope in scopes: @@ -503,8 +499,6 @@ def _git_auth_config(repo: str) -> tuple[str, ...]: f"http.{scope}.extraheader=", f"http.{scope}.sslverify=true", f"http.{scope}.followredirects=false", - f"http.{scope}.sslcert=", - f"http.{scope}.sslkey=", f"http.{scope}.cookiefile=", ): args.extend(("-c", setting)) @@ -546,6 +540,28 @@ def _git_base_environment() -> dict[str, str]: return env +def _client_certificate_policy_error(path: str) -> str | None: + """Reject local TLS client credentials instead of passing invalid empty file paths. + + System/global config and certificate environment overrides are already excluded + by _git_base_environment. Includes are evaluated, but only key names are read. + """ + try: + result = _git( + ["git", "-C", path, "config", "--includes", "--name-only", "--get-regexp", + r"^http(\..*)?\.ssl(cert|key)$"], + capture_output=True, text=True, encoding="utf-8", errors="replace", + timeout=120, env=_git_base_environment(), + ) + except Exception: + return "local Git client-certificate policy could not be checked" + if result.returncode == 1 and not (result.stdout or "").strip(): + return None + if result.returncode == 0: + return "local Git client-certificate configuration is not allowed for bot transport" + return "local Git client-certificate policy could not be checked" + + @contextmanager def _git_auth_environment(token: str, account: str): """Prepare a short-lived, secret-free AskPass helper and a process-only env.""" @@ -708,6 +724,9 @@ def exec_push_branch(transport, params: dict, config: dict): if error or checked_branch != branch: return None, "branch ref is invalid" ref = f"refs/heads/{branch}" + certificate_error = _client_certificate_policy_error(path) + if certificate_error: + return None, certificate_error try: with _git_auth_environment(token, account) as env: # No '+' refspec and no --force: a non-fast-forward is rejected by git itself. diff --git a/tests/test_git_tls_policy.py b/tests/test_git_tls_policy.py new file mode 100644 index 0000000..4912027 --- /dev/null +++ b/tests/test_git_tls_policy.py @@ -0,0 +1,45 @@ +"""Client certificate state must fail closed without empty TLS-path overrides.""" +import subprocess + +import pytest + +from githubbot_core import typed_actions + + +def test_bot_transport_does_not_pass_empty_client_certificate_paths(): + flags = typed_actions._git_auth_config("example/repo") + assert not any(flag.lower().endswith(("sslcert=", "sslkey=")) for flag in flags) + assert "http.sslVerify=true" in flags + assert "credential.helper=" in flags + + +@pytest.mark.parametrize("key", ["http.sslCert", "http.sslKey", "http.https://github.com/.sslCert"]) +def test_local_client_certificate_is_rejected_even_with_empty_value(tmp_path, key): + subprocess.run(["git", "init", "-q", str(tmp_path)], check=True, capture_output=True) + subprocess.run(["git", "-C", str(tmp_path), "config", key, ""], check=True, capture_output=True) + assert "client-certificate" in typed_actions._client_certificate_policy_error(str(tmp_path)) + + +def test_clean_local_configuration_needs_no_client_certificate(tmp_path): + subprocess.run(["git", "init", "-q", str(tmp_path)], check=True, capture_output=True) + assert typed_actions._client_certificate_policy_error(str(tmp_path)) is None + + +def test_included_client_key_is_also_rejected(tmp_path): + subprocess.run(["git", "init", "-q", str(tmp_path)], check=True, capture_output=True) + included = tmp_path / "certificate.config" + included.write_text('[http "https://github.com/"]\n sslKey = ignored-private-path\n', encoding="utf-8") + subprocess.run( + ["git", "-C", str(tmp_path), "config", "include.path", str(included)], + check=True, capture_output=True, + ) + assert "client-certificate" in typed_actions._client_certificate_policy_error(str(tmp_path)) + + +def test_certificate_probe_errors_are_fail_closed_and_redacted(monkeypatch): + monkeypatch.setattr( + typed_actions, "_git", + lambda *args, **kwargs: subprocess.CompletedProcess([], 128, "private-config", "private-error"), + ) + error = typed_actions._client_certificate_policy_error(".") + assert error and "private" not in error diff --git a/tests/test_typed_actions.py b/tests/test_typed_actions.py index a8035a1..5b02a4c 100644 --- a/tests/test_typed_actions.py +++ b/tests/test_typed_actions.py @@ -290,9 +290,11 @@ def reader(providers): def _fake_git_output(command, *, origin=None, pushurl=None, readback=None, fail_push=False): op_index = next( - i for i, part in enumerate(command) if part in {"remote", "rev-parse", "check-ref-format", "push", "ls-remote"} + i for i, part in enumerate(command) if part in {"remote", "rev-parse", "check-ref-format", "push", "ls-remote", "config"} ) op = command[op_index] + if op == "config": + return SimpleNamespace(returncode=1, stdout="", stderr="") if op == "remote": if "--push" in command: output = pushurl if pushurl is not None else origin @@ -379,6 +381,21 @@ def missing_reader(providers): assert calls == [] +def test_push_branch_stops_before_network_push_for_client_certificate_policy(monkeypatch): + _mock_push_token(monkeypatch) + calls = [] + + def git(cmd, **kwargs): + calls.append(cmd) + return _fake_git_output(cmd, origin=f"https://github.com/{REPO}.git") + + monkeypatch.setattr(typed_actions, "_git", git) + monkeypatch.setattr(typed_actions, "_client_certificate_policy_error", lambda path: "certificate policy denied") + receipt = actions.apply([_push_action()], _push_transport(), config())["receipts"][0] + assert receipt["status"] == "failed" and "certificate policy denied" in receipt["detail"] + assert not any("push" in cmd or "ls-remote" in cmd for cmd in calls) + + def test_push_branch_rejects_bad_prefix_before_git_or_credential_lookup(monkeypatch): calls = [] monkeypatch.setattr( From 9d9870609367fe1d983a8c9ead06bf5e2a16b518 Mon Sep 17 00:00:00 2001 From: Lukas Geiger Date: Mon, 2 Nov 2026 01:52:04 +0100 Subject: [PATCH 2/2] style: apply Core CI formatter to transport correction --- src/githubbot_core/typed_actions.py | 11 +++++++---- tests/test_git_tls_policy.py | 7 +++++-- tests/test_typed_actions.py | 4 +++- 3 files changed, 15 insertions(+), 7 deletions(-) diff --git a/src/githubbot_core/typed_actions.py b/src/githubbot_core/typed_actions.py index 7ef49f3..15bfb18 100644 --- a/src/githubbot_core/typed_actions.py +++ b/src/githubbot_core/typed_actions.py @@ -548,10 +548,13 @@ def _client_certificate_policy_error(path: str) -> str | None: """ try: result = _git( - ["git", "-C", path, "config", "--includes", "--name-only", "--get-regexp", - r"^http(\..*)?\.ssl(cert|key)$"], - capture_output=True, text=True, encoding="utf-8", errors="replace", - timeout=120, env=_git_base_environment(), + ["git", "-C", path, "config", "--includes", "--name-only", "--get-regexp", r"^http(\..*)?\.ssl(cert|key)$"], + capture_output=True, + text=True, + encoding="utf-8", + errors="replace", + timeout=120, + env=_git_base_environment(), ) except Exception: return "local Git client-certificate policy could not be checked" diff --git a/tests/test_git_tls_policy.py b/tests/test_git_tls_policy.py index 4912027..c003f26 100644 --- a/tests/test_git_tls_policy.py +++ b/tests/test_git_tls_policy.py @@ -1,4 +1,5 @@ """Client certificate state must fail closed without empty TLS-path overrides.""" + import subprocess import pytest @@ -31,14 +32,16 @@ def test_included_client_key_is_also_rejected(tmp_path): included.write_text('[http "https://github.com/"]\n sslKey = ignored-private-path\n', encoding="utf-8") subprocess.run( ["git", "-C", str(tmp_path), "config", "include.path", str(included)], - check=True, capture_output=True, + check=True, + capture_output=True, ) assert "client-certificate" in typed_actions._client_certificate_policy_error(str(tmp_path)) def test_certificate_probe_errors_are_fail_closed_and_redacted(monkeypatch): monkeypatch.setattr( - typed_actions, "_git", + typed_actions, + "_git", lambda *args, **kwargs: subprocess.CompletedProcess([], 128, "private-config", "private-error"), ) error = typed_actions._client_certificate_policy_error(".") diff --git a/tests/test_typed_actions.py b/tests/test_typed_actions.py index 5b02a4c..5dc8ec8 100644 --- a/tests/test_typed_actions.py +++ b/tests/test_typed_actions.py @@ -290,7 +290,9 @@ def reader(providers): def _fake_git_output(command, *, origin=None, pushurl=None, readback=None, fail_push=False): op_index = next( - i for i, part in enumerate(command) if part in {"remote", "rev-parse", "check-ref-format", "push", "ls-remote", "config"} + i + for i, part in enumerate(command) + if part in {"remote", "rev-parse", "check-ref-format", "push", "ls-remote", "config"} ) op = command[op_index] if op == "config":