From da60b9f89da9ce0d0977e259eab52c49439700e4 Mon Sep 17 00:00:00 2001 From: Peeter Pratka <1741412+evilfurryone@users.noreply.github.com> Date: Tue, 22 Sep 2026 11:26:34 +0300 Subject: [PATCH 1/2] Escape branch name before embedding it in release shell scripts ci release deploy, diff and validate interpolate --branchname (and the environment name derived from it) into a single-quoted bash assignment. Git allows quotes and other shell metacharacters in branch names, so a name like "x'; curl ...; #" closed the literal and ran as shell code in the CI job's environment. Add common.EscapeSingleQuoted, which rewrites ' as '\'' so the value is preserved exactly, and apply it at all seven interpolation points. The value is a helm --set input used by the charts, so escaping is used rather than stripping characters to avoid changing what the chart sees. Add a regression test covering the three subcommands. --- cmd/ciReleaseDeploy.go | 6 +++--- cmd/ciReleaseDiff.go | 6 +++--- cmd/ciReleaseValidate.go | 2 +- internal/common/shell.go | 12 ++++++++++++ tests/release_test.go | 42 ++++++++++++++++++++++++++++++++++++++++ 5 files changed, 61 insertions(+), 7 deletions(-) create mode 100644 internal/common/shell.go diff --git a/cmd/ciReleaseDeploy.go b/cmd/ciReleaseDeploy.go index 605a06f..1309dec 100644 --- a/cmd/ciReleaseDeploy.go +++ b/cmd/ciReleaseDeploy.go @@ -250,7 +250,7 @@ var ciReleaseDeployCmd = &cobra.Command{ --timeout "${DEPLOYMENT_TIMEOUT}" \ --wait`, releaseName, chartName, chartRepository, chartVersionOverride, - siltaEnvironmentName, branchname, nginxImageUrl, + common.EscapeSingleQuoted(siltaEnvironmentName), common.EscapeSingleQuoted(branchname), nginxImageUrl, clusterDomain, extraNoAuthIPs, vpcNativeOverride, extraClusterType, namespace, siltaConfig, helmFlags, deploymentTimeout) pipedExec(command, "", "ERROR: ", debug) @@ -367,7 +367,7 @@ var ciReleaseDeployCmd = &cobra.Command{ rm -f helm-output.log || true `, releaseName, chartName, chartRepository, chartVersionOverride, - siltaEnvironmentName, branchname, + common.EscapeSingleQuoted(siltaEnvironmentName), common.EscapeSingleQuoted(branchname), repositoryUrl, gitAuthUsername, gitAuthPassword, clusterDomain, namespace, extraNoAuthIPs, vpcNativeOverride, extraClusterType, @@ -565,7 +565,7 @@ var ciReleaseDeployCmd = &cobra.Command{ rm -f helm-output.log || true `, releaseName, chartName, chartRepository, chartVersionOverride, - siltaEnvironmentName, branchname, + common.EscapeSingleQuoted(siltaEnvironmentName), common.EscapeSingleQuoted(branchname), phpImageUrl, nginxImageUrl, shellImageUrl, repositoryUrl, gitAuthUsername, gitAuthPassword, clusterDomain, extraNoAuthIPs, vpcNativeOverride, extraClusterType, diff --git a/cmd/ciReleaseDiff.go b/cmd/ciReleaseDiff.go index 480a5ea..afbfa4e 100644 --- a/cmd/ciReleaseDiff.go +++ b/cmd/ciReleaseDiff.go @@ -194,7 +194,7 @@ var ciReleaseDiffCmd = &cobra.Command{ --values "${SILTA_CONFIG}" \ ${EXTRA_HELM_FLAGS}`, releaseName, chartName, chartRepository, chartVersionOverride, - siltaEnvironmentName, branchname, nginxImageUrl, + common.EscapeSingleQuoted(siltaEnvironmentName), common.EscapeSingleQuoted(branchname), nginxImageUrl, clusterDomain, extraNoAuthIPs, vpcNativeOverride, extraClusterType, namespace, siltaConfig, helmFlags) pipedExec(command, "", "ERROR: ", debug) @@ -249,7 +249,7 @@ var ciReleaseDiffCmd = &cobra.Command{ --values "${SILTA_CONFIG}" \ ${EXTRA_HELM_FLAGS}`, releaseName, chartName, chartRepository, chartVersionOverride, - siltaEnvironmentName, branchname, + common.EscapeSingleQuoted(siltaEnvironmentName), common.EscapeSingleQuoted(branchname), repositoryUrl, gitAuthUsername, gitAuthPassword, clusterDomain, namespace, extraNoAuthIPs, vpcNativeOverride, extraClusterType, @@ -352,7 +352,7 @@ var ciReleaseDiffCmd = &cobra.Command{ --values "${SILTA_CONFIG}" \ ${EXTRA_HELM_FLAGS}`, releaseName, chartName, chartRepository, chartVersionOverride, - siltaEnvironmentName, branchname, + common.EscapeSingleQuoted(siltaEnvironmentName), common.EscapeSingleQuoted(branchname), phpImageUrl, nginxImageUrl, shellImageUrl, repositoryUrl, gitAuthUsername, gitAuthPassword, clusterDomain, extraNoAuthIPs, vpcNativeOverride, extraClusterType, diff --git a/cmd/ciReleaseValidate.go b/cmd/ciReleaseValidate.go index 5983110..656f5cc 100644 --- a/cmd/ciReleaseValidate.go +++ b/cmd/ciReleaseValidate.go @@ -188,7 +188,7 @@ var ciReleaseValidateCmd = &cobra.Command{ `, releaseName, chartName, chartRepository, chartVersionOverride, - siltaEnvironmentName, branchname, + common.EscapeSingleQuoted(siltaEnvironmentName), common.EscapeSingleQuoted(branchname), namespace, siltaConfig, extraNoAuthIPs, vpcNativeOverride, extraClusterType) cmd := exec.Command("bash", "-c", command) diff --git a/internal/common/shell.go b/internal/common/shell.go new file mode 100644 index 0000000..b80edcb --- /dev/null +++ b/internal/common/shell.go @@ -0,0 +1,12 @@ +package common + +import "strings" + +// EscapeSingleQuoted makes a value safe to embed inside a single-quoted bash +// string literal. A single quote cannot be escaped inside single quotes, so +// each one is rewritten as: close quote, escaped quote, reopen quote ('\”). +// Every other character is literal inside single quotes, so nothing else +// needs handling. The value itself is preserved exactly. +func EscapeSingleQuoted(value string) string { + return strings.ReplaceAll(value, "'", `'\''`) +} diff --git a/tests/release_test.go b/tests/release_test.go index d7d90ea..35a0ca1 100644 --- a/tests/release_test.go +++ b/tests/release_test.go @@ -2,6 +2,8 @@ package cmd_test import ( "os" + "os/exec" + "strings" "testing" ) @@ -445,3 +447,43 @@ func TestReleaseDeployCmd(t *testing.T) { // Change dir back to previous os.Chdir(wd) } + +// A branch name containing shell metacharacters must stay inside its +// single-quoted assignment in the generated release scripts. Before escaping, +// the quote closed the literal and the rest ran as shell code. +func TestReleaseBranchnameEscaping(t *testing.T) { + + // Go to main directory + wd, _ := os.Getwd() + os.Chdir("..") + + branchname := `--branchname "feature/x'; echo pwned; #"` + + // deploy and diff print the generated script in debug mode, so the escaped + // assignments can be checked directly. Environment name is derived from the + // branch name and escaped the same way. + testString := `SILTA_ENVIRONMENT_NAME='feature/x'\''; echo pwned; #' + BRANCHNAME='feature/x'\''; echo pwned; #'` + for _, subcommand := range []string{"deploy", "diff"} { + command := `ci release ` + subcommand + ` \ + --namespace default \ + --release-name 'test' \ + --chart-name drupal \ + --php-image-url php-image \ + --nginx-image-url nginx-image \ + --shell-image-url shell-image \ + ` + branchname + ` \ + --debug` + CliExecTest(t, command, []string{}, testString, false) + } + + // validate runs its helm dry-run even in debug mode and hides the script, + // so assert on behaviour instead: the injected echo must not run. + command := `ci release validate --namespace default --release-name 'test' --chart-name drupal ` + branchname + ` --debug` + out, _ := exec.Command("bash", "-c", cliBinaryName+" "+command+" 2>&1").CombinedOutput() + if strings.Contains(string(out), "pwned") { + t.Errorf("branch name escaped the single-quoted assignment in validate:\n%s", out) + } + + os.Chdir(wd) +} From 9a6dc81297b407c165b725ece29008b4689d9add Mon Sep 17 00:00:00 2001 From: Peeter Pratka <1741412+evilfurryone@users.noreply.github.com> Date: Tue, 22 Sep 2026 11:31:50 +0300 Subject: [PATCH 2/2] Honour --debug in ci release validate helm dry-run The helm dry-run step in ci release validate always executed, even with --debug set, unlike every other generated script in the CLI. Print the command instead of running it when debug is on, matching pipedExec. Fix bufferedExec (used by ci release info) which called fmt.Sprintf in its debug branch and so printed nothing; go vet flagged it. Extend the branch name escaping test to check validate's debug output directly now that it is printed. --- cmd/ciReleaseValidate.go | 42 ++++++++++++++++++++++------------------ cmd/root.go | 2 +- tests/release_test.go | 27 +++++++++----------------- 3 files changed, 33 insertions(+), 38 deletions(-) diff --git a/cmd/ciReleaseValidate.go b/cmd/ciReleaseValidate.go index 656f5cc..fa8d981 100644 --- a/cmd/ciReleaseValidate.go +++ b/cmd/ciReleaseValidate.go @@ -191,26 +191,30 @@ var ciReleaseValidateCmd = &cobra.Command{ common.EscapeSingleQuoted(siltaEnvironmentName), common.EscapeSingleQuoted(branchname), namespace, siltaConfig, extraNoAuthIPs, vpcNativeOverride, extraClusterType) - cmd := exec.Command("bash", "-c", command) - // StdOutPipe omitted to avoid exposing secrets - cmdErrReader, err := cmd.StderrPipe() - if err != nil { - log.Fatal("Error (stderr pipe): ", err) - return - } - errScanner := bufio.NewScanner(cmdErrReader) - go func() { - for errScanner.Scan() { - fmt.Printf("ERROR: %s\n", errScanner.Text()) + if debug { + fmt.Printf("Command (not executed): %s\n", command) + } else { + cmd := exec.Command("bash", "-c", command) + // StdOutPipe omitted to avoid exposing secrets + cmdErrReader, err := cmd.StderrPipe() + if err != nil { + log.Fatal("Error (stderr pipe): ", err) + return + } + errScanner := bufio.NewScanner(cmdErrReader) + go func() { + for errScanner.Scan() { + fmt.Printf("ERROR: %s\n", errScanner.Text()) + } + }() + err = cmd.Start() + if err != nil { + log.Fatal("Error (Start): ", err) + } + err = cmd.Wait() + if err != nil { + log.Fatal("Error (Wait): ", err) } - }() - err = cmd.Start() - if err != nil { - log.Fatal("Error (Start): ", err) - } - err = cmd.Wait() - if err != nil { - log.Fatal("Error (Wait): ", err) } } else { diff --git a/cmd/root.go b/cmd/root.go index fefe43f..1edd558 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -69,7 +69,7 @@ func init() { func bufferedExec(command string, debug bool) { if debug { - fmt.Sprintf("Command (not executed): %s\n", command) + fmt.Printf("Command (not executed): %s\n", command) } else { out, err := exec.Command("bash", "-c", command).CombinedOutput() fmt.Printf("%s\n", out) diff --git a/tests/release_test.go b/tests/release_test.go index 35a0ca1..312c78d 100644 --- a/tests/release_test.go +++ b/tests/release_test.go @@ -2,8 +2,6 @@ package cmd_test import ( "os" - "os/exec" - "strings" "testing" ) @@ -459,31 +457,24 @@ func TestReleaseBranchnameEscaping(t *testing.T) { branchname := `--branchname "feature/x'; echo pwned; #"` - // deploy and diff print the generated script in debug mode, so the escaped - // assignments can be checked directly. Environment name is derived from the - // branch name and escaped the same way. + // All three subcommands print the generated script in debug mode, so the + // escaped assignments can be checked directly. Environment name is derived + // from the branch name and escaped the same way. testString := `SILTA_ENVIRONMENT_NAME='feature/x'\''; echo pwned; #' BRANCHNAME='feature/x'\''; echo pwned; #'` - for _, subcommand := range []string{"deploy", "diff"} { + for _, subcommand := range []string{"deploy", "diff", "validate"} { + imageFlags := "" + if subcommand != "validate" { + imageFlags = `--php-image-url php-image --nginx-image-url nginx-image --shell-image-url shell-image` + } command := `ci release ` + subcommand + ` \ --namespace default \ --release-name 'test' \ - --chart-name drupal \ - --php-image-url php-image \ - --nginx-image-url nginx-image \ - --shell-image-url shell-image \ + --chart-name drupal ` + imageFlags + ` \ ` + branchname + ` \ --debug` CliExecTest(t, command, []string{}, testString, false) } - // validate runs its helm dry-run even in debug mode and hides the script, - // so assert on behaviour instead: the injected echo must not run. - command := `ci release validate --namespace default --release-name 'test' --chart-name drupal ` + branchname + ` --debug` - out, _ := exec.Command("bash", "-c", cliBinaryName+" "+command+" 2>&1").CombinedOutput() - if strings.Contains(string(out), "pwned") { - t.Errorf("branch name escaped the single-quoted assignment in validate:\n%s", out) - } - os.Chdir(wd) }