From 2900dc90a792204a02f4a249399f221d3f9b9c52 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Fri, 4 Nov 2022 17:04:08 +0800 Subject: [PATCH] Improve valid user name check (#20136) Close https://github.com/go-gitea/gitea/issues/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 Co-authored-by: Lunny Xiao Co-authored-by: delvh --- models/user/user.go | 3 ++- modules/structs/admin_user.go | 2 +- modules/validation/binding.go | 20 ++++++++++++++++ modules/validation/helpers.go | 12 ++++++++++ modules/validation/helpers_test.go | 31 +++++++++++++++++++++++++ modules/web/middleware/binding.go | 2 ++ options/locale/locale_en-US.ini | 1 + services/forms/admin.go | 4 ++-- services/forms/org.go | 4 ++-- services/forms/user_form.go | 6 ++--- services/forms/user_form_auth_openid.go | 2 +- tests/integration/user_test.go | 22 ++++++++++++++---- 12 files changed, 95 insertions(+), 14 deletions(-) diff --git a/models/user/user.go b/models/user/user.go index 9a2da6dbc..84e2c4a9c 100644 --- a/models/user/user.go +++ b/models/user/user.go @@ -29,6 +29,7 @@ import ( "code.gitea.io/gitea/modules/structs" "code.gitea.io/gitea/modules/timeutil" "code.gitea.io/gitea/modules/util" + "code.gitea.io/gitea/modules/validation" "golang.org/x/crypto/argon2" "golang.org/x/crypto/bcrypt" @@ -621,7 +622,7 @@ var ( // IsUsableUsername returns an error when a username is reserved func IsUsableUsername(name string) error { // Validate username make sure it satisfies requirement. - if db.AlphaDashDotPattern.MatchString(name) { + if !validation.IsValidUsername(name) { // Note: usually this error is normally caught up earlier in the UI return db.ErrNameCharsNotAllowed{Name: name} } diff --git a/modules/structs/admin_user.go b/modules/structs/admin_user.go index eccbf29a4..2f6f502af 100644 --- a/modules/structs/admin_user.go +++ b/modules/structs/admin_user.go @@ -10,7 +10,7 @@ type CreateUserOption struct { SourceID int64 `json:"source_id"` LoginName string `json:"login_name"` // required: true - Username string `json:"username" binding:"Required;AlphaDashDot;MaxSize(40)"` + Username string `json:"username" binding:"Required;Username;MaxSize(40)"` FullName string `json:"full_name" binding:"MaxSize(100)"` // required: true // swagger:strfmt email diff --git a/modules/validation/binding.go b/modules/validation/binding.go index f08f63242..c054fbe7b 100644 --- a/modules/validation/binding.go +++ b/modules/validation/binding.go @@ -24,6 +24,9 @@ const ( // ErrRegexPattern is returned when a regex pattern is invalid ErrRegexPattern = "RegexPattern" + + // ErrUsername is username error + ErrUsername = "UsernameError" ) // AddBindingRules adds additional binding rules @@ -34,6 +37,7 @@ func AddBindingRules() { addGlobPatternRule() addRegexPatternRule() addGlobOrRegexPatternRule() + addUsernamePatternRule() } func addGitRefNameBindingRule() { @@ -148,6 +152,22 @@ func addGlobOrRegexPatternRule() { }) } +func addUsernamePatternRule() { + binding.AddRule(&binding.Rule{ + IsMatch: func(rule string) bool { + return rule == "Username" + }, + IsValid: func(errs binding.Errors, name string, val interface{}) (bool, binding.Errors) { + str := fmt.Sprintf("%v", val) + if !IsValidUsername(str) { + errs.Add([]string{name}, ErrUsername, "invalid username") + return false, errs + } + return true, errs + }, + }) +} + func portOnly(hostport string) string { colon := strings.IndexByte(hostport, ':') if colon == -1 { diff --git a/modules/validation/helpers.go b/modules/validation/helpers.go index 484b12b2a..8e49c7855 100644 --- a/modules/validation/helpers.go +++ b/modules/validation/helpers.go @@ -91,3 +91,15 @@ func IsValidExternalTrackerURLFormat(uri string) bool { return true } + +var ( + validUsernamePattern = regexp.MustCompile(`^[\da-zA-Z][-.\w]*$`) + invalidUsernamePattern = regexp.MustCompile(`[-._]{2,}|[-._]$`) // No consecutive or trailing non-alphanumeric chars +) + +// IsValidUsername checks if username is valid +func IsValidUsername(name string) bool { + // It is difficult to find a single pattern that is both readable and effective, + // but it's easier to use positive and negative checks. + return validUsernamePattern.MatchString(name) && !invalidUsernamePattern.MatchString(name) +} diff --git a/modules/validation/helpers_test.go b/modules/validation/helpers_test.go index f6f897e82..9bdbdb4a7 100644 --- a/modules/validation/helpers_test.go +++ b/modules/validation/helpers_test.go @@ -155,3 +155,34 @@ func Test_IsValidExternalTrackerURLFormat(t *testing.T) { }) } } + +func TestIsValidUsername(t *testing.T) { + tests := []struct { + arg string + want bool + }{ + {arg: "a", want: true}, + {arg: "abc", want: true}, + {arg: "0.b-c", want: true}, + {arg: "a.b-c_d", want: true}, + {arg: "", want: false}, + {arg: ".abc", want: false}, + {arg: "abc.", want: false}, + {arg: "a..bc", want: false}, + {arg: "a...bc", want: false}, + {arg: "a.-bc", want: false}, + {arg: "a._bc", want: false}, + {arg: "a_-bc", want: false}, + {arg: "a/bc", want: false}, + {arg: "☁️", want: false}, + {arg: "-", want: false}, + {arg: "--diff", want: false}, + {arg: "-im-here", want: false}, + {arg: "a space", want: false}, + } + for _, tt := range tests { + t.Run(tt.arg, func(t *testing.T) { + assert.Equalf(t, tt.want, IsValidUsername(tt.arg), "IsValidUsername(%v)", tt.arg) + }) + } +} diff --git a/modules/web/middleware/binding.go b/modules/web/middleware/binding.go index 636e655b9..cced9717b 100644 --- a/modules/web/middleware/binding.go +++ b/modules/web/middleware/binding.go @@ -135,6 +135,8 @@ func Validate(errs binding.Errors, data map[string]interface{}, f Form, l transl data["ErrorMsg"] = trName + l.Tr("form.glob_pattern_error", errs[0].Message) case validation.ErrRegexPattern: data["ErrorMsg"] = trName + l.Tr("form.regex_pattern_error", errs[0].Message) + case validation.ErrUsername: + data["ErrorMsg"] = trName + l.Tr("form.username_error") default: msg := errs[0].Classification if msg != "" && errs[0].Message != "" { diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index 8cda25055..32da24b6b 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -463,6 +463,7 @@ url_error = `'%s' is not a valid URL.` include_error = ` must contain substring '%s'.` glob_pattern_error = ` glob pattern is invalid: %s.` regex_pattern_error = ` regex pattern is invalid: %s.` +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.` unknown_error = Unknown error: captcha_incorrect = The CAPTCHA code is incorrect. password_not_match = The passwords do not match. diff --git a/services/forms/admin.go b/services/forms/admin.go index 5abef0550..537b9f982 100644 --- a/services/forms/admin.go +++ b/services/forms/admin.go @@ -18,7 +18,7 @@ import ( type AdminCreateUserForm struct { LoginType string `binding:"Required"` LoginName string - UserName string `binding:"Required;AlphaDashDot;MaxSize(40)"` + UserName string `binding:"Required;Username;MaxSize(40)"` Email string `binding:"Required;Email;MaxSize(254)"` Password string `binding:"MaxSize(255)"` SendNotify bool @@ -35,7 +35,7 @@ func (f *AdminCreateUserForm) Validate(req *http.Request, errs binding.Errors) b // AdminEditUserForm form for admin to create user type AdminEditUserForm struct { LoginType string `binding:"Required"` - UserName string `binding:"AlphaDashDot;MaxSize(40)"` + UserName string `binding:"Username;MaxSize(40)"` LoginName string FullName string `binding:"MaxSize(100)"` Email string `binding:"Required;Email;MaxSize(254)"` diff --git a/services/forms/org.go b/services/forms/org.go index dec2dbfa6..c7ee91134 100644 --- a/services/forms/org.go +++ b/services/forms/org.go @@ -24,7 +24,7 @@ import ( // CreateOrgForm form for creating organization type CreateOrgForm struct { - OrgName string `binding:"Required;AlphaDashDot;MaxSize(40)" locale:"org.org_name_holder"` + OrgName string `binding:"Required;Username;MaxSize(40)" locale:"org.org_name_holder"` Visibility structs.VisibleType RepoAdminChangeTeamAccess bool } @@ -37,7 +37,7 @@ func (f *CreateOrgForm) Validate(req *http.Request, errs binding.Errors) binding // UpdateOrgSettingForm form for updating organization settings type UpdateOrgSettingForm struct { - Name string `binding:"Required;AlphaDashDot;MaxSize(40)" locale:"org.org_name_holder"` + Name string `binding:"Required;Username;MaxSize(40)" locale:"org.org_name_holder"` FullName string `binding:"MaxSize(100)"` Description string `binding:"MaxSize(255)"` Website string `binding:"ValidUrl;MaxSize(255)"` diff --git a/services/forms/user_form.go b/services/forms/user_form.go index 95e4f9ed0..ed8ccf12e 100644 --- a/services/forms/user_form.go +++ b/services/forms/user_form.go @@ -65,7 +65,7 @@ type InstallForm struct { PasswordAlgorithm string - AdminName string `binding:"OmitEmpty;AlphaDashDot;MaxSize(30)" locale:"install.admin_name"` + AdminName string `binding:"OmitEmpty;Username;MaxSize(30)" locale:"install.admin_name"` AdminPasswd string `binding:"OmitEmpty;MaxSize(255)" locale:"install.admin_password"` AdminConfirmPasswd string 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 // RegisterForm form for registering type RegisterForm struct { - UserName string `binding:"Required;AlphaDashDot;MaxSize(40)"` + UserName string `binding:"Required;Username;MaxSize(40)"` Email string `binding:"Required;MaxSize(254)"` Password string `binding:"MaxSize(255)"` Retype string @@ -243,7 +243,7 @@ func (f *IntrospectTokenForm) Validate(req *http.Request, errs binding.Errors) b // UpdateProfileForm form for updating profile type UpdateProfileForm struct { - Name string `binding:"AlphaDashDot;MaxSize(40)"` + Name string `binding:"Username;MaxSize(40)"` FullName string `binding:"MaxSize(100)"` KeepEmailPrivate bool Website string `binding:"ValidSiteUrl;MaxSize(255)"` diff --git a/services/forms/user_form_auth_openid.go b/services/forms/user_form_auth_openid.go index 992517f34..d1ed0a23c 100644 --- a/services/forms/user_form_auth_openid.go +++ b/services/forms/user_form_auth_openid.go @@ -27,7 +27,7 @@ func (f *SignInOpenIDForm) Validate(req *http.Request, errs binding.Errors) bind // SignUpOpenIDForm form for signin up with OpenID type SignUpOpenIDForm struct { - UserName string `binding:"Required;AlphaDashDot;MaxSize(40)"` + UserName string `binding:"Required;Username;MaxSize(40)"` Email string `binding:"Required;Email;MaxSize(254)"` GRecaptchaResponse string `form:"g-recaptcha-response"` HcaptchaResponse string `form:"h-captcha-response"` diff --git a/tests/integration/user_test.go b/tests/integration/user_test.go index 110f5c89b..017700ad4 100644 --- a/tests/integration/user_test.go +++ b/tests/integration/user_test.go @@ -53,6 +53,22 @@ func TestRenameInvalidUsername(t *testing.T) { "%00", "thisHas ASpace", "ptho>lo