Repository navigation
(remote): run SWITCHBOARD_SSH_PATH for every remote ssh, and a matching scp - #371
Conversation
…ng scp Only the attach PTY read SWITCHBOARD_SSH_PATH; the mirror inventory and range fetch, the watch channel and the remote command runner (probe, stop, Changes) spawned a bare `ssh` from PATH, and the mirror copy a bare `scp`. A user who set the variable because ssh is not on PATH got an attach that worked and a mirror that did not. remote-ssh-binary.js now holds the one resolver for each binary. scp is SWITCHBOARD_SCP_PATH, else the scp beside an absolute SWITCHBOARD_SSH_PATH when it exists, else the same system candidates as ssh, else PATH. test/remote-ssh-spawn-sites.test.js parses the remote modules and lists every child-process call with its program argument; a call that does not go through the resolver, or a new call site, fails it. It also checks the default wiring of each operation with both variables set. Fixes #359
|
Reviewing |
devsuitup
left a comment
There was a problem hiding this comment.
Adversarial review at e717de7.
Completeness holds: every spawn/execFile/execSync/pty.spawn of ssh or scp in the JS at this head goes through a resolver (remote-attach.js:245, :368; remote-transport.js:221, :244, :296; remote-watch.js:139); no sftp/rsync; no shell: true anywhere; options and args unchanged (windowsHide, stdio, timeouts). A non-existent configured path fails with ENOENT rather than falling back to another ssh — the safer choice.
Blocking — the transcript copy still connects through the PATH ssh. fetchOne (remote-transport.js:244) runs resolveScpPath() with [...SSH_BASE_OPTS, '-p', '-q', remote, tmpPath] and no -S. OpenSSH's scp spawns its own ssh (compiled-in path / PATH) unless given -S <program>, so with SWITCHBOARD_SSH_PATH pointing at a wrapper or another OpenSSH, inventory, range fetch, watch and commands use it while the copy uses a different ssh — possibly different config, keys, ProxyCommand, agent. That is the #359 defect for one path, and docs/remote-hosts.md / CHANGELOG.md say the variable now applies to every connection. Fix: pass -S, resolveSshPath()` to scp (or narrow the wording to "the scp binary"). Not tested against a real scp.
Non-blocking:
- Relative
SWITCHBOARD_SSH_PATH(e.g../wrap/ssh): the attach PTY runs withcwd: os.homedir()(main.js:554), thechild_processsites with the app's cwd — one value can name two binaries, and the scp-beside derivation is skipped. Require an absolute path in the docs, orpath.resolveonce. resolveSshPath/resolveScpPathcallexistsSyncon every spawn (up to 3 per transcript file for scp); a UNC path on Windows could stall the main thread. Memoise per process, or probe only when the variable is unset.test/remote-ssh-spawn-sites.test.jsguardsremote-*.jswell (AST walk, exact program args) but outside them only catches literal first arguments named exactlyssh/scp(.exe);const SSH = 'ssh'; spawn(SSH, …)inmain.jswould pass. Its header comment (lines 3-12) is a design note (comment-sweep rule).- Whitespace-only value is truthy and fails with ENOENT (an empty one is ignored and tested).
Not blocking for me: the tests and the fix are one commit, so there is no CI red run; locally, the spawn-sites test against origin/main's three remote modules fails 8 of 9 (only the attach PTY, which already honoured the variable, passes) — that is sufficient evidence.
Verified: node --test remote-ssh-binary 12/12, remote-ssh-spawn-sites 9/9, remote-transport 33/33, remote-transport-incremental 7/7; CI green on this head. No real ssh connection made.
…n-site test scp starts its own ssh from a path compiled into it, so a wrapper named by SWITCHBOARD_SSH_PATH never carried the copies; fetchOne now passes `-S <resolved ssh>`. Resolution order is SWITCHBOARD_SSH_PATH, then ssh from PATH, then the system candidates, for every operation, attach included: the mirror, watch, stop and Changes ran ssh from PATH, and a Homebrew OpenSSH or Git's ssh on Windows must keep being found first. scp: SWITCHBOARD_SCP_PATH, the scp beside SWITCHBOARD_SSH_PATH, PATH, system. The PATH is searched by the resolver itself, so every consumer gets the same absolute path. The variables are trimmed and a blank one is unset; a relative value is ignored with a warning, since the attach PTY and the child_process sites run with different cwds; a Windows .cmd/.bat logs that it cannot be spawned without a shell. The result is memoised per process, since each lookup probes the disk and scp runs once per transcript file. The spawn-site test missed renamed imports, member calls on the require result and callers of exported wrappers. It now follows values with eslint-scope across every main-process module, fails on any site whose program it cannot resolve outside a named allowlist, and is tested on fixtures for each evasion. Its design note moves to session-cache.md.
…awn-site test
A memoised ssh or scp that disappears (an upgrade, a removed Homebrew
keg) gave ENOENT until a restart. A path the search found is now probed
again on each call, one stat of a known file, and searched for again once
it is gone. A configured value and the bare-name fallback are not
re-checked.
The Windows PATH test answered with a path that is also a system
candidate, so it never proved the PATH was searched; it now expects an
ssh.exe in a directory that is not one. New tests run the real probe on
a directory named ssh and on a non-executable ssh file, and check that
setResolverLog applies to a resolver that already ran.
The scanner did not recognise require('node:child_process') or
node:path; module names are now matched without the node: prefix. The
program-name check flagged any call whose first argument was 'ssh', such
as String(s).endsWith('ssh'); it now looks only at calls that start a
process, including the known wrappers runToExit and spawnPty.
process.execPath counts as a known program. Each failure message says
what to change.
espree and eslint-scope were reached only through eslint; they are now
devDependencies at the versions already locked.
|
Reviewing |
devsuitup
left a comment
There was a problem hiding this comment.
Re-review at 7dd0ccc (delta from e717de7: 0ce0329 and 7dd0ccc around the merge of main). Approve — CI green on this head (the test (22, windows-2022) failure was the known Git Bash crash in pre-launch-cmd-guard, #342; the re-run passed).
Round 1: the blocker is fixed — remote-transport.js:244 runs resolveScpPath() with -S resolveSshPath(), and dropping -S turns the mirror copy test red. A relative SWITCHBOARD_SSH_PATH is now ignored with a warning; a whitespace-only value counts as unset; the spawn-site test walks the AST with scope analysis instead of matching literals. Every ssh/scp spawn (remote-attach.js:245,368, remote-transport.js:221,244,296, remote-watch.js:139) goes through the resolver; no rsync/sftp. The merge dropped nothing from main (two-dot and three-dot stats match).
Resolution order: SWITCHBOARD_SSH_PATH (absolute, wins) → absolute PATH entries → system locations → bare ssh; scp: SWITCHBOARD_SCP_PATH → scp beside the configured ssh → PATH → system. A PATH-found binary is re-probed before reuse (~2 stats per spawn), a configured one is not.
Mutations: bare ssh in remote-watch.js, bare scp at :244, -S dropped, and new modules spawning ssh via a literal, a concatenated constant, a config field, an exec shell string or runToExit — each turns tests red. 56/56 locally (2 Linux-only skipped).
Non-blocking:
- With
-S, scp passes ssh flags (-x -oClearAllForwardings=yes -- host scp -f …) to the program; aSWITCHBOARD_SSH_PATHwrapper must accept them — worth a line indocs/remote-hosts.md. - A Windows path with spaces reaches scp's
-Sas one argv element; whether Win32-OpenSSH scp re-quotes it for CreateProcess is untested. pre-launch-cmd-guard.test.jsandcli-session-procstart-batch.test.jsedits are unrelated to #359 — name them in the description.
Nit: the spawn-site scan covers the repo root and workers/ only; a future module in another subdirectory would be missed.
Handing overdevsuitup takes this PR from here. State at
CI is green and main is merged in. What is left:
|
Closes #359.
Only the attach PTY read
SWITCHBOARD_SSH_PATH. The mirror inventory and range fetch, the watch channel and the remote command runner (probe, stop, Changes) spawned a baresshfrom PATH, and the mirror copy spawned a barescp.remote-ssh-binary.jsholds one resolver per binary, used by every remote spawn.SWITCHBOARD_SCP_PATH; thescpbeside an absoluteSWITCHBOARD_SSH_PATH, when it exists; the same system candidates as ssh; then PATH.test/remote-ssh-spawn-sites.test.jsparses the remote modules and lists every child-process call with its program argument. A call that bypasses the resolver, or a new call site, fails it.docs/remote-hosts.md("Which ssh and scp run") anddocs/settings.mddescribe both variables.Tests: the new tests went 1 pass / 9 fail before the fix. Full suite: 2508 + 120, 0 failures. Six mutations of the resolver wiring were each caught.