Skip to content

Commit 14130de

Browse files
dannykoppingdannykopping
authored
fix: clean template destination path for pull (coder#12559)
1 parent 395bf54 commit 14130de

File tree

2 files changed

+116
-110
lines changed

2 files changed

+116
-110
lines changed

‎cli/templatepull.go

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"bytes"
55
"fmt"
66
"os"
7+
"path/filepath"
78
"sort"
89

910
"github.com/codeclysm/extract/v3"
@@ -130,6 +131,13 @@ func (r *RootCmd) templatePull() *clibase.Cmd {
130131
dest = templateName
131132
}
132133

134+
clean, err := filepath.Abs(filepath.Clean(dest))
135+
if err != nil {
136+
return xerrors.Errorf("cleaning destination path %s failed: %w", dest, err)
137+
}
138+
139+
dest = clean
140+
133141
err = os.MkdirAll(dest, 0o750)
134142
if err != nil {
135143
return xerrors.Errorf("mkdirall %q: %w", dest, err)

‎cli/templatepull_test.go

Lines changed: 108 additions & 110 deletions
Original file line numberDiff line numberDiff line change
@@ -230,118 +230,116 @@ func TestTemplatePull_LatestStdout(t *testing.T) {
230230

231231
// ToDir tests that 'templates pull' pulls down the active template
232232
// and writes it to the correct directory.
233+
//
234+
// nolint: paralleltest // The subtests cannot be run in parallel; see the inner loop.
233235
func TestTemplatePull_ToDir(t *testing.T) {
234-
t.Parallel()
235-
236-
client := coderdtest.New(t, &coderdtest.Options{
237-
IncludeProvisionerDaemon: true,
238-
})
239-
owner := coderdtest.CreateFirstUser(t, client)
240-
templateAdmin, _ := coderdtest.CreateAnotherUser(t, client, owner.OrganizationID, rbac.RoleTemplateAdmin())
241-
242-
// Create an initial template bundle.
243-
source1 := genTemplateVersionSource()
244-
// Create an updated template bundle. This will be used to ensure
245-
// that templates are correctly returned in order from latest to oldest.
246-
source2 := genTemplateVersionSource()
247-
248-
expected, err := echo.Tar(source2)
249-
require.NoError(t, err)
250-
251-
version1 := coderdtest.CreateTemplateVersion(t, client, owner.OrganizationID, source1)
252-
_ = coderdtest.AwaitTemplateVersionJobCompleted(t, client, version1.ID)
253-
254-
template := coderdtest.CreateTemplate(t, client, owner.OrganizationID, version1.ID)
255-
256-
// Update the template version so that we can assert that templates
257-
// are being sorted correctly.
258-
updatedVersion := coderdtest.UpdateTemplateVersion(t, client, owner.OrganizationID, source2, template.ID)
259-
_ = coderdtest.AwaitTemplateVersionJobCompleted(t, client, updatedVersion.ID)
260-
coderdtest.UpdateActiveTemplateVersion(t, client, template.ID, updatedVersion.ID)
261-
262-
dir := t.TempDir()
263-
264-
expectedDest := filepath.Join(dir, "expected")
265-
actualDest := filepath.Join(dir, "actual")
266-
ctx := context.Background()
267-
268-
err = extract.Tar(ctx, bytes.NewReader(expected), expectedDest, nil)
269-
require.NoError(t, err)
270-
271-
inv, root := clitest.New(t, "templates", "pull", template.Name, actualDest)
272-
clitest.SetupConfig(t, templateAdmin, root)
273-
274-
ptytest.New(t).Attach(inv)
275-
276-
require.NoError(t, inv.Run())
277-
278-
require.Equal(t,
279-
dirSum(t, expectedDest),
280-
dirSum(t, actualDest),
281-
)
282-
}
283-
284-
// ToDir tests that 'templates pull' pulls down the active template and writes
285-
// it to a directory with the name of the template if the path is not implicitly
286-
// supplied.
287-
// nolint: paralleltest
288-
func TestTemplatePull_ToImplicit(t *testing.T) {
289-
client := coderdtest.New(t, &coderdtest.Options{
290-
IncludeProvisionerDaemon: true,
291-
})
292-
owner := coderdtest.CreateFirstUser(t, client)
293-
templateAdmin, _ := coderdtest.CreateAnotherUser(t, client, owner.OrganizationID, rbac.RoleTemplateAdmin())
294-
295-
// Create an initial template bundle.
296-
source1 := genTemplateVersionSource()
297-
// Create an updated template bundle. This will be used to ensure
298-
// that templates are correctly returned in order from latest to oldest.
299-
source2 := genTemplateVersionSource()
300-
301-
expected, err := echo.Tar(source2)
302-
require.NoError(t, err)
303-
304-
version1 := coderdtest.CreateTemplateVersion(t, client, owner.OrganizationID, source1)
305-
_ = coderdtest.AwaitTemplateVersionJobCompleted(t, client, version1.ID)
306-
307-
template := coderdtest.CreateTemplate(t, client, owner.OrganizationID, version1.ID)
308-
309-
// Update the template version so that we can assert that templates
310-
// are being sorted correctly.
311-
updatedVersion := coderdtest.UpdateTemplateVersion(t, client, owner.OrganizationID, source2, template.ID)
312-
_ = coderdtest.AwaitTemplateVersionJobCompleted(t, client, updatedVersion.ID)
313-
coderdtest.UpdateActiveTemplateVersion(t, client, template.ID, updatedVersion.ID)
314-
315-
// create a tempdir and change the working directory to it for the duration of the test (cannot run in parallel)
316-
dir := t.TempDir()
317-
wd, err := os.Getwd()
318-
require.NoError(t, err)
319-
err = os.Chdir(dir)
320-
require.NoError(t, err)
321-
defer func() {
322-
err := os.Chdir(wd)
323-
require.NoError(t, err, "if this fails, it can break other subsequent tests due to wrong working directory")
324-
}()
325-
326-
expectedDest := filepath.Join(dir, "expected")
327-
actualDest := filepath.Join(dir, template.Name)
328-
329-
ctx := context.Background()
330-
331-
err = extract.Tar(ctx, bytes.NewReader(expected), expectedDest, nil)
332-
require.NoError(t, err)
333-
334-
inv, root := clitest.New(t, "templates", "pull", template.Name)
335-
clitest.SetupConfig(t, templateAdmin, root)
336-
337-
ptytest.New(t).Attach(inv)
338-
339-
require.NoError(t, inv.Run())
236+
tests := []struct {
237+
name string
238+
destPath string
239+
useDefaultDest bool
240+
}{
241+
{
242+
name: "absolute path works",
243+
useDefaultDest: true,
244+
},
245+
{
246+
name: "relative path to specific dir is sanitized",
247+
destPath: "./pulltmp",
248+
},
249+
{
250+
name: "relative path to current dir is sanitized",
251+
destPath: ".",
252+
},
253+
{
254+
name: "directory traversal is acceptable",
255+
destPath: "../mytmpl",
256+
},
257+
{
258+
name: "empty path falls back to using template name",
259+
destPath: "",
260+
},
261+
}
340262

341-
require.Equal(t,
342-
dirSum(t, expectedDest),
343-
dirSum(t, actualDest),
344-
)
263+
// nolint: paralleltest // These tests change the current working dir, and is therefore unsuitable for parallelisation.
264+
for _, tc := range tests {
265+
tc := tc
266+
267+
t.Run(tc.name, func(t *testing.T) {
268+
dir := t.TempDir()
269+
270+
cwd, err := os.Getwd()
271+
require.NoError(t, err)
272+
t.Cleanup(func() {
273+
require.NoError(t, os.Chdir(cwd))
274+
})
275+
276+
// Change working directory so that relative path tests don't affect the original working directory.
277+
newWd := filepath.Join(dir, "new-cwd")
278+
require.NoError(t, os.MkdirAll(newWd, 0o750))
279+
require.NoError(t, os.Chdir(newWd))
280+
281+
expectedDest := filepath.Join(dir, "expected")
282+
actualDest := tc.destPath
283+
if tc.useDefaultDest {
284+
actualDest = filepath.Join(dir, "actual")
285+
}
286+
287+
client := coderdtest.New(t, &coderdtest.Options{
288+
IncludeProvisionerDaemon: true,
289+
})
290+
owner := coderdtest.CreateFirstUser(t, client)
291+
templateAdmin, _ := coderdtest.CreateAnotherUser(t, client, owner.OrganizationID, rbac.RoleTemplateAdmin())
292+
293+
// Create an initial template bundle.
294+
source1 := genTemplateVersionSource()
295+
// Create an updated template bundle. This will be used to ensure
296+
// that templates are correctly returned in order from latest to oldest.
297+
source2 := genTemplateVersionSource()
298+
299+
expected, err := echo.Tar(source2)
300+
require.NoError(t, err)
301+
302+
version1 := coderdtest.CreateTemplateVersion(t, client, owner.OrganizationID, source1)
303+
_ = coderdtest.AwaitTemplateVersionJobCompleted(t, client, version1.ID)
304+
305+
template := coderdtest.CreateTemplate(t, client, owner.OrganizationID, version1.ID)
306+
307+
// Update the template version so that we can assert that templates
308+
// are being sorted correctly.
309+
updatedVersion := coderdtest.UpdateTemplateVersion(t, client, owner.OrganizationID, source2, template.ID)
310+
_ = coderdtest.AwaitTemplateVersionJobCompleted(t, client, updatedVersion.ID)
311+
coderdtest.UpdateActiveTemplateVersion(t, client, template.ID, updatedVersion.ID)
312+
313+
ctx := context.Background()
314+
315+
err = extract.Tar(ctx, bytes.NewReader(expected), expectedDest, nil)
316+
require.NoError(t, err)
317+
318+
ents, _ := os.ReadDir(actualDest)
319+
if len(ents) > 0 {
320+
t.Logf("%s is not empty", actualDest)
321+
t.FailNow()
322+
}
323+
324+
inv, root := clitest.New(t, "templates", "pull", template.Name, actualDest)
325+
clitest.SetupConfig(t, templateAdmin, root)
326+
327+
ptytest.New(t).Attach(inv)
328+
329+
require.NoError(t, inv.Run())
330+
331+
// Validate behaviour of choosing template name in the absence of an output path argument.
332+
destPath := actualDest
333+
if destPath == "" {
334+
destPath = template.Name
335+
}
336+
337+
require.Equal(t,
338+
dirSum(t, expectedDest),
339+
dirSum(t, destPath),
340+
)
341+
})
342+
}
345343
}
346344

347345
// FolderConflict tests that 'templates pull' fails when a folder with has

0 commit comments

Comments
 (0)