From 31655aabfc397db203d39b468cad1ecbdc1879db Mon Sep 17 00:00:00 2001 From: guillep2k <18600385+guillep2k@users.noreply.github.com> Date: Wed, 16 Oct 2019 00:09:58 -0300 Subject: [PATCH] Fix password complexity regex for special characters (on master) (#8525) * Fix extra space * Fix regular expression * Fix error template name * Simplify check code, fix default values, add test * Fix router tests * Fix fmt * Fix setting and lint * Move cleaning up code to test, improve comments * Tidy up variable declaration --- custom/conf/app.ini.sample | 3 +- .../doc/advanced/config-cheat-sheet.en-us.md | 3 +- modules/password/password.go | 63 ++++++++++------ modules/password/password_test.go | 75 +++++++++++++++++++ modules/setting/setting.go | 24 ++---- options/locale/locale_en-US.ini | 2 +- routers/admin/users_test.go | 4 +- routers/user/setting/account.go | 2 +- routers/user/setting/account_test.go | 36 +++------ 9 files changed, 140 insertions(+), 72 deletions(-) create mode 100644 modules/password/password_test.go diff --git a/custom/conf/app.ini.sample b/custom/conf/app.ini.sample index 08a2c8e32..aa526804f 100644 --- a/custom/conf/app.ini.sample +++ b/custom/conf/app.ini.sample @@ -345,7 +345,8 @@ IMPORT_LOCAL_PATHS = false ; Set to true to prevent all users (including admin) from creating custom git hooks DISABLE_GIT_HOOKS = false ;Comma separated list of character classes required to pass minimum complexity. -;If left empty or no valid values are specified, the default values (`lower,upper,digit,spec`) will be used. +;If left empty or no valid values are specified, the default values ("lower,upper,digit,spec") will be used. +;Use "off" to disable checking. PASSWORD_COMPLEXITY = lower,upper,digit,spec ; Password Hash algorithm, either "pbkdf2", "argon2", "scrypt" or "bcrypt" PASSWORD_HASH_ALGO = pbkdf2 diff --git a/docs/content/doc/advanced/config-cheat-sheet.en-us.md b/docs/content/doc/advanced/config-cheat-sheet.en-us.md index 6fb92f272..959607dd1 100644 --- a/docs/content/doc/advanced/config-cheat-sheet.en-us.md +++ b/docs/content/doc/advanced/config-cheat-sheet.en-us.md @@ -219,7 +219,8 @@ Values containing `#` or `;` must be quoted using `` ` `` or `"""`. - lower - use one or more lower latin characters - upper - use one or more upper latin characters - digit - use one or more digits - - spec - use one or more special characters as ``][!"#$%&'()*+,./:;<=>?@\^_{|}~`-`` and space symbol. + - spec - use one or more special characters as ``!"#$%&'()*+,-./:;<=>?@[\\]^_`{|}~`` + - off - do not check password complexity ## OpenID (`openid`) diff --git a/modules/password/password.go b/modules/password/password.go index 54131b964..92986977e 100644 --- a/modules/password/password.go +++ b/modules/password/password.go @@ -7,45 +7,60 @@ package password import ( "crypto/rand" "math/big" - "regexp" + "strings" "sync" "code.gitea.io/gitea/modules/setting" ) -var matchComplexities = map[string]regexp.Regexp{} -var matchComplexityOnce sync.Once -var validChars string -var validComplexities = map[string]string{ - "lower": "abcdefghijklmnopqrstuvwxyz", - "upper": "ABCDEFGHIJKLMNOPQRSTUVWXYZ", - "digit": "0123456789", - "spec": `][ !"#$%&'()*+,./:;<=>?@\^_{|}~` + "`-", -} +var ( + matchComplexityOnce sync.Once + validChars string + requiredChars []string + + charComplexities = map[string]string{ + "lower": `abcdefghijklmnopqrstuvwxyz`, + "upper": `ABCDEFGHIJKLMNOPQRSTUVWXYZ`, + "digit": `0123456789`, + "spec": ` !"#$%&'()*+,-./:;<=>?@[\]^_{|}~` + "`", + } +) // NewComplexity for preparation func NewComplexity() { matchComplexityOnce.Do(func() { - if len(setting.PasswordComplexity) > 0 { - for key, val := range setting.PasswordComplexity { - matchComplexity := regexp.MustCompile(val) - matchComplexities[key] = *matchComplexity - validChars += validComplexities[key] + setupComplexity(setting.PasswordComplexity) + }) +} + +func setupComplexity(values []string) { + if len(values) != 1 || values[0] != "off" { + for _, val := range values { + if chars, ok := charComplexities[val]; ok { + validChars += chars + requiredChars = append(requiredChars, chars) } - } else { - for _, val := range validComplexities { - validChars += val + } + if len(requiredChars) == 0 { + // No valid character classes found; use all classes as default + for _, chars := range charComplexities { + validChars += chars + requiredChars = append(requiredChars, chars) } } - }) + } + if validChars == "" { + // No complexities to check; provide a sensible default for password generation + validChars = charComplexities["lower"] + charComplexities["upper"] + charComplexities["digit"] + } } -// IsComplexEnough return True if password is Complexity +// IsComplexEnough return True if password meets complexity settings func IsComplexEnough(pwd string) bool { - if len(setting.PasswordComplexity) > 0 { - NewComplexity() - for _, val := range matchComplexities { - if !val.MatchString(pwd) { + NewComplexity() + if len(validChars) > 0 { + for _, req := range requiredChars { + if !strings.ContainsAny(req, pwd) { return false } } diff --git a/modules/password/password_test.go b/modules/password/password_test.go new file mode 100644 index 000000000..d46a6d157 --- /dev/null +++ b/modules/password/password_test.go @@ -0,0 +1,75 @@ +// Copyright 2019 The Gitea Authors. All rights reserved. +// Use of this source code is governed by a MIT-style +// license that can be found in the LICENSE file. + +package password + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestComplexity_IsComplexEnough(t *testing.T) { + matchComplexityOnce.Do(func() {}) + + testlist := []struct { + complexity []string + truevalues []string + falsevalues []string + }{ + {[]string{"lower"}, []string{"abc", "abc!"}, []string{"ABC", "123", "=!$", ""}}, + {[]string{"upper"}, []string{"ABC"}, []string{"abc", "123", "=!$", "abc!", ""}}, + {[]string{"digit"}, []string{"123"}, []string{"abc", "ABC", "=!$", "abc!", ""}}, + {[]string{"spec"}, []string{"=!$", "abc!"}, []string{"abc", "ABC", "123", ""}}, + {[]string{"off"}, []string{"abc", "ABC", "123", "=!$", "abc!", ""}, nil}, + {[]string{"lower", "spec"}, []string{"abc!"}, []string{"abc", "ABC", "123", "=!$", "abcABC123", ""}}, + {[]string{"lower", "upper", "digit"}, []string{"abcABC123"}, []string{"abc", "ABC", "123", "=!$", "abc!", ""}}, + } + + for _, test := range testlist { + testComplextity(test.complexity) + for _, val := range test.truevalues { + assert.True(t, IsComplexEnough(val)) + } + for _, val := range test.falsevalues { + assert.False(t, IsComplexEnough(val)) + } + } + + // Remove settings for other tests + testComplextity([]string{"off"}) +} + +func TestComplexity_Generate(t *testing.T) { + matchComplexityOnce.Do(func() {}) + + const maxCount = 50 + const pwdLen = 50 + + test := func(t *testing.T, modes []string) { + testComplextity(modes) + for i := 0; i < maxCount; i++ { + pwd, err := Generate(pwdLen) + assert.NoError(t, err) + assert.Equal(t, pwdLen, len(pwd)) + assert.True(t, IsComplexEnough(pwd), "Failed complexities with modes %+v for generated: %s", modes, pwd) + } + } + + test(t, []string{"lower"}) + test(t, []string{"upper"}) + test(t, []string{"lower", "upper", "spec"}) + test(t, []string{"off"}) + test(t, []string{""}) + + // Remove settings for other tests + testComplextity([]string{"off"}) +} + +func testComplextity(values []string) { + // Cleanup previous values + validChars = "" + requiredChars = make([]string, 0, len(values)) + setupComplexity(values) +} diff --git a/modules/setting/setting.go b/modules/setting/setting.go index afcb943d0..d05c52fea 100644 --- a/modules/setting/setting.go +++ b/modules/setting/setting.go @@ -149,7 +149,7 @@ var ( MinPasswordLength int ImportLocalPaths bool DisableGitHooks bool - PasswordComplexity map[string]string + PasswordComplexity []string PasswordHashAlgo string // UI settings @@ -781,26 +781,14 @@ func NewContext() { InternalToken = loadInternalToken(sec) - var dictPC = map[string]string{ - "lower": "[a-z]+", - "upper": "[A-Z]+", - "digit": "[0-9]+", - "spec": `][ !"#$%&'()*+,./:;<=>?@\\^_{|}~` + "`-", - } - PasswordComplexity = make(map[string]string) cfgdata := sec.Key("PASSWORD_COMPLEXITY").Strings(",") - for _, y := range cfgdata { - ts := strings.TrimSpace(y) - for a := range dictPC { - if strings.ToLower(ts) == a { - PasswordComplexity[ts] = dictPC[ts] - break - } + PasswordComplexity = make([]string, 0, len(cfgdata)) + for _, name := range cfgdata { + name := strings.ToLower(strings.Trim(name, `"`)) + if name != "" { + PasswordComplexity = append(PasswordComplexity, name) } } - if len(PasswordComplexity) == 0 { - PasswordComplexity = dictPC - } sec = Cfg.Section("attachment") AttachmentPath = sec.Key("PATH").MustString(path.Join(AppDataPath, "attachments")) diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index 45b86dea9..76a1daa45 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -315,7 +315,7 @@ team_no_units_error = Allow access to at least one repository section. email_been_used = The email address is already used. openid_been_used = The OpenID address '%s' is already used. username_password_incorrect = Username or password is incorrect. -password_complexity = Password does not pass complexity requirements. +password_complexity = Password does not pass complexity requirements. enterred_invalid_repo_name = The repository name you entered is incorrect. enterred_invalid_owner_name = The new owner name is not valid. enterred_invalid_password = The password you entered is incorrect. diff --git a/routers/admin/users_test.go b/routers/admin/users_test.go index e054524fd..2b36b45d4 100644 --- a/routers/admin/users_test.go +++ b/routers/admin/users_test.go @@ -34,7 +34,7 @@ func TestNewUserPost_MustChangePassword(t *testing.T) { LoginName: "local", UserName: username, Email: email, - Password: "xxxxxxxx", + Password: "abc123ABC!=$", SendNotify: false, MustChangePassword: true, } @@ -71,7 +71,7 @@ func TestNewUserPost_MustChangePasswordFalse(t *testing.T) { LoginName: "local", UserName: username, Email: email, - Password: "xxxxxxxx", + Password: "abc123ABC!=$", SendNotify: false, MustChangePassword: false, } diff --git a/routers/user/setting/account.go b/routers/user/setting/account.go index c78222421..e7de2dffd 100644 --- a/routers/user/setting/account.go +++ b/routers/user/setting/account.go @@ -54,7 +54,7 @@ func AccountPost(ctx *context.Context, form auth.ChangePasswordForm) { } else if form.Password != form.Retype { ctx.Flash.Error(ctx.Tr("form.password_not_match")) } else if !password.IsComplexEnough(form.Password) { - ctx.Flash.Error(ctx.Tr("settings.password_complexity")) + ctx.Flash.Error(ctx.Tr("form.password_complexity")) } else { var err error if ctx.User.Salt, err = models.GetUserSalt(); err != nil { diff --git a/routers/user/setting/account_test.go b/routers/user/setting/account_test.go index 497ee658b..41783e19d 100644 --- a/routers/user/setting/account_test.go +++ b/routers/user/setting/account_test.go @@ -19,76 +19,64 @@ import ( func TestChangePassword(t *testing.T) { oldPassword := "password" setting.MinPasswordLength = 6 - setting.PasswordComplexity = map[string]string{ - "lower": "[a-z]+", - "upper": "[A-Z]+", - "digit": "[0-9]+", - "spec": "[-_]+", - } - var pcLUN = map[string]string{ - "lower": "[a-z]+", - "upper": "[A-Z]+", - "digit": "[0-9]+", - } - var pcLU = map[string]string{ - "lower": "[a-z]+", - "upper": "[A-Z]+", - } + var pcALL = []string{"lower", "upper", "digit", "spec"} + var pcLUN = []string{"lower", "upper", "digit"} + var pcLU = []string{"lower", "upper"} for _, req := range []struct { OldPassword string NewPassword string Retype string Message string - PasswordComplexity map[string]string + PasswordComplexity []string }{ { OldPassword: oldPassword, NewPassword: "Qwerty123456-", Retype: "Qwerty123456-", Message: "", - PasswordComplexity: setting.PasswordComplexity, + PasswordComplexity: pcALL, }, { OldPassword: oldPassword, NewPassword: "12345", Retype: "12345", Message: "auth.password_too_short", - PasswordComplexity: setting.PasswordComplexity, + PasswordComplexity: pcALL, }, { OldPassword: "12334", NewPassword: "123456", Retype: "123456", Message: "settings.password_incorrect", - PasswordComplexity: setting.PasswordComplexity, + PasswordComplexity: pcALL, }, { OldPassword: oldPassword, NewPassword: "123456", Retype: "12345", Message: "form.password_not_match", - PasswordComplexity: setting.PasswordComplexity, + PasswordComplexity: pcALL, }, { OldPassword: oldPassword, NewPassword: "Qwerty", Retype: "Qwerty", - Message: "settings.password_complexity", - PasswordComplexity: setting.PasswordComplexity, + Message: "form.password_complexity", + PasswordComplexity: pcALL, }, { OldPassword: oldPassword, NewPassword: "Qwerty", Retype: "Qwerty", - Message: "settings.password_complexity", + Message: "form.password_complexity", PasswordComplexity: pcLUN, }, { OldPassword: oldPassword, NewPassword: "QWERTY", Retype: "QWERTY", - Message: "settings.password_complexity", + Message: "form.password_complexity", PasswordComplexity: pcLU, }, } {