Skip to content

Remove a lot of escaping logic that's unreachable and never used #369

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
elharo wants to merge 5 commits into
base: master
Choose a base branch
from
+10 −98
Open

Remove a lot of escaping logic that's unreachable and never used #369

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f0c3da6
Remove a lot of escaping logic that's never used
elharo Feb 10, 2026
c3deefd
Remove a lot of escaping logic that's never used
elharo Feb 10, 2026
649444f
remove more logic
elharo Feb 10, 2026
cfa13ea
one more
elharo Feb 10, 2026
d30f9a9
one more
elharo Feb 10, 2026
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
11 changes: 5 additions & 6 deletions src/main/java/org/apache/maven/shared/utils/cli/shell/BourneShell.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,18 +32,13 @@ public class BourneShell extends Shell {
* Create instance of BourneShell.
*/
public BourneShell() {
setUnconditionalQuoting(true);
setShellCommand("/bin/sh");
setArgumentQuoteDelimiter('\'');
setExecutableQuoteDelimiter('\'');
setSingleQuotedArgumentEscaped(true);
setSingleQuotedExecutableEscaped(false);
setQuotedExecutableEnabled(true);
}

/**
* {@inheritDoc}
*/
@Override
public String getExecutable() {
if (Os.isFamily(Os.FAMILY_WINDOWS)) {
return super.getExecutable();
Expand All @@ -55,6 +50,7 @@ public String getExecutable() {
/**
* {@inheritDoc}
*/
@Override
public List<String> getShellArgsList() {
List<String> shellArgs = new ArrayList<>();
List<String> existingShellArgs = super.getShellArgsList();
Expand All @@ -71,6 +67,7 @@ public List<String> getShellArgsList() {
/**
* {@inheritDoc}
*/
@Override
public String[] getShellArgs() {
String[] shellArgs = super.getShellArgs();
if (shellArgs == null) {
Expand All @@ -92,6 +89,7 @@ public String[] getShellArgs() {
/**
* {@inheritDoc}
*/
@Override
protected String getExecutionPreamble() {
if (getWorkingDirectoryAsString() == null) {
return null;
Expand All @@ -118,6 +116,7 @@ protected String getExecutionPreamble() {
* @param path not null path
* @return the path unified correctly for the Bourne shell
*/
@Override
protected String quoteOneItem(String path, boolean isExecutable) {
if (path == null) {
return null;
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/org/apache/maven/shared/utils/cli/shell/CmdShell.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ public class CmdShell extends Shell {
*/
public CmdShell() {
setShellCommand("cmd.exe");
setQuotedExecutableEnabled(true);
setShellArgs(new String[] {"/X", "/C"});
}

Expand Down Expand Up @@ -77,6 +76,7 @@ public CmdShell() {
* @param arguments the arguments for the executable
* @return the resulting command line
*/
@Override
public List<String> getCommandLine(String executable, String... arguments) {
StringBuilder sb = new StringBuilder();
sb.append('"');
Expand Down
95 changes: 4 additions & 91 deletions src/main/java/org/apache/maven/shared/utils/cli/shell/Shell.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,6 @@
import java.util.Arrays;
import java.util.List;

import org.apache.maven.shared.utils.StringUtils;

/**
* Class that abstracts the Shell functionality,
* with subclasses for shells that behave particularly, like
Expand All @@ -51,15 +49,7 @@ public class Shell {

private String workingDir;

private boolean quotedExecutableEnabled = true;

private boolean singleQuotedArgumentEscaped = false;

private boolean singleQuotedExecutableEscaped = false;

private char argQuoteDelimiter = '\"';

private char exeQuoteDelimiter = '\"';
private final boolean singleQuotedArgumentEscaped = false;

/**
* Set the command to execute the shell (e.g. COMMAND.COM, /bin/bash,...).
Expand Down Expand Up @@ -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;
Copy link

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

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

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).

Suggested change
return inputString;
// 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();

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

@elharo elharo Feb 10, 2026

Choose a reason for hiding this comment

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

copilot missed that getEscapeChars always returned an empty list so nothing was ever escaped.

}

/**
Expand Down Expand Up @@ -140,18 +123,14 @@ List<String> getRawCommandLine(String executableParameter, String... argumentsPa
sb.append(preamble);
}

if (isQuotedExecutableEnabled()) {
sb.append(quoteOneItem(executableParameter, true));
} else {
sb.append(executableParameter);
}
sb.append(quoteOneItem(executableParameter, true));
}
for (String argument : argumentsParameter) {
if (sb.length() > 0) {
sb.append(' ');
}

if (isQuotedArgumentsEnabled()) {
if (quotedArgumentsEnabled) {
sb.append(quoteOneItem(argument, false));
} else {
sb.append(argument);
Expand All @@ -171,22 +150,6 @@ String getExecutionPreamble() {
return null;
}

char[] getEscapeChars(boolean includeSingleQuote, boolean includeDoubleQuote) {
StringBuilder buf = new StringBuilder(2);
if (includeSingleQuote) {
buf.append('\'');
}

if (includeDoubleQuote) {
buf.append('\"');
}

char[] result = new char[buf.length()];
buf.getChars(0, buf.length(), result, 0);

return result;
}

/**
* @return false in all cases
*/
Expand All @@ -201,36 +164,6 @@ protected boolean isSingleQuotedArgumentEscaped() {
return singleQuotedArgumentEscaped;
}

boolean isDoubleQuotedExecutableEscaped() {
return false;
}

boolean isSingleQuotedExecutableEscaped() {
return singleQuotedExecutableEscaped;
}

/**
* @param argQuoteDelimiterParameter {@link #argQuoteDelimiter}
*/
void setArgumentQuoteDelimiter(char argQuoteDelimiterParameter) {
this.argQuoteDelimiter = argQuoteDelimiterParameter;
}

char getArgumentQuoteDelimiter() {
return argQuoteDelimiter;
}

/**
* @param exeQuoteDelimiterParameter {@link #exeQuoteDelimiter}
*/
void setExecutableQuoteDelimiter(char exeQuoteDelimiterParameter) {
this.exeQuoteDelimiter = exeQuoteDelimiterParameter;
}

char getExecutableQuoteDelimiter() {
return exeQuoteDelimiter;
}

/**
* Get the full command line to execute, including shell command, shell arguments,
* executable and executable arguments.
Expand Down Expand Up @@ -267,18 +200,6 @@ public void setQuotedArgumentsEnabled(boolean quotedArgumentsEnabled) {
this.quotedArgumentsEnabled = quotedArgumentsEnabled;
}

boolean isQuotedArgumentsEnabled() {
return quotedArgumentsEnabled;
}

void setQuotedExecutableEnabled(boolean quotedExecutableEnabled) {
this.quotedExecutableEnabled = quotedExecutableEnabled;
}

boolean isQuotedExecutableEnabled() {
return quotedExecutableEnabled;
}

/**
* Sets the executable to run.
*
Expand Down Expand Up @@ -331,14 +252,6 @@ String getWorkingDirectoryAsString() {
return workingDir;
}

void setSingleQuotedArgumentEscaped(boolean singleQuotedArgumentEscaped) {
this.singleQuotedArgumentEscaped = singleQuotedArgumentEscaped;
}

void setSingleQuotedExecutableEscaped(boolean singleQuotedExecutableEscaped) {
this.singleQuotedExecutableEscaped = singleQuotedExecutableEscaped;
}

public boolean isUnconditionalQuoting() {
return unconditionalQuoting;
}
Expand Down