From bed61f7d2a89411474c51fe38026de1f630027f2 Mon Sep 17 00:00:00 2001
From: Cian Johnston <cian@coder.com>
Date: Mon, 11 Mar 2024 14:06:41 +0000
Subject: [PATCH] fix(coderd): correctly handle tar dir entries with missing
 path separator (#12479)

* coderd: add test to reproduce trailing directory issue
* coderd: add trailing path separator to dir entries when converting to zip
* provisionersdk: add trailing path separator to directory entries
---
 coderd/fileszip.go        |  7 +++++
 coderd/fileszip_test.go   | 66 ++++++++++++++++++++++++++++++---------
 provisionersdk/archive.go |  5 +++
 3 files changed, 64 insertions(+), 14 deletions(-)

diff --git a/coderd/fileszip.go b/coderd/fileszip.go
index 0d9c6ae478..389e524746 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) {
@@ -87,6 +88,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 {
diff --git a/coderd/fileszip_test.go b/coderd/fileszip_test.go
index b5f5057e3b..1c3781c39d 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
diff --git a/provisionersdk/archive.go b/provisionersdk/archive.go
index eb4d7ed7a9..4051698fa4 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
 		}