Skip to content

Commit 2900dc9

Browse files
wxiaoguangwolfogrelunnydelvh
authored
Improve valid user name check (#20136)
Close #21640 Before: Gitea can create users like ".xxx" or "x..y", which is not ideal, it's already a consensus that dot filenames have special meanings, and `a..b` is a confusing name when doing cross repo compare. After: stricter Co-authored-by: Jason Song <i@wolfogre.com> Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com> Co-authored-by: delvh <dev.lh@web.de>
1 parent 4c6b4a6 commit 2900dc9

File tree

12 files changed

+95
-14
lines changed

12 files changed

+95
-14
lines changed

Diff for: ‎models/user/user.go

+2-1
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ import (
2929
"code.gitea.io/gitea/modules/structs"
3030
"code.gitea.io/gitea/modules/timeutil"
3131
"code.gitea.io/gitea/modules/util"
32+
"code.gitea.io/gitea/modules/validation"
3233

3334
"golang.org/x/crypto/argon2"
3435
"golang.org/x/crypto/bcrypt"
@@ -621,7 +622,7 @@ var (
621622
// IsUsableUsername returns an error when a username is reserved
622623
func IsUsableUsername(name string) error {
623624
// Validate username make sure it satisfies requirement.
624-
if db.AlphaDashDotPattern.MatchString(name) {
625+
if !validation.IsValidUsername(name) {
625626
// Note: usually this error is normally caught up earlier in the UI
626627
return db.ErrNameCharsNotAllowed{Name: name}
627628
}

Diff for: ‎modules/structs/admin_user.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ type CreateUserOption struct {
1010
SourceID int64 `json:"source_id"`
1111
LoginName string `json:"login_name"`
1212
// required: true
13-
Username string `json:"username" binding:"Required;AlphaDashDot;MaxSize(40)"`
13+
Username string `json:"username" binding:"Required;Username;MaxSize(40)"`
1414
FullName string `json:"full_name" binding:"MaxSize(100)"`
1515
// required: true
1616
// swagger:strfmt email

Diff for: ‎modules/validation/binding.go

+20
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,9 @@ const (
2424

2525
// ErrRegexPattern is returned when a regex pattern is invalid
2626
ErrRegexPattern = "RegexPattern"
27+
28+
// ErrUsername is username error
29+
ErrUsername = "UsernameError"
2730
)
2831

2932
// AddBindingRules adds additional binding rules
@@ -34,6 +37,7 @@ func AddBindingRules() {
3437
addGlobPatternRule()
3538
addRegexPatternRule()
3639
addGlobOrRegexPatternRule()
40+
addUsernamePatternRule()
3741
}
3842

3943
func addGitRefNameBindingRule() {
@@ -148,6 +152,22 @@ func addGlobOrRegexPatternRule() {
148152
})
149153
}
150154

155+
func addUsernamePatternRule() {
156+
binding.AddRule(&binding.Rule{
157+
IsMatch: func(rule string) bool {
158+
return rule == "Username"
159+
},
160+
IsValid: func(errs binding.Errors, name string, val interface{}) (bool, binding.Errors) {
161+
str := fmt.Sprintf("%v", val)
162+
if !IsValidUsername(str) {
163+
errs.Add([]string{name}, ErrUsername, "invalid username")
164+
return false, errs
165+
}
166+
return true, errs
167+
},
168+
})
169+
}
170+
151171
func portOnly(hostport string) string {
152172
colon := strings.IndexByte(hostport, ':')
153173
if colon == -1 {

Diff for: ‎modules/validation/helpers.go

+12
Original file line numberDiff line numberDiff line change
@@ -91,3 +91,15 @@ func IsValidExternalTrackerURLFormat(uri string) bool {
9191

9292
return true
9393
}
94+
95+
var (
96+
validUsernamePattern = regexp.MustCompile(`^[\da-zA-Z][-.\w]*$`)
97+
invalidUsernamePattern = regexp.MustCompile(`[-._]{2,}|[-._]$`) // No consecutive or trailing non-alphanumeric chars
98+
)
99+
100+
// IsValidUsername checks if username is valid
101+
func IsValidUsername(name string) bool {
102+
// It is difficult to find a single pattern that is both readable and effective,
103+
// but it's easier to use positive and negative checks.
104+
return validUsernamePattern.MatchString(name) && !invalidUsernamePattern.MatchString(name)
105+
}

Diff for: ‎modules/validation/helpers_test.go

+31
Original file line numberDiff line numberDiff line change
@@ -155,3 +155,34 @@ func Test_IsValidExternalTrackerURLFormat(t *testing.T) {
155155
})
156156
}
157157
}
158+
159+
func TestIsValidUsername(t *testing.T) {
160+
tests := []struct {
161+
arg string
162+
want bool
163+
}{
164+
{arg: "a", want: true},
165+
{arg: "abc", want: true},
166+
{arg: "0.b-c", want: true},
167+
{arg: "a.b-c_d", want: true},
168+
{arg: "", want: false},
169+
{arg: ".abc", want: false},
170+
{arg: "abc.", want: false},
171+
{arg: "a..bc", want: false},
172+
{arg: "a...bc", want: false},
173+
{arg: "a.-bc", want: false},
174+
{arg: "a._bc", want: false},
175+
{arg: "a_-bc", want: false},
176+
{arg: "a/bc", want: false},
177+
{arg: "☁️", want: false},
178+
{arg: "-", want: false},
179+
{arg: "--diff", want: false},
180+
{arg: "-im-here", want: false},
181+
{arg: "a space", want: false},
182+
}
183+
for _, tt := range tests {
184+
t.Run(tt.arg, func(t *testing.T) {
185+
assert.Equalf(t, tt.want, IsValidUsername(tt.arg), "IsValidUsername(%v)", tt.arg)
186+
})
187+
}
188+
}

Diff for: ‎modules/web/middleware/binding.go

+2
Original file line numberDiff line numberDiff line change
@@ -135,6 +135,8 @@ func Validate(errs binding.Errors, data map[string]interface{}, f Form, l transl
135135
data["ErrorMsg"] = trName + l.Tr("form.glob_pattern_error", errs[0].Message)
136136
case validation.ErrRegexPattern:
137137
data["ErrorMsg"] = trName + l.Tr("form.regex_pattern_error", errs[0].Message)
138+
case validation.ErrUsername:
139+
data["ErrorMsg"] = trName + l.Tr("form.username_error")
138140
default:
139141
msg := errs[0].Classification
140142
if msg != "" && errs[0].Message != "" {

Diff for: ‎options/locale/locale_en-US.ini

+1
Original file line numberDiff line numberDiff line change
@@ -463,6 +463,7 @@ url_error = `'%s' is not a valid URL.`
463463
include_error = ` must contain substring '%s'.`
464464
glob_pattern_error = ` glob pattern is invalid: %s.`
465465
regex_pattern_error = ` regex pattern is invalid: %s.`
466+
username_error = ` can only contain alphanumeric chars ('0-9','a-z','A-Z'), dash ('-'), underscore ('_') and dot ('.'). It cannot begin or end with non-alphanumeric chars, and consecutive non-alphanumeric chars are also forbidden.`
466467
unknown_error = Unknown error:
467468
captcha_incorrect = The CAPTCHA code is incorrect.
468469
password_not_match = The passwords do not match.

Diff for: ‎services/forms/admin.go

+2-2
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ import (
1818
type AdminCreateUserForm struct {
1919
LoginType string `binding:"Required"`
2020
LoginName string
21-
UserName string `binding:"Required;AlphaDashDot;MaxSize(40)"`
21+
UserName string `binding:"Required;Username;MaxSize(40)"`
2222
Email string `binding:"Required;Email;MaxSize(254)"`
2323
Password string `binding:"MaxSize(255)"`
2424
SendNotify bool
@@ -35,7 +35,7 @@ func (f *AdminCreateUserForm) Validate(req *http.Request, errs binding.Errors) b
3535
// AdminEditUserForm form for admin to create user
3636
type AdminEditUserForm struct {
3737
LoginType string `binding:"Required"`
38-
UserName string `binding:"AlphaDashDot;MaxSize(40)"`
38+
UserName string `binding:"Username;MaxSize(40)"`
3939
LoginName string
4040
FullName string `binding:"MaxSize(100)"`
4141
Email string `binding:"Required;Email;MaxSize(254)"`

Diff for: ‎services/forms/org.go

+2-2
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ import (
2424

2525
// CreateOrgForm form for creating organization
2626
type CreateOrgForm struct {
27-
OrgName string `binding:"Required;AlphaDashDot;MaxSize(40)" locale:"org.org_name_holder"`
27+
OrgName string `binding:"Required;Username;MaxSize(40)" locale:"org.org_name_holder"`
2828
Visibility structs.VisibleType
2929
RepoAdminChangeTeamAccess bool
3030
}
@@ -37,7 +37,7 @@ func (f *CreateOrgForm) Validate(req *http.Request, errs binding.Errors) binding
3737

3838
// UpdateOrgSettingForm form for updating organization settings
3939
type UpdateOrgSettingForm struct {
40-
Name string `binding:"Required;AlphaDashDot;MaxSize(40)" locale:"org.org_name_holder"`
40+
Name string `binding:"Required;Username;MaxSize(40)" locale:"org.org_name_holder"`
4141
FullName string `binding:"MaxSize(100)"`
4242
Description string `binding:"MaxSize(255)"`
4343
Website string `binding:"ValidUrl;MaxSize(255)"`

Diff for: ‎services/forms/user_form.go

+3-3
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,7 @@ type InstallForm struct {
6565

6666
PasswordAlgorithm string
6767

68-
AdminName string `binding:"OmitEmpty;AlphaDashDot;MaxSize(30)" locale:"install.admin_name"`
68+
AdminName string `binding:"OmitEmpty;Username;MaxSize(30)" locale:"install.admin_name"`
6969
AdminPasswd string `binding:"OmitEmpty;MaxSize(255)" locale:"install.admin_password"`
7070
AdminConfirmPasswd string
7171
AdminEmail string `binding:"OmitEmpty;MinSize(3);MaxSize(254);Include(@)" locale:"install.admin_email"`
@@ -91,7 +91,7 @@ func (f *InstallForm) Validate(req *http.Request, errs binding.Errors) binding.E
9191

9292
// RegisterForm form for registering
9393
type RegisterForm struct {
94-
UserName string `binding:"Required;AlphaDashDot;MaxSize(40)"`
94+
UserName string `binding:"Required;Username;MaxSize(40)"`
9595
Email string `binding:"Required;MaxSize(254)"`
9696
Password string `binding:"MaxSize(255)"`
9797
Retype string
@@ -243,7 +243,7 @@ func (f *IntrospectTokenForm) Validate(req *http.Request, errs binding.Errors) b
243243

244244
// UpdateProfileForm form for updating profile
245245
type UpdateProfileForm struct {
246-
Name string `binding:"AlphaDashDot;MaxSize(40)"`
246+
Name string `binding:"Username;MaxSize(40)"`
247247
FullName string `binding:"MaxSize(100)"`
248248
KeepEmailPrivate bool
249249
Website string `binding:"ValidSiteUrl;MaxSize(255)"`

Diff for: ‎services/forms/user_form_auth_openid.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ func (f *SignInOpenIDForm) Validate(req *http.Request, errs binding.Errors) bind
2727

2828
// SignUpOpenIDForm form for signin up with OpenID
2929
type SignUpOpenIDForm struct {
30-
UserName string `binding:"Required;AlphaDashDot;MaxSize(40)"`
30+
UserName string `binding:"Required;Username;MaxSize(40)"`
3131
Email string `binding:"Required;Email;MaxSize(254)"`
3232
GRecaptchaResponse string `form:"g-recaptcha-response"`
3333
HcaptchaResponse string `form:"h-captcha-response"`

Diff for: ‎tests/integration/user_test.go

+18-4
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,22 @@ func TestRenameInvalidUsername(t *testing.T) {
5353
"%00",
5454
"thisHas ASpace",
5555
"p<A>tho>lo<gical",
56+
".",
57+
"..",
58+
".well-known",
59+
".abc",
60+
"abc.",
61+
"a..bc",
62+
"a...bc",
63+
"a.-bc",
64+
"a._bc",
65+
"a_-bc",
66+
"a/bc",
67+
"☁️",
68+
"-",
69+
"--diff",
70+
"-im-here",
71+
"a space",
5672
}
5773

5874
session := loginUser(t, "user2")
@@ -68,7 +84,7 @@ func TestRenameInvalidUsername(t *testing.T) {
6884
htmlDoc := NewHTMLParser(t, resp.Body)
6985
assert.Contains(t,
7086
htmlDoc.doc.Find(".ui.negative.message").Text(),
71-
translation.NewLocale("en-US").Tr("form.alpha_dash_dot_error"),
87+
translation.NewLocale("en-US").Tr("form.username_error"),
7288
)
7389

7490
unittest.AssertNotExistsBean(t, &user_model.User{Name: invalidUsername})
@@ -79,9 +95,7 @@ func TestRenameReservedUsername(t *testing.T) {
7995
defer tests.PrepareTestEnv(t)()
8096

8197
reservedUsernames := []string{
82-
".",
83-
"..",
84-
".well-known",
98+
// ".", "..", ".well-known", // The names are not only reserved but also invalid
8599
"admin",
86100
"api",
87101
"assets",

0 commit comments

Comments
 (0)