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 <art27@cantab.net>

4 files changed, 60 insertions, 7 deletions

diff --git a/models/fixtures/user.yml b/models/fixtures/user.yml
index dc3de2a2e1..3a44946bb2 100644
--- a/models/fixtures/user.yml
+++ b/models/fixtures/user.yml
@@ -19,7 +19,7 @@
   id: 2
   lower_name: user2
   name: user2
-  full_name: User Two
+  full_name: "   < U<se>r Tw<o > ><  "
   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 8363e534d5..bbd8cf29fe 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 0d8f608861..4ab78ec04e 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 ba700d3659..9d011f32a0 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)
+	}
+}