From 8f0a35fe2ad88912f5929208dcabb89f8e63a602 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Sat, 19 Jan 2019 19:05:22 +0000 Subject: [PATCH 1/4] Ensure valid git author names passed in signatures Fix #5772 - Git author names are not allowed to include `\n` `<` or `>` and must not be empty. Ensure that the name passed in a signature is valid. Signed-off-by: Andrew Thornton --- models/user.go | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/models/user.go b/models/user.go index 764c2280d734..f49196e22076 100644 --- a/models/user.go +++ b/models/user.go @@ -416,7 +416,7 @@ func (u *User) GetFollowing(page int) ([]*User, error) { // NewGitSig generates and returns the signature of given user. func (u *User) NewGitSig() *git.Signature { return &git.Signature{ - Name: u.DisplayName(), + Name: u.GitName(), Email: u.getEmail(), When: time.Now(), } @@ -629,8 +629,18 @@ func (u *User) GetOrganizations(all bool) error { // DisplayName returns full name if it's not empty, // returns username otherwise. func (u *User) DisplayName() string { - if len(u.FullName) > 0 { - return u.FullName + trimmed := strings.TrimSpace(u.FullName) + if len(trimmed) > 0 { + return trimmed + } + return u.Name +} + +// GitName returns a git safe name +func (u *User) GitName() string { + gitName := strings.TrimSpace(strings.NewReplacer("\n", "", "<", "", ">", "").Replace(u.FullName)) + if len(gitName) > 0 { + return gitName } return u.Name } From 67fcd64fa01eaf636fe18b3da2c73326b5445c07 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Sun, 20 Jan 2019 12:06:33 +0000 Subject: [PATCH 2/4] Account for pathologically named external users LDAP and the like usernames are not checked in the same way that users who signup are. Therefore just ensure that user names are also git safe and if totally pathological - Set them to "user-$UID" Signed-off-by: Andrew Thornton --- models/user.go | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/models/user.go b/models/user.go index f49196e22076..55809dbc332f 100644 --- a/models/user.go +++ b/models/user.go @@ -636,13 +636,24 @@ func (u *User) DisplayName() string { return u.Name } +func gitSafeName(name string) string { + return strings.TrimSpace(strings.NewReplacer("\n", "", "<", "", ">", "").Replace(name)) +} + // GitName returns a git safe name func (u *User) GitName() string { - gitName := strings.TrimSpace(strings.NewReplacer("\n", "", "<", "", ">", "").Replace(u.FullName)) + gitName := gitSafeName(u.FullName) if len(gitName) > 0 { return gitName } - return u.Name + // Although u.Name should be safe if created in our system + // LDAP users may have bad names + gitName = gitSafeName(u.Name) + if len(gitName) > 0 { + return gitName + } + // Totally pathological name so it's got to be: + return fmt.Sprintf("user-%d", u.ID) } // ShortName ellipses username to length From 1ae279a48b718eaf62baa831ce139857a1424df6 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Sun, 20 Jan 2019 12:08:05 +0000 Subject: [PATCH 3/4] Add Tests and adjust test users Make our testcases a little more pathological so that we be sure that integration tests have a chance to spot these cases. Signed-off-by: Andrew Thornton --- integrations/api_user_orgs_test.go | 16 +++++++++------ integrations/user_test.go | 1 + models/fixtures/user.yml | 6 +++--- models/issue_reaction_test.go | 2 +- models/user_test.go | 32 ++++++++++++++++++++++++++++++ 5 files changed, 47 insertions(+), 10 deletions(-) diff --git a/integrations/api_user_orgs_test.go b/integrations/api_user_orgs_test.go index f8372d611576..1439541d68d8 100644 --- a/integrations/api_user_orgs_test.go +++ b/integrations/api_user_orgs_test.go @@ -9,6 +9,7 @@ import ( "net/http" "testing" + "code.gitea.io/gitea/models" api "code.gitea.io/sdk/gitea" "github.com/stretchr/testify/assert" ) @@ -23,14 +24,16 @@ func TestUserOrgs(t *testing.T) { req := NewRequest(t, "GET", urlStr) resp := session.MakeRequest(t, req, http.StatusOK) var orgs []*api.Organization + user3 := models.AssertExistsAndLoadBean(t, &models.User{Name: "user3"}).(*models.User) + DecodeJSON(t, resp, &orgs) assert.Equal(t, []*api.Organization{ { ID: 3, - UserName: "user3", - FullName: "User Three", - AvatarURL: "https://secure.gravatar.com/avatar/97d6d9441ff85fdc730e02a6068d267b?d=identicon", + UserName: user3.Name, + FullName: user3.FullName, + AvatarURL: user3.AvatarLink(), Description: "", Website: "", Location: "", @@ -48,13 +51,14 @@ func TestMyOrgs(t *testing.T) { resp := session.MakeRequest(t, req, http.StatusOK) var orgs []*api.Organization DecodeJSON(t, resp, &orgs) + user3 := models.AssertExistsAndLoadBean(t, &models.User{Name: "user3"}).(*models.User) assert.Equal(t, []*api.Organization{ { ID: 3, - UserName: "user3", - FullName: "User Three", - AvatarURL: "https://secure.gravatar.com/avatar/97d6d9441ff85fdc730e02a6068d267b?d=identicon", + UserName: user3.Name, + FullName: user3.FullName, + AvatarURL: user3.AvatarLink(), Description: "", Website: "", Location: "", diff --git a/integrations/user_test.go b/integrations/user_test.go index 7ff986d5460c..a6ad164d614d 100644 --- a/integrations/user_test.go +++ b/integrations/user_test.go @@ -47,6 +47,7 @@ func TestRenameInvalidUsername(t *testing.T) { "%2f..", "%00", "thisHas ASpace", + "ptho>lor Tw >< " email: user2@example.com passwd: 7d93daa0d1e6f2305cc8fa496847d61dc7320bb16262f9c55dd753480207234cdd96a93194e408341971742f4701772a025a # password type: 0 # individual @@ -37,7 +37,7 @@ id: 3 lower_name: user3 name: user3 - full_name: User Three + full_name: " <<<< >> >> > >> > >>> >> " email: user3@example.com passwd: 7d93daa0d1e6f2305cc8fa496847d61dc7320bb16262f9c55dd753480207234cdd96a93194e408341971742f4701772a025a # password type: 1 # organization @@ -53,7 +53,7 @@ id: 4 lower_name: user4 name: user4 - full_name: User Four + full_name: " " email: user4@example.com passwd: 7d93daa0d1e6f2305cc8fa496847d61dc7320bb16262f9c55dd753480207234cdd96a93194e408341971742f4701772a025a # password type: 0 # individual diff --git a/models/issue_reaction_test.go b/models/issue_reaction_test.go index 8363e534d573..bbd8cf29fec1 100644 --- a/models/issue_reaction_test.go +++ b/models/issue_reaction_test.go @@ -99,7 +99,7 @@ func TestIssueReactionCount(t *testing.T) { reactions := issue1.Reactions.GroupByType() assert.Len(t, reactions["heart"], 4) assert.Equal(t, 2, reactions["heart"].GetMoreUserCount()) - assert.Equal(t, "User One, User Two", reactions["heart"].GetFirstUsers()) + assert.Equal(t, user1.DisplayName()+", "+user2.DisplayName(), reactions["heart"].GetFirstUsers()) assert.True(t, reactions["heart"].HasUser(1)) assert.False(t, reactions["heart"].HasUser(5)) assert.False(t, reactions["heart"].HasUser(0)) diff --git a/models/user_test.go b/models/user_test.go index ba700d36599d..9d011f32a096 100644 --- a/models/user_test.go +++ b/models/user_test.go @@ -6,6 +6,7 @@ package models import ( "math/rand" + "strings" "testing" "code.gitea.io/gitea/modules/setting" @@ -181,3 +182,34 @@ func TestGetOrgRepositoryIDs(t *testing.T) { // User 5's team has no access to any repo assert.Len(t, accessibleRepos, 0) } + +func TestNewGitSig(t *testing.T) { + users := make([]*User, 0, 20) + sess := x.NewSession() + defer sess.Close() + sess.Find(&users) + + for _, user := range users { + sig := user.NewGitSig() + assert.NotContains(t, sig.Name, "<") + assert.NotContains(t, sig.Name, ">") + assert.NotContains(t, sig.Name, "\n") + assert.NotEqual(t, len(strings.TrimSpace(sig.Name)), 0) + } +} + +func TestDisplayName(t *testing.T) { + users := make([]*User, 0, 20) + sess := x.NewSession() + defer sess.Close() + sess.Find(&users) + + for _, user := range users { + displayName := user.DisplayName() + assert.Equal(t, strings.TrimSpace(displayName), displayName) + if len(strings.TrimSpace(user.FullName)) == 0 { + assert.Equal(t, user.Name, displayName) + } + assert.NotEqual(t, len(strings.TrimSpace(displayName)), 0) + } +} From 40bdc556a184f1c348dacdec4318b65750cfb273 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Sun, 20 Jan 2019 21:55:25 +0000 Subject: [PATCH 4/4] Update spacing in imports Signed-off-by: Andrew Thornton --- integrations/api_user_orgs_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/integrations/api_user_orgs_test.go b/integrations/api_user_orgs_test.go index 1439541d68d8..9b250c063647 100644 --- a/integrations/api_user_orgs_test.go +++ b/integrations/api_user_orgs_test.go @@ -11,6 +11,7 @@ import ( "code.gitea.io/gitea/models" api "code.gitea.io/sdk/gitea" + "github.com/stretchr/testify/assert" )