peezy-tech/jojo
2
0
Fork 0
mirror of https://codeberg.org/forgejo/forgejo.git synced 2026-05-12 22:10:25 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions

[v14.0/forgejo] fix: skip repo avatar upload when no file is selected (#11555)

Browse source 
**Backport:** https://codeberg.org/forgejo/forgejo/pulls/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 != ""`](e1cecbd276/routers/web/user/setting/profile.go (L141)), I've done the same for the repo avatar handler.

Co-authored-by: Bram Hagens <bram@bramh.me>
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/11555
Reviewed-by: Gusted <gusted@noreply.codeberg.org>
Co-authored-by: forgejo-backport-action <forgejo-backport-action@noreply.codeberg.org>
Co-committed-by: forgejo-backport-action <forgejo-backport-action@noreply.codeberg.org>
This commit is contained in:
forgejo-backport-action 2026-03-07 22:39:20 +01:00 • committed by Gusted
parent 4123ace6c4
commit 9421d22215
4 changed files with 151 additions and 58 deletions
Download patch file Download diff file Expand all files Collapse all files

40
routers/web/repo/setting/avatar.go
View file

@ -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

30
routers/web/user/setting/profile.go
View file

@ -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"
@ -135,27 +132,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 == "" {

45
services/forms/avatar.go Normal file
View file

@ -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
}

94
tests/integration/repo_avatar_test.go Normal file
View file

@ -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")
}
})
}