X Tutup
Skip to content

Commit 5b4a08d

Browse files
mislavmislav
committed
Ensure that only PATH is searched when shelling out to external commands
Works around golang/go#38736 for Windows.
1 parent fa86743 commit 5b4a08d

File tree

15 files changed

+168
-46
lines changed

15 files changed

+168
-46
lines changed

‎cmd/gh/main.go‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ import (
2222
"github.com/cli/cli/pkg/cmdutil"
2323
"github.com/cli/cli/update"
2424
"github.com/cli/cli/utils"
25+
"github.com/cli/safeexec"
2526
"github.com/mattn/go-colorable"
2627
"github.com/mgutz/ansi"
2728
"github.com/spf13/cobra"
@@ -107,7 +108,13 @@ func main() {
107108
}
108109

109110
if isShell {
110-
externalCmd := exec.Command(expandedArgs[0], expandedArgs[1:]...)
111+
exe, err := safeexec.LookPath(expandedArgs[0])
112+
if err != nil {
113+
fmt.Fprintf(stderr, "failed to run external command: %s", err)
114+
os.Exit(3)
115+
}
116+
117+
externalCmd := exec.Command(exe, expandedArgs[1:]...)
111118
externalCmd.Stderr = os.Stderr
112119
externalCmd.Stdout = os.Stdout
113120
externalCmd.Stdin = os.Stdin

‎git/git.go‎

Lines changed: 70 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import (
1313
"strings"
1414

1515
"github.com/cli/cli/internal/run"
16+
"github.com/cli/safeexec"
1617
)
1718

1819
// ErrNotOnAnyBranch indicates that the user is in detached HEAD state
@@ -37,7 +38,10 @@ func (r TrackingRef) String() string {
3738
// ShowRefs resolves fully-qualified refs to commit hashes
3839
func ShowRefs(ref ...string) ([]Ref, error) {
3940
args := append([]string{"show-ref", "--verify", "--"}, ref...)
40-
showRef := exec.Command("git", args...)
41+
showRef, err := GitCommand(args...)
42+
if err != nil {
43+
return nil, err
44+
}
4145
output, err := run.PrepareCmd(showRef).Output()
4246

4347
var refs []Ref
@@ -57,7 +61,10 @@ func ShowRefs(ref ...string) ([]Ref, error) {
5761

5862
// CurrentBranch reads the checked-out branch for the git repository
5963
func CurrentBranch() (string, error) {
60-
refCmd := GitCommand("symbolic-ref", "--quiet", "HEAD")
64+
refCmd, err := GitCommand("symbolic-ref", "--quiet", "HEAD")
65+
if err != nil {
66+
return "", err
67+
}
6168

6269
output, err := run.PrepareCmd(refCmd).Output()
6370
if err == nil {
@@ -78,13 +85,19 @@ func CurrentBranch() (string, error) {
7885
}
7986

8087
func listRemotes() ([]string, error) {
81-
remoteCmd := exec.Command("git", "remote", "-v")
88+
remoteCmd, err := GitCommand("remote", "-v")
89+
if err != nil {
90+
return nil, err
91+
}
8292
output, err := run.PrepareCmd(remoteCmd).Output()
8393
return outputLines(output), err
8494
}
8595

8696
func Config(name string) (string, error) {
87-
configCmd := exec.Command("git", "config", name)
97+
configCmd, err := GitCommand("config", name)
98+
if err != nil {
99+
return "", err
100+
}
88101
output, err := run.PrepareCmd(configCmd).Output()
89102
if err != nil {
90103
return "", fmt.Errorf("unknown config key: %s", name)
@@ -94,12 +107,19 @@ func Config(name string) (string, error) {
94107

95108
}
96109

97-
var GitCommand = func(args ...string) *exec.Cmd {
98-
return exec.Command("git", args...)
110+
var GitCommand = func(args ...string) (*exec.Cmd, error) {
111+
gitExe, err := safeexec.LookPath("git")
112+
if err != nil {
113+
return nil, err
114+
}
115+
return exec.Command(gitExe, args...), nil
99116
}
100117

101118
func UncommittedChangeCount() (int, error) {
102-
statusCmd := GitCommand("status", "--porcelain")
119+
statusCmd, err := GitCommand("status", "--porcelain")
120+
if err != nil {
121+
return 0, err
122+
}
103123
output, err := run.PrepareCmd(statusCmd).Output()
104124
if err != nil {
105125
return 0, err
@@ -123,10 +143,13 @@ type Commit struct {
123143
}
124144

125145
func Commits(baseRef, headRef string) ([]*Commit, error) {
126-
logCmd := GitCommand(
146+
logCmd, err := GitCommand(
127147
"-c", "log.ShowSignature=false",
128148
"log", "--pretty=format:%H,%s",
129149
"--cherry", fmt.Sprintf("%s...%s", baseRef, headRef))
150+
if err != nil {
151+
return nil, err
152+
}
130153
output, err := run.PrepareCmd(logCmd).Output()
131154
if err != nil {
132155
return []*Commit{}, err
@@ -154,7 +177,10 @@ func Commits(baseRef, headRef string) ([]*Commit, error) {
154177
}
155178

156179
func CommitBody(sha string) (string, error) {
157-
showCmd := GitCommand("-c", "log.ShowSignature=false", "show", "-s", "--pretty=format:%b", sha)
180+
showCmd, err := GitCommand("-c", "log.ShowSignature=false", "show", "-s", "--pretty=format:%b", sha)
181+
if err != nil {
182+
return "", err
183+
}
158184
output, err := run.PrepareCmd(showCmd).Output()
159185
if err != nil {
160186
return "", err
@@ -164,7 +190,10 @@ func CommitBody(sha string) (string, error) {
164190

165191
// Push publishes a git ref to a remote and sets up upstream configuration
166192
func Push(remote string, ref string, cmdOut, cmdErr io.Writer) error {
167-
pushCmd := GitCommand("push", "--set-upstream", remote, ref)
193+
pushCmd, err := GitCommand("push", "--set-upstream", remote, ref)
194+
if err != nil {
195+
return err
196+
}
168197
pushCmd.Stdout = cmdOut
169198
pushCmd.Stderr = cmdErr
170199
return run.PrepareCmd(pushCmd).Run()
@@ -179,7 +208,10 @@ type BranchConfig struct {
179208
// ReadBranchConfig parses the `branch.BRANCH.(remote|merge)` part of git config
180209
func ReadBranchConfig(branch string) (cfg BranchConfig) {
181210
prefix := regexp.QuoteMeta(fmt.Sprintf("branch.%s.", branch))
182-
configCmd := GitCommand("config", "--get-regexp", fmt.Sprintf("^%s(remote|merge)$", prefix))
211+
configCmd, err := GitCommand("config", "--get-regexp", fmt.Sprintf("^%s(remote|merge)$", prefix))
212+
if err != nil {
213+
return
214+
}
183215
output, err := run.PrepareCmd(configCmd).Output()
184216
if err != nil {
185217
return
@@ -209,21 +241,28 @@ func ReadBranchConfig(branch string) (cfg BranchConfig) {
209241
}
210242

211243
func DeleteLocalBranch(branch string) error {
212-
branchCmd := GitCommand("branch", "-D", branch)
213-
err := run.PrepareCmd(branchCmd).Run()
214-
return err
244+
branchCmd, err := GitCommand("branch", "-D", branch)
245+
if err != nil {
246+
return err
247+
}
248+
return run.PrepareCmd(branchCmd).Run()
215249
}
216250

217251
func HasLocalBranch(branch string) bool {
218-
configCmd := GitCommand("rev-parse", "--verify", "refs/heads/"+branch)
219-
_, err := run.PrepareCmd(configCmd).Output()
252+
configCmd, err := GitCommand("rev-parse", "--verify", "refs/heads/"+branch)
253+
if err != nil {
254+
return false
255+
}
256+
_, err = run.PrepareCmd(configCmd).Output()
220257
return err == nil
221258
}
222259

223260
func CheckoutBranch(branch string) error {
224-
configCmd := GitCommand("checkout", branch)
225-
err := run.PrepareCmd(configCmd).Run()
226-
return err
261+
configCmd, err := GitCommand("checkout", branch)
262+
if err != nil {
263+
return err
264+
}
265+
return run.PrepareCmd(configCmd).Run()
227266
}
228267

229268
func parseCloneArgs(extraArgs []string) (args []string, target string) {
@@ -252,7 +291,10 @@ func RunClone(cloneURL string, args []string) (target string, err error) {
252291

253292
cloneArgs = append([]string{"clone"}, cloneArgs...)
254293

255-
cloneCmd := GitCommand(cloneArgs...)
294+
cloneCmd, err := GitCommand(cloneArgs...)
295+
if err != nil {
296+
return "", err
297+
}
256298
cloneCmd.Stdin = os.Stdin
257299
cloneCmd.Stdout = os.Stdout
258300
cloneCmd.Stderr = os.Stderr
@@ -262,7 +304,10 @@ func RunClone(cloneURL string, args []string) (target string, err error) {
262304
}
263305

264306
func AddUpstreamRemote(upstreamURL, cloneDir string) error {
265-
cloneCmd := GitCommand("-C", cloneDir, "remote", "add", "-f", "upstream", upstreamURL)
307+
cloneCmd, err := GitCommand("-C", cloneDir, "remote", "add", "-f", "upstream", upstreamURL)
308+
if err != nil {
309+
return err
310+
}
266311
cloneCmd.Stdout = os.Stdout
267312
cloneCmd.Stderr = os.Stderr
268313
return run.PrepareCmd(cloneCmd).Run()
@@ -274,7 +319,10 @@ func isFilesystemPath(p string) bool {
274319

275320
// ToplevelDir returns the top-level directory path of the current repository
276321
func ToplevelDir() (string, error) {
277-
showCmd := exec.Command("git", "rev-parse", "--show-toplevel")
322+
showCmd, err := GitCommand("rev-parse", "--show-toplevel")
323+
if err != nil {
324+
return "", err
325+
}
278326
output, err := run.PrepareCmd(showCmd).Output()
279327
return firstLine(output), err
280328

‎git/remote.go‎

Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@ package git
33
import (
44
"fmt"
55
"net/url"
6-
"os/exec"
76
"regexp"
87
"strings"
98

@@ -45,7 +44,10 @@ func Remotes() (RemoteSet, error) {
4544
remotes := parseRemotes(list)
4645

4746
// this is affected by SetRemoteResolution
48-
remoteCmd := exec.Command("git", "config", "--get-regexp", `^remote\..*\.gh-resolved$`)
47+
remoteCmd, err := GitCommand("config", "--get-regexp", `^remote\..*\.gh-resolved$`)
48+
if err != nil {
49+
return nil, err
50+
}
4951
output, _ := run.PrepareCmd(remoteCmd).Output()
5052
for _, l := range outputLines(output) {
5153
parts := strings.SplitN(l, " ", 2)
@@ -107,8 +109,11 @@ func parseRemotes(gitRemotes []string) (remotes RemoteSet) {
107109

108110
// AddRemote adds a new git remote and auto-fetches objects from it
109111
func AddRemote(name, u string) (*Remote, error) {
110-
addCmd := exec.Command("git", "remote", "add", "-f", name, u)
111-
err := run.PrepareCmd(addCmd).Run()
112+
addCmd, err := GitCommand("remote", "add", "-f", name, u)
113+
if err != nil {
114+
return nil, err
115+
}
116+
err = run.PrepareCmd(addCmd).Run()
112117
if err != nil {
113118
return nil, err
114119
}
@@ -136,6 +141,9 @@ func AddRemote(name, u string) (*Remote, error) {
136141
}
137142

138143
func SetRemoteResolution(name, resolution string) error {
139-
addCmd := exec.Command("git", "config", "--add", fmt.Sprintf("remote.%s.gh-resolved", name), resolution)
144+
addCmd, err := GitCommand("config", "--add", fmt.Sprintf("remote.%s.gh-resolved", name), resolution)
145+
if err != nil {
146+
return err
147+
}
140148
return run.PrepareCmd(addCmd).Run()
141149
}

‎go.mod‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ require (
77
github.com/MakeNowJust/heredoc v1.0.0
88
github.com/briandowns/spinner v1.11.1
99
github.com/charmbracelet/glamour v0.2.1-0.20200724174618-1246d13c1684
10+
github.com/cli/safeexec v1.0.0
1011
github.com/cpuguy83/go-md2man/v2 v2.0.0
1112
github.com/enescakir/emoji v1.0.0
1213
github.com/google/go-cmp v0.5.2

‎go.sum‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,8 @@ github.com/briandowns/spinner v1.11.1/go.mod h1:QOuQk7x+EaDASo80FEXwlwiA+j/PPIcX
4444
github.com/cespare/xxhash v1.1.0/go.mod h1:XrSqR1VqqWfGrhpAt58auRo0WTKS1nRRg3ghfAqPWnc=
4545
github.com/charmbracelet/glamour v0.2.1-0.20200724174618-1246d13c1684 h1:YMyvXRstOQc7n6eneHfudVMbARSCmZ7EZGjtTkkeB3A=
4646
github.com/charmbracelet/glamour v0.2.1-0.20200724174618-1246d13c1684/go.mod h1:UA27Kwj3QHialP74iU6C+Gpc8Y7IOAKupeKMLLBURWM=
47+
github.com/cli/safeexec v1.0.0 h1:0VngyaIyqACHdcMNWfo6+KdUYnqEr2Sg+bSP1pdF+dI=
48+
github.com/cli/safeexec v1.0.0/go.mod h1:Z/D4tTN8Vs5gXYHDCbaM1S/anmEDnJb1iW0+EJ5zx3Q=
4749
github.com/cli/shurcooL-graphql v0.0.0-20200707151639-0f7232a2bf7e h1:aq/1jlmtZoS6nlSp3yLOTZQ50G+dzHdeRNENgE/iBew=
4850
github.com/cli/shurcooL-graphql v0.0.0-20200707151639-0f7232a2bf7e/go.mod h1:it23pLwxmz6OyM6I5O0ATIXQS1S190Nas26L5Kahp4U=
4951
github.com/client9/misspell v0.3.4/go.mod h1:qj6jICC3Q7zFZvVWo7KLAzC3yx5G7kyvSDkc90ppPyw=

‎internal/run/run.go‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,13 @@ var PrepareCmd = func(cmd *exec.Cmd) Runnable {
2323
// SetPrepareCmd overrides PrepareCmd and returns a func to revert it back
2424
func SetPrepareCmd(fn func(*exec.Cmd) Runnable) func() {
2525
origPrepare := PrepareCmd
26-
PrepareCmd = fn
26+
PrepareCmd = func(cmd *exec.Cmd) Runnable {
27+
// normalize git executable name for consistency in tests
28+
if baseName := filepath.Base(cmd.Args[0]); baseName == "git" || baseName == "git.exe" {
29+
cmd.Args[0] = baseName
30+
}
31+
return fn(cmd)
32+
}
2733
return func() {
2834
PrepareCmd = origPrepare
2935
}

‎pkg/browser/browser.go‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import (
66
"runtime"
77
"strings"
88

9+
"github.com/cli/safeexec"
910
"github.com/google/shlex"
1011
)
1112

@@ -31,7 +32,7 @@ func ForOS(goos, url string) *exec.Cmd {
3132
case "darwin":
3233
args = append(args, url)
3334
case "windows":
34-
exe = "cmd"
35+
exe, _ = lookPath("cmd")
3536
r := strings.NewReplacer("&", "^&")
3637
args = append(args, "/c", "start", r.Replace(url))
3738
default:
@@ -51,8 +52,13 @@ func FromLauncher(launcher, url string) (*exec.Cmd, error) {
5152
return nil, err
5253
}
5354

55+
exe, err := lookPath(args[0])
56+
if err != nil {
57+
return nil, err
58+
}
59+
5460
args = append(args, url)
55-
cmd := exec.Command(args[0], args[1:]...)
61+
cmd := exec.Command(exe, args[1:]...)
5662
cmd.Stderr = os.Stderr
5763
return cmd, nil
5864
}
@@ -71,4 +77,4 @@ func linuxExe() string {
7177
return exe
7278
}
7379

74-
var lookPath = exec.LookPath
80+
var lookPath = safeexec.LookPath

‎pkg/browser/browser_test.go‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,17 +49,22 @@ func TestForOS(t *testing.T) {
4949
goos: "windows",
5050
url: "https://example.com/path?a=1&b=2&c=3",
5151
},
52+
exe: "cmd",
5253
want: []string{"cmd", "/c", "start", "https://example.com/path?a=1^&b=2^&c=3"},
5354
},
5455
}
5556
for _, tt := range tests {
57+
origLookPath := lookPath
5658
lookPath = func(file string) (string, error) {
5759
if file == tt.exe {
5860
return file, nil
5961
} else {
6062
return "", errors.New("not found")
6163
}
6264
}
65+
defer func() {
66+
lookPath = origLookPath
67+
}()
6368

6469
t.Run(tt.name, func(t *testing.T) {
6570
if cmd := ForOS(tt.args.goos, tt.args.url); !reflect.DeepEqual(cmd.Args, tt.want) {

‎pkg/cmd/alias/expand/expand.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,13 +4,13 @@ import (
44
"errors"
55
"fmt"
66
"os"
7-
"os/exec"
87
"path/filepath"
98
"regexp"
109
"runtime"
1110
"strings"
1211

1312
"github.com/cli/cli/internal/config"
13+
"github.com/cli/safeexec"
1414
"github.com/google/shlex"
1515
)
1616

@@ -80,15 +80,15 @@ func ExpandAlias(cfg config.Config, args []string, findShFunc func() (string, er
8080
}
8181

8282
func findSh() (string, error) {
83-
shPath, err := exec.LookPath("sh")
83+
shPath, err := safeexec.LookPath("sh")
8484
if err == nil {
8585
return shPath, nil
8686
}
8787

8888
if runtime.GOOS == "windows" {
8989
winNotFoundErr := errors.New("unable to locate sh to execute the shell alias with. The sh.exe interpreter is typically distributed with Git for Windows.")
9090
// We can try and find a sh executable in a Git for Windows install
91-
gitPath, err := exec.LookPath("git")
91+
gitPath, err := safeexec.LookPath("git")
9292
if err != nil {
9393
return "", winNotFoundErr
9494
}

0 commit comments

Comments
 (0)
X Tutup