From e7eab9b2bca73d0e923735d04fdbdefa6c9b074f Mon Sep 17 00:00:00 2001
From: Jacob Vosmaer <jacob@gitlab.com>
Date: Tue, 11 Aug 2020 15:00:43 +0200
Subject: [PATCH 1/9] Add VFS for local disk

---
 internal/serving/disk/helpers.go           |  9 +++--
 internal/serving/disk/reader.go            | 43 +++++++++++++---------
 internal/serving/disk/symlink/path_test.go | 21 +++++++----
 internal/serving/disk/symlink/shims.go     | 24 ++++++++----
 internal/serving/disk/symlink/symlink.go   | 11 ++++--
 internal/vfs/local/local_test.go           | 23 ++++++++++++
 internal/vfs/local/testdata/file           |  1 +
 internal/vfs/local/testdata/link           |  1 +
 internal/vfs/local/vfs.go                  | 19 ++++++++++
 internal/vfs/vfs.go                        | 19 ++++++++++
 10 files changed, 129 insertions(+), 42 deletions(-)
 create mode 100644 internal/vfs/local/local_test.go
 create mode 100644 internal/vfs/local/testdata/file
 create mode 120000 internal/vfs/local/testdata/link
 create mode 100644 internal/vfs/local/vfs.go
 create mode 100644 internal/vfs/vfs.go

