Properly handle processing failures with `5xx`

Prior to this change ALL processing failures
unrelated to "file missing" would return `404`.

This is inaccurate.

Processing failures are failure of GitLab Pages
and `500` should be returned in such cases.

4 files changed, 154 insertions, 49 deletions

diff --git a/internal/serving/disk/local/serving_test.go b/internal/serving/disk/local/serving_test.go
index 60f01acd..2602451f 100644
--- a/internal/serving/disk/local/serving_test.go
+++ b/internal/serving/disk/local/serving_test.go
@@ -15,29 +15,79 @@ import (
 func TestDisk_ServeFileHTTP(t *testing.T) {
 	defer setUpTests(t)()
 
-	s := Instance()
-	w := httptest.NewRecorder()
-	r := httptest.NewRequest("GET", "http://group.gitlab-example.com/serving/index.html", nil)
-	handler := serving.Handler{
-		Writer:  w,
-		Request: r,
-		LookupPath: &serving.LookupPath{
-			Prefix: "/serving",
-			Path:   "group/serving/public",
+	tests := map[string]struct {
+		vfsPath        string
+		path           string
+		expectedStatus int
+		expectedBody   string
+	}{
+		"accessing /index.html": {
+			vfsPath:        "group/serving/public",
+			path:           "/index.html",
+			expectedStatus: http.StatusOK,
+			expectedBody:   "HTML Document",
+		},
+		"accessing /": {
+			vfsPath:        "group/serving/public",
+			path:           "/",
+			expectedStatus: http.StatusOK,
+			expectedBody:   "HTML Document",
+		},
+		"accessing without /": {
+			vfsPath:        "group/serving/public",
+			path:           "",
+			expectedStatus: http.StatusFound,
+			expectedBody:   `<a href="//group.gitlab-example.com/serving/">Found</a>.`,
+		},
+		"accessing vfs path that is missing": {
+			vfsPath: "group/serving/public-missing",
+			path:    "/index.html",
+			// we expect the status to not be set
+			expectedStatus: 0,
+		},
+		"accessing vfs path that is forbidden (like file)": {
+			vfsPath:        "group/serving/public/index.html",
+			path:           "/index.html",
+			expectedStatus: http.StatusInternalServerError,
 		},
-		SubPath: "/index.html",
 	}
 
-	require.True(t, s.ServeFileHTTP(handler))
+	s := Instance()
 
-	resp := w.Result()
-	defer resp.Body.Close()
+	for name, test := range tests {
+		t.Run(name, func(t *testing.T) {
+			w := httptest.NewRecorder()
+			w.Code = 0 // ensure that code is not set, and it is being set by handler
+			r := httptest.NewRequest("GET", "http://group.gitlab-example.com/serving"+test.path, nil)
 
-	require.Equal(t, http.StatusOK, resp.StatusCode)
-	body, err := ioutil.ReadAll(resp.Body)
-	require.NoError(t, err)
+			handler := serving.Handler{
+				Writer:  w,
+				Request: r,
+				LookupPath: &serving.LookupPath{
+					Prefix: "/serving/",
+					Path:   test.vfsPath,
+				},
+				SubPath: test.path,
+			}
 
-	require.Contains(t, string(body), "HTML Document")
+			if test.expectedStatus == 0 {
+				require.False(t, s.ServeFileHTTP(handler))
+				require.Zero(t, w.Code, "we expect status to not be set")
+				return
+			}
+
+			require.True(t, s.ServeFileHTTP(handler))
+
+			resp := w.Result()
+			defer resp.Body.Close()
+
+			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/serving/disk/reader.go b/internal/serving/disk/reader.go
index e7f15a1e..5b0c56f0 100644
--- a/internal/serving/disk/reader.go
+++ b/internal/serving/disk/reader.go
@@ -11,7 +11,9 @@ import (
 	"time"
 
 	"github.com/prometheus/client_golang/prometheus"
+	"gitlab.com/gitlab-org/labkit/errortracking"
 
+	"gitlab.com/gitlab-org/gitlab-pages/internal/httperrors"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/redirects"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/serving"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/serving/disk/symlink"
@@ -25,39 +27,51 @@ type Reader struct {
 }
 
 // Show the user some validation messages for their _redirects file
-func (reader *Reader) serveRedirectsStatus(h serving.Handler, redirects *redirects.Redirects) error {
+func (reader *Reader) serveRedirectsStatus(h serving.Handler, redirects *redirects.Redirects) {
 	h.Writer.Header().Set("Content-Type", "text/plain; charset=utf-8")
 	h.Writer.Header().Set("X-Content-Type-Options", "nosniff")
 	h.Writer.WriteHeader(http.StatusOK)
-	_, err := fmt.Fprintln(h.Writer, redirects.Status())
-	return err
+	fmt.Fprintln(h.Writer, redirects.Status())
 }
 
-func (reader *Reader) tryRedirects(h serving.Handler) error {
+// tryRedirects returns true if it successfully handled request
+func (reader *Reader) tryRedirects(h serving.Handler) bool {
 	ctx := h.Request.Context()
 	root, err := reader.vfs.Root(ctx, h.LookupPath.Path)
-	if err != nil {
-		return err
+	if vfs.IsNotExist(err) {
+		return false
+	} else if err != nil {
+		httperrors.Serve500WithRequest(h.Writer, h.Request, "vfs.Root", err)
+		return true
 	}
 
 	r := redirects.ParseRedirects(ctx, root)
 
 	rewrittenURL, status, err := r.Rewrite(h.Request.URL)
 	if err != nil {
-		return err
+		if err != redirects.ErrNoRedirect {
+			// We assume that rewrite failure is not fatal
+			// and we only capture the error
+			errortracking.Capture(err, errortracking.WithRequest(h.Request))
+		}
+		return false
 	}
 
 	http.Redirect(h.Writer, h.Request, rewrittenURL.Path, status)
-
-	return nil
+	return true
 }
 
-func (reader *Reader) tryFile(h serving.Handler) error {
+// tryFile returns true if it successfully handled request
+func (reader *Reader) tryFile(h serving.Handler) bool {
 	ctx := h.Request.Context()
 
 	root, err := reader.vfs.Root(ctx, h.LookupPath.Path)
-	if err != nil {
-		return err
+	if vfs.IsNotExist(err) {
+		return false
+	} else if err != nil {
+		httperrors.Serve500WithRequest(h.Writer, h.Request,
+			"vfs.Root", err)
+		return true
 	}
 
 	fullPath, err := reader.resolvePath(ctx, root, h.SubPath)
@@ -80,7 +94,7 @@ func (reader *Reader) tryFile(h serving.Handler) error {
 			// Ensure that there's always "/" at end
 			redirectPath = strings.TrimSuffix(redirectPath, "/") + "/"
 			http.Redirect(h.Writer, h.Request, redirectPath, 302)
-			return nil
+			return true
 		}
 	}
 
@@ -89,7 +103,9 @@ func (reader *Reader) tryFile(h serving.Handler) error {
 	}
 
 	if err != nil {
-		return err
+		// We assume that this is mostly missing file type of the error
+		// and additional handlers should try to process the request
+		return false
 	}
 
 	// Serve status of `_redirects` under `_redirects`
@@ -97,34 +113,42 @@ func (reader *Reader) tryFile(h serving.Handler) error {
 	if fullPath == redirects.ConfigFile {
 		if os.Getenv("FF_ENABLE_REDIRECTS") != "false" {
 			r := redirects.ParseRedirects(ctx, root)
-			return reader.serveRedirectsStatus(h, r)
+			reader.serveRedirectsStatus(h, r)
+			return true
 		}
 
 		h.Writer.WriteHeader(http.StatusForbidden)
-		return nil
+		return true
 	}
 
 	return reader.serveFile(ctx, h.Writer, h.Request, root, fullPath, h.LookupPath.HasAccessControl)
 }
 
-func (reader *Reader) tryNotFound(h serving.Handler) error {
+func (reader *Reader) tryNotFound(h serving.Handler) bool {
 	ctx := h.Request.Context()
 
 	root, err := reader.vfs.Root(ctx, h.LookupPath.Path)
-	if err != nil {
-		return err
+	if vfs.IsNotExist(err) {
+		return false
+	} else if err != nil {
+		httperrors.Serve500WithRequest(h.Writer, h.Request, "vfs.Root", err)
+		return true
 	}
 
 	page404, err := reader.resolvePath(ctx, root, "404.html")
 	if err != nil {
-		return err
+		// We assume that this is mostly missing file type of the error
+		// and additional handlers should try to process the request
+		return false
 	}
 
 	err = reader.serveCustomFile(ctx, h.Writer, h.Request, http.StatusNotFound, root, page404)
 	if err != nil {
-		return err
+		httperrors.Serve500WithRequest(h.Writer, h.Request, "serveCustomFile", err)
+		return true
 	}
-	return nil
+
+	return true
 }
 
 // Resolve the HTTP request to a path on disk, converting requests for
@@ -168,19 +192,21 @@ func (reader *Reader) resolvePath(ctx context.Context, root vfs.Root, subPath ..
 	return fullPath, nil
 }
 
-func (reader *Reader) serveFile(ctx context.Context, w http.ResponseWriter, r *http.Request, root vfs.Root, origPath string, accessControl bool) error {
+func (reader *Reader) serveFile(ctx context.Context, w http.ResponseWriter, r *http.Request, root vfs.Root, origPath string, accessControl bool) bool {
 	fullPath := reader.handleContentEncoding(ctx, w, r, root, origPath)
 
 	file, err := root.Open(ctx, fullPath)
 	if err != nil {
-		return err
+		httperrors.Serve500WithRequest(w, r, "root.Open", err)
+		return true
 	}
 
 	defer file.Close()
 
 	fi, err := root.Lstat(ctx, fullPath)
 	if err != nil {
-		return err
+		httperrors.Serve500WithRequest(w, r, "root.Lstat", err)
+		return true
 	}
 
 	if !accessControl {
@@ -191,7 +217,8 @@ func (reader *Reader) serveFile(ctx context.Context, w http.ResponseWriter, r *h
 
 	contentType, err := reader.detectContentType(ctx, root, origPath)
 	if err != nil {
-		return err
+		httperrors.Serve500WithRequest(w, r, "detectContentType", err)
+		return true
 	}
 
 	w.Header().Set("Content-Type", contentType)
@@ -208,7 +235,7 @@ func (reader *Reader) serveFile(ctx context.Context, w http.ResponseWriter, r *h
 		io.Copy(w, file)
 	}
 
-	return nil
+	return true
 }
 
 func (reader *Reader) serveCustomFile(ctx context.Context, w http.ResponseWriter, r *http.Request, code int, root vfs.Root, origPath string) error {
diff --git a/internal/serving/disk/serving.go b/internal/serving/disk/serving.go
index 11b1689e..30c821ea 100644
--- a/internal/serving/disk/serving.go
+++ b/internal/serving/disk/serving.go
@@ -17,12 +17,12 @@ type Disk struct {
 // ServeFileHTTP serves a file from disk and returns true. It returns false
 // when a file could not been found.
 func (s *Disk) ServeFileHTTP(h serving.Handler) bool {
-	if s.reader.tryFile(h) == nil {
+	if s.reader.tryFile(h) {
 		return true
 	}
 
 	if os.Getenv("FF_ENABLE_REDIRECTS") != "false" {
-		if s.reader.tryRedirects(h) == nil {
+		if s.reader.tryRedirects(h) {
 			return true
 		}
 	}
@@ -32,7 +32,7 @@ func (s *Disk) ServeFileHTTP(h serving.Handler) bool {
 
 // ServeNotFoundHTTP tries to read a custom 404 page
 func (s *Disk) ServeNotFoundHTTP(h serving.Handler) {
-	if s.reader.tryNotFound(h) == nil {
+	if s.reader.tryNotFound(h) {
 		return
 	}
 
diff --git a/internal/serving/disk/zip/serving_test.go b/internal/serving/disk/zip/serving_test.go
index e95432ae..bca2ae51 100644
--- a/internal/serving/disk/zip/serving_test.go
+++ b/internal/serving/disk/zip/serving_test.go
@@ -17,25 +17,40 @@ func TestZip_ServeFileHTTP(t *testing.T) {
 	defer cleanup()
 
 	tests := map[string]struct {
+		vfsPath        string
 		path           string
 		expectedStatus int
 		expectedBody   string
 	}{
 		"accessing /index.html": {
+			vfsPath:        testServerURL + "/public.zip",
 			path:           "/index.html",
 			expectedStatus: http.StatusOK,
 			expectedBody:   "zip.gitlab.io/project/index.html\n",
 		},
 		"accessing /": {
+			vfsPath:        testServerURL + "/public.zip",
 			path:           "/",
 			expectedStatus: http.StatusOK,
 			expectedBody:   "zip.gitlab.io/project/index.html\n",
 		},
 		"accessing without /": {
+			vfsPath:        testServerURL + "/public.zip",
 			path:           "",
 			expectedStatus: http.StatusFound,
 			expectedBody:   `<a href="//zip.gitlab.io/zip/">Found</a>.`,
 		},
+		"accessing archive that is 404": {
+			vfsPath: testServerURL + "/invalid.zip",
+			path:    "/index.html",
+			// we expect the status to not be set
+			expectedStatus: 0,
+		},
+		"accessing archive that is 500": {
+			vfsPath:        testServerURL + "/500",
+			path:           "/index.html",
+			expectedStatus: http.StatusInternalServerError,
+		},
 	}
 
 	s := Instance()
@@ -43,6 +58,7 @@ func TestZip_ServeFileHTTP(t *testing.T) {
 	for name, test := range tests {
 		t.Run(name, func(t *testing.T) {
 			w := httptest.NewRecorder()
+			w.Code = 0 // ensure that code is not set, and it is being set by handler
 			r := httptest.NewRequest("GET", "http://zip.gitlab.io/zip"+test.path, nil)
 
 			handler := serving.Handler{
@@ -50,11 +66,17 @@ func TestZip_ServeFileHTTP(t *testing.T) {
 				Request: r,
 				LookupPath: &serving.LookupPath{
 					Prefix: "/zip/",
-					Path:   testServerURL + "/public.zip",
+					Path:   test.vfsPath,
 				},
 				SubPath: test.path,
 			}
 
+			if test.expectedStatus == 0 {
+				require.False(t, s.ServeFileHTTP(handler))
+				require.Zero(t, w.Code, "we expect status to not be set")
+				return
+			}
+
 			require.True(t, s.ServeFileHTTP(handler))
 
 			resp := w.Result()
@@ -76,9 +98,15 @@ func newZipFileServerURL(t *testing.T, zipFilePath string) (string, func()) {
 
 	chdir := testhelpers.ChdirInPath(t, "../../../../shared/pages", &chdirSet)
 
-	testServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+	m := http.NewServeMux()
+	m.HandleFunc("/public.zip", http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		http.ServeFile(w, r, zipFilePath)
 	}))
+	m.HandleFunc("/500", http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.WriteHeader(http.StatusInternalServerError)
+	}))
+
+	testServer := httptest.NewServer(m)
 
 	return testServer.URL, func() {
 		chdir()