Skip to content

fix: clean template destination path for pull #12559

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
Mar 14, 2024
Merged

fix: clean template destination path for pull #12559

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
5306e06
Clean template destination path
dannykopping Mar 12, 2024
9e037cd
Review comments
dannykopping Mar 13, 2024
00d5e61
Move nolint to correct location
dannykopping Mar 13, 2024
d15b5e3
Review comments
dannykopping Mar 13, 2024
80addee
Changing the working dir to temp and then traversing up to many level…
dannykopping Mar 13, 2024
9d035e3
Refactoring for cleaner approach
dannykopping Mar 14, 2024
5bf6dd6
Tests cannot run in parallel with others
dannykopping Mar 14, 2024
0fc2275
Reordering such that the Chdir cleanup is called before the TempDir
dannykopping Mar 14, 2024
b4114be
Update cli/templatepull.go
dannykopping Mar 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions cli/templatepull.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"bytes"
"fmt"
"os"
"path/filepath"
"sort"

"github.com/codeclysm/extract/v3"
Expand Down Expand Up @@ -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)
Expand Down
218 changes: 108 additions & 110 deletions cli/templatepull_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()

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.
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()

expected, err := echo.Tar(source2)
require.NoError(t, err)

version1 := coderdtest.CreateTemplateVersion(t, client, owner.OrganizationID, source1)
_ = coderdtest.AwaitTemplateVersionJobCompleted(t, client, version1.ID)

template := coderdtest.CreateTemplate(t, client, owner.OrganizationID, version1.ID)

// 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)

dir := t.TempDir()

expectedDest := filepath.Join(dir, "expected")
actualDest := filepath.Join(dir, "actual")
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, actualDest)
clitest.SetupConfig(t, templateAdmin, root)

ptytest.New(t).Attach(inv)

require.NoError(t, inv.Run())

require.Equal(t,
dirSum(t, expectedDest),
dirSum(t, actualDest),
)
}

// ToDir tests that 'templates pull' pulls down the active template and writes
// it to a directory with the name of the template if the path is not implicitly
// supplied.
// nolint: paralleltest
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.
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()

expected, err := echo.Tar(source2)
require.NoError(t, err)

version1 := coderdtest.CreateTemplateVersion(t, client, owner.OrganizationID, source1)
_ = coderdtest.AwaitTemplateVersionJobCompleted(t, client, version1.ID)

template := coderdtest.CreateTemplate(t, client, owner.OrganizationID, version1.ID)

// 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)

// 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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do these tests actually work with chdir removed? The files will be created inside the repo since we're not in the tmpdir, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It pulls into the givenPath:

inv, root := clitest.New(t, "templates", "pull", template.Name, tc.givenPath)

AFAICS it's functionally equivalent

Copy link
Member

@mafredri mafredri Mar 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the given path is relative, won't it be relative to the Go process, i.e. the test which is running in coder/cli directory? Thus it'll store files in there instead of tmp.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're correct! I added these two lines to account for that in the last commit:

			newWD := t.TempDir()
			require.NoError(t, os.Chdir(newWD))

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You also need to restore the wd after the test has ended, otherwise it could break other tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oooh good point!

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())
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: "",
},
}

require.Equal(t,
dirSum(t, expectedDest),
dirSum(t, actualDest),
)
// nolint: paralleltest // These tests change the current working dir, and is therefore unsuitable for parallelisation.
for _, tc := range tests {
tc := tc

t.Run(tc.name, func(t *testing.T) {
dir := t.TempDir()

cwd, err := os.Getwd()
require.NoError(t, err)
t.Cleanup(func() {
require.NoError(t, os.Chdir(cwd))
})

// Change working directory so that relative path tests don't affect the original working directory.
newWd := filepath.Join(dir, "new-cwd")
require.NoError(t, os.MkdirAll(newWd, 0o750))
require.NoError(t, os.Chdir(newWd))

expectedDest := filepath.Join(dir, "expected")
actualDest := tc.destPath
if tc.useDefaultDest {
actualDest = filepath.Join(dir, "actual")
}

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.
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()

expected, err := echo.Tar(source2)
require.NoError(t, err)

version1 := coderdtest.CreateTemplateVersion(t, client, owner.OrganizationID, source1)
_ = coderdtest.AwaitTemplateVersionJobCompleted(t, client, version1.ID)

template := coderdtest.CreateTemplate(t, client, owner.OrganizationID, version1.ID)

// 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)

ctx := context.Background()

err = extract.Tar(ctx, bytes.NewReader(expected), expectedDest, nil)
require.NoError(t, err)

ents, _ := os.ReadDir(actualDest)
if len(ents) > 0 {
t.Logf("%s is not empty", actualDest)
t.FailNow()
}

inv, root := clitest.New(t, "templates", "pull", template.Name, actualDest)
clitest.SetupConfig(t, templateAdmin, root)

ptytest.New(t).Attach(inv)

require.NoError(t, inv.Run())

// Validate behaviour of choosing template name in the absence of an output path argument.
destPath := actualDest
if destPath == "" {
destPath = template.Name
}

require.Equal(t,
dirSum(t, expectedDest),
dirSum(t, destPath),
)
})
}
}

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