From 48d2af5561f3b2fb1064899b63013266ebddc6f9 Mon Sep 17 00:00:00 2001 From: Bram Hagens Date: Sat, 7 Mar 2026 20:53:23 +0100 Subject: [PATCH] fix: skip repo avatar upload when no file is selected (#11335) Submitting the repo avatar form without selecting a file shows a raw Go error: `Avatar.Open: open : no such file or directory.`. The existing `nil` check does not prevent this from happening. The user avatar handler already guards against this same problem with [`form.Avatar != nil && form.Avatar.Filename != ""`](https://codeberg.org/forgejo/forgejo/src/commit/e1cecbd276841a5762ad034a920be8c2732e295f/routers/web/user/setting/profile.go#L141), I've done the same for the repo avatar handler. Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/11335 Reviewed-by: Gusted Co-authored-by: Bram Hagens Co-committed-by: Bram Hagens --- routers/web/repo/setting/avatar.go | 40 ++---------- routers/web/user/setting/profile.go | 30 ++------- services/forms/avatar.go | 45 +++++++++++++ tests/integration/repo_avatar_test.go | 94 +++++++++++++++++++++++++++ 4 files changed, 151 insertions(+), 58 deletions(-) create mode 100644 services/forms/avatar.go create mode 100644 tests/integration/repo_avatar_test.go diff --git a/routers/web/repo/setting/avatar.go b/routers/web/repo/setting/avatar.go index 20e211316d..be45bb8e7b 100644 --- a/routers/web/repo/setting/avatar.go +++ b/routers/web/repo/setting/avatar.go @@ -4,13 +4,8 @@ package setting import ( - "errors" "fmt" - "io" - "forgejo.org/modules/log" - "forgejo.org/modules/setting" - "forgejo.org/modules/typesniffer" "forgejo.org/modules/web" "forgejo.org/services/context" "forgejo.org/services/forms" @@ -19,37 +14,14 @@ import ( // UpdateAvatarSetting update repo's avatar func UpdateAvatarSetting(ctx *context.Context, form forms.AvatarForm) error { - ctxRepo := ctx.Repo.Repository - - if form.Avatar == nil { - // No avatar is uploaded and we not removing it here. - // No random avatar generated here. - // Just exit, no action. - if ctxRepo.CustomAvatarRelativePath() == "" { - log.Trace("No avatar was uploaded for repo: %d. Default icon will appear instead.", ctxRepo.ID) - } + data, err := forms.ReadAvatar(form.Avatar, ctx.Locale) + if err != nil { + return err + } + if data == nil { return nil } - - r, err := form.Avatar.Open() - if err != nil { - return fmt.Errorf("Avatar.Open: %w", err) - } - defer r.Close() - - if form.Avatar.Size > setting.Avatar.MaxFileSize { - return errors.New(ctx.Locale.TrString("settings.uploaded_avatar_is_too_big", form.Avatar.Size/1024, setting.Avatar.MaxFileSize/1024)) - } - - data, err := io.ReadAll(r) - if err != nil { - return fmt.Errorf("io.ReadAll: %w", err) - } - st := typesniffer.DetectContentType(data, "") - if !st.IsImage() || st.IsSvgImage() { - return errors.New(ctx.Locale.TrString("settings.uploaded_avatar_not_a_image")) - } - if err = repo_service.UploadAvatar(ctx, ctxRepo, data); err != nil { + if err := repo_service.UploadAvatar(ctx, ctx.Repo.Repository, data); err != nil { return fmt.Errorf("UploadAvatar: %w", err) } return nil diff --git a/routers/web/user/setting/profile.go b/routers/web/user/setting/profile.go index 466fbff435..0190e5bdac 100644 --- a/routers/web/user/setting/profile.go +++ b/routers/web/user/setting/profile.go @@ -5,9 +5,7 @@ package setting import ( - "errors" "fmt" - "io" "math/big" "net/http" "os" @@ -25,7 +23,6 @@ import ( "forgejo.org/modules/optional" "forgejo.org/modules/setting" "forgejo.org/modules/translation" - "forgejo.org/modules/typesniffer" "forgejo.org/modules/util" "forgejo.org/modules/web" "forgejo.org/modules/web/middleware" @@ -138,27 +135,12 @@ func UpdateAvatarSetting(ctx *context.Context, form *forms.AvatarForm, ctxUser * ctxUser.AvatarEmail = form.Gravatar } - if form.Avatar != nil && form.Avatar.Filename != "" { - fr, err := form.Avatar.Open() - if err != nil { - return fmt.Errorf("Avatar.Open: %w", err) - } - defer fr.Close() - - if form.Avatar.Size > setting.Avatar.MaxFileSize { - return errors.New(ctx.Locale.TrString("settings.uploaded_avatar_is_too_big", form.Avatar.Size/1024, setting.Avatar.MaxFileSize/1024)) - } - - data, err := io.ReadAll(fr) - if err != nil { - return fmt.Errorf("io.ReadAll: %w", err) - } - - st := typesniffer.DetectContentType(data, "") - if !st.IsImage() || st.IsSvgImage() { - return errors.New(ctx.Locale.TrString("settings.uploaded_avatar_not_a_image")) - } - if err = user_service.UploadAvatar(ctx, ctxUser, data); err != nil { + data, err := forms.ReadAvatar(form.Avatar, ctx.Locale) + if err != nil { + return err + } + if data != nil { + if err := user_service.UploadAvatar(ctx, ctxUser, data); err != nil { return fmt.Errorf("UploadAvatar: %w", err) } } else if ctxUser.UseCustomAvatar && ctxUser.Avatar == "" { diff --git a/services/forms/avatar.go b/services/forms/avatar.go new file mode 100644 index 0000000000..47cdc6c0df --- /dev/null +++ b/services/forms/avatar.go @@ -0,0 +1,45 @@ +// Copyright 2018 The Gitea Authors. All rights reserved. +// Copyright 2026 The Forgejo Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package forms + +import ( + "errors" + "fmt" + "io" + "mime/multipart" + + "forgejo.org/modules/setting" + "forgejo.org/modules/translation" + "forgejo.org/modules/typesniffer" +) + +// ReadAvatar reads and validates an avatar from a multipart file header. +func ReadAvatar(header *multipart.FileHeader, locale translation.Locale) ([]byte, error) { + if header == nil || header.Filename == "" { + return nil, nil + } + + r, err := header.Open() + if err != nil { + return nil, fmt.Errorf("Avatar.Open: %w", err) + } + defer r.Close() + + if header.Size > setting.Avatar.MaxFileSize { + return nil, errors.New(locale.TrString("settings.uploaded_avatar_is_too_big", header.Size/1024, setting.Avatar.MaxFileSize/1024)) + } + + data, err := io.ReadAll(r) + if err != nil { + return nil, fmt.Errorf("io.ReadAll: %w", err) + } + + st := typesniffer.DetectContentType(data, "") + if !st.IsImage() || st.IsSvgImage() { + return nil, errors.New(locale.TrString("settings.uploaded_avatar_not_a_image")) + } + + return data, nil +} diff --git a/tests/integration/repo_avatar_test.go b/tests/integration/repo_avatar_test.go new file mode 100644 index 0000000000..451557119c --- /dev/null +++ b/tests/integration/repo_avatar_test.go @@ -0,0 +1,94 @@ +// Copyright 2026 The Forgejo Authors. All rights reserved. +// SPDX-License-Identifier: GPL-3.0-or-later + +package integration + +import ( + "bytes" + "image/png" + "io" + "mime/multipart" + "net/http" + "testing" + + repo_model "forgejo.org/models/repo" + "forgejo.org/models/unittest" + user_model "forgejo.org/models/user" + "forgejo.org/modules/avatar" + app_context "forgejo.org/services/context" + "forgejo.org/tests" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestRepoAvatar(t *testing.T) { + defer tests.PrepareTestEnv(t)() + + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) + user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + + session := loginUser(t, user2.Name) + avatarURL := "/" + repo.OwnerName + "/" + repo.Name + "/settings/avatar" + + t.Run("valid avatar", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + img, err := avatar.RandomImage([]byte("seed")) + require.NoError(t, err) + + imgData := &bytes.Buffer{} + require.NoError(t, png.Encode(imgData, img)) + + body := &bytes.Buffer{} + writer := multipart.NewWriter(body) + require.NoError(t, writer.WriteField("source", "local")) + part, err := writer.CreateFormFile("avatar", "avatar.png") + require.NoError(t, err) + _, err = io.Copy(part, imgData) + require.NoError(t, err) + require.NoError(t, writer.Close()) + + req := NewRequestWithBody(t, "POST", avatarURL, body) + req.Header.Add("Content-Type", writer.FormDataContentType()) + session.MakeRequest(t, req, http.StatusSeeOther) + + flashCookie := session.GetCookie(app_context.CookieNameFlash) + require.NotNil(t, flashCookie) + assert.Contains(t, flashCookie.Value, "success") + + // Verify avatar was actually set + repo = unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) + assert.NotEmpty(t, repo.CustomAvatarRelativePath()) + + // Verify avatar is accessible + req = NewRequest(t, "GET", "/repo-avatars/"+repo.Avatar) + _ = session.MakeRequest(t, req, http.StatusOK) + + // Clean up + req = NewRequest(t, "POST", avatarURL+"/delete") + session.MakeRequest(t, req, http.StatusOK) + }) + + t.Run("no file selected", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + // Simulate what a browser sends when user did not select a file + body := &bytes.Buffer{} + writer := multipart.NewWriter(body) + require.NoError(t, writer.WriteField("source", "local")) + _, err := writer.CreateFormFile("avatar", "") + require.NoError(t, err) + require.NoError(t, writer.Close()) + + req := NewRequestWithBody(t, "POST", avatarURL, body) + req.Header.Add("Content-Type", writer.FormDataContentType()) + session.MakeRequest(t, req, http.StatusSeeOther) + + // Should silently skip + flashCookie := session.GetCookie(app_context.CookieNameFlash) + if flashCookie != nil { + assert.NotContains(t, flashCookie.Value, "error") + } + }) +}