Copilot commented on code in PR #369:
URL:
https://github.com/apache/maven-shared-utils/pull/369#discussion_r2787887179
##########
src/main/java/org/apache/maven/shared/utils/cli/shell/BourneShell.java:
##########
@@ -34,16 +34,12 @@ public class BourneShell extends Shell {
public BourneShell() {
setUnconditionalQuoting(true);
Review Comment:
The constructor still calls `setUnconditionalQuoting(true)`, but
unconditional quoting no longer has any effect because neither
`Shell.quoteOneItem()` nor this class's override consults
`unconditionalQuoting` anymore. Either remove this call (if the flag is being
deprecated) or reintroduce behavior where `unconditionalQuoting` influences
quoting so the public API remains meaningful.
```suggestion
```
##########
src/main/java/org/apache/maven/shared/utils/cli/shell/Shell.java:
##########
@@ -104,14 +94,7 @@ String[] getShellArgs() {
}
protected String quoteOneItem(String inputString, boolean isExecutable) {
- char[] escapeChars = getEscapeChars(isSingleQuotedExecutableEscaped(),
isDoubleQuotedExecutableEscaped());
- return StringUtils.quoteAndEscape(
- inputString,
- isExecutable ? getExecutableQuoteDelimiter() :
getArgumentQuoteDelimiter(),
- escapeChars,
- getQuotingTriggerChars(),
- '\\',
- unconditionalQuoting);
+ return inputString;
Review Comment:
`quoteOneItem()` now returns the input unchanged, which effectively disables
all quoting/escaping in the base `Shell`. This changes behavior for callers
that rely on `setQuotedArgumentsEnabled(true)` (default) to safely/accurately
build a shell command string—most notably `CmdShell`, where an executable or
argument containing spaces (e.g., `C:\Program Files\...`) will be emitted
unquoted inside the `/C "..."` command string and can no longer be executed
correctly. Consider restoring quoting/escaping here (e.g., via
`StringUtils.quoteAndEscape` or an equivalent implementation), or moving the
required quoting logic into each concrete shell implementation (and ensuring
`CmdShell` handles Windows quoting rules).
```suggestion
// If quoting is disabled, honor that and return the input unchanged.
if (!quotedArgumentsEnabled) {
return inputString;
}
if (inputString == null) {
return null;
}
boolean needsQuoting = unconditionalQuoting;
// Determine if any trigger characters are present that require
quoting (e.g., spaces).
if (!needsQuoting) {
for (char trigger : DEFAULT_QUOTING_TRIGGER_CHARS) {
if (inputString.indexOf(trigger) >= 0) {
needsQuoting = true;
break;
}
}
}
if (!needsQuoting) {
return inputString;
}
// Quote the argument and escape any embedded double quotes.
StringBuilder buf = new StringBuilder(inputString.length() + 2);
buf.append('"');
for (int i = 0; i < inputString.length(); i++) {
char ch = inputString.charAt(i);
if (ch == '"') {
buf.append('\\').append('"');
} else {
buf.append(ch);
}
}
buf.append('"');
return buf.toString();
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
