From eff261099161ed0e08d0e2d4c2a5a12267f00246 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Fri, 8 Mar 2024 17:53:50 +0000 Subject: [PATCH 1/4] chore(coderd): add test to reproduce trailing directory issue --- coderd/fileszip_test.go | 66 ++++++++++++++++++++++++++++++++--------- 1 file changed, 52 insertions(+), 14 deletions(-) diff --git a/coderd/fileszip_test.go b/coderd/fileszip_test.go index b5f5057e3b6ae..1c3781c39d70b 100644 --- a/coderd/fileszip_test.go +++ b/coderd/fileszip_test.go @@ -58,26 +58,64 @@ func TestCreateZipFromTar(t *testing.T) { if runtime.GOOS != "linux" { t.Skip("skipping this test on non-Linux platform") } + t.Run("OK", func(t *testing.T) { + t.Parallel() + tarBytes, err := os.ReadFile(filepath.Join(".", "testdata", "test.tar")) + require.NoError(t, err, "failed to read sample tar file") - tarBytes, err := os.ReadFile(filepath.Join("testdata", "test.tar")) - require.NoError(t, err, "failed to read sample tar file") + tr := tar.NewReader(bytes.NewReader(tarBytes)) + zipBytes, err := coderd.CreateZipFromTar(tr) + require.NoError(t, err) - tr := tar.NewReader(bytes.NewReader(tarBytes)) - zipBytes, err := coderd.CreateZipFromTar(tr) - require.NoError(t, err) + assertSampleZipFile(t, zipBytes) - assertSampleZipFile(t, zipBytes) + tempDir := t.TempDir() + tempFilePath := filepath.Join(tempDir, "test.zip") + err = os.WriteFile(tempFilePath, zipBytes, 0o600) + require.NoError(t, err, "failed to write converted zip file") - tempDir := t.TempDir() - tempFilePath := filepath.Join(tempDir, "test.zip") - err = os.WriteFile(tempFilePath, zipBytes, 0o600) - require.NoError(t, err, "failed to write converted zip file") + ctx := testutil.Context(t, testutil.WaitShort) + cmd := exec.CommandContext(ctx, "unzip", tempFilePath, "-d", tempDir) + require.NoError(t, cmd.Run(), "failed to extract converted zip file") - ctx := testutil.Context(t, testutil.WaitShort) - cmd := exec.CommandContext(ctx, "unzip", tempFilePath, "-d", tempDir) - require.NoError(t, cmd.Run(), "failed to extract converted zip file") + assertExtractedFiles(t, tempDir, false) + }) - assertExtractedFiles(t, tempDir, false) + t.Run("MissingSlashInDirectoryHeader", func(t *testing.T) { + t.Parallel() + + // Given: a tar archive containing a directory entry that has the directory + // mode bit set but the name is missing a trailing slash + + var tarBytes bytes.Buffer + tw := tar.NewWriter(&tarBytes) + tw.WriteHeader(&tar.Header{ + Name: "dir", + Typeflag: tar.TypeDir, + Size: 0, + }) + require.NoError(t, tw.Flush()) + require.NoError(t, tw.Close()) + + // When: we convert this to a zip + tr := tar.NewReader(&tarBytes) + zipBytes, err := coderd.CreateZipFromTar(tr) + require.NoError(t, err) + + // Then: the resulting zip should contain a corresponding directory + zr, err := zip.NewReader(bytes.NewReader(zipBytes), int64(len(zipBytes))) + require.NoError(t, err) + for _, zf := range zr.File { + switch zf.Name { + case "dir": + require.Fail(t, "missing trailing slash in directory name") + case "dir/": + require.True(t, zf.Mode().IsDir(), "should be a directory") + default: + require.Fail(t, "unexpected file in archive") + } + } + }) } // nolint:revive // this is a control flag but it's in a unit test From 0d6c03e4d12d1a2577120a5130206bc5ecc641fb Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Fri, 8 Mar 2024 17:54:19 +0000 Subject: [PATCH 2/4] fix(coderd): add trailing path separator to dir entries when converting to zip --- coderd/fileszip.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/coderd/fileszip.go b/coderd/fileszip.go index 0d9c6ae478fae..fb5eb121ad22a 100644 --- a/coderd/fileszip.go +++ b/coderd/fileszip.go @@ -87,6 +87,12 @@ func WriteZipArchive(w io.Writer, tarReader *tar.Reader) error { return err } zipHeader.Name = tarHeader.Name + // Some versions of unzip do not check the mode on a file entry and + // simply assume that entries with a trailing path separator (/) are + // directories, and that everything else is a file. Give them a hint. + if tarHeader.FileInfo().IsDir() && !strings.HasSuffix(tarHeader.Name, "/") { + zipHeader.Name += "/" + } zipEntry, err := zipWriter.CreateHeader(zipHeader) if err != nil { From 7815d3ea34043d3722a61643715c7ffe6111de93 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Fri, 8 Mar 2024 17:54:49 +0000 Subject: [PATCH 3/4] fix(provisionersdk): add trailing path separator to directory entries --- provisionersdk/archive.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/provisionersdk/archive.go b/provisionersdk/archive.go index eb4d7ed7a974b..4051698fa48f3 100644 --- a/provisionersdk/archive.go +++ b/provisionersdk/archive.go @@ -107,6 +107,11 @@ func Tar(w io.Writer, logger slog.Logger, directory string, limit int64) error { } // Use unix paths in the tar archive. header.Name = filepath.ToSlash(rel) + // tar.FileInfoHeader() will do this, but filepath.Rel() calls filepath.Clean() + // which strips trailing path separators for directories. + if fileInfo.IsDir() { + header.Name += "/" + } if err := tarWriter.WriteHeader(header); err != nil { return err } From 9fa67f57e1fa5367299ee99e6a009702b74102d0 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Mon, 11 Mar 2024 13:35:41 +0000 Subject: [PATCH 4/4] fixup! fix(provisionersdk): add trailing path separator to directory entries --- coderd/fileszip.go | 1 + 1 file changed, 1 insertion(+) diff --git a/coderd/fileszip.go b/coderd/fileszip.go index fb5eb121ad22a..389e524746291 100644 --- a/coderd/fileszip.go +++ b/coderd/fileszip.go @@ -7,6 +7,7 @@ import ( "errors" "io" "log" + "strings" ) func CreateTarFromZip(zipReader *zip.Reader) ([]byte, error) {