RTECO-1885 - Fix Gradle -D/-P property values getting wrapped in literal quotes by naveenku-jfrog · Pull Request #416 · jfrog/build-info-go

naveenku-jfrog · 2026-08-24T09:30:39Z

Summary

jf gradle artifactorypublish -Ddeploy.test.property='test test' uploaded the property value to Artifactory with the surrounding single quotes included ('test test' instead of test test), causing downstream builds to fail when checking that property.
Root cause: quotePropertyIfNeeded() wrapped -D/-P property values containing spaces in literal single quotes before building the exec.Command argv in gradleRunConfig.GetCmd(). Since exec.Command execs Gradle directly without a shell, those quote characters are never stripped — they're passed straight through to Gradle as part of the value.
Fix: quoting is now applied only to the string used for the human-readable "Running gradle command" log line, never to the actual arguments passed to exec.Command.

Test plan

go build ./...
go test ./build/... -run 'TestQuoteArgsForLog|TestGetCmdDoesNotQuotePropertyValues|TestParseGradleVersion' — updated existing test (renamed to TestQuoteArgsForLog, now testing the log-only helper) and added TestGetCmdDoesNotQuotePropertyValues, which asserts cmd.Args from GetCmd() contains the raw unquoted property value.

Screenshots:

Before fix:

After fix:

…ral quotes quotePropertyIfNeeded() was applied when building the actual exec.Command argv, wrapping values containing spaces in single quotes (e.g. -Ddeploy.test.property='test test'). Since exec.Command execs Gradle directly without a shell, those quote characters are never stripped and end up as part of the property value uploaded to Artifactory. Quoting is now only applied to the command string used for the human-readable log line, not to the arguments actually passed to Gradle. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Not re-adding quotes when building the exec.Command args (previous commit) is not enough on its own: on Windows, cmd.exe/PowerShell do not strip single quotes from arguments the way bash/zsh do, so a value like -Ddeploy.test.property='test test' can still reach this process with the quotes literally part of the string. Add stripPropertyQuotes(), which removes a matching leading/trailing quote pair (single or double) from -D/-P property values before they are passed to Gradle. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

naveenku-jfrog · 2026-08-24T09:52:24Z

Follow-up commit added: not re-adding quotes wasn't sufficient on its own, since on Windows cmd.exe/PowerShell don't strip single quotes from arguments the way bash/zsh do. A quoted value can still reach this process with the quotes literally part of the string.

Added stripPropertyQuotes(), which strips a matching leading/trailing quote pair (single or double) from -D/-P property values before they're passed to Gradle, so the property is uploaded clean regardless of shell.

bhanurp · 2026-08-26T05:28:55Z

