From d4cd47e503bd4955039037314f88dea34730d95b Mon Sep 17 00:00:00 2001 From: Konrad Wojas Date: Sun, 31 May 2020 21:36:39 +0800 Subject: [PATCH] Minor cleanup and fixes - Do not allow '.' as path component, because it undermines depth checks, and add tests - Fix GiB reporting - Fix metrics label - Helper function for http errors --- handlers.go | 29 ++++++++++++++++++++--------- handlers_test.go | 8 ++++++++ metrics.go | 2 +- mux.go | 6 +++++- repo/repo.go | 14 +++++++------- 5 files changed, 41 insertions(+), 18 deletions(-) diff --git a/handlers.go b/handlers.go index d1ee6f4..8645905 100644 --- a/handlers.go +++ b/handlers.go @@ -32,30 +32,41 @@ type Server struct { quotaManager *quota.Manager } +// MaxFolderDepth is the maxDepth param passed to splitURLPath. +// A max depth of 2 mean that we accept folders like: '/', '/foo' and '/foo/bar' +// TODO: Move to a Server option +const MaxFolderDepth = 2 + +// httpDefaultError write a HTTP error with the default description +func httpDefaultError(w http.ResponseWriter, code int) { + http.Error(w, http.StatusText(code), code) +} + // ServeHTTP makes this server an http.Handler. It handlers the administrative // part of the request (figuring out the filesystem location, performing // authentication, etc) and then passes it on to repo.Handler for actual // REST API processing. func (s *Server) ServeHTTP(w http.ResponseWriter, r *http.Request) { - // First of all, check auth + // First of all, check auth (will always pass if NoAuth is set) username, ok := s.checkAuth(r) if !ok { - http.Error(w, http.StatusText(http.StatusUnauthorized), http.StatusUnauthorized) + httpDefaultError(w, http.StatusUnauthorized) + return } // Perform the path parsing to determine the repo folder and remainder for the - // repo handler - folderPath, remainder := splitURLPath(r.URL.Path, 2) // FIXME: configurable + // repo handler. + folderPath, remainder := splitURLPath(r.URL.Path, MaxFolderDepth) if !folderPathValid(folderPath) { log.Printf("Invalid request path: %s", r.URL.Path) - http.Error(w, http.StatusText(http.StatusNotFound), http.StatusNotFound) + httpDefaultError(w, http.StatusNotFound) return } // Check if the current user is allowed to access this path if !s.NoAuth && s.PrivateRepos { if len(folderPath) == 0 || folderPath[0] != username { - http.Error(w, http.StatusText(http.StatusUnauthorized), http.StatusUnauthorized) + httpDefaultError(w, http.StatusUnauthorized) return } } @@ -65,7 +76,7 @@ func (s *Server) ServeHTTP(w http.ResponseWriter, r *http.Request) { if err != nil { // We did not expect an error at this stage, because we just checked the path log.Printf("Unexpected join error for path %q", r.URL.Path) - http.Error(w, http.StatusText(http.StatusBadRequest), http.StatusBadRequest) + httpDefaultError(w, http.StatusNotFound) return } @@ -81,7 +92,7 @@ func (s *Server) ServeHTTP(w http.ResponseWriter, r *http.Request) { repoHandler, err := repo.New(fsPath, opt) if err != nil { log.Printf("repo.New error: %v", err) - http.Error(w, http.StatusText(http.StatusInternalServerError), http.StatusInternalServerError) + httpDefaultError(w, http.StatusInternalServerError) return } r.URL.Path = remainder // strip folderPath for next handler @@ -170,7 +181,7 @@ func splitURLPath(urlPath string, maxDepth int) (folderPath []string, remainder // safe. func folderPathValid(folderPath []string) bool { for _, name := range folderPath { - if name == "" || name == ".." || !valid(name) { + if name == "" || name == ".." || name == "." || !valid(name) { return false } } diff --git a/handlers_test.go b/handlers_test.go index b3c799f..4d07fbb 100644 --- a/handlers_test.go +++ b/handlers_test.go @@ -248,10 +248,18 @@ func TestSplitURLPath(t *testing.T) { {"/foo/bar/zzz/locks/0123", 2, []string{"foo", "bar"}, "/zzz/locks/0123"}, {"/foo/bar/zzz/locks/0123", 3, []string{"foo", "bar", "zzz"}, "/locks/0123"}, {"/foo/bar/locks/", 2, []string{"foo", "bar"}, "/locks/"}, + {"/foo/locks/", 2, []string{"foo"}, "/locks/"}, + {"/foo/data/", 2, []string{"foo"}, "/data/"}, + {"/foo/index/", 2, []string{"foo"}, "/index/"}, + {"/foo/keys/", 2, []string{"foo"}, "/keys/"}, + {"/foo/snapshots/", 2, []string{"foo"}, "/snapshots/"}, + {"/foo/config", 2, []string{"foo"}, "/config"}, + {"/foo/", 2, []string{"foo"}, "/"}, {"/foo/bar/", 2, []string{"foo", "bar"}, "/"}, {"/foo/bar", 2, []string{"foo"}, "/bar"}, {"/locks/", 2, nil, "/locks/"}, // This function only splits, it does not check the path components! + {"/././locks/", 2, []string{"..", ".."}, "/locks/"}, {"/../../locks/", 2, []string{"..", ".."}, "/locks/"}, {"///locks/", 2, []string{"", ""}, "/locks/"}, {"////locks/", 2, []string{"", ""}, "//locks/"}, diff --git a/metrics.go b/metrics.go index fa64f0b..2419a60 100644 --- a/metrics.go +++ b/metrics.go @@ -63,7 +63,7 @@ func makeBlobMetricFunc(username string, folderPath []string) repo.BlobMetricFun var f repo.BlobMetricFunc = func(objectType string, operation repo.BlobOperation, nBytes uint64) { labels := prometheus.Labels{ "user": username, - "repo": strings.Join(folderPath, ""), + "repo": strings.Join(folderPath, "/"), "type": objectType, } switch operation { diff --git a/mux.go b/mux.go index 9db037d..694056c 100644 --- a/mux.go +++ b/mux.go @@ -12,6 +12,10 @@ import ( "github.com/restic/rest-server/quota" ) +const ( + GiB = 1024 * 1024 * 1024 +) + func (s *Server) debugHandler(next http.Handler) http.Handler { return http.HandlerFunc( func(w http.ResponseWriter, r *http.Request) { @@ -58,7 +62,7 @@ func NewHandler(server *Server) (http.Handler, error) { return nil, err } server.quotaManager = qm - log.Printf("Quota initialized, currenly using %.2f GiB", float64(qm.SpaceUsed()/1024/1024)) + log.Printf("Quota initialized, currenly using %.2f GiB", float64(qm.SpaceUsed())/GiB) } mux := http.NewServeMux() diff --git a/repo/repo.go b/repo/repo.go index e2591c4..c43dcb8 100644 --- a/repo/repo.go +++ b/repo/repo.go @@ -165,14 +165,14 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { // getObject parses the URL path and returns the objectType and objectID, // if any. The objectID is optional. func (h *Handler) getObject(urlPath string) (objectType, objectID string) { - if m := BlobPathRE.FindStringSubmatch(urlPath); len(m) > 0 { - if len(m) == 2 || m[2] == "" { - return m[1], "" - } - return m[1], m[2] - } else { - return "", "" + m := BlobPathRE.FindStringSubmatch(urlPath) + if len(m) == 0 { + return "", "" // no match } + if len(m) == 2 || m[2] == "" { + return m[1], "" // no objectID + } + return m[1], m[2] } // getSubPath returns the path for a file or subdir in the root of the repo.