Bug: make all collected args use single quote#11609
Conversation
Thus, preventing expansion, if applicable. Fixes apache#10421
|
@cstamas any update on this? |
|
Someone should review this... |
|
It looks fine to me, but I'm far from a bash expert… |
|
@michael-o ping |
| # Add remaining arguments with proper quoting | ||
| for arg in "$@"; do | ||
| cmd="$cmd \"$arg\"" | ||
| cmd="$cmd '$arg'" |
There was a problem hiding this comment.
This changes semantics "@" does already the quoting for us. I think this needs an IT.
There was a problem hiding this comment.
So, according to your comment, this change is not needed and the issue it fixes is fluke?
There was a problem hiding this comment.
I didn't say its fluke, but we need to understand what this change causes and an IT with it. I wouldn't easily apply it.
gnodet
left a comment
There was a problem hiding this comment.
Claude Code on behalf of Guillaume Nodet
This PR changes how user arguments are passed to the JVM: instead of passing them directly via "$@" (which preserves them exactly as the shell received them), it embeds them into the cmd string with single quotes and then evals the whole thing.
While this does fix the specific ${placeholder} expansion issue from MNG-8672, it introduces a worse problem: any argument containing a literal single quote will break the command. For example:
mvn -Dprop="it's a test" clean
would produce cmd="... 'it's a test' 'clean'" which, after eval, misparses due to the unescaped embedded single quote. This is a classic shell-quoting pitfall and would be a regression for users with apostrophes in paths, property values, or profile names.
The previous approach (eval exec "$cmd" '"$@"') was actually correct -- "$@" preserves each argument literally, including those containing ${...}. The real bug reported in MNG-8672 was likely a different issue (possibly related to how MAVEN_ARGS or MAVEN_OPTS were handled, or an earlier version of the script that used $* or did additional expansion).
I agree with @michael-o that this needs an integration test to validate the behavior, and I think the approach itself needs reconsideration -- swapping double-quote quoting for single-quote quoting trades one class of bugs for another, more common one.
| # Add remaining arguments with proper quoting | ||
| for arg in "$@"; do | ||
| cmd="$cmd \"$arg\"" | ||
| cmd="$cmd '$arg'" |
There was a problem hiding this comment.
Arguments containing literal single quotes will break here. For example, -Dprop="it's a value" produces the cmd fragment 'it's a value', which eval will mispars as three tokens.
The previous approach of passing '"$@"' to eval was safer because "$@" preserves each argument exactly as received by the shell, with no re-quoting needed.
If the goal is to prevent ${...} expansion during eval, the safest fix would be to escape the problematic characters inside double quotes rather than switching to single quotes:
| cmd="$cmd '$arg'" | |
| cmd="$cmd \"$(printf '%s' "$arg" | sed "s/'/'\\\\''/g")\"" |
...though honestly keeping the current eval exec "$cmd" '"$@"' approach and investigating why ${...} was being expanded there would be the better path.
Thus, preventing expansion, if applicable.
Fixes #10421