Merge branch '485-add-more-tests-to-serve-from-file' into 'master'

Add extra tests for serving zip archives from disk

Closes #485

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

8 files changed, 331 insertions, 40 deletions

diff --git a/internal/httpfs/http_fs.go b/internal/httpfs/http_fs.go
index 274bca57..cd2edb83 100644
--- a/internal/httpfs/http_fs.go
+++ b/internal/httpfs/http_fs.go
@@ -13,6 +13,8 @@ import (
 	"path"
 	"path/filepath"
 	"strings"
+
+	"gitlab.com/gitlab-org/labkit/log"
 )
 
 var (
@@ -52,13 +54,15 @@ func (p *fileSystemPaths) Open(name string) (http.File, error) {
 	if err != nil {
 		return nil, err
 	}
-
 	for _, allowedPath := range p.allowedPaths {
 		if strings.HasPrefix(absPath, allowedPath+"/") {
 			return os.Open(absPath)
 		}
 	}
 
+	log.WithError(os.ErrPermission).Errorf("requested filepath %q not in allowed paths: %q",
+		absPath, strings.Join(p.allowedPaths, string(os.PathListSeparator)))
+
 	// os.ErrPermission is converted to http.StatusForbidden
 	// https://github.com/golang/go/blob/release-branch.go1.15/src/net/http/fs.go#L635
 	return nil, os.ErrPermission
diff --git a/internal/testhelpers/testhelpers.go b/internal/testhelpers/testhelpers.go
index 422a3d9a..3ec97a79 100644
--- a/internal/testhelpers/testhelpers.go
+++ b/internal/testhelpers/testhelpers.go
@@ -1,10 +1,12 @@
 package testhelpers
 
 import (
+	"fmt"
 	"mime"
 	"net/http"
 	"net/http/httptest"
 	"net/url"
+	"os"
 	"testing"
 
 	"github.com/sirupsen/logrus"
@@ -55,3 +57,23 @@ func AssertLogContains(t *testing.T, wantLogEntry string, entries []*logrus.Entr
 		require.Contains(t, messages, wantLogEntry)
 	}
 }
+
+// ToFileProtocol appends the file:// protocol to the current os.Getwd
+// and formats path to be a full filepath
+func ToFileProtocol(t *testing.T, path string) string {
+	t.Helper()
+
+	wd := Getwd(t)
+
+	return fmt.Sprintf("file://%s/%s", wd, path)
+}
+
+// Getwd must return current working directory
+func Getwd(t *testing.T) string {
+	t.Helper()
+
+	wd, err := os.Getwd()
+	require.NoError(t, err)
+
+	return wd
+}
diff --git a/internal/vfs/zip/archive_test.go b/internal/vfs/zip/archive_test.go
index 58b7c74a..421990db 100644
--- a/internal/vfs/zip/archive_test.go
+++ b/internal/vfs/zip/archive_test.go
@@ -33,9 +33,11 @@ var (
 )
 
 func TestOpen(t *testing.T) {
-	zip, cleanup := openZipArchive(t, nil)
-	defer cleanup()
+	t.Run("open_from_server", runZipTest(t, testOpen, false))
+	t.Run("open_from_disk", runZipTest(t, testOpen, true))
+}
 
+func testOpen(t *testing.T, zip *zipArchive) {
 	tests := map[string]struct {
 		file            string
 		expectedContent string
@@ -190,9 +192,11 @@ func TestOpenCached(t *testing.T) {
 }
 
 func TestLstat(t *testing.T) {
-	zip, cleanup := openZipArchive(t, nil)
-	defer cleanup()
+	t.Run("lstat_from_server", runZipTest(t, testLstat, false))
+	t.Run("lstat_from_disk", runZipTest(t, testLstat, true))
+}
 
+func testLstat(t *testing.T, zip *zipArchive) {
 	tests := map[string]struct {
 		file         string
 		isDir        bool
@@ -270,9 +274,11 @@ func TestLstat(t *testing.T) {
 }
 
 func TestReadLink(t *testing.T) {
-	zip, cleanup := openZipArchive(t, nil)
-	defer cleanup()
+	t.Run("read_link_from_server", runZipTest(t, testReadLink, false))
+	t.Run("read_link_from_disk", runZipTest(t, testReadLink, true))
+}
 
+func testReadLink(t *testing.T, zip *zipArchive) {
 	tests := map[string]struct {
 		file        string
 		expectedErr error
@@ -314,7 +320,7 @@ func TestReadLink(t *testing.T) {
 
 func TestReadlinkCached(t *testing.T) {
 	var requests int64
-	zip, cleanup := openZipArchive(t, &requests)
+	zip, cleanup := openZipArchive(t, &requests, false)
 	defer cleanup()
 
 	t.Run("readlink first time", func(t *testing.T) {
@@ -370,7 +376,7 @@ func TestReadArchiveFails(t *testing.T) {
 	require.EqualError(t, err, os.ErrNotExist.Error())
 }
 
-func openZipArchive(t *testing.T, requests *int64) (*zipArchive, func()) {
+func openZipArchive(t *testing.T, requests *int64, fromDisk bool) (*zipArchive, func()) {
 	t.Helper()
 
 	if requests == nil {
@@ -379,17 +385,31 @@ func openZipArchive(t *testing.T, requests *int64) (*zipArchive, func()) {
 
 	testServerURL, cleanup := newZipFileServerURL(t, "group/zip.gitlab.io/public-without-dirs.zip", requests)
 
+	wd, err := os.Getwd()
+	require.NoError(t, err)
+
+	zipCfg.AllowedPaths = []string{wd}
+
 	fs := New(zipCfg).(*zipVFS)
+	err = fs.Reconfigure(&config.Config{Zip: zipCfg})
+	require.NoError(t, err)
+
 	zip := newArchive(fs, time.Second)
 
-	err := zip.openArchive(context.Background(), testServerURL+"/public.zip")
-	require.NoError(t, err)
+	if fromDisk {
+		fileName := testhelpers.ToFileProtocol(t, "group/zip.gitlab.io/public-without-dirs.zip")
+		err := zip.openArchive(context.Background(), fileName)
+		require.NoError(t, err)
+	} else {
+		err := zip.openArchive(context.Background(), testServerURL+"/public.zip")
+		require.NoError(t, err)
+		require.Equal(t, int64(3), atomic.LoadInt64(requests), "we expect three requests to open ZIP archive: size and two to seek central directory")
+	}
 
 	// public/ public/index.html public/404.html public/symlink.html
 	// public/subdir/ public/subdir/hello.html public/subdir/linked.html
 	// public/bad_symlink.html public/subdir/2bp3Qzs...
 	require.NotZero(t, zip.files)
-	require.Equal(t, int64(3), atomic.LoadInt64(requests), "we expect three requests to open ZIP archive: size and two to seek central directory")
 
 	return zip, func() {
 		cleanup()
@@ -472,3 +492,14 @@ func BenchmarkArchiveRead(b *testing.B) {
 		})
 	}
 }
+
+func runZipTest(t *testing.T, runTest func(t *testing.T, zip *zipArchive), fromDisk bool) func(t *testing.T) {
+	t.Helper()
+
+	return func(t *testing.T) {
+		zip, cleanup := openZipArchive(t, nil, fromDisk)
+		defer cleanup()
+
+		runTest(t, zip)
+	}
+}
diff --git a/internal/vfs/zip/vfs_test.go b/internal/vfs/zip/vfs_test.go
index ffda1fb6..5d7015f5 100644
--- a/internal/vfs/zip/vfs_test.go
+++ b/internal/vfs/zip/vfs_test.go
@@ -6,6 +6,9 @@ import (
 	"testing"
 	"time"
 
+	"gitlab.com/gitlab-org/gitlab-pages/internal/config"
+	"gitlab.com/gitlab-org/gitlab-pages/internal/testhelpers"
+
 	"github.com/prometheus/client_golang/prometheus/testutil"
 	"github.com/stretchr/testify/require"
 
@@ -213,6 +216,35 @@ func TestVFSFindOrOpenArchiveRefresh(t *testing.T) {
 	}
 }
 
+func TestVFSReconfigureTransport(t *testing.T) {
+	chdir := false
+	cleanup := testhelpers.ChdirInPath(t, "../../../shared/pages", &chdir)
+	defer cleanup()
+
+	fileURL := testhelpers.ToFileProtocol(t, "group/zip.gitlab.io/public.zip")
+
+	vfs := New(zipCfg)
+
+	// try to open a file URL without registering the file protocol
+	_, err := vfs.Root(context.Background(), fileURL)
+	require.Error(t, err)
+	require.Contains(t, err.Error(), "unsupported protocol scheme \"file\"")
+
+	// reconfigure VFS with allowed paths and try to open file://
+	cfg := *zipCfg
+	cfg.AllowedPaths = []string{testhelpers.Getwd(t)}
+
+	err = vfs.Reconfigure(&config.Config{Zip: &cfg})
+	require.NoError(t, err)
+
+	root, err := vfs.Root(context.Background(), fileURL)
+	require.NoError(t, err)
+
+	fi, err := root.Lstat(context.Background(), "index.html")
+	require.NoError(t, err)
+	require.Equal(t, "index.html", fi.Name())
+}
+
 func withExpectedArchiveCount(t *testing.T, archiveCount int, fn func(t *testing.T)) {
 	t.Helper()
 
diff --git a/shared/pages/@hashed/67/06/670671cd97404156226e507973f2ab8330d3022ca96e0c93bdbdb320c41adcaf/pages_deployments/01/artifacts.zip b/shared/pages/@hashed/67/06/670671cd97404156226e507973f2ab8330d3022ca96e0c93bdbdb320c41adcaf/pages_deployments/01/artifacts.zip
new file mode 100644
index 00000000..f1278bce
--- /dev/null
+++ b/shared/pages/@hashed/67/06/670671cd97404156226e507973f2ab8330d3022ca96e0c93bdbdb320c41adcaf/pages_deployments/01/artifacts.zip
diff --git a/test/acceptance/helpers_test.go b/test/acceptance/helpers_test.go
index d3b4f7b9..1ece8148 100644
--- a/test/acceptance/helpers_test.go
+++ b/test/acceptance/helpers_test.go
@@ -5,6 +5,7 @@ import (
 	"context"
 	"crypto/tls"
 	"crypto/x509"
+	"encoding/json"
 	"fmt"
 	"io"
 	"io/ioutil"
@@ -19,10 +20,11 @@ import (
 	"testing"
 	"time"
 
-	proxyproto "github.com/pires/go-proxyproto"
+	"github.com/pires/go-proxyproto"
 	"github.com/stretchr/testify/require"
 
 	"gitlab.com/gitlab-org/gitlab-pages/internal/request"
+	"gitlab.com/gitlab-org/gitlab-pages/test/acceptance/testdata"
 )
 
 // The HTTPS certificate isn't signed by anyone. This http client is set up
@@ -347,7 +349,7 @@ func getPagesArgs(t *testing.T, listeners []ListenSpec, promPort string, extraAr
 		args = append(args, "-root-key", key, "-root-cert", cert)
 	}
 
-	if !contains(args, "pages-root") {
+	if !contains(extraArgs, "-pages-root") {
 		args = append(args, "-pages-root", "../../shared/pages")
 	}
 
@@ -541,13 +543,13 @@ type stubOpts struct {
 	statusHandler       http.HandlerFunc
 	pagesHandler        http.HandlerFunc
 	pagesStatusResponse int
+	pagesRoot           string
 }
 
 func NewGitlabDomainsSourceStub(t *testing.T, opts *stubOpts) *httptest.Server {
 	t.Helper()
 	require.NotNil(t, opts)
 
-	opts.apiCalled = false
 	currentStatusCount := 0
 
 	mux := http.NewServeMux()
@@ -566,7 +568,36 @@ func NewGitlabDomainsSourceStub(t *testing.T, opts *stubOpts) *httptest.Server {
 
 	mux.HandleFunc("/api/v4/internal/pages/status", statusHandler)
 
-	pagesHandler := func(w http.ResponseWriter, r *http.Request) {
+	pagesHandler := defaultAPIHandler(t, opts)
+	if opts.pagesHandler != nil {
+		pagesHandler = opts.pagesHandler
+	}
+
+	mux.HandleFunc("/api/v4/internal/pages", pagesHandler)
+
+	return httptest.NewServer(mux)
+}
+
+func lookupFromFile(t *testing.T, domain string, w http.ResponseWriter) {
+	fixture, err := os.Open("../../shared/lookups/" + domain + ".json")
+	if os.IsNotExist(err) {
+		w.WriteHeader(http.StatusNoContent)
+
+		t.Logf("GitLab domain %s source stub served 204", domain)
+		return
+	}
+
+	defer fixture.Close()
+	require.NoError(t, err)
+
+	_, err = io.Copy(w, fixture)
+	require.NoError(t, err)
+
+	t.Logf("GitLab domain %s source stub served lookup", domain)
+}
+
+func defaultAPIHandler(t *testing.T, opts *stubOpts) http.HandlerFunc {
+	return func(w http.ResponseWriter, r *http.Request) {
 		domain := r.URL.Query().Get("host")
 		if domain == "127.0.0.1" {
 			// shortcut for healthy checkup done by WaitUntilRequestSucceeds
@@ -581,31 +612,16 @@ func NewGitlabDomainsSourceStub(t *testing.T, opts *stubOpts) *httptest.Server {
 			return
 		}
 
-		path := "../../shared/lookups/" + domain + ".json"
-
-		fixture, err := os.Open(path)
-		if os.IsNotExist(err) {
-			w.WriteHeader(http.StatusNoContent)
-
-			t.Logf("GitLab domain %s source stub served 204", domain)
+		// check if predefined response exists
+		if responseFn, ok := testdata.DomainResponses[domain]; ok {
+			err := json.NewEncoder(w).Encode(responseFn(opts.pagesRoot))
+			require.NoError(t, err)
 			return
 		}
 
-		defer fixture.Close()
-		require.NoError(t, err)
-
-		_, err = io.Copy(w, fixture)
-		require.NoError(t, err)
-
-		t.Logf("GitLab domain %s source stub served lookup", domain)
-	}
-	if opts.pagesHandler != nil {
-		pagesHandler = opts.pagesHandler
+		// serve lookup from files
+		lookupFromFile(t, domain, w)
 	}
-
-	mux.HandleFunc("/api/v4/internal/pages", pagesHandler)
-
-	return httptest.NewServer(mux)
 }
 
 func newConfigFile(t *testing.T, configs ...string) string {
diff --git a/test/acceptance/testdata/api_responses.go b/test/acceptance/testdata/api_responses.go
new file mode 100644
index 00000000..a79d2130
--- /dev/null
+++ b/test/acceptance/testdata/api_responses.go
@@ -0,0 +1,81 @@
+package testdata
+
+import (
+	"fmt"
+
+	"gitlab.com/gitlab-org/gitlab-pages/internal/source/gitlab/api"
+)
+
+type responseFn func(string) api.VirtualDomain
+
+// DomainResponses holds the predefined API responses for certain domains
+// that can be used with the GitLab API stub in acceptance tests
+var DomainResponses = map[string]responseFn{
+	"zip-from-disk.gitlab.io":           ZipFromFile,
+	"zip-from-disk-not-found.gitlab.io": ZipFromFileNotFound,
+	"zip-not-allowed-path.gitlab.io":    ZipFromNotAllowedPath,
+}
+
+// ZipFromFile response for zip.gitlab.io
+func ZipFromFile(wd string) api.VirtualDomain {
+	return api.VirtualDomain{
+		Certificate: "",
+		Key:         "",
+		LookupPaths: []api.LookupPath{
+			{
+				ProjectID:     123,
+				AccessControl: false,
+				HTTPSOnly:     false,
+				Prefix:        "/",
+				Source: api.Source{
+					Type:       "zip",
+					Path:       fmt.Sprintf("file://%s/@hashed/67/06/670671cd97404156226e507973f2ab8330d3022ca96e0c93bdbdb320c41adcaf/pages_deployments/01/artifacts.zip", wd),
+					Serverless: api.Serverless{},
+				},
+			},
+		},
+	}
+}
+
+// ZipFromFileNotFound response for zip-from-disk-not-found.gitlab.io
+func ZipFromFileNotFound(wd string) api.VirtualDomain {
+	return api.VirtualDomain{
+		Certificate: "",
+		Key:         "",
+		LookupPaths: []api.LookupPath{
+			{
+				ProjectID:     123,
+				AccessControl: false,
+				HTTPSOnly:     false,
+				Prefix:        "/",
+				Source: api.Source{
+					Type:       "zip",
+					Path:       fmt.Sprintf("file://%s/@hashed/67/06/670671cd97404156226e507973f2ab8330d3022ca96e0c93bdbdb320c41adcaf/pages_deployments/01/unknown.zip", wd),
+					Serverless: api.Serverless{},
+				},
+			},
+		},
+	}
+}
+
+// ZipFromNotAllowedPath response for zip-not-allowed-path.gitlab.io
+func ZipFromNotAllowedPath(wd string) api.VirtualDomain {
+	return api.VirtualDomain{
+		Certificate: "",
+		Key:         "",
+		LookupPaths: []api.LookupPath{
+			{
+				ProjectID:     123,
+				AccessControl: false,
+				HTTPSOnly:     false,
+				Prefix:        "/",
+				Source: api.Source{
+					Type: "zip",
+					// path outside of `pages-root`
+					Path:       "file:///some/random/path/public.zip",
+					Serverless: api.Serverless{},
+				},
+			},
+		},
+	}
+}
diff --git a/test/acceptance/zip_test.go b/test/acceptance/zip_test.go
index 6257458e..a7e82d27 100644
--- a/test/acceptance/zip_test.go
+++ b/test/acceptance/zip_test.go
@@ -5,14 +5,20 @@ import (
 	"net"
 	"net/http"
 	"net/http/httptest"
+	"os"
 	"testing"
 
 	"github.com/stretchr/testify/require"
+
+	"gitlab.com/gitlab-org/gitlab-pages/internal/testhelpers"
 )
 
 func TestZipServing(t *testing.T) {
 	skipUnlessEnabled(t)
 
+	_, cleanup := newZipFileServerURL(t, "../../shared/pages/group/zip.gitlab.io/public.zip")
+	defer cleanup()
+
 	source := NewGitlabDomainsSourceStub(t, &stubOpts{})
 	defer source.Close()
 
@@ -22,9 +28,6 @@ func TestZipServing(t *testing.T) {
 	teardown := RunPagesProcessWithEnvs(t, true, *pagesBinary, listeners, "", []string{}, pagesArgs...)
 	defer teardown()
 
-	_, cleanup := newZipFileServerURL(t, "../../shared/pages/group/zip.gitlab.io/public.zip")
-	defer cleanup()
-
 	tests := map[string]struct {
 		host               string
 		urlSuffix          string
@@ -103,6 +106,108 @@ func TestZipServing(t *testing.T) {
 	}
 }
 
+func TestZipServingFromDisk(t *testing.T) {
+	skipUnlessEnabled(t, "not-inplace-chroot")
+
+	chdir := false
+	defer testhelpers.ChdirInPath(t, "../../shared/pages", &chdir)()
+
+	_, cleanup := newZipFileServerURL(t, "shared/pages/group/zip.gitlab.io/public.zip")
+	defer cleanup()
+
+	wd, err := os.Getwd()
+	require.NoError(t, err)
+
+	source := NewGitlabDomainsSourceStub(t, &stubOpts{
+		pagesRoot: wd,
+	})
+
+	defer source.Close()
+
+	gitLabAPISecretKey := CreateGitLabAPISecretKeyFixtureFile(t)
+
+	pagesArgs := []string{"-gitlab-server", source.URL, "-api-secret-key", gitLabAPISecretKey, "-domain-config-source", "gitlab", "-pages-root", wd}
+	teardown := RunPagesProcessWithEnvs(t, true, *pagesBinary, listeners, "", []string{}, pagesArgs...)
+	defer teardown()
+
+	tests := map[string]struct {
+		host               string
+		urlSuffix          string
+		expectedStatusCode int
+		expectedContent    string
+	}{
+		"base_domain_no_suffix": {
+			host:               "zip-from-disk.gitlab.io",
+			urlSuffix:          "/",
+			expectedStatusCode: http.StatusOK,
+			expectedContent:    "zip.gitlab.io/project/index.html\n",
+		},
+		"file_exists": {
+			host:               "zip-from-disk.gitlab.io",
+			urlSuffix:          "/index.html",
+			expectedStatusCode: http.StatusOK,
+			expectedContent:    "zip.gitlab.io/project/index.html\n",
+		},
+		"file_exists_in_subdir": {
+			host:               "zip-from-disk.gitlab.io",
+			urlSuffix:          "/subdir/hello.html",
+			expectedStatusCode: http.StatusOK,
+			expectedContent:    "zip.gitlab.io/project/subdir/hello.html\n",
+		},
+		"file_exists_symlink": {
+			host:               "zip-from-disk.gitlab.io",
+			urlSuffix:          "/symlink.html",
+			expectedStatusCode: http.StatusOK,
+			expectedContent:    "symlink.html->subdir/linked.html\n",
+		},
+		"dir": {
+			host:               "zip-from-disk.gitlab.io",
+			urlSuffix:          "/subdir/",
+			expectedStatusCode: http.StatusNotFound,
+			expectedContent:    "zip.gitlab.io/project/404.html\n",
+		},
+		"file_does_not_exist": {
+			host:               "zip-from-disk.gitlab.io",
+			urlSuffix:          "/unknown.html",
+			expectedStatusCode: http.StatusNotFound,
+			expectedContent:    "zip.gitlab.io/project/404.html\n",
+		},
+		"bad_symlink": {
+			host:               "zip-from-disk.gitlab.io",
+			urlSuffix:          "/bad-symlink.html",
+			expectedStatusCode: http.StatusNotFound,
+			expectedContent:    "zip.gitlab.io/project/404.html\n",
+		},
+		"with_not_found_zip": {
+			host:               "zip-from-disk-not-found.gitlab.io",
+			urlSuffix:          "/",
+			expectedStatusCode: http.StatusNotFound,
+			expectedContent:    "The page you're looking for could not be found",
+		},
+		"file_not_allowed_in_path": {
+			host:               "zip-not-allowed-path.gitlab.io",
+			urlSuffix:          "/",
+			expectedStatusCode: http.StatusInternalServerError,
+			expectedContent:    "Whoops, something went wrong on our end.",
+		},
+	}
+
+	for name, tt := range tests {
+		t.Run(name, func(t *testing.T) {
+			response, err := GetPageFromListener(t, httpListener, tt.host, tt.urlSuffix)
+			require.NoError(t, err)
+			defer response.Body.Close()
+
+			require.Equal(t, tt.expectedStatusCode, response.StatusCode)
+
+			body, err := ioutil.ReadAll(response.Body)
+			require.NoError(t, err)
+
+			require.Contains(t, string(body), tt.expectedContent, "content mismatch")
+		})
+	}
+}
+
 func TestZipServingConfigShortTimeout(t *testing.T) {
 	skipUnlessEnabled(t)