From 2c624d4deb11be876a3cd66c26cd071813e3e1f6 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Sun, 1 Mar 2026 21:32:35 +0800 Subject: [PATCH] Refactor avatar package, support default avatar fallback (#36788) * Fix #34715 --- models/repo/avatar.go | 12 +- models/user/avatar.go | 12 +- modules/assetfs/embed.go | 6 +- modules/avatar/avatar.go | 17 +-- modules/avatar/avatar_test.go | 28 ++-- modules/avatar/identicon/identicon.go | 31 ++--- modules/avatar/identicon/identicon_test.go | 2 +- modules/httpcache/httpcache.go | 34 ++--- modules/httpcache/httpcache_test.go | 143 +++++++++------------ routers/api/v1/repo/file.go | 8 +- routers/common/serve.go | 2 +- routers/web/base.go | 102 +++++++-------- routers/web/repo/attachment.go | 2 +- routers/web/repo/download.go | 4 +- tests/integration/user_avatar_test.go | 78 +++++------ 15 files changed, 205 insertions(+), 276 deletions(-) diff --git a/models/repo/avatar.go b/models/repo/avatar.go index eff64bd239..bfb08d9982 100644 --- a/models/repo/avatar.go +++ b/models/repo/avatar.go @@ -41,20 +41,14 @@ func generateRandomAvatar(ctx context.Context, repo *Repository) error { idToString := strconv.FormatInt(repo.ID, 10) seed := idToString - img, err := avatar.RandomImage([]byte(seed)) - if err != nil { - return fmt.Errorf("RandomImage: %w", err) - } + img := avatar.RandomImageDefaultSize([]byte(seed)) repo.Avatar = idToString if err := storage.SaveFrom(storage.RepoAvatars, repo.CustomAvatarRelativePath(), func(w io.Writer) error { - if err := png.Encode(w, img); err != nil { - log.Error("Encode: %v", err) - } - return err + return png.Encode(w, img) }); err != nil { - return fmt.Errorf("Failed to create dir %s: %w", repo.CustomAvatarRelativePath(), err) + return fmt.Errorf("failed to create dir %s: %w", repo.CustomAvatarRelativePath(), err) } log.Info("New random avatar created for repository: %d", repo.ID) diff --git a/models/user/avatar.go b/models/user/avatar.go index 8a5ff30bdf..5811d85911 100644 --- a/models/user/avatar.go +++ b/models/user/avatar.go @@ -30,22 +30,16 @@ func GenerateRandomAvatar(ctx context.Context, u *User) error { seed = u.Name } - img, err := avatar.RandomImage([]byte(seed)) - if err != nil { - return fmt.Errorf("RandomImage: %w", err) - } + img := avatar.RandomImageDefaultSize([]byte(seed)) u.Avatar = avatars.HashEmail(seed) - _, err = storage.Avatars.Stat(u.CustomAvatarRelativePath()) + _, err := storage.Avatars.Stat(u.CustomAvatarRelativePath()) if err != nil { // If unable to Stat the avatar file (usually it means non-existing), then try to save a new one // Don't share the images so that we can delete them easily if err := storage.SaveFrom(storage.Avatars, u.CustomAvatarRelativePath(), func(w io.Writer) error { - if err := png.Encode(w, img); err != nil { - log.Error("Encode: %v", err) - } - return nil + return png.Encode(w, img) }); err != nil { return fmt.Errorf("failed to save avatar %s: %w", u.CustomAvatarRelativePath(), err) } diff --git a/modules/assetfs/embed.go b/modules/assetfs/embed.go index 0b544635db..7c90e3aca2 100644 --- a/modules/assetfs/embed.go +++ b/modules/assetfs/embed.go @@ -260,7 +260,7 @@ func (fi *embeddedFileInfo) Mode() fs.FileMode { } func (fi *embeddedFileInfo) ModTime() time.Time { - return getExecutableModTime() + return GetExecutableModTime() } func (fi *embeddedFileInfo) IsDir() bool { @@ -279,9 +279,9 @@ func (fi *embeddedFileInfo) Info() (fs.FileInfo, error) { return fi, nil } -// getExecutableModTime returns the modification time of the executable file. +// GetExecutableModTime returns the modification time of the executable file. // In bindata, we can't use the ModTime of the files because we need to make the build reproducible -var getExecutableModTime = sync.OnceValue(func() (modTime time.Time) { +var GetExecutableModTime = sync.OnceValue(func() (modTime time.Time) { exePath, err := os.Executable() if err != nil { return modTime diff --git a/modules/avatar/avatar.go b/modules/avatar/avatar.go index 106215ec0b..3b622b99af 100644 --- a/modules/avatar/avatar.go +++ b/modules/avatar/avatar.go @@ -28,21 +28,18 @@ import ( // than the size after resizing. const DefaultAvatarSize = 256 -// RandomImageSize generates and returns a random avatar image unique to input data +// RandomImageWithSize generates and returns a random avatar image unique to input data // in custom size (height and width). -func RandomImageSize(size int, data []byte) (image.Image, error) { +func RandomImageWithSize(size int, data []byte) image.Image { // we use white as background, and use dark colors to draw blocks - imgMaker, err := identicon.New(size, color.White, identicon.DarkColors...) - if err != nil { - return nil, fmt.Errorf("identicon.New: %w", err) - } - return imgMaker.Make(data), nil + imgMaker := identicon.New(size, color.White, identicon.DarkColors) + return imgMaker.Make(data) } -// RandomImage generates and returns a random avatar image unique to input data +// RandomImageDefaultSize generates and returns a random avatar image unique to input data // in default size (height and width). -func RandomImage(data []byte) (image.Image, error) { - return RandomImageSize(DefaultAvatarSize*setting.Avatar.RenderedSizeFactor, data) +func RandomImageDefaultSize(data []byte) image.Image { + return RandomImageWithSize(DefaultAvatarSize*setting.Avatar.RenderedSizeFactor, data) } // processAvatarImage process the avatar image data, crop and resize it if necessary. diff --git a/modules/avatar/avatar_test.go b/modules/avatar/avatar_test.go index a5a1a7c1b0..5414184b6b 100644 --- a/modules/avatar/avatar_test.go +++ b/modules/avatar/avatar_test.go @@ -15,19 +15,6 @@ import ( "github.com/stretchr/testify/assert" ) -func Test_RandomImageSize(t *testing.T) { - _, err := RandomImageSize(0, []byte("gitea@local")) - assert.Error(t, err) - - _, err = RandomImageSize(64, []byte("gitea@local")) - assert.NoError(t, err) -} - -func Test_RandomImage(t *testing.T) { - _, err := RandomImage([]byte("gitea@local")) - assert.NoError(t, err) -} - func Test_ProcessAvatarPNG(t *testing.T) { setting.Avatar.MaxWidth = 4096 setting.Avatar.MaxHeight = 4096 @@ -134,3 +121,18 @@ func Test_ProcessAvatarImage(t *testing.T) { _, err = processAvatarImage(origin, 262144) assert.ErrorContains(t, err, "image width is too large: 10 > 5") } + +func BenchmarkRandomImage(b *testing.B) { + b.Run("size-48", func(b *testing.B) { + for b.Loop() { + // BenchmarkRandomImage/size-48-12 49549 22899 ns/op + RandomImageWithSize(48, []byte("test-content")) + } + }) + b.Run("size-96", func(b *testing.B) { + for b.Loop() { + // BenchmarkRandomImage/size-96-12 13816 88187 ns/op + RandomImageWithSize(96, []byte("test-content")) + } + }) +} diff --git a/modules/avatar/identicon/identicon.go b/modules/avatar/identicon/identicon.go index 19f87da85a..a1a0f0d7bf 100644 --- a/modules/avatar/identicon/identicon.go +++ b/modules/avatar/identicon/identicon.go @@ -8,13 +8,14 @@ package identicon import ( "crypto/sha256" - "errors" - "fmt" "image" "image/color" ) -const minImageSize = 16 +const ( + minImageSize = 16 + maxImageSize = 2048 +) // Identicon is used to generate pseudo-random avatars type Identicon struct { @@ -24,25 +25,17 @@ type Identicon struct { rect image.Rectangle } -// New returns an Identicon struct with the correct settings -// size image size -// back background color -// fore all possible foreground colors. only one foreground color will be picked randomly for one image -func New(size int, back color.Color, fore ...color.Color) (*Identicon, error) { - if len(fore) == 0 { - return nil, errors.New("foreground is not set") - } - - if size < minImageSize { - return nil, fmt.Errorf("size %d is smaller than min size %d", size, minImageSize) - } - +// New returns an Identicon struct. +// Only one foreground color will be picked randomly for one image. +func New(size int, backColor color.Color, foreColors []color.Color) *Identicon { + size = max(size, minImageSize) + size = min(size, maxImageSize) return &Identicon{ - foreColors: fore, - backColor: back, + foreColors: foreColors, + backColor: backColor, size: size, rect: image.Rect(0, 0, size, size), - }, nil + } } // Make generates an avatar by data diff --git a/modules/avatar/identicon/identicon_test.go b/modules/avatar/identicon/identicon_test.go index 23bcc73e2e..6af4b41cce 100644 --- a/modules/avatar/identicon/identicon_test.go +++ b/modules/avatar/identicon/identicon_test.go @@ -23,7 +23,7 @@ func TestGenerate(t *testing.T) { } backColor := color.White - imgMaker, err := New(64, backColor, DarkColors...) + imgMaker, err := New(64, backColor, DarkColors) assert.NoError(t, err) for i := 0; i < 100; i++ { s := strconv.Itoa(i) diff --git a/modules/httpcache/httpcache.go b/modules/httpcache/httpcache.go index dd3efab7a5..e23cf0a92a 100644 --- a/modules/httpcache/httpcache.go +++ b/modules/httpcache/httpcache.go @@ -60,21 +60,6 @@ func CacheControlForPrivateStatic() *CacheControlOptions { } } -// HandleGenericETagCache handles ETag-based caching for a HTTP request. -// It returns true if the request was handled. -func HandleGenericETagCache(req *http.Request, w http.ResponseWriter, etag string) (handled bool) { - if len(etag) > 0 { - w.Header().Set("Etag", etag) - if checkIfNoneMatchIsValid(req, etag) { - w.WriteHeader(http.StatusNotModified) - return true - } - } - // not sure whether it is a public content, so just use "private" (old behavior) - SetCacheControlInHeader(w.Header(), CacheControlForPrivateStatic()) - return false -} - // checkIfNoneMatchIsValid tests if the header If-None-Match matches the ETag func checkIfNoneMatchIsValid(req *http.Request, etag string) bool { ifNoneMatch := req.Header.Get("If-None-Match") @@ -89,10 +74,18 @@ func checkIfNoneMatchIsValid(req *http.Request, etag string) bool { return false } -// HandleGenericETagTimeCache handles ETag-based caching with Last-Modified caching for a HTTP request. +func HandleGenericETagPublicCache(req *http.Request, w http.ResponseWriter, etag string, lastModified *time.Time) bool { + return handleGenericETagTimeCache(req, w, etag, lastModified, CacheControlForPublicStatic()) +} + +func HandleGenericETagPrivateCache(req *http.Request, w http.ResponseWriter, etag string, lastModified *time.Time) bool { + return handleGenericETagTimeCache(req, w, etag, lastModified, CacheControlForPrivateStatic()) +} + +// handleGenericETagTimeCache handles ETag-based caching with Last-Modified caching for the HTTP request. // It returns true if the request was handled. -func HandleGenericETagTimeCache(req *http.Request, w http.ResponseWriter, etag string, lastModified *time.Time) (handled bool) { - if len(etag) > 0 { +func handleGenericETagTimeCache(req *http.Request, w http.ResponseWriter, etag string, lastModified *time.Time, cacheControlOpts *CacheControlOptions) (handled bool) { + if etag != "" { w.Header().Set("Etag", etag) } if lastModified != nil && !lastModified.IsZero() { @@ -100,7 +93,7 @@ func HandleGenericETagTimeCache(req *http.Request, w http.ResponseWriter, etag s w.Header().Set("Last-Modified", lastModified.UTC().Format(http.TimeFormat)) } - if len(etag) > 0 { + if etag != "" { if checkIfNoneMatchIsValid(req, etag) { w.WriteHeader(http.StatusNotModified) return true @@ -117,7 +110,6 @@ func HandleGenericETagTimeCache(req *http.Request, w http.ResponseWriter, etag s } } - // not sure whether it is a public content, so just use "private" (old behavior) - SetCacheControlInHeader(w.Header(), CacheControlForPrivateStatic()) + SetCacheControlInHeader(w.Header(), cacheControlOpts) return false } diff --git a/modules/httpcache/httpcache_test.go b/modules/httpcache/httpcache_test.go index d81f06097c..d510cbe4d7 100644 --- a/modules/httpcache/httpcache_test.go +++ b/modules/httpcache/httpcache_test.go @@ -6,92 +6,73 @@ package httpcache import ( "net/http" "net/http/httptest" - "strings" "testing" + "time" + + "code.gitea.io/gitea/modules/util" "github.com/stretchr/testify/assert" ) -func countFormalHeaders(h http.Header) (c int) { - for k := range h { - // ignore our headers for internal usage - if strings.HasPrefix(k, "X-Gitea-") { - continue - } - c++ - } - return c -} - func TestHandleGenericETagCache(t *testing.T) { - etag := `"test"` + matchedEtag := `"matched-etag"` + lastModifiedTime := new(time.Date(2021, time.January, 2, 15, 4, 5, 0, time.FixedZone("test-zone", 8*3600))) + lastModified := lastModifiedTime.UTC().Format(http.TimeFormat) + cacheControl := "max-age=0, private, must-revalidate, no-transform" + type testCase struct { + name string + reqHeaders map[string]string + wantHandled bool + wantHeaders map[string]string + wantStatus int + } + cases := []testCase{ + { + name: "No If-None-Match", + wantHandled: false, + wantHeaders: map[string]string{"Last-Modified": lastModified, "Cache-Control": cacheControl, "Etag": matchedEtag}, + }, + { + name: "Mismatched If-None-Match", + reqHeaders: map[string]string{"If-None-Match": `"mismatched-etag"`}, + wantHandled: false, + wantHeaders: map[string]string{"Last-Modified": lastModified, "Cache-Control": cacheControl, "Etag": matchedEtag}, + }, + { + name: "Matched If-None-Match", + reqHeaders: map[string]string{"If-None-Match": matchedEtag}, + wantHandled: true, + wantHeaders: map[string]string{"Last-Modified": lastModified, "Cache-Control": "", "Etag": matchedEtag}, + wantStatus: http.StatusNotModified, + }, + { + name: "Multiple Mismatched If-None-Match", + reqHeaders: map[string]string{"If-None-Match": `"mismatched-etag1", "mismatched-etag2"`}, + wantHandled: false, + wantHeaders: map[string]string{"Last-Modified": lastModified, "Cache-Control": cacheControl, "Etag": matchedEtag}, + }, + { + name: "Multiple Matched If-None-Match", + reqHeaders: map[string]string{"If-None-Match": `"mismatched-etag", ` + matchedEtag}, + wantHandled: true, + wantHeaders: map[string]string{"Last-Modified": lastModified, "Cache-Control": "", "Etag": matchedEtag}, + wantStatus: http.StatusNotModified, + }, + } - t.Run("No_If-None-Match", func(t *testing.T) { - req := &http.Request{Header: make(http.Header)} - w := httptest.NewRecorder() - - handled := HandleGenericETagCache(req, w, etag) - - assert.False(t, handled) - assert.Equal(t, 2, countFormalHeaders(w.Header())) - assert.Contains(t, w.Header(), "Cache-Control") - assert.Contains(t, w.Header(), "Etag") - assert.Equal(t, etag, w.Header().Get("Etag")) - }) - t.Run("Wrong_If-None-Match", func(t *testing.T) { - req := &http.Request{Header: make(http.Header)} - w := httptest.NewRecorder() - - req.Header.Set("If-None-Match", `"wrong etag"`) - - handled := HandleGenericETagCache(req, w, etag) - - assert.False(t, handled) - assert.Equal(t, 2, countFormalHeaders(w.Header())) - assert.Contains(t, w.Header(), "Cache-Control") - assert.Contains(t, w.Header(), "Etag") - assert.Equal(t, etag, w.Header().Get("Etag")) - }) - t.Run("Correct_If-None-Match", func(t *testing.T) { - req := &http.Request{Header: make(http.Header)} - w := httptest.NewRecorder() - - req.Header.Set("If-None-Match", etag) - - handled := HandleGenericETagCache(req, w, etag) - - assert.True(t, handled) - assert.Equal(t, 1, countFormalHeaders(w.Header())) - assert.Contains(t, w.Header(), "Etag") - assert.Equal(t, etag, w.Header().Get("Etag")) - assert.Equal(t, http.StatusNotModified, w.Code) - }) - t.Run("Multiple_Wrong_If-None-Match", func(t *testing.T) { - req := &http.Request{Header: make(http.Header)} - w := httptest.NewRecorder() - - req.Header.Set("If-None-Match", `"wrong etag", "wrong etag "`) - - handled := HandleGenericETagCache(req, w, etag) - - assert.False(t, handled) - assert.Equal(t, 2, countFormalHeaders(w.Header())) - assert.Contains(t, w.Header(), "Cache-Control") - assert.Contains(t, w.Header(), "Etag") - assert.Equal(t, etag, w.Header().Get("Etag")) - }) - t.Run("Multiple_Correct_If-None-Match", func(t *testing.T) { - req := &http.Request{Header: make(http.Header)} - w := httptest.NewRecorder() - - req.Header.Set("If-None-Match", `"wrong etag", `+etag) - - handled := HandleGenericETagCache(req, w, etag) - - assert.True(t, handled) - assert.Equal(t, 1, countFormalHeaders(w.Header())) - assert.Contains(t, w.Header(), "Etag") - assert.Equal(t, etag, w.Header().Get("Etag")) - assert.Equal(t, http.StatusNotModified, w.Code) - }) + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + req := httptest.NewRequest(http.MethodGet, "http://example.com/test", nil) + for k, v := range tc.reqHeaders { + req.Header.Set(k, v) + } + w := httptest.NewRecorder() + assert.Equal(t, tc.wantHandled, HandleGenericETagPrivateCache(req, w, matchedEtag, lastModifiedTime)) + resp := w.Result() + for k, v := range tc.wantHeaders { + assert.Equal(t, v, resp.Header.Get(k)) + } + assert.Equal(t, tc.wantStatus, util.Iif(resp.StatusCode == http.StatusOK, 0, resp.StatusCode)) + }) + } } diff --git a/routers/api/v1/repo/file.go b/routers/api/v1/repo/file.go index deb68963c2..e31bc24eef 100644 --- a/routers/api/v1/repo/file.go +++ b/routers/api/v1/repo/file.go @@ -138,7 +138,7 @@ func GetRawFileOrLFS(ctx *context.APIContext) { // LFS Pointer files are at most 1024 bytes - so any blob greater than 1024 bytes cannot be an LFS file if blob.Size() > lfs.MetaFileMaxSize { // First handle caching for the blob - if httpcache.HandleGenericETagTimeCache(ctx.Req, ctx.Resp, `"`+blob.ID.String()+`"`, lastModified) { + if httpcache.HandleGenericETagPrivateCache(ctx.Req, ctx.Resp, `"`+blob.ID.String()+`"`, lastModified) { return } @@ -174,7 +174,7 @@ func GetRawFileOrLFS(ctx *context.APIContext) { // if it's not a pointer, just serve the data directly if !pointer.IsValid() { // First handle caching for the blob - if httpcache.HandleGenericETagTimeCache(ctx.Req, ctx.Resp, `"`+blob.ID.String()+`"`, lastModified) { + if httpcache.HandleGenericETagPrivateCache(ctx.Req, ctx.Resp, `"`+blob.ID.String()+`"`, lastModified) { return } @@ -189,7 +189,7 @@ func GetRawFileOrLFS(ctx *context.APIContext) { // If there isn't one, just serve the data directly if errors.Is(err, git_model.ErrLFSObjectNotExist) { // Handle caching for the blob SHA (not the LFS object OID) - if httpcache.HandleGenericETagTimeCache(ctx.Req, ctx.Resp, `"`+blob.ID.String()+`"`, lastModified) { + if httpcache.HandleGenericETagPrivateCache(ctx.Req, ctx.Resp, `"`+blob.ID.String()+`"`, lastModified) { return } @@ -201,7 +201,7 @@ func GetRawFileOrLFS(ctx *context.APIContext) { } // Handle caching for the LFS object OID - if httpcache.HandleGenericETagCache(ctx.Req, ctx.Resp, `"`+pointer.Oid+`"`) { + if httpcache.HandleGenericETagPrivateCache(ctx.Req, ctx.Resp, `"`+pointer.Oid+`"`, meta.UpdatedUnix.AsTimePtr()) { return } diff --git a/routers/common/serve.go b/routers/common/serve.go index 862230b30f..4bb1a48b0d 100644 --- a/routers/common/serve.go +++ b/routers/common/serve.go @@ -20,7 +20,7 @@ import ( // ServeBlob download a git.Blob func ServeBlob(ctx *context.Base, repo *repo_model.Repository, filePath string, blob *git.Blob, lastModified *time.Time) error { - if httpcache.HandleGenericETagTimeCache(ctx.Req, ctx.Resp, `"`+blob.ID.String()+`"`, lastModified) { + if httpcache.HandleGenericETagPrivateCache(ctx.Req, ctx.Resp, `"`+blob.ID.String()+`"`, lastModified) { return nil } diff --git a/routers/web/base.go b/routers/web/base.go index 0f06cb5e4b..db96432bed 100644 --- a/routers/web/base.go +++ b/routers/web/base.go @@ -6,11 +6,14 @@ package web import ( "errors" "fmt" + "image/png" "net/http" "os" "path" "strings" + "code.gitea.io/gitea/modules/assetfs" + "code.gitea.io/gitea/modules/avatar" "code.gitea.io/gitea/modules/httpcache" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/setting" @@ -22,79 +25,70 @@ import ( func avatarStorageHandler(storageSetting *setting.Storage, prefix string, objStore storage.ObjectStorage) http.HandlerFunc { prefix = strings.Trim(prefix, "/") funcInfo := routing.GetFuncInfo(avatarStorageHandler, prefix) + exeModTime := assetfs.GetExecutableModTime() + fallbackEtag := fmt.Sprintf(`"avatar-%s"`, exeModTime.Format("20060102150405")) - if storageSetting.ServeDirect() { - return func(w http.ResponseWriter, req *http.Request) { - if req.Method != http.MethodGet && req.Method != http.MethodHead { - http.Error(w, http.StatusText(http.StatusMethodNotAllowed), http.StatusMethodNotAllowed) - return - } - - if !strings.HasPrefix(req.URL.Path, "/"+prefix+"/") { - http.Error(w, http.StatusText(http.StatusNotFound), http.StatusNotFound) - return - } - defer routing.RecordFuncInfo(req.Context(), funcInfo)() - - rPath := strings.TrimPrefix(req.URL.Path, "/"+prefix+"/") - rPath = util.PathJoinRelX(rPath) - - u, err := objStore.URL(rPath, path.Base(rPath), req.Method, nil) - if err != nil { - if os.IsNotExist(err) || errors.Is(err, os.ErrNotExist) { - log.Warn("Unable to find %s %s", prefix, rPath) - http.Error(w, http.StatusText(http.StatusNotFound), http.StatusNotFound) - return - } - log.Error("Error whilst getting URL for %s %s. Error: %v", prefix, rPath, err) - http.Error(w, fmt.Sprintf("Error whilst getting URL for %s %s", prefix, rPath), http.StatusInternalServerError) - return - } - - http.Redirect(w, req, u.String(), http.StatusTemporaryRedirect) + handleError := func(w http.ResponseWriter, req *http.Request, avatarPath string, err error) bool { + if err == nil { + return false } + + if errors.Is(err, os.ErrNotExist) || errors.Is(err, util.ErrNotExist) { + // if avatar doesn't exist, generate a random one and serve it with proper cache control headers + w.Header().Set("Content-Type", "image/png") + if !httpcache.HandleGenericETagPublicCache(req, w, fallbackEtag, &exeModTime) { + if req.Method == http.MethodGet { + img := avatar.RandomImageWithSize(96, []byte(avatarPath)) + _ = png.Encode(w, img) + } // else: for HEAD request, just return the headers without body + } + } else { + // for internal errors, log the error and return 500 + log.Error("Error when serving avatar %s: %s", req.URL.Path, err) + http.Error(w, "unable to serve avatar image", http.StatusInternalServerError) + } + return true } return func(w http.ResponseWriter, req *http.Request) { - if req.Method != http.MethodGet && req.Method != http.MethodHead { - http.Error(w, http.StatusText(http.StatusMethodNotAllowed), http.StatusMethodNotAllowed) - return - } - - if !strings.HasPrefix(req.URL.Path, "/"+prefix+"/") { - http.Error(w, http.StatusText(http.StatusNotFound), http.StatusNotFound) - return - } defer routing.RecordFuncInfo(req.Context(), funcInfo)() - rPath := strings.TrimPrefix(req.URL.Path, "/"+prefix+"/") - rPath = util.PathJoinRelX(rPath) - if rPath == "" || rPath == "." { - http.Error(w, http.StatusText(http.StatusNotFound), http.StatusNotFound) + avatarPath, ok := strings.CutPrefix(req.URL.Path, "/"+prefix+"/") + if !ok { + http.Error(w, "invalid avatar path", http.StatusBadRequest) + return + } + avatarPath = util.PathJoinRelX(avatarPath) + if avatarPath == "" || avatarPath == "." { + http.Error(w, "not found", http.StatusNotFound) return } - fi, err := objStore.Stat(rPath) - if err != nil { - if os.IsNotExist(err) || errors.Is(err, os.ErrNotExist) { - log.Warn("Unable to find %s %s", prefix, rPath) - http.Error(w, http.StatusText(http.StatusNotFound), http.StatusNotFound) + if storageSetting.ServeDirect() { + // Old logic: no check for existence by Stat, so old code's "errors.Is(err, os.ErrNotExist)" didn't work. + // So in theory, it doesn't work with the non-existing avatar fallback, it just gets the URL and redirects to it. + // Checking "stat" requires one more request to the storage, which is inefficient. + // Workaround: disable "SERVE_DIRECT". Leave the problem to the future. + u, err := objStore.URL(avatarPath, path.Base(avatarPath), req.Method, nil) + if handleError(w, req, avatarPath, err) { return } - log.Error("Error whilst opening %s %s. Error: %v", prefix, rPath, err) - http.Error(w, fmt.Sprintf("Error whilst opening %s %s", prefix, rPath), http.StatusInternalServerError) + http.Redirect(w, req, u.String(), http.StatusTemporaryRedirect) return } - fr, err := objStore.Open(rPath) - if err != nil { - log.Error("Error whilst opening %s %s. Error: %v", prefix, rPath, err) - http.Error(w, fmt.Sprintf("Error whilst opening %s %s", prefix, rPath), http.StatusInternalServerError) + fr, err := objStore.Open(avatarPath) + if handleError(w, req, avatarPath, err) { return } defer fr.Close() + fi, err := fr.Stat() + if handleError(w, req, avatarPath, err) { + return + } + httpcache.SetCacheControlInHeader(w.Header(), httpcache.CacheControlForPublicStatic()) - http.ServeContent(w, req, path.Base(rPath), fi.ModTime(), fr) + http.ServeContent(w, req, path.Base(avatarPath), fi.ModTime(), fr) } } diff --git a/routers/web/repo/attachment.go b/routers/web/repo/attachment.go index a83f0f40ec..be711afe6d 100644 --- a/routers/web/repo/attachment.go +++ b/routers/web/repo/attachment.go @@ -187,7 +187,7 @@ func ServeAttachment(ctx *context.Context, uuid string) { } } - if httpcache.HandleGenericETagCache(ctx.Req, ctx.Resp, `"`+attach.UUID+`"`) { + if httpcache.HandleGenericETagPrivateCache(ctx.Req, ctx.Resp, `"`+attach.UUID+`"`, attach.CreatedUnix.AsTimePtr()) { return } diff --git a/routers/web/repo/download.go b/routers/web/repo/download.go index 6f394aae27..9412d2045d 100644 --- a/routers/web/repo/download.go +++ b/routers/web/repo/download.go @@ -20,7 +20,7 @@ import ( // ServeBlobOrLFS download a git.Blob redirecting to LFS if necessary func ServeBlobOrLFS(ctx *context.Context, blob *git.Blob, lastModified *time.Time) error { - if httpcache.HandleGenericETagTimeCache(ctx.Req, ctx.Resp, `"`+blob.ID.String()+`"`, lastModified) { + if httpcache.HandleGenericETagPrivateCache(ctx.Req, ctx.Resp, `"`+blob.ID.String()+`"`, lastModified) { return nil } @@ -48,7 +48,7 @@ func ServeBlobOrLFS(ctx *context.Context, blob *git.Blob, lastModified *time.Tim closed = true return common.ServeBlob(ctx.Base, ctx.Repo.Repository, ctx.Repo.TreePath, blob, lastModified) } - if httpcache.HandleGenericETagCache(ctx.Req, ctx.Resp, `"`+pointer.Oid+`"`) { + if httpcache.HandleGenericETagPrivateCache(ctx.Req, ctx.Resp, `"`+pointer.Oid+`"`, meta.UpdatedUnix.AsTimePtr()) { return nil } diff --git a/tests/integration/user_avatar_test.go b/tests/integration/user_avatar_test.go index eeb0b15b93..add1554b7a 100644 --- a/tests/integration/user_avatar_test.go +++ b/tests/integration/user_avatar_test.go @@ -17,72 +17,54 @@ import ( "code.gitea.io/gitea/tests" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestUserAvatar(t *testing.T) { defer tests.PrepareTestEnv(t)() - user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) // owner of the repo3, is an org - - seed := user2.Email - if len(seed) == 0 { - seed = user2.Name - } - - img, err := avatar.RandomImage([]byte(seed)) - if err != nil { - assert.NoError(t, err) - return - } - session := loginUser(t, "user2") - imgData := &bytes.Buffer{} + img := avatar.RandomImageDefaultSize([]byte("any-random-image-seed")) + originAvatarData := &bytes.Buffer{} + require.NoError(t, png.Encode(originAvatarData, img)) + // setup multipart form to upload avatar body := &bytes.Buffer{} - - // Setup multi-part writer := multipart.NewWriter(body) - writer.WriteField("source", "local") + _ = writer.WriteField("source", "local") part, err := writer.CreateFormFile("avatar", "avatar-for-testuseravatar.png") - if err != nil { - assert.NoError(t, err) - return - } - - if err := png.Encode(imgData, img); err != nil { - assert.NoError(t, err) - return - } - - if _, err := io.Copy(part, imgData); err != nil { - assert.NoError(t, err) - return - } - - if err := writer.Close(); err != nil { - assert.NoError(t, err) - return - } + require.NoError(t, err) + _, _ = io.Copy(part, bytes.NewReader(originAvatarData.Bytes())) + require.NoError(t, writer.Close()) + // upload avatar req := NewRequestWithBody(t, "POST", "/user/settings/avatar", body) req.Header.Add("Content-Type", writer.FormDataContentType()) - session.MakeRequest(t, req, http.StatusSeeOther) - user2 = unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) // owner of the repo3, is an org - + // check user2's avatar can be accessed + user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) req = NewRequest(t, "GET", user2.AvatarLinkWithSize(t.Context(), 0)) _ = session.MakeRequest(t, req, http.StatusOK) - testGetAvatarRedirect(t, user2) + req = NewRequest(t, "GET", "/user2.png") + resp := MakeRequest(t, req, http.StatusSeeOther) + avatarRedirect := resp.Header().Get("Location") + assert.Equal(t, "/avatars/"+user2.Avatar, avatarRedirect) - // Can't test if the response matches because the image is re-generated on upload but checking that this at least doesn't give a 404 should be enough. -} + // check the content of the avatar is correct + resp = MakeRequest(t, NewRequest(t, "GET", avatarRedirect), http.StatusOK) + assert.Equal(t, "image/png", resp.Header().Get("Content-Type")) + avatarData, _ := io.ReadAll(resp.Body) + assert.Equal(t, originAvatarData.Bytes(), avatarData) -func testGetAvatarRedirect(t *testing.T, user *user_model.User) { - t.Run("getAvatarRedirect_"+user.Name, func(t *testing.T) { - req := NewRequestf(t, "GET", "/%s.png", user.Name) - resp := MakeRequest(t, req, http.StatusSeeOther) - assert.Equal(t, "/avatars/"+user.Avatar, resp.Header().Get("location")) - }) + // for non-existing avatar, it should return a random one with proper cache control headers + resp = MakeRequest(t, NewRequest(t, "GET", "/avatars/no-such-avatar"), http.StatusOK) + assert.Equal(t, "image/png", resp.Header().Get("Content-Type")) + assert.NotEmpty(t, resp.Header().Get("ETag")) + assert.NotEmpty(t, resp.Header().Get("Last-Modified")) + assert.Contains(t, resp.Header().Get("Cache-Control"), "public") + avatarData, _ = io.ReadAll(resp.Body) + _, err = png.Decode(bytes.NewReader(avatarData)) + require.NoError(t, err) }