diff --git a/internal/serving/disk/helpers.go b/internal/serving/disk/helpers.go
index 271b63cc7..e082d1ed0 100644
--- a/internal/serving/disk/helpers.go
+++ b/internal/serving/disk/helpers.go
@@ -1,6 +1,7 @@
 package disk
 
 import (
+	"context"
 	"io"
 	"mime"
 	"net/http"
@@ -29,13 +30,13 @@ func openNoFollow(path string) (*os.File, error) {
 // Detect file's content-type either by extension or mime-sniffing.
 // Implementation is adapted from Golang's `http.serveContent()`
 // See https://github.com/golang/go/blob/902fc114272978a40d2e65c2510a18e870077559/src/net/http/fs.go#L194
-func detectContentType(path string) (string, error) {
+func (reader *Reader) detectContentType(ctx context.Context, path string) (string, error) {
 	contentType := mime.TypeByExtension(filepath.Ext(path))
 
 	if contentType == "" {
 		var buf [512]byte
 
-		file, err := os.Open(path)
+		file, err := reader.vfs().Open(ctx, path)
 		if err != nil {
 			return "", err
 		}
@@ -61,7 +62,7 @@ func acceptsGZip(r *http.Request) bool {
 	return acceptedEncoding == "gzip"
 }
 
-func handleGZip(w http.ResponseWriter, r *http.Request, fullPath string) string {
+func (reader *Reader) handleGZip(ctx context.Context, w http.ResponseWriter, r *http.Request, fullPath string) string {
 	if !acceptsGZip(r) {
 		return fullPath
 	}
@@ -69,7 +70,7 @@ func handleGZip(w http.ResponseWriter, r *http.Request, fullPath string) string
 	gzipPath := fullPath + ".gz"
 
 	// Ensure the .gz file is not a symlink
-	fi, err := os.Lstat(gzipPath)
+	fi, err := reader.vfs().Lstat(ctx, gzipPath)
 	if err != nil || !fi.Mode().IsRegular() {
 		return fullPath
 	}
diff --git a/internal/serving/disk/reader.go b/internal/serving/disk/reader.go
index b8dce5421..63b75ad7e 100644
--- a/internal/serving/disk/reader.go
+++ b/internal/serving/disk/reader.go
@@ -1,10 +1,10 @@
 package disk
 
 import (
+	"context"
 	"fmt"
 	"io"
 	"net/http"
-	"os"
 	"path/filepath"
 	"strconv"
 	"strings"
@@ -13,6 +13,9 @@ import (
 	"github.com/prometheus/client_golang/prometheus"
 
 	"gitlab.com/gitlab-org/gitlab-pages/internal/serving"
+	"gitlab.com/gitlab-org/gitlab-pages/internal/serving/disk/symlink"
+	"gitlab.com/gitlab-org/gitlab-pages/internal/vfs"
+	"gitlab.com/gitlab-org/gitlab-pages/internal/vfs/local"
 )
 
 // Reader is a disk access driver
@@ -20,8 +23,11 @@ type Reader struct {
 	fileSizeMetric prometheus.Histogram
 }
 
+func (reader *Reader) vfs() vfs.VFS { return local.VFS{} }
+
 func (reader *Reader) tryFile(h serving.Handler) error {
-	fullPath, err := reader.resolvePath(h.LookupPath.Path, h.SubPath)
+	ctx := h.Request.Context()
+	fullPath, err := reader.resolvePath(ctx, h.LookupPath.Path, h.SubPath)
 
 	request := h.Request
 	host := request.Host
@@ -29,7 +35,7 @@ func (reader *Reader) tryFile(h serving.Handler) error {
 
 	if locationError, _ := err.(*locationDirectoryError); locationError != nil {
 		if endsWithSlash(urlPath) {
-			fullPath, err = reader.resolvePath(h.LookupPath.Path, h.SubPath, "index.html")
+			fullPath, err = reader.resolvePath(ctx, h.LookupPath.Path, h.SubPath, "index.html")
 		} else {
 			// TODO why are we doing that? In tests it redirects to HTTPS. This seems wrong,
 			// issue about this: https://gitlab.com/gitlab-org/gitlab-pages/issues/273
@@ -46,23 +52,24 @@ func (reader *Reader) tryFile(h serving.Handler) error {
 	}
 
 	if locationError, _ := err.(*locationFileNoExtensionError); locationError != nil {
-		fullPath, err = reader.resolvePath(h.LookupPath.Path, strings.TrimSuffix(h.SubPath, "/")+".html")
+		fullPath, err = reader.resolvePath(ctx, h.LookupPath.Path, strings.TrimSuffix(h.SubPath, "/")+".html")
 	}
 
 	if err != nil {
 		return err
 	}
 
-	return reader.serveFile(h.Writer, h.Request, fullPath, h.LookupPath.HasAccessControl)
+	return reader.serveFile(ctx, h.Writer, h.Request, fullPath, h.LookupPath.HasAccessControl)
 }
 
 func (reader *Reader) tryNotFound(h serving.Handler) error {
-	page404, err := reader.resolvePath(h.LookupPath.Path, "404.html")
+	ctx := h.Request.Context()
+	page404, err := reader.resolvePath(ctx, h.LookupPath.Path, "404.html")
 	if err != nil {
 		return err
 	}
 
-	err = reader.serveCustomFile(h.Writer, h.Request, http.StatusNotFound, page404)
+	err = reader.serveCustomFile(ctx, h.Writer, h.Request, http.StatusNotFound, page404)
 	if err != nil {
 		return err
 	}
@@ -71,7 +78,7 @@ func (reader *Reader) tryNotFound(h serving.Handler) error {
 
 // Resolve the HTTP request to a path on disk, converting requests for
 // directories to requests for index.html inside the directory if appropriate.
-func (reader *Reader) resolvePath(publicPath string, subPath ...string) (string, error) {
+func (reader *Reader) resolvePath(ctx context.Context, publicPath string, subPath ...string) (string, error) {
 	// Ensure that publicPath always ends with "/"
 	publicPath = strings.TrimSuffix(publicPath, "/") + "/"
 
@@ -79,7 +86,7 @@ func (reader *Reader) resolvePath(publicPath string, subPath ...string) (string,
 	// where we want to traverse full path as supplied by user
 	// (including ..)
 	testPath := publicPath + strings.Join(subPath, "/")
-	fullPath, err := filepath.EvalSymlinks(testPath)
+	fullPath, err := symlink.EvalSymlinks(ctx, reader.vfs(), testPath)
 
 	if err != nil {
 		if endsWithoutHTMLExtension(testPath) {
@@ -96,7 +103,7 @@ func (reader *Reader) resolvePath(publicPath string, subPath ...string) (string,
 		return "", fmt.Errorf("%q should be in %q", fullPath, publicPath)
 	}
 
-	fi, err := os.Lstat(fullPath)
+	fi, err := reader.vfs().Lstat(ctx, fullPath)
 	if err != nil {
 		return "", err
 	}
@@ -118,8 +125,8 @@ func (reader *Reader) resolvePath(publicPath string, subPath ...string) (string,
 	return fullPath, nil
 }
 
-func (reader *Reader) serveFile(w http.ResponseWriter, r *http.Request, origPath string, accessControl bool) error {
-	fullPath := handleGZip(w, r, origPath)
+func (reader *Reader) serveFile(ctx context.Context, w http.ResponseWriter, r *http.Request, origPath string, accessControl bool) error {
+	fullPath := reader.handleGZip(ctx, w, r, origPath)
 
 	file, err := openNoFollow(fullPath)
 	if err != nil {
@@ -139,7 +146,7 @@ func (reader *Reader) serveFile(w http.ResponseWriter, r *http.Request, origPath
 		w.Header().Set("Expires", time.Now().Add(10*time.Minute).Format(time.RFC1123))
 	}
 
-	contentType, err := detectContentType(origPath)
+	contentType, err := reader.detectContentType(ctx, origPath)
 	if err != nil {
 		return err
 	}
@@ -152,22 +159,22 @@ func (reader *Reader) serveFile(w http.ResponseWriter, r *http.Request, origPath
 	return nil
 }
 
-func (reader *Reader) serveCustomFile(w http.ResponseWriter, r *http.Request, code int, origPath string) error {
-	fullPath := handleGZip(w, r, origPath)
+func (reader *Reader) serveCustomFile(ctx context.Context, w http.ResponseWriter, r *http.Request, code int, origPath string) error {
+	fullPath := reader.handleGZip(ctx, w, r, origPath)
 
 	// Open and serve content of file
-	file, err := openNoFollow(fullPath)
+	file, err := reader.vfs().Open(ctx, fullPath)
 	if err != nil {
 		return err
 	}
 	defer file.Close()
 
-	fi, err := file.Stat()
+	fi, err := reader.vfs().Lstat(ctx, fullPath)
 	if err != nil {
 		return err
 	}
 
-	contentType, err := detectContentType(origPath)
+	contentType, err := reader.detectContentType(ctx, origPath)
 	if err != nil {
 		return err
 	}
diff --git a/internal/serving/disk/symlink/path_test.go b/internal/serving/disk/symlink/path_test.go
index 077b0ad2c..be2ba8451 100644
--- a/internal/serving/disk/symlink/path_test.go
+++ b/internal/serving/disk/symlink/path_test.go
@@ -2,17 +2,22 @@
 // Use of this source code is governed by a BSD-style
 // license that can be found in the LICENSE file.
 
-package filepath_test
+package symlink_test
 
 import (
+	"context"
 	"io/ioutil"
 	"os"
+	stdlib "path/filepath"
 	"runtime"
 	"testing"
 
 	filepath "gitlab.com/gitlab-org/gitlab-pages/internal/serving/disk/symlink"
+	"gitlab.com/gitlab-org/gitlab-pages/internal/vfs/local"
 )
 
+var fs = local.VFS{}
+
 func chtmpdir(t *testing.T) (restore func()) {
 	oldwd, err := os.Getwd()
 	if err != nil {
@@ -81,7 +86,7 @@ func simpleJoin(dir, path string) string {
 }
 
 func testEvalSymlinks(t *testing.T, path, want string) {
-	have, err := filepath.EvalSymlinks(path)
+	have, err := filepath.EvalSymlinks(context.Background(), fs, path)
 	if err != nil {
 		t.Errorf("EvalSymlinks(%q) error: %v", path, err)
 		return
@@ -108,7 +113,7 @@ func testEvalSymlinksAfterChdir(t *testing.T, wd, path, want string) {
 		t.Fatal(err)
 	}
 
-	have, err := filepath.EvalSymlinks(path)
+	have, err := filepath.EvalSymlinks(context.Background(), fs, path)
 	if err != nil {
 		t.Errorf("EvalSymlinks(%q) in %q directory error: %v", path, wd, err)
 		return
@@ -127,7 +132,7 @@ func TestEvalSymlinks(t *testing.T) {
 
 	// /tmp may itself be a symlink! Avoid the confusion, although
 	// it means trusting the thing we're testing.
-	tmpDir, err = filepath.EvalSymlinks(tmpDir)
+	tmpDir, err = stdlib.EvalSymlinks(tmpDir)
 	if err != nil {
 		t.Fatal("eval symlink for tmp dir:", err)
 	}
@@ -183,7 +188,7 @@ func TestEvalSymlinks(t *testing.T) {
 func TestEvalSymlinksIsNotExist(t *testing.T) {
 	defer chtmpdir(t)()
 
-	_, err := filepath.EvalSymlinks("notexist")
+	_, err := filepath.EvalSymlinks(context.Background(), fs, "notexist")
 	if !os.IsNotExist(err) {
 		t.Errorf("expected the file is not found, got %v\n", err)
 	}
@@ -194,7 +199,7 @@ func TestEvalSymlinksIsNotExist(t *testing.T) {
 	}
 	defer os.Remove("link")
 
-	_, err = filepath.EvalSymlinks("link")
+	_, err = filepath.EvalSymlinks(context.Background(), fs, "link")
 	if !os.IsNotExist(err) {
 		t.Errorf("expected the file is not found, got %v\n", err)
 	}
@@ -234,7 +239,7 @@ func TestIssue13582(t *testing.T) {
 	}
 
 	// /tmp may itself be a symlink!
-	realTmpDir, err := filepath.EvalSymlinks(tmpDir)
+	realTmpDir, err := stdlib.EvalSymlinks(tmpDir)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -251,7 +256,7 @@ func TestIssue13582(t *testing.T) {
 		{link2, realFile},
 	}
 	for i, test := range tests {
-		have, err := filepath.EvalSymlinks(test.path)
+		have, err := filepath.EvalSymlinks(context.Background(), fs, test.path)
 		if err != nil {
 			t.Fatal(err)
 		}
diff --git a/internal/serving/disk/symlink/shims.go b/internal/serving/disk/symlink/shims.go
index 4b3441806..e29c92641 100644
--- a/internal/serving/disk/symlink/shims.go
+++ b/internal/serving/disk/symlink/shims.go
@@ -1,13 +1,21 @@
-package filepath
+package symlink
 
-import stdlib "path/filepath"
+import (
+	"context"
+	"path/filepath"
+
+	"gitlab.com/gitlab-org/gitlab-pages/internal/vfs"
+)
 
 func volumeNameLen(s string) int { return 0 }
 
-func IsAbs(path string) bool                   { return stdlib.IsAbs(path) }
-func Clean(path string) string                 { return stdlib.Clean(path) }
-func Join(elem ...string) string               { return stdlib.Join(elem...) }
-func VolumeName(path string) string            { return stdlib.VolumeName(path) }
-func EvalSymlinks(path string) (string, error) { return walkSymlinks(path) }
+func IsAbs(path string) bool        { return filepath.IsAbs(path) }
+func Clean(path string) string      { return filepath.Clean(path) }
+func Join(elem ...string) string    { return filepath.Join(elem...) }
+func VolumeName(path string) string { return filepath.VolumeName(path) }
+
+func EvalSymlinks(ctx context.Context, fs vfs.VFS, path string) (string, error) {
+	return walkSymlinks(ctx, fs, path)
+}
 
-const Separator = stdlib.Separator
+const Separator = filepath.Separator
diff --git a/internal/serving/disk/symlink/symlink.go b/internal/serving/disk/symlink/symlink.go
index 335b315a2..507148112 100644
--- a/internal/serving/disk/symlink/symlink.go
+++ b/internal/serving/disk/symlink/symlink.go
@@ -2,16 +2,19 @@
 // Use of this source code is governed by a BSD-style
 // license that can be found in the LICENSE file.
 
-package filepath
+package symlink
 
 import (
+	"context"
 	"errors"
 	"os"
 	"runtime"
 	"syscall"
+
+	"gitlab.com/gitlab-org/gitlab-pages/internal/vfs"
 )
 
-func walkSymlinks(path string) (string, error) {
+func walkSymlinks(ctx context.Context, fs vfs.VFS, path string) (string, error) {
 	volLen := volumeNameLen(path)
 	pathSeparator := string(os.PathSeparator)
 
@@ -80,7 +83,7 @@ func walkSymlinks(path string) (string, error) {
 
 		// Resolve symlink.
 
-		fi, err := os.Lstat(dest)
+		fi, err := fs.Lstat(ctx, dest)
 		if err != nil {
 			return "", err
 		}
@@ -99,7 +102,7 @@ func walkSymlinks(path string) (string, error) {
 			return "", errors.New("EvalSymlinks: too many links")
 		}
 
-		link, err := os.Readlink(dest)
+		link, err := fs.Readlink(ctx, dest)
 		if err != nil {
 			return "", err
 		}
diff --git a/internal/vfs/local/local_test.go b/internal/vfs/local/local_test.go
new file mode 100644
index 000000000..651ab5ac0
--- /dev/null
+++ b/internal/vfs/local/local_test.go
@@ -0,0 +1,23 @@
+package local
+
+import (
+	"context"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func TestReadlink(t *testing.T) {
+	ctx := context.Background()
+	fs := VFS{}
+
+	target, err := fs.Readlink(ctx, "testdata/link")
+	require.NoError(t, err)
+	require.Equal(t, "target", target)
+
+	_, err = fs.Readlink(ctx, "testdata")
+	require.Error(t, err, "expect readlink to fail on directory")
+
+	_, err = fs.Readlink(ctx, "testdata/file")
+	require.Error(t, err, "expect readlink to fail on regular file")
+}
diff --git a/internal/vfs/local/testdata/file b/internal/vfs/local/testdata/file
new file mode 100644
index 000000000..ce0136250
--- /dev/null
+++ b/internal/vfs/local/testdata/file
@@ -0,0 +1 @@
+hello
diff --git a/internal/vfs/local/testdata/link b/internal/vfs/local/testdata/link
new file mode 120000
index 000000000..1de565933
--- /dev/null
+++ b/internal/vfs/local/testdata/link
@@ -0,0 +1 @@
+target
\ No newline at end of file
diff --git a/internal/vfs/local/vfs.go b/internal/vfs/local/vfs.go
new file mode 100644
index 000000000..7c6f3ba6b
--- /dev/null
+++ b/internal/vfs/local/vfs.go
@@ -0,0 +1,19 @@
+package local
+
+import (
+	"context"
+	"os"
+
+	"golang.org/x/sys/unix"
+
+	"gitlab.com/gitlab-org/gitlab-pages/internal/vfs"
+)
+
+type VFS struct{}
+
+func (fs VFS) Lstat(ctx context.Context, name string) (os.FileInfo, error) { return os.Lstat(name) }
+func (fs VFS) Readlink(ctx context.Context, name string) (string, error)   { return os.Readlink(name) }
+
+func (fs VFS) Open(ctx context.Context, name string) (vfs.File, error) {
+	return os.OpenFile(name, os.O_RDONLY|unix.O_NOFOLLOW, 0)
+}
diff --git a/internal/vfs/vfs.go b/internal/vfs/vfs.go
new file mode 100644
index 000000000..97e5f6211
--- /dev/null
+++ b/internal/vfs/vfs.go
@@ -0,0 +1,19 @@
+package vfs
+
+import (
+	"context"
+	"io"
+	"os"
+)
+
+type VFS interface {
+	Lstat(ctx context.Context, name string) (os.FileInfo, error)
+	Readlink(ctx context.Context, name string) (string, error)
+	Open(ctx context.Context, name string) (File, error)
+}
+
+type File interface {
+	io.Reader
+	io.Seeker
+	io.Closer
+}
-- 
GitLab


From d78263b96fb08b98a7b64598cf9482d4f6286c88 Mon Sep 17 00:00:00 2001
From: Jacob Vosmaer <jacob@gitlab.com>
Date: Tue, 11 Aug 2020 15:05:19 +0200
Subject: [PATCH 2/9] Simplify test imports

---
 internal/serving/disk/symlink/path_test.go | 18 +++++++++---------
 internal/serving/disk/symlink/shims.go     |  8 ++------
 2 files changed, 11 insertions(+), 15 deletions(-)

diff --git a/internal/serving/disk/symlink/path_test.go b/internal/serving/disk/symlink/path_test.go
index be2ba8451..4d590db52 100644
--- a/internal/serving/disk/symlink/path_test.go
+++ b/internal/serving/disk/symlink/path_test.go
@@ -8,11 +8,11 @@ import (
 	"context"
 	"io/ioutil"
 	"os"
-	stdlib "path/filepath"
+	"path/filepath"
 	"runtime"
 	"testing"
 
-	filepath "gitlab.com/gitlab-org/gitlab-pages/internal/serving/disk/symlink"
+	"gitlab.com/gitlab-org/gitlab-pages/internal/serving/disk/symlink"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/vfs/local"
 )
 
@@ -86,7 +86,7 @@ func simpleJoin(dir, path string) string {
 }
 
 func testEvalSymlinks(t *testing.T, path, want string) {
-	have, err := filepath.EvalSymlinks(context.Background(), fs, path)
+	have, err := symlink.EvalSymlinks(context.Background(), fs, path)
 	if err != nil {
 		t.Errorf("EvalSymlinks(%q) error: %v", path, err)
 		return
@@ -113,7 +113,7 @@ func testEvalSymlinksAfterChdir(t *testing.T, wd, path, want string) {
 		t.Fatal(err)
 	}
 
-	have, err := filepath.EvalSymlinks(context.Background(), fs, path)
+	have, err := symlink.EvalSymlinks(context.Background(), fs, path)
 	if err != nil {
 		t.Errorf("EvalSymlinks(%q) in %q directory error: %v", path, wd, err)
 		return
@@ -132,7 +132,7 @@ func TestEvalSymlinks(t *testing.T) {
 
 	// /tmp may itself be a symlink! Avoid the confusion, although
 	// it means trusting the thing we're testing.
-	tmpDir, err = stdlib.EvalSymlinks(tmpDir)
+	tmpDir, err = filepath.EvalSymlinks(tmpDir)
 	if err != nil {
 		t.Fatal("eval symlink for tmp dir:", err)
 	}
@@ -188,7 +188,7 @@ func TestEvalSymlinks(t *testing.T) {
 func TestEvalSymlinksIsNotExist(t *testing.T) {
 	defer chtmpdir(t)()
 
-	_, err := filepath.EvalSymlinks(context.Background(), fs, "notexist")
+	_, err := symlink.EvalSymlinks(context.Background(), fs, "notexist")
 	if !os.IsNotExist(err) {
 		t.Errorf("expected the file is not found, got %v\n", err)
 	}
@@ -199,7 +199,7 @@ func TestEvalSymlinksIsNotExist(t *testing.T) {
 	}
 	defer os.Remove("link")
 
-	_, err = filepath.EvalSymlinks(context.Background(), fs, "link")
+	_, err = symlink.EvalSymlinks(context.Background(), fs, "link")
 	if !os.IsNotExist(err) {
 		t.Errorf("expected the file is not found, got %v\n", err)
 	}
@@ -239,7 +239,7 @@ func TestIssue13582(t *testing.T) {
 	}
 
 	// /tmp may itself be a symlink!
-	realTmpDir, err := stdlib.EvalSymlinks(tmpDir)
+	realTmpDir, err := filepath.EvalSymlinks(tmpDir)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -256,7 +256,7 @@ func TestIssue13582(t *testing.T) {
 		{link2, realFile},
 	}
 	for i, test := range tests {
-		have, err := filepath.EvalSymlinks(context.Background(), fs, test.path)
+		have, err := symlink.EvalSymlinks(context.Background(), fs, test.path)
 		if err != nil {
 			t.Fatal(err)
 		}
diff --git a/internal/serving/disk/symlink/shims.go b/internal/serving/disk/symlink/shims.go
index e29c92641..d383b96ba 100644
--- a/internal/serving/disk/symlink/shims.go
+++ b/internal/serving/disk/symlink/shims.go
@@ -9,13 +9,9 @@ import (
 
 func volumeNameLen(s string) int { return 0 }
 
-func IsAbs(path string) bool        { return filepath.IsAbs(path) }
-func Clean(path string) string      { return filepath.Clean(path) }
-func Join(elem ...string) string    { return filepath.Join(elem...) }
-func VolumeName(path string) string { return filepath.VolumeName(path) }
+func IsAbs(path string) bool   { return filepath.IsAbs(path) }
+func Clean(path string) string { return filepath.Clean(path) }
 
 func EvalSymlinks(ctx context.Context, fs vfs.VFS, path string) (string, error) {
 	return walkSymlinks(ctx, fs, path)
 }
-
-const Separator = filepath.Separator
-- 
GitLab


From a53b8fa4b14b9001d6ea1b6a409d196822308013 Mon Sep 17 00:00:00 2001
From: Jacob Vosmaer <jacob@gitlab.com>
Date: Tue, 11 Aug 2020 17:48:44 +0200
Subject: [PATCH 3/9] Add more tests

---
 internal/serving/disk/helpers.go      |  7 ---
 internal/serving/disk/helpers_test.go | 32 -----------
 internal/serving/disk/reader.go       |  4 +-
 internal/vfs/local/local_test.go      | 79 +++++++++++++++++++++++++--
 internal/vfs/local/testdata/link      |  2 +-
 5 files changed, 77 insertions(+), 47 deletions(-)
 delete mode 100644 internal/serving/disk/helpers_test.go

diff --git a/internal/serving/disk/helpers.go b/internal/serving/disk/helpers.go
index e082d1ed0..73adfa799 100644
--- a/internal/serving/disk/helpers.go
+++ b/internal/serving/disk/helpers.go
@@ -5,13 +5,10 @@ import (
 	"io"
 	"mime"
 	"net/http"
-	"os"
 	"path/filepath"
 	"strconv"
 	"strings"
 
-	"golang.org/x/sys/unix"
-
 	"gitlab.com/gitlab-org/gitlab-pages/internal/httputil"
 )
 
@@ -23,10 +20,6 @@ func endsWithoutHTMLExtension(path string) bool {
 	return !strings.HasSuffix(path, ".html")
 }
 
-func openNoFollow(path string) (*os.File, error) {
-	return os.OpenFile(path, os.O_RDONLY|unix.O_NOFOLLOW, 0)
-}
-
 // Detect file's content-type either by extension or mime-sniffing.
 // Implementation is adapted from Golang's `http.serveContent()`
 // See https://github.com/golang/go/blob/902fc114272978a40d2e65c2510a18e870077559/src/net/http/fs.go#L194
diff --git a/internal/serving/disk/helpers_test.go b/internal/serving/disk/helpers_test.go
deleted file mode 100644
index eea51500e..000000000
--- a/internal/serving/disk/helpers_test.go
+++ /dev/null
@@ -1,32 +0,0 @@
-package disk
-
-import (
-	"io/ioutil"
-	"os"
-	"testing"
-
-	"github.com/stretchr/testify/require"
-)
-
-func TestOpenNoFollow(t *testing.T) {
-	tmpfile, err := ioutil.TempFile("", "link-test")
-	require.NoError(t, err)
-	defer tmpfile.Close()
-
-	orig := tmpfile.Name()
-	softLink := orig + ".link"
-	defer os.Remove(orig)
-
-	source, err := openNoFollow(orig)
-	require.NoError(t, err)
-	require.NotNil(t, source)
-	defer source.Close()
-
-	err = os.Symlink(orig, softLink)
-	require.NoError(t, err)
-	defer os.Remove(softLink)
-
-	link, err := openNoFollow(softLink)
-	require.Error(t, err)
-	require.Nil(t, link)
-}
diff --git a/internal/serving/disk/reader.go b/internal/serving/disk/reader.go
index 63b75ad7e..fd6f9bc0d 100644
--- a/internal/serving/disk/reader.go
+++ b/internal/serving/disk/reader.go
@@ -128,14 +128,14 @@ func (reader *Reader) resolvePath(ctx context.Context, publicPath string, subPat
 func (reader *Reader) serveFile(ctx context.Context, w http.ResponseWriter, r *http.Request, origPath string, accessControl bool) error {
 	fullPath := reader.handleGZip(ctx, w, r, origPath)
 
-	file, err := openNoFollow(fullPath)
+	file, err := reader.vfs().Open(ctx, fullPath)
 	if err != nil {
 		return err
 	}
 
 	defer file.Close()
 
-	fi, err := file.Stat()
+	fi, err := reader.vfs().Lstat(ctx, fullPath)
 	if err != nil {
 		return err
 	}
diff --git a/internal/vfs/local/local_test.go b/internal/vfs/local/local_test.go
index 651ab5ac0..856eb46b8 100644
--- a/internal/vfs/local/local_test.go
+++ b/internal/vfs/local/local_test.go
@@ -2,6 +2,8 @@ package local
 
 import (
 	"context"
+	"io/ioutil"
+	"os"
 	"testing"
 
 	"github.com/stretchr/testify/require"
@@ -13,11 +15,78 @@ func TestReadlink(t *testing.T) {
 
 	target, err := fs.Readlink(ctx, "testdata/link")
 	require.NoError(t, err)
-	require.Equal(t, "target", target)
+	require.Equal(t, "file", target)
+}
+
+func TestReadlinkNotSymlink(t *testing.T) {
+	ctx := context.Background()
+	fs := VFS{}
+
+	for _, path := range []string{"testdata", "testdata/file"} {
+		t.Run(path, func(t *testing.T) {
+			_, err := os.Lstat(path)
+			require.NoError(t, err, "sanity check: input must actually exist")
+
+			_, err = fs.Readlink(ctx, "testdata")
+			require.Error(t, err, "expect readlink to fail")
+		})
+	}
+}
+
+func TestLstat(t *testing.T) {
+	ctx := context.Background()
+	fs := VFS{}
+
+	testCases := []struct {
+		path string
+		mode os.FileMode
+	}{
+		{path: "testdata", mode: os.ModeDir | 0755},
+		{path: "testdata/file", mode: 0644},
+		{path: "testdata/link", mode: os.ModeSymlink | 0755},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.path, func(t *testing.T) {
+			// We cannot chmod a symlink: it would chmod the symlink target.
+			if tc.mode&os.ModeSymlink == 0 {
+				require.NoError(t, os.Chmod(tc.path, tc.mode&os.ModePerm), "preparation: deterministic permissions")
+			}
+
+			fi, err := fs.Lstat(ctx, tc.path)
+			require.NoError(t, err, "lstat error")
+			require.Equal(t, tc.mode, fi.Mode(), "file mode")
+		})
+	}
+}
+
+func TestOpen(t *testing.T) {
+	ctx := context.Background()
+	fs := VFS{}
+
+	f, err := fs.Open(ctx, "testdata/file")
+	require.NoError(t, err, "open file")
+
+	data, err := ioutil.ReadAll(f)
+	require.NoError(t, err, "read from file")
+	require.Equal(t, "hello\n", string(data), "file contents")
+
+	require.NoError(t, f.Close(), "close file")
+}
+
+func TestOpenDenySymlink(t *testing.T) {
+	ctx := context.Background()
+	fs := VFS{}
+	const symlinkPath = "testdata/link"
+
+	fi, err := os.Stat(symlinkPath)
+	require.NoError(t, err, "stat link")
+	require.Equal(t, os.FileMode(0), fi.Mode()&os.ModeType, "sanity check: link target should be a regular file")
 
-	_, err = fs.Readlink(ctx, "testdata")
-	require.Error(t, err, "expect readlink to fail on directory")
+	fi, err = os.Lstat(symlinkPath)
+	require.NoError(t, err, "lstat link")
+	require.Equal(t, os.ModeSymlink, fi.Mode()&os.ModeType, "sanity check: testdata/link should be a symlink")
 
-	_, err = fs.Readlink(ctx, "testdata/file")
-	require.Error(t, err, "expect readlink to fail on regular file")
+	_, err = fs.Open(ctx, symlinkPath)
+	require.Error(t, err, "opening symlink should fail (security mechanism)")
 }
diff --git a/internal/vfs/local/testdata/link b/internal/vfs/local/testdata/link
index 1de565933..1a010b1c0 120000
--- a/internal/vfs/local/testdata/link
+++ b/internal/vfs/local/testdata/link
@@ -1 +1 @@
-target
\ No newline at end of file
+file
\ No newline at end of file
-- 
GitLab


From 1230cf19d2a85be397037bb934f33787e3d91b87 Mon Sep 17 00:00:00 2001
From: Jacob Vosmaer <jacob@gitlab.com>
Date: Tue, 11 Aug 2020 18:07:17 +0200
Subject: [PATCH 4/9] Add VFS metrics

---
 internal/serving/disk/reader.go |  2 +-
 internal/vfs/vfs.go             | 36 +++++++++++++++++++++++++++++++++
 metrics/metrics.go              |  7 +++++++
 3 files changed, 44 insertions(+), 1 deletion(-)

diff --git a/internal/serving/disk/reader.go b/internal/serving/disk/reader.go
index fd6f9bc0d..e4f160a73 100644
--- a/internal/serving/disk/reader.go
+++ b/internal/serving/disk/reader.go
@@ -23,7 +23,7 @@ type Reader struct {
 	fileSizeMetric prometheus.Histogram
 }
 
-func (reader *Reader) vfs() vfs.VFS { return local.VFS{} }
+func (reader *Reader) vfs() vfs.VFS { return vfs.Instrumented(local.VFS{}, "disk") }
 
 func (reader *Reader) tryFile(h serving.Handler) error {
 	ctx := h.Request.Context()
diff --git a/internal/vfs/vfs.go b/internal/vfs/vfs.go
index 97e5f6211..07c99b776 100644
--- a/internal/vfs/vfs.go
+++ b/internal/vfs/vfs.go
@@ -4,16 +4,52 @@ import (
 	"context"
 	"io"
 	"os"
+	"strconv"
+
+	"gitlab.com/gitlab-org/gitlab-pages/metrics"
 )
 
+// VFS abstracts the things Pages needs to serve a static site from disk.
 type VFS interface {
 	Lstat(ctx context.Context, name string) (os.FileInfo, error)
 	Readlink(ctx context.Context, name string) (string, error)
 	Open(ctx context.Context, name string) (File, error)
 }
 
+// File represents an open file, which will typically be the response body of a Pages request.
 type File interface {
 	io.Reader
 	io.Seeker
 	io.Closer
 }
+
+func Instrumented(fs VFS, name string) VFS {
+	return &InstrumentedVFS{fs: fs, name: name}
+}
+
+type InstrumentedVFS struct {
+	fs   VFS
+	name string
+}
+
+func (i *InstrumentedVFS) increment(operation string, err error) {
+	metrics.VFSOperations.WithLabelValues(i.name, operation, strconv.FormatBool(err == nil)).Inc()
+}
+
+func (i *InstrumentedVFS) Lstat(ctx context.Context, name string) (os.FileInfo, error) {
+	fi, err := i.fs.Lstat(ctx, name)
+	i.increment("Lstat", err)
+	return fi, err
+}
+
+func (i *InstrumentedVFS) Readlink(ctx context.Context, name string) (string, error) {
+	target, err := i.fs.Readlink(ctx, name)
+	i.increment("Readlink", err)
+	return target, err
+}
+
+func (i *InstrumentedVFS) Open(ctx context.Context, name string) (File, error) {
+	f, err := i.fs.Open(ctx, name)
+	i.increment("Open", err)
+	return f, err
+}
diff --git a/metrics/metrics.go b/metrics/metrics.go
index 886362799..0792a41f4 100644
--- a/metrics/metrics.go
+++ b/metrics/metrics.go
@@ -91,6 +91,12 @@ var (
 		Help:    "The time (in seconds) taken to serve a file",
 		Buckets: []float64{0.1, 0.5, 1, 2.5, 5, 10, 60, 180},
 	})
+
+	// VFSOperations metric for VFS operations (lstat, readlink, open)
+	VFSOperations = prometheus.NewCounterVec(prometheus.CounterOpts{
+		Name: "gitlab_pages_vfs_operations_total",
+		Help: "The number of VFS operations",
+	}, []string{"vfs_name", "operation", "success"})
 )
 
 // MustRegister collectors with the Prometheus client
@@ -110,5 +116,6 @@ func MustRegister() {
 		ServerlessLatency,
 		DiskServingFileSize,
 		ServingTime,
+		VFSOperations,
 	)
 }
-- 
GitLab


From 7f1075b5d6108d4816f65a20840c8b08d84b934c Mon Sep 17 00:00:00 2001
From: Jacob Vosmaer <jacob@gitlab.com>
Date: Tue, 11 Aug 2020 18:11:51 +0200
Subject: [PATCH 5/9] Make lstat test more portable

---
 internal/vfs/local/local_test.go | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/internal/vfs/local/local_test.go b/internal/vfs/local/local_test.go
index 856eb46b8..abc96461c 100644
--- a/internal/vfs/local/local_test.go
+++ b/internal/vfs/local/local_test.go
@@ -38,24 +38,28 @@ func TestLstat(t *testing.T) {
 	fs := VFS{}
 
 	testCases := []struct {
-		path string
-		mode os.FileMode
+		path     string
+		modePerm os.FileMode
+		modeType os.FileMode
 	}{
-		{path: "testdata", mode: os.ModeDir | 0755},
-		{path: "testdata/file", mode: 0644},
-		{path: "testdata/link", mode: os.ModeSymlink | 0755},
+		{path: "testdata", modeType: os.ModeDir, modePerm: 0755},
+		{path: "testdata/file", modeType: 0, modePerm: 0644},
+		{path: "testdata/link", modeType: os.ModeSymlink}, // Permissions of symlinks are platform dependent
 	}
 
 	for _, tc := range testCases {
 		t.Run(tc.path, func(t *testing.T) {
-			// We cannot chmod a symlink: it would chmod the symlink target.
-			if tc.mode&os.ModeSymlink == 0 {
-				require.NoError(t, os.Chmod(tc.path, tc.mode&os.ModePerm), "preparation: deterministic permissions")
+			if tc.modePerm > 0 {
+				require.NoError(t, os.Chmod(tc.path, tc.modePerm), "preparation: deterministic permissions")
 			}
 
 			fi, err := fs.Lstat(ctx, tc.path)
 			require.NoError(t, err, "lstat error")
-			require.Equal(t, tc.mode, fi.Mode(), "file mode")
+
+			require.Equal(t, tc.modeType, fi.Mode()&os.ModeType, "file mode: type")
+			if tc.modePerm > 0 {
+				require.Equal(t, tc.modeType, fi.Mode()&os.ModeType, "file mode: type")
+			}
 		})
 	}
 }
-- 
GitLab


From 2fa8107654a2ad82ee3dee05fafcb5b04ed64b7c Mon Sep 17 00:00:00 2001
From: Jacob Vosmaer <jacob@gitlab.com>
Date: Wed, 12 Aug 2020 10:00:03 +0000
Subject: [PATCH 6/9] Apply 2 suggestion(s) to 1 file(s)

---
 internal/vfs/local/local_test.go | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/internal/vfs/local/local_test.go b/internal/vfs/local/local_test.go
index abc96461c..ee1912540 100644
--- a/internal/vfs/local/local_test.go
+++ b/internal/vfs/local/local_test.go
@@ -27,7 +27,7 @@ func TestReadlinkNotSymlink(t *testing.T) {
 			_, err := os.Lstat(path)
 			require.NoError(t, err, "sanity check: input must actually exist")
 
-			_, err = fs.Readlink(ctx, "testdata")
+			_, err = fs.Readlink(ctx, path)
 			require.Error(t, err, "expect readlink to fail")
 		})
 	}
@@ -43,7 +43,7 @@ func TestLstat(t *testing.T) {
 		modeType os.FileMode
 	}{
 		{path: "testdata", modeType: os.ModeDir, modePerm: 0755},
-		{path: "testdata/file", modeType: 0, modePerm: 0644},
+		{path: "testdata/file", modeType: os.FileMode(0), modePerm: 0644},
 		{path: "testdata/link", modeType: os.ModeSymlink}, // Permissions of symlinks are platform dependent
 	}
 
-- 
GitLab


From 683b78dac1f92a0975176b21b7cfb79e054b18f3 Mon Sep 17 00:00:00 2001
From: Jacob Vosmaer <jacob@gitlab.com>
Date: Wed, 12 Aug 2020 12:01:39 +0200
Subject: [PATCH 7/9] Fix copy paste error

---
 internal/vfs/local/local_test.go | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/internal/vfs/local/local_test.go b/internal/vfs/local/local_test.go
index ee1912540..c41f96bc9 100644
--- a/internal/vfs/local/local_test.go
+++ b/internal/vfs/local/local_test.go
@@ -58,7 +58,7 @@ func TestLstat(t *testing.T) {
 
 			require.Equal(t, tc.modeType, fi.Mode()&os.ModeType, "file mode: type")
 			if tc.modePerm > 0 {
-				require.Equal(t, tc.modeType, fi.Mode()&os.ModeType, "file mode: type")
+				require.Equal(t, tc.modePerm, fi.Mode()&os.ModePerm, "file mode: permissions")
 			}
 		})
 	}
-- 
GitLab


From 24cd6ac4bbc9ab6b255cd54d22b4131b60709aac Mon Sep 17 00:00:00 2001
From: Jacob Vosmaer <jacob@gitlab.com>
Date: Wed, 12 Aug 2020 15:35:21 +0200
Subject: [PATCH 8/9] Use struct field instead of getter

---
 internal/serving/disk/helpers.go |  4 ++--
 internal/serving/disk/reader.go  | 16 +++++++---------
 internal/serving/disk/serving.go | 15 ++++++++-------
 3 files changed, 17 insertions(+), 18 deletions(-)

diff --git a/internal/serving/disk/helpers.go b/internal/serving/disk/helpers.go
index 73adfa799..e6d3f8ab6 100644
--- a/internal/serving/disk/helpers.go
+++ b/internal/serving/disk/helpers.go
@@ -29,7 +29,7 @@ func (reader *Reader) detectContentType(ctx context.Context, path string) (strin
 	if contentType == "" {
 		var buf [512]byte
 
-		file, err := reader.vfs().Open(ctx, path)
+		file, err := reader.vfs.Open(ctx, path)
 		if err != nil {
 			return "", err
 		}
@@ -63,7 +63,7 @@ func (reader *Reader) handleGZip(ctx context.Context, w http.ResponseWriter, r *
 	gzipPath := fullPath + ".gz"
 
 	// Ensure the .gz file is not a symlink
-	fi, err := reader.vfs().Lstat(ctx, gzipPath)
+	fi, err := reader.vfs.Lstat(ctx, gzipPath)
 	if err != nil || !fi.Mode().IsRegular() {
 		return fullPath
 	}
diff --git a/internal/serving/disk/reader.go b/internal/serving/disk/reader.go
index e4f160a73..8c7ee3bf3 100644
--- a/internal/serving/disk/reader.go
+++ b/internal/serving/disk/reader.go
@@ -15,16 +15,14 @@ import (
 	"gitlab.com/gitlab-org/gitlab-pages/internal/serving"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/serving/disk/symlink"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/vfs"
-	"gitlab.com/gitlab-org/gitlab-pages/internal/vfs/local"
 )
 
 // Reader is a disk access driver
 type Reader struct {
 	fileSizeMetric prometheus.Histogram
+	vfs            vfs.VFS
 }
 
-func (reader *Reader) vfs() vfs.VFS { return vfs.Instrumented(local.VFS{}, "disk") }
-
 func (reader *Reader) tryFile(h serving.Handler) error {
 	ctx := h.Request.Context()
 	fullPath, err := reader.resolvePath(ctx, h.LookupPath.Path, h.SubPath)
@@ -86,7 +84,7 @@ func (reader *Reader) resolvePath(ctx context.Context, publicPath string, subPat
 	// where we want to traverse full path as supplied by user
 	// (including ..)
 	testPath := publicPath + strings.Join(subPath, "/")
-	fullPath, err := symlink.EvalSymlinks(ctx, reader.vfs(), testPath)
+	fullPath, err := symlink.EvalSymlinks(ctx, reader.vfs, testPath)
 
 	if err != nil {
 		if endsWithoutHTMLExtension(testPath) {
@@ -103,7 +101,7 @@ func (reader *Reader) resolvePath(ctx context.Context, publicPath string, subPat
 		return "", fmt.Errorf("%q should be in %q", fullPath, publicPath)
 	}
 
-	fi, err := reader.vfs().Lstat(ctx, fullPath)
+	fi, err := reader.vfs.Lstat(ctx, fullPath)
 	if err != nil {
 		return "", err
 	}
@@ -128,14 +126,14 @@ func (reader *Reader) resolvePath(ctx context.Context, publicPath string, subPat
 func (reader *Reader) serveFile(ctx context.Context, w http.ResponseWriter, r *http.Request, origPath string, accessControl bool) error {
 	fullPath := reader.handleGZip(ctx, w, r, origPath)
 
-	file, err := reader.vfs().Open(ctx, fullPath)
+	file, err := reader.vfs.Open(ctx, fullPath)
 	if err != nil {
 		return err
 	}
 
 	defer file.Close()
 
-	fi, err := reader.vfs().Lstat(ctx, fullPath)
+	fi, err := reader.vfs.Lstat(ctx, fullPath)
 	if err != nil {
 		return err
 	}
@@ -163,13 +161,13 @@ func (reader *Reader) serveCustomFile(ctx context.Context, w http.ResponseWriter
 	fullPath := reader.handleGZip(ctx, w, r, origPath)
 
 	// Open and serve content of file
-	file, err := reader.vfs().Open(ctx, fullPath)
+	file, err := reader.vfs.Open(ctx, fullPath)
 	if err != nil {
 		return err
 	}
 	defer file.Close()
 
-	fi, err := reader.vfs().Lstat(ctx, fullPath)
+	fi, err := reader.vfs.Lstat(ctx, fullPath)
 	if err != nil {
 		return err
 	}
diff --git a/internal/serving/disk/serving.go b/internal/serving/disk/serving.go
index f5ec0d785..6df70c971 100644
--- a/internal/serving/disk/serving.go
+++ b/internal/serving/disk/serving.go
@@ -3,15 +3,11 @@ package disk
 import (
 	"gitlab.com/gitlab-org/gitlab-pages/internal/httperrors"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/serving"
+	"gitlab.com/gitlab-org/gitlab-pages/internal/vfs"
+	"gitlab.com/gitlab-org/gitlab-pages/internal/vfs/local"
 	"gitlab.com/gitlab-org/gitlab-pages/metrics"
 )
 
-var disk = &Disk{
-	reader: Reader{
-		fileSizeMetric: metrics.DiskServingFileSize,
-	},
-}
-
 // Disk describes a disk access serving
 type Disk struct {
 	reader Reader
@@ -36,5 +32,10 @@ func (s *Disk) ServeNotFoundHTTP(h serving.Handler) {
 // New returns a serving instance that is capable of reading files
 // from the disk
 func New() serving.Serving {
-	return disk
+	return &Disk{
+		reader: Reader{
+			fileSizeMetric: metrics.DiskServingFileSize,
+			vfs:            vfs.Instrumented(local.VFS{}, "disk"),
+		},
+	}
 }
-- 
GitLab


From 4d3f11df1ba130d1c378dcd716d8733e11edd6dd Mon Sep 17 00:00:00 2001
From: Jacob Vosmaer <jacob@gitlab.com>
Date: Wed, 12 Aug 2020 15:37:51 +0200
Subject: [PATCH 9/9] Play diff golf

---
 internal/serving/disk/serving.go | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/internal/serving/disk/serving.go b/internal/serving/disk/serving.go
index 6df70c971..b7501b802 100644
--- a/internal/serving/disk/serving.go
+++ b/internal/serving/disk/serving.go
@@ -8,6 +8,13 @@ import (
 	"gitlab.com/gitlab-org/gitlab-pages/metrics"
 )
 
+var disk = &Disk{
+	reader: Reader{
+		fileSizeMetric: metrics.DiskServingFileSize,
+		vfs:            vfs.Instrumented(local.VFS{}, "disk"),
+	},
+}
+
 // Disk describes a disk access serving
 type Disk struct {
 	reader Reader
@@ -32,10 +39,5 @@ func (s *Disk) ServeNotFoundHTTP(h serving.Handler) {
 // New returns a serving instance that is capable of reading files
 // from the disk
 func New() serving.Serving {
-	return &Disk{
-		reader: Reader{
-			fileSizeMetric: metrics.DiskServingFileSize,
-			vfs:            vfs.Instrumented(local.VFS{}, "disk"),
-		},
-	}
+	return disk
 }
-- 
GitLab