Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve valid user name check #20136

Merged
merged 23 commits into from
Nov 4, 2022
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion models/user/user.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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}
}
Expand Down
2 changes: 1 addition & 1 deletion modules/structs/admin_user.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
20 changes: 20 additions & 0 deletions modules/validation/binding.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -34,6 +37,7 @@ func AddBindingRules() {
addGlobPatternRule()
addRegexPatternRule()
addGlobOrRegexPatternRule()
addUsernamePatternRule()
}

func addGitRefNameBindingRule() {
Expand Down Expand Up @@ -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 {
Expand Down
12 changes: 12 additions & 0 deletions modules/validation/helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -91,3 +91,15 @@ func IsValidExternalTrackerURLFormat(uri string) bool {

return true
}

var (
validUsernamePattern = regexp.MustCompile(`^[\da-zA-Z][-.\w]*$`)
invalidUsernamePattern = regexp.MustCompile(`[-._]{2,}|[-._]$`)
wxiaoguang marked this conversation as resolved.
Show resolved Hide resolved
wolfogre marked this conversation as resolved.
Show resolved Hide resolved
)

// 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)
}
27 changes: 27 additions & 0 deletions modules/validation/helpers_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -155,3 +155,30 @@ 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},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think edge case tests such as "-", "--diff", "-im-here", "a space" would also be a good idea in this case.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

{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},
}
for _, tt := range tests {
t.Run(tt.arg, func(t *testing.T) {
assert.Equalf(t, tt.want, IsValidUsername(tt.arg), "IsValidUsername(%v)", tt.arg)
})
}
}
2 changes: 2 additions & 0 deletions modules/web/middleware/binding.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 != "" {
Expand Down
1 change: 1 addition & 0 deletions options/locale/locale_en-US.ini
Original file line number Diff line number Diff line change
Expand Up @@ -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 = ` should contain only alphanumeric, dash ('-'), underscore ('_') and dot ('.') characters, and cannot begin or end with non-alphanumeric, consecutive non-alphanumerics are also not allowed.`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the other translations mention explicitly the faulty type, should we perhaps prepend username?

Suggested change
username_error = ` should contain only alphanumeric, dash ('-'), underscore ('_') and dot ('.') characters, and cannot begin or end with non-alphanumeric, consecutive non-alphanumerics are also not allowed.`
username_error = ` username 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.`

Copy link
Member

@wolfogre wolfogre Nov 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll take your suggestion, but without username.

The subject can be Username or Organization Name.

image

image

unknown_error = Unknown error:
captcha_incorrect = The CAPTCHA code is incorrect.
password_not_match = The passwords do not match.
Expand Down
4 changes: 2 additions & 2 deletions services/forms/admin.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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)"`
Expand Down
4 changes: 2 additions & 2 deletions services/forms/org.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand All @@ -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)"`
Expand Down
6 changes: 3 additions & 3 deletions services/forms/user_form.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"`
Expand All @@ -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
Expand Down Expand Up @@ -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)"`
Expand Down
2 changes: 1 addition & 1 deletion services/forms/user_form_auth_openid.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"`
Expand Down
18 changes: 14 additions & 4 deletions tests/integration/user_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,18 @@ 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",
"☁️",
}

session := loginUser(t, "user2")
Expand All @@ -68,7 +80,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})
Expand All @@ -79,9 +91,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",
Expand Down