+}
+
+// Strips a matching pair of leading/trailing single or double quotes from a system or project property's value.
+func unquoteProperty(task string) string {


move this function to common/utils

Actually implemented this in 741d054 — pulled the quote-stripping logic out into utils.StripSurroundingQuotes(value string) string (utils/utils.go). unquoteProperty in gradle.go now just handles the key=value split and delegates the quote-stripping to the shared helper. Added TestStripSurroundingQuotes in the new utils/utils_test.go.

why not move this function unquoteProperty as well this doesnt look like specific to gradle

unquoteProperty itself is tied specifically to Gradle's -D/-P flag convention: it only runs after isSystemOrProjectProperty(task) confirms the arg starts with the systemPropertiesFlag/projectPropertiesFlag constants defined in this file, and no other build tool in this repo uses that -D/-P key=value pattern (checked with a repo-wide grep). What I moved to utils.StripSurroundingQuotes is the generic part — stripping a matching quote pair from an arbitrary string — which has no Gradle dependency. Keeping the -D/-P parsing here avoids pulling Gradle-specific concepts into the shared utils package; happy to revisit if another build tool ends up needing the same -D/-P handling.

What about maven? cant this be reused in maven?

Fair question — structurally maven.go's GetCmd (mvnRunConfig, build/maven.go:316) has the same shape: a flat []string of -D properties appended straight into exec.Command with no shell, so the same Windows quote-leak could theoretically happen there too. That said, Maven's GetCmd currently does zero quote handling (it never had the original wrap-in-quotes bug either), and I don't have a reported case or repro of a quote-wrapped property actually reaching mvnRunConfig.goals on Windows the way this ticket (RTECO-1885) reported for Gradle's artifactoryPublish. Rather than bolt speculative handling onto a different build tool's file as part of this fix, I'd suggest tracking Maven as a follow-up if/when it's actually hit — utils.StripSurroundingQuotes is already extracted and would be directly reusable there without needing another refactor.

bhanurp · 2026-08-26T05:34:09Z

+// This must never be used to build the actual arguments passed to exec.Command: those are executed directly,
+// without going through a shell, so added quote characters would be passed through literally instead of being
+// stripped, corrupting the property value.
+func quoteArgsForLog(args []string) []string {


why is this needed? If logged before stripping it should be good enough right?

Logging before stripping wouldn't actually reproduce the readability behavior we want in the common (non-Windows) case: on Unix the shell has already stripped any quotes by the time we see config.tasks, so a value like "test test" arrives with no quotes at all, before or after stripPropertyQuotes runs. quoteArgsForLog's job is independent of that — it's purely to wrap space-containing values in quotes for the printed log line so the property boundary is visually clear, regardless of whether the raw arg had quotes. So it's needed either way; keeping it as a separate step (used only for the log string, never for the actual exec.Command args) is what keeps that log-only quoting from leaking into the real argv again, which was the original bug this PR fixes.

isnt this overkill just for once in a while kind of properties? why not add quotes for all of the property values instad of checking or completely removing?

Good point — simplified in 3ac0220. quoteProperty (renamed from quotePropertyIfNeeded) now always wraps the value in quotes for the log line, dropping the space check entirely. Tests updated.

…st missing "=" Address PR review feedback: restrict stripPropertyQuotes to Windows only, since on other platforms the shell has already stripped shell-level quoting and any leftover quotes were typed deliberately as part of the value. Also guard unquoteProperty against a task with no "=" to avoid an index-out-of-range panic if it is ever called without the isSystemOrProjectProperty pre-check. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Extract the generic "strip a matching leading/trailing quote pair" logic out of unquoteProperty into a reusable utils.StripSurroundingQuotes helper, per PR review feedback. unquoteProperty keeps the Gradle-specific key=value splitting and now delegates the quote-stripping to the shared helper. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

github-actions · 2026-08-26T06:14:59Z

🐸 JFrog Frogbot

bhanurp · 2026-08-26T06:14:30Z

+}
+
+// Strips a matching pair of leading/trailing single or double quotes from a system or project property's value.
+func unquoteProperty(task string) string {


why not move this function unquoteProperty as well this doesnt look like specific to gradle

bhanurp · 2026-08-26T06:16:54Z

+// This must never be used to build the actual arguments passed to exec.Command: those are executed directly,
+// without going through a shell, so added quote characters would be passed through literally instead of being
+// stripped, corrupting the property value.
+func quoteArgsForLog(args []string) []string {


isnt this overkill just for once in a while kind of properties? why not add quotes for all of the property values instad of checking or completely removing?

…hen spaced Simplify quoteArgsForLog per review feedback: instead of conditionally quoting a -D/-P value only when it contains a space, always wrap it in quotes for the log line. Renamed quotePropertyIfNeeded to quoteProperty and dropped the space check; also added a len(parts) < 2 guard for consistency with unquoteProperty. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

naveenku-jfrog requested a deployment to frogbot August 24, 2026 09:30 — with GitHub Actions Waiting

naveenku-jfrog deployed to frogbot August 24, 2026 09:52 — with GitHub Actions Active

naveenku-jfrog added bug Something isn't working safe to test labels Aug 24, 2026

github-actions Bot removed the safe to test label Aug 25, 2026

naveenku-jfrog added the safe to test label Aug 25, 2026

github-actions Bot removed the safe to test label Aug 25, 2026

naveenku-jfrog requested review from agrasth, bhanurp, fluxxBot and reshmifrog August 25, 2026 08:51

bhanurp reviewed Aug 26, 2026

View reviewed changes

fluxxBot reviewed Aug 26, 2026

View reviewed changes

Comment thread build/gradle.go Outdated

naveenku-jfrog requested a deployment to frogbot August 26, 2026 06:01 — with GitHub Actions Waiting

naveenku-jfrog deployed to frogbot August 26, 2026 06:06 — with GitHub Actions Active

fluxxBot approved these changes Aug 26, 2026

View reviewed changes

naveenku-jfrog added the safe to test label Aug 26, 2026

github-actions Bot removed the safe to test label Aug 26, 2026

bhanurp requested changes Aug 26, 2026

View reviewed changes

naveenku-jfrog requested a deployment to frogbot August 26, 2026 06:22 — with GitHub Actions Waiting

naveenku-jfrog requested a review from bhanurp August 26, 2026 06:24

bhanurp approved these changes Aug 26, 2026

View reviewed changes

naveenku-jfrog merged commit 7c983a1 into main Aug 26, 2026
30 of 31 checks passed

BrewTestBot mentioned this pull request Aug 26, 2026

jfrog-cli 2.122.0 Homebrew/homebrew-core#300775

Merged

Uh oh!

Conversation

naveenku-jfrog commented Aug 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Uh oh!

naveenku-jfrog commented Aug 24, 2026

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Aug 26, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

naveenku-jfrog commented Aug 24, 2026 •

edited

Loading