fix: clean template destination path for `pull` #12559

dannykopping · 2024-03-12T15:34:09Z

Cleaning the path and making it absolute causes the path to not be seen as unsafe by the underlying library.

Note to reviewer: this PR obviates the need for TestTemplatePull_ToImplicit.
It made sense to me to merge them together, but you may want to keep them separate; I'm easy either way.

Side-note: I was testing to see if I could inject problematic template names like --help and ended up finding a CLI bug: #12575

mtojek

LGTM, but please wait for review from Mathias 👍

mtojek · 2024-03-13T08:27:26Z

cli/templatepull.go

+			}
+
+			if dest != clean {
+				cliui.Warn(inv.Stderr, fmt.Sprintf("cleaning destination path from %s to %s", dest, clean))


nit: I wouldn't warn users here, it the tool can fix the path then we don't need to them

I was thinking this process of cleaning the dir might cause unexpected results, but on reflection I think you're right; I've removed it.

cli/templatepull_test.go

mafredri · 2024-03-13T11:44:11Z

cli/templatepull.go

+			clean, err := filepath.Abs(filepath.Clean(dest))
+			if err != nil {
+				cliui.Error(inv.Stderr, fmt.Sprintf("cleaning destination path %s failed: %q", dest, err))
+				return err


Could this be an xerrors.Errorf("lookup of absolute path failed ...: %w") instead of the manual cliui error call? I'd expect the error to be printed twice this way.

cli/templatepull_test.go

mafredri · 2024-03-13T11:55:17Z

cli/templatepull_test.go

-	dir := t.TempDir()
-	wd, err := os.Getwd()
-	require.NoError(t, err)
-	err = os.Chdir(dir)


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?

It pulls into the givenPath:

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

AFAICS it's functionally equivalent

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.

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

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

Oooh good point!

dannykopping

@mafredri @mtojek responded, thank you for your reviews!

dannykopping · 2024-03-13T13:05:17Z

cli/templatepull.go

+			}
+
+			if dest != clean {
+				cliui.Warn(inv.Stderr, fmt.Sprintf("cleaning destination path from %s to %s", dest, clean))


I was thinking this process of cleaning the dir might cause unexpected results, but on reflection I think you're right; I've removed it.

cli/templatepull_test.go

dannykopping · 2024-03-13T13:30:57Z

cli/templatepull_test.go

-	dir := t.TempDir()
-	wd, err := os.Getwd()
-	require.NoError(t, err)
-	err = os.Chdir(dir)


It pulls into the givenPath:

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

AFAICS it's functionally equivalent

mafredri

A few more things but once those are fixed I think this is good to go. Thanks for tackling this issue!

mafredri · 2024-03-13T15:51:28Z

cli/templatepull_test.go

@@ -230,118 +230,111 @@ func TestTemplatePull_LatestStdout(t *testing.T) {

 // ToDir tests that 'templates pull' pulls down the active template
 // and writes it to the correct directory.
+//
+// nolint: tparallel // The subtests cannot be run in parallel; see the inner loop.
 func TestTemplatePull_ToDir(t *testing.T) {
 	t.Parallel()


We'll need to remove this parallel now that we're doing chdir again.

Note that this is tparallel not paralleltest.

I wasn't referring to linting. A test that modifies the environment of the process (e.g. setenv or chdir) can't be parallel or have parallel ancestors.

OH you're completely right!

mafredri · 2024-03-13T15:54:09Z

cli/templatepull_test.go

-	templateAdmin, _ := coderdtest.CreateAnotherUser(t, client, owner.OrganizationID, rbac.RoleTemplateAdmin())
+			t.Cleanup(func() {
+				_ = os.RemoveAll(tc.givenPath)
+				_ = os.RemoveAll(expectedDest)


We could perhaps combine these and chdir into a single cleanup? Or perhaps even better would be to implement testutil.Chdir since it's something we do every now and a then.

mafredri · 2024-03-13T15:59:11Z

cli/templatepull_test.go

+		},
+		{
+			name:      "directory traversal is acceptable",
+			givenPath: "../mytmpl",


I suppose this write is outside the test tmpdir? Perhaps we should increase the depth down below (pseudo newWD := filepath.Join(t.TempDir(), "work"); mkdir(newWD))?

dannykopping · 2024-03-14T08:37:20Z

@mafredri I took a slightly different approach in 9d035e3 which is a bit cleaner. Please let me know what you think.

Signed-off-by: Danny Kopping <danny@coder.com>

…s broke a test Signed-off-by: Danny Kopping <danny@coder.com>

Signed-off-by: Danny Kopping <danny@coder.com>

cleanup; t.Cleanup is ordered by last-added-first-called. Signed-off-by: Danny Kopping <danny@coder.com>

mafredri

Found one last simplification, but otherwise LGTM! 😄

cli/templatepull.go

Co-authored-by: Mathias Fredriksson <mafredri@gmail.com>

github-actions bot assigned dannykopping Mar 12, 2024

dannykopping marked this pull request as ready for review March 13, 2024 07:52

dannykopping requested review from mafredri and mtojek March 13, 2024 07:53

mtojek approved these changes Mar 13, 2024

View reviewed changes

mafredri reviewed Mar 13, 2024

View reviewed changes

dannykopping commented Mar 13, 2024

View reviewed changes

mafredri reviewed Mar 13, 2024

View reviewed changes

dannykopping added 6 commits March 14, 2024 10:50

Clean template destination path

5306e06

Signed-off-by: Danny Kopping <danny@coder.com>

Review comments

9e037cd

Signed-off-by: Danny Kopping <danny@coder.com>

Move nolint to correct location

00d5e61

Signed-off-by: Danny Kopping <danny@coder.com>

Review comments

d15b5e3

Signed-off-by: Danny Kopping <danny@coder.com>

Changing the working dir to temp and then traversing up to many level…

80addee

…s broke a test Signed-off-by: Danny Kopping <danny@coder.com>

Refactoring for cleaner approach

9d035e3

Signed-off-by: Danny Kopping <danny@coder.com>

dannykopping force-pushed the dk/template-pull-path branch from cda50fc to 9d035e3 Compare March 14, 2024 08:51

dannykopping added 2 commits March 14, 2024 11:18

Tests cannot run in parallel with others

5bf6dd6

Signed-off-by: Danny Kopping <danny@coder.com>

Reordering such that the Chdir cleanup is called before the TempDir

0fc2275

cleanup; t.Cleanup is ordered by last-added-first-called. Signed-off-by: Danny Kopping <danny@coder.com>

mafredri approved these changes Mar 14, 2024

View reviewed changes

cli/templatepull.go Outdated Show resolved Hide resolved

Update cli/templatepull.go

b4114be

Co-authored-by: Mathias Fredriksson <mafredri@gmail.com>

dannykopping enabled auto-merge (squash) March 14, 2024 12:34

dannykopping merged commit 14130de into coder:main Mar 14, 2024

github-actions bot locked and limited conversation to collaborators Mar 14, 2024

dannykopping deleted the dk/template-pull-path branch March 14, 2024 12:59

fix: clean template destination path for pull #12559

fix: clean template destination path for pull #12559

Uh oh!

Conversation

dannykopping commented Mar 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mtojek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mafredri Mar 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dannykopping left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mafredri left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mafredri Mar 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dannykopping commented Mar 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mafredri left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

fix: clean template destination path for `pull` #12559

fix: clean template destination path for `pull` #12559

dannykopping commented Mar 12, 2024 •

edited

Loading

mafredri Mar 13, 2024 •

edited

Loading

mafredri Mar 14, 2024 •

edited

Loading

dannykopping commented Mar 14, 2024 •

edited

Loading