Skip to content

feat: resolve git, GitHub and SSH accounts in the Pi hook - #396

Open
AakashVelusamy wants to merge 13 commits into
stagingfrom
aakashvelusamy/web-6295-feat-resolve-accounts-in-the-pi-hook
Open

AakashVelusamy wants to merge 13 commits into
stagingfrom
aakashvelusamy/web-6295-feat-resolve-accounts-in-the-pi-hook

Conversation

@AakashVelusamy

@AakashVelusamy AakashVelusamy commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

Pi now sends the same destination_account as the other five agents, so a git push or ssh run 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

  • New hooks-ts/packages/core/src/destinationAccount.ts. It is a TypeScript port of the Python _destination_account block, with the same behaviour:
    • git push: the push remote's host and owner, with any login stripped, plus the gh login when gh is the credential helper. It skips -o option values and honours --repo.
    • gh: host and owner from -R, GH_REPO, a positional repo argument or the repo's remote; GH_HOST; the login is dropped under GH_TOKEN.
    • ssh, scp, sftp and rsync: the real host, user and port via ssh -G. It honours -l, -p/-P, -o, -F and -J, whether given separately or attached, and plain sftp user@host. rsync host::module reaches no account.
    • Commands: newlines and ; && || | separate commands, and cd changes the folder the later segments run in.
  • Same safety rules. Only git remote get-url --push, git rev-parse @{push}, git config --get, gh config get -h … user and ssh -G can run. Each has a 5-second cap and the turn shares a 10-second budget. A failure yields {platform, source: "unknown"}.
  • Never blocks pi. The lookups run on the turn log's existing fire-and-forget chain in dispatchTurnLog. agent_end still returns at once, and the post waits only for the lookups.
  • The turn log. TurnLogToolUse gains an optional destination_account, attached by tool_use_id.
  • Bundle. pi/index.js and pi/index.js.sha256 are rebuilt from source.

Before → after (Pi turn log, tool_use[])

Pi runs Before After
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 -la unchanged unchanged

Tests

  • packages/core/test/destinationAccount.test.ts: 8 tests mirroring the Python suite, run against real git and ssh with 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 through handleAgentEnd and the mock API that the posted turn log carries the account on the bash call.
  • Build checks: npm run test:build passes, confirming only node: builtins in the bundle. shasum -c pi/index.js.sha256 passes.
  • Python suite: tests/pi passes, 303 tests.
  • Timing flakiness on this machine. npm run test:unit shows 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.

SOC 2 note: this changes what the Pi hook sends to the control plane. It adds identity fields only, never secrets, and needs named-approver review.

🤖 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_account on bash/powershell tool_use entries, matching the Python hooks: which GitHub/git remote or SSH host a shell command would reach, without blocking the agent.

