- Notifications
You must be signed in to change notification settings - Fork928
fix: clean template destination path forpull#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.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM, but please wait for review from Mathias 👍
cli/templatepull.go Outdated
| } | ||
| if dest != clean { | ||
| cliui.Warn(inv.Stderr, fmt.Sprintf("cleaning destination path from %s to %s", dest, clean)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
nit: I wouldn't warn users here, it the tool can fix the path then we don't need to them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
cli/templatepull.go Outdated
| 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Could this be anxerrors.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.
Uh oh!
There was an error while loading.Please reload this page.
| dir := t.TempDir() | ||
| wd, err := os.Getwd() | ||
| require.NoError(t, err) | ||
| err = os.Chdir(dir) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It pulls into thegivenPath:
inv,root:=clitest.New(t,"templates","pull",template.Name,tc.givenPath)
AFAICS it's functionally equivalent
There was a problem hiding this comment.
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 incoder/cli directory? Thus it'll store files in there instead of tmp.
There was a problem hiding this comment.
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))
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Oooh good point!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
cli/templatepull.go Outdated
| } | ||
| if dest != clean { | ||
| cliui.Warn(inv.Stderr, fmt.Sprintf("cleaning destination path from %s to %s", dest, clean)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Uh oh!
There was an error while loading.Please reload this page.
| dir := t.TempDir() | ||
| wd, err := os.Getwd() | ||
| require.NoError(t, err) | ||
| err = os.Chdir(dir) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It pulls into thegivenPath:
inv,root:=clitest.New(t,"templates","pull",template.Name,tc.givenPath)
AFAICS it's functionally equivalent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
A few more things but once those are fixed I think this is good to go. Thanks for tackling this issue!
cli/templatepull_test.go Outdated
| @@ -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() | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We'll need to remove this parallel now that we're doing chdir again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Note that this istparallel notparalleltest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
OH you're completely right!
cli/templatepull_test.go Outdated
| templateAdmin, _ := coderdtest.CreateAnotherUser(t, client, owner.OrganizationID, rbac.RoleTemplateAdmin()) | ||
| t.Cleanup(func() { | ||
| _ = os.RemoveAll(tc.givenPath) | ||
| _ = os.RemoveAll(expectedDest) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We could perhaps combine these and chdir into a single cleanup? Or perhaps even better would be to implementtestutil.Chdir since it's something we do every now and a then.
cli/templatepull_test.go Outdated
| }, | ||
| { | ||
| name: "directory traversal is acceptable", | ||
| givenPath: "../mytmpl", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I suppose this write is outside the test tmpdir? Perhaps we should increase the depth down below (pseudonewWD := filepath.Join(t.TempDir(), "work"); mkdir(newWD))?
dannykopping commentedMar 14, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Signed-off-by: Danny Kopping <danny@coder.com>
Signed-off-by: Danny Kopping <danny@coder.com>
Signed-off-by: Danny Kopping <danny@coder.com>
Signed-off-by: Danny Kopping <danny@coder.com>
…s broke a testSigned-off-by: Danny Kopping <danny@coder.com>
Signed-off-by: Danny Kopping <danny@coder.com>
cda50fc to9d035e3CompareSigned-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>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Found one last simplification, but otherwise LGTM! 😄
Uh oh!
There was an error while loading.Please reload this page.
Co-authored-by: Mathias Fredriksson <mafredri@gmail.com>
Uh oh!
There was an error while loading.Please reload this page.
Fixes#10281
Cleaning the path and making it absolute causes the path to not be seen asunsafe 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
--helpand ended up finding a CLI bug:#12575