aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorwxiaoguang <wxiaoguang@gmail.com>2022-11-04 17:04:08 +0800
committerGitHub <noreply@github.com>2022-11-04 17:04:08 +0800
commit2900dc90a792204a02f4a249399f221d3f9b9c52 (patch)
tree84758fc47a0b8a76bd56c061b72eb0d869e9b1c3
parent4c6b4a67d9cc5c10c5f40a2420ffc96a6bd9517a (diff)
downloadgitea-2900dc90a792204a02f4a249399f221d3f9b9c52.tar.gz
gitea-2900dc90a792204a02f4a249399f221d3f9b9c52.zip
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 <i@wolfogre.com> Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com> Co-authored-by: delvh <dev.lh@web.de>
-rw-r--r--models/user/user.go3
-rw-r--r--modules/structs/admin_user.go2
-rw-r--r--modules/validation/binding.go20
-rw-r--r--modules/validation/helpers.go12
-rw-r--r--modules/validation/helpers_test.go31
-rw-r--r--modules/web/middleware/binding.go2
-rw-r--r--options/locale/locale_en-US.ini1
-rw-r--r--services/forms/admin.go4
-rw-r--r--services/forms/org.go4
-rw-r--r--services/forms/user_form.go6
-rw-r--r--services/forms/user_form_auth_openid.go2
-rw-r--r--tests/integration/user_test.go22
12 files changed, 95 insertions, 14 deletions
diff --git a/models/user/user.go b/models/user/user.go
index 9a2da6dbc1..84e2c4a9cc 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 eccbf29a46..2f6f502af6 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 f08f632426..c054fbe7b6 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 484b12b2a2..8e49c7855e 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 f6f897e821..9bdbdb4a77 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 636e655b9e..cced9717be 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 8cda250559..32da24b6b0 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 5abef0550e..537b9f982c 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 dec2dbfa65..c7ee911345 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 95e4f9ed0e..ed8ccf12ea 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 992517f34f..d1ed0a23c7 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 110f5c89bf..017700ad40 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",
"p<A>tho>lo<gical",
+ ".",
+ "..",
+ ".well-known",
+ ".abc",
+ "abc.",
+ "a..bc",
+ "a...bc",
+ "a.-bc",
+ "a._bc",
+ "a_-bc",
+ "a/bc",
+ "☁️",
+ "-",
+ "--diff",
+ "-im-here",
+ "a space",
}
session := loginUser(t, "user2")
@@ -68,7 +84,7 @@ func TestRenameInvalidUsername(t *testing.T) {
htmlDoc := NewHTMLParser(t, resp.Body)
assert.Contains(t,
htmlDoc.doc.Find(".ui.negative.message").Text(),
- translation.NewLocale("en-US").Tr("form.alpha_dash_dot_error"),
+ translation.NewLocale("en-US").Tr("form.username_error"),
)
unittest.AssertNotExistsBean(t, &user_model.User{Name: invalidUsername})
@@ -79,9 +95,7 @@ func TestRenameReservedUsername(t *testing.T) {
defer tests.PrepareTestEnv(t)()
reservedUsernames := []string{
- ".",
- "..",
- ".well-known",
+ // ".", "..", ".well-known", // The names are not only reserved but also invalid
"admin",
"api",
"assets",