diff options
author | Kamil TrzciĆski <ayufan@ayufan.eu> | 2020-10-27 06:49:07 +0300 |
---|---|---|
committer | Vladimir Shushlin <vshushlin@gitlab.com> | 2020-10-27 06:49:07 +0300 |
commit | 5be346a94902c7147f3b1571e6ebb2c1f4e98eb8 (patch) | |
tree | 1e6328dd475c3578a3910181d6e2a26f20285bef /internal/serving | |
parent | b382faeec6491bb544d33549570610a476f597b7 (diff) |
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.
Diffstat (limited to 'internal/serving')
-rw-r--r-- | internal/serving/disk/local/serving_test.go | 84 | ||||
-rw-r--r-- | internal/serving/disk/reader.go | 81 | ||||
-rw-r--r-- | internal/serving/disk/serving.go | 6 | ||||
-rw-r--r-- | internal/serving/disk/zip/serving_test.go | 32 |
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() |