fix: clean template destination path for pull (#12559)

cli/templatepull.go

@@ -4,6 +4,7 @@ import (
 	"bytes"
 	"fmt"
 	"os"
+	"path/filepath"
 	"sort"
 
 	"github.com/codeclysm/extract/v3"
@@ -130,6 +131,13 @@ func (r *RootCmd) templatePull() *clibase.Cmd {
 				dest = templateName
 			}
 
+			clean, err := filepath.Abs(filepath.Clean(dest))
+			if err != nil {
+				return xerrors.Errorf("cleaning destination path %s failed: %w", dest, err)
+			}
+
+			dest = clean
+
 			err = os.MkdirAll(dest, 0o750)
 			if err != nil {
 				return xerrors.Errorf("mkdirall %q: %w", dest, err)

cli/templatepull_test.go

@@ -230,118 +230,116 @@ func TestTemplatePull_LatestStdout(t *testing.T) {
 
 // ToDir tests that 'templates pull' pulls down the active template
 // and writes it to the correct directory.
+//
+// nolint: paralleltest // The subtests cannot be run in parallel; see the inner loop.
 func TestTemplatePull_ToDir(t *testing.T) {
-	t.Parallel()
+	tests := []struct {
+		name           string
+		destPath       string
+		useDefaultDest bool
+	}{
+		{
+			name:           "absolute path works",
+			useDefaultDest: true,
+		},
+		{
+			name:     "relative path to specific dir is sanitized",
+			destPath: "./pulltmp",
+		},
+		{
+			name:     "relative path to current dir is sanitized",
+			destPath: ".",
+		},
+		{
+			name:     "directory traversal is acceptable",
+			destPath: "../mytmpl",
+		},
+		{
+			name:     "empty path falls back to using template name",
+			destPath: "",
+		},
+	}
 
-	client := coderdtest.New(t, &coderdtest.Options{
+	// nolint: paralleltest // These tests change the current working dir, and is therefore unsuitable for parallelisation.
-		IncludeProvisionerDaemon: true,
+	for _, tc := range tests {
-	})
+		tc := tc
-	owner := coderdtest.CreateFirstUser(t, client)
-	templateAdmin, _ := coderdtest.CreateAnotherUser(t, client, owner.OrganizationID, rbac.RoleTemplateAdmin())
 
-	// Create an initial template bundle.
+		t.Run(tc.name, func(t *testing.T) {
-	source1 := genTemplateVersionSource()
+			dir := t.TempDir()
-	// Create an updated template bundle. This will be used to ensure
-	// that templates are correctly returned in order from latest to oldest.
-	source2 := genTemplateVersionSource()
 
-	expected, err := echo.Tar(source2)
+			cwd, err := os.Getwd()
-	require.NoError(t, err)
+			require.NoError(t, err)
+			t.Cleanup(func() {
+				require.NoError(t, os.Chdir(cwd))
+			})
 
-	version1 := coderdtest.CreateTemplateVersion(t, client, owner.OrganizationID, source1)
+			// Change working directory so that relative path tests don't affect the original working directory.
-	_ = coderdtest.AwaitTemplateVersionJobCompleted(t, client, version1.ID)
+			newWd := filepath.Join(dir, "new-cwd")
+			require.NoError(t, os.MkdirAll(newWd, 0o750))
+			require.NoError(t, os.Chdir(newWd))
 
-	template := coderdtest.CreateTemplate(t, client, owner.OrganizationID, version1.ID)
+			expectedDest := filepath.Join(dir, "expected")
+			actualDest := tc.destPath
+			if tc.useDefaultDest {
+				actualDest = filepath.Join(dir, "actual")
+			}
 
-	// Update the template version so that we can assert that templates
+			client := coderdtest.New(t, &coderdtest.Options{
-	// are being sorted correctly.
+				IncludeProvisionerDaemon: true,
-	updatedVersion := coderdtest.UpdateTemplateVersion(t, client, owner.OrganizationID, source2, template.ID)
+			})
-	_ = coderdtest.AwaitTemplateVersionJobCompleted(t, client, updatedVersion.ID)
+			owner := coderdtest.CreateFirstUser(t, client)
-	coderdtest.UpdateActiveTemplateVersion(t, client, template.ID, updatedVersion.ID)
+			templateAdmin, _ := coderdtest.CreateAnotherUser(t, client, owner.OrganizationID, rbac.RoleTemplateAdmin())
 
-	dir := t.TempDir()
+			// Create an initial template bundle.
+			source1 := genTemplateVersionSource()
+			// Create an updated template bundle. This will be used to ensure
+			// that templates are correctly returned in order from latest to oldest.
+			source2 := genTemplateVersionSource()
 
-	expectedDest := filepath.Join(dir, "expected")
+			expected, err := echo.Tar(source2)
-	actualDest := filepath.Join(dir, "actual")
+			require.NoError(t, err)
-	ctx := context.Background()
 
-	err = extract.Tar(ctx, bytes.NewReader(expected), expectedDest, nil)
+			version1 := coderdtest.CreateTemplateVersion(t, client, owner.OrganizationID, source1)
-	require.NoError(t, err)
+			_ = coderdtest.AwaitTemplateVersionJobCompleted(t, client, version1.ID)
 
-	inv, root := clitest.New(t, "templates", "pull", template.Name, actualDest)
+			template := coderdtest.CreateTemplate(t, client, owner.OrganizationID, version1.ID)
-	clitest.SetupConfig(t, templateAdmin, root)
 
-	ptytest.New(t).Attach(inv)
+			// Update the template version so that we can assert that templates
+			// are being sorted correctly.
+			updatedVersion := coderdtest.UpdateTemplateVersion(t, client, owner.OrganizationID, source2, template.ID)
+			_ = coderdtest.AwaitTemplateVersionJobCompleted(t, client, updatedVersion.ID)
+			coderdtest.UpdateActiveTemplateVersion(t, client, template.ID, updatedVersion.ID)
 
-	require.NoError(t, inv.Run())
+			ctx := context.Background()
 
-	require.Equal(t,
+			err = extract.Tar(ctx, bytes.NewReader(expected), expectedDest, nil)
-		dirSum(t, expectedDest),
+			require.NoError(t, err)
-		dirSum(t, actualDest),
-	)
-}
 
-// ToDir tests that 'templates pull' pulls down the active template and writes
+			ents, _ := os.ReadDir(actualDest)
-// it to a directory with the name of the template if the path is not implicitly
+			if len(ents) > 0 {
-// supplied.
+				t.Logf("%s is not empty", actualDest)
-// nolint: paralleltest
+				t.FailNow()
-func TestTemplatePull_ToImplicit(t *testing.T) {
+			}
-	client := coderdtest.New(t, &coderdtest.Options{
-		IncludeProvisionerDaemon: true,
-	})
-	owner := coderdtest.CreateFirstUser(t, client)
-	templateAdmin, _ := coderdtest.CreateAnotherUser(t, client, owner.OrganizationID, rbac.RoleTemplateAdmin())
 
-	// Create an initial template bundle.
+			inv, root := clitest.New(t, "templates", "pull", template.Name, actualDest)
-	source1 := genTemplateVersionSource()
+			clitest.SetupConfig(t, templateAdmin, root)
-	// Create an updated template bundle. This will be used to ensure
-	// that templates are correctly returned in order from latest to oldest.
-	source2 := genTemplateVersionSource()
 
-	expected, err := echo.Tar(source2)
+			ptytest.New(t).Attach(inv)
-	require.NoError(t, err)
 
-	version1 := coderdtest.CreateTemplateVersion(t, client, owner.OrganizationID, source1)
+			require.NoError(t, inv.Run())
-	_ = coderdtest.AwaitTemplateVersionJobCompleted(t, client, version1.ID)
 
-	template := coderdtest.CreateTemplate(t, client, owner.OrganizationID, version1.ID)
+			// Validate behaviour of choosing template name in the absence of an output path argument.
+			destPath := actualDest
+			if destPath == "" {
+				destPath = template.Name
+			}
 
-	// Update the template version so that we can assert that templates
+			require.Equal(t,
-	// are being sorted correctly.
+				dirSum(t, expectedDest),
-	updatedVersion := coderdtest.UpdateTemplateVersion(t, client, owner.OrganizationID, source2, template.ID)
+				dirSum(t, destPath),
-	_ = coderdtest.AwaitTemplateVersionJobCompleted(t, client, updatedVersion.ID)
+			)
-	coderdtest.UpdateActiveTemplateVersion(t, client, template.ID, updatedVersion.ID)
+		})
-
+	}
-	// create a tempdir and change the working directory to it for the duration of the test (cannot run in parallel)
-	dir := t.TempDir()
-	wd, err := os.Getwd()
-	require.NoError(t, err)
-	err = os.Chdir(dir)
-	require.NoError(t, err)
-	defer func() {
-		err := os.Chdir(wd)
-		require.NoError(t, err, "if this fails, it can break other subsequent tests due to wrong working directory")
-	}()
-
-	expectedDest := filepath.Join(dir, "expected")
-	actualDest := filepath.Join(dir, template.Name)
-
-	ctx := context.Background()
-
-	err = extract.Tar(ctx, bytes.NewReader(expected), expectedDest, nil)
-	require.NoError(t, err)
-
-	inv, root := clitest.New(t, "templates", "pull", template.Name)
-	clitest.SetupConfig(t, templateAdmin, root)
-
-	ptytest.New(t).Attach(inv)
-
-	require.NoError(t, inv.Run())
-
-	require.Equal(t,
-		dirSum(t, expectedDest),
-		dirSum(t, actualDest),
-	)
 }
 
 // FolderConflict tests that 'templates pull' fails when a folder with has