Repository navigation
feat: resolve git, GitHub and SSH accounts in every agent's hook - #395
AakashVelusamy wants to merge 13 commits into
Conversation
Each shell call now carries a destination_account: the push remote's host and owner (login stripped) with the gh login when gh is the credential helper, the gh host/owner/login for gh commands, and the real host/user/port for ssh, scp, sftp and rsync via ssh -G. Only allowlisted local lookups ever run, each with a 5s timeout under a 10s per-turn budget; anything that fails leaves the call account unknown. One byte-identical block in all five Python hooks. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
/review |
|
@cursoragent review |
|
I left a review on pull request #395. I would not ship this as-is. The Pushes are mis-attributed as well.   |
vigneshsubbiah16
left a comment
vigneshsubbiah16
left a comment
There was a problem hiding this comment.
🛡️ Automated Security Review (raw — no orchestrator available)
Reviewers: Semgrep, Gitleaks
SEMGREP
- [MEDIUM]
/home/azureuser/pr-review/work/websentry-ai__setup-395/scan/augment/hooks/unbound.py:4369— These permissions0o700are widely permissive and grant access to more people than may be necessary. A good default is0o644which gives read and write access to yourself and read access to everyo (insecure-file-permissions) - [HIGH]
/home/azureuser/pr-review/work/websentry-ai__setup-395/scan/claude-code/hooks/unbound.py:1624— theencodingargument to Popen is only available on Python 3.6+ (python36-compatibility-Popen2) - [MEDIUM]
/home/azureuser/pr-review/work/websentry-ai__setup-395/scan/claude-code/hooks/unbound.py:5917— These permissions0o700are widely permissive and grant access to more people than may be necessary. A good default is0o644which gives read and write access to yourself and read access to everyo (insecure-file-permissions) - [MEDIUM]
/home/azureuser/pr-review/work/websentry-ai__setup-395/scan/codex/hooks/unbound.py:4702— These permissions0o700are widely permissive and grant access to more people than may be necessary. A good default is0o644which gives read and write access to yourself and read access to everyo (insecure-file-permissions) - [MEDIUM]
/home/azureuser/pr-review/work/websentry-ai__setup-395/scan/copilot/hooks/unbound.py:7470— These permissions0o700are widely permissive and grant access to more people than may be necessary. A good default is0o644which gives read and write access to yourself and read access to everyo (insecure-file-permissions) - [HIGH]
/home/azureuser/pr-review/work/websentry-ai__setup-395/scan/cursor/unbound.py:1092— Avoiding SQL string concatenation: untrusted input concatenated with raw SQL query can result in SQL Injection. In order to execute raw query safely, prepared statement should be used. SQLAlchemy prov (sqlalchemy-execute-raw-query) - [MEDIUM]
/home/azureuser/pr-review/work/websentry-ai__setup-395/scan/cursor/unbound.py:4527— These permissions0o700are widely permissive and grant access to more people than may be necessary. A good default is0o644which gives read and write access to yourself and read access to everyo (insecure-file-permissions)
GITLEAKS
(no secrets detected)
🤖 consensus review · reviewers: Semgrep, Gitleaks · head 6ab9f6d3 · 2026-10-19T14:00Z
| def _account_segments(command: str) -> List[List[str]]: | ||
| """The command split into simple commands on ; && || | and newlines, each as argv; [] when it can't be parsed.""" | ||
| try: | ||
| lexer = shlex.shlex(command, posix=True, punctuation_chars=';&|\n') |
There was a problem hiding this comment.
P1 · Preserve newlines as command separators (codex)
Newlines remain in shlex's default whitespace, so they disappear instead of becoming separator tokens. For example, echo ready\ngit push personal main becomes one argv beginning with echo, and the push receives no destination_account. This affects all five copies.
claude-code/hooks/unbound.py:5238-5247 configures punctuation_chars without removing newline from lexer.whitespace. Executing this parser returned [['echo', 'ready', 'git', 'push', 'personal', 'main']]. The dispatcher only examines each segment's first program.
| flag = arg[1] | ||
| if flag in _ACCOUNT_SSH_VALUE_FLAGS and len(arg) == 2 and i + 1 < len(args): | ||
| value = args[i + 1] | ||
| if flag in 'lF' or (flag == 'p' and program == 'ssh') or (flag == 'P' and program != 'ssh'): |
There was a problem hiding this comment.
P1 · Retain SSH options that determine identity (codex)
The resolver discards -o settings and attached option values. For ssh -o HostName=10.0.4.12 -o User=deploy -p2222 prod-bastion, it runs only ssh -G prod-bastion, reporting the alias's default host, user and port instead of the actual destination. All five hooks share this defect.
claude-code/hooks/unbound.py:5393-5399 forwards only selected separate-value flags; attached flags fall through without preservation. A direct parser check returned ('prod-bastion', []) for this command. claude-code/hooks/unbound.py:5420 constructs the lookup exclusively from those returned options.
| else: | ||
| positional.append(arg) | ||
| i += 1 | ||
| if program == 'ssh': |
There was a problem hiding this comment.
P1 · Recognize plain SFTP destinations (codex)
A normal sftp deploy@prod-bastion invocation receives no account metadata. Only ssh accepts a plain destination; sftp falls into the scp-style parser, which requires a colon. This leaves the standard interactive SFTP connection form untracked in every hook.
claude-code/hooks/unbound.py:5404-5412 special-cases only program == 'ssh' and requires a colon for other tools. Directly evaluating _account_ssh_destination('sftp', ['deploy@prod-bastion']) returned (None, []). Lines 5417-5419 then omit attribution.
| 'project': tool_project | ||
| } | ||
| if tool_name == 'Bash' and isinstance(tool_input, dict): | ||
| account = _destination_account(tool_input.get('command'), shell_dir, account_state) |
There was a problem hiding this comment.
P1 · Resolve accounts using the command's starting directory (codex)
Account resolution receives the directory after the entire command has been processed. Starting in repository A, git push origin main; cd /path/to/B therefore looks up origin in repository B and attributes A's push to B's account. The same ordering occurs in Augment and Copilot; Cursor and explicit-workdir Codex calls also fail to follow preceding cd segments inside the resolver.
claude-code/hooks/unbound.py:4646 updates shell_dir from the command's cd; line 5516 assigns that updated directory before line 5527 calls _destination_account. The resolver iterates command segments without applying cd changes. augment/hooks/unbound.py:4084-4089 and copilot/hooks/unbound.py:5896-5900 use the same post-command directory ordering.
| rest.pop(0) | ||
| if not rest or rest[0] != 'push': | ||
| return None | ||
| positional = [a for a in rest[1:] if not a.startswith('-')] |
There was a problem hiding this comment.
P1 · Consume push option values before selecting remote (codex)
Filtering out dash-prefixed tokens leaves option values in the positional list. For the valid command git push -o ci.skip personal main, the resolver treats ci.skip as the remote and returns unknown instead of resolving personal. Every hook contains this parser.
claude-code/hooks/unbound.py:5326-5336 selects the first non-dash token as the remote without consuming push options. A direct resolver check showed the lookup argv ['git', '-C', '.', 'remote', 'get-url', '--push', 'ci.skip'] for this command.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| env_host = (env.get('GH_HOST') or os.environ.get('GH_HOST') or '').lower() | ||
| host, owner = '', '' | ||
| if repo: | ||
| parts = [p for p in re.sub(r'^[a-z]+://', '', repo).split('/') if p] |
There was a problem hiding this comment.
gh repo URLs never go through _account_strip_login, so user:token@host is copied into host and sent on the tool entry. redact_secrets only covers error reports, not the exchange.
I ran these through _destination_account:
gh pr create -R https://jdoe:ghp_Sup3rSecretToken@github.com/acme/app→host: "jdoe:ghp_sup3rsecrettoken@github.com"GH_REPO=https://jdoe:ghp_Sup3rSecretToken@github.com/acme/app gh repo view→ samegh repo view https://jdoe:ghp_Sup3rSecretToken@ghe.acme.com/acme/app→ token inhostagain
The token is only lowercased. Git push URLs are stripped; this path is not. Strip userinfo with the same helper before splitting the path.
Same function also lets GH_REPO (inline or from the process environment) beat an explicit positional repo. With GH_REPO=acme/app, gh repo view other-org/other-repo was reported as account acme. gh itself prefers -R, then the positional OWNER/REPO, then GH_REPO. Use the env default only when the command did not name a repo.
This block is copied in all five hooks, so the fix has to land in every copy.
| positional = [a for a in rest[1:] if not a.startswith('-')] | ||
| target = positional[0] if positional else None | ||
| base = ['git', '-C', git_dir or '.'] | ||
| if target and re.search(r'://|^[^/:]+@[^/:]+:|^[^/:]+:[^/]', target): |
There was a problem hiding this comment.
A refspec is treated as an scp URL, so git push without a named remote is attributed to a fake host.
In a repo whose origin is https://github.com/acme/app.git:
git push HEAD:main→{platform: git, host: head, account: main}git push main:refs/heads/main→{platform: git, host: main, account: refs}git push refs/heads/main:refs/heads/feature→{platform: git, source: unknown}instead of origin
re.search(r'://|^[^/:]+@[^/:]+:|^[^/:]+:[^/]', target) matches any src:dst. git push origin HEAD:main is fine only because the remote is the first positional. Treat a first positional as a URL when it has :// or user@host, and otherwise let a colon fall through to @{push} / origin. HEAD:main is the form agents actually run.
| destination, passthrough = _account_ssh_destination(program, args) | ||
| if not destination: | ||
| return None | ||
| out = _account_run(['ssh', '-G'] + passthrough + [destination], cwd, state) |
There was a problem hiding this comment.
ssh -G is not a pure local read, and the segment walker runs it for commands the shell never executed.
ssh -G evaluates Match exec while parsing config (and CanonicalizeHostname yes can DNS). I used a config of Match host * exec "touch <marker>" and called _destination_account('false && ssh -F <config> prod-bastion'). The false && means ssh never ran, but the marker was created and the account came back as host 10.9.9.9, user root.
The walker splits on ;, &&, ||, and | and then resolves the first account-bearing segment. A short-circuited or piped ssh -F <file> still reaches this allowlisted ssh -G, with the command's own -F passed through. The config file can run an arbitrary command. That is outside the "five local commands, no network" claim: the allowlist bounds the argv prefix, not what OpenSSH executes while applying config.
Minimum: only probe a segment that actually ran. Even then, ssh -G re-runs Match exec for a command that already ran. Worth deciding explicitly whether telemetry is allowed to do that, and dropping the "no network / no other commands" wording if it stays.
| flag = arg[1] | ||
| if flag in _ACCOUNT_SSH_VALUE_FLAGS and len(arg) == 2 and i + 1 < len(args): | ||
| value = args[i + 1] | ||
| if flag in 'lF' or (flag == 'p' and program == 'ssh') or (flag == 'P' and program != 'ssh'): |
There was a problem hiding this comment.
scp -l / sftp -l are bandwidth limits, not ssh logins, but -l is forwarded into ssh -G for every program.
scp -l 8192 -F <config> file prod-bastion:/tmp/ came back as user: "8192" (host 10.0.4.12:2222 was otherwise right). Forward -l only for ssh. -F and scp/sftp -P → -p are the ones that should pass through.
| if not candidates and shell_state.get('dir'): | ||
| candidates.append(shell_state['dir']) | ||
| project = _project_for_paths(candidates, root_projects) | ||
| account = _destination_account(command, shell_state.get('dir'), shell_state.setdefault('accounts', {})) |
There was a problem hiding this comment.
This lookup sees the directory the shell ended in, not the directory git/ssh ran in. shell_state['dir'] was just replaced with _next_shell_dir, which follows the last cd in the whole command. Claude (build_llm_exchange after _project_for_tool_use) and Codex (no explicit workdir) do the same.
Two throwaway repos, origin acme/app and other/repo, starting in acme:
git push origin main && cd <other>→ accountotheron Copilot and Claude. The push wasacme.cd <other> && git pushhappens to beotheronly because thatcdis the last one.cd <other> && git push && cd /tmpwould resolve in/tmp.
_git_path_opt_targets already tracks per-segment cwd. The account lookup needs the cwd of the segment it resolved, not the cwd after the whole script. Cursor has the opposite bug: comment on cursor/unbound.py afterShellExecution.
| 'project': _project_for_paths(candidates, root_projects) | ||
| }) | ||
| } | ||
| account = _destination_account(command, event.get('cwd') or workspace_cwd, account_state) |
There was a problem hiding this comment.
Cursor passes the event cwd and never applies cd. Same two repos as the Copilot note, event cwd = the acme/app checkout:
cd <other> && git push → account acme, not other.
_destination_account itself does not track cd (the test uses git -C, which hides this). Copilot, Claude, and Codex go the other way and pass the directory after the last cd, so git push && cd <other> is attributed to other. Both should use the cwd of the segment that actually ran the push or ssh.
ReviewThe sync block, the allowlist, the timeout budget, and the per-hook exchange wiring are in good shape, and the parity test will keep the five copies honest. I would not ship this as-is. I ran the hook against throwaway repos and an ssh config; the inline comments have the commands and the accounts that came back. Sends a credential. Runs code the shell did not. Attributes the push to the wrong place.
Smaller miss, not commented inline: The tests cover the happy paths (named remote, token stripped from a git URL, |
… in the command's own folder Newlines now separate commands; git push skips -o/--push-option values and honours --repo; ssh-family passes -l, -p/-P, -o, -F and -J (separate or attached) to ssh -G; sftp takes a plain [user@]host; and each segment resolves in the folder it runs in, following cd from the command's starting directory. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
/review |
|
@cursoragent review |
|
I did a second pass on pull request #395 and left new comments. The first review still stands. A bare
|
vigneshsubbiah16
left a comment
vigneshsubbiah16
left a comment
There was a problem hiding this comment.
🛡️ Automated Security Review (raw — no orchestrator available)
Reviewers: Semgrep, Gitleaks
SEMGREP
- [MEDIUM]
/home/azureuser/pr-review/work/websentry-ai__setup-395/scan/augment/hooks/unbound.py:4369— These permissions0o700are widely permissive and grant access to more people than may be necessary. A good default is0o644which gives read and write access to yourself and read access to everyo (insecure-file-permissions) - [HIGH]
/home/azureuser/pr-review/work/websentry-ai__setup-395/scan/claude-code/hooks/unbound.py:1624— theencodingargument to Popen is only available on Python 3.6+ (python36-compatibility-Popen2) - [MEDIUM]
/home/azureuser/pr-review/work/websentry-ai__setup-395/scan/claude-code/hooks/unbound.py:5917— These permissions0o700are widely permissive and grant access to more people than may be necessary. A good default is0o644which gives read and write access to yourself and read access to everyo (insecure-file-permissions) - [MEDIUM]
/home/azureuser/pr-review/work/websentry-ai__setup-395/scan/codex/hooks/unbound.py:4702— These permissions0o700are widely permissive and grant access to more people than may be necessary. A good default is0o644which gives read and write access to yourself and read access to everyo (insecure-file-permissions) - [MEDIUM]
/home/azureuser/pr-review/work/websentry-ai__setup-395/scan/copilot/hooks/unbound.py:7470— These permissions0o700are widely permissive and grant access to more people than may be necessary. A good default is0o644which gives read and write access to yourself and read access to everyo (insecure-file-permissions) - [HIGH]
/home/azureuser/pr-review/work/websentry-ai__setup-395/scan/cursor/unbound.py:1092— Avoiding SQL string concatenation: untrusted input concatenated with raw SQL query can result in SQL Injection. In order to execute raw query safely, prepared statement should be used. SQLAlchemy prov (sqlalchemy-execute-raw-query) - [MEDIUM]
/home/azureuser/pr-review/work/websentry-ai__setup-395/scan/cursor/unbound.py:4527— These permissions0o700are widely permissive and grant access to more people than may be necessary. A good default is0o644which gives read and write access to yourself and read access to everyo (insecure-file-permissions)
GITLEAKS
(no secrets detected)
🤖 consensus review · reviewers: Semgrep, Gitleaks · head bd37ff57 · 2026-10-19T14:11Z
| destination, passthrough = _account_ssh_destination(program, args) | ||
| if not destination: | ||
| return None | ||
| out = _account_run(['ssh', '-G'] + passthrough + [destination], cwd, state) |
There was a problem hiding this comment.
P1 · Prevent command execution during SSH identity lookup (codex)
ssh -G evaluates Match exec directives in SSH configuration. Consequently, collecting an audit entry re-executes arbitrary configured shell commands, potentially repeating network requests or other side effects outside the agent's tool execution. The executable-prefix allowlist does not prevent this.
claude-code/hooks/unbound.py:5439 invokes ssh -G with configuration options forwarded at lines 5414–5415; lines 5221–5222 execute it directly. A read-only reproduction using ssh -G -F /dev/stdin with Match exec "echo ACCOUNT_LOOKUP_EXECUTED >&2" printed the marker, proving command execution during configuration lookup. The same resolver exists in all five hooks.
| if flag == '-C' and rest: | ||
| path = rest.pop(0) | ||
| git_dir = path if os.path.isabs(path) else os.path.join(git_dir or os.getcwd(), path) | ||
| elif flag in ('-c', '--git-dir', '--work-tree', '--namespace') and rest: |
There was a problem hiding this comment.
P1 · Honor Git configuration overrides during resolution (codex)
The resolver discards Git's -c configuration rather than applying it to lookups. For git -c remote.origin.url=https://github.com/personal/app.git push origin main, it reports the repository's stored origin owner instead of personal, recording the wrong destination account.
claude-code/hooks/unbound.py:5323–5324 discards the configuration argument; line 5348 constructs a lookup containing only git -C, and line 5357 reads the stored remote. A resolver reproduction with stored origin acme returned account acme for the command overriding origin to personal.
| continue | ||
| if arg.startswith('-') and len(arg) >= 2 and arg != '--': | ||
| flag, attached = arg[1], arg[2:] | ||
| takes_value = flag in _ACCOUNT_SSH_VALUE_FLAGS |
There was a problem hiding this comment.
P1 · Use program-specific SSH family option arities (codex)
The shared value-taking flag set treats scp's boolean -p and -O options as consuming the following argument. Valid downloads such as scp -p prod:/tmp/file . therefore lose their remote operand and receive no destination account.
claude-code/hooks/unbound.py:5207 includes p and O in _ACCOUNT_SSH_VALUE_FLAGS. Lines 5411–5416 consume the next argument for either flag. Directly evaluating _account_ssh_destination('scp', ['-p', 'prod:/tmp/file', '.']) returned (None, []), as did the -O form; lines 5436–5438 then omit attribution.
Second passThe findings from the first review still stand (token left in I re-ran the resolver against throwaway repos. New inline comments:
|
| lexer.whitespace_split = True | ||
| lexer.whitespace = ' \t\r' | ||
| segments, current = [], [] | ||
| for token in lexer: |
There was a problem hiding this comment.
A line continuation is split into two commands, so the remote on the next line is never part of the push.
git push \\\n personal main tokenizes as [['git', 'push'], ['personal', 'main']]. In a repo whose personal remote is jdoe-dev/app and whose origin is acme/app, that reported account acme, context origin.
Newline is in punctuation_chars, and a backslash-newline is a shell continuation, not a separator. Agents wrap long pushes this way. The same split turns git push \\\n+ personal main into a +personal remote and source: unknown.
Related, on the push side: git push +main is a force refspec, not a remote named +main. It came back source: unknown instead of the default remote. The colon form (HEAD:main) is already commented; +src has no colon so it misses that check and is used as the remote name.
| if not rest or rest[0] != 'push': | ||
| return None | ||
| target, positional, push_args, i = None, [], rest[1:], 0 | ||
| while i < len(push_args): |
There was a problem hiding this comment.
--git-dir is popped and ignored, and inline GIT_DIR is never read. The lookup then runs in the cwd it was given, or in the hook process's cwd when that is missing (git -C .).
Two repos, cwd = acme/app, the command pointed at other/repo:
GIT_DIR=<other>/.git git push→ accountacmegit --git-dir <other>/.git push→ accountacmegit --git-dir=<other>/.git push→ accountacme(the=form is not even recognized as--git-dir)
With no cwd at all, GIT_DIR=<repo>/.git git push resolved this checkout and returned account websentry-ai. A push that names its own git dir must not be reported as whatever repo the hook process happens to be standing in. Apply --git-dir / GIT_DIR to the lookup, and if there is still no directory, return unknown instead of git -C ..
vigneshsubbiah16
left a comment
vigneshsubbiah16
left a comment
There was a problem hiding this comment.
🛡️ Automated Security Review (consensus)
3 findings — 1 high-confidence, 2 to triage. Reviewers: claude (lead), semgrep, gitleaks.
-
[MEDIUM] Option injection into
ssh -Gbypasses theMatch execguard 🔴 —claude-code/hooks/unbound.py(_account_ssh/_account_remote_operand/_account_ssh_destination; same block in all five hooks)- What: A destination that starts with
-reachesssh -Gas an option, because no--is placed before it. Two routes get there:ssh -- -Fevil: the--branch adds-Fevilto the operands.scp file -Fevil:x(also sftp): any word after the first operand counts as an operand, and_account_remote_operandreturns-Fevilas the host.
The hook then runsssh -G -Fevil. That loads./evilfrom the shell call's cwd.
- Why:
_account_ssh_config_runs_commandsonly checks-Fvalues inpassthrough. It never sees this injected-F, so aMatch exec "<cmd>"in an agent-written file runs inside the hook process. That is outside the agent's tool sandbox and approval flow, and it happens at end of turn. It also happens for commands that were short-circuited, and possibly for commands that were never approved.-E<file>can be injected the same way, which makes ssh open or create an arbitrary file. This undoes the guard added for the earlierMatch execthread. - Fix: Never pass a destination that starts with
-; return{source: unknown}instead. Also build the call as['ssh', '-G', *passthrough, '--', destination]. Apply both in all five copies. - Flagged by: claude
- What: A destination that starts with
-
[LOW] Config guard can check different files than
ssh -Greads 🟡 —claude-code/hooks/unbound.py(_account_ssh_config_runs_commandsvs_account_run)- What: For a relative
-F, the guard checksos.path.join(cwd, config)and treats a missing file as safe. But_account_runpassescwd=Nonewhenevercwdis not an existing directory, for example aftercd /nonexistent. In that casessh -Gresolves the relative-Fagainst the hook process's own cwd, which is a file the guard never checked. - Why: A
Match execin that other file runs during the lookup. The same gap exists when thesshon PATH uses a different system config than the hardcoded/etc/ssh/ssh_config.⚠️ Homebrew OpenSSH, for example, reads/opt/homebrew/etc/ssh/ssh_config. That file is admin-owned, so the risk there is lower. - Fix: Skip the lookup (return unknown) when a relative
-Fis given andcwdis not an existing directory. Give the guard and the subprocess the same resolved absolute config path. - Flagged by: claude
- What: For a relative
-
[LOW]
ssh user:password@hostsends the password indestination_account.user🟡 —claude-code/hooks/unbound.py(_account_ssh)- What: For
ssh, the first operand goes tossh -Gunchanged. ssh splits on the last@, sossh -G alice:S3cret@hostreportsuser alice:S3cret. The hook copies that intouserand sends it to the control plane. - Why: This conflicts with the PR's claim that only identity fields leave the machine and that logins are stripped. scp/sftp operands and git/gh URLs are cleaned already; this path is not.
⚠️ It needs an unusual command, which is why the severity is low. - Fix: Cut the user part at the first
:(or dropuserwhen it contains:) before emitting it. Or reject destinations whose user part contains:. - Flagged by: claude
- What: For
🤖 consensus review · reviewers: Claude, Semgrep, Gitleaks · head 8d1d9ff9 · 2026-10-21T01:03Z
| if re.match(r'^https?://', url.strip()): | ||
| helper = _account_run(base + ['config', '--get', f'credential.https://{host}.helper'], git_dir, state) or \ | ||
| _account_run(base + ['config', '--get', 'credential.helper'], git_dir, state) or '' | ||
| if 'auth git-credential' in helper: |
There was a problem hiding this comment.
P1 · Do not infer helpers override embedded credentials (codex)
For an HTTPS remote containing complete credentials, Git uses those credentials without consulting its configured credential helper. If that repository also configures gh as its helper, this resolver nevertheless reports the stored gh login. A push authenticated by Alice's embedded token is therefore attributed to Bob when Bob is the active gh user.
claude-code/hooks/unbound.py:5579 reads the remote URL; :5587-5591 assigns the gh login solely from the configured helper without checking embedded credentials. A local git credential fill check with complete Alice credentials and a helper returning Bob preserved Alice's credentials. The existing helper test at tests/test_destination_account.py:106 covers only a URL without credentials. This resol
vigneshsubbiah16
left a comment
vigneshsubbiah16
left a comment
There was a problem hiding this comment.
✅ Security consensus: no issues found. (reviewers: claude, semgrep, gitleaks)
🤖 consensus review · reviewers: Claude, Semgrep, Gitleaks · head 8d1d9ff9 · 2026-10-21T01:05Z
|
Reviewed These match the program that runs:
Two shell forms still run and come back with nothing. bash records A drive letter is the wrong host on this scp. Detail on |
| # A bracketed host may hold an IPv6 address; a second colon after the host marks an rsync daemon. | ||
| match = re.match(r'^((?:[^@/:\[\]]+@)?(?:\[[^\]/]+\]|[^/:\[\]]+)):(:)?', arg) | ||
| # A drive letter (C:\x or C:/x) is a local path, not a host. | ||
| if match and not re.match(r'^[A-Za-z]:[\\/]', arg): |
There was a problem hiding this comment.
This scp dials the drive letter. The resolver treats C:/... as a local path and keeps the other operand.
scp -F cfg prod-bastion:/tmp/file C:/Users/me/out is host 10.0.4.12:2222, user deploy, context prod-bastion.
OpenSSH 9.6 scp on this machine does not copy from prod-bastion. The debug line is Executing: program /usr/bin/ssh host C, and the proxy command prints HOST=c PORT=22 USER=ubuntu.
scp /etc/hostname C:/Users/me/out and scp C:/a D:/b come back with no account. The same scp dials host C for the first and host D for the second. C:\Users\... is a local path here, and that form still resolves to prod-bastion.
… api URLs, and read drive letters as scp does - a destination starting with - is never passed to ssh -G, where it would be an option - unset hides an inherited variable; GH_REPO only applies to repo commands - gh api with a full URL reads its host and repository - a remote that carries its own password never reports the gh login - C:\x is local everywhere and C:/x is host C except on Windows, as OpenSSH reads them - the slash-remote test commits with its own identity, so it runs where git has none Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
/review |
|
@cursoragent review |
|
Reviewed
Three holes in this commit:
|
|
Reviewed These match:
Three holes in this commit: a backslash drive is still the other host ( |
| return None, None | ||
| # A bracketed host may hold an IPv6 address; a second colon after the host marks an rsync daemon. | ||
| match = re.match(r'^((?:[^@/:\[\]]+@)?(?:\[[^\]/]+\]|[^/:\[\]]+)):(:)?', arg) | ||
| # A drive letter is local in C:\x form, and in C:/x form too on Windows; elsewhere scp dials host C. |
There was a problem hiding this comment.
A backslash drive is still a host on this scp. The exclusion only applies after this lexer has kept the backslash, and scp dials host C for that word too.
scp -F cfg prod-bastion:/tmp/file C:\\Users\\me and the quoted form 'C:\Users\me' are host 10.0.4.12:2222, user deploy, context prod-bastion.
The argument scp receives is C:\Users\me. OpenSSH 9.6 prints Executing: program /usr/bin/ssh host C and HOST=c PORT=22 USER=ubuntu.
C:/Users/me/out is host c, which matches this scp. An unquoted C:\Users in the command text loses the backslash in the lexer and is also host c.
| # GH_REPO only stands in for a command that works on a repository. | ||
| repo = repo or ('' if cwd is None and not checkout_bound else _account_env(env, 'GH_REPO')) | ||
| # A host never carries a login, so anything before an @ is dropped. | ||
| env_host = _account_env(env, 'GH_HOST').lower().rsplit('@', 1)[-1] |
There was a problem hiding this comment.
env -u removes the variable for the command, and this still reads the hook's environment.
With GH_HOST=ghe.example.com on the hook, unset GH_HOST; gh repo view acme/app is host github.com. env -u GH_HOST gh repo view acme/app is host ghe.example.com, account acme. env -u is a wrapper flag here: the name is consumed and not stored, so _account_env falls through to os.environ.
env -u GH_HOST bash -c 'echo $GH_HOST' prints nothing. gh then has no GH_HOST, and acme/app is github.com.
| named = re.match(r'^https?://([^/]+)/(?:api/v3/)?repos/([^/{}@]+/[^/{}@]+)', url) or next( | ||
| (m for m in (re.match(r'^/?repos/([^/{}@]+/[^/{}@]+)', a) for a in positional[1:]) if m), None) | ||
| if named and named.lastindex == 2: | ||
| repo = f'{named.group(1)}/{named.group(2)}' |
There was a problem hiding this comment.
The host and the repo are joined with a colon, and the next split reads that colon as an scp URL. The port becomes the owner.
gh api https://ghe.example.com:8443/api/v3/repos/acme/app is host ghe.example.com, account 8443.
The URL's owner is acme. The same shape without a port, https://ghe.example.com/api/v3/repos/acme/app, is account acme. https://api.github.com/repos/acme/app is host api.github.com, account acme.
vigneshsubbiah16
left a comment
vigneshsubbiah16
left a comment
There was a problem hiding this comment.
🛡️ Automated Security Review (consensus)
2 findings: 0 high-confidence, 2 to triage. Reviewers: claude (lead), semgrep, gitleaks.
-
[MEDIUM] The
Match execpre-scan can read a different file thanssh -Greads 🟡:claude-code/hooks/unbound.py(_account_ssh_config_runs_commands, same in all 5 hooks)- What: The new guard scans the ssh config files for
Match exec(a config rule that runs a shell command) before runningssh -G. In some cases it scans a different file than the one ssh actually loads:- The guard finds the default config with
os.path.expanduser('~/.ssh'), which follows$HOME. OpenSSH uses the home directory from the password database and ignores$HOME. - A relative
-F cfgis scanned atcwd/cfg. But_account_runfalls back to the hook process's own folder whencwdis not a real directory, so ssh openscfgthere instead. The guard treats the missingcwd/cfgas "no file" and letsssh -Grun. ⚠️ Include paths are expanded only withexpanduserandglob. If the installed OpenSSH also expands environment variables or%tokens inInclude, the included file is never scanned. This depends on the OpenSSH version and is unverified.
- The guard finds the default config with
- Why: Each case brings back the earlier P1: building telemetry runs commands from the ssh config again, outside the agent's own tool call. That covers segments the shell may never have run (only literal
true/falseshort-circuits are modeled). It also contradicts the PR's claim that only "five local commands" ever run. - Fix:
- Find the home directory with
pwd.getpwuid(os.getuid()).pw_dirinstead of$HOME. - Pass
cwdtossh -Gonly when it is a directory. Otherwise returnunknownwhenever-Fis relative. - Treat any
Includecontaining$or%as unscannable and returnunknown.
- Find the home directory with
- Flagged by: claude
- What: The new guard scans the ssh config files for
-
[LOW]
ssh -Gcan still make DNS lookups 🟡:claude-code/hooks/unbound.py(_account_ssh, all 5 hooks)- What: The exec scan does not check
CanonicalizeHostname yes/alwaysorMatch canonical. With those set,ssh -Gresolves the destination over DNS while it parses the config. - Why: Every ssh, scp, sftp or rsync segment can send a DNS query for a host the agent named, even when that segment never ran. This contradicts the "no network call" guarantee in the PR and its SOC 2 note. The impact is small: one name lookup that can leak the host name, and no command runs.
- Fix: Add
-o CanonicalizeHostname=noto thessh -Gcall, since an explicit-osetting overrides the config file. Or returnunknownwhen the scan finds canonicalization turned on. Also correct the PR description. - Flagged by: claude
- What: The exec scan does not check
Pre-existing (not introduced by this PR)
- Semgrep
insecure-file-permissions(0o700) in augment, claude-code, codex, copilot and cursorunbound.py;python36-compatibility-Popen2atclaude-code/hooks/unbound.py:1624; andsqlalchemy-execute-raw-queryatcursor/unbound.py:1108. These lines are outside the new destination-account block.0o700is owner-only, which is the safe choice. None of these is raised against this PR.
🤖 consensus review · reviewers: Claude, Semgrep, Gitleaks · head 2c93717f · 2026-10-21T01:17Z
| cwd = path if os.path.isabs(path) else os.path.normpath(os.path.join(cwd or '', path)) | ||
| elif value is not None and name in ('-S', '--split-string') and wrapper == 'env': | ||
| argv[i:i] = [w for segment, _ in _account_segments(value) for w in segment] | ||
| continue |
There was a problem hiding this comment.
P1 · Honor environment removal in env wrappers (codex)
The wrapper parser consumes env's options without applying environment removal. With inherited GH_HOST=ghe.example.com, env -u GH_HOST gh repo view acme/app incorrectly reports ghe.example.com although gh uses github.com. env -i has the same problem. This affects all five identical resolver copies.
claude-code/hooks/unbound.py:5400-5429 consumes wrapper options but only applies chdir and split-string. At :5379-5381, _account_env falls back to os.environ for removed variables. A read-only resolver probe reproduced the incorrect enterprise host; the equivalent unset GH_HOST; gh repo view acme/app correctly returned github.com.
| destination, passthrough = _account_ssh_destination(program, args, env) | ||
| if not destination or destination.startswith('-'): | ||
| return None # a word starting with - would reach ssh -G as an option | ||
| if other_user and '-F' not in passthrough[::2]: |
There was a problem hiding this comment.
P1 · Preserve sudo uncertainty with explicit SSH configs (codex)
Supplying -F bypasses the other-user safeguard, but an explicit config need not specify User. For sudo -u nobody ssh -F none example.com, SSH defaults the remote login to nobody, while the hook runs ssh -G as its own user and reports that user's login. The same error affects scp and all five hook copies. Keep the user unresolved unless it can be established independently of the hook's local identity.
claude-code/hooks/unbound.py:5877-5882 permits other-user lookups whenever -F is present and executes the lookup without switching users; :5896 emits its user setting. A read-only probe of both sudo SSH and SCP commands returned user=azureuser for commands explicitly targeting local user nobody. tests/test_destination_account.py:530-547 only checks sudo uncertainty without -F.
| else: | ||
| word += ch | ||
| i += 1 | ||
| return word, i |
There was a problem hiding this comment.
P1 · Subshell (cd dir && git push ...) resolved in wrong repo (claude)
_account_segments does not treat (, ), { or } as grouping. Agents often write (cd frontend && git push origin main). That parses into the argv ['(cd', 'frontend'], so _account_program returns the program (cd, which is not cd, and the working directory never moves to frontend. The next segment is ['git', 'push', 'origin', 'main)'], and _account_git_push reads the target origin from the outer working directory's repository. The hook then reports that repo's origin host and owner with source: command, a confident wrong account. It should report the nested repo or unknown. The same bug in all five hooks and the Cursor copy.
claude-code/hooks/unbound.py _account_segments: the only special characters are quotes, \, #, <>, ;&|\n and blanks; ( falls into the else: word += ch branch. _account_program returns os.path.basename(token) = '(cd'. _destination_account changes the working directory only when program == 'cd', so seg_cwd for the git segment stays the outer folder. _account_git_push then ru
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.
There are 5 total unresolved issues (including 3 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 2c93717. Configure here.
|
|
||
| def _account_looks_like_url(value: str) -> bool: | ||
| """Whether a git remote or target names a URL rather than a remote.""" | ||
| return bool(re.search(r'://|^[^/:]+@[^/:]+:|^[^/:]+:[^/]', value)) |
There was a problem hiding this comment.
Refspec push reported as a git host
Medium Severity
_account_looks_like_url treats any name:path token as a remote URL. git push HEAD:main and git push +HEAD:refs/heads/main therefore become {platform: git, host: head, account: main, source: command} instead of the default remote or unknown.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 2c93717. Configure here.
…rg scope and url ports, and scan only ssh configs ssh would read Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
/review |
|
@cursoragent review |
|
Reviewed Subshells,
|
| if program == 'git': | ||
| account = _account_git_push(args, env, seg_cwd, state) | ||
| elif program == 'gh': | ||
| account = _account_gh(args, env, seg_cwd, state) |
There was a problem hiding this comment.
P1 · Preserve user uncertainty for GitHub CLI wrappers (codex)
The parser detects sudo/doas as another user, but only the SSH resolver receives that information. For sudo -u bob gh pr list -R acme/app, the hook queries its own gh configuration and reports its own login, such as alice, although the command uses bob's configuration and credentials. Return unknown or omit the login when the execution user differs. This affects all five copies.
claude-code/hooks/unbound.py:6011 captures other_user; lines 6039-6044 pass it only to _account_ssh. _account_gh_user at line 5496 runs gh through _account_run as the hook user. An in-memory probe with the local gh lookup returning alice confirmed that sudo -u bob gh pr list -R acme/app emits user alice.
| if argv is None: | ||
| # A subshell's cd and exports end with it. | ||
| if operator == '(': | ||
| subshells.append((cwd, dict(exported))) |
There was a problem hiding this comment.
P1 · Skip entire short-circuited subshells (codex)
Short-circuit skipping applies only to the first simple command inside a subshell. For false && (echo skipped; git push https://github.com/never/app main); git push https://github.com/actual/app main, the resolver skips echo but then resolves the unreachable first push. It reports account never instead of actual. The equivalent true/|| form also fails, and all five hooks share this behavior.
claude-code/hooks/unbound.py:5997-6003 saves only cwd and exports at subshell boundaries. Lines 6008-6010 skip individual segments without tracking whether their enclosing subshell executes. An in-memory probe confirmed both false/&& and true/|| examples return account never; tests/test_destination_account.py:596 covers only a skipped subshell containing a single command.
| if program == 'unset': | ||
| # Unset hides the inherited value too, so it is kept as empty rather than dropped. | ||
| exported.update((name, '') for name in args if not name.startswith('-')) | ||
| continue |
There was a problem hiding this comment.
P1 · Branch switch or gh set-default leaves earlier results stale (claude)
All lookups run at the end of the turn, so they read the repo's final state. Only git remote add|set-url|rename|remove and git config remote.*|url.*|branch.* mark earlier results as unknown. Two other in-turn changes alter what an earlier command resolves to, and neither is caught. (1) A bare git push resolved through @{push} or branch.<b>.pushRemote on a feature branch, followed later in the turn by git checkout main/git switch main: the end-of-turn rev-parse @{push} and symbolic-ref HEAD read main's push remote, so the push is confidently reported against the wrong remote and account. (2) gh pr create followed by gh repo set-default other-org/app: _account_gh_repo_remote reads the final gh-resolved and reports other-org for the earlier call. Both results carry source: command and are not flagged unknown.
_destination_account checks only _account_rewrites_remotes(args, ...) for git (claude-code/hooks/unbound.py ~6020-6027). _account_rewrites_remotes matches only remote add/set-url/rename/remove/rm and config writes to remote./url./branch. keys. It ignores checkout/switch, and gh commands never reach it. _account_default_remote resolves via `rev-parse --symbolic-full-name @{pus
vigneshsubbiah16
left a comment
vigneshsubbiah16
left a comment
There was a problem hiding this comment.
✅ Security consensus: no issues found. (reviewers: Claude (lead), Semgrep, Gitleaks)
🤖 consensus review · reviewers: Claude, Semgrep, Gitleaks · head 2925b05d · 2026-10-21T01:48Z
|
Reviewed These match the command that runs:
|
| if in_word: | ||
| argv.append(word) | ||
| word, in_word = '', False | ||
| elif ch in '()': |
There was a problem hiding this comment.
A ) always ends a subshell, and $(...) is only recognized outside quotes.
(cd frontend; case x in y) cd ..;; esac; git push origin main)issource: unknown. The)afterypops the subshell opened by(cd, so the push is resolved outsidefrontend. Bash keeps that)in the case pattern. A logginggitrecorded the push infrontend, ownerfront.echo "$(cd frontend && git push origin main)"has no account. Inside double quotes the parentheses stay part of the word, so this branch never sees the push. Bash runs it infrontend, ownerfront. The unquoted formecho $(cd frontend && git push origin main)isfront.
| cwd, exported = subshells.pop() | ||
| continue | ||
| while argv and argv[0] in _ACCOUNT_SHELL_KEYWORDS: | ||
| argv = argv[1:] |
There was a problem hiding this comment.
if, else, case and a function body are not conditional here. The keyword is removed and the body is resolved as a command that ran. & is a ;, so a background cd changes the folder of the next command.
From a checkout of acme/app, with frontend pointing at front/web and remote personal pointing at jdoe-dev/app:
if false; then git push personal main; else git push origin main; fiis accountjdoe-dev, sourcecommand. Bash runs the else branch. A logginggitrecorded that push in the outer repo, owneracme.if false; then cd frontend; fi; git push origin mainisfront. Bash leaves the shell in the outer repo. The push isacme.f() { cd frontend; }; git push origin mainisfront. Bash only defines the function. The push isacme.case x in y) cd frontend;; esac; git push origin mainisfront. The pattern does not match, so bash does notcd. The push isacme.cd frontend & git push origin mainisfront. Bash runscdin a background subshell. The push stays in the outer repo, owneracme.
The forms covered by the new tests match bash. (cd frontend && git push origin main) is front, and { cd frontend; git push origin main; } is front.
This branch has not been deployed
2 participants
Every shell call an agent makes now says whose account it reached.
git push personal mainreportsgithub.com/jdoe-devand the gh login doing the push.ssh prod-bastionreports the real host and user behind the alias. This is phase 1 of WEB-6171 (WEB-6295), for the five Python hooks: Claude Code, Codex, Cursor, Copilot and Augment.The backend half, which stores the field and links targets to it, is websentry-ai/ai-gateway-data#3157. That PR is safe to ship first. Until it lands, the new field is simply ignored.
What each call carries
destination_accounton the tool entrygit push personal main(remotehttps://jdoe:TOKEN@github.com/jdoe-dev/app){platform: github, host: github.com, account: jdoe-dev, user: jdoe, context: personal}. The login and token never leave the machinegit pushwith no remote@{push}, elseorigin)gh pr create -R acme/app{platform: github, host: github.com, account: acme, user: <gh login>}GH_HOST=ghe.acme.com GH_TOKEN=… gh repo view acme/appghe.acme.com, no user, because a token variable overrides the stored loginssh -F cfg prod-bastion/scp f prod-bastion:/tmp{platform: ssh, host: 10.0.4.12:2222, user: deploy, context: prod-bastion}git status,ls,rsync dir host::module{platform: …, source: unknown}How it stays safe
flowchart LR C[shell command] --> P[parse: inline VAR=x, sudo/env wrappers, ; && pipe] P --> M{git push / gh / ssh family?} M -- no --> N[nothing] M -- yes --> A[allowlisted local lookup, 5s each, 10s per turn] A --> F[identity fields only] A -- fails or slow --> U[account unknown]git remote get-url --push,git rev-parse @{push},git config --get,gh config get -h <host> userandssh -Gcan ever run. Anything else, such asgh auth token, is refused before it runs. All five are local, with no network call.projectlookup. Each has a 5-second cap, and the whole turn shares a 10-second budget.user:token@is stripped from remote URLs before anything is read from them.KEEP IN SYNC, and a parity test fails if the copies drift.Not in this PR (still part of WEB-6295)
hooks-ts) and the second Cursor copy incursor-pluginfollow in their own PRs. Phase 1 is done only when all six agents send the account.Tests
tests/test_destination_account.pyruns realgitandsshagainst throwaway repos and ssh configs:-R, positional repo, inlineGH_HOSTand a token override;gh auth token;Two guards were proven by breaking them and watching the tests fail: login stripping and the run allowlist.
Full suite: 3,028 passed. One failure,
test_copilot_home_contains_cli_transcripts, also fails on staging on this Mac, because it picks up the machine's real VS Code transcripts.🤖 Generated with Claude Code
Note
Medium Risk
Hooks run allowlisted git/gh/ssh subprocesses and emit identity metadata to the control plane; complex shell parsing can mis-attribute accounts despite fail-open safeguards.
Overview
Adds destination account metadata to shell/Bash tool entries in end-of-turn audit payloads across Augment, Claude Code, Codex, Copilot, and Cursor hooks (same ~870-line block, marked keep-in-sync).
For each shell command, the hooks parse the string (wrappers,
cd/export, pipelines) and, when the first relevant program isgit push,gh, or ssh/scp/sftp/rsync, resolve identity fields (platform,host,account,user,context,source) via allowlisted local subprocess calls only (gitconfig/remote,gh config get user,ssh -G), with per-command timeouts and a 10s per-turn budget. Unresolvable or risky cases returnsource: unknownor omit the field; remote URLs are stripped of embedded logins before parsing.destination_accountis attached alongside existing per-callprojectresolution, using the shell directory before the command updates cwd.Codex transcript parsing and Copilot’s shell tool mapper get the same field; git remote rewrites in the same turn can invalidate earlier push resolutions cached in turn state.
Reviewed by Cursor Bugbot for commit 2925b05. Bugbot is set up for automated code reviews on this repo. Configure here.
Unbound Review · Score 2/5 (3 P1s) ·
2925b05destination_accountfield to shell tool entries in the five Python hooks. The field comes from allowlisted local lookups (git config/remote/rev-parse,gh config get,ssh -G). The code fixes most earlier-round items: newline/heredoc parsing, subshell scoping,env -u, sftp plain hosts, scp/sftp URIs and redirections. One gap remains: lookups run at end of turn against the repo's final state, but stale results are invalidated onfindings: 0 P0 · 3 P1 · 0 P2 (top 5 P0/P1 inline) · engines: codex ✅ · claude ✅