Repository navigation
feat: resolve git, GitHub and SSH accounts in the Pi hook - #396
AakashVelusamy wants to merge 13 commits into
Conversation
Ports the Python hooks' destination-account block to hooks-ts: each bash or powershell call on the agent_end turn log carries the same destination_account (push remote host/owner with logins stripped and the gh login, gh host/owner, ssh-family host/user/port via ssh -G). Allowlisted local lookups only, 5s each under a 10s turn budget, resolved on the already-detached turn-log chain. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
/review |
|
@cursoragent review |
|
The Pi port matches the Python resolver, and the turn-log wiring is safe: lookups stay on the existing fire-and-forget path, so Blocked commands still run
|
vigneshsubbiah16
left a comment
There was a problem hiding this comment.
🛡️ Automated Security Review (raw — no orchestrator available)
Reviewers: Semgrep, Gitleaks
SEMGREP
(no findings)
GITLEAKS
(no secrets detected)
🤖 consensus review · reviewers: Semgrep, Gitleaks · head 44124174 · 2026-10-19T14:18Z
…s out of gh repos - deny and unavailable calls never ran, so no lookup runs for them - ssh -G is skipped when the config it would read has a Match exec - a relative ssh -F is resolved against the folder the command ran in - a login or token in a gh repo URL is dropped before the host is read - line continuations and comments, scp switches, gh api paths and a failed earlier lookup are read the same way as the Python hooks Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
/review |
|
@cursoragent review |
|
The account field is wrong for ordinary Blocked commands still run
|
vigneshsubbiah16
left a comment
There was a problem hiding this comment.
🛡️ Automated Security Review (raw — no orchestrator available)
Reviewers: Semgrep, Gitleaks
SEMGREP
(no findings)
GITLEAKS
(no secrets detected)
🤖 consensus review · reviewers: Semgrep, Gitleaks · head 0c5c305b · 2026-10-19T14:42Z
…figs that can run commands Same changes as the Python hooks: options stop at the ssh destination, option clusters are walked, only identity -o settings pass, rsync's remote shell is parsed, the Match exec scan follows Include and reads only small regular files, git overrides are unknown, and gh commands that reach no repo report nothing. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
/review |
|
@cursoragent review |
|
The fixes in
|
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-396/scan/hooks-ts/packages/core/src/destinationAccount.ts:395— RegExp() called with abasefunction argument, this might allow an attacker to cause a Regular Expression Denial-of-Service (ReDoS) within your application as RegExP blocks the main thread. For this (detect-non-literal-regexp) - [MEDIUM]
/home/azureuser/pr-review/work/websentry-ai__setup-396/scan/pi/index.js:2174— RegExp() called with abasefunction argument, this might allow an attacker to cause a Regular Expression Denial-of-Service (ReDoS) within your application as RegExP blocks the main thread. For this (detect-non-literal-regexp)
GITLEAKS
(no secrets detected)
🤖 consensus review · reviewers: Semgrep, Gitleaks · head 05235c96 · 2026-10-19T15:32Z
Matches the Python hooks. Include patterns are matched without building a regular expression, and a ~user include is refused since only ssh can resolve it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
/review |
|
@cursoragent review |
|
The quoted-include and bare-push fixes in
|
vigneshsubbiah16
left a comment
There was a problem hiding this comment.
🛡️ Automated Security Review (raw — no orchestrator available)
Reviewers: Semgrep, Gitleaks
SEMGREP
(no findings)
GITLEAKS
(no secrets detected)
🤖 consensus review · reviewers: Semgrep, Gitleaks · head 2d9946c0 · 2026-10-19T16:13Z
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
/review |
|
@cursoragent review |
|
The home-folder A quoted
|
vigneshsubbiah16
left a comment
There was a problem hiding this comment.
🛡️ Automated Security Review (raw — no orchestrator available)
Reviewers: Semgrep, Gitleaks
SEMGREP
(no findings)
GITLEAKS
(no secrets detected)
🤖 consensus review · reviewers: Semgrep, Gitleaks · head e4a768eb · 2026-10-19T17:51Z
Review disposition: every finding, every reviewer (head
|
| Finding | Reviewer | Disposition |
|---|---|---|
Blocked (deny, unavailable) calls still run ssh -G |
Cursor | Fixed in 0c5c305b: calls that never ran are skipped |
A declined confirm still resolves |
Cursor | Accepted limitation: the record keeps the policy's confirm, not the person's answer. The ssh-config scan below keeps a lookup from running config commands |
A relative ssh -F resolves in the temp folder |
Cursor, codex | Fixed in 0c5c305b |
ssh -F ~/config is always unknown |
Cursor | Fixed in e4a768eb |
RegExp built from config text (Semgrep, ReDoS) |
Consensus | Fixed in 2d9946c0: include patterns are matched without a regular expression |
Include ~user/... |
Cursor | Fixed in 2d9946c0: refused, since only ssh can resolve another user's home |
Same as the Python hooks
| Finding | Disposition |
|---|---|
A token in a gh repo URL reaches host |
Fixed in 0c5c305b |
scp/sftp read with ssh's flag table; -l taken as the login |
Fixed in 0c5c305b |
Options after the ssh destination; option clusters such as -vF |
Fixed in 05235c96 |
| rsync's remote shell and option values | Fixed in 05235c96 |
Match exec in included, system or quoted-include configs |
Fixed in 05235c96 and 2d9946c0 |
A bare push falls back to origin |
Fixed in 2d9946c0: pushRemote, then pushDefault, then the branch's remote, then origin |
git -c, --git-dir, GIT_DIR |
Fixed in 05235c96: reported unknown |
gh help / gh auth claim the local repo |
Fixed in 05235c96 |
sudo -u / env -u hide the command |
Accepted limitation: rare in agent commands, and the result is no account |
A quoted ssh keyword ("Match" exec) |
Accepted limitation: it takes a deliberately crafted config, and the scan is defence in depth |
IPv6 copy destinations (user@[2001:db8::1]:/x) |
Accepted limitation: rare, and the result is no account, not a wrong one |
Security consensus (Semgrep, Gitleaks)
The only Semgrep finding was the RegExp one above, now fixed. Gitleaks was clean.
|
@cursoragent review |
|
The option-order and unquoted-escape fixes on Options after the hostname. OpenSSH 9.6 applies Unquoted and double-quoted includes. Single quotes keep the backslash. OpenSSH still expands } else if (ch === "\\" && quote !== "'") {
escaped = true; // ssh, like a shell, takes the next character literallyFor Explicit |
vigneshsubbiah16
left a comment
There was a problem hiding this comment.
🛡️ Automated Security Review (raw — no orchestrator available)
Reviewers: Semgrep, Gitleaks
SEMGREP
(no findings)
GITLEAKS
(no secrets detected)
🤖 consensus review · reviewers: Semgrep, Gitleaks · head 1357bb02 · 2026-10-19T20:24Z
| overridden = true; | ||
| } else if (flag === "--namespace" && rest.length > 0) { | ||
| rest.shift(); | ||
| } |
There was a problem hiding this comment.
Attached git -C uses wrong repo
Medium Severity
The git global-option loop only honours a separate -C argument. Git accepts the attached form, so that directory is ignored and remotes are read from the original cwd. A push that actually ran elsewhere can be reported as source: "command" for the wrong host and owner.
Reviewed by Cursor Bugbot for commit 1357bb0. Configure here.
commented
Oct 19, 2026
Review disposition addendum (head
|
| Finding | Disposition |
|---|---|
A quoted ssh keyword ("Match" exec, "Include" "…") slips past the command scan |
Fixed: config lines are split the way ssh splits them |
Bracketed IPv6 copy targets (user@[2001:db8::1]:/x) are read as an rsync daemon, or cut short |
Fixed |
A token survives in a remote shaped https://x@TOKEN@host/… or user:TOKEN@host:owner/repo |
Fixed: everything up to the last @ before the path is dropped, and a host or owner that still holds @ is never kept |
The same, for gh repos (-R, --repo=, GH_REPO, gh api repos/…) |
Fixed: gh repos go through the same strip |
A login in GH_HOST or --hostname |
Fixed: only the part after the last @ is kept |
GIT_CONFIG_GLOBAL / GIT_CONFIG_SYSTEM |
Fixed: the push is reported unknown, like -c |
gh --hostname=, -R<repo>, -R=<repo> |
Fixed |
Match =exec, Include =path |
Fixed: a leading = is only a separator |
With no repo named, gh reported origin's push URL |
Fixed: it now uses origin's fetch URL, which is what gh queries |
ssh options right after the destination (ssh host -p 2200 -l deploy) |
Fixed: options are read up to the first command word, as OpenSSH does |
scp:// / sftp:// URIs were reported as a host named after the scheme |
Fixed: left unresolved, so no account rather than a wrong one |
scp:// / sftp:// URIs should resolve rather than stay unknown |
Accepted limitation: rare, and unknown is safe |
sudo -u user … / env -u VAR … hide the command |
Accepted limitation: rare in agent commands, and the result is no account |
gh repo set-default, or a URL argument such as gh pr view <url> |
Accepted limitation: rare. Reported from origin's fetch URL |
An explicit push URL bypasses pushInsteadOf |
Accepted limitation: rare |
A gh option value that looks like -R… (e.g. --title '-Rpersonal/app') |
Accepted limitation: contrived |
| A backslash escape inside a single-quoted include path | Accepted limitation: it takes a deliberately crafted config. The scan is defence in depth: commands are resolved only after they ran, and blocked calls are skipped |
gh api repos/<login>@host/owner/… names the wrong owner |
Accepted: contrived, and the token is never kept |
Security consensus: clean on 1357bb02. Score: 2/5 from Codex alone. Its open P1s are the accepted limitations above.
left a comment
There was a problem hiding this comment.
🛡️ Automated Security Review (consensus)
5 findings — 3 high-confidence, 2 to triage. Reviewers: Cursor, Claude, Semgrep, Gitleaks.
Findings
🔴 HIGH — ssh -G lookup treats the destination as more OpenSSH flags (hooks-ts/packages/core/src/destinationAccount.ts:542)
- Impact: A destination starting with
-(e.g. fromssh -- -F/tmp/evil host) is appended without--, sossh -Gmay load an agent-controlled config;configRunsCommandsnever scans that-F, andMatch execin the config can run during enrichment even though the real command would not have used that config. - Fix: Invoke
["ssh", "-G", ...passthrough, "--", dest]; reject destinations (and scp/sftp operands) that start with-; add a regression test withssh -- -F<exec-config> xand assert no side effects. - Reviewers: Claude, Cursor
🔴 HIGH — Policy-blocked shell calls with decision: "confirm" still run lookups (hooks-ts/packages/core/src/destinationAccount.ts:61)
- Impact:
NEVER_RANonly skipsdenyandunavailable; declined or headless-blocked confirms keepconfirm, sogit/gh/ssh -Gstill run on command text the tool never executed—amplifying thessh -Ginjection path above. - Fix: Resolve accounts only for calls known to have run (e.g. allowlist
allow/ post-confirm outcomes), not a denylist of non-run verdicts. - Reviewers: Claude, Cursor
🔴 HIGH — expandHome resolves ~/… to paths under filesystem root (hooks-ts/packages/core/src/destinationAccount.ts:384)
- Impact:
join(homedir(), path.slice(1))drops the home prefix whenslice(1)is absolute (/…), socd ~/…,git -C ~/…,ssh -F ~/…, andInclude ~/.ssh/…scans miss real files;configRunsCommandscan clear a config that still loads viassh -G, allowingMatch execduring lookup. - Fix: Expand
~/withjoin(homedir(), path.slice(2))(and handle~alone); add tests forInclude ~/.ssh/config.d/*withMatch exec. - Reviewers: Cursor
🟡 TRIAGE — Parser can report the wrong destination account (hooks-ts/packages/core/src/destinationAccount.ts:155, 254, 286)
- Impact: Agent-controlled command text can make
destination_accountname a different org/host than the tool actually used (ghdoes not skip other options’ values; explicitgit pushURLs skipinsteadOf/pushInsteadOf;sudo -u/envwrappers hide the real program), weakening audit and policy that trust this field. - Fix: Consume value-taking flags when scanning
gh; resolve explicit push URLs viagit ls-remote --get-urlor emitunknown; extend wrapper peeling for value-taking wrapper flags; preferunknownover confidentsource: "command"when parsing is ambiguous. - Reviewers: Claude, Cursor (Unbound/codex P1s on git/gh parsing)
🟡 TRIAGE — destination_account is not passed through redactSecrets (hooks-ts/packages/core/src/turnLog.ts:320)
- Impact:
tool_input/tool_responseare redacted withapiKey;host,account,context, anduserare attached raw—unusual tokens in-R,GH_REPO, remotes, orGH_HOSTcould reach the control plane ifstripLogin/withoutLoginmiss a shape. - Fix: Run each string field of
destination_accountthrough the sameredactSecrets(…, opts.apiKey)before attaching. - Reviewers: Claude
Semgrep: no findings. Gitleaks: no secrets detected.
🤖 consensus review · reviewers: Cursor, Claude, Semgrep, Gitleaks · head 1357bb02 · 2026-10-20T17:17Z
…e person declined Ports the Python hooks' changes: wrapper option values, exports, literal short circuits, gh's default repo and URLs, git URL rewrites, scp:// URIs, RSYNC_RSH, remotes rewritten later in the turn, and ssh config lines split as OpenSSH splits them. A confirm the person declines now marks its recorded call as never run, and a !cmd posted before its confirm is answered is treated the same way, so no lookup runs for either. The flag is internal and never sent. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
left a comment
There was a problem hiding this comment.
🛡️ Automated Security Review (consensus)
5 findings — 2 high-confidence, 3 to triage. Reviewers: Cursor, Claude, Semgrep, Gitleaks.
-
[HIGH]
ssh -Gdestination parsed as extra OpenSSH options 🔴 —hooks-ts/packages/core/src/destinationAccount.ts:448- What: The enrichment path invokes
ssh -Gwith...passthrough, destinationand no--, so a destination token beginning with-is interpreted as ssh flags (e.g.-F,-E). - Why:
configRunsCommandsonly inspects-Fvalues already inpassthrough. Paths such asssh -- -F/tmp/evilorscp x '-Fevil:y'can makessh -Gload an attacker-controlled config under the project cwd while the guard still scans the default tree;Match execin that file can run during turn-log enrichment (policy bypass outside the visible bash command). - Fix: Insert
--before the destination (["ssh", "-G", ...passthrough, "--", dest]); treat destinations starting with-asunknown; add regression tests (ssh -- -F<evil>,scp … '-F<evil>:path'). - Flagged by: Claude, Cursor
- What: The enrichment path invokes
-
[HIGH]
expandHomebreaks~/…paths and weakens ssh config scanning 🔴 —hooks-ts/packages/core/src/destinationAccount.ts:384- What: For
~/foo,path.slice(1)is/foo;path.join(homedir(), "/foo")resolves to/foo, not$HOME/foo. - Why:
cd ~/repo,git -C ~/repo, andInclude ~/.ssh/config.d/*resolve to filesystem-root paths, soconfigRunsCommandsmay miss realMatch exec/Includecontent whilessh -Gstill loads it—same class of guard bypass as above, plus wrong git/gh cwd attribution. - Fix: Expand with
path.join(homedir(), path.slice(2))for~/…(and handle~alone); mirror OpenSSH tilde rules where configs are involved; extend tests beyond theHOME=override case. - Flagged by: Cursor
- What: For
-
[MEDIUM] Shell redirections are not stripped from argv 🟡 —
hooks-ts/packages/core/src/destinationAccount.ts:130- What:
segments()does not treat2>&1,>file, etc. as redirections; tokens like2>can become git remotes or split the command across segments. - Why: Common forms (
git push 2>&1) can yield confident but wrongdestination_account(compliance/telemetry mis-attribution), not secret leakage. - Fix: Strip or skip redirection operators when tokenizing (or delegate to a small shell-metacharacter pass) before
gitPush/ghparsing; add tests forgit push origin main 2>&1. - Flagged by: Claude (Unbound/codex P1; PR author claims parity with Python—unverified here)
- What:
-
[MEDIUM]
confirmpolicy outcomes can still trigger lookups 🟡 —hooks-ts/packages/core/src/destinationAccount.ts:581- What:
NEVER_RANis onlydenyandunavailable; a recordeddecision: "confirm"withranunset still entersresolveDestinationAccountsuntilmarkNotRun/ran: falseis applied. - Why: Declined or headless-blocked confirms that never executed the shell command may still spawn allowlisted
git/gh/ssh -Gon the telemetry path—the surface this PR was meant to keep off blocked calls. - Fix: Skip when
decision === "confirm"unless the call is known to have run (or addconfirmto the never-ran set once execution is proven); ensure every non-run confirm path setsran: falsebeforeagent_endposting. - Flagged by: Cursor
- What:
-
[LOW] Sync ssh-config read can block on FIFO swap 🟡 —
hooks-ts/packages/core/src/destinationAccount.ts:463- What:
statSyncthenreadFileSyncon the same path allows replacing a regular file with a FIFO between checks. - Why: A blocking read stalls the Pi event loop; subprocess timeouts do not bound synchronous
readFileSync, so a writable config directory could be used for denial of service on enrichment. - Fix: Open with
O_RDONLY | O_NONBLOCK,fstatthe fd, verify regular file and size cap, then read from that handle. - Flagged by: Claude
- What:
🤖 consensus review · reviewers: Cursor, Claude, Semgrep, Gitleaks · head 4ab7393d · 2026-10-21T00:42Z
commented
Oct 21, 2026
|
/review |
commented
Oct 21, 2026
|
@cursoragent review |
|
The rare-shape fixes on These now match the command that actually ran
What still disagrees
const url = positional.slice(1).map((a) => /^https?:\/\/([^/@]+\/[^/@]+\/[^/@]+)/.exec(a)).find((m) => m !== null);
if (url) repo = url[1] as string;
|
left a comment
There was a problem hiding this comment.
🛡️ Automated Security Review (consensus)
2 findings: 1 high-confidence, 1 to triage. Reviewers: Claude (lead), Semgrep, Gitleaks.
-
[MEDIUM]
ssh -Gdestination can inject options and bypass the Match-exec guard 🔴 —hooks-ts/packages/core/src/destinationAccount.ts:772- What:
ssh()passes the parsed destination straight torun(["ssh", "-G", ...passthrough, destination])with no--before it. A destination that starts with-is read by OpenSSH as another option. - Why:
sshDestinationsends every word after--tooperands. So the agent commandssh -- -F/tmp/evilgivesdestination = "-F/tmp/evil"and an emptypassthrough.configRunsCommandsthen sees no-F. It scans only~/.ssh/configand/etc/ssh/ssh_config, and passes.- The hook then runs
ssh -G -F/tmp/evil, which loads that file and runs anyMatch execcommand in it. - That command runs as the user, in the background turn-log path. This is exactly the code execution the guard exists to prevent.
- scp, sftp and rsync have the same gap. After their first operand, a word like
-oX=y:zcan reachremoteOperandand come back as the destination.
- Fix: Build the lookup as
["ssh", "-G", ...passthrough, "--", destination]. Also refuse any destination that starts with-, and return{platform: "ssh", source: "unknown"}for it. Add a test usingssh -- -F<config with Match exec>that checks the marker file is never created. - Flagged by: Claude. It was also raised earlier by @cursor[bot]. That is not a maintainer decision, and the current diff still has no
--.
- What:
-
[LOW] Match-exec scan reads
$HOME's ssh config, but ssh reads the passwd home 🟡 —hooks-ts/packages/core/src/destinationAccount.ts:734- What:
configRunsCommandsscansjoin(homedir(), ".ssh/config"). Node'shomedir()uses$HOME. OpenSSH finds~/.ssh/configthroughgetpwuid()->pw_dir, the home folder in the system's user database. - Why: Pi can run with
HOMEset to a different folder, for example in a sandbox, a container or a test harness. Then the guard scans the wrong file.ssh -Gstill reads the real~/.ssh/configand runs anyMatch execin it. - Fix: Choose one:
- Pass
-Fwith the exact scanned path explicitly tossh -G, so the scanned file and the loaded file are always the same. - Return unknown when
os.userInfo().homedir !== homedir().
- Pass
- Flagged by: Claude. This matches an earlier Unbound Review P2.
- What:
🤖 consensus review · reviewers: Claude, Semgrep, Gitleaks · head 4ab7393d · 2026-10-21T00:46Z
… in its own folder, and close the remaining parser gaps Matches the Python hooks: redirection operators and targets are not arguments, heredoc bodies are input, gh commands outside the checkout name no repo, a gh URL with a login is read, scp:// hosts must end at : or /, drive letters are local, ssh under sudo without -F is unknown, and git config set|unset counts as a rewrite. A typed !cmd now resolves in the event's cwd. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
commented
Oct 21, 2026
|
/review |
commented
Oct 21, 2026
|
@cursoragent review |
|
The fixes on What matches the command
What still disagrees
while (rest.length > 0 && (rest[0] as string).startsWith("-")) {
const flag = rest.shift() as string;
if (flag === "-C" && rest.length > 0) {
const path = expandHome(rest.shift() as string);
gitDir = isAbsolute(path) ? path : gitDir === undefined ? undefined : join(gitDir, path);
|
left a comment
There was a problem hiding this comment.
🛡️ Automated Security Review (consensus)
5 findings — 0 high-confidence, 5 to triage. Reviewers: claude (lead), semgrep, gitleaks.
-
[MEDIUM] The ssh config scanner treats a mid-line
#differently from older OpenSSH, so a hiddenMatch execcan still run 🟡 —hooks-ts/packages/core/src/destinationAccount.ts:758- What:
configWordstreats any word that starts with#as the start of a comment. OpenSSH before 8.7 only treats#as a comment at the start of a keyword. So on a line likeMatch !host #x exec "cmd", the scanner reads onlyMatch !hostand calls it safe. Old ssh reads#xas a host pattern, the negated host check passes, and it runsexec "cmd". - Why:
ssh -Gthen runs a command from the config inside the hook's process. This breaks the PR's "only allowlisted lookups ever run" rule. Ubuntu 20.04 (OpenSSH 8.2) and RHEL 8 (OpenSSH 8.0) are still common. The effect grows if the hook runs with a different environment or sandbox than the agent's bash. - Fix: Make the check stricter instead of copying ssh's parser. Refuse the lookup when any
Matchline containsexecanywhere (case-insensitive), or contains a#after the keyword. An even safer rule is to refuse when any line in the config files containsexec. - Flagged by: claude
- What:
-
[LOW] Identity fields skip
redactSecrets, andssh user:pass@hostputs the password inuser🟡 —hooks-ts/packages/core/src/destinationAccount.ts:840,hooks-ts/packages/core/src/turnLog.ts:320- What: For
ssh, the raw destination goes straight tossh -G. ssh splits on the last@, sossh admin:S3cret@hostprintsuser admin:S3cret, and that value is posted asdestination_account.user. InbuildTurnLogBody,tool_inputgoes throughtoolInputFor/redactSecrets, butdestination_accountis attached as-is. - Why: A password typed into the destination reaches the control plane in a field the PR describes as "identity only, never secrets". This matters for the SOC 2 note in the PR description.
- Fix: Strip anything from
:onward in the user part of the ssh destination before callingssh -G(scp and sftp URIs already do this). Also rundestination_accountvalues throughredactSecrets(…, apiKey)inbuildTurnLogBody. - Flagged by: claude
- What: For
-
[LOW]
ssh -Glookup has no--before the destination 🟡 —hooks-ts/packages/core/src/destinationAccount.ts:822- What: The destination comes from the command text and can start with
-. For example,ssh -- -F/tmp/x, or a later scp operand such as-Fx:path. It is added after-Gand the passthrough options with no--separator. - Why: This is not exploitable today, because the destination is the only extra argument and ssh stops at "no host" before it reads any config. Still, it is one refactor away from letting a smuggled
-FskipconfigRunsCommands. - Fix: Refuse destinations that start with
-, or add"--"before the destination:["ssh", "-G", ...passthrough, "--", dest]. - Flagged by: claude (also raised earlier by Cursor Bugbot)
- What: The destination comes from the command text and can start with
-
[LOW] File check race: a file swapped for a fifo can block Pi's event loop 🟡 —
hooks-ts/packages/core/src/destinationAccount.ts:790- What:
statSyncchecks for a small regular file, thenreadFileSync(path)opens the file again by name. - Why: A background job the agent started can swap the config or Include target for a fifo between those two calls. The synchronous read then blocks the whole Node event loop, not just the fire-and-forget chain. This breaks the "never blocks pi" promise.
- Fix: Open the file once with
O_RDONLY | O_NONBLOCK,fstatSyncthe descriptor, and read through that descriptor with a byte cap. - Flagged by: claude
- What:
-
[LOW] Never-ran filter is fail-open for
confirm🟡 —hooks-ts/packages/core/src/destinationAccount.ts:96- What:
NEVER_RANonly skipsdenyandunavailable. Aconfirmcall is resolved unless something else marks itran: false. - Why: Any declined or blocked confirm path that misses
markNotRunstill triggers localgit/gh/ssh -Glookups for a command that never ran. Cursor Bugbot raised the headless no-UI case. Thedecide.tspart of the diff is cut off here, so it could not be checked. - Fix: Fail closed. Skip
confirmunless the call is clearly marked as having run (for exampleran: trueset on approval), instead of relying onran: falsebeing set on every decline path. - Flagged by: claude (also raised earlier by Cursor Bugbot)
- What:
🤖 consensus review · reviewers: Claude, Semgrep, Gitleaks · head bf16d677 · 2026-10-21T01:06Z
… api URLs, and read drive letters as scp does Matches the Python hooks. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
commented
Oct 21, 2026
|
/review |
commented
Oct 21, 2026
|
@cursoragent review |
|
The fixes on What matches the command
What still disagrees
if (name === "unset") {
// Unset hides the inherited value too, so it is kept as empty rather than dropped.
for (const arg of args) if (!arg.startsWith("-")) exported[arg] = "";
host = flagHost || host || envHost || "github.com";
// A drive letter is local in C:\x form, and in C:/x form too on Windows; elsewhere scp dials host C.
const drive = process.platform === "win32" ? /^[A-Za-z]:[\\/]/ : /^[A-Za-z]:\\/;
if (match !== null && !drive.test(arg)) return match[2] === undefined ? { destination: match[1] as string } : {};
|
left a comment
There was a problem hiding this comment.
🛡️ Automated Security Review (consensus)
1 finding: 0 high-confidence, 1 to triage. Reviewers: claude (lead), semgrep, gitleaks.
- [LOW] The
Match execguard checks$HOME, butsshreads the passwd home folder 🟡:hooks-ts/packages/core/src/destinationAccount.ts(configRunsCommands/expandHome)- What:
configRunsCommandsdecides which ssh config to scan withjoin(homedir(), ".ssh").includeTargetsexpands a~inIncludepaths the same way. Node'shomedir()reads$HOMEfirst. OpenSSH finds~/.ssh/configand expands~from the passwd entry (pw_dir), not from$HOME. - Why: Sometimes the hook runs with a
HOMEthat differs from the real account home. Examples are containers,sudo -E, CI runners, or a test harness that overridesHOME. Then the guard scans one file whilessh -Gloads another. AMatch execin the real~/.ssh/config, or in a file it includes, would be judged safe, andssh -Gwould run that command inside the hook during turn-log enrichment (the background lookups that adddestination_account). This is the exact thing the guard exists to stop. The impact is limited because the command comes from the user's own ssh config, not from the agent. That is why this is LOW. - Fix: Get the home folder from
os.userInfo().homedir, which reads passwd like ssh does, for both the default config path and~expansion inconfigRunsCommands/includeTargets. A simpler option: ifprocess.env.HOME !== os.userInfo().homedir, returntrue(treat the config as unsafe and skip the lookup). Keep$HOMEonly for~incd/git -Cpaths, where the shell's behaviour is what matters. - Flagged by: claude
- What:
Pre-existing (not introduced by this PR)
None.
Earlier bot comments on this PR come from [NONE] authors, so none of them were suppressed. I checked each against the current diff:
| Earlier comment | Status now |
|---|---|
ssh -G destination flag injection |
Fixed: a destination starting with - is refused. |
Quoted Match/Include keywords |
Fixed: configWords now parses quotes. |
Includes and /etc/ssh/ssh_config not scanned |
Fixed: both are now walked. |
| Blocked confirm still runs lookups | Fixed in this file: ran === false is now honoured. |
| Tilde resolves to the filesystem root | False positive: path.join does not drop the home prefix when the second part starts with /. |
The remaining earlier comments (clustered flags, wrapper flags, IPv6, gh URLs) are telemetry accuracy problems, not security issues.
🤖 consensus review · reviewers: Claude, Semgrep, Gitleaks · head d75786b9 · 2026-10-21T01:16Z
| for (const key of Object.keys(result)) delete (result as unknown as Record<string, unknown>)[key]; | ||
| Object.assign(result, { platform: "git", source: "unknown" }); | ||
| } | ||
| continue; |
There was a problem hiding this comment.
Rewrite leaves remotes cache stale
Medium Severity
A later git remote or git config rewrite in the same turn invalidates earlier pushResults but leaves state.remotes in place. A following gh call in that turn still reads the cached remotes and can attach the old owner as a confident command source after the checkout now points somewhere else.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit d75786b. 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>
commented
Oct 21, 2026
|
/review |
commented
Oct 21, 2026
|
@cursoragent review |
|
The subshell, What holds
while (argv.length > 0 && SHELL_KEYWORDS.has(argv[0] as string)) argv = argv.slice(1);
if (argv.length === 0) continue;
// A literal true or false decides whether && or || runs the next command; any other status is unknown.
if ((operator === "&&" && status === 1) || (operator === "||" && status === 0)) continue;Against a repo whose origin is
The new test covers
Default config and function expandHome(path: string, home = homedir()): string {
return path === "~" || path.startsWith("~/") ? join(home, path.slice(1)) : path;
} for (let i = 0; i + 1 < passthrough.length; i += 2) {
const value = expandHome(passthrough[i + 1] as string);
if (passthrough[i] === "-F") passthrough[i + 1] = value === "none" || isAbsolute(value) ? value : join(dir ?? "", value);With Still in the resolver, shorter
|
left a comment
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.
There are 4 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 f7a5975. Configure here.
| const targets = includeTargets(pattern, base); | ||
| if (targets === undefined) return true; | ||
| pending.push(...targets.map((target): [string, string] => [target, base])); | ||
| } |
There was a problem hiding this comment.
SSH config line-continuation bypasses exec scan
High Severity
configUnsafe splits an ssh config on raw newlines and never joins a line that ends with \, which OpenSSH treats as a continuation. A Match / exec or Include split across lines is missed, so ssh -G still runs and will execute that Match exec (or read an unscanned include). A project -F config or a user's continued Match exec therefore runs attacker- or user-controlled commands during turn-log enrichment.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit f7a5975. Configure here.
| args: string[], env: Record<string, string>, cwd: string | undefined, state: State, | ||
| ): Promise<{ account?: DestinationAccount; dir?: string }> { | ||
| let gitDir = cwd; | ||
| let overridden = GIT_OVERRIDE_ENV.some((k) => env[k]); |
There was a problem hiding this comment.
Git lookups ignore a cleaned environment
Medium Severity
gitPush treats override variables as present only when the command env value is truthy, and every git lookup still inherits the hook process environment. env -i, env -u GIT_DIR, or unset GIT_DIR (and the other GIT_OVERRIDE_ENV keys) therefore still read the process GIT_DIR / GIT_CONFIG_*, which can attribute the push to a different repository than the command reached.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit f7a5975. Configure here.




Pi now sends the same
destination_accountas the other five agents, so agit pushorsshrun by Pi says whose account it reached. This is the Pi half of WEB-6171 phase 1 (WEB-6295). The Python hooks are #395, and the backend is websentry-ai/ai-gateway-data#3157.What changes
hooks-ts/packages/core/src/destinationAccount.ts. It is a TypeScript port of the Python_destination_accountblock, with the same behaviour:-ooption values and honours--repo.-R,GH_REPO, a positionalrepoargument or the repo's remote;GH_HOST; the login is dropped underGH_TOKEN.ssh -G. It honours-l,-p/-P,-o,-Fand-J, whether given separately or attached, and plainsftp user@host.rsync host::modulereaches no account.; && || |separate commands, andcdchanges the folder the later segments run in.git remote get-url --push,git rev-parse @{push},git config --get,gh config get -h … userandssh -Gcan run. Each has a 5-second cap and the turn shares a 10-second budget. A failure yields{platform, source: "unknown"}.dispatchTurnLog.agent_endstill returns at once, and the post waits only for the lookups.TurnLogToolUsegains an optionaldestination_account, attached bytool_use_id.pi/index.jsandpi/index.js.sha256are rebuilt from source.Before → after (Pi turn log,
tool_use[])git push personal main{tool_name: bash, tool_input: {command}}…, destination_account: {platform: github, host: github.com, account: jdoe-dev, context: personal}ssh prod-bastion{tool_name: bash, …}…, destination_account: {platform: ssh, host: 10.0.4.12:2222, user: deploy}ls -laTests
packages/core/test/destinationAccount.test.ts: 8 tests mirroring the Python suite, run against realgitandsshwith throwaway repos and ssh configs. They cover login stripping, newlines and push options, per-segment folders, no-account commands, gh flags and token override, the ssh options and plain sftp, the allowlist and spent budget, and shell-only calls.packages/pi/test/agentEnd.test.ts: an end-to-end check throughhandleAgentEndand the mock API that the posted turn log carries the account on the bash call.npm run test:buildpasses, confirming onlynode:builtins in the bundle.shasum -c pi/index.js.sha256passes.tests/pipasses, 303 tests.npm run test:unitshows 11 to 13 failures in the existing mock-API timing tests here, and unmodified staging shows 21 to 28 on the same machine. CI's clean runners are the reference.🤖 Generated with Claude Code
Note
Medium Risk
Adds bounded local subprocess and ssh-config reads during turn log posting; behavior is allowlisted and fail-soft, but mis-parsed shell or env edge cases could mis-attribute accounts in telemetry.
Overview
Pi turn logs now include
destination_accounton bash/powershelltool_useentries, matching the Python hooks: which GitHub/git remote or SSH host a shell command would reach, without blocking the agent.A new
resolveDestinationAccountsmodule parses shell commands (includingcd, wrappers likesudo/env, and compound lines) and resolves git push, gh, and ssh/scp/sftp/rsync using an allowlisted set of local lookups (git config,gh config get,ssh -G) with per-run and per-turn timeouts. Failures or unsafe ssh config paths yield{platform, source: "unknown"}; tokens/logins are stripped from reported fields.Turn posting runs resolution on the existing fire-and-forget path before
postTurnLog.TurnToolCallgains an internalran: falseflag (never sent) plusmarkNotRun/onDeclinedso denied, unavailable, declined confirms, and!cmdrows still awaiting confirm skip lookups. Standalone!cmdlogs resolve using the eventcwd, not only the session cwd.Extensive unit/integration tests and a rebuilt
pi/index.jsbundle ship with the change.Reviewed by Cursor Bugbot for commit f7a5975. Bugbot is set up for automated code reviews on this repo. Configure here.
Unbound Review · Score 3/5 (1 P1) ·
f7a5975_destination_accountresolver to TypeScript for the Pi hook. It resolves git push, gh and ssh-family destinations using allowlisted, time-limited local lookups, and runs them on the existing fire-and-forget turn-log chain. It also marks declined confirms and unanswered!cmdconfirms as never run, and resolves!cmdin the event's own cwd. I traced the shell segmenting, subshell and short-circuit handling, wrapper parfindings: 0 P0 · 1 P1 · 0 P2 (top 5 P0/P1 inline) · engines: codex ✅ · claude ✅
Findings not posted inline
destinationAccount.ts:1000Preserve parent directory across pipelines and background commands — The resolver applies every cd to subsequent commands, including when cd runs in a pipeline or background process. Forcd /other | cat; git push origin mainorcd /other & git push origin main, Bash pushes from the original directory, but the resolver reads /other's origin and reports that reposi (codex)