From 96797fed311151ff889f87c94c7b6aaa16c5d535 Mon Sep 17 00:00:00 2001 From: Gusted Date: Mon, 2 Jan 2023 22:46:39 +0100 Subject: [PATCH] Unify hashing for avatar (#22289) - Unify the hashing code for repository and user avatars into a function. - Use a sane hash function instead of MD5. - Only require hashing once instead of twice(w.r.t. hashing for user avatar). - Improve the comment for the hashing code of why it works. Co-authored-by: Lunny Xiao Co-authored-by: Yarden Shoham --- modules/avatar/hash.go | 28 ++++++++++++++++++++++++++++ services/repository/avatar.go | 3 +-- services/repository/avatar_test.go | 5 ++--- services/user/user.go | 7 +------ 4 files changed, 32 insertions(+), 11 deletions(-) create mode 100644 modules/avatar/hash.go diff --git a/modules/avatar/hash.go b/modules/avatar/hash.go new file mode 100644 index 0000000000..50db9c1943 --- /dev/null +++ b/modules/avatar/hash.go @@ -0,0 +1,28 @@ +// Copyright 2023 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package avatar + +import ( + "crypto/sha256" + "encoding/hex" + "strconv" +) + +// HashAvatar will generate a unique string, which ensures that when there's a +// different unique ID while the data is the same, it will generate a different +// output. It will generate the output according to: +// HEX(HASH(uniqueID || - || data)) +// The hash being used is SHA256. +// The sole purpose of the unique ID is to generate a distinct hash Such that +// two unique IDs with the same data will have a different hash output. +// The "-" byte is important to ensure that data cannot be modified such that +// the first byte is a number, which could lead to a "collision" with the hash +// of another unique ID. +func HashAvatar(uniqueID int64, data []byte) string { + h := sha256.New() + h.Write([]byte(strconv.FormatInt(uniqueID, 10))) + h.Write([]byte{'-'}) + h.Write(data) + return hex.EncodeToString(h.Sum(nil)) +} diff --git a/services/repository/avatar.go b/services/repository/avatar.go index a829a1000a..5fe8bd2c72 100644 --- a/services/repository/avatar.go +++ b/services/repository/avatar.go @@ -5,7 +5,6 @@ package repository import ( "context" - "crypto/md5" "fmt" "image/png" "io" @@ -27,7 +26,7 @@ func UploadAvatar(repo *repo_model.Repository, data []byte) error { return err } - newAvatar := fmt.Sprintf("%d-%x", repo.ID, md5.Sum(data)) + newAvatar := avatar.HashAvatar(repo.ID, data) if repo.Avatar == newAvatar { // upload the same picture return nil } diff --git a/services/repository/avatar_test.go b/services/repository/avatar_test.go index 3875302696..5ec899ec3f 100644 --- a/services/repository/avatar_test.go +++ b/services/repository/avatar_test.go @@ -5,14 +5,13 @@ package repository import ( "bytes" - "crypto/md5" - "fmt" "image" "image/png" "testing" repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/models/unittest" + "code.gitea.io/gitea/modules/avatar" "github.com/stretchr/testify/assert" ) @@ -28,7 +27,7 @@ func TestUploadAvatar(t *testing.T) { err := UploadAvatar(repo, buff.Bytes()) assert.NoError(t, err) - assert.Equal(t, fmt.Sprintf("%d-%x", 10, md5.Sum(buff.Bytes())), repo.Avatar) + assert.Equal(t, avatar.HashAvatar(10, buff.Bytes()), repo.Avatar) } func TestUploadBigAvatar(t *testing.T) { diff --git a/services/user/user.go b/services/user/user.go index 65db732bf9..c95eb67a85 100644 --- a/services/user/user.go +++ b/services/user/user.go @@ -5,7 +5,6 @@ package user import ( "context" - "crypto/md5" "fmt" "image/png" "io" @@ -241,11 +240,7 @@ func UploadAvatar(u *user_model.User, data []byte) error { defer committer.Close() u.UseCustomAvatar = true - // Different users can upload same image as avatar - // If we prefix it with u.ID, it will be separated - // Otherwise, if any of the users delete his avatar - // Other users will lose their avatars too. - u.Avatar = fmt.Sprintf("%x", md5.Sum([]byte(fmt.Sprintf("%d-%x", u.ID, md5.Sum(data))))) + u.Avatar = avatar.HashAvatar(u.ID, data) if err = user_model.UpdateUserCols(ctx, u, "use_custom_avatar", "avatar"); err != nil { return fmt.Errorf("updateUser: %w", err) }