Merge branch 'fix-zip-directories' into 'master'

Fix support for archives without directory structure

Closes #482

See merge request gitlab-org/gitlab-pages!373

4 files changed, 126 insertions, 39 deletions

diff --git a/internal/serving/disk/zip/serving_test.go b/internal/serving/disk/zip/serving_test.go
index 14fdabdf..e95432ae 100644
--- a/internal/serving/disk/zip/serving_test.go
+++ b/internal/serving/disk/zip/serving_test.go
@@ -13,32 +13,60 @@ import (
 )
 
 func TestZip_ServeFileHTTP(t *testing.T) {
-	testServerURL, cleanup := newZipFileServerURL(t, "group/zip.gitlab.io/public.zip")
+	testServerURL, cleanup := newZipFileServerURL(t, "group/zip.gitlab.io/public-without-dirs.zip")
 	defer cleanup()
 
-	s := Instance()
-	w := httptest.NewRecorder()
-	r := httptest.NewRequest("GET", "http://zip.gitlab.io/zip/index.html", nil)
-	handler := serving.Handler{
-		Writer:  w,
-		Request: r,
-		LookupPath: &serving.LookupPath{
-			Prefix: "",
-			Path:   testServerURL + "/public.zip",
+	tests := map[string]struct {
+		path           string
+		expectedStatus int
+		expectedBody   string
+	}{
+		"accessing /index.html": {
+			path:           "/index.html",
+			expectedStatus: http.StatusOK,
+			expectedBody:   "zip.gitlab.io/project/index.html\n",
+		},
+		"accessing /": {
+			path:           "/",
+			expectedStatus: http.StatusOK,
+			expectedBody:   "zip.gitlab.io/project/index.html\n",
+		},
+		"accessing without /": {
+			path:           "",
+			expectedStatus: http.StatusFound,
+			expectedBody:   `<a href="//zip.gitlab.io/zip/">Found</a>.`,
 		},
-		SubPath: "/index.html",
 	}
 
-	require.True(t, s.ServeFileHTTP(handler))
+	s := Instance()
+
+	for name, test := range tests {
+		t.Run(name, func(t *testing.T) {
+			w := httptest.NewRecorder()
+			r := httptest.NewRequest("GET", "http://zip.gitlab.io/zip"+test.path, nil)
+
+			handler := serving.Handler{
+				Writer:  w,
+				Request: r,
+				LookupPath: &serving.LookupPath{
+					Prefix: "/zip/",
+					Path:   testServerURL + "/public.zip",
+				},
+				SubPath: test.path,
+			}
 
-	resp := w.Result()
-	defer resp.Body.Close()
+			require.True(t, s.ServeFileHTTP(handler))
 
-	require.Equal(t, http.StatusOK, resp.StatusCode)
-	body, err := ioutil.ReadAll(resp.Body)
-	require.NoError(t, err)
+			resp := w.Result()
+			defer resp.Body.Close()
 
-	require.Contains(t, string(body), "zip.gitlab.io/project/index.html\n")
+			require.Equal(t, test.expectedStatus, resp.StatusCode)
+			body, err := ioutil.ReadAll(resp.Body)
+			require.NoError(t, err)
+
+			require.Contains(t, string(body), test.expectedBody)
+		})
+	}
 }
 
 var chdirSet = false
diff --git a/internal/vfs/zip/archive.go b/internal/vfs/zip/archive.go
index 548ba651..ba15af20 100644
--- a/internal/vfs/zip/archive.go
+++ b/internal/vfs/zip/archive.go
@@ -53,7 +53,8 @@ type zipArchive struct {
 	archive  *zip.Reader
 	err      error
 
-	files map[string]*zip.File
+	files       map[string]*zip.File
+	directories map[string]*zip.FileHeader
 }
 
 func newArchive(fs *zipVFS, path string, openTimeout time.Duration) *zipArchive {
@@ -62,6 +63,7 @@ func newArchive(fs *zipVFS, path string, openTimeout time.Duration) *zipArchive
 		path:           path,
 		done:           make(chan struct{}),
 		files:          make(map[string]*zip.File),
+		directories:    make(map[string]*zip.FileHeader),
 		openTimeout:    openTimeout,
 		cacheNamespace: strconv.FormatInt(atomic.AddInt64(&fs.archiveCount, 1), 10) + ":",
 	}
@@ -133,7 +135,14 @@ func (a *zipArchive) readArchive() {
 		if !strings.HasPrefix(file.Name, dirPrefix) {
 			continue
 		}
-		a.files[file.Name] = file
+
+		if file.Mode().IsDir() {
+			a.directories[file.Name] = &file.FileHeader
+		} else {
+			a.files[file.Name] = file
+		}
+
+		a.addPathDirectory(file.Name)
 	}
 
 	// recycle memory
@@ -145,6 +154,23 @@ func (a *zipArchive) readArchive() {
 	metrics.ZipArchiveEntriesCached.Add(fileCount)
 }
 
+// addPathDirectory adds a directory for a given path
+func (a *zipArchive) addPathDirectory(path string) {
+	// Split dir and file from `path`
+	path, _ = filepath.Split(path)
+	if path == "" {
+		return
+	}
+
+	if a.directories[path] != nil {
+		return
+	}
+
+	a.directories[path] = &zip.FileHeader{
+		Name: path,
+	}
+}
+
 func (a *zipArchive) findFile(name string) *zip.File {
 	name = filepath.Join(dirPrefix, name)
 
@@ -152,17 +178,22 @@ func (a *zipArchive) findFile(name string) *zip.File {
 		return file
 	}
 
-	if dir := a.files[name+"/"]; dir != nil {
-		return dir
-	}
-
 	return nil
 }
 
+func (a *zipArchive) findDirectory(name string) *zip.FileHeader {
+	name = filepath.Join(dirPrefix, name)
+
+	return a.directories[name+"/"]
+}
+
 // Open finds the file by name inside the zipArchive and returns a reader that can be served by the VFS
 func (a *zipArchive) Open(ctx context.Context, name string) (vfs.File, error) {
 	file := a.findFile(name)
 	if file == nil {
+		if a.findDirectory(name) != nil {
+			return nil, errNotFile
+		}
 		return nil, os.ErrNotExist
 	}
 
@@ -193,17 +224,25 @@ func (a *zipArchive) Open(ctx context.Context, name string) (vfs.File, error) {
 // Lstat finds the file by name inside the zipArchive and returns its FileInfo
 func (a *zipArchive) Lstat(ctx context.Context, name string) (os.FileInfo, error) {
 	file := a.findFile(name)
-	if file == nil {
-		return nil, os.ErrNotExist
+	if file != nil {
+		return file.FileInfo(), nil
 	}
 
-	return file.FileInfo(), nil
+	directory := a.findDirectory(name)
+	if directory != nil {
+		return directory.FileInfo(), nil
+	}
+
+	return nil, os.ErrNotExist
 }
 
 // ReadLink finds the file by name inside the zipArchive and returns the contents of the symlink
 func (a *zipArchive) Readlink(ctx context.Context, name string) (string, error) {
 	file := a.findFile(name)
 	if file == nil {
+		if a.findDirectory(name) != nil {
+			return "", errNotSymlink
+		}
 		return "", os.ErrNotExist
 	}
 
diff --git a/internal/vfs/zip/archive_test.go b/internal/vfs/zip/archive_test.go
index bd7627b1..ef6785b5 100644
--- a/internal/vfs/zip/archive_test.go
+++ b/internal/vfs/zip/archive_test.go
@@ -102,24 +102,44 @@ func TestLstat(t *testing.T) {
 	defer cleanup()
 
 	tests := map[string]struct {
-		file        string
-		isDir       bool
-		isSymlink   bool
-		expectedErr error
+		file         string
+		isDir        bool
+		isSymlink    bool
+		expectedName string
+		expectedErr  error
 	}{
 		"file_exists": {
-			file: "index.html",
+			file:         "index.html",
+			expectedName: "index.html",
 		},
 		"file_exists_in_subdir": {
-			file: "subdir/hello.html",
+			file:         "subdir/hello.html",
+			expectedName: "hello.html",
 		},
 		"file_exists_symlink": {
-			file:      "symlink.html",
-			isSymlink: true,
+			file:         "symlink.html",
+			isSymlink:    true,
+			expectedName: "symlink.html",
+		},
+		"has_root": {
+			file:         "",
+			isDir:        true,
+			expectedName: "public",
+		},
+		"has_root_dot": {
+			file:         ".",
+			isDir:        true,
+			expectedName: "public",
+		},
+		"has_root_slash": {
+			file:         "/",
+			isDir:        true,
+			expectedName: "public",
 		},
 		"is_dir": {
-			file:  "subdir",
-			isDir: true,
+			file:         "subdir",
+			isDir:        true,
+			expectedName: "subdir",
 		},
 		"file_does_not_exist": {
 			file:        "unknown.html",
@@ -136,7 +156,7 @@ func TestLstat(t *testing.T) {
 			}
 
 			require.NoError(t, err)
-			require.Contains(t, tt.file, fi.Name())
+			require.Equal(t, tt.expectedName, fi.Name())
 			require.Equal(t, tt.isDir, fi.IsDir())
 			require.NotEmpty(t, fi.ModTime())
 
@@ -265,7 +285,7 @@ func openZipArchive(t *testing.T, requests *int64) (*zipArchive, func()) {
 		requests = new(int64)
 	}
 
-	testServerURL, cleanup := newZipFileServerURL(t, "group/zip.gitlab.io/public.zip", requests)
+	testServerURL, cleanup := newZipFileServerURL(t, "group/zip.gitlab.io/public-without-dirs.zip", requests)
 
 	fs := New().(*zipVFS)
 	zip := newArchive(fs, testServerURL+"/public.zip", time.Second)
diff --git a/shared/pages/group/zip.gitlab.io/public-without-dirs.zip b/shared/pages/group/zip.gitlab.io/public-without-dirs.zip
new file mode 100644
index 00000000..a6cfdfcf
--- /dev/null
+++ b/shared/pages/group/zip.gitlab.io/public-without-dirs.zip