A new resolveDestinationAccounts module parses shell commands (including cd, wrappers like sudo/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. TurnToolCall gains an internal ran: false flag (never sent) plus markNotRun / onDeclined so denied, unavailable, declined confirms, and !cmd rows still awaiting confirm skip lookups. Standalone !cmd logs resolve using the event cwd, not only the session cwd.

Extensive unit/integration tests and a rebuilt pi/index.js bundle 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

  • codex (own score 3/5): The PR adds destination-account resolution to Pi turn logs, including bounded subprocess lookups and handling for declined commands. The integration is generally coherent, but pipeline and background commands can cause account telemetry to identify the wrong repository.
  • claude (own score 5/5): This PR ports the Python _destination_account resolver 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 !cmd confirms as never run, and resolves !cmd in the event's own cwd. I traced the shell segmenting, subshell and short-circuit handling, wrapper par

findings: 0 P0 · 1 P1 · 0 P2 (top 5 P0/P1 inline) · engines: codex ✅ · claude ✅

Findings not posted inline
  • P1 destinationAccount.ts:1000 Preserve 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. For cd /other | cat; git push origin main or cd /other & git push origin main, Bash pushes from the original directory, but the resolver reads /other's origin and reports that reposi (codex)

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>
@AakashVelusamy
AakashVelusamy requested a review from a team October 19, 2026 14:17
@AakashVelusamy

Copy link
Copy Markdown
Contributor Author

/review

@AakashVelusamy

Copy link
Copy Markdown
Contributor Author

@cursoragent review

@cursor

cursor Bot commented Oct 19, 2026 •

Copy link
Copy Markdown

The Pi port matches the Python resolver, and the turn-log wiring is safe: lookups stay on the existing fire-and-forget path, so agent_end still returns immediately. Two gaps are specific to this port and should be fixed before it ships.

Blocked commands still run ssh -G

resolveDestinationAccounts runs for every bash and powershell call in the turn record. That record is written at decision time, including deny, unavailable, and confirm (the stored decision stays confirm even when the developer declines). Python only resolves on PostToolUse, after the command has actually run.

ssh -G is not side-effect free on OpenSSH 9.6. ProxyCommand, LocalCommand, and RemoteCommand do not run, but a config Match exec does. I confirmed that with ssh -G -F <config>. So a blocked ssh, scp, or sftp still re-evaluates ~/.ssh/config, and an absolute -F path is passed through on purpose:

      if (takesValue && value !== undefined && ["l", "o", "F", "J", portFlag].includes(flag)) {
        passthrough.push(flag === portFlag ? "-p" : `-${flag}`, value);
      }

A write earlier in the turn can plant that file. Policy then blocks ssh -F /tmp/evil.cfg host, and the turn-log lookup still executes the file’s Match exec. The same applies to a declined !cmd, because postStandaloneTurn uses this same path.

Resolve only calls that actually ran. deny and unavailable should be skipped, and a declined confirm needs to be recorded as declined before this lookup.

A gh URL keeps the token in host

git remotes go through stripLogin. gh does not. For gh pr view -R https://x-access-token:ghp_…@github.com/acme/app, the parser does replace(/^[a-z]+:\/\//, "") and takes the first path segment, so host becomes x-access-token:ghp_…@github.com (lowercased). That is the field this PR says will never carry a secret. The same hole is in the Python block; the git path already shows the fix: run repo URLs through stripLogin / remoteParts.

Relative ssh -F is resolved in the wrong directory

Python runs the lookup with the shell’s cwd. This port uses defaultExecFile, which always sets cwd to the temp dir (accountIdentity.ts). git -C is unaffected. ssh -G -F cfg host is not: OpenSSH reports Can't open user config file cfg and the account is stored as {platform: "ssh", source: "unknown"}. Pass the turn cwd into this lookup.

The allowlist, 5s/10s budget, login stripping on git remotes, and the tool_use_id attachment all match the Python behavior. ~/dir happens to resolve because path.join collapses the extra slash. Still untested relative to the Python suite: the gh credential-helper login, sudo ssh, and attached -F/-l. sudo -u user ssh is treated as a command named user in both implementations.

Open in Web Open in Cursor 

@vigneshsubbiah16 vigneshsubbiah16 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛡️ 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>
@AakashVelusamy

Copy link
Copy Markdown
Contributor Author

/review

@AakashVelusamy

Copy link
Copy Markdown
Contributor Author

@cursoragent review

@cursor

cursor Bot commented Oct 19, 2026 •

Copy link
Copy Markdown

The account field is wrong for ordinary scp and rsync commands, and a blocked ssh still runs ssh -G. I re-ran the parser against the real resolver.

Blocked commands still run ssh -G

resolveDestinationAccounts walks every recorded bash and powershell call. The turn record is written when the policy decides, so deny, unavailable, and a declined confirm are all included. Python resolves only on PostToolUse, after the command has run.

On OpenSSH 9.6, ssh -G executes a config Match exec. ProxyCommand and LocalCommand stay inert. An absolute -F is forwarded on purpose, so a file written earlier in the turn is enough: policy blocks ssh -F /tmp/evil.cfg host, and the turn-log lookup still runs that file’s Match exec. A declined !cmd takes the same path through postStandaloneTurn. A blocked ssh host with no -F also re-reads ~/.ssh/config.

Skip calls that did not run. A declined confirm has to be stored as declined first, because today the record stays confirm either way.

scp and sftp share ssh’s flag table

SSH_VALUE_FLAGS is ssh’s option set. For scp and sftp the port flag is -P, and lowercase -p means “preserve times” and takes no argument. The parser still treats -p as taking a value and swallows the next token.

scp -p deploy@prod:/var/log/app.log . and sftp -p deploy@prod produce no account at all.

Lowercase -l is a bandwidth limit for both tools, and it is forwarded to ssh -G as a login name. scp -l 8192 deploy@prod:/var/log/app.log . becomes ssh -G -l 8192 deploy@prod, which resolves to user 8192 and is posted with source: "command".

rsync -e drops the remote shell and still reports a hit

rsync -e 'ssh -p 2222 -l deploy' ./build/ prod:/srv/app/ discards the -e argument and runs ssh -G prod. The posted user and port are the local defaults, with source: "command", so the row looks resolved. Those two flag bugs are in the Python block as well; fixing them only here drifts from that copy.

Relative ssh -F is opened in the temp dir

ssh -F ./deploy/ssh_config prod keeps the relative path, then defaultExecFile always uses the OS temp directory as cwd. OpenSSH looks for cfg there, fails, and the account is stored as {platform: "ssh", source: "unknown"}. Python passes the shell cwd into the same lookup. This one is specific to the Pi port.

A gh URL keeps the token in host

git remotes go through stripLogin. gh -R does not. gh pr view -R https://x-access-token:ghp_SECRETVALUE@github.com/acme/app posts host: "x-access-token:ghp_secretvalue@github.com" and passes that string to gh config get -h. The same hole is in the Python block. Run those repo URLs through stripLogin / remoteParts before anything is copied onto the wire.

Open in Web Open in Cursor 

@vigneshsubbiah16 vigneshsubbiah16 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛡️ 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

@cursor cursor Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale Bugbot comment from a previous run.

Comment thread hooks-ts/packages/core/src/destinationAccount.ts Outdated
Comment thread hooks-ts/packages/core/src/destinationAccount.ts
Comment thread hooks-ts/packages/core/src/destinationAccount.ts Outdated
Comment thread hooks-ts/packages/core/src/destinationAccount.ts
…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>
@AakashVelusamy

Copy link
Copy Markdown
Contributor Author

/review

@AakashVelusamy

Copy link
Copy Markdown
Contributor Author

@cursoragent review

@cursor

cursor Bot commented Oct 19, 2026 •

Copy link
Copy Markdown

The fixes in 05235c96 hold. The 21 destination-account tests pass, and the earlier scp/rsync/ssh -p mistakes are gone. Two problems are still real; I reproduced both against this commit.

Include ~user still runs Match exec

configRunsCommands follows Include and refuses a config that contains Match exec. It only expands ~ and ~/. OpenSSH also expands ~user.

A config like Include ~ubuntu/.ssh-review-include/inner, where inner is Match host * exec "touch …", is scanned at ~/.ssh/~ubuntu/.... That path is not the file. The scanner treats the config as safe, then ssh -G reads the real include and runs the command. The turn log still comes back as a normal hit: {platform: "ssh", source: "command", host: "10.8.8.8"}. ProxyCommand, LocalCommand, and KnownHostsCommand do not run under ssh -G; this path does.

deny and unavailable never get here. A declined confirm still does, because the stored decision stays confirm. Expand ~user the same way ssh does before deciding the config is safe, and skip a confirm the developer refused.

sudo -u hides the real command

program skips a wrapper and any following token that starts with -. The option's argument is left as the program name. sudo -u deploy ssh prod-bastion resolves nothing and runs no lookup. The same happens for sudo -g, env -u, and time -o. sudo ssh still works, which is the only wrapper form the tests cover. Skip the value of those wrapper options before looking for git, gh, or ssh.

Open in Web Open in Cursor 

@vigneshsubbiah16 vigneshsubbiah16 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛡️ 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 a base function 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 a base function 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

@cursor cursor Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale Bugbot comment from a previous run.

Comment thread hooks-ts/packages/core/src/destinationAccount.ts
Comment thread hooks-ts/packages/core/src/destinationAccount.ts
Comment thread hooks-ts/packages/core/src/destinationAccount.ts Outdated
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>
@AakashVelusamy

Copy link
Copy Markdown
Contributor Author

/review

@AakashVelusamy

Copy link
Copy Markdown
Contributor Author

@cursoragent review

@cursor

cursor Bot commented Oct 19, 2026 •

Copy link
Copy Markdown

The quoted-include and bare-push fixes in 2d9946c0 hold. All 23 destination-account tests pass, and a bare git push follows branch.*.pushRemote, remote.pushDefault, and branch.*.remote through to personal. ~/src resolves to /home/ubuntu/src. Two attribution bugs are still real.

ssh -F ~/config is always unknown

expandHome("~/config") is already absolute, and the -F rewrite only stores the expanded path when it is still relative:

  for (let i = 0; i + 1 < passthrough.length; i += 2) {
    const value = expandHome(passthrough[i + 1] as string);
    if (passthrough[i] === "-F" && !isAbsolute(value) && value !== "none") passthrough[i + 1] = join(dir ?? "", value);
  }

The passthrough keeps ~/config. configRunsCommands treats every non-absolute -F as unchecked and returns unknown, so the lookup never runs. I reproduced this with a real config under the home directory: ssh -F ~/.ssh-tilde-f-… prod-bastion came back {platform: "ssh", source: "unknown"}. Write the expanded path back before that check.

sudo -u still hides the command

program skips a wrapper and the following dash tokens, then treats the option’s argument as the program. sudo -u deploy ssh prod produces no account and runs no lookup. The same applies to sudo -g, env -u, and time -o. Skip those option values before looking for git, gh, or ssh.

ssh -- -F/path/to/config becomes ssh -G -F/path with no hostname. On OpenSSH 9.6 that exits with a usage error before Match exec. A declined confirm is still stored as confirm, so it is still resolved.

Open in Web Open in Cursor 

@vigneshsubbiah16 vigneshsubbiah16 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛡️ 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>
@AakashVelusamy

Copy link
Copy Markdown
Contributor Author

/review

@AakashVelusamy

Copy link
Copy Markdown
Contributor Author

@cursoragent review

@cursor

cursor Bot commented Oct 19, 2026 •

Copy link
Copy Markdown

The home-folder -F fix in e4a768eb holds. Two defects on this commit are real; I reproduced both with OpenSSH 9.6.

A quoted Match still runs

The scanner only recognizes a line that begins with the bare word Match:

      if (/^\s*match\b.*\bexec\b/i.test(line)) return true;
      const include = /^\s*include\s*[=\s]\s*(.+)/i.exec(line);

OpenSSH accepts a quoted keyword. A config containing "Match" exec "touch …" passes the scan, then ssh -G runs the command and the turn log comes back as a normal hit: {platform: "ssh", source: "command", host: "10.9.9.9"}. The same is true of "Include" "/path/to/inner": the scanner never opens the included file, and ssh -G still executes a Match exec inside it. Strip quotes from the keyword before deciding the file is safe, and follow a quoted Include the same way as a bare one.

IPv6 copy targets are dropped

remoteOperand treats every :: as an rsync daemon address and returns no destination:

function remoteOperand(operands: readonly string[]): string | undefined {
  for (const arg of [...operands].reverse()) {
    if (arg.startsWith("rsync://") || arg.includes("::")) return undefined;
    const match = /^((?:[^@/:]+@)?\[?[^/:\]]+\]?):/.exec(arg);

scp file 'deploy@[2001:db8::1]:/tmp/' produces no account and no lookup. The hostname pattern also stops at the first colon, so a bracketed IPv6 address would still fail after the :: check is narrowed to daemon syntax (host::module outside brackets). ssh deploy@[2001:db8::1] is unaffected, because that path does not use remoteOperand.

sudo -u deploy ssh still resolves to nothing. The wrapper skip leaves deploy as the program name.

Open in Web Open in Cursor 

@vigneshsubbiah16 vigneshsubbiah16 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛡️ 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

@AakashVelusamy

Copy link
Copy Markdown
Contributor Author

Review disposition: every finding, every reviewer (head e4a768eb)

Where it stands: the Pi port now matches the Python hooks' resolution (setup#395), plus two Pi-only fixes: blocked calls never run a lookup, and relative or ~ ssh configs are read from the right folder. Every fix below has a test that fails on the code before it. What's left is rare command shapes that come back with no account rather than a wrong one. Unbound Review's score comes from Codex alone, because the Claude engine is logged out.

Pi-specific

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.

@AakashVelusamy

Copy link
Copy Markdown
Contributor Author

@cursoragent review

@cursor

cursor Bot commented Oct 19, 2026 •

Copy link
Copy Markdown

The option-order and unquoted-escape fixes on 1357bb02 hold. All 35 destination-account tests pass. A backslash inside single quotes still gets past the scanner, and ssh -G runs the included Match exec.

Options after the hostname. OpenSSH 9.6 applies -p, -l, -o, and -F between the host and the command. ssh -F none example.invalid -p 2200 -l deploy comes back as user deploy, host example.invalid:2200. ssh example.invalid ls -l and ssh example.invalid uptime -p 2200 keep the local user and port 22. scp and sftp do not take options after the destination, and the parser leaves those alone.

Unquoted and double-quoted includes. Include /path/inner\_config and Include "/path/inner\_config" both expand to inner_config. The scanner follows that file, returns {platform: "ssh", source: "unknown"}, and does not create the marker.

Single quotes keep the backslash. OpenSSH still expands \ and \_ inside '…'. The scanner only treats \ as an escape outside single quotes:

    } else if (ch === "\\" && quote !== "'") {
      escaped = true; // ssh, like a shell, takes the next character literally

For Include '/tmp/sub\ dir/inner' containing Match exec "touch …", the lookup returns {platform: "ssh", source: "command", host: "10.6.6.6"} and the marker is created. The same happens for '…/inner\_config'.

Explicit git push URLs still skip pushInsteadOf, and gh pr view https://github.com/other/app/pull/1 still reports origin’s fetch repo. A GH_HOST value with a login is now reduced to the host.

Open in Web Open in Cursor 

@vigneshsubbiah16 vigneshsubbiah16 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛡️ 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

ghost left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale Bugbot comment from a previous run.

Comment thread hooks-ts/packages/core/src/destinationAccount.ts
overridden = true;
} else if (flag === "--namespace" && rest.length > 0) {
rest.shift();
}

ghost Oct 19, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 1357bb0. Configure here.

@AakashVelusamy

ghost commented Oct 19, 2026

Copy link
Copy Markdown
Contributor Author

Review disposition addendum (head 1357bb02): everything raised since the audit at e4a768eb

Same fixes as the Python hooks (setup#395), ported in baef37c5, dccbc13b, e596e092 and 1357bb02. Each has a test that fails on the code before it. Pi-only fixes: its ssh-config reader no longer splits a path at ;, and it now applies backslash escapes outside single quotes.

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.

ghost left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛡️ 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. from ssh -- -F/tmp/evil host) is appended without --, so ssh -G may load an agent-controlled config; configRunsCommands never scans that -F, and Match exec in 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 with ssh -- -F<exec-config> x and 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_RAN only skips deny and unavailable; declined or headless-blocked confirms keep confirm, so git / gh / ssh -G still run on command text the tool never executed—amplifying the ssh -G injection 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 when slice(1) is absolute (/…), so cd ~/…, git -C ~/…, ssh -F ~/…, and Include ~/.ssh/… scans miss real files; configRunsCommands can clear a config that still loads via ssh -G, allowing Match exec during lookup.
  • Fix: Expand ~/ with join(homedir(), path.slice(2)) (and handle ~ alone); add tests for Include ~/.ssh/config.d/* with Match 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_account name a different org/host than the tool actually used (gh does not skip other options’ values; explicit git push URLs skip insteadOf / pushInsteadOf; sudo -u / env wrappers hide the real program), weakening audit and policy that trust this field.
  • Fix: Consume value-taking flags when scanning gh; resolve explicit push URLs via git ls-remote --get-url or emit unknown; extend wrapper peeling for value-taking wrapper flags; prefer unknown over confident source: "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_response are redacted with apiKey; host, account, context, and user are attached raw—unusual tokens in -R, GH_REPO, remotes, or GH_HOST could reach the control plane if stripLogin / withoutLogin miss a shape.
  • Fix: Run each string field of destination_account through the same redactSecrets(…, 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>

ghost left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛡️ Automated Security Review (consensus)

5 findings — 2 high-confidence, 3 to triage. Reviewers: Cursor, Claude, Semgrep, Gitleaks.

  • [HIGH] ssh -G destination parsed as extra OpenSSH options 🔴 — hooks-ts/packages/core/src/destinationAccount.ts:448

    • What: The enrichment path invokes ssh -G with ...passthrough, destination and no --, so a destination token beginning with - is interpreted as ssh flags (e.g. -F, -E).
    • Why: configRunsCommands only inspects -F values already in passthrough. Paths such as ssh -- -F/tmp/evil or scp x '-Fevil:y' can make ssh -G load an attacker-controlled config under the project cwd while the guard still scans the default tree; Match exec in 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 - as unknown; add regression tests (ssh -- -F<evil>, scp … '-F<evil>:path').
    • Flagged by: Claude, Cursor
  • [HIGH] expandHome breaks ~/… 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, and Include ~/.ssh/config.d/* resolve to filesystem-root paths, so configRunsCommands may miss real Match exec / Include content while ssh -G still 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 the HOME= override case.
    • Flagged by: Cursor
  • [MEDIUM] Shell redirections are not stripped from argv 🟡 — hooks-ts/packages/core/src/destinationAccount.ts:130

    • What: segments() does not treat 2>&1, >file, etc. as redirections; tokens like 2> can become git remotes or split the command across segments.
    • Why: Common forms (git push 2>&1) can yield confident but wrong destination_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 / gh parsing; add tests for git push origin main 2>&1.
    • Flagged by: Claude (Unbound/codex P1; PR author claims parity with Python—unverified here)
  • [MEDIUM] confirm policy outcomes can still trigger lookups 🟡 — hooks-ts/packages/core/src/destinationAccount.ts:581

    • What: NEVER_RAN is only deny and unavailable; a recorded decision: "confirm" with ran unset still enters resolveDestinationAccounts until markNotRun / ran: false is applied.
    • Why: Declined or headless-blocked confirms that never executed the shell command may still spawn allowlisted git/gh/ssh -G on 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 add confirm to the never-ran set once execution is proven); ensure every non-run confirm path sets ran: false before agent_end posting.
    • Flagged by: Cursor
  • [LOW] Sync ssh-config read can block on FIFO swap 🟡 — hooks-ts/packages/core/src/destinationAccount.ts:463

    • What: statSync then readFileSync on 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, fstat the fd, verify regular file and size cap, then read from that handle.
    • Flagged by: Claude

🤖 consensus review · reviewers: Cursor, Claude, Semgrep, Gitleaks · head 4ab7393d · 2026-10-21T00:42Z

@AakashVelusamy

ghost commented Oct 21, 2026

Copy link
Copy Markdown
Contributor Author

/review

@AakashVelusamy

ghost commented Oct 21, 2026

Copy link
Copy Markdown
Contributor Author

@cursoragent review

@cursor

ghost commented Oct 21, 2026 •

Copy link
Copy Markdown

The rare-shape fixes on 4ab7393d match real git, gh, and OpenSSH 9.6. All 44 destination-account tests pass. The two high findings on this commit do not reproduce on this machine. One gh URL shape still posts the wrong owner.

These now match the command that actually ran

  • git push work:app.git with url.https://github.com/acme/.pushInsteadOf=work: is rewritten to https://github.com/acme/app.git by git itself. The resolver reports github.com / acme. An explicit https://github.com/acme/app.git push does too, and the credentialed global insteadOf on this machine does not appear in the account.
  • gh pr view https://github.com/other/app/pull/1 sends GraphQL {"owner":"other","repo":"app"}. So does the resolver, including when gh-resolved points somewhere else. gh pr list with remote.upstream.gh-resolved=base queries other; a repo whose origin fetches acme and pushes to personal makes gh pr view 1 query acme. gh pr create -R acme/app --title '-Rpersonal/app' stays acme.
  • sudo -u, env -u, and timeout no longer hide ssh. scp://deploy:secret@10.1.2.3:2222/… is user deploy, host 10.1.2.3:2222, and the password is absent.
  • A single-quoted Include with \ or \_ is followed. Real ssh -G runs the included Match exec and creates the marker. The resolver returns {platform: "ssh", source: "unknown"} and leaves the marker uncreated. A benign Include '…/inner\_config' resolves to 10.6.6.6 / quoted, the same host ssh -G prints.
  • ssh -F ~/tilde.cfg under a different HOME resolves to 10.4.4.4. path.join(homedir(), "~/src".slice(1)) is /home/ubuntu/src.
  • A declined confirm is skipped. resolveDestinationAccounts ignores ran: false, and ssh -F <evil> on that call does not create the marker. decide.ts sets that flag when the confirm comes back blocked; the bundle has the same wiring. An accepted in-turn confirm (decision: "confirm", ran unset) still resolves.

What still disagrees

  • A login in a gh URL is posted as the wrong owner. gh pr view https://user:notatoken@github.com/other/app/pull/1 queries other. From a repo whose origin fetches acme, the resolver returns {platform: "github", host: "github.com", account: "acme", source: "command"}. The login is not kept. The plain URL regex never gets past the @:
    const url = positional.slice(1).map((a) => /^https?:\/\/([^/@]+\/[^/@]+\/[^/@]+)/.exec(a)).find((m) => m !== null);
    if (url) repo = url[1] as string;
  • Redirections are still words. git push 2>&1 is split into git push 2> and 1, and the account is {platform: "git", source: "unknown"}. Bash runs a bare git push, which tells you to push origin. git push origin main 2>&1 still reports personal. 2>&1 git push origin and git push>out produce no account.
  • Match host executive forces {source: "unknown"} because the criterion only has to start with exec. ssh -G returns 10.8.8.8 and runs nothing.
  • A !cmd whose policy is confirm is posted before the dialog with ran: false, so that row never gets an account, including when the person later accepts. Declined tool calls are the path that needed to stop spawning lookups, and that path is closed.
Open in Web Open in Cursor 

ghost left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛡️ Automated Security Review (consensus)

2 findings: 1 high-confidence, 1 to triage. Reviewers: Claude (lead), Semgrep, Gitleaks.

  • [MEDIUM] ssh -G destination can inject options and bypass the Match-exec guard 🔴 — hooks-ts/packages/core/src/destinationAccount.ts:772

    • What: ssh() passes the parsed destination straight to run(["ssh", "-G", ...passthrough, destination]) with no -- before it. A destination that starts with - is read by OpenSSH as another option.
    • Why: sshDestination sends every word after -- to operands. So the agent command ssh -- -F/tmp/evil gives destination = "-F/tmp/evil" and an empty passthrough.
      • configRunsCommands then sees no -F. It scans only ~/.ssh/config and /etc/ssh/ssh_config, and passes.
      • The hook then runs ssh -G -F/tmp/evil, which loads that file and runs any Match exec command 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:z can reach remoteOperand and 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 using ssh -- -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 --.
  • [LOW] Match-exec scan reads $HOME's ssh config, but ssh reads the passwd home 🟡 — hooks-ts/packages/core/src/destinationAccount.ts:734

    • What: configRunsCommands scans join(homedir(), ".ssh/config"). Node's homedir() uses $HOME. OpenSSH finds ~/.ssh/config through getpwuid()->pw_dir, the home folder in the system's user database.
    • Why: Pi can run with HOME set to a different folder, for example in a sandbox, a container or a test harness. Then the guard scans the wrong file. ssh -G still reads the real ~/.ssh/config and runs any Match exec in it.
    • Fix: Choose one:
      • Pass -F with the exact scanned path explicitly to ssh -G, so the scanned file and the loaded file are always the same.
      • Return unknown when os.userInfo().homedir !== homedir().
    • Flagged by: Claude. This matches an earlier Unbound Review P2.

🤖 consensus review · reviewers: Claude, Semgrep, Gitleaks · head 4ab7393d · 2026-10-21T00:46Z

ghost left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale Bugbot comment from a previous run.

Comment thread hooks-ts/packages/core/src/destinationAccount.ts
… 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>
@AakashVelusamy

ghost commented Oct 21, 2026

Copy link
Copy Markdown
Contributor Author

/review

@AakashVelusamy

ghost commented Oct 21, 2026

Copy link
Copy Markdown
Contributor Author

@cursoragent review

@cursor

ghost commented Oct 21, 2026 •

Copy link
Copy Markdown

The fixes on bf16d677 hold. All 49 destination-account tests pass, including the new !cmd cwd test. Two lookup paths still post an account that the command did not reach.

What matches the command

  • Redirections are no longer arguments. Bash runs git push 2>&1 and git push>/tmp/out as a bare git push, and the resolver reports that repo's origin (acme).
  • A quoted heredoc body is input. cat <<'EOF' followed by git push origin main produces no account, and bash does not run the body. A push after the closing line is still resolved.
  • gh pr view https://user:notatoken@github.com/other/app/pull/1 reports other. The login is absent from the account. gh search, gh gist, and gh repo create do not take the checkout's owner.
  • A typed !cmd resolves in the event's cwd. The test pushes personal from a repo that is not the session directory and the posted account is jdoe-dev. The bundle uses the same event.cwd.
  • sudo -u nobody ssh example.invalid with no -F is {platform: "ssh", source: "unknown"}. git config set marks an earlier push in the same command unknown. On git 2.43 that subcommand is rejected (key does not contain a section: set) and does not change the remote, so the unknown result is the safe side.

What still disagrees

  • An attached git -C is still one flag. git -C /other push origin reports personal. git -C/other push origin reports the session repo, acme, with source: "command". On git 2.43.0 that attached form is an unknown option and exits 129, so the push does not run.
  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);
  • sudo -u with -F keeps the hook's user. sudo -n -u nobody ssh -G -F cfg example.invalid prints user nobody, host 10.9.9.9. The resolver posts user ubuntu for that same host, because -F makes it run ssh -G as itself. Without a User line, OpenSSH's user is the uid that invoked ssh.
  • An unquoted heredoc still runs command substitutions. cat <<EOF / $(touch marker) / EOF creates the marker; the resolver returns no account. The quoted form, which was the false push, stays unresolved.
Open in Web Open in Cursor 

ghost left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛡️ 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 hidden Match exec can still run 🟡 — hooks-ts/packages/core/src/destinationAccount.ts:758

    • What: configWords treats 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 like Match !host #x exec "cmd", the scanner reads only Match !host and calls it safe. Old ssh reads #x as a host pattern, the negated host check passes, and it runs exec "cmd".
    • Why: ssh -G then 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 Match line contains exec anywhere (case-insensitive), or contains a # after the keyword. An even safer rule is to refuse when any line in the config files contains exec.
    • Flagged by: claude
  • [LOW] Identity fields skip redactSecrets, and ssh user:pass@host puts the password in user 🟡 — hooks-ts/packages/core/src/destinationAccount.ts:840, hooks-ts/packages/core/src/turnLog.ts:320

    • What: For ssh, the raw destination goes straight to ssh -G. ssh splits on the last @, so ssh admin:S3cret@host prints user admin:S3cret, and that value is posted as destination_account.user. In buildTurnLogBody, tool_input goes through toolInputFor/redactSecrets, but destination_account is 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 calling ssh -G (scp and sftp URIs already do this). Also run destination_account values through redactSecrets(…, apiKey) in buildTurnLogBody.
    • Flagged by: claude
  • [LOW] ssh -G lookup 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 -G and 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 -F skip configRunsCommands.
    • Fix: Refuse destinations that start with -, or add "--" before the destination: ["ssh", "-G", ...passthrough, "--", dest].
    • Flagged by: claude (also raised earlier by Cursor Bugbot)
  • [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: statSync checks for a small regular file, then readFileSync(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, fstatSync the descriptor, and read through that descriptor with a byte cap.
    • Flagged by: claude
  • [LOW] Never-ran filter is fail-open for confirm 🟡 — hooks-ts/packages/core/src/destinationAccount.ts:96

    • What: NEVER_RAN only skips deny and unavailable. A confirm call is resolved unless something else marks it ran: false.
    • Why: Any declined or blocked confirm path that misses markNotRun still triggers local git/gh/ssh -G lookups for a command that never ran. Cursor Bugbot raised the headless no-UI case. The decide.ts part of the diff is cut off here, so it could not be checked.
    • Fix: Fail closed. Skip confirm unless the call is clearly marked as having run (for example ran: true set on approval), instead of relying on ran: false being set on every decline path.
    • Flagged by: claude (also raised earlier by Cursor Bugbot)

🤖 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>
@AakashVelusamy

ghost commented Oct 21, 2026

Copy link
Copy Markdown
Contributor Author

/review

@AakashVelusamy

ghost commented Oct 21, 2026

Copy link
Copy Markdown
Contributor Author

@cursoragent review

@cursor

ghost commented Oct 21, 2026 •

Copy link
Copy Markdown

The fixes on d75786b9 hold. All 50 destination-account tests pass. Three lookup paths still post a host the command did not use.

What matches the command

  • unset GH_HOST; gh repo view acme/app, with GH_HOST=ghe.example.com in the environment, makes gh request Host: api.github.com. The resolver reports github.com / acme.
  • gh api https://ghe.example.com/api/v3/repos/acme/app requests that host and path. The resolver reports ghe.example.com / acme. A login in the URL is dropped.
  • ssh -- -F<evil> prod-bastion never reaches ssh -G. The account is omitted and the Match exec marker is not created.
  • On this Linux scp, C:/Users/me is host c. The resolver reports c for that form.

What still disagrees

  • env -u removes the variable for gh, and the resolver keeps the inherited value. GH_HOST=ghe.example.com env -u GH_HOST gh repo view acme/app requests api.github.com. The resolver reports ghe.example.com / acme with source: "command". env -u consumes the name and does not record an unset; only the unset builtin does.
    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] = "";
  • A full gh api URL beats --hostname. gh api --hostname ghe.example.com https://api.github.com/repos/acme/app requests api.github.com. The resolver reports ghe.example.com / acme, because the flag is applied after the URL host.
  host = flagHost || host || envHost || "github.com";
  • C:\Users is host c for this scp, same as C:/Users. scp example.invalid:/tmp/file 'C:\Users\me' runs ssh host C. The resolver treats the backslash form as a local path on Linux, so scp prod-bastion:/tmp/file C:\Users\me is posted as bastion 10.0.4.12.
    // 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 } : {};

git -C/other still reports the session repo, and sudo -u nobody ssh -F cfg still posts the hook user ubuntu where ssh -G under that uid prints nobody.

Open in Web Open in Cursor 

ghost left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛡️ Automated Security Review (consensus)

1 finding: 0 high-confidence, 1 to triage. Reviewers: claude (lead), semgrep, gitleaks.

  • [LOW] The Match exec guard checks $HOME, but ssh reads the passwd home folder 🟡: hooks-ts/packages/core/src/destinationAccount.ts (configRunsCommands / expandHome)
    • What: configRunsCommands decides which ssh config to scan with join(homedir(), ".ssh"). includeTargets expands a ~ in Include paths the same way. Node's homedir() reads $HOME first. OpenSSH finds ~/.ssh/config and expands ~ from the passwd entry (pw_dir), not from $HOME.
    • Why: Sometimes the hook runs with a HOME that differs from the real account home. Examples are containers, sudo -E, CI runners, or a test harness that overrides HOME. Then the guard scans one file while ssh -G loads another. A Match exec in the real ~/.ssh/config, or in a file it includes, would be judged safe, and ssh -G would run that command inside the hook during turn-log enrichment (the background lookups that add destination_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 in configRunsCommands/includeTargets. A simpler option: if process.env.HOME !== os.userInfo().homedir, return true (treat the config as unsafe and skip the lookup). Keep $HOME only for ~ in cd/git -C paths, where the shell's behaviour is what matters.
    • Flagged by: claude

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. ⚠️ The Pi-side code that sets it is past the diff truncation, so I could not see it.
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

ghost left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale Bugbot comment from a previous run.

Comment thread hooks-ts/packages/core/src/destinationAccount.ts
for (const key of Object.keys(result)) delete (result as unknown as Record<string, unknown>)[key];
Object.assign(result, { platform: "git", source: "unknown" });
}
continue;

ghost Oct 21, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Fix in Cursor Fix in Web

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>
@AakashVelusamy

ghost commented Oct 21, 2026

Copy link
Copy Markdown
Contributor Author

/review

@AakashVelusamy

ghost commented Oct 21, 2026

Copy link
Copy Markdown
Contributor Author

@cursoragent review

@cursor

ghost commented Oct 21, 2026 •

Copy link
Copy Markdown

The subshell, env -u/env -i, and sudo-ssh fixes on f7a59754 hold, and all 55 destination-account tests pass. Two lookup paths still post an account the command did not reach, both with source: "command".

What holds

  • (cd frontend && git push origin main) reports front. A cd inside (…) does not leak: cd frontend; (cd .. && true); git push stays on front, and (cd frontend; git status) && git push stays on acme.
  • With GH_HOST=ghe.example.com inherited, env -u GH_HOST gh repo view acme/app and env -i gh repo view acme/app both report github.com / acme. That matches gh requesting api.github.com.
  • sudo -u nobody ssh -F ~/cfg is {platform: "ssh", source: "unknown"}. The hook user is no longer posted for a command that ran as someone else.
  • A full gh api URL overwrites --hostname (flagHost = hostOf(url) at line 598, after the flag at line 571). On Linux, C:\ and C:/ both resolve as host c, which is the host scp dials.
  • A Match exec that exists only under $HOME/.ssh/config is not run. Default config scanning uses accountHome() (the passwd home), and ssh reads /etc/ssh/ssh_config.

if / else / while are words, so a branch that does not run is still posted

SHELL_KEYWORDS is stripped and the body keeps operator ;, so false never skips it:

    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 acme:

Command Resolver Bash
if false; then git push origin main; fi {account: acme, source: "command"} the then body does not run (ELSE only)
if true; then :; else git push origin main; fi same acme the else body does not run
while false; do git push origin main; done same acme BODY does not run

The new test covers if cd frontend; then git push, which is the branch that does run.

ssh -F ~/ follows $HOME; OpenSSH follows the passwd home

Default config and Include use accountHome(). The -F value does not. expandHome defaults to homedir(), which is $HOME:

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 HOME pointed at a directory whose cfg sets HostName 10.4.4.4 and User fromhome, ssh -F ~/cfg example.invalid is posted as {host: "10.4.4.4", user: "fromhome", source: "command"}. The same HOME with real ssh -G -F ~/cfg exits 255: Can't open user config file /home/ubuntu/cfg.

Still in the resolver, shorter

  • echo $(false) && git push origin main returns no account. Bash runs the push: echo $(false) succeeds, so && runs. The false inside $(…) is what skips the lookup.
  • A later git remote or git config rewrite in the same turn clears state.pushResults and leaves state.remotes in place (gitRemotes returns the cached map at line 414). A following gh in that turn can still name the old owner.
  • git -C/other push origin is still read from the session repo. Git 2.43 rejects the attached form (unknown option: -C/tmp, exit 129) and does not push. Spaced git -C /other is unchanged and correct.
  • Match host executive still forces unknown: the scanner treats any argument that starts with exec as Match exec, and ssh -G runs nothing for that word.
Open in Web Open in Cursor 

ghost left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Security consensus: no issues found. (reviewers: claude, semgrep, gitleaks)


🤖 consensus review · reviewers: Claude, Semgrep, Gitleaks · head f7a59754 · 2026-10-21T01:48Z

ghost left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Fix All in Cursor

❌ 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]));
}

ghost Oct 21, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Fix in Cursor Fix in Web

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]);

ghost Oct 21, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit f7a5975. Configure here.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants