Skip to content

Commit 57ffcb1

Browse files
TMHSDigitalclaude
andcommitted
fix(drift-check): keep the clone token out of argv and error output
Remote sparse clones now use a clean URL and pass credentials through git's environment-config channel (http.extraheader), so the token never appears in argv, the process list, .git/config, or error text. Errors are also redacted as defense in depth. git calls get a timeout, and temp clone cleanup clears read-only pack files on Windows. Closes #105 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: TMHSDigital <154358121+TMHSDigital@users.noreply.github.com>
1 parent fe0c902 commit 57ffcb1

2 files changed

Lines changed: 92 additions & 40 deletions

File tree

‎scripts/drift_check/snapshot.py‎

Lines changed: 65 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -21,10 +21,12 @@
2121
"""
2222
from __future__ import annotations
2323

24+
import base64
2425
import contextlib
2526
import json
2627
import os
2728
import shutil
29+
import stat
2830
import subprocess
2931
import sys
3032
import tempfile
@@ -266,40 +268,84 @@ def build_local_snapshot(
266268
# ---------------------------------------------------------------------------
267269

268270

271+
GIT_TIMEOUT_SECONDS = 300
272+
273+
274+
def _git_auth_env(token: str) -> dict[str, str]:
275+
"""Credentials for github.com passed through git's environment-config
276+
channel (``GIT_CONFIG_COUNT``/``KEY``/``VALUE``) instead of the clone URL,
277+
so the token never appears in argv, the process list, ``.git/config`` or
278+
error messages."""
279+
basic = base64.b64encode(f"x-access-token:{token}".encode()).decode()
280+
return {
281+
"GIT_CONFIG_COUNT": "1",
282+
"GIT_CONFIG_KEY_0": "http.https://github.com/.extraheader",
283+
"GIT_CONFIG_VALUE_0": f"AUTHORIZATION: basic {basic}",
284+
}
285+
286+
287+
def _redact(text: str, secrets: tuple[str, ...]) -> str:
288+
for secret in secrets:
289+
if secret:
290+
text = text.replace(secret, "***")
291+
return text
292+
293+
269294
def _run_git(
270295
args: list[str],
271296
*,
272297
cwd: Path | None = None,
273298
env: dict[str, str] | None = None,
274299
check: bool = True,
300+
redact: tuple[str, ...] = (),
275301
) -> subprocess.CompletedProcess:
276302
"""Thin wrapper around git subprocess for testability and uniform
277303
error handling. Uses ``GIT_TERMINAL_PROMPT=0`` to fail fast on auth
278-
issues instead of hanging for input."""
304+
issues instead of hanging for input, and a timeout so a stalled
305+
network clone cannot block the job indefinitely. Any value in
306+
*redact* is masked in error messages."""
279307
full_env = {**os.environ, "GIT_TERMINAL_PROMPT": "0"}
280308
if env:
281309
full_env.update(env)
282-
result = subprocess.run(
283-
["git", *args],
284-
cwd=str(cwd) if cwd else None,
285-
env=full_env,
286-
capture_output=True,
287-
text=True,
288-
)
310+
try:
311+
result = subprocess.run(
312+
["git", *args],
313+
cwd=str(cwd) if cwd else None,
314+
env=full_env,
315+
capture_output=True,
316+
text=True,
317+
timeout=GIT_TIMEOUT_SECONDS,
318+
)
319+
except subprocess.TimeoutExpired:
320+
raise RemoteSnapshotError(
321+
_redact(f"git {' '.join(args)} timed out after {GIT_TIMEOUT_SECONDS}s", redact)
322+
) from None
289323
if check and result.returncode != 0:
290324
raise RemoteSnapshotError(
291-
f"git {' '.join(args)} failed (rc={result.returncode}): "
292-
f"{result.stderr.strip() or result.stdout.strip()}"
325+
_redact(
326+
f"git {' '.join(args)} failed (rc={result.returncode}): "
327+
f"{result.stderr.strip() or result.stdout.strip()}",
328+
redact,
329+
)
293330
)
294331
return result
295332

296333

297-
def _scrub_token_from_remote(repo_dir: Path, owner: str, slug: str) -> None:
298-
"""After clone, replace the token-bearing remote URL with a clean one.
299-
Defense-in-depth: prevents the token from leaking via subsequent ``git
300-
remote -v`` calls or accidental ``.git/config`` exposure."""
301-
clean_url = f"https://github.com/{owner}/{slug}.git"
302-
_run_git(["remote", "set-url", "origin", clean_url], cwd=repo_dir, check=False)
334+
def _force_remove_readonly(func, path, _exc) -> None:
335+
"""rmtree error hook: git pack files are read-only on Windows, so clear
336+
the bit and retry once."""
337+
try:
338+
os.chmod(path, stat.S_IWRITE)
339+
func(path)
340+
except OSError:
341+
pass
342+
343+
344+
def _rmtree(path: Path) -> None:
345+
if sys.version_info >= (3, 12):
346+
shutil.rmtree(path, onexc=_force_remove_readonly)
347+
else: # pragma: no cover - older interpreters
348+
shutil.rmtree(path, onerror=_force_remove_readonly)
303349

304350

