Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 43 additions & 0 deletions actions/setup/js/mcp_cli_bridge.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -778,6 +778,40 @@ function resolveSchemaPropertyKey(key, schemaProperties, normalizedSchemaKeyMap,
return normalizedSchemaKeyMap.get(normalized) || key;
}

/**
* Unescape standard escape sequences in a CLI string argument.
*
* Converts the same escape sequences that JSON string parsing recognises so
* that agents can write `--body "Hello\nWorld"` and get an actual newline,
* matching the behaviour of JSON stdin mode where `JSON.parse` handles `\n`.
*
* Supported sequences:
* `\n` → newline
* `\t` → tab
* `\r` → carriage return
* `\\` → single backslash
* Any other `\X` is left unchanged (the backslash is preserved).
*
* @param {string} str - Raw CLI string argument
* @returns {string} String with escape sequences replaced by their literal characters
*/
function unescapeCliStringArg(str) {
return str.replace(/\\([\s\S])/g, (match, char) => {
switch (char) {
case "n":
return "\n";
case "t":
return "\t";
case "r":
return "\r";
case "\\":

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[/diagnosing-bugs] The PR claims parity with JSON.parse, but \b (backspace), \f (form feed), and \uXXXX (Unicode) are valid JSON escape sequences that JSON.parse handles yet unescapeCliStringArg leaves unchanged.

💡 Example gap
// JSON stdin mode
JSON.parse('"\u2019"') // → right single quote \u2019

// CLI mode after this PR
unescapeCliStringArg("\u2019") // → "\u2019" (unchanged)

If agents ever pass Unicode escapes or \b/\f via CLI flags, those will silently differ from JSON stdin mode. Consider adding \b, \f, and a \uXXXX handler, or documenting the known gap in the JSDoc.

@copilot please address this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in da0a061: unescapeCliStringArg now also handles \b, \f, and \uXXXX, and the tests cover those escape sequences.

return "\\";
default:
return match;
}
});
}

