From d09faeca3e8a80219c6be2354148be38d45fb88d Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Sat, 29 Jun 2024 15:24:05 +0200 Subject: [PATCH 1/9] Use FullName in Emails to address the recipient if possible --- models/user/user.go | 31 +++++++++++++++++++++++++++++++ models/user/user_test.go | 23 +++++++++++++++++++++++ services/mailer/mail.go | 8 +++++--- services/mailer/mail_release.go | 8 ++++---- services/mailer/mail_repo.go | 4 ++-- 5 files changed, 65 insertions(+), 9 deletions(-) diff --git a/models/user/user.go b/models/user/user.go index 23637f4616288..dc1f01f0dfe83 100644 --- a/models/user/user.go +++ b/models/user/user.go @@ -8,9 +8,11 @@ import ( "context" "encoding/hex" "fmt" + "net/mail" "net/url" "path/filepath" "regexp" + "strconv" "strings" "time" "unicode" @@ -413,6 +415,35 @@ func (u *User) DisplayName() string { return u.Name } +// EmailTo returns full name and email. +func (u *User) EmailTo() string { + // we don't deal with utf8 to ascii conversion + if u.DisplayName() != strings.Trim(strconv.QuoteToASCII(u.DisplayName()), `"`) { + return u.Email + } + // ok just be sure we don't let the user break things somehow + sanitizedDisplayName := strings.ReplaceAll(u.DisplayName(), "\n", "") + sanitizedDisplayName = strings.ReplaceAll(sanitizedDisplayName, "\r", "") + sanitizedDisplayName = strings.ReplaceAll(sanitizedDisplayName, "<", "") + sanitizedDisplayName = strings.ReplaceAll(sanitizedDisplayName, ">", "") + sanitizedDisplayName = strings.ReplaceAll(sanitizedDisplayName, ",", "") + sanitizedDisplayName = strings.ReplaceAll(sanitizedDisplayName, ":", "") + sanitizedDisplayName = strings.ReplaceAll(sanitizedDisplayName, ";", "") + + // should be an edge case but nice to have + if sanitizedDisplayName == u.Email { + return u.Email + } + + to := fmt.Sprintf("%s <%s>", sanitizedDisplayName, u.Email) + add, err := mail.ParseAddress(to) + if err != nil { + return u.Email + } + + return fmt.Sprintf("%s <%s>", add.Name, add.Address) +} + // GetDisplayName returns full name if it's not empty and DEFAULT_SHOW_FULL_NAME is set, // returns username otherwise. func (u *User) GetDisplayName() string { diff --git a/models/user/user_test.go b/models/user/user_test.go index c4e278caab756..0711d877f0018 100644 --- a/models/user/user_test.go +++ b/models/user/user_test.go @@ -529,6 +529,29 @@ func Test_NormalizeUserFromEmail(t *testing.T) { } } +func TestEmailTo(t *testing.T) { + testCases := []struct { + fullName string + mail string + result string + }{ + {"Awareness Hub", "awareness@hub.net", "Awareness Hub "}, + {"name@example.com", "name@example.com", "name@example.com"}, + {"Hi Its ", "ee@mail.box", "Hi Its Mee ee@mail.box"}, + {"Sinéad.O'Connor", "sinead.oconnor@gmail.com", "sinead.oconnor@gmail.com"}, + {"Æsir", "aesir@gmx.de", "aesir@gmx.de"}, + {"new😀user", "new.user@alo.com", "new.user@alo.com"}, + {`"quoted"`, "quoted@test.com", "quoted@test.com"}, + } + + for _, testCase := range testCases { + t.Run(testCase.result, func(t *testing.T) { + testUser := &user_model.User{FullName: testCase.fullName, Email: testCase.mail} + assert.EqualValues(t, testCase.result, testUser.EmailTo()) + }) + } +} + func TestDisabledUserFeatures(t *testing.T) { assert.NoError(t, unittest.PrepareTestDatabase()) diff --git a/services/mailer/mail.go b/services/mailer/mail.go index 000cc835c8b77..c16759407d561 100644 --- a/services/mailer/mail.go +++ b/services/mailer/mail.go @@ -82,7 +82,7 @@ func sendUserMail(language string, u *user_model.User, tpl base.TplName, code, s return } - msg := NewMessage(u.Email, subject, content.String()) + msg := NewMessage(u.EmailTo(), subject, content.String()) msg.Info = fmt.Sprintf("UID: %d, %s", u.ID, info) SendAsync(msg) @@ -130,6 +130,8 @@ func SendActivateEmailMail(u *user_model.User, email string) { return } + email = fmt.Sprintf("%s <%s>", u.DisplayName(), email) // TODO: do we need to sanitize the display name? + msg := NewMessage(email, locale.TrString("mail.activate_email"), content.String()) msg.Info = fmt.Sprintf("UID: %d, activate email", u.ID) @@ -158,7 +160,7 @@ func SendRegisterNotifyMail(u *user_model.User) { return } - msg := NewMessage(u.Email, locale.TrString("mail.register_notify"), content.String()) + msg := NewMessage(u.EmailTo(), locale.TrString("mail.register_notify"), content.String()) msg.Info = fmt.Sprintf("UID: %d, registration notify", u.ID) SendAsync(msg) @@ -189,7 +191,7 @@ func SendCollaboratorMail(u, doer *user_model.User, repo *repo_model.Repository) return } - msg := NewMessage(u.Email, subject, content.String()) + msg := NewMessage(u.EmailTo(), subject, content.String()) msg.Info = fmt.Sprintf("UID: %d, add collaborator", u.ID) SendAsync(msg) diff --git a/services/mailer/mail_release.go b/services/mailer/mail_release.go index b7a4da0db92b3..f1f2e558a776b 100644 --- a/services/mailer/mail_release.go +++ b/services/mailer/mail_release.go @@ -40,10 +40,10 @@ func MailNewRelease(ctx context.Context, rel *repo_model.Release) { return } - langMap := make(map[string][]string) + langMap := make(map[string][]*user_model.User) for _, user := range recipients { if user.ID != rel.PublisherID { - langMap[user.Language] = append(langMap[user.Language], user.Email) + langMap[user.Language] = append(langMap[user.Language], user) } } @@ -52,7 +52,7 @@ func MailNewRelease(ctx context.Context, rel *repo_model.Release) { } } -func mailNewRelease(ctx context.Context, lang string, tos []string, rel *repo_model.Release) { +func mailNewRelease(ctx context.Context, lang string, tos []*user_model.User, rel *repo_model.Release) { locale := translation.NewLocale(lang) var err error @@ -89,7 +89,7 @@ func mailNewRelease(ctx context.Context, lang string, tos []string, rel *repo_mo publisherName := rel.Publisher.DisplayName() msgID := generateMessageIDForRelease(rel) for _, to := range tos { - msg := NewMessageFrom(to, publisherName, setting.MailService.FromEmail, subject, mailBody.String()) + msg := NewMessageFrom(to.EmailTo(), publisherName, setting.MailService.FromEmail, subject, mailBody.String()) msg.Info = subject msg.SetHeader("Message-ID", msgID) msgs = append(msgs, msg) diff --git a/services/mailer/mail_repo.go b/services/mailer/mail_repo.go index e0d55bb120043..3befe55d94342 100644 --- a/services/mailer/mail_repo.go +++ b/services/mailer/mail_repo.go @@ -34,7 +34,7 @@ func SendRepoTransferNotifyMail(ctx context.Context, doer, newOwner *user_model. // don't send emails to inactive users continue } - langMap[user.Language] = append(langMap[user.Language], user.Email) + langMap[user.Language] = append(langMap[user.Language], user.EmailTo()) } for lang, tos := range langMap { @@ -46,7 +46,7 @@ func SendRepoTransferNotifyMail(ctx context.Context, doer, newOwner *user_model. return nil } - return sendRepoTransferNotifyMailPerLang(newOwner.Language, newOwner, doer, []string{newOwner.Email}, repo) + return sendRepoTransferNotifyMailPerLang(newOwner.Language, newOwner, doer, []string{newOwner.EmailTo()}, repo) } // sendRepoTransferNotifyMail triggers a notification e-mail when a pending repository transfer was created for each language From 6111bb60a0e6526fce1892dcca7771a33d129abc Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Sat, 29 Jun 2024 15:27:23 +0200 Subject: [PATCH 2/9] not worth it --- services/mailer/mail.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/services/mailer/mail.go b/services/mailer/mail.go index c16759407d561..11810a036d62d 100644 --- a/services/mailer/mail.go +++ b/services/mailer/mail.go @@ -130,8 +130,6 @@ func SendActivateEmailMail(u *user_model.User, email string) { return } - email = fmt.Sprintf("%s <%s>", u.DisplayName(), email) // TODO: do we need to sanitize the display name? - msg := NewMessage(email, locale.TrString("mail.activate_email"), content.String()) msg.Info = fmt.Sprintf("UID: %d, activate email", u.ID) From 484894da1120f4ce033a790171d9af9e932f40df Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Sat, 29 Jun 2024 15:28:17 +0200 Subject: [PATCH 3/9] jaja --- models/user/user_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/models/user/user_test.go b/models/user/user_test.go index 0711d877f0018..6aa72a3e1cefe 100644 --- a/models/user/user_test.go +++ b/models/user/user_test.go @@ -537,7 +537,7 @@ func TestEmailTo(t *testing.T) { }{ {"Awareness Hub", "awareness@hub.net", "Awareness Hub "}, {"name@example.com", "name@example.com", "name@example.com"}, - {"Hi Its ", "ee@mail.box", "Hi Its Mee ee@mail.box"}, + {"Hi Its ", "ee@mail.box", "Hi Its Mee "}, {"Sinéad.O'Connor", "sinead.oconnor@gmail.com", "sinead.oconnor@gmail.com"}, {"Æsir", "aesir@gmx.de", "aesir@gmx.de"}, {"new😀user", "new.user@alo.com", "new.user@alo.com"}, From d8bf307bc9deed6ffbda1a02027699ff54f4e97b Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Sat, 29 Jun 2024 16:12:39 +0200 Subject: [PATCH 4/9] more copy-by-ref --- services/mailer/mail_repo.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/services/mailer/mail_repo.go b/services/mailer/mail_repo.go index 3befe55d94342..619223033259a 100644 --- a/services/mailer/mail_repo.go +++ b/services/mailer/mail_repo.go @@ -28,13 +28,13 @@ func SendRepoTransferNotifyMail(ctx context.Context, doer, newOwner *user_model. return err } - langMap := make(map[string][]string) + langMap := make(map[string][]*user_model.User) for _, user := range users { if !user.IsActive { // don't send emails to inactive users continue } - langMap[user.Language] = append(langMap[user.Language], user.EmailTo()) + langMap[user.Language] = append(langMap[user.Language], user) } for lang, tos := range langMap { @@ -46,11 +46,11 @@ func SendRepoTransferNotifyMail(ctx context.Context, doer, newOwner *user_model. return nil } - return sendRepoTransferNotifyMailPerLang(newOwner.Language, newOwner, doer, []string{newOwner.EmailTo()}, repo) + return sendRepoTransferNotifyMailPerLang(newOwner.Language, newOwner, doer, []string{newOwner}, repo) } // sendRepoTransferNotifyMail triggers a notification e-mail when a pending repository transfer was created for each language -func sendRepoTransferNotifyMailPerLang(lang string, newOwner, doer *user_model.User, emails []string, repo *repo_model.Repository) error { +func sendRepoTransferNotifyMailPerLang(lang string, newOwner, doer *user_model.User, emailTos []*user_model.User, repo *repo_model.Repository) error { var ( locale = translation.NewLocale(lang) content bytes.Buffer @@ -78,8 +78,8 @@ func sendRepoTransferNotifyMailPerLang(lang string, newOwner, doer *user_model.U return err } - for _, to := range emails { - msg := NewMessage(to, subject, content.String()) + for _, to := range emailTos { + msg := NewMessage(to.EmailTo(), subject, content.String()) msg.Info = fmt.Sprintf("UID: %d, repository pending transfer notification", newOwner.ID) SendAsync(msg) From afc8257d0eb7414d598920b058f93e26ef1a4d86 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Sat, 29 Jun 2024 16:41:49 +0200 Subject: [PATCH 5/9] fix --- services/mailer/mail_repo.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/mailer/mail_repo.go b/services/mailer/mail_repo.go index 619223033259a..28b9cef8a75e1 100644 --- a/services/mailer/mail_repo.go +++ b/services/mailer/mail_repo.go @@ -46,7 +46,7 @@ func SendRepoTransferNotifyMail(ctx context.Context, doer, newOwner *user_model. return nil } - return sendRepoTransferNotifyMailPerLang(newOwner.Language, newOwner, doer, []string{newOwner}, repo) + return sendRepoTransferNotifyMailPerLang(newOwner.Language, newOwner, doer, []*user_model.User{newOwner}, repo) } // sendRepoTransferNotifyMail triggers a notification e-mail when a pending repository transfer was created for each language From dc9ce7cde598ae800b8e941096ce23d3a8ef3011 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Sun, 30 Jun 2024 02:29:53 +0200 Subject: [PATCH 6/9] isLatin --- models/user/user.go | 34 +++++++++++++++++++++++----------- 1 file changed, 23 insertions(+), 11 deletions(-) diff --git a/models/user/user.go b/models/user/user.go index dc1f01f0dfe83..33203291f4201 100644 --- a/models/user/user.go +++ b/models/user/user.go @@ -12,7 +12,6 @@ import ( "net/url" "path/filepath" "regexp" - "strconv" "strings" "time" "unicode" @@ -415,20 +414,24 @@ func (u *User) DisplayName() string { return u.Name } +var emailToReplacer = strings.NewReplacer( + "\n", "", + "\r", "", + "<", "", + ">", "", + ",", "", + ":", "", + ";", "", +) + // EmailTo returns full name and email. func (u *User) EmailTo() string { - // we don't deal with utf8 to ascii conversion - if u.DisplayName() != strings.Trim(strconv.QuoteToASCII(u.DisplayName()), `"`) { + sanitizedDisplayName := emailToReplacer.Replace(u.DisplayName()) + + // we don't deal with non ascii strings + if !isLatin(sanitizedDisplayName) { return u.Email } - // ok just be sure we don't let the user break things somehow - sanitizedDisplayName := strings.ReplaceAll(u.DisplayName(), "\n", "") - sanitizedDisplayName = strings.ReplaceAll(sanitizedDisplayName, "\r", "") - sanitizedDisplayName = strings.ReplaceAll(sanitizedDisplayName, "<", "") - sanitizedDisplayName = strings.ReplaceAll(sanitizedDisplayName, ">", "") - sanitizedDisplayName = strings.ReplaceAll(sanitizedDisplayName, ",", "") - sanitizedDisplayName = strings.ReplaceAll(sanitizedDisplayName, ":", "") - sanitizedDisplayName = strings.ReplaceAll(sanitizedDisplayName, ";", "") // should be an edge case but nice to have if sanitizedDisplayName == u.Email { @@ -444,6 +447,15 @@ func (u *User) EmailTo() string { return fmt.Sprintf("%s <%s>", add.Name, add.Address) } +func isLatin(s string) bool { + for _, char := range s { + if !unicode.IsLetter(char) && !unicode.IsDigit(char) { + return false + } + } + return true +} + // GetDisplayName returns full name if it's not empty and DEFAULT_SHOW_FULL_NAME is set, // returns username otherwise. func (u *User) GetDisplayName() string { From 72cb96b717e0072580e2a625b10f46230a5a9840 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Sun, 30 Jun 2024 03:28:45 +0200 Subject: [PATCH 7/9] regex --- models/user/user.go | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/models/user/user.go b/models/user/user.go index 33203291f4201..f224eefd9c21c 100644 --- a/models/user/user.go +++ b/models/user/user.go @@ -424,12 +424,14 @@ var emailToReplacer = strings.NewReplacer( ";", "", ) +var isAZ09 = regexp.MustCompile(`^[a-zA-Z0-9 ]+$`) + // EmailTo returns full name and email. func (u *User) EmailTo() string { sanitizedDisplayName := emailToReplacer.Replace(u.DisplayName()) // we don't deal with non ascii strings - if !isLatin(sanitizedDisplayName) { + if !isAZ09.Match([]byte(sanitizedDisplayName)) { return u.Email } @@ -447,15 +449,6 @@ func (u *User) EmailTo() string { return fmt.Sprintf("%s <%s>", add.Name, add.Address) } -func isLatin(s string) bool { - for _, char := range s { - if !unicode.IsLetter(char) && !unicode.IsDigit(char) { - return false - } - } - return true -} - // GetDisplayName returns full name if it's not empty and DEFAULT_SHOW_FULL_NAME is set, // returns username otherwise. func (u *User) GetDisplayName() string { From 4a47d3255d9880d5ce874d4342e0257b345fc54d Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Tue, 2 Jul 2024 06:42:52 +0200 Subject: [PATCH 8/9] Update models/user/user.go Co-authored-by: silverwind --- models/user/user.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/models/user/user.go b/models/user/user.go index f224eefd9c21c..f80e477a1e66b 100644 --- a/models/user/user.go +++ b/models/user/user.go @@ -426,7 +426,7 @@ var emailToReplacer = strings.NewReplacer( var isAZ09 = regexp.MustCompile(`^[a-zA-Z0-9 ]+$`) -// EmailTo returns full name and email. +// EmailTo returns a string suitable to be put into a e-mail `To:` header. func (u *User) EmailTo() string { sanitizedDisplayName := emailToReplacer.Replace(u.DisplayName()) From 872d8da262285e12e5ef28ed785317648f26de5b Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Thu, 4 Jul 2024 16:51:04 +0200 Subject: [PATCH 9/9] use mime-encode --- models/user/user.go | 10 ++-------- models/user/user_test.go | 8 ++++---- 2 files changed, 6 insertions(+), 12 deletions(-) diff --git a/models/user/user.go b/models/user/user.go index f80e477a1e66b..5ac24f09778c3 100644 --- a/models/user/user.go +++ b/models/user/user.go @@ -8,6 +8,7 @@ import ( "context" "encoding/hex" "fmt" + "mime" "net/mail" "net/url" "path/filepath" @@ -424,17 +425,10 @@ var emailToReplacer = strings.NewReplacer( ";", "", ) -var isAZ09 = regexp.MustCompile(`^[a-zA-Z0-9 ]+$`) - // EmailTo returns a string suitable to be put into a e-mail `To:` header. func (u *User) EmailTo() string { sanitizedDisplayName := emailToReplacer.Replace(u.DisplayName()) - // we don't deal with non ascii strings - if !isAZ09.Match([]byte(sanitizedDisplayName)) { - return u.Email - } - // should be an edge case but nice to have if sanitizedDisplayName == u.Email { return u.Email @@ -446,7 +440,7 @@ func (u *User) EmailTo() string { return u.Email } - return fmt.Sprintf("%s <%s>", add.Name, add.Address) + return fmt.Sprintf("%s <%s>", mime.QEncoding.Encode("utf-8", add.Name), add.Address) } // GetDisplayName returns full name if it's not empty and DEFAULT_SHOW_FULL_NAME is set, diff --git a/models/user/user_test.go b/models/user/user_test.go index 6aa72a3e1cefe..26cda3f89e324 100644 --- a/models/user/user_test.go +++ b/models/user/user_test.go @@ -538,10 +538,10 @@ func TestEmailTo(t *testing.T) { {"Awareness Hub", "awareness@hub.net", "Awareness Hub "}, {"name@example.com", "name@example.com", "name@example.com"}, {"Hi Its ", "ee@mail.box", "Hi Its Mee "}, - {"Sinéad.O'Connor", "sinead.oconnor@gmail.com", "sinead.oconnor@gmail.com"}, - {"Æsir", "aesir@gmx.de", "aesir@gmx.de"}, - {"new😀user", "new.user@alo.com", "new.user@alo.com"}, - {`"quoted"`, "quoted@test.com", "quoted@test.com"}, + {"Sinéad.O'Connor", "sinead.oconnor@gmail.com", "=?utf-8?q?Sin=C3=A9ad.O'Connor?= "}, + {"Æsir", "aesir@gmx.de", "=?utf-8?q?=C3=86sir?= "}, + {"new😀user", "new.user@alo.com", "=?utf-8?q?new=F0=9F=98=80user?= "}, + {`"quoted"`, "quoted@test.com", "quoted "}, } for _, testCase := range testCases {