diff --git a/api/session_test.go b/api/session_test.go index fea0bbd6c..8817de62c 100644 --- a/api/session_test.go +++ b/api/session_test.go @@ -14,6 +14,7 @@ import ( "github.com/gotify/server/v2/model" "github.com/gotify/server/v2/test/testdb" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "github.com/stretchr/testify/suite" ) @@ -39,9 +40,12 @@ func (s *SessionSuite) BeforeTest(suiteName, testName string) { s.notified = false s.a = &SessionAPI{DB: s.db, NotifyDeleted: s.notify} + pw, err := password.CreatePassword("testpass", 5) + require.NoError(s.T(), err) + s.db.CreateUser(&model.User{ Name: "testuser", - Pass: password.CreatePassword("testpass", 5), + Pass: pw, }) } diff --git a/api/user.go b/api/user.go index 6ce764d83..448c6eeb0 100644 --- a/api/user.go +++ b/api/user.go @@ -188,10 +188,15 @@ func (a *UserAPI) GetCurrentUser(ctx *gin.Context) { func (a *UserAPI) CreateUser(ctx *gin.Context) { user := model.CreateUserExternal{} if err := ctx.Bind(&user); err == nil { + pw, err := password.CreatePassword(user.Pass, a.PasswordStrength) + if err != nil { + ctx.AbortWithError(http.StatusInternalServerError, fmt.Errorf("failed to prepare password: %s", err)) + return + } internal := &model.User{ Name: user.Name, Admin: user.Admin, - Pass: password.CreatePassword(user.Pass, a.PasswordStrength), + Pass: pw, } existingUser, err := a.DB.GetUserByName(internal.Name) if success := successOrAbort(ctx, 500, err); !success { @@ -393,7 +398,12 @@ func (a *UserAPI) ChangePassword(ctx *gin.Context) { if success := successOrAbort(ctx, 500, err); !success { return } - user.Pass = password.CreatePassword(pw.Pass, a.PasswordStrength) + pw, err := password.CreatePassword(pw.Pass, a.PasswordStrength) + if err != nil { + ctx.AbortWithError(http.StatusInternalServerError, fmt.Errorf("failed to prepare password: %s", err)) + return + } + user.Pass = pw successOrAbort(ctx, 500, a.DB.UpdateUser(user)) } } @@ -465,7 +475,12 @@ func (a *UserAPI) UpdateUserByID(ctx *gin.Context) { dbUser.Admin = updatedUser.Admin if updatedUser.Pass != "" { - dbUser.Pass = password.CreatePassword(updatedUser.Pass, a.PasswordStrength) + pw, err := password.CreatePassword(updatedUser.Pass, a.PasswordStrength) + if err != nil { + ctx.AbortWithError(http.StatusInternalServerError, fmt.Errorf("failed to prepare password: %s", err)) + return + } + dbUser.Pass = pw } if success := successOrAbort(ctx, 500, a.DB.UpdateUser(dbUser)); !success { return diff --git a/api/user_test.go b/api/user_test.go index 638052518..41ca20537 100644 --- a/api/user_test.go +++ b/api/user_test.go @@ -14,6 +14,7 @@ import ( "github.com/gotify/server/v2/test" "github.com/gotify/server/v2/test/testdb" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "github.com/stretchr/testify/suite" ) @@ -359,7 +360,9 @@ func (s *UserSuite) Test_UpdateUserByID_UnknownUser() { } func (s *UserSuite) Test_UpdateUserByID_UpdateNotPassword() { - s.db.CreateUser(&model.User{ID: 2, Name: "nico", Pass: password.CreatePassword("old", 5)}) + pw, err := password.CreatePassword("old", 5) + require.NoError(s.T(), err) + s.db.CreateUser(&model.User{ID: 2, Name: "nico", Pass: pw}) s.ctx.Params = gin.Params{{Key: "id", Value: "2"}} @@ -376,7 +379,9 @@ func (s *UserSuite) Test_UpdateUserByID_UpdateNotPassword() { } func (s *UserSuite) Test_UpdateUserByID_UpdatePassword() { - s.db.CreateUser(&model.User{ID: 2, Name: "tom", Pass: password.CreatePassword("old", 5)}) + pw, err := password.CreatePassword("old", 5) + require.NoError(s.T(), err) + s.db.CreateUser(&model.User{ID: 2, Name: "tom", Pass: pw}) s.ctx.Params = gin.Params{{Key: "id", Value: "2"}} @@ -413,7 +418,9 @@ func (s *UserSuite) Test_UpdateUserByID_PreservesOIDCID() { } func (s *UserSuite) Test_UpdatePassword() { - s.db.CreateUser(&model.User{ID: 1, Name: "jmattheis", Pass: password.CreatePassword("old", 5)}) + pw, err := password.CreatePassword("old", 5) + require.NoError(s.T(), err) + s.db.CreateUser(&model.User{ID: 1, Name: "jmattheis", Pass: pw}) test.WithUser(s.ctx, 1) s.ctx.Request = httptest.NewRequest("POST", "/user/current/password", strings.NewReader(`{"pass": "new"}`)) @@ -429,7 +436,9 @@ func (s *UserSuite) Test_UpdatePassword() { } func (s *UserSuite) Test_UpdatePassword_EmptyPassword() { - s.db.CreateUser(&model.User{ID: 1, Name: "jmattheis", Pass: password.CreatePassword("old", 5)}) + pw, err := password.CreatePassword("old", 5) + require.NoError(s.T(), err) + s.db.CreateUser(&model.User{ID: 1, Name: "jmattheis", Pass: pw}) test.WithUser(s.ctx, 1) s.ctx.Request = httptest.NewRequest("POST", "/user/current/password", strings.NewReader(`{"pass":""}`)) diff --git a/auth/authentication_test.go b/auth/authentication_test.go index d92ecf10f..ce35c7a09 100644 --- a/auth/authentication_test.go +++ b/auth/authentication_test.go @@ -13,6 +13,7 @@ import ( "github.com/gotify/server/v2/model" "github.com/gotify/server/v2/test/testdb" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "github.com/stretchr/testify/suite" ) @@ -37,9 +38,12 @@ func (s *AuthenticationSuite) SetupSuite() { elevated := now.Add(time.Hour) expired := now.Add(-time.Hour) + pw, err := password.CreatePassword("pw", 5) + require.NoError(s.T(), err) + s.DB.CreateUser(&model.User{ Name: "existing", - Pass: password.CreatePassword("pw", 5), + Pass: pw, Admin: false, Applications: []model.Application{{Token: "apptoken", Name: "backup server1", Description: "irrelevant"}}, Clients: []model.Client{ @@ -51,7 +55,7 @@ func (s *AuthenticationSuite) SetupSuite() { s.DB.CreateUser(&model.User{ Name: "admin", - Pass: password.CreatePassword("pw", 5), + Pass: pw, Admin: true, Applications: []model.Application{{Token: "apptoken_admin", Name: "backup server2", Description: "irrelevant"}}, Clients: []model.Client{ diff --git a/auth/password/password.go b/auth/password/password.go index 9dbaca9de..a4b4c03d6 100644 --- a/auth/password/password.go +++ b/auth/password/password.go @@ -1,14 +1,20 @@ package password -import "golang.org/x/crypto/bcrypt" +import ( + "errors" + + "golang.org/x/crypto/bcrypt" +) + +var ErrUnexpectedError = errors.New("unexpected error") // CreatePassword returns a hashed version of the given password. -func CreatePassword(pw string, strength int) []byte { +func CreatePassword(pw string, strength int) ([]byte, error) { hashedPassword, err := bcrypt.GenerateFromPassword([]byte(pw), strength) - if err != nil { - panic(err) + if err != nil && err != bcrypt.ErrPasswordTooLong { + err = ErrUnexpectedError } - return hashedPassword + return hashedPassword, err } // ComparePassword compares a hashed password with its possible plaintext equivalent. diff --git a/auth/password/password_test.go b/auth/password/password_test.go index 738c7a1a5..ce5f758f5 100644 --- a/auth/password/password_test.go +++ b/auth/password/password_test.go @@ -1,21 +1,32 @@ package password import ( + "strings" "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "golang.org/x/crypto/bcrypt" ) func TestPasswordSuccess(t *testing.T) { - password := CreatePassword("secret", 5) + password, err := CreatePassword("secret", 5) + require.NoError(t, err) assert.Equal(t, true, ComparePassword(password, []byte("secret"))) } func TestPasswordFailure(t *testing.T) { - password := CreatePassword("secret", 5) + password, err := CreatePassword("secret", 5) + require.NoError(t, err) assert.Equal(t, false, ComparePassword(password, []byte("secretx"))) } -func TestBCryptFailure(t *testing.T) { - assert.Panics(t, func() { CreatePassword("secret", 12312) }) +func TestBCryptoTooLongErrorIsReturned(t *testing.T) { + _, err := CreatePassword(strings.Repeat("a", 100), 5) + assert.ErrorIs(t, err, bcrypt.ErrPasswordTooLong) +} + +func TestBCryptErrorIsMasked(t *testing.T) { + _, err := CreatePassword("secret", 12312) + assert.ErrorIs(t, err, ErrUnexpectedError) } diff --git a/config/config_test.go b/config/config_test.go index 216d80789..e570db969 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -13,7 +13,8 @@ func TestConfigEnv(t *testing.T) { mode.Set(mode.TestDev) os.Setenv("GOTIFY_DEFAULTUSER_NAME", "jmattheis") os.Setenv("GOTIFY_SERVER_SSL_LETSENCRYPT_HOSTS", "push.example.tld,push.other.tld") - os.Setenv("GOTIFY_SERVER_RESPONSEHEADERS", + os.Setenv( + "GOTIFY_SERVER_RESPONSEHEADERS", `{"Access-Control-Allow-Origin":"*","Access-Control-Allow-Methods":"GET,POST"}`, ) os.Setenv("GOTIFY_SERVER_CORS_ALLOWORIGINS", ".+.example.com,otherdomain.com") diff --git a/database/database.go b/database/database.go index 205489ac7..12bb79950 100644 --- a/database/database.go +++ b/database/database.go @@ -94,7 +94,11 @@ func New(dialect, connection, defaultUser, defaultPass string, strength int, cre userCount := int64(0) db.Find(new(model.User)).Count(&userCount) if createDefaultUserIfNotExist && userCount == 0 { - db.Create(&model.User{Name: defaultUser, Pass: password.CreatePassword(defaultPass, strength), Admin: true}) + pass, err := password.CreatePassword(defaultPass, strength) + if err != nil { + return nil, err + } + db.Create(&model.User{Name: defaultUser, Pass: pass, Admin: true}) } if err := db.Transaction(fillMissingSortKeys, &sql.TxOptions{Isolation: sql.LevelSerializable}); err != nil { diff --git a/plugin/testing/mock/mock.go b/plugin/testing/mock/mock.go index 51c15d969..16fa5e7b0 100644 --- a/plugin/testing/mock/mock.go +++ b/plugin/testing/mock/mock.go @@ -151,7 +151,7 @@ func (c *PluginInstance) DefaultConfig() any { // ValidateAndSetConfig implements compat.Configuror func (c *PluginInstance) ValidateAndSetConfig(config any) error { - if (config.(*PluginConfig)).IsNotValid { + if config.(*PluginConfig).IsNotValid { return errors.New("conf is not valid") } c.Config = config.(*PluginConfig) diff --git a/router/router.go b/router/router.go index cf4d8da46..9a2a4854b 100644 --- a/router/router.go +++ b/router/router.go @@ -66,7 +66,8 @@ func Create(db *database.GormDatabase, vInfo *model.VersionInfo, conf *config.Co }) } streamHandler := stream.New( - time.Duration(conf.Server.Stream.PingPeriodSeconds)*time.Second, 15*time.Second, conf.Server.Stream.AllowedOrigins) + time.Duration(conf.Server.Stream.PingPeriodSeconds)*time.Second, 15*time.Second, conf.Server.Stream.AllowedOrigins, + ) go func() { ticker := time.NewTicker(5 * time.Minute) for range ticker.C { diff --git a/router/router_test.go b/router/router_test.go index f72e95a8f..34f6a909a 100644 --- a/router/router_test.go +++ b/router/router_test.go @@ -39,7 +39,8 @@ func (s *IntegrationSuite) BeforeTest(string, string) { s.db = testdb.NewDBWithDefaultUser(s.T()) assert.Nil(s.T(), err) - g, closable := Create(s.db.GormDatabase, + g, closable := Create( + s.db.GormDatabase, &model.VersionInfo{Version: "1.0.0", BuildDate: "2018-02-20-17:30:47", Commit: "asdasds"}, &config.Configuration{PassStrength: 5}, ) @@ -79,7 +80,8 @@ func TestHeadersFromConfiguration(t *testing.T) { "Access-Control-Allow-Origin": "http://test1.com", } - g, closable := Create(db.GormDatabase, + g, closable := Create( + db.GormDatabase, &model.VersionInfo{Version: "1.0.0", BuildDate: "2018-02-20-17:30:47", Commit: "asdasds"}, &config, ) @@ -108,7 +110,8 @@ func TestHeadersFromCORSConfig(t *testing.T) { config := config.Configuration{PassStrength: 5} config.Server.Cors.AllowOrigins = []string{"---", "http://test.com"} - g, closable := Create(db.GormDatabase, + g, closable := Create( + db.GormDatabase, &model.VersionInfo{Version: "1.0.0", BuildDate: "2018-02-20-17:30:47", Commit: "asdasds"}, &config, ) @@ -137,7 +140,8 @@ func TestInvalidOrigin(t *testing.T) { config := config.Configuration{PassStrength: 5} config.Server.Cors.AllowOrigins = []string{"---", "http://test.com"} - g, closable := Create(db.GormDatabase, + g, closable := Create( + db.GormDatabase, &model.VersionInfo{Version: "1.0.0", BuildDate: "2018-02-20-17:30:47", Commit: "asdasds"}, &config, ) @@ -169,7 +173,8 @@ func TestAllowedOriginFromResponseHeaders(t *testing.T) { "Access-Control-Allow-Methods": "GET,POST", } - g, closable := Create(db.GormDatabase, + g, closable := Create( + db.GormDatabase, &model.VersionInfo{Version: "1.0.0", BuildDate: "2018-02-20-17:30:47", Commit: "asdasds"}, &config, ) @@ -207,7 +212,8 @@ func TestAllowedWildcardOriginInHeader(t *testing.T) { "Access-Control-Allow-Methods": "GET,POST", } - g, closable := Create(db.GormDatabase, + g, closable := Create( + db.GormDatabase, &model.VersionInfo{Version: "1.0.0", BuildDate: "2018-02-20-17:30:47", Commit: "asdasds"}, &config, ) @@ -236,7 +242,8 @@ func TestCORSHeaderRegex(t *testing.T) { config := config.Configuration{PassStrength: 5} config.Server.Cors.AllowOrigins = []string{"---", "^http://test\\d{3}.com$"} - g, closable := Create(db.GormDatabase, + g, closable := Create( + db.GormDatabase, &model.VersionInfo{Version: "1.0.0", BuildDate: "2018-02-20-17:30:47", Commit: "asdasds"}, &config, ) @@ -274,7 +281,8 @@ func TestCORSConfigOverride(t *testing.T) { config.Server.Cors.AllowMethods = []string{"GET", "OPTIONS"} config.Server.Cors.AllowHeaders = []string{"Content-Type"} - g, closable := Create(db.GormDatabase, + g, closable := Create( + db.GormDatabase, &model.VersionInfo{Version: "1.0.0", BuildDate: "2018-02-20-17:30:47", Commit: "asdasds"}, &config, )