From 44371b96f56d408ed9af487d482ea021bfabeafa Mon Sep 17 00:00:00 2001 From: zeripath Date: Thu, 24 Jan 2019 14:12:17 +0000 Subject: [PATCH] Ensure valid git author names passed in signatures (#5774) * 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. * 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" * 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 | 17 ++++++++++------ integrations/user_test.go | 1 + models/fixtures/user.yml | 6 +++--- models/issue_reaction_test.go | 2 +- models/user.go | 27 ++++++++++++++++++++++--- models/user_test.go | 32 ++++++++++++++++++++++++++++++ 6 files changed, 72 insertions(+), 13 deletions(-) diff --git a/integrations/api_user_orgs_test.go b/integrations/api_user_orgs_test.go index f8372d611576..9b250c063647 100644 --- a/integrations/api_user_orgs_test.go +++ b/integrations/api_user_orgs_test.go @@ -9,7 +9,9 @@ import ( "net/http" "testing" + "code.gitea.io/gitea/models" api "code.gitea.io/sdk/gitea" + "github.com/stretchr/testify/assert" ) @@ -23,14 +25,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 +52,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.go b/models/user.go index 0d8f60886137..4ab78ec04ef0 100644 --- a/models/user.go +++ b/models/user.go @@ -417,7 +417,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(), } @@ -630,12 +630,33 @@ 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 } +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 := gitSafeName(u.FullName) + if len(gitName) > 0 { + return gitName + } + // 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 func (u *User) ShortName(length int) string { return base.EllipsisString(u.Name, length) 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) + } +}