305351
@contextlib.contextmanager
@@ -317,7 +363,7 @@ def _sparse_clone(
317363
token = gh_token.strip()
318364
if not token:
319365
raise RemoteSnapshotError(f"gh_token is empty for {owner}/{slug}")
320-
clone_url = f"https://x-access-token:{token}@github.com/{owner}/{slug}.git"
366+
clone_url = f"https://github.com/{owner}/{slug}.git"
321367
clone_args = [
322368
"clone",
323369
"--filter=blob:none",
@@ -327,8 +373,7 @@ def _sparse_clone(
327373
if branch:
328374
clone_args += ["--branch", branch]
329375
clone_args += [clone_url, str(tmp)]
330-
_run_git(clone_args)
331-
_scrub_token_from_remote(tmp, owner, slug)
376+
_run_git(clone_args, env=_git_auth_env(token), redact=(token,))
332377
# `--no-cone` lets us mix file-level patterns (AGENTS.md, CLAUDE.md)
333378
# with directory patterns (skills, rules). Cone-mode is the default
334379
# since git 2.35 and rejects file patterns. The deprecation warning
@@ -340,7 +385,7 @@ def _sparse_clone(
340385
)
341386
yield tmp
342387
finally:
343-
shutil.rmtree(tmp, ignore_errors=True)
388+
_rmtree(tmp)
344389

345390

346391
def build_remote_snapshot(

‎tests/test_snapshot_remote.py‎

Lines changed: 27 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,6 @@
2222
from scripts.drift_check.snapshot import (
2323
SPARSE_PATHS,
2424
RemoteSnapshotError,
25-
_scrub_token_from_remote,
2625
build_remote_snapshot,
2726
)
2827

@@ -64,7 +63,6 @@ class _Result:
6463
def test_remote_snapshot_issues_correct_git_commands(tmp_path: Path):
6564
fake = _FakeRun([
6665
(0, "", ""), # clone
67-
(0, "", ""), # remote set-url (token scrub)
6866
(0, "", ""), # sparse-checkout set
6967
])
7068
# Patch tempfile so we know exactly which tempdir gets used.
@@ -93,20 +91,19 @@ def test_remote_snapshot_issues_correct_git_commands(tmp_path: Path):
9391
assert "--filter=blob:none" in clone_args
9492
assert "--sparse" in clone_args
9593
assert "--depth" in clone_args
96-
# Token must be in the clone URL
94+
# Token must never be in argv; it travels in the env-config header.
95+
assert "ghp_test" not in " ".join(clone_args)
9796
url = [a for a in clone_args if a.startswith("https://")]
98-
assert url and "ghp_test" in url[0]
99-
assert "github.com/TMHSDigital/CFX-Developer-Tools.git" in url[0]
100-
101-
# Verify token scrub is the second call
102-
scrub_args = fake.calls[1][0]
103-
assert scrub_args[1] == "remote"
104-
assert scrub_args[2] == "set-url"
105-
assert "ghp_test" not in " ".join(scrub_args)
97+
assert url == ["https://github.com/TMHSDigital/CFX-Developer-Tools.git"]
98+
clone_env = fake.calls[0][1]["env"]
99+
assert clone_env["GIT_CONFIG_KEY_0"] == "http.https://github.com/.extraheader"
100+
assert clone_env["GIT_CONFIG_VALUE_0"].startswith("AUTHORIZATION: basic ")
101+
assert "ghp_test" not in clone_env["GIT_CONFIG_VALUE_0"]
102+
assert fake.calls[0][1]["timeout"] > 0
106103

107104
# Verify sparse-checkout uses --no-cone (required for file-level
108105
# patterns like AGENTS.md) and includes all SPARSE_PATHS
109-
sparse_args = fake.calls[2][0]
106+
sparse_args = fake.calls[1][0]
110107
assert sparse_args[1] == "sparse-checkout"
111108
assert sparse_args[2] == "set"
112109
assert "--no-cone" in sparse_args
@@ -136,7 +133,6 @@ def test_remote_snapshot_cleans_up_on_clone_failure(tmp_path: Path):
136133
def test_remote_snapshot_cleans_up_on_sparse_failure(tmp_path: Path):
137134
fake = _FakeRun([
138135
(0, "", ""), # clone OK
139-
(0, "", ""), # scrub OK
140136
(1, "", "bad path"), # sparse-checkout FAIL
141137
])
142138
sparse_dir = tmp_path / "sparse"
@@ -169,13 +165,24 @@ def test_remote_snapshot_rejects_empty_token(tmp_path: Path):
169165
assert not sparse_dir.exists()
170166

171167

172-
def test_scrub_does_not_raise_on_failure(tmp_path: Path):
173-
"""Scrub is best-effort. If git remote set-url fails for some reason
174-
(e.g., the clone partially succeeded), the snapshot still completes."""
175-
fake = _FakeRun([(1, "", "no remote")])
176-
with patch("scripts.drift_check.snapshot.subprocess.run", fake):
177-
# Should not raise
178-
_scrub_token_from_remote(tmp_path, "TMHSDigital", "x")
168+
def test_clone_failure_message_never_contains_token(tmp_path: Path):
169+
"""git can echo the remote or auth details on failure; the raised
170+
error must mask the token either way."""
171+
fake = _FakeRun([(128, "", "fatal: auth failed for ghp_secret_value")])
172+
sparse_dir = tmp_path / "sparse"
173+
sparse_dir.mkdir()
174+
with patch("scripts.drift_check.snapshot.subprocess.run", fake), \
175+
patch("scripts.drift_check.snapshot.tempfile.mkdtemp", return_value=str(sparse_dir)):
176+
with pytest.raises(RemoteSnapshotError) as exc_info:
177+
build_remote_snapshot(
178+
repo_slug="x",
179+
meta_version=META,
180+
meta_commit="sha",
181+
config=_cfg(),
182+
gh_token="ghp_secret_value",
183+
)
184+
assert "ghp_secret_value" not in str(exc_info.value)
185+
assert "***" in str(exc_info.value)
179186

180187

181188
@pytest.mark.skipif(

0 commit comments

Comments
 (0)