From f1834163ec7bf0d2efd507eeb1cd52975e972c57 Mon Sep 17 00:00:00 2001 From: Emil Tullstedt Date: Wed, 15 Jun 2022 15:11:36 +0200 Subject: [PATCH] ShortURL: Use new Error type (#50859) --- pkg/api/short_url.go | 20 +++---------------- pkg/models/shorturl.go | 9 +++++++-- pkg/services/shorturls/short_url_service.go | 19 ++++++++++++++---- .../shorturls/short_url_service_test.go | 5 +++-- pkg/util/errutil/errors.go | 12 +++++++++++ 5 files changed, 40 insertions(+), 25 deletions(-) diff --git a/pkg/api/short_url.go b/pkg/api/short_url.go index 6e9d1614d1f..a7d562ef9c2 100644 --- a/pkg/api/short_url.go +++ b/pkg/api/short_url.go @@ -1,10 +1,8 @@ package api import ( - "errors" "fmt" "net/http" - "path" "strings" "github.com/grafana/grafana/pkg/api/dtos" @@ -19,24 +17,12 @@ import ( func (hs *HTTPServer) createShortURL(c *models.ReqContext) response.Response { cmd := dtos.CreateShortURLCmd{} if err := web.Bind(c.Req, &cmd); err != nil { - return response.Error(http.StatusBadRequest, "bad request data", err) + return response.Err(models.ErrShortURLBadRequest.Errorf("bad request data: %w", err)) } hs.log.Debug("Received request to create short URL", "path", cmd.Path) - - cmd.Path = strings.TrimSpace(cmd.Path) - - if path.IsAbs(cmd.Path) { - hs.log.Error("Invalid short URL path", "path", cmd.Path) - return response.Error(400, "Path should be relative", nil) - } - if strings.Contains(cmd.Path, "../") { - hs.log.Error("Invalid short URL path", "path", cmd.Path) - return response.Error(400, "Invalid path", nil) - } - shortURL, err := hs.ShortURLService.CreateShortURL(c.Req.Context(), c.SignedInUser, cmd.Path) if err != nil { - return response.Error(500, "Failed to create short URL", err) + return response.Err(err) } url := fmt.Sprintf("%s/goto/%s?orgId=%d", strings.TrimSuffix(setting.AppUrl, "/"), shortURL.Uid, c.OrgId) @@ -59,7 +45,7 @@ func (hs *HTTPServer) redirectFromShortURL(c *models.ReqContext) { shortURL, err := hs.ShortURLService.GetShortURLByUID(c.Req.Context(), c.SignedInUser, shortURLUID) if err != nil { - if errors.Is(err, models.ErrShortURLNotFound) { + if models.ErrShortURLNotFound.Is(err) { hs.log.Debug("Not redirecting short URL since not found") return } diff --git a/pkg/models/shorturl.go b/pkg/models/shorturl.go index 480f09ca058..1d008bb5b35 100644 --- a/pkg/models/shorturl.go +++ b/pkg/models/shorturl.go @@ -1,12 +1,17 @@ package models import ( - "errors" "time" + + "github.com/grafana/grafana/pkg/util/errutil" ) var ( - ErrShortURLNotFound = errors.New("short URL not found") + ErrShortURLBadRequest = errutil.NewBase(errutil.StatusBadRequest, "shorturl.bad-request") + ErrShortURLNotFound = errutil.NewBase(errutil.StatusNotFound, "shorturl.not-found") + ErrShortURLAbsolutePath = errutil.NewBase(errutil.StatusValidationFailed, "shorturl.absolute-path", errutil.WithPublicMessage("Path should be relative")) + ErrShortURLInvalidPath = errutil.NewBase(errutil.StatusValidationFailed, "shorturl.invalid-path", errutil.WithPublicMessage("Invalid short URL path")) + ErrShortURLInternal = errutil.NewBase(errutil.StatusInternal, "shorturl.internal") ) type ShortUrl struct { diff --git a/pkg/services/shorturls/short_url_service.go b/pkg/services/shorturls/short_url_service.go index 09470283835..f2b48d708e4 100644 --- a/pkg/services/shorturls/short_url_service.go +++ b/pkg/services/shorturls/short_url_service.go @@ -2,6 +2,8 @@ package shorturls import ( "context" + "path" + "strings" "time" "github.com/grafana/grafana/pkg/models" @@ -36,7 +38,7 @@ func (s ShortURLService) GetShortURLByUID(ctx context.Context, user *models.Sign return err } if !exists { - return models.ErrShortURLNotFound + return models.ErrShortURLNotFound.Errorf("short URL not found") } return nil @@ -60,12 +62,21 @@ func (s ShortURLService) UpdateLastSeenAt(ctx context.Context, shortURL *models. }) } -func (s ShortURLService) CreateShortURL(ctx context.Context, user *models.SignedInUser, path string) (*models.ShortUrl, error) { +func (s ShortURLService) CreateShortURL(ctx context.Context, user *models.SignedInUser, relPath string) (*models.ShortUrl, error) { + relPath = strings.TrimSpace(relPath) + + if path.IsAbs(relPath) { + return nil, models.ErrShortURLAbsolutePath.Errorf("expected relative path: %s", relPath) + } + if strings.Contains(relPath, "../") { + return nil, models.ErrShortURLInvalidPath.Errorf("path cannot contain '../': %s", relPath) + } + now := time.Now().Unix() shortURL := models.ShortUrl{ OrgId: user.OrgId, Uid: util.GenerateShortUID(), - Path: path, + Path: relPath, CreatedBy: user.UserId, CreatedAt: now, } @@ -75,7 +86,7 @@ func (s ShortURLService) CreateShortURL(ctx context.Context, user *models.Signed return err }) if err != nil { - return nil, err + return nil, models.ErrShortURLInternal.Errorf("failed to insert shorturl: %w", err) } return &shortURL, nil diff --git a/pkg/services/shorturls/short_url_service_test.go b/pkg/services/shorturls/short_url_service_test.go index 9a6c66d388a..61278d9d3ac 100644 --- a/pkg/services/shorturls/short_url_service_test.go +++ b/pkg/services/shorturls/short_url_service_test.go @@ -5,9 +5,10 @@ import ( "testing" "time" + "github.com/stretchr/testify/require" + "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/services/sqlstore" - "github.com/stretchr/testify/require" ) func TestShortURLService(t *testing.T) { @@ -79,7 +80,7 @@ func TestShortURLService(t *testing.T) { shortURL, err := service.GetShortURLByUID(context.Background(), user, "testnotfounduid") require.Error(t, err) - require.Equal(t, models.ErrShortURLNotFound, err) + require.True(t, models.ErrShortURLNotFound.Is(err)) require.Nil(t, shortURL) }) } diff --git a/pkg/util/errutil/errors.go b/pkg/util/errutil/errors.go index 555bb639d9d..afdba2604f3 100644 --- a/pkg/util/errutil/errors.go +++ b/pkg/util/errutil/errors.go @@ -80,6 +80,18 @@ func (b Base) Errorf(format string, args ...interface{}) Error { } } +// Is validates that an Error has the same reason and messageID as the +// Base. +func (b Base) Is(err error) bool { + gfErr := Error{} + ok := errors.As(err, &gfErr) + if !ok { + return false + } + + return b.reason.Status() == gfErr.Reason.Status() && b.messageID == gfErr.MessageID +} + // Error is the error type for errors within Grafana, extending // the Go error type with Grafana specific metadata to reduce // boilerplate error handling for status codes and internationalization