fix(security): 2 improvements across 2 files#662
fix(security): 2 improvements across 2 files#662tomaioo wants to merge 2 commits intoversion-fox:mainfrom
Conversation
- Security: Command injection risk when exporting PATH in Zsh activation script - Security: Windows shell export does not escape values before emitting `set` commands Signed-off-by: tomaioo <203048277+tomaioo@users.noreply.github.com>
- Security: Command injection risk when exporting PATH in Zsh activation script - Security: Windows shell export does not escape values before emitting `set` commands Signed-off-by: tomaioo <203048277+tomaioo@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR hardens shell activation script generation by improving environment variable escaping to reduce command-injection risk when exporting variables in different shells.
Changes:
- Remove zsh-specific
PATHexport logic that interpolated raw values into a double-quoted assignment, and instead use the standard escaping path. - Add escaping for
clink(cmd.exeset "K=V") exports to mitigate special-character expansion/injection vectors.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| internal/shell/zsh.go | Removes the unsafe PATH special-case and relies on the shared escaping helper for exports. |
| internal/shell/clink.go | Escapes keys/values for set "KEY=VALUE" and strips CR/LF to prevent script breaking/injection. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func (b clink) set(key, value string) string { | ||
| return fmt.Sprintf("set \"%s=%s\"\n", key, value) | ||
| return fmt.Sprintf("set \"%s=%s\"\n", escapeCmdSet(key), escapeCmdSet(value)) | ||
| } | ||
|
|
||
| func escapeCmdSet(value string) string { | ||
| value = strings.ReplaceAll(value, "\r", " ") | ||
| value = strings.ReplaceAll(value, "\n", " ") | ||
| return strings.NewReplacer( | ||
| "^", "^^", | ||
| "\"", "^\"", | ||
| "%", "%%", | ||
| "!", "^!", | ||
| ).Replace(value) |
There was a problem hiding this comment.
clink.set() previously interpolated key and value directly into set "KEY=VALUE", which is a common injection vector in cmd/batch contexts (quotes/newlines/%/delayed expansion). Please add unit tests covering escapeCmdSet() for cases like embedded ", %VAR%, !VAR! (delayed expansion), and CR/LF to ensure the generated activation script can't be broken out of.
| func escapeCmdSet(value string) string { | ||
| value = strings.ReplaceAll(value, "\r", " ") | ||
| value = strings.ReplaceAll(value, "\n", " ") | ||
| return strings.NewReplacer( | ||
| "^", "^^", | ||
| "\"", "^\"", | ||
| "%", "%%", | ||
| "!", "^!", | ||
| ).Replace(value) |
There was a problem hiding this comment.
escapeCmdSet() allocates a new strings.Replacer on every call. Since Export() may run frequently and processes multiple env vars, consider moving the replacer to a package-level var (and just calling .Replace) to reduce per-export allocations.
| func (z zsh) export(key, value string) string { | ||
| // Use double quotes for PATH-like variables to avoid unnecessary ANSI-C quoting | ||
| if key == "PATH" { | ||
| return fmt.Sprintf("export %s=\"%s\";", key, value) | ||
| } | ||
| return "export " + z.escape(key) + "=" + z.escape(value) + ";" | ||
| } |
There was a problem hiding this comment.
This change fixes PATH escaping for zsh, but the same unescaped PATH interpolation pattern still exists in the bash shell exporter (internal/shell/bash.go:75-80 uses export PATH="%s";). That leaves the overall activation surface vulnerable for bash users; consider applying the same escaping approach there (or consolidating PATH handling across bash/zsh to avoid drift).
| func (z zsh) Export(envs env.Vars) (out string) { | ||
| for key, value := range envs { | ||
| if value == nil { | ||
| out += z.unset(key) | ||
| } else { | ||
| out += z.export(key, *value) | ||
| } | ||
| } | ||
| return out | ||
| } | ||
|
|
||
| func (z zsh) export(key, value string) string { | ||
| // Use double quotes for PATH-like variables to avoid unnecessary ANSI-C quoting | ||
| if key == "PATH" { | ||
| return fmt.Sprintf("export %s=\"%s\";", key, value) | ||
| } | ||
| return "export " + z.escape(key) + "=" + z.escape(value) + ";" | ||
| } |
There was a problem hiding this comment.
There are no unit tests covering Zsh.Export() output escaping. Since this is addressing a high-severity command-injection vector (e.g., PATH segments containing $()/backticks), please add a test that asserts exported values are safely quoted/escaped and do not appear as raw interpolated content in the generated script.
Summary
fix(security): 2 improvements across 2 files
Problem
Severity:
High| File:internal/shell/zsh.go:L78The
zsh.exportmethod special-casesPATHand interpolates it directly into a double-quoted shell assignment (export PATH="%s";) without escaping. If a PATH segment contains shell metacharacters or command substitution syntax (e.g.,$(...)or backticks), it can be evaluated when the activation script is sourced, leading to code execution in the user shell context.Solution
Apply the same escaping logic used for other variables to PATH values (or use a robust shell-quoting helper for all vars). Avoid raw interpolation into double quotes.
Changes
internal/shell/zsh.go(modified)internal/shell/clink.go(modified)