fix(omp): reject ssh:// query/fragment and non-POSIX login shells (#3553 review)
remotePathFromUrl now rejects a URL query string or fragment, so a mistyped ssh://host/tmp/a?draft no longer silently operates on /tmp/a; a literal ?/# in a remote filename must be percent-encoded (%3F/%23). ssh:// transfers now require a sh/bash/zsh login shell. fish can't parse the POSIX transfer snippets and csh/tcsh apply ! history expansion to the command line, so they're refused (use the ssh tool). Host-shell detection no longer misclassifies fish/csh/tcsh as sh (basename allowlist over endsWith), and HOST_INFO_VERSION is bumped to re-probe caches that stored the old classification.
This commit is contained in:
Tommaso Fontana
@@ -1,6 +1,9 @@
|
||||
import { describe, expect, it } from "bun:test";
|
||||
import { buildRemoteCommand, type SSHConnectionTarget } from "../connection-manager";
|
||||
import { buildSshTarget } from "../utils";
|
||||
import * as fs from "node:fs";
|
||||
import * as path from "node:path";
|
||||
import { getRemoteHostDir } from "@oh-my-pi/pi-utils";
|
||||
import { buildRemoteCommand, getHostInfo, type SSHConnectionTarget, type SSHHostShell } from "../connection-manager";
|
||||
import { buildSshTarget, sanitizeHostName } from "../utils";
|
||||
|
||||
const TARGET: SSHConnectionTarget = { name: "h", host: "h" };
|
||||
|
||||
@@ -36,3 +39,31 @@ describe("buildSshTarget argument-injection guard", () => {
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
describe("ssh host shell classification", () => {
|
||||
it("treats fish/csh/tcsh as non-POSIX (unknown) and keeps real sh-family as sh", async () => {
|
||||
// parseHostInfo re-runs parseShell on the stored shell field, so getHostInfo
|
||||
// exercises the classifier through a public seam. The ensurePosixRemote
|
||||
// whitelist then refuses anything that isn't sh/bash/zsh.
|
||||
const cases: Array<[string, SSHHostShell]> = [
|
||||
["/usr/bin/fish", "unknown"],
|
||||
["/bin/csh", "unknown"],
|
||||
["/bin/tcsh", "unknown"],
|
||||
["/bin/dash", "sh"],
|
||||
["/bin/sh", "sh"],
|
||||
["/usr/bin/bash", "bash"],
|
||||
["/usr/bin/zsh", "zsh"],
|
||||
];
|
||||
for (const [shellValue, expected] of cases) {
|
||||
const name = `omp-shellclf-${crypto.randomUUID()}`;
|
||||
const file = path.join(getRemoteHostDir(), `${sanitizeHostName(name)}.json`);
|
||||
await Bun.write(file, JSON.stringify({ version: 3, os: "linux", shell: shellValue, compatEnabled: false }));
|
||||
try {
|
||||
const info = await getHostInfo(name);
|
||||
expect(info?.shell).toBe(expected);
|
||||
} finally {
|
||||
await fs.promises.rm(file, { force: true });
|
||||
}
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
@@ -26,4 +26,43 @@ describe("ssh file-transfer POSIX guard", () => {
|
||||
expect(ensureConnectionSpy).toHaveBeenCalled();
|
||||
expect(ensureHostInfoSpy).toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("rejects a non-POSIX login shell (csh/tcsh/fish classify as non-sh) before any transfer", async () => {
|
||||
// csh/tcsh history-expand `!`, and fish can't parse our POSIX source; all
|
||||
// classify as a non-sh shell, so the guard must refuse them before any spawn.
|
||||
vi.spyOn(connectionManager, "ensureConnection").mockResolvedValue(undefined);
|
||||
vi.spyOn(connectionManager, "ensureHostInfo").mockResolvedValue({
|
||||
version: 3,
|
||||
os: "linux",
|
||||
shell: "unknown",
|
||||
compatEnabled: false,
|
||||
});
|
||||
const target: SSHConnectionTarget = { name: "fishbox", host: "fishbox" };
|
||||
await expect(readRemoteFile(target, "/etc/hosts", { maxBytes: 1024 })).rejects.toThrow(/non-POSIX login shell/);
|
||||
await expect(writeRemoteFile(target, "/tmp/x", new Uint8Array([1]), {})).rejects.toThrow(/non-POSIX login shell/);
|
||||
});
|
||||
|
||||
it("allows a POSIX login shell (sh/bash/zsh) to run the transfer commands directly", async () => {
|
||||
// A POSIX login shell runs our snippets verbatim; the guard must let it through.
|
||||
// Reject at buildRemoteCommand to capture the command before any real ssh spawn.
|
||||
vi.spyOn(connectionManager, "ensureConnection").mockResolvedValue(undefined);
|
||||
vi.spyOn(connectionManager, "ensureHostInfo").mockResolvedValue({
|
||||
version: 3,
|
||||
os: "linux",
|
||||
shell: "sh",
|
||||
compatEnabled: false,
|
||||
});
|
||||
const buildSpy = vi
|
||||
.spyOn(connectionManager, "buildRemoteCommand")
|
||||
.mockRejectedValue(new Error("stop-before-spawn"));
|
||||
const target: SSHConnectionTarget = { name: "shbox", host: "shbox" };
|
||||
|
||||
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/);
|
||||
|
||||
// Reached buildRemoteCommand → the guard allowed the POSIX shell. Commands are
|
||||
// sent verbatim (no `sh -c` wrapper); write keeps its stdin staging.
|
||||
expect(buildSpy.mock.calls[0]?.[1]).toContain("head -c 1025");
|
||||
expect(buildSpy.mock.calls[1]?.[2]).toMatchObject({ allowStdin: true });
|
||||
});
|
||||
});
|
||||
|
||||
@@ -32,7 +32,7 @@ export interface SSHHostInfo {
|
||||
const CONTROL_DIR = getSshControlDir();
|
||||
const CONTROL_PATH = path.join(CONTROL_DIR, "%C.sock");
|
||||
const HOST_INFO_DIR = getRemoteHostDir();
|
||||
const HOST_INFO_VERSION = 2;
|
||||
const HOST_INFO_VERSION = 3;
|
||||
|
||||
const activeHosts = new Map<string, SSHConnectionTarget>();
|
||||
const pendingConnections = new Map<string, Promise<void>>();
|
||||
@@ -153,7 +153,11 @@ function parseShell(value: unknown): SSHHostShell | null {
|
||||
if (normalized.includes("zsh")) return "zsh";
|
||||
if (normalized.includes("pwsh") || normalized.includes("powershell")) return "powershell";
|
||||
if (normalized.includes("cmd.exe") || normalized === "cmd") return "cmd";
|
||||
if (normalized.endsWith("sh") || normalized.includes("/sh")) return "sh";
|
||||
// Only genuine POSIX sh-family by basename — fish/csh/tcsh also end in "sh"
|
||||
// but are non-POSIX (csh/tcsh history-expand `!`), so they fall through to
|
||||
// "unknown" and are refused by the ssh:// transfer guard.
|
||||
const base = normalized.slice(normalized.lastIndexOf("/") + 1);
|
||||
if (base === "sh" || base === "dash" || base === "ash" || base === "ksh" || base === "mksh") return "sh";
|
||||
return "unknown";
|
||||
}
|
||||
|
||||
@@ -295,18 +299,9 @@ async function probeHostInfo(host: SSHConnectionTarget): Promise<SSHHostInfo> {
|
||||
os = "linux";
|
||||
}
|
||||
|
||||
let shell: SSHHostShell = "unknown";
|
||||
if (shellLower.includes("bash")) {
|
||||
shell = "bash";
|
||||
} else if (shellLower.includes("zsh")) {
|
||||
shell = "zsh";
|
||||
} else if (shellLower.includes("pwsh") || shellLower.includes("powershell")) {
|
||||
shell = "powershell";
|
||||
} else if (shellLower.includes("cmd.exe") || shellLower === "cmd") {
|
||||
shell = "cmd";
|
||||
} else if (shellLower.endsWith("sh") || shellLower.includes("/sh")) {
|
||||
shell = "sh";
|
||||
} else if (os === "windows" && !shellLower) {
|
||||
// Reuse parseShell so probe-time and cached classification stay identical.
|
||||
let shell = parseShell(shellLower) ?? "unknown";
|
||||
if (shell === "unknown" && os === "windows" && !shellLower) {
|
||||
shell = "cmd";
|
||||
}
|
||||
|
||||
|
||||
@@ -14,8 +14,12 @@ import { quotePosixPath } from "./utils";
|
||||
const DEFAULT_TIMEOUT_MS = 30_000;
|
||||
|
||||
/**
|
||||
* Ensure the ControlMaster connection and reject non-POSIX (Windows) remotes,
|
||||
* which the POSIX `head`/`cat`/`mv`/`ls` commands in this module cannot drive.
|
||||
* Ensure the ControlMaster connection and restrict transfers to remotes whose
|
||||
* *login* shell runs our POSIX snippets directly. OpenSSH hands each command to
|
||||
* `$SHELL -c`, so the login shell must be POSIX: Windows (cmd/powershell) can't
|
||||
* drive `head`/`cat`/`mv`, and csh/tcsh apply `!` history expansion to the
|
||||
* command line. `ensureHostInfo` classifies those (and fish) as a non-sh shell,
|
||||
* so accept only sh, bash, and zsh; anything else is refused here.
|
||||
*/
|
||||
async function ensurePosixRemote(target: SSHConnectionTarget): Promise<void> {
|
||||
await ensureConnection(target);
|
||||
@@ -25,6 +29,11 @@ async function ensurePosixRemote(target: SSHConnectionTarget): Promise<void> {
|
||||
`ssh://: ${target.name} is a Windows host; ssh:// supports POSIX remotes only (head/cat/mv) — use the ssh tool for Windows hosts`,
|
||||
);
|
||||
}
|
||||
if (info.shell !== "sh" && info.shell !== "bash" && info.shell !== "zsh") {
|
||||
throw new Error(
|
||||
`ssh://: ${target.name} uses a non-POSIX login shell (${info.shell}); ssh:// read/write needs sh, bash, or zsh — use the ssh tool for this host`,
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
export interface RemoteFileReadOptions {
|
||||
|
||||
Reference in New Issue
Block a user