/**
* Parse and coerce a CLI argument value based on the MCP tool schema property type.
*
Expand Down Expand Up @@ -857,6 +891,14 @@ function coerceToolArgValue(key, rawValue, schemaProperty, existingValue, allowN
}
}

// Unescape standard escape sequences in string-typed values so that agents
// can write `--body "Hello\nWorld"` and get an actual newline. This mirrors
// the behaviour of JSON stdin mode where JSON.parse interprets `\n` as a
// newline character.
if (types.includes("string")) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Silent data corruption on Windows paths and regex patterns: unescapeCliStringArg rewrites backslash sequences in every string-typed field, so legitimate values like C:\\temp\\file or --pattern \\d+ are silently mangled before the tool ever sees them.

💡 Details and suggested fix

The shell strips one level of quoting but leaves backslashes intact in the argv array. When a user passes:

safeoutputs some_tool --path "C:\\Users\\alice\\docs"

Argv delivers C:\Users\alice\docs. After unescapeCliStringArg, \U is left as-is (unknown sequence), but \a (alert) would be silently replaced with \x07, and \d would survive only by luck. More critically:

safeoutputs some_tool --path "C:\temp\new_file"
# \t → tab, \n → newline — path becomes "C:<TAB>emp<LF>ew_file"

The transformation is irreversible and undetectable by callers.

Suggested approach: apply unescaping only when the flag value was explicitly quoted/escaped by the agent in a context where \n is meaningful (e.g., the body field of issue/PR tools). Consider an opt-in schema annotation (e.g., x-unescape-cli: true) rather than transforming all string fields universally. Alternatively, document clearly that Windows-style paths must use forward slashes or double-escaped backslashes.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in da0a061: the bridge now limits CLI escape decoding to body-like fields, so values like Windows paths and regex patterns are no longer rewritten.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Unescaping is skipped for untyped/schema-less fields, creating inconsistent behavior: when schemaProperty is absent or has no type field, types is empty and the types.includes("string") guard is false — raw backslash-n sequences fall through unchanged.

💡 Details

The code path after the new block:

// When schema metadata is unavailable, apply conservative numeric coercion fallback
if (allowNumericFallback && types.length === 0) {
  // ...
}
return rawValue; // unescaped rawValue returned for typeless fields

If the stated goal is parity with JSON stdin mode for all string arguments, the fix is incomplete for typeless schemas. At minimum, document why typeless strings are intentionally excluded.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated in da0a061: typeless fields are now intentionally left untouched. The narrowed behavior is schema-gated to body-like string fields only, so we avoid mutating arbitrary CLI strings when schema metadata is missing.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[/diagnosing-bugs] coerceToolArgValue applies unescapeCliStringArg to all string-typed schema properties, including file paths (--path), regex patterns, and URL slugs. A Windows path like C:\temp\file passed as a CLI argument would have \t and \f silently converted to tab and form-feed characters, corrupting the value.

💡 Suggested mitigation

The existing Copilot review comment on this line already flags this concern. Possible options:

  1. Opt-in via an explicit schema annotation, e.g. "x-unescape": true
  2. Restrict to known tools/fields that accept multiline text (e.g. body, message, content)
  3. Document clearly in the JSDoc that callers should NOT pass raw Windows paths through CLI mode and must double-escape backslashes

Option 3 is lowest risk short-term but option 2 is safer.

@copilot please address this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in da0a061: coerceToolArgValue no longer unescapes every string schema property. It now applies only to body-like text fields, which preserves paths, regexes, and other ordinary string inputs.

return unescapeCliStringArg(rawValue);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in da0a061: CLI escape decoding now only applies to body-like fields (body, draft_body), so ordinary string arguments like paths and regexes are left unchanged.

}

// When schema metadata is unavailable (e.g. empty tools cache), apply
// conservative numeric coercion fallback for CLI ergonomics.
if (allowNumericFallback && types.length === 0) {
Expand Down Expand Up @@ -1376,6 +1418,7 @@ if (require.main === module) {
module.exports = {
parseToolArgs,
coerceToolArgValue,
unescapeCliStringArg,
extractJSONRPCMessages,
renderProgressMessages,
formatResponse,
Expand Down
76 changes: 75 additions & 1 deletion actions/setup/js/mcp_cli_bridge.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import fs from "fs";
import os from "os";
import path from "path";

import { ensureSafeOutputsTools, formatResponse, getToolCallTimeoutMs, hasStdinJsonPayload, parseToolArgs, readStdinSync, shouldShowToolHelpForEmptyArgs, showHelp, showToolHelp, writeStdoutAndFlush } from "./mcp_cli_bridge.cjs";
import { ensureSafeOutputsTools, formatResponse, getToolCallTimeoutMs, hasStdinJsonPayload, parseToolArgs, readStdinSync, shouldShowToolHelpForEmptyArgs, showHelp, showToolHelp, unescapeCliStringArg, writeStdoutAndFlush } from "./mcp_cli_bridge.cjs";

describe("mcp_cli_bridge.cjs", () => {
let originalCore;
Expand Down Expand Up @@ -750,6 +750,80 @@ describe("mcp_cli_bridge.cjs", () => {
});
});

describe("unescapeCliStringArg", () => {
it("converts \\n to an actual newline", () => {
expect(unescapeCliStringArg("Hello\\nWorld")).toBe("Hello\nWorld");
});

it("converts \\t to a tab character", () => {
expect(unescapeCliStringArg("col1\\tcol2")).toBe("col1\tcol2");
});

it("converts \\r to a carriage return", () => {
expect(unescapeCliStringArg("line1\\rline2")).toBe("line1\rline2");
});

it("converts \\\\ to a single backslash", () => {
expect(unescapeCliStringArg("path\\\\to\\\\file")).toBe("path\\to\\file");
});

it("converts \\\\n to a literal backslash followed by n (not a newline)", () => {
// \\n in the CLI arg should become \n (backslash + n), not a newline
expect(unescapeCliStringArg("Hello\\\\nWorld")).toBe("Hello\\nWorld");
});

it("leaves unknown escape sequences unchanged", () => {
expect(unescapeCliStringArg("value\\xunknown")).toBe("value\\xunknown");
});

it("handles multiple escape sequences in the same string", () => {
expect(unescapeCliStringArg("line1\\nline2\\nline3")).toBe("line1\nline2\nline3");
});

it("returns a plain string unchanged when no escape sequences are present", () => {
expect(unescapeCliStringArg("no escapes here")).toBe("no escapes here");
});
});

describe("parseToolArgs — string escape unescaping", () => {
it("unescapes \\n in string-typed CLI flag arguments", () => {
const schemaProperties = { body: { type: "string" } };
const { args } = parseToolArgs(["--body", "Hello\\nWorld"], schemaProperties);
expect(args).toEqual({ body: "Hello\nWorld" });
});

it("unescapes \\n in string-typed --key=value arguments", () => {
const schemaProperties = { body: { type: "string" } };
const { args } = parseToolArgs(["--body=Hello\\nWorld"], schemaProperties);
expect(args).toEqual({ body: "Hello\nWorld" });
});

it("unescapes \\n when type is ['string', 'null']", () => {
const schemaProperties = { body: { type: ["string", "null"] } };
const { args } = parseToolArgs(["--body", "line1\\nline2"], schemaProperties);
expect(args).toEqual({ body: "line1\nline2" });
});

it("does not unescape \\n when schema type is integer", () => {
// A value with \n for an integer field should not be unescaped (it would fail coercion anyway)
const schemaProperties = { count: { type: "integer" } };
const { args } = parseToolArgs(["--count", "5\\n"], schemaProperties);
// "5\n" is not a valid integer, falls through to rawValue
expect(args).toEqual({ count: "5\\n" });
});

it("produces actual newlines matching JSON stdin mode behaviour", () => {
// Verify that --body "title\n\nbody" (CLI flags) gives the same result as
// JSON stdin with {"body":"title\n\nbody"}
const schemaProperties = { body: { type: "string" } };

const { args: cliArgs } = parseToolArgs(["--body", "title\\n\\nbody"], schemaProperties);
const { args: jsonArgs } = parseToolArgs(["."], schemaProperties, '{"body":"title\\n\\nbody"}');

expect(cliArgs).toEqual(jsonArgs);
});
});

describe("writeStdoutAndFlush", () => {
it("resolves immediately when stdout.write returns true (no backpressure)", async () => {
// The beforeEach mock captures chunks and returns true (no backpressure).
Expand Down
Loading