fix(ssh): dispatch transfer snippets through verified transferShell
The previous fix gated on a verified `transferShell` but still let OpenSSH hand the snippet to whatever `$SHELL` happens to be on the remote. On a fish/csh/tcsh host the new gate would accept the host, then fail anyway because the login shell can't parse `if [ ... ]; then ...` (P1 from PR #3722 review). Each transfer command (read, write, stat, list) is now wrapped in `<transferShell> -c '...'` via a shared `wrapInPosixShell` helper, so the snippet is parsed by the same shell OMP's capability probe verified can run it. `ensurePosixRemote` returns the verified shell so each call site can do the wrap. `ssh-executor.ts`'s identical Windows-compat helper is consolidated onto the same primitive (`buildCompatCommand` / `quoteForCompatShell` removed). Stays POSIX-clean across all four call sites; `-c` (not `-lc`) since the snippets only call absolute builtins and don't need login-profile setup. Capability *probing* still uses `-lc` to mirror the user env. Test additions: one case asserts every dispatch starts with `bash -c '...'` and embeds the original POSIX snippet (read/write/stat/list) when transferShell is bash and login shell is unknown; another covers the `sh -c` happy path.
This commit is contained in:
roboomp
@@ -1,7 +1,7 @@
|
||||
import { afterEach, describe, expect, it, vi } from "bun:test";
|
||||
import type { SSHConnectionTarget } from "../connection-manager";
|
||||
import * as connectionManager from "../connection-manager";
|
||||
import { readRemoteFile, writeRemoteFile } from "../file-transfer";
|
||||
import { listRemoteDir, readRemoteFile, statRemotePath, writeRemoteFile } from "../file-transfer";
|
||||
|
||||
describe("ssh file-transfer POSIX guard", () => {
|
||||
afterEach(() => {
|
||||
@@ -46,15 +46,18 @@ describe("ssh file-transfer POSIX guard", () => {
|
||||
);
|
||||
});
|
||||
|
||||
it("allows a non-Windows remote whose transferShell is verified even when login shell is unknown", async () => {
|
||||
// The bug fix: ssh:// must accept a POSIX-capable host even when the
|
||||
// first-line probe couldn't classify the login shell, as long as a
|
||||
// capability probe verified sh/bash/zsh works. Stop at buildRemoteCommand
|
||||
// so we capture the dispatch without spawning a real ssh.
|
||||
it("dispatches transfer commands through the verified transferShell, not the login shell", async () => {
|
||||
// The bug fix: if the login shell is fish/csh/tcsh, the legacy guard
|
||||
// would refuse the host — but allowing it isn't enough on its own.
|
||||
// OpenSSH still hands our snippets to `$SHELL -c`, so a fish login
|
||||
// shell would choke on `if [ … ]; then …`. Every transfer command
|
||||
// must be wrapped in `<transferShell> -c '…'` to force parsing
|
||||
// under the shell we verified can run it (#3719).
|
||||
vi.spyOn(connectionManager, "ensureConnection").mockResolvedValue(undefined);
|
||||
vi.spyOn(connectionManager, "ensureHostInfo").mockResolvedValue({
|
||||
version: 4,
|
||||
os: "linux",
|
||||
// Login shell is fish; only `transferShell` indicates a working POSIX shell.
|
||||
shell: "unknown",
|
||||
transferShell: "bash",
|
||||
compatEnabled: false,
|
||||
@@ -62,20 +65,27 @@ describe("ssh file-transfer POSIX guard", () => {
|
||||
const buildSpy = vi
|
||||
.spyOn(connectionManager, "buildRemoteCommand")
|
||||
.mockRejectedValue(new Error("stop-before-spawn"));
|
||||
const target: SSHConnectionTarget = { name: "shellnoise", host: "shellnoise" };
|
||||
const target: SSHConnectionTarget = { name: "fishbox", host: "fishbox" };
|
||||
|
||||
await expect(readRemoteFile(target, "/etc/hosts", { maxBytes: 1024 })).rejects.toThrow(/stop-before-spawn/);
|
||||
await expect(writeRemoteFile(target, "/tmp/x", new Uint8Array([1]), {})).rejects.toThrow(/stop-before-spawn/);
|
||||
await expect(statRemotePath(target, "/etc/hosts")).rejects.toThrow(/stop-before-spawn/);
|
||||
await expect(listRemoteDir(target, "/etc")).rejects.toThrow(/stop-before-spawn/);
|
||||
|
||||
// Reached buildRemoteCommand → the guard let us through despite the
|
||||
// login-shell being "unknown". Commands are still sent verbatim (no
|
||||
// `sh -c` wrapper) and write keeps its stdin staging.
|
||||
expect(buildSpy.mock.calls[0]?.[1]).toContain("head -c 1025");
|
||||
// Each dispatch must start with `bash -c '…'` and embed the original
|
||||
// POSIX snippet inside the quoted command. Read also drops `-n`
|
||||
// (allowStdin: true) because cat-staging needs stdin streaming.
|
||||
const dispatches = buildSpy.mock.calls.map(call => call[1] as string);
|
||||
expect(dispatches[0]).toMatch(/^bash -c '.*head -c 1025/);
|
||||
expect(dispatches[1]).toMatch(/^bash -c '.*cat > /);
|
||||
expect(buildSpy.mock.calls[1]?.[2]).toMatchObject({ allowStdin: true });
|
||||
expect(dispatches[2]).toMatch(/^bash -c '.*if \[ -d /);
|
||||
expect(dispatches[3]).toMatch(/^bash -c '.*LC_ALL=C ls -1Ap /);
|
||||
});
|
||||
|
||||
it("allows a POSIX login shell when transferShell is also set", async () => {
|
||||
// Belt-and-suspenders: the common happy path where both fields agree.
|
||||
it("uses sh -c when transferShell is sh (the most universal POSIX fallback)", async () => {
|
||||
// Belt-and-suspenders: the common happy path with a sh-family login
|
||||
// shell still routes through `sh -c` to keep one dispatch shape.
|
||||
vi.spyOn(connectionManager, "ensureConnection").mockResolvedValue(undefined);
|
||||
vi.spyOn(connectionManager, "ensureHostInfo").mockResolvedValue({
|
||||
version: 4,
|
||||
@@ -90,6 +100,6 @@ describe("ssh file-transfer POSIX guard", () => {
|
||||
const target: SSHConnectionTarget = { name: "shbox", host: "shbox" };
|
||||
|
||||
await expect(readRemoteFile(target, "/etc/hosts", { maxBytes: 1024 })).rejects.toThrow(/stop-before-spawn/);
|
||||
expect(buildSpy.mock.calls[0]?.[1]).toContain("head -c 1025");
|
||||
expect(buildSpy.mock.calls[0]?.[1]).toMatch(/^sh -c '.*head -c 1025/);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -8,23 +8,24 @@
|
||||
*/
|
||||
import { ptree } from "@oh-my-pi/pi-utils";
|
||||
import { buildRemoteCommand, ensureConnection, ensureHostInfo, type SSHConnectionTarget } from "./connection-manager";
|
||||
import { quotePosixPath } from "./utils";
|
||||
import { quotePosixPath, wrapInPosixShell } from "./utils";
|
||||
|
||||
/** Per-operation timeout for remote transfers (matches the ssh tool's grep window). */
|
||||
const DEFAULT_TIMEOUT_MS = 30_000;
|
||||
|
||||
/**
|
||||
* Ensure the ControlMaster connection and restrict transfers to remotes where
|
||||
* OMP has verified a working POSIX transfer shell (`info.transferShell`).
|
||||
* Ensure the ControlMaster connection and pick the verified POSIX shell to
|
||||
* run transfer commands under. Returns the shell name so the caller can
|
||||
* wrap its snippet in `<shell> -c '…'`; OpenSSH otherwise hands the command
|
||||
* to the user's login shell, which on fish/csh/tcsh hosts can't parse our
|
||||
* `if [ … ]; then …` constructs (#3719).
|
||||
*
|
||||
* Windows hosts are refused up front — `ssh://` runs `head`/`cat`/`mv`/`test`
|
||||
* directly, and cmd/powershell can't drive those. Everywhere else, we trust
|
||||
* the capability probe (`sh -lc` / `bash -lc` / `zsh -lc` against the remote)
|
||||
* over the self-reported login-shell name, because login-shell classification
|
||||
* can be wrong or noisy (#3719). When the probe couldn't verify any candidate,
|
||||
* `transferShell` is `undefined` and the guard rejects.
|
||||
* directly and cmd/powershell can't drive those. Everywhere else, we require
|
||||
* a non-empty `transferShell` (set by `probeHostInfo` after `sh -lc` /
|
||||
* `bash -lc` / `zsh -lc` round-trips a marker against the remote).
|
||||
*/
|
||||
async function ensurePosixRemote(target: SSHConnectionTarget): Promise<void> {
|
||||
async function ensurePosixRemote(target: SSHConnectionTarget): Promise<"sh" | "bash" | "zsh"> {
|
||||
await ensureConnection(target);
|
||||
const info = await ensureHostInfo(target);
|
||||
if (info.os === "windows") {
|
||||
@@ -37,6 +38,7 @@ async function ensurePosixRemote(target: SSHConnectionTarget): Promise<void> {
|
||||
`ssh://: ${target.name} has no verified POSIX shell for ssh:// read/write — none of sh/bash/zsh round-tripped a capability probe (use the ssh tool for this host)`,
|
||||
);
|
||||
}
|
||||
return info.transferShell;
|
||||
}
|
||||
|
||||
export interface RemoteFileReadOptions {
|
||||
@@ -70,9 +72,9 @@ export async function readRemoteFile(
|
||||
remotePath: string,
|
||||
opts: RemoteFileReadOptions,
|
||||
): Promise<RemoteFileReadResult> {
|
||||
await ensurePosixRemote(target);
|
||||
const shell = await ensurePosixRemote(target);
|
||||
const command = `head -c ${opts.maxBytes + 1} ${quotePosixPath(remotePath)}`;
|
||||
const args = await buildRemoteCommand(target, command);
|
||||
const args = await buildRemoteCommand(target, wrapInPosixShell(shell, command));
|
||||
using child = ptree.spawn(["ssh", ...args], {
|
||||
signal: ptree.combineSignals(opts.signal, opts.timeoutMs ?? DEFAULT_TIMEOUT_MS),
|
||||
});
|
||||
@@ -113,7 +115,7 @@ export async function writeRemoteFile(
|
||||
content: Uint8Array,
|
||||
opts: RemoteFileWriteOptions,
|
||||
): Promise<void> {
|
||||
await ensurePosixRemote(target);
|
||||
const shell = await ensurePosixRemote(target);
|
||||
if (remotePath.endsWith("/")) {
|
||||
throw new Error("ssh://: destination is a directory path (trailing '/'); ssh:// write requires a file path");
|
||||
}
|
||||
@@ -138,7 +140,7 @@ export async function writeRemoteFile(
|
||||
`elif [ -e ${dest} ] && [ ! -L ${dest} ]; then echo 'ssh://: destination is a special file (not a regular file)' >&2; exit 1; ` +
|
||||
`else mv "$t" ${dest}; fi; ` +
|
||||
`}`;
|
||||
const args = await buildRemoteCommand(target, command, { allowStdin: true });
|
||||
const args = await buildRemoteCommand(target, wrapInPosixShell(shell, command), { allowStdin: true });
|
||||
using child = ptree.spawn(["ssh", ...args], {
|
||||
stdin: content,
|
||||
signal: ptree.combineSignals(opts.signal, opts.timeoutMs ?? DEFAULT_TIMEOUT_MS),
|
||||
@@ -158,10 +160,10 @@ export async function statRemotePath(
|
||||
remotePath: string,
|
||||
opts: { signal?: AbortSignal; timeoutMs?: number } = {},
|
||||
): Promise<RemotePathKind> {
|
||||
await ensurePosixRemote(target);
|
||||
const shell = await ensurePosixRemote(target);
|
||||
const p = quotePosixPath(remotePath);
|
||||
const command = `if [ -d ${p} ]; then echo directory; elif [ -f ${p} ]; then echo file; elif [ -e ${p} ]; then echo other; else echo missing; fi`;
|
||||
const args = await buildRemoteCommand(target, command);
|
||||
const args = await buildRemoteCommand(target, wrapInPosixShell(shell, command));
|
||||
using child = ptree.spawn(["ssh", ...args], {
|
||||
signal: ptree.combineSignals(opts.signal, opts.timeoutMs ?? DEFAULT_TIMEOUT_MS),
|
||||
});
|
||||
@@ -191,9 +193,9 @@ export async function listRemoteDir(
|
||||
remotePath: string,
|
||||
opts: { signal?: AbortSignal; timeoutMs?: number } = {},
|
||||
): Promise<RemoteDirEntry[]> {
|
||||
await ensurePosixRemote(target);
|
||||
const shell = await ensurePosixRemote(target);
|
||||
const command = `LC_ALL=C ls -1Ap -- ${quotePosixPath(remotePath)}`;
|
||||
const args = await buildRemoteCommand(target, command);
|
||||
const args = await buildRemoteCommand(target, wrapInPosixShell(shell, command));
|
||||
using child = ptree.spawn(["ssh", ...args], {
|
||||
signal: ptree.combineSignals(opts.signal, opts.timeoutMs ?? DEFAULT_TIMEOUT_MS),
|
||||
});
|
||||
|
||||
@@ -4,6 +4,7 @@ import { OutputSink } from "../session/streaming-output";
|
||||
import { resolveOutputMaxColumns, resolveOutputSinkHeadBytes } from "../tools/output-meta";
|
||||
import { buildRemoteCommand, ensureConnection, ensureHostInfo, type SSHConnectionTarget } from "./connection-manager";
|
||||
import { hasSshfs, mountRemote } from "./sshfs-mount";
|
||||
import { wrapInPosixShell } from "./utils";
|
||||
|
||||
export interface SSHExecutorOptions {
|
||||
/** Timeout in milliseconds */
|
||||
@@ -78,18 +79,6 @@ function createAbortWaiter(
|
||||
return { promise, cleanup: () => signal.removeEventListener("abort", onAbort) };
|
||||
}
|
||||
|
||||
function quoteForCompatShell(command: string): string {
|
||||
if (command.length === 0) {
|
||||
return "''";
|
||||
}
|
||||
const escaped = command.replace(/'/g, "'\\''");
|
||||
return `'${escaped}'`;
|
||||
}
|
||||
|
||||
function buildCompatCommand(shell: "bash" | "sh", command: string): string {
|
||||
return `${shell} -c ${quoteForCompatShell(command)}`;
|
||||
}
|
||||
|
||||
export async function executeSSH(
|
||||
host: SSHConnectionTarget,
|
||||
command: string,
|
||||
@@ -108,7 +97,7 @@ export async function executeSSH(
|
||||
if (options?.compatEnabled) {
|
||||
const info = await ensureHostInfo(host);
|
||||
if (info.compatShell) {
|
||||
resolvedCommand = buildCompatCommand(info.compatShell, command);
|
||||
resolvedCommand = wrapInPosixShell(info.compatShell, command);
|
||||
} else {
|
||||
logger.warn("SSH compat enabled without detected compat shell", { host: host.name });
|
||||
}
|
||||
|
||||
@@ -30,3 +30,22 @@ export function quotePosixPath(value: string): string {
|
||||
if (value.length === 0) return "''";
|
||||
return `'${value.replace(/'/g, "'\\''")}'`;
|
||||
}
|
||||
|
||||
/**
|
||||
* Wrap a POSIX command in `<shell> -c '<command>'` so it runs under the
|
||||
* named shell rather than whatever `$SHELL` happens to be on the remote.
|
||||
*
|
||||
* Used by the `ssh://` transfer helpers and the Windows compat dispatch:
|
||||
* OpenSSH passes our snippets to `<login-shell> -c`, so a remote whose
|
||||
* login shell is fish/csh/tcsh (or cmd/powershell on Windows compat)
|
||||
* can't parse `if [ … ]; then …`. Wrapping forces parsing under the
|
||||
* shell OMP actually verified can run the snippet.
|
||||
*
|
||||
* `-c` (not `-lc`): the transfer snippets only call absolute POSIX
|
||||
* builtins (`head`/`cat`/`mv`/`test`/`ls`/`mkdir`/`rm`/`dirname`) and
|
||||
* don't need login-profile setup. Capability *probing* still uses
|
||||
* `-lc` to mirror the user's real environment.
|
||||
*/
|
||||
export function wrapInPosixShell(shell: "sh" | "bash" | "zsh", command: string): string {
|
||||
return `${shell} -c ${quotePosixPath(command)}`;
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user