Fix shell command injection: escape all interpolated values before exec() - #16

Hi @SyedAnas01! Sorry for the late reply, I have finally reviewed it. The core escaper is correct: I verified shq("O'Brien") → 'O'\''Brien', which is the part these implementations usually get wrong, and the disclosure payload does become inert single-quoted text.

That said, I'd like to hold this before merge. There are three blocking issues, one of which I've now reproduced on Windows.

1. shq() breaks every sf CLI call on Windows — reproduced

src/utils/shellEscape.ts:22

child_process.exec() runs through cmd.exe /d /s /c on win32, and cmd.exe does not treat ' as a quoting character — it's an ordinary literal passed straight through to argv. manifest.json:209 declares "platforms": ["win32","darwin","linux"] and sfCommand.ts has a populated COMMON_SF_PATHS.win32, so this is a live target.

I ran this build on a Windows machine. The results separate exactly along the shq() line:

get_default_org is the control: same exec()/cmd.exe path, succeeds, because resolveTargetOrg.ts:23 builds a command with zero interpolation. Every call that fails is one where shq() wrapped a value.

The actual error from query_records:

NamedOrgNotFoundError: Parsing --target-org
  No authorization information found for 'my-dev-hub'.

…while list_connected_salesforce_orgs, reading the same auth store via the native SDK, returns my-dev-hub with a valid orgId and instanceUrl. The auth exists — sf is just receiving 'my-dev-hub' with the apostrophes attached. Passing the literal username instead of the alias gives the identical error, which rules out an unregistered-alias explanation.

Confirm directly in cmd.exe:

sf sobject describe --sobject 'Account' --target-org 'my-dev-hub' --json   REM fails
sf sobject describe --sobject Account --target-org my-dev-hub --json       REM works

On main the command is built unquoted and works fine under cmd.exe, so this is introduced by the PR, not pre-existing.

There's also a narrower security regression on Windows. query.ts:42 went from --query "${query}" to --query ${shq(query)}. Under cmd.exe, & is inert inside double quotes but live outside single quotes, so a where-clause of Name != null & calc & now splits and executes calc. Windows injection already existed (an embedded " escaped the old form), but this widens it.

Worth noting the PR is already aware of the asymmetry — records.ts deliberately skips the win32 branch for precisely this reason, while every sf command goes through shq() unconditionally through the same exec().

2. Two files still build unescaped commands

src/prompts/prompts.ts:208, 486, 494, 504 · src/resources/resources.ts:103, 308

The description says "every interpolated value in every sf-command-building call site across the repo", but neither file is in the diff, and both are reachable:

// prompts.ts:208 — className is a bare z.string()
`... WHERE Name = '${args.className}'" --target-org ${org} --json`
// prompts.ts:486 — logId, no quoting at all
`sf apex get log --log-id ${logId} --target-org ${org} --json`
// resources.ts:308 — orgAlias from the MCP resource URI template
`sf limits api display --target-org ${orgAlias} --json`;

A backtick or $() payload in className doesn't even need to break out of the double quotes. validateOrgAccess doesn't gate the resources path either — ALLOWED_ORGS defaults to "ALL" (permissions.ts:12), so isOrgAllowed() returns true for any string. I confirmed a URI-safe payload executes (no /, no literal space, so it survives URI-template matching as a single path segment).

src/tools/admin.ts and skill.ts are correctly untouched — no CLI calls there — so that part of the audit holds.

3. Single-quoting collapses multi-value deploy flags

src/tools/project.ts:32, 44, 48

--metadata, --tests and --source-dir are oclif multiple flags that relied on shell word-splitting. project.ts:107 literally instructs the model "Separate multiple with spaces or repeat flag."

So tests="Test1 Test2" now produces --tests 'Test1 Test2' — one test name — and the deploy fails. Same for metadata="ApexClass:Foo ApexClass:Bar".

Fix: split on whitespace and emit a repeated shq()-escaped flag per token, the way orgs.ts already does for --name / --on-behalf-of.

Thanks for testing this on Windows. You were right on all three, and the description overstated things: it said every call site was covered, and prompts.ts and resources.ts were not touched at all. prompts.ts also had org unescaped at 284, 287, 381 and 384, which I missed too.

What changed:

1. Windows. shq() now picks the escaper by platform. POSIX keeps the single-quote form. On win32 it wraps the value in double quotes and caret-escapes every cmd metacharacter, quotes included. After the cmd /c parse the value is "value", so when sf.cmd re-parses %* the metacharacters sit inside quotes and stay inert. Every % gets a caret after it so %VAR% and %VAR:a=b% cannot expand. Values with " or line breaks are rejected on Windows. I looked at execFile with an argv array, but Node won't spawn .cmd files without shell: true, and with it Node does no escaping, so Windows needs an escaper anyway.

2. prompts.ts and resources.ts now escape org, logId and the SOQL string. className has to be a valid Apex identifier and a user-supplied logId has to be a 15 or 18 character Id. In resources, validateOrgAccess also requires the alias to match an authenticated org, since ALLOWED_ORGS defaults to ALL. open_record validates recordId as an Id, which closes the start branch I had left open.

3. --metadata, --source-dir and --tests are split on whitespace (double-quoted parts stay together, as the tool description promises) and each token becomes its own escaped flag.

Added npm test (node:test): real /bin/sh round trips for POSIX payloads, and a model of cmd.exe parsing for the Windows payloads.

I don't have a Windows machine, so the Windows side is tested only against that model. Could you rerun your table on this build?

Hi @SyedAnas01,

Thank you very much for the contribution! I'm merging it now.

Credit: this shell command injection vulnerability was responsibly disclosed and fixed by Syed Anas Mohiuddin (@SyedAnas01). Thank you for the report, the fix, and the quick follow-up on the review feedback!

@SyedAnas01 I can mention you in the change log for the next release. Also, we didn't enable the security advisories here yet, and I don't know when we will enable it, sorry.

Hi, thanks again for merging this and releasing 1.7.0. Would you be open to publishing a short GitHub security advisory for it and clicking Request CVE ID? Its quick (Security tab, New draft advisory), and gives anyone on an older version a standard ID to track. If that's not something you want to do, I can request one from MITRE myself and cite this PR. Just let me know which. Anas