enhancement(graph): check the received bytes content type for profile photos

Author: fschade

services/graph/pkg/service/v0/api_users_user_profile_photo.go

@@ -3,8 +3,10 @@ package svc
 import (
 	"context"
 	"errors"
+	"fmt"
 	"io"
 	"net/http"
+	"strings"
 
 	"github.com/go-chi/render"
 	"github.com/opencloud-eu/reva/v2/pkg/storage/utils/metadata"
@@ -33,6 +35,12 @@ var (
 
 	// ErrNoBytes is returned when no bytes are found
 	ErrNoBytes = errors.New("no bytes")
+
+	// ErrInvalidContentType is returned when the content type is invalid
+	ErrInvalidContentType = errors.New("invalid content type")
+
+	// ErrMissingArgument is returned when a required argument is missing
+	ErrMissingArgument = errors.New("required argument is missing")
 )
 
 // UsersUserProfilePhotoService is the implementation of the UsersUserProfilePhotoProvider interface
@@ -53,12 +61,7 @@ func NewUsersUserProfilePhotoService(storage metadata.Storage) (UsersUserProfile
 
 // GetPhoto retrieves the requested photo
 func (s UsersUserProfilePhotoService) GetPhoto(ctx context.Context, id string) ([]byte, error) {
-	photo, err := s.storage.SimpleDownload(ctx, id)
-	if err != nil {
-		return nil, err
-	}
-
-	return photo, nil
+	return s.storage.SimpleDownload(ctx, id)
 }
 
 // DeletePhoto deletes the requested photo
@@ -68,6 +71,10 @@ func (s UsersUserProfilePhotoService) DeletePhoto(ctx context.Context, id string
 
 // UpdatePhoto updates the requested photo
 func (s UsersUserProfilePhotoService) UpdatePhoto(ctx context.Context, id string, r io.Reader) error {
+	if id == "" {
+		return fmt.Errorf("%w: %s", ErrMissingArgument, "id")
+	}
+
 	photo, err := io.ReadAll(r)
 	if err != nil {
 		return err
@@ -77,6 +84,11 @@ func (s UsersUserProfilePhotoService) UpdatePhoto(ctx context.Context, id string
 		return ErrNoBytes
 	}
 
+	contentType := http.DetectContentType(photo)
+	if !strings.HasPrefix(contentType, "image/") {
+		return fmt.Errorf("%w: %s", ErrInvalidContentType, contentType)
+	}
+
 	return s.storage.SimpleUpload(ctx, id, photo)
 }
 

services/graph/pkg/service/v0/api_users_user_profile_photo_test.go

@@ -1,6 +1,7 @@
 package svc_test
 
 import (
+	"bytes"
 	"context"
 	"errors"
 	"io"
@@ -17,25 +18,50 @@ import (
 	svc "github.com/opencloud-eu/opencloud/services/graph/pkg/service/v0"
 )
 
+func TestNewUsersUserProfilePhotoService(t *testing.T) {
+	storage := mocks.NewStorage(t)
+	storage.EXPECT().Init(mock.Anything, mock.Anything).RunAndReturn(func(ctx context.Context, id string) error { return nil })
+
+	service, err := svc.NewUsersUserProfilePhotoService(storage)
+	assert.NoError(t, err)
+
+	t.Run("UpdatePhoto", func(t *testing.T) {
+		t.Run("reports an error if id is empty", func(t *testing.T) {
+			err := service.UpdatePhoto(context.Background(), "", bytes.NewReader([]byte{}))
+			assert.ErrorIs(t, err, svc.ErrMissingArgument)
+		})
+
+		t.Run("reports an error if the reader does not contain any bytes", func(t *testing.T) {
+			err := service.UpdatePhoto(context.Background(), "123", bytes.NewReader([]byte{}))
+			assert.ErrorIs(t, err, svc.ErrNoBytes)
+		})
+
+		t.Run("reports an error if data is not an image", func(t *testing.T) {
+			err := service.UpdatePhoto(context.Background(), "234", bytes.NewReader([]byte("not an image")))
+			assert.ErrorIs(t, err, svc.ErrInvalidContentType)
+		})
+	})
+}
+
 func TestUsersUserProfilePhotoApi(t *testing.T) {
 	var (
-		usersUserProfilePhotoProvider = mocks.NewUsersUserProfilePhotoProvider(t)
-		dummyDataProvider             = func(w http.ResponseWriter, r *http.Request) (string, bool) {
+		serviceProvider = mocks.NewUsersUserProfilePhotoProvider(t)
+		dataProvider    = func(w http.ResponseWriter, r *http.Request) (string, bool) {
 			return "123", true
 		}
 	)
 
-	api, err := svc.NewUsersUserProfilePhotoApi(usersUserProfilePhotoProvider, log.NopLogger())
+	api, err := svc.NewUsersUserProfilePhotoApi(serviceProvider, log.NopLogger())
 	assert.NoError(t, err)
 
 	t.Run("GetProfilePhoto", func(t *testing.T) {
 		r := httptest.NewRequest(http.MethodGet, "/", nil)
-		ep := api.GetProfilePhoto(dummyDataProvider)
+		ep := api.GetProfilePhoto(dataProvider)
 
 		t.Run("fails if photo provider errors", func(t *testing.T) {
 			w := httptest.NewRecorder()
 
-			usersUserProfilePhotoProvider.EXPECT().GetPhoto(mock.Anything, mock.Anything).RunAndReturn(func(ctx context.Context, s string) ([]byte, error) {
+			serviceProvider.EXPECT().GetPhoto(mock.Anything, mock.Anything).RunAndReturn(func(ctx context.Context, s string) ([]byte, error) {
 				return nil, errors.New("any")
 			}).Once()
 
@@ -47,7 +73,7 @@ func TestUsersUserProfilePhotoApi(t *testing.T) {
 		t.Run("successfully returns the requested photo", func(t *testing.T) {
 			w := httptest.NewRecorder()
 
-			usersUserProfilePhotoProvider.EXPECT().GetPhoto(mock.Anything, mock.Anything).RunAndReturn(func(ctx context.Context, s string) ([]byte, error) {
+			serviceProvider.EXPECT().GetPhoto(mock.Anything, mock.Anything).RunAndReturn(func(ctx context.Context, s string) ([]byte, error) {
 				return []byte("photo"), nil
 			}).Once()
 
@@ -60,12 +86,12 @@ func TestUsersUserProfilePhotoApi(t *testing.T) {
 
 	t.Run("DeleteProfilePhoto", func(t *testing.T) {
 		r := httptest.NewRequest(http.MethodDelete, "/", nil)
-		ep := api.DeleteProfilePhoto(dummyDataProvider)
+		ep := api.DeleteProfilePhoto(dataProvider)
 
 		t.Run("fails if photo provider errors", func(t *testing.T) {
 			w := httptest.NewRecorder()
 
-			usersUserProfilePhotoProvider.EXPECT().DeletePhoto(mock.Anything, mock.Anything).RunAndReturn(func(ctx context.Context, s string) error {
+			serviceProvider.EXPECT().DeletePhoto(mock.Anything, mock.Anything).RunAndReturn(func(ctx context.Context, s string) error {
 				return errors.New("any")
 			}).Once()
 
@@ -77,7 +103,7 @@ func TestUsersUserProfilePhotoApi(t *testing.T) {
 		t.Run("successfully deletes the requested photo", func(t *testing.T) {
 			w := httptest.NewRecorder()
 
-			usersUserProfilePhotoProvider.EXPECT().DeletePhoto(mock.Anything, mock.Anything).RunAndReturn(func(ctx context.Context, s string) error {
+			serviceProvider.EXPECT().DeletePhoto(mock.Anything, mock.Anything).RunAndReturn(func(ctx context.Context, s string) error {
 				return nil
 			}).Once()
 
@@ -89,12 +115,12 @@ func TestUsersUserProfilePhotoApi(t *testing.T) {
 
 	t.Run("UpsertProfilePhoto", func(t *testing.T) {
 		r := httptest.NewRequest(http.MethodPut, "/", strings.NewReader("body"))
-		ep := api.UpsertProfilePhoto(dummyDataProvider)
+		ep := api.UpsertProfilePhoto(dataProvider)
 
 		t.Run("fails if photo provider errors", func(t *testing.T) {
 			w := httptest.NewRecorder()
 
-			usersUserProfilePhotoProvider.EXPECT().UpdatePhoto(mock.Anything, mock.Anything, mock.Anything).RunAndReturn(func(ctx context.Context, s string, r io.Reader) error {
+			serviceProvider.EXPECT().UpdatePhoto(mock.Anything, mock.Anything, mock.Anything).RunAndReturn(func(ctx context.Context, s string, r io.Reader) error {
 				return errors.New("any")
 			}).Once()
 
@@ -106,7 +132,7 @@ func TestUsersUserProfilePhotoApi(t *testing.T) {
 		t.Run("successfully upserts the photo", func(t *testing.T) {
 			w := httptest.NewRecorder()
 
-			usersUserProfilePhotoProvider.EXPECT().UpdatePhoto(mock.Anything, mock.Anything, mock.Anything).RunAndReturn(func(ctx context.Context, s string, r io.Reader) error {
+			serviceProvider.EXPECT().UpdatePhoto(mock.Anything, mock.Anything, mock.Anything).RunAndReturn(func(ctx context.Context, s string, r io.Reader) error {
 				return nil
 			}).Once()