diff --git a/CHANGELOG.md b/CHANGELOG.md index 4c27cde..58ecb16 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,28 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- **MCP `ssh_command` now auto-approves read-only pipelines and three more commands.** + Unquoted pipes between allowlisted commands no longer need `confirm: true` — + previously every token was shell-quoted, so `find /home/web/monitoring -mtime +90 | wc -l` + was both rejected by the read-only check and, worse, silently sent to the remote + host with the pipe as a literal argument (the pipeline never ran). `find`, `awk`, + and `wc` join the allowlist: `find` is refused when it carries mutating primaries + (`-delete`, `-exec`, `-execdir`, `-ok`, `-okdir`, `-fprint*`, `-fls`), and `awk` + is refused when its program text contains escape hatches (`system()`, `getline`, + `close()`, `|`, or output redirection) or when it carries any option other than + `-F`/`-v` (gawk's `-i inplace`, `-o`, `--profile`, `-f` and friends can write + files or load code). A pipeline is auto-approved only when + every segment passes on its own; leading, trailing, or doubled pipes are + rejected. Quoted pipes (`"a|b"` as an argument) keep their literal meaning. + +### Fixed + +- **MCP `ssh_command` no longer drops empty quoted arguments.** `grep '' file` + was sent to the remote host as `grep file`, changing what the command did. + The tokenizer now keeps `''` and `""` as real empty arguments. + ## [5.41.0] - 2026-10-28 ### Removed diff --git a/mcp-server/package.json b/mcp-server/package.json index d600288..e1d9471 100644 --- a/mcp-server/package.json +++ b/mcp-server/package.json @@ -11,7 +11,8 @@ "build": "tsc", "prestart": "tsc", "start": "node dist/index.js", - "dev": "tsx src/index.ts" + "dev": "tsx src/index.ts", + "test": "tsx --test test/*.test.ts" }, "dependencies": { "@modelcontextprotocol/sdk": "^1.12.0", diff --git a/mcp-server/src/tools/sshPassthrough.ts b/mcp-server/src/tools/sshPassthrough.ts index cc0b744..8cc564f 100644 --- a/mcp-server/src/tools/sshPassthrough.ts +++ b/mcp-server/src/tools/sshPassthrough.ts @@ -14,12 +14,14 @@ export interface SshCommandResult { // needs confirm: true. Each token is shell-quoted before being sent to ssh, so // even a command like "cat file > other" is treated as literal arguments to cat. const READ_ONLY_COMMANDS = new Set([ + "awk", "cat", "date", "df", "dmesg", "du", "file", + "find", "free", "grep", "head", @@ -42,22 +44,84 @@ const READ_ONLY_COMMANDS = new Set([ "uname", "uptime", "vmstat", + "wc", "whoami", ]); +// find mutates remote state through primaries like -delete and -exec, so it is +// only read-only when none of those appear in its arguments. +const FIND_MUTATING_PRIMARIES = new Set([ + "-delete", + "-exec", + "-execdir", + "-ok", + "-okdir", + "-fprint", + "-fprint0", + "-fprintf", + "-fls", +]); + +// awk can execute arbitrary remote commands via system()/| and write files via +// redirection inside its program text, so it is only allowed without any of +// those escape hatches. +const AWK_ESCAPE_PATTERNS = [/system\s*\(/, /\|/, /getline/, /\bclose\s*\(/, />>/, /(^|[^<>])>/]; + +function isReadOnlySegment(base: string, tokens: string[]): boolean { + if (!READ_ONLY_COMMANDS.has(base)) return false; + if (touchesCredentialPath(tokens)) return false; + if (base === "find") { + return !tokens + .slice(1) + .some((token) => FIND_MUTATING_PRIMARIES.has(token)); + } + if (base === "awk") { + return tokens.slice(1).every((token) => { + // -F (field separator) and -v (variable) are the only options that stay + // read-only. gawk's -i inplace, -o/--pretty-print, --profile, -f progfile + // and -l/-E/--include can write files or load code, so any other option + // needs confirm: true. A bare "-" (stdin) is a plain operand. + if (/^-[Fv]/.test(token)) return true; + if (token.startsWith("-") && token !== "-") return false; + return !AWK_ESCAPE_PATTERNS.some((pattern) => pattern.test(token)); + }); + } + return true; +} + // Split a simple shell-like command string into tokens, respecting single and // double quotes and backslash escapes. This is intentionally minimal: it lets // callers pass arguments with spaces ("ls -la /some dir") without invoking a // local shell or letting ssh re-interpret metacharacters. -function tokenizeCommand(command: string): string[] { - const tokens: string[] = []; +// An unquoted "|" becomes a pipe operator token, so callers can validate and +// rebuild pipelines. A quoted or escaped "|" ("echo 'a|b'") is a plain argument +// like any other, as is an operator glued to other text ("a|b") — only a +// standalone unquoted "|" counts. +export type Token = { value: string; isPipe: boolean }; + +export function tokenizeCommand(command: string): Token[] { + const tokens: Token[] = []; let current = ""; + let currentQuoted = false; let quote: "'" | '"' | null = null; let escaped = false; + const flush = () => { + // A quoted empty string ("" or '') is a real, empty argument; only drop + // the buffer when nothing was typed at all. + if (current === "" && !currentQuoted) return; + tokens.push({ + value: current, + isPipe: current === "|" && !currentQuoted, + }); + current = ""; + currentQuoted = false; + }; + for (const char of command) { if (escaped) { current += char; + currentQuoted = true; escaped = false; continue; } @@ -74,14 +138,12 @@ function tokenizeCommand(command: string): string[] { continue; } if (char === '"' || char === "'") { + currentQuoted = true; quote = char; continue; } if (/\s/.test(char)) { - if (current !== "") { - tokens.push(current); - current = ""; - } + flush(); continue; } current += char; @@ -89,14 +151,30 @@ function tokenizeCommand(command: string): string[] { if (escaped) { current += "\\"; + currentQuoted = true; } - if (current !== "") { - tokens.push(current); - } + flush(); return tokens; } +// Split a token stream into pipeline segments at unquoted pipe operators. +// Returns null when a pipe is leading, trailing, or doubled — those are shell +// syntax errors or constructs this minimal parser should not approve. +function splitPipeline(tokens: Token[]): Token[][] | null { + const segments: Token[][] = [[]]; + for (const token of tokens) { + if (token.isPipe) { + if (segments[segments.length - 1].length === 0) return null; + segments.push([]); + continue; + } + segments[segments.length - 1].push(token); + } + if (segments[segments.length - 1].length === 0) return null; + return segments; +} + // Path fragments that indicate a credential-bearing file. Even a command in // READ_ONLY_COMMANDS (cat, grep, head, tail, ...) needs confirm: true when any // argument targets one of these — "cat" being harmless in general doesn't mean @@ -122,14 +200,26 @@ function touchesCredentialPath(tokens: string[]): boolean { .some((token) => CREDENTIAL_PATH_PATTERNS.some((pattern) => pattern.test(token))); } +// Read-only when the command is a simple command or a pipeline whose every +// segment is read-only on its own. A pipe is only sent to the remote shell as +// an operator if all segments pass, so "find ... | wc -l" can be auto-approved +// while "find ... -delete" or "awk '...system(...)'" cannot. export function isReadOnlySshCommand(command: string): boolean { const tokens = tokenizeCommand(command); - const first = tokens[0]; - if (!first) return false; - const base = path.basename(first); - if (!READ_ONLY_COMMANDS.has(base)) return false; - if (touchesCredentialPath(tokens)) return false; - return true; + if (tokens.length === 0) return false; + const segments = splitPipeline(tokens); + if (!segments) return false; + return segments.every((segment) => { + const base = path.basename(segment[0].value); + return isReadOnlySegment(base, segment.map((t) => t.value)); + }); +} + +// Rebuild the remote command line: every token shell-quoted, except validated +// pipe operators, which are passed through unquoted so the remote shell runs +// them as a pipeline. +export function buildRemoteCommand(tokens: Token[]): string { + return tokens.map((t) => (t.isPipe ? "|" : shellQuote(t.value))).join(" "); } export function runSshCommand(entry: EnvEntry, command: string): Promise { @@ -143,7 +233,7 @@ export function runSshCommand(entry: EnvEntry, command: string): Promise { const child = spawn("ssh", [sshHost, remoteCommand]); diff --git a/mcp-server/test/sshPassthrough.test.ts b/mcp-server/test/sshPassthrough.test.ts new file mode 100644 index 0000000..afa4b74 --- /dev/null +++ b/mcp-server/test/sshPassthrough.test.ts @@ -0,0 +1,78 @@ +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { + buildRemoteCommand, + isReadOnlySshCommand, + tokenizeCommand, +} from "../src/tools/sshPassthrough.js"; + +const readOnly = (cmd: string) => isReadOnlySshCommand(cmd); + +test("simple allowlisted commands are read-only", () => { + assert.equal(readOnly("ls -la /srv/www"), true); + assert.equal(readOnly("wc -l file"), true); + assert.equal(readOnly("rm -rf /"), false); + assert.equal(readOnly(""), false); +}); + +test("find is read-only unless it carries mutating primaries", () => { + assert.equal(readOnly("find /home/web -mtime +90"), true); + for (const bad of ["-delete", "-exec", "-execdir", "-ok", "-okdir", "-fprint", "-fls"]) { + assert.equal(readOnly(`find /home/web ${bad} x`), false, bad); + } +}); + +test("awk allows plain programs and -F/-v only", () => { + assert.equal(readOnly("awk '{print $1}' file"), true); + assert.equal(readOnly("awk -F'|' '{print $1}' file"), true); + assert.equal(readOnly("awk -v x=1 '{print x}' file"), true); + assert.equal(readOnly("awk '{system(\"id\")}' file"), false); + assert.equal(readOnly("awk '{print > \"out\"}' file"), false); + assert.equal(readOnly("awk '{print | \"sh\"}' file"), false); + assert.equal(readOnly("awk '{getline}' file"), false); +}); + +test("awk refuses file-writing and code-loading options", () => { + for (const opt of ["-i inplace", "-o out", "--profile=out", "-f prog.awk", "-l ext", "-E prog.awk", "--include x"]) { + assert.equal(readOnly(`awk ${opt} '{print}' file`), false, opt); + } +}); + +test("pipelines need every segment read-only", () => { + assert.equal(readOnly("find /home/web -mtime +90 | wc -l"), true); + assert.equal(readOnly("cat a | grep b | wc -l"), true); + assert.equal(readOnly("find /x -delete | wc -l"), false); + assert.equal(readOnly("cat a | sh"), false); +}); + +test("malformed pipelines are rejected", () => { + assert.equal(readOnly("| wc -l"), false); + assert.equal(readOnly("cat a |"), false); + assert.equal(readOnly("cat a | | wc"), false); +}); + +test("quoted or glued pipes are literal arguments", () => { + assert.equal(readOnly("grep 'a|b' file"), true); + assert.equal(readOnly("ls|wc"), false); + assert.ok(tokenizeCommand("grep 'a|b' file").every((t) => !t.isPipe)); + assert.ok(tokenizeCommand("grep \"|\" file").every((t) => !t.isPipe)); + assert.ok(tokenizeCommand("grep \\| file").every((t) => !t.isPipe)); +}); + +test("buildRemoteCommand passes only real pipes through unquoted", () => { + const piped = buildRemoteCommand(tokenizeCommand("cat a | wc -l")); + assert.match(piped, / \| /); + const quoted = buildRemoteCommand(tokenizeCommand("echo '|'")); + assert.doesNotMatch(quoted, /(^| )\|( |$)/); +}); + +test("credential paths still require confirmation inside a pipeline", () => { + assert.equal(readOnly("cat .env | wc -l"), false); +}); + +test("empty quoted arguments are preserved", () => { + assert.deepEqual(tokenizeCommand("grep '' file").map((t) => t.value), ["grep", "", "file"]); + assert.deepEqual(tokenizeCommand('grep "" file').map((t) => t.value), ["grep", "", "file"]); + assert.equal(buildRemoteCommand(tokenizeCommand("grep '' file")), "'grep' '' 'file'"); + assert.deepEqual(tokenizeCommand(" a b ").map((t) => t.value), ["a", "b"]); +});