Skip to content

CmdShell: quote items that cmd.exe would otherwise interpret - #434

Merged
slachiewicz merged 1 commit into
masterfrom
agent/cmdshell-quote-specials
Sep 11, 2026
Merged

slachiewicz merged 1 commit into
masterfrom
agent/cmdshell-quote-specials

Conversation

@slachiewicz

@slachiewicz slachiewicz commented Sep 11, 2026

Copy link
Copy Markdown
Member

CmdShell builds one cmd.exe /X /C "<line>" string and never quoted the items inside it, so an executable path with parentheses or an argument with & was parsed by cmd.exe as operators. This adds CmdShell.quoteOneItem: an item that contains whitespace or one of the characters cmd.exe /? lists as special (& < > ( ) @ ^ |) is wrapped in double quotes; an item already wrapped in double quotes is left alone; setQuotedArgumentsEnabled(false) and setUnconditionalQuoting(true) keep their meaning.

Two things differ from the earlier attempts (#40, MSHARED-851). The quoting is conditional on the documented character set rather than unconditional, and it is confined to CmdShell, so BourneShell is untouched. It also restores something master lost in ef89c5a (#369): before that commit Shell.quoteOneItem double-quoted items containing a space through quoteAndEscape, and 3.4.2 still ships that behaviour. Shell.getQuotingTriggerChars was the dead remainder of that mechanism and is removed here.

Two behaviour changes to be aware of:

  • The ^& workaround from [MSHARED-765] Password not escaped (on windows?) #268 stops working. ^& contains &, so it is now quoted, and ^ is literal inside quotes, so the program receives ^&. Users who applied it must drop it.
  • Under -Djdk.lang.Process.allowAmbiguousCommands=false, Java rejects a quoted argument that contains further quotes. Previously only caller-supplied inner quotes could trigger that; now any space or special character does. Default JVMs are unaffected.

CmdShellTest pins the string shape on every platform. Two tests in CommandLineUtilsTest run only on Windows and execute a real cmd.exe: echo a&b must print "a&b" rather than run b, and a script under a lol(1) directory must run. The three windows-latest CI jobs are the evidence for those.

Fixes #268, which also covers the parentheses case from #286.

Verified: mvn -B verify on JDK 17 (macOS) -> Tests run: 800, Failures: 0, Errors: 0, Skipped: 19; spotless clean; the Windows execution tests pass on all three windows-latest jobs (JDK 8, 21, 25).

This change was created with AI assistance.

@slachiewicz slachiewicz added this to the 3.5.0 milestone Sep 11, 2026
@slachiewicz slachiewicz added the bug Something isn't working label Sep 11, 2026
@slachiewicz
slachiewicz marked this pull request as ready for review September 11, 2026 08:11
@slachiewicz

Copy link
Copy Markdown
Member Author

For reviewers: how plexus-utils handles the same problem, since its CmdShell is the sibling of this one.

Behaviour. plexus-utils Shell.quoteOneItem goes through quoteAndEscape with a single trigger, the space: an item with a space gets double quotes, an already-quoted item is left alone, nothing is escaped by default. That is what shared-utils 3.4.2 did before ef89c5a removed the path. This PR restores that and adds the cmd.exe /? specials & < > ( ) @ ^ | as triggers; plexus-utils has no equivalent, so lol(1) and a&b still fail there.

Reported there, never fixed by quoting. #37 (asterisk in a password) has been open since 2018; #36 (embedded double quote) was closed as its duplicate. The maintainers' direction is architectural: PR #109 drops the cmd.exe wrapper and hands the argument vector to ProcessBuilder, and #37 says explicitly that the shell implementations should be replaced rather than taught more escaping. That PR has been open since 2021.

What this PR shares with plexus-utils, on purpose. An embedded " is not escaped and * is not quoted, because cmd.exe toggles quote state on every " and no escaping survives it reliably. Those cases belong with the "drop the shell" change, not with trigger characters. One small divergence: under unconditional quoting plexus-utils re-wraps an already-quoted item, this PR leaves it alone.

Follow-up, separate from this PR. plexus-utils runs cmd.exe /X /D /C since 9093271 (2024, for MJAVADOC-811): /D skips the Command Processor AutoRun registry entry, which in some corporate setups changes the working directory of every spawned cmd.exe. Our CmdShell still runs /X /C and has the same exposure.

This comment was created with AI assistance.

@slachiewicz
slachiewicz marked this pull request as draft September 11, 2026 09:39
@slachiewicz
slachiewicz requested a review from elharo September 11, 2026 09:39
@slachiewicz
slachiewicz marked this pull request as ready for review September 11, 2026 14:21
CmdShell wraps the whole command line in one pair of quotes but never
quoted the items inside it, so an executable path with parentheses or
an argument with & was parsed by cmd.exe as operators. Items containing
whitespace or one of the characters cmd.exe /? lists as special are now
double-quoted; already-quoted items are left alone.

Master had also lost the space quoting that 3.4.2 ships: ef89c5a
removed the quoteAndEscape path from Shell and left the trigger hook
dead. That hook is removed and the behaviour lives in CmdShell.

Fixes #268
@slachiewicz
slachiewicz force-pushed the agent/cmdshell-quote-specials branch from b2dfc08 to dce8d10 Compare September 11, 2026 16:34
@slachiewicz
slachiewicz merged commit 517e861 into master Sep 11, 2026
15 checks passed
@slachiewicz
slachiewicz deleted the agent/cmdshell-quote-specials branch September 11, 2026 17:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[MSHARED-765] Password not escaped (on windows?)

2 participants