Fix JAVA_OPTS quoting and glob expansion (#1301)#1303
Draft
stokpop wants to merge 10 commits into
Draft
Conversation
…cloudfoundry#1301) - Replace xargs-based JAVA_OPTS normalization with newline-only normalization to preserve quotes and backslashes. - Protect user-provided JAVA_OPTS during opts-file eval expansion by using a placeholder and restoring it after eval. - Introduce JavaExecCommand() and switch Java Main/Spring Boot start commands to eval "exec ... $JAVA_OPTS ..." so shell globbing/word-splitting does not corrupt args. - Add regression tests for quoted values, cron/glob expressions, multiple quoted args, backslashes, and start-command invocation behavior. - Clean up overlapping/no-op test code and improve test readability.
There was a problem hiding this comment.
Pull request overview
This PR addresses v5.x regressions in how the buildpack assembles and invokes JAVA_OPTS, aiming to preserve quoted JVM arguments and prevent unintended glob/word-splitting during the start-command eval path.
Changes:
- Removes
xargsfromJAVA_OPTSnormalization and avoids exposing userJAVA_OPTStoevalduring.optsexpansion. - Introduces a shared
JavaExecCommand(...)helper so container start commands consistently useeval "exec ... $JAVA_OPTS ..."(quoted eval argument). - Adds/extends regression tests to cover quoted args, backslashes, and cron/glob-style expressions.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/java/frameworks/java_opts_writer.go | Adjusts runtime assembly script to preserve quoting/backslashes and avoid eval-mangling of user JAVA_OPTS. |
| src/java/frameworks/java_opts_writer_test.go | Adds regression tests for quoted args, backslashes, and glob-like cron expressions. |
| src/java/containers/container.go | Adds JavaExecCommand(...) helper to standardize quoted-eval Java start command construction. |
| src/java/containers/spring_boot.go | Switches Spring Boot start command construction to JavaExecCommand(...). |
| src/java/containers/spring_boot_test.go | Verifies Spring Boot start commands use quoted eval form. |
| src/java/containers/java_main.go | Switches Java Main start command construction to JavaExecCommand(...). |
| src/java/containers/java_main_test.go | Adds assertions that Java Main start commands use quoted eval form (though currently placed under Describe("buildClasspath")). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…nused test param Arguments from JBP_CONFIG_JAVA_MAIN containing double quotes would break the outer eval "..." string in the start command. Escape them before building the javaArgs string. Add regression test. Rename unused 'javaOpts' param in setupScript test helper to '_' to document that it has no effect on script generation.
Comment on lines
+109
to
+113
| # Expand any remaining environment variables in opts content via eval. | ||
| # Note: eval executes commands, but .opts files are written by the buildpack | ||
| # at staging time and run within the container context. | ||
| # This matches how the Ruby buildpack naturally expanded variables via shell. | ||
| opts_content=$(eval "echo \"$opts_content\"") | ||
| opts_content=$(eval "printf '%%s' \"$opts_content\"") |
eval "printf '%s' \"\$opts_content\"" mangles .opts content containing literal double quotes (e.g. Datadog writes -Ddd.service="myapp"): the inner " terminates the outer eval string and the value is stripped. Escape \ (first) and " in opts_content into a temporary _eval_safe variable before expanding it into the eval command, so both chars are preserved. Add regression tests for double-quoted and backslash .opts values.
Contributor
Author
|
There are many levels of escapes needed to get JAVA_OPTS replacement right with the use of eval and the need for escaping and making the quoting work correctly. I have experimented with a solution that does not depend on eval and all the escaping rules. Let me test this an I can submit another PR as follow-up on to this one (or as replacement). Turned this PR into draft for now. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1301.