From 9c72216346c9d856669c65ea0cd39220031f77a1 Mon Sep 17 00:00:00 2001 From: Gianmaria Del Monte <39946305+gmgigi96@users.noreply.github.com> Date: Fri, 19 Nov 2021 10:57:48 +0100 Subject: [PATCH] Fix HTTP status code when path is invalid (#2294) --- changelog/unreleased/fix-archiver-errors.md | 7 +++ internal/http/services/archiver/handler.go | 36 +++++++++------- .../services/archiver/manager/archiver.go | 20 +++------ .../archiver/manager/archiver_test.go | 12 +++--- .../http/services/archiver/manager/errors.go | 43 +++++++++++++++++++ pkg/storage/utils/downloader/downloader.go | 7 ++- 6 files changed, 88 insertions(+), 37 deletions(-) create mode 100644 changelog/unreleased/fix-archiver-errors.md create mode 100644 internal/http/services/archiver/manager/errors.go diff --git a/changelog/unreleased/fix-archiver-errors.md b/changelog/unreleased/fix-archiver-errors.md new file mode 100644 index 0000000000..5ee3bd00d8 --- /dev/null +++ b/changelog/unreleased/fix-archiver-errors.md @@ -0,0 +1,7 @@ +Bugfix: Fix HTTP return code when path is invalid + +Before when a path was invalid, the archiver returned a +500 error code. +Now this is fixed and returns a 404 code. + +https://github.com/cs3org/reva/pull/2294 \ No newline at end of file diff --git a/internal/http/services/archiver/handler.go b/internal/http/services/archiver/handler.go index 3d5106e8ec..8249df4d08 100644 --- a/internal/http/services/archiver/handler.go +++ b/internal/http/services/archiver/handler.go @@ -44,7 +44,6 @@ import ( "github.com/gdexlab/go-render/render" ua "github.com/mileusna/useragent" "github.com/mitchellh/mapstructure" - "github.com/pkg/errors" "github.com/rs/zerolog" ) @@ -193,6 +192,23 @@ func (s *svc) allAllowed(paths []string) error { return nil } +func (s *svc) writeHTTPError(rw http.ResponseWriter, err error) { + s.log.Error().Msg(err.Error()) + + switch err.(type) { + case errtypes.NotFound: + rw.WriteHeader(http.StatusNotFound) + case manager.ErrMaxSize, manager.ErrMaxFileCount: + rw.WriteHeader(http.StatusRequestEntityTooLarge) + case errtypes.BadRequest: + rw.WriteHeader(http.StatusBadRequest) + default: + rw.WriteHeader(http.StatusInternalServerError) + } + + _, _ = rw.Write([]byte(err.Error())) +} + func (s *svc) Handler() http.Handler { return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { // get the paths and/or the resources id from the query @@ -210,9 +226,7 @@ func (s *svc) Handler() http.Handler { files, err := s.getFiles(ctx, paths, ids) if err != nil { - s.log.Error().Msg(err.Error()) - rw.WriteHeader(http.StatusBadRequest) - _, _ = rw.Write([]byte(err.Error())) + s.writeHTTPError(rw, err) return } @@ -221,9 +235,7 @@ func (s *svc) Handler() http.Handler { MaxSize: s.config.MaxSize, }) if err != nil { - s.log.Error().Msg(err.Error()) - rw.WriteHeader(http.StatusInternalServerError) - _, _ = rw.Write([]byte(err.Error())) + s.writeHTTPError(rw, err) return } @@ -248,14 +260,8 @@ func (s *svc) Handler() http.Handler { err = arch.CreateTar(ctx, rw) } - if err == manager.ErrMaxFileCount || err == manager.ErrMaxSize { - s.log.Error().Msg(err.Error()) - rw.WriteHeader(http.StatusRequestEntityTooLarge) - return - } if err != nil { - s.log.Error().Msg(err.Error()) - rw.WriteHeader(http.StatusInternalServerError) + s.writeHTTPError(rw, err) return } @@ -277,7 +283,7 @@ func (s *svc) Unprotected() []string { func decodeResourceID(encodedID string) (string, string, error) { decodedID, err := base64.URLEncoding.DecodeString(encodedID) if err != nil { - return "", "", errors.Wrap(err, "resource ID does not follow the required format") + return "", "", errtypes.BadRequest("resource ID does not follow the required format") } parts := strings.Split(string(decodedID), ":") diff --git a/internal/http/services/archiver/manager/archiver.go b/internal/http/services/archiver/manager/archiver.go index a410fe63b0..7c90b6a459 100644 --- a/internal/http/services/archiver/manager/archiver.go +++ b/internal/http/services/archiver/manager/archiver.go @@ -28,20 +28,10 @@ import ( "time" provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1" - "github.com/cs3org/reva/pkg/errtypes" "github.com/cs3org/reva/pkg/storage/utils/downloader" "github.com/cs3org/reva/pkg/storage/utils/walker" ) -const ( - // ErrMaxFileCount is the error returned when the max files count specified in the config has reached - ErrMaxFileCount = errtypes.InternalError("reached max files count") - // ErrMaxSize is the error returned when the max total files size specified in the config has reached - ErrMaxSize = errtypes.InternalError("reached max total files size") - // ErrEmptyList is the error returned when an empty list is passed when an archiver is created - ErrEmptyList = errtypes.BadRequest("list of files to archive empty") -) - // Config is the config for the Archiver type Config struct { MaxNumFiles int64 @@ -60,7 +50,7 @@ type Archiver struct { // NewArchiver creates a new archiver able to create an archive containing the files in the list func NewArchiver(files []string, w walker.Walker, d downloader.Downloader, config Config) (*Archiver, error) { if len(files) == 0 { - return nil, ErrEmptyList + return nil, ErrEmptyList{} } dir := getDeepestCommonDir(files) @@ -140,7 +130,7 @@ func (a *Archiver) CreateTar(ctx context.Context, dst io.Writer) error { filesCount++ if filesCount > a.config.MaxNumFiles { - return ErrMaxFileCount + return ErrMaxFileCount{} } if !isDir { @@ -149,7 +139,7 @@ func (a *Archiver) CreateTar(ctx context.Context, dst io.Writer) error { // count the files not only once sizeFiles += int64(info.Size) if sizeFiles > a.config.MaxSize { - return ErrMaxSize + return ErrMaxSize{} } } @@ -214,7 +204,7 @@ func (a *Archiver) CreateZip(ctx context.Context, dst io.Writer) error { filesCount++ if filesCount > a.config.MaxNumFiles { - return ErrMaxFileCount + return ErrMaxFileCount{} } if !isDir { @@ -223,7 +213,7 @@ func (a *Archiver) CreateZip(ctx context.Context, dst io.Writer) error { // count the files not only once sizeFiles += int64(info.Size) if sizeFiles > a.config.MaxSize { - return ErrMaxSize + return ErrMaxSize{} } } diff --git a/internal/http/services/archiver/manager/archiver_test.go b/internal/http/services/archiver/manager/archiver_test.go index 05ff8f2a54..fd102c5549 100644 --- a/internal/http/services/archiver/manager/archiver_test.go +++ b/internal/http/services/archiver/manager/archiver_test.go @@ -207,7 +207,7 @@ func TestCreateTar(t *testing.T) { }, files: []string{"foo"}, expected: nil, - err: ErrMaxFileCount, + err: ErrMaxFileCount{}, }, { name: "one file - error max size reached", @@ -222,7 +222,7 @@ func TestCreateTar(t *testing.T) { }, files: []string{"foo"}, expected: nil, - err: ErrMaxSize, + err: ErrMaxSize{}, }, { name: "one folder empty", @@ -250,7 +250,7 @@ func TestCreateTar(t *testing.T) { }, files: []string{"foo"}, expected: nil, - err: ErrMaxFileCount, + err: ErrMaxFileCount{}, }, { name: "one folder - one file in", @@ -655,7 +655,7 @@ func TestCreateZip(t *testing.T) { }, files: []string{"foo"}, expected: nil, - err: ErrMaxFileCount, + err: ErrMaxFileCount{}, }, { name: "one file - error max size reached", @@ -670,7 +670,7 @@ func TestCreateZip(t *testing.T) { }, files: []string{"foo"}, expected: nil, - err: ErrMaxSize, + err: ErrMaxSize{}, }, { name: "one folder empty", @@ -698,7 +698,7 @@ func TestCreateZip(t *testing.T) { }, files: []string{"foo"}, expected: nil, - err: ErrMaxFileCount, + err: ErrMaxFileCount{}, }, { name: "one folder - one file in", diff --git a/internal/http/services/archiver/manager/errors.go b/internal/http/services/archiver/manager/errors.go new file mode 100644 index 0000000000..178e76b25d --- /dev/null +++ b/internal/http/services/archiver/manager/errors.go @@ -0,0 +1,43 @@ +// Copyright 2018-2021 CERN +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// +// In applying this license, CERN does not waive the privileges and immunities +// granted to it by virtue of its status as an Intergovernmental Organization +// or submit itself to any jurisdiction. + +package manager + +// ErrMaxFileCount is the error returned when the max files count specified in the config has reached +type ErrMaxFileCount struct{} + +// ErrMaxSize is the error returned when the max total files size specified in the config has reached +type ErrMaxSize struct{} + +// ErrEmptyList is the error returned when an empty list is passed when an archiver is created +type ErrEmptyList struct{} + +// Error returns the string error msg for ErrMaxFileCount +func (ErrMaxFileCount) Error() string { + return "reached max files count" +} + +// Error returns the string error msg for ErrMaxSize +func (ErrMaxSize) Error() string { + return "reached max total files size" +} + +// Error returns the string error msg for ErrEmptyList +func (ErrEmptyList) Error() string { + return "list of files to archive empty" +} diff --git a/pkg/storage/utils/downloader/downloader.go b/pkg/storage/utils/downloader/downloader.go index 728393e1d2..b004032982 100644 --- a/pkg/storage/utils/downloader/downloader.go +++ b/pkg/storage/utils/downloader/downloader.go @@ -93,7 +93,12 @@ func (r *revaDownloader) Download(ctx context.Context, path string, dst io.Write defer httpRes.Body.Close() if httpRes.StatusCode != http.StatusOK { - return errtypes.InternalError(httpRes.Status) + switch httpRes.StatusCode { + case http.StatusNotFound: + return errtypes.NotFound(path) + default: + return errtypes.InternalError(httpRes.Status) + } } _, err = io.Copy(dst, httpRes.Body)