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>
This commit is contained in:
zeripath 2019-01-24 14:12:17 +00:00 committed by Lauris BH
parent cd83c2ca05
commit 44371b96f5
6 changed files with 72 additions and 13 deletions

View file

@ -9,7 +9,9 @@ import (
"net/http" "net/http"
"testing" "testing"
"code.gitea.io/gitea/models"
api "code.gitea.io/sdk/gitea" api "code.gitea.io/sdk/gitea"
"github.com/stretchr/testify/assert" "github.com/stretchr/testify/assert"
) )
@ -23,14 +25,16 @@ func TestUserOrgs(t *testing.T) {
req := NewRequest(t, "GET", urlStr) req := NewRequest(t, "GET", urlStr)
resp := session.MakeRequest(t, req, http.StatusOK) resp := session.MakeRequest(t, req, http.StatusOK)
var orgs []*api.Organization var orgs []*api.Organization
user3 := models.AssertExistsAndLoadBean(t, &models.User{Name: "user3"}).(*models.User)
DecodeJSON(t, resp, &orgs) DecodeJSON(t, resp, &orgs)
assert.Equal(t, []*api.Organization{ assert.Equal(t, []*api.Organization{
{ {
ID: 3, ID: 3,
UserName: "user3", UserName: user3.Name,
FullName: "User Three", FullName: user3.FullName,
AvatarURL: "https://secure.gravatar.com/avatar/97d6d9441ff85fdc730e02a6068d267b?d=identicon", AvatarURL: user3.AvatarLink(),
Description: "", Description: "",
Website: "", Website: "",
Location: "", Location: "",
@ -48,13 +52,14 @@ func TestMyOrgs(t *testing.T) {
resp := session.MakeRequest(t, req, http.StatusOK) resp := session.MakeRequest(t, req, http.StatusOK)
var orgs []*api.Organization var orgs []*api.Organization
DecodeJSON(t, resp, &orgs) DecodeJSON(t, resp, &orgs)
user3 := models.AssertExistsAndLoadBean(t, &models.User{Name: "user3"}).(*models.User)
assert.Equal(t, []*api.Organization{ assert.Equal(t, []*api.Organization{
{ {
ID: 3, ID: 3,
UserName: "user3", UserName: user3.Name,
FullName: "User Three", FullName: user3.FullName,
AvatarURL: "https://secure.gravatar.com/avatar/97d6d9441ff85fdc730e02a6068d267b?d=identicon", AvatarURL: user3.AvatarLink(),
Description: "", Description: "",
Website: "", Website: "",
Location: "", Location: "",

View file

@ -47,6 +47,7 @@ func TestRenameInvalidUsername(t *testing.T) {
"%2f..", "%2f..",
"%00", "%00",
"thisHas ASpace", "thisHas ASpace",
"p<A>tho>lo<gical",
} }
session := loginUser(t, "user2") session := loginUser(t, "user2")

View file

@ -19,7 +19,7 @@
id: 2 id: 2
lower_name: user2 lower_name: user2
name: user2 name: user2
full_name: User Two full_name: " < U<se>r Tw<o > >< "
email: user2@example.com email: user2@example.com
passwd: 7d93daa0d1e6f2305cc8fa496847d61dc7320bb16262f9c55dd753480207234cdd96a93194e408341971742f4701772a025a # password passwd: 7d93daa0d1e6f2305cc8fa496847d61dc7320bb16262f9c55dd753480207234cdd96a93194e408341971742f4701772a025a # password
type: 0 # individual type: 0 # individual
@ -37,7 +37,7 @@
id: 3 id: 3
lower_name: user3 lower_name: user3
name: user3 name: user3
full_name: User Three full_name: " <<<< >> >> > >> > >>> >> "
email: user3@example.com email: user3@example.com
passwd: 7d93daa0d1e6f2305cc8fa496847d61dc7320bb16262f9c55dd753480207234cdd96a93194e408341971742f4701772a025a # password passwd: 7d93daa0d1e6f2305cc8fa496847d61dc7320bb16262f9c55dd753480207234cdd96a93194e408341971742f4701772a025a # password
type: 1 # organization type: 1 # organization
@ -53,7 +53,7 @@
id: 4 id: 4
lower_name: user4 lower_name: user4
name: user4 name: user4
full_name: User Four full_name: " "
email: user4@example.com email: user4@example.com
passwd: 7d93daa0d1e6f2305cc8fa496847d61dc7320bb16262f9c55dd753480207234cdd96a93194e408341971742f4701772a025a # password passwd: 7d93daa0d1e6f2305cc8fa496847d61dc7320bb16262f9c55dd753480207234cdd96a93194e408341971742f4701772a025a # password
type: 0 # individual type: 0 # individual

View file

@ -99,7 +99,7 @@ func TestIssueReactionCount(t *testing.T) {
reactions := issue1.Reactions.GroupByType() reactions := issue1.Reactions.GroupByType()
assert.Len(t, reactions["heart"], 4) assert.Len(t, reactions["heart"], 4)
assert.Equal(t, 2, reactions["heart"].GetMoreUserCount()) 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.True(t, reactions["heart"].HasUser(1))
assert.False(t, reactions["heart"].HasUser(5)) assert.False(t, reactions["heart"].HasUser(5))
assert.False(t, reactions["heart"].HasUser(0)) assert.False(t, reactions["heart"].HasUser(0))

View file

@ -417,7 +417,7 @@ func (u *User) GetFollowing(page int) ([]*User, error) {
// NewGitSig generates and returns the signature of given user. // NewGitSig generates and returns the signature of given user.
func (u *User) NewGitSig() *git.Signature { func (u *User) NewGitSig() *git.Signature {
return &git.Signature{ return &git.Signature{
Name: u.DisplayName(), Name: u.GitName(),
Email: u.getEmail(), Email: u.getEmail(),
When: time.Now(), When: time.Now(),
} }
@ -630,12 +630,33 @@ func (u *User) GetOrganizations(all bool) error {
// DisplayName returns full name if it's not empty, // DisplayName returns full name if it's not empty,
// returns username otherwise. // returns username otherwise.
func (u *User) DisplayName() string { func (u *User) DisplayName() string {
if len(u.FullName) > 0 { trimmed := strings.TrimSpace(u.FullName)
return u.FullName if len(trimmed) > 0 {
return trimmed
} }
return u.Name 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 // ShortName ellipses username to length
func (u *User) ShortName(length int) string { func (u *User) ShortName(length int) string {
return base.EllipsisString(u.Name, length) return base.EllipsisString(u.Name, length)

View file

@ -6,6 +6,7 @@ package models
import ( import (
"math/rand" "math/rand"
"strings"
"testing" "testing"
"code.gitea.io/gitea/modules/setting" "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 // User 5's team has no access to any repo
assert.Len(t, accessibleRepos, 0) 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)
}
}