Skip to content

Commit 608ba98

Browse files
gsquared94gsquared94
authored
fix: sanitize helm setFiles property (#5648)
1 parent efd6c5b commit 608ba98

File tree

3 files changed

+179
-1
lines changed

3 files changed

+179
-1
lines changed

‎pkg/skaffold/deploy/helm/args.go

+53
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ package helm
1818

1919
import (
2020
"fmt"
21+
"runtime"
2122
"strconv"
2223

2324
"github.com/blang/semver"
@@ -55,6 +56,8 @@ func constructOverrideArgs(r *latest_v1.HelmRelease, builds []graph.Artifact, ar
5556
if err != nil {
5657
return nil, fmt.Errorf("unable to expand %q: %w", r.SetFiles[k], err)
5758
}
59+
exp = sanitizeFilePath(exp, runtime.GOOS == "windows")
60+
5861
record(exp)
5962
args = append(args, "--set-file", fmt.Sprintf("%s=%s", k, exp))
6063
}
@@ -201,3 +204,53 @@ func (h *Deployer) installArgs(r latest_v1.HelmRelease, builds []graph.Artifact,
201204

202205
return args, nil
203206
}
207+
208+
// sanitizeFilePath is used to sanitize filepaths that are provided to the `setFiles` flag
209+
// helm `setFiles` doesn't work with the unescaped filepath separator (\) for Windows or if there are unescaped tabs and spaces in the directory names.
210+
// So we escape all odd count occurrences of `\` for Windows, and wrap the entire string in quotes if it has spaces.
211+
// This is very specific to the way helm handles its flags.
212+
// See https://github.com/helm/helm/blob/d55c53df4e394fb62b0514a09c57bce235dd7877/pkg/cli/values/options.
213+
// Otherwise the windows `syscall` package implements its own sanitizing for command args that's used by `exec.Cmd`.
214+
// See https://github.com/golang/go/blob/6951da56b0ae2cd4250fc1b0350d090aed633ac1/src/syscall/exec_windows.go#L27
215+
func sanitizeFilePath(s string, isWindowsOS bool) string {
216+
if len(s) == 0 {
217+
return `""`
218+
}
219+
needsQuotes := false
220+
for i := 0; i < len(s); i++ {
221+
if s[i] == ' ' || s[i] == '\t' {
222+
needsQuotes = true
223+
break
224+
}
225+
}
226+
227+
if !isWindowsOS {
228+
if needsQuotes {
229+
return fmt.Sprintf(`"%s"`, s)
230+
}
231+
return s
232+
}
233+
234+
var b []byte
235+
slashes := 0
236+
for i := 0; i < len(s); i++ {
237+
switch s[i] {
238+
case '\\':
239+
slashes++
240+
default:
241+
// ensure a single slash is escaped
242+
if slashes == 1 {
243+
b = append(b, '\\')
244+
}
245+
slashes = 0
246+
}
247+
b = append(b, s[i])
248+
}
249+
if slashes == 1 {
250+
b = append(b, '\\')
251+
}
252+
if needsQuotes {
253+
return fmt.Sprintf(`"%s"`, string(b))
254+
}
255+
return string(b)
256+
}

‎pkg/skaffold/deploy/helm/args_test.go

+124
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,124 @@
1+
/*
2+
Copyright 2021 The Skaffold Authors
3+
4+
Licensed under the Apache License, Version 2.0 (the "License");
5+
you may not use this file except in compliance with the License.
6+
You may obtain a copy of the License at
7+
8+
http://www.apache.org/licenses/LICENSE-2.0
9+
10+
Unless required by applicable law or agreed to in writing, software
11+
distributed under the License is distributed on an "AS IS" BASIS,
12+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
See the License for the specific language governing permissions and
14+
limitations under the License.
15+
*/
16+
17+
package helm
18+
19+
import (
20+
"testing"
21+
22+
"github.com/GoogleContainerTools/skaffold/testutil"
23+
)
24+
25+
func TestSanitizeFilePath(t *testing.T) {
26+
tests := []struct {
27+
description string
28+
isWindowsOS bool
29+
input string
30+
output string
31+
}{
32+
{
33+
description: "unescaped relative path on Windows",
34+
isWindowsOS: true,
35+
input: `.\foo\not.escaped.relative.yaml`,
36+
output: `.\\foo\\not.escaped.relative.yaml`,
37+
},
38+
{
39+
description: "unescaped absolute path on Windows",
40+
isWindowsOS: true,
41+
input: `C:\Users\foo\not.escaped.abs.yaml`,
42+
output: `C:\\Users\\foo\\not.escaped.abs.yaml`,
43+
},
44+
{
45+
description: "escaped relative path on Windows",
46+
isWindowsOS: true,
47+
input: `.\\foo\\escaped.relative.yaml`,
48+
output: `.\\foo\\escaped.relative.yaml`,
49+
},
50+
{
51+
description: "escaped absolute path on Windows",
52+
isWindowsOS: true,
53+
input: `C:\\Users\\foo\\escaped.abs.yaml`,
54+
output: `C:\\Users\\foo\\escaped.abs.yaml`,
55+
},
56+
{
57+
description: "escaped relative path with spaces on Windows",
58+
isWindowsOS: true,
59+
input: `.\\foo bar\\escaped.spaces.relative.yaml`,
60+
output: `".\\foo bar\\escaped.spaces.relative.yaml"`,
61+
},
62+
{
63+
description: "escaped absolute path with spaces on Windows",
64+
isWindowsOS: true,
65+
input: `C:\\Users\\foo bar\\escaped.spaces.abs.yaml`,
66+
output: `"C:\\Users\\foo bar\\escaped.spaces.abs.yaml"`,
67+
},
68+
{
69+
description: "unescaped relative path with spaces on Windows",
70+
isWindowsOS: true,
71+
input: `.\foo bar\not.escaped.spaces.relative.yaml`,
72+
output: `".\\foo bar\\not.escaped.spaces.relative.yaml"`,
73+
},
74+
{
75+
description: "unescaped absolute path with spaces on Windows",
76+
isWindowsOS: true,
77+
input: `C:\Users\foo bar\not.escaped.spaces.abs.yaml`,
78+
output: `"C:\\Users\\foo bar\\not.escaped.spaces.abs.yaml"`,
79+
},
80+
{
81+
description: "relative path on non-Windows",
82+
input: `./foo/spaces.relative.yaml`,
83+
output: `./foo/spaces.relative.yaml`,
84+
},
85+
{
86+
description: "absolute path on non-Windows",
87+
input: `z/foo/spaces.abs.yaml`,
88+
output: `z/foo/spaces.abs.yaml`,
89+
},
90+
{
91+
description: "relative path with spaces on non-Windows",
92+
input: `./foo bar/spaces.relative.yaml`,
93+
output: `"./foo bar/spaces.relative.yaml"`,
94+
},
95+
{
96+
description: "absolute path with spaces on non-Windows",
97+
input: `z/foo bar/spaces.abs.yaml`,
98+
output: `"z/foo bar/spaces.abs.yaml"`,
99+
},
100+
{
101+
description: "unescaped relative dir path on Windows",
102+
isWindowsOS: true,
103+
input: `.\foo\not_escaped_relative\`,
104+
output: `.\\foo\\not_escaped_relative\\`,
105+
},
106+
{
107+
description: "escaped relative dir path on Windows",
108+
isWindowsOS: true,
109+
input: `.\\foo\\escaped_relative\\`,
110+
output: `.\\foo\\escaped_relative\\`,
111+
},
112+
{
113+
description: "unescaped relative dir path on non-Windows",
114+
input: `./foo/not_escaped_relative/`,
115+
output: `./foo/not_escaped_relative/`,
116+
},
117+
}
118+
for _, test := range tests {
119+
testutil.Run(t, test.description, func(t *testutil.T) {
120+
output := sanitizeFilePath(test.input, test.isWindowsOS)
121+
t.CheckDeepEqual(test.output, output)
122+
})
123+
}
124+
}

‎pkg/skaffold/deploy/helm/helm_test.go

+2-1
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ import (
2121
"fmt"
2222
"io/ioutil"
2323
"path/filepath"
24+
"strings"
2425
"testing"
2526

2627
"github.com/mitchellh/go-homedir"
@@ -876,7 +877,7 @@ func TestHelmDeploy(t *testing.T) {
876877
CmdRunWithOutput("helm version --client", version31).
877878
AndRun("helm --kube-context kubecontext get all skaffold-helm --kubeconfig kubeconfig").
878879
AndRun("helm --kube-context kubecontext dep build examples/test --kubeconfig kubeconfig").
879-
AndRun(fmt.Sprintf("helm --kube-context kubecontext upgrade skaffold-helm examples/test -f skaffold-overrides.yaml --set-string image=docker.io:5000/skaffold-helm:3605e7bc17cf46e53f4d81c4cbc24e5b4c495184 --set-file expanded=%s --set-file value=/some/file.yaml --kubeconfig kubeconfig", filepath.Join(home, "file.yaml"))).
880+
AndRun(fmt.Sprintf("helm --kube-context kubecontext upgrade skaffold-helm examples/test -f skaffold-overrides.yaml --set-string image=docker.io:5000/skaffold-helm:3605e7bc17cf46e53f4d81c4cbc24e5b4c495184 --set-file expanded=%s --set-file value=/some/file.yaml --kubeconfig kubeconfig", strings.ReplaceAll(filepath.Join(home, "file.yaml"), "\\", "\\\\"))).
880881
AndRun("helm --kube-context kubecontext get all skaffold-helm --template {{.Release.Manifest}} --kubeconfig kubeconfig"),
881882
helm: testDeployConfigSetFiles,
882883
builds: testBuilds,

0 commit comments

Comments
 (0)