feat(hooks): capture git_remote_url for Repo Allowlist (claude-code + copilot)#192
feat(hooks): capture git_remote_url for Repo Allowlist (claude-code + copilot)#192MohamedAklamaash wants to merge 7 commits into
Conversation
…po Allowlist client-hook)
Capture the credential-stripped origin remote URL for the event cwd via a
short-timeout `git config --get remote.origin.url` subprocess, cached per
(session_id, cwd), and inject it as `git_remote_url` into both the user_prompt
and pre_tool_use bodies. Never raises; null when not a git repo, no origin,
git missing/timeout, or no cwd. Copilot UserPromptSubmit cwd is unverified
in-repo, so its prompt-submit path reads event.get('cwd') defensively.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
_strip_git_credentials only stripped scheme:// URLs via first-@, so scp-form remotes (user:token@host:org/repo) leaked credentials on the wire and an @ in the password left a fragment. Rewrite to strip userinfo using the last @ before the path boundary for both scheme and scp forms; total (never raises). Helper kept byte-for-byte identical across claude-code and copilot. Extends the credential test matrix (scp-with-token, @-in-password, ssh port). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| def _get_git_context(session_id: Optional[str], cwd: Optional[str]) -> Optional[str]: | ||
| """Credential-stripped origin remote URL for cwd, or None. Cached per | ||
| (session_id, cwd); never raises.""" | ||
| key = (session_id, cwd) | ||
| if key in _GIT_CONTEXT_CACHE: | ||
| return _GIT_CONTEXT_CACHE[key] | ||
| result = None | ||
| if cwd: | ||
| try: | ||
| out = subprocess.run( | ||
| ['git', '-C', cwd, 'config', '--get', 'remote.origin.url'], | ||
| capture_output=True, text=True, timeout=2, | ||
| ) | ||
| if out.returncode == 0: | ||
| url = out.stdout.strip() | ||
| if url: | ||
| result = _strip_git_credentials(url) | ||
| except Exception: | ||
| result = None | ||
| _GIT_CONTEXT_CACHE[key] = result | ||
| return result |
There was a problem hiding this comment.
_get_git_context introduces a new subprocess call path with caching but ships zero Prometheus metrics. Per the team's instrumentation requirement, every new flow must be instrumented now regardless of dashboard use. At minimum this flow needs: (1) a latency histogram for the subprocess.run call (labeled with outcome: hit, miss, timeout, error), (2) a success/failure counter, and (3) a cache hit/miss counter. Without these, a regression (e.g., a git binary path change causing silent None for all users) would be completely invisible in production. The same gap exists identically in copilot/hooks/unbound.py.
Context Used: P0 — Critical (must block merge)
Django / Backend ... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Intentional — these are client-side hooks that run as short-lived subprocesses inside the user's editor (Claude Code / Copilot / Cursor), not a long-running server. There is no Prometheus registry, scrape endpoint, or process for a collector to reach, so a histogram/counter would have nothing to expose. The server-side instrumentation requirement doesn't apply to this runtime. Observability for this flow is handled the way the rest of the hook is: _get_git_context now emits a log_error (category git_context, fire-and-forward to the gateway error sink) on the failure path, which is the signal a 'git binary path change → silent None' regression would surface through.
There was a problem hiding this comment.
That's a valid correction — the client-side subprocess runtime makes Prometheus metrics inapplicable here, and the log_error path to the gateway error sink is the right observability primitive for this context. Withdrawing this comment.
Tip: You can customize Greptile's behavior for this repo with .greptile/rules.md and .greptile/config.json.
| except Exception: | ||
| result = None |
There was a problem hiding this comment.
The bare except Exception: result = None swallows every failure — FileNotFoundError (git not on PATH), TimeoutExpired, permission errors — without emitting any log entry. If _get_git_context starts returning None in production due to a misconfiguration or environment issue, there is no signal in the logs to investigate. The same silent-swallow pattern exists in copilot/hooks/unbound.py at the equivalent line. At minimum, a log_error (or equivalent) with session_id and cwd should be emitted on the exception branch so failures are reconstructable.
Context Used: P0 — Critical (must block merge)
Django / Backend ... (source)
There was a problem hiding this comment.
Fixed in 122e7ed — the subprocess call is now wrapped so the exception branch emits log_error("git context lookup failed session=… cwd=…: <exc>") (category git_context) before returning None. Control flow is unchanged; failures are now reconstructable. Mirrored across all three hooks.
vigneshsubbiah16
left a comment
vigneshsubbiah16
left a comment
There was a problem hiding this comment.
🛡️ Automated Security Review (consensus)
1 finding — 0 high-confidence, 1 to triage. Reviewers: Cursor, Claude, Semgrep, Gitleaks.
Findings
🟡 TRIAGE — _strip_git_credentials can leave credentials in git_remote_url
claude-code/hooks/unbound.py:943 · copilot/hooks/unbound.py:925
Impact: Authority is truncated at the first / before locating @; a hand-set remote with a literal / in userinfo (e.g. https://user:p/ss@github.com/org/repo) returns the URL unchanged and may send/store the password in gateway audit records.
Fix: Resolve @ on the full credential span before splitting on /, or fail closed (return None/redact) when @ remains after stripping; add a /-in-password test case.
Flagged by: Claude, Cursor (lead)
Previously acknowledged (not re-flagged)
- Client-supplied
git_remote_urlas an enforcement boundary — PR threat model: careless-insider deter/visibility control, trivially bypassable via rewrittenoriginor direct provider access; default audit, block opt-in. Accepted by design.
🤖 consensus review · reviewers: Cursor,Claude,Semgrep,Gitleaks · head df98388a · 2026-06-29T19:44Z
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
🛡️ Automated Security Review (consensus)2 findings — 2 high-confidence, 0 to triage. Reviewers: Cursor, Claude, Semgrep, Gitleaks. Credential sanitizer fails open on parse errors — 🔴 HIGH
Copilot
|
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
There are 3 total unresolved issues (including 2 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 7849e78. Configure here.
🛡️ Automated Security Review (consensus)2 findings — 2 high-confidence, 0 to triage. Reviewers: Cursor, Claude, Semgrep, Gitleaks. Credential sanitizer fails open on parse errors — 🔴 HIGH
Credential sanitizer leaks when password contains
|
…hook MultiEdit and NotebookEdit mutate files but were absent from the claude-code native-file-tool set, so edits through them skipped the git_remote_url stamp and never reached the repo-allowlist gate. Add both to ALLOWED_NON_MCP_HOOK_NAMES and NATIVE_FILE_TOOLS, and extract the target path (file_path for MultiEdit, notebook_path for NotebookEdit) so the gate resolves the governing repo exactly like Edit. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Assert _tool_file_path returns None for empty/missing file_path so the file-tool repo resolution never treats a blank path as a target. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Update (repo-allowlist hook parity):
|
🛡️ Automated Security Review (consensus)2 findings — 1 high-confidence, 1 to triage. Reviewers: Cursor, Claude, Semgrep, Gitleaks. Credential sanitizer fails open on parse errors — 🔴 HIGH
Copilot
|
- Fail closed in _strip_git_credentials: return None on parse failure instead of the raw credential-bearing URL (no userinfo leak to gateway). - Do not cache transient git-subprocess failures; log them at debug level with session/cwd. Bound _GIT_CONTEXT_CACHE to 256 entries (FIFO evict). - Skip the git-context lookup on the Slack approval-retry path whose body is never sent. - copilot: resolve apply_patch repo identity from the patch payload path. Mirrored across claude-code, copilot, and cursor hooks. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Review dispositions — addressed in
|
🛡️ Automated Security Review (consensus)2 findings — 1 high-confidence, 1 to triage. Reviewers: Cursor, Claude, Semgrep, Gitleaks. Credential sanitizer leaks userinfo when password contains
|
2 participants
Summary
Client hooks capture the working repo's git remote and send it to the gateway so the Repo Allowlist policy can be enforced. v1 tools: claude-code + copilot.
Changes
_get_git_context(cwd): onegit config --get remote.origin.urlsubprocess (2s timeout), credentials stripped (scheme and scp form, last-@), cached per (session_id, cwd), never crashes the hook (null on no-git/no-remote/timeout).git_remote_urladded to bothuser_promptandpre_tool_usebodies in claude-code and copilotunbound.py; helper byte-for-byte identical across both copies. Backward-compatible (null/absent → unchanged shape). git via subprocess only (Zscaler curl rule untouched).Notes
Copilot's UserPromptSubmit
cwdis unconfirmed against a live transcript — handled safely (passes through, null if absent; pre-tool-use path always works). Follow-up: confirm Copilot prompt-submit payload.Tests
16 tests / 40 subtests ✓ — credential-strip matrix (scheme/scp/@-in-password/ssh-port), no-git→None, timeout→None, caching (1 shell-out for 2 calls), 4-body parity.
Threat model (honest labeling): careless-insider control. Repo identity is client-supplied git context and is trivially bypassable by rewriting
originor going direct-to-provider — a deter/visibility control, not a hard security boundary. Default audit; block is opt-in per policy.Part of a 4-PR feature (Repo Allowlist):
ai-gateway-data(policy type) ·ai-gateway(enforcement) ·setup(hook capture) ·unbound-fe(editor).🤖 Generated with Claude Code
Note
Medium Risk
Changes the policy pre-check payload on a latency-sensitive path (subprocess + caching) across three integrations; repo identity is client-supplied and bypassable, though failures degrade to null without blocking the hook.
Overview
Adds Repo Allowlist support by resolving the working repository’s credential-stripped
originURL and sending it asgit_remote_urlon pre-tool-use gateway requests in claude-code, copilot, and cursor hooks.New helpers run a bounded
git configlookup (2s timeout, per-(session_id, cwd)cache), strip embedded credentials from remotes, and pick the repo directory via_repo_context_dir(target file’s nearest existing ancestor, so new paths still map to the enclosing repo). Claude Code also treatsMultiEdit/NotebookEditas native file tools and unifies path extraction with_tool_file_path(file_path/notebook_path). Copilot can derive the target path fromapply_patchpatch text when building metadata.test_repo_allowlist.pycovers stripping, caching, repo-dir resolution, payload parity, and approval-retry behavior (git lookup skipped on retry).Reviewed by Cursor Bugbot for commit 122e7ed. Bugbot is set up for automated code reviews on this repo. Configure here.
Linear: WEB-5002 — Check the cwd and allow AI tools only for organization repositories.
Greptile Summary
This PR captures the working repository's git remote URL (
git config --get remote.origin.url) in the pre-tool-use hook path for claude-code, copilot, and cursor, attaching a credential-strippedgit_remote_urlfield to each gateway request so the Repo Allowlist policy can be enforced server-side._get_git_context/_strip_git_credentials/_repo_context_dirhelpers are added identically to all three hooks; results are cached per(session_id, cwd)with a 256-entry FIFO bound; transient subprocess failures log an error and skip caching, avoiding the poison-cache problem.NATIVE_FILE_TOOLSto includeMultiEditandNotebookEdit, with a new_tool_file_pathhelper that normalisesfile_path/notebook_pathextraction.test_repo_allowlist.py(16 test classes, 40+ subtests) covers credential-strip edge cases, caching, ancestor-dir walk, subdir repo resolution, approval-retry bypass, and payload parity across all three hooks.Confidence Score: 4/5
The change is fail-open (null on any error), credential-stripping is thoroughly tested, and the approval-retry early return correctly bypasses the git subprocess. The one gap is that cursor/unbound.py ships the new subprocess and cache path with zero instrumentation, matching the already-noted absence in claude-code and copilot.
The core logic is sound and the test suite is thorough, but the cursor hook introduces the same unmonitored subprocess/cache flow that was flagged for claude-code and copilot — a regression there (git binary unreachable, wrong remote resolved) would be invisible in production with no latency histogram, counter, or cache-hit metric to surface it.
cursor/unbound.py — new _get_git_context flow has no Prometheus metrics; same instrumentation gap as claude-code and copilot but not yet flagged for cursor.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[process_pre_tool_use] --> B{is_retry?} B -- yes, marker_data found --> C[poll_approval_status] C --> D[return early — no git lookup] B -- no, or marker_data missing --> E[build request_body] E --> F[_tool_file_path / file_path extraction] F --> G[_repo_context_dir\ncwd + file_path → nearest existing dir] G --> H{cache hit?\nkey = session_id × cwd} H -- yes --> I[return cached URL or None] H -- no --> J[subprocess: git -C cwd config --get remote.origin.url\ntimeout=2s] J -- exception\nTimeout / FileNotFoundError --> K[log_error, return None\nnot cached] J -- returncode != 0 --> L[result = None\ncache entry written] J -- returncode == 0 --> M[_strip_git_credentials\nremove userinfo from URL] M --> N[result = stripped URL\ncache entry written] L --> O[request_body.git_remote_url = result] N --> O I --> O O --> P[send_to_hook_api]%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% flowchart TD A[process_pre_tool_use] --> B{is_retry?} B -- yes, marker_data found --> C[poll_approval_status] C --> D[return early — no git lookup] B -- no, or marker_data missing --> E[build request_body] E --> F[_tool_file_path / file_path extraction] F --> G[_repo_context_dir\ncwd + file_path → nearest existing dir] G --> H{cache hit?\nkey = session_id × cwd} H -- yes --> I[return cached URL or None] H -- no --> J[subprocess: git -C cwd config --get remote.origin.url\ntimeout=2s] J -- exception\nTimeout / FileNotFoundError --> K[log_error, return None\nnot cached] J -- returncode != 0 --> L[result = None\ncache entry written] J -- returncode == 0 --> M[_strip_git_credentials\nremove userinfo from URL] M --> N[result = stripped URL\ncache entry written] L --> O[request_body.git_remote_url = result] N --> O I --> O O --> P[send_to_hook_api]Reviews (5): Last reviewed commit: "fix(repo-allowlist): harden git-context ..." | Re-trigger Greptile
Context used:
Django / Backend ... (source)