Skip to content

Commit 726c34b

Browse files
eternal-flame-ADeternal-flame-AD
committed
fix: remove panic() and relay password hashing errors to UI
1 parent d543a8a commit 726c34b

11 files changed

Lines changed: 168 additions & 31 deletions

File tree

‎api/session_test.go‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import (
1414
"github.com/gotify/server/v2/model"
1515
"github.com/gotify/server/v2/test/testdb"
1616
"github.com/stretchr/testify/assert"
17+
"github.com/stretchr/testify/require"
1718
"github.com/stretchr/testify/suite"
1819
)
1920

@@ -39,9 +40,12 @@ func (s *SessionSuite) BeforeTest(suiteName, testName string) {
3940
s.notified = false
4041
s.a = &SessionAPI{DB: s.db, NotifyDeleted: s.notify}
4142

43+
pw, err := password.CreatePassword("testpass", 5)
44+
require.NoError(s.T(), err)
45+
4246
s.db.CreateUser(&model.User{
4347
Name: "testuser",
44-
Pass: password.CreatePassword("testpass", 5),
48+
Pass: pw,
4549
})
4650
}
4751

‎api/user.go‎

Lines changed: 30 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -188,10 +188,19 @@ func (a *UserAPI) GetCurrentUser(ctx *gin.Context) {
188188
func (a *UserAPI) CreateUser(ctx *gin.Context) {
189189
user := model.CreateUserExternal{}
190190
if err := ctx.Bind(&user); err == nil {
191+
if err := password.ValidateNewPassword(user.Pass); err != nil {
192+
ctx.AbortWithError(http.StatusBadRequest, err)
193+
return
194+
}
195+
pw, err := password.CreatePassword(user.Pass, a.PasswordStrength)
196+
if err != nil {
197+
ctx.AbortWithError(http.StatusInternalServerError, fmt.Errorf("failed to prepare password: %s", err))
198+
return
199+
}
191200
internal := &model.User{
192201
Name: user.Name,
193202
Admin: user.Admin,
194-
Pass: password.CreatePassword(user.Pass, a.PasswordStrength),
203+
Pass: pw,
195204
}
196205
existingUser, err := a.DB.GetUserByName(internal.Name)
197206
if success := successOrAbort(ctx, 500, err); !success {
@@ -389,11 +398,20 @@ func (a *UserAPI) DeleteUserByID(ctx *gin.Context) {
389398
func (a *UserAPI) ChangePassword(ctx *gin.Context) {
390399
pw := model.UserExternalPass{}
391400
if err := ctx.Bind(&pw); err == nil {
401+
if err := password.ValidateNewPassword(pw.Pass); err != nil {
402+
ctx.AbortWithError(http.StatusBadRequest, err)
403+
return
404+
}
392405
user, err := a.DB.GetUserByID(auth.GetUserID(ctx))
393406
if success := successOrAbort(ctx, 500, err); !success {
394407
return
395408
}
396-
user.Pass = password.CreatePassword(pw.Pass, a.PasswordStrength)
409+
pw, err := password.CreatePassword(pw.Pass, a.PasswordStrength)
410+
if err != nil {
411+
ctx.AbortWithError(http.StatusInternalServerError, fmt.Errorf("failed to prepare password: %s", err))
412+
return
413+
}
414+
user.Pass = pw
397415
successOrAbort(ctx, 500, a.DB.UpdateUser(user))
398416
}
399417
}
@@ -465,7 +483,16 @@ func (a *UserAPI) UpdateUserByID(ctx *gin.Context) {
465483
dbUser.Admin = updatedUser.Admin
466484

467485
if updatedUser.Pass != "" {
468-
dbUser.Pass = password.CreatePassword(updatedUser.Pass, a.PasswordStrength)
486+
if err := password.ValidateNewPassword(updatedUser.Pass); err != nil {
487+
ctx.AbortWithError(http.StatusBadRequest, err)
488+
return
489+
}
490+
pw, err := password.CreatePassword(updatedUser.Pass, a.PasswordStrength)
491+
if err != nil {
492+
ctx.AbortWithError(http.StatusInternalServerError, fmt.Errorf("failed to prepare password: %s", err))
493+
return
494+
}
495+
dbUser.Pass = pw
469496
}
470497
if success := successOrAbort(ctx, 500, a.DB.UpdateUser(dbUser)); !success {
471498
return

‎api/user_test.go‎

Lines changed: 65 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import (
1414
"github.com/gotify/server/v2/test"
1515
"github.com/gotify/server/v2/test/testdb"
1616
"github.com/stretchr/testify/assert"
17+
"github.com/stretchr/testify/require"
1718
"github.com/stretchr/testify/suite"
1819
)
1920

@@ -320,6 +321,24 @@ func (s *UserSuite) Test_CreateUser_NameAlreadyExists() {
320321
assert.Equal(s.T(), 400, s.recorder.Code)
321322
}
322323

324+
func (s *UserSuite) Test_CreateUser_EmptyPassword_Expect400() {
325+
s.loginAdmin()
326+
327+
s.ctx.Request = httptest.NewRequest("POST", "/user", strings.NewReader(`{"name": "admin", "pass": "", "admin": false}`))
328+
s.ctx.Request.Header.Set("Content-Type", "application/json")
329+
s.a.CreateUser(s.ctx)
330+
assert.Equal(s.T(), 400, s.recorder.Code)
331+
}
332+
333+
func (s *UserSuite) Test_CreateUser_TooLongPassword_Expect400() {
334+
s.loginAdmin()
335+
336+
s.ctx.Request = httptest.NewRequest("POST", "/user", strings.NewReader(`{"name": "admin", "pass": "`+strings.Repeat("a", 100)+`", "admin": false}`))
337+
s.ctx.Request.Header.Set("Content-Type", "application/json")
338+
s.a.CreateUser(s.ctx)
339+
assert.Equal(s.T(), 400, s.recorder.Code)
340+
}
341+
323342
func (s *UserSuite) Test_UpdateUserByID_InvalidID() {
324343
s.ctx.Params = gin.Params{{Key: "id", Value: "abc"}}
325344

@@ -331,6 +350,28 @@ func (s *UserSuite) Test_UpdateUserByID_InvalidID() {
331350
assert.Equal(s.T(), 400, s.recorder.Code)
332351
}
333352

353+
func (s *UserSuite) Test_UpdateUserByID_EmptyPassword_Expect400() {
354+
s.loginAdmin()
355+
356+
s.ctx.Params = gin.Params{{Key: "id", Value: "1"}}
357+
358+
s.ctx.Request = httptest.NewRequest("POST", "/user/1", strings.NewReader(`{"name": "admin", "pass": "", "admin": false}`))
359+
s.ctx.Request.Header.Set("Content-Type", "application/json")
360+
s.a.UpdateUserByID(s.ctx)
361+
assert.Equal(s.T(), 400, s.recorder.Code)
362+
}
363+
364+
func (s *UserSuite) Test_UpdateUserByID_TooLongPassword_Expect400() {
365+
s.loginAdmin()
366+
367+
s.ctx.Params = gin.Params{{Key: "id", Value: "1"}}
368+
369+
s.ctx.Request = httptest.NewRequest("POST", "/user/1", strings.NewReader(`{"name": "admin", "pass": "`+strings.Repeat("a", 100)+`", "admin": false}`))
370+
s.ctx.Request.Header.Set("Content-Type", "application/json")
371+
s.a.UpdateUserByID(s.ctx)
372+
assert.Equal(s.T(), 400, s.recorder.Code)
373+
}
374+
334375
func (s *UserSuite) Test_UpdateUserByID_LastAdmin_Expect400() {
335376
s.db.CreateUser(&model.User{
336377
ID: 7,
@@ -359,7 +400,9 @@ func (s *UserSuite) Test_UpdateUserByID_UnknownUser() {
359400
}
360401

361402
func (s *UserSuite) Test_UpdateUserByID_UpdateNotPassword() {
362-
s.db.CreateUser(&model.User{ID: 2, Name: "nico", Pass: password.CreatePassword("old", 5)})
403+
pw, err := password.CreatePassword("old", 5)
404+
require.NoError(s.T(), err)
405+
s.db.CreateUser(&model.User{ID: 2, Name: "nico", Pass: pw})
363406

364407
s.ctx.Params = gin.Params{{Key: "id", Value: "2"}}
365408

@@ -376,7 +419,9 @@ func (s *UserSuite) Test_UpdateUserByID_UpdateNotPassword() {
376419
}
377420

378421
func (s *UserSuite) Test_UpdateUserByID_UpdatePassword() {
379-
s.db.CreateUser(&model.User{ID: 2, Name: "tom", Pass: password.CreatePassword("old", 5)})
422+
pw, err := password.CreatePassword("old", 5)
423+
require.NoError(s.T(), err)
424+
s.db.CreateUser(&model.User{ID: 2, Name: "tom", Pass: pw})
380425

381426
s.ctx.Params = gin.Params{{Key: "id", Value: "2"}}
382427

@@ -413,7 +458,9 @@ func (s *UserSuite) Test_UpdateUserByID_PreservesOIDCID() {
413458
}
414459

415460
func (s *UserSuite) Test_UpdatePassword() {
416-
s.db.CreateUser(&model.User{ID: 1, Name: "jmattheis", Pass: password.CreatePassword("old", 5)})
461+
pw, err := password.CreatePassword("old", 5)
462+
require.NoError(s.T(), err)
463+
s.db.CreateUser(&model.User{ID: 1, Name: "jmattheis", Pass: pw})
417464

418465
test.WithUser(s.ctx, 1)
419466
s.ctx.Request = httptest.NewRequest("POST", "/user/current/password", strings.NewReader(`{"pass": "new"}`))
@@ -429,7 +476,9 @@ func (s *UserSuite) Test_UpdatePassword() {
429476
}
430477

431478
func (s *UserSuite) Test_UpdatePassword_EmptyPassword() {
432-
s.db.CreateUser(&model.User{ID: 1, Name: "jmattheis", Pass: password.CreatePassword("old", 5)})
479+
pw, err := password.CreatePassword("old", 5)
480+
require.NoError(s.T(), err)
481+
s.db.CreateUser(&model.User{ID: 1, Name: "jmattheis", Pass: pw})
433482

434483
test.WithUser(s.ctx, 1)
435484
s.ctx.Request = httptest.NewRequest("POST", "/user/current/password", strings.NewReader(`{"pass":""}`))
@@ -444,6 +493,18 @@ func (s *UserSuite) Test_UpdatePassword_EmptyPassword() {
444493
assert.True(s.T(), password.ComparePassword(user.Pass, []byte("old")))
445494
}
446495

496+
func (s *UserSuite) Test_UpdatePassword_TooLongPassword_Expect400() {
497+
pw, err := password.CreatePassword("old", 5)
498+
require.NoError(s.T(), err)
499+
s.db.CreateUser(&model.User{ID: 1, Name: "jmattheis", Pass: pw})
500+
501+
test.WithUser(s.ctx, 1)
502+
s.ctx.Request = httptest.NewRequest("POST", "/user/current/password", strings.NewReader(`{"pass": "`+strings.Repeat("a", 100)+`"}`))
503+
s.ctx.Request.Header.Set("Content-Type", "application/json")
504+
s.a.ChangePassword(s.ctx)
505+
assert.Equal(s.T(), 400, s.recorder.Code)
506+
}
507+
447508
func (s *UserSuite) loginAdmin() {
448509
s.db.CreateUser(&model.User{ID: 1, Name: "admin", Admin: true})
449510
auth.RegisterUser(s.ctx, &model.User{ID: 1, Admin: true})

‎auth/authentication_test.go‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import (
1313
"github.com/gotify/server/v2/model"
1414
"github.com/gotify/server/v2/test/testdb"
1515
"github.com/stretchr/testify/assert"
16+
"github.com/stretchr/testify/require"
1617
"github.com/stretchr/testify/suite"
1718
)
1819

@@ -37,9 +38,12 @@ func (s *AuthenticationSuite) SetupSuite() {
3738
elevated := now.Add(time.Hour)
3839
expired := now.Add(-time.Hour)
3940

41+
pw, err := password.CreatePassword("pw", 5)
42+
require.NoError(s.T(), err)
43+
4044
s.DB.CreateUser(&model.User{
4145
Name: "existing",
42-
Pass: password.CreatePassword("pw", 5),
46+
Pass: pw,
4347
Admin: false,
4448
Applications: []model.Application{{Token: "apptoken", Name: "backup server1", Description: "irrelevant"}},
4549
Clients: []model.Client{
@@ -51,7 +55,7 @@ func (s *AuthenticationSuite) SetupSuite() {
5155

5256
s.DB.CreateUser(&model.User{
5357
Name: "admin",
54-
Pass: password.CreatePassword("pw", 5),
58+
Pass: pw,
5559
Admin: true,
5660
Applications: []model.Application{{Token: "apptoken_admin", Name: "backup server2", Description: "irrelevant"}},
5761
Clients: []model.Client{

‎auth/password/password.go‎

Lines changed: 21 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,30 @@
11
package password
22

3-
import "golang.org/x/crypto/bcrypt"
3+
import (
4+
"errors"
5+
6+
"golang.org/x/crypto/bcrypt"
7+
)
8+
9+
var ErrUnexpectedError = errors.New("unexpected error")
10+
11+
func ValidateNewPassword(pw string) error {
12+
if pw == "" {
13+
return errors.New("password must not be empty")
14+
}
15+
if len([]byte(pw)) > 72 {
16+
return bcrypt.ErrPasswordTooLong
17+
}
18+
return nil
19+
}
420

521
// CreatePassword returns a hashed version of the given password.
6-
func CreatePassword(pw string, strength int) []byte {
22+
func CreatePassword(pw string, strength int) ([]byte, error) {
723
hashedPassword, err := bcrypt.GenerateFromPassword([]byte(pw), strength)
8-
if err != nil {
9-
panic(err)
24+
if err != nil && err != bcrypt.ErrPasswordTooLong {
25+
err = ErrUnexpectedError
1026
}
11-
return hashedPassword
27+
return hashedPassword, err
1228
}
1329

1430
// ComparePassword compares a hashed password with its possible plaintext equivalent.

‎auth/password/password_test.go‎

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,21 +1,32 @@
11
package password
22

33
import (
4+
"strings"
45
"testing"
56

67
"github.com/stretchr/testify/assert"
8+
"github.com/stretchr/testify/require"
9+
"golang.org/x/crypto/bcrypt"
710
)
811

912
func TestPasswordSuccess(t *testing.T) {
10-
password := CreatePassword("secret", 5)
13+
password, err := CreatePassword("secret", 5)
14+
require.NoError(t, err)
1115
assert.Equal(t, true, ComparePassword(password, []byte("secret")))
1216
}
1317

1418
func TestPasswordFailure(t *testing.T) {
15-
password := CreatePassword("secret", 5)
19+
password, err := CreatePassword("secret", 5)
20+
require.NoError(t, err)
1621
assert.Equal(t, false, ComparePassword(password, []byte("secretx")))
1722
}
1823

19-
func TestBCryptFailure(t *testing.T) {
20-
assert.Panics(t, func() { CreatePassword("secret", 12312) })
24+
func TestBCryptoTooLongErrorIsReturned(t *testing.T) {
25+
_, err := CreatePassword(strings.Repeat("a", 100), 5)
26+
assert.ErrorIs(t, err, bcrypt.ErrPasswordTooLong)
27+
}
28+
29+
func TestBCryptErrorIsMasked(t *testing.T) {
30+
_, err := CreatePassword("secret", 12312)
31+
assert.ErrorIs(t, err, ErrUnexpectedError)
2132
}

‎config/config_test.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,8 @@ func TestConfigEnv(t *testing.T) {
1313
mode.Set(mode.TestDev)
1414
os.Setenv("GOTIFY_DEFAULTUSER_NAME", "jmattheis")
1515
os.Setenv("GOTIFY_SERVER_SSL_LETSENCRYPT_HOSTS", "push.example.tld,push.other.tld")
16-
os.Setenv("GOTIFY_SERVER_RESPONSEHEADERS",
16+
os.Setenv(
17+
"GOTIFY_SERVER_RESPONSEHEADERS",
1718
`{"Access-Control-Allow-Origin":"*","Access-Control-Allow-Methods":"GET,POST"}`,
1819
)
1920
os.Setenv("GOTIFY_SERVER_CORS_ALLOWORIGINS", ".+.example.com,otherdomain.com")

‎database/database.go‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,11 @@ func New(dialect, connection, defaultUser, defaultPass string, strength int, cre
9494
userCount := int64(0)
9595
db.Find(new(model.User)).Count(&userCount)
9696
if createDefaultUserIfNotExist && userCount == 0 {
97-
db.Create(&model.User{Name: defaultUser, Pass: password.CreatePassword(defaultPass, strength), Admin: true})
97+
pass, err := password.CreatePassword(defaultPass, strength)
98+
if err != nil {
99+
return nil, err
100+
}
101+
db.Create(&model.User{Name: defaultUser, Pass: pass, Admin: true})
98102
}
99103

100104
if err := db.Transaction(fillMissingSortKeys, &sql.TxOptions{Isolation: sql.LevelSerializable}); err != nil {

‎plugin/testing/mock/mock.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -151,7 +151,7 @@ func (c *PluginInstance) DefaultConfig() any {
151151

152152
// ValidateAndSetConfig implements compat.Configuror
153153
func (c *PluginInstance) ValidateAndSetConfig(config any) error {
154-
if (config.(*PluginConfig)).IsNotValid {
154+
if config.(*PluginConfig).IsNotValid {
155155
return errors.New("conf is not valid")
156156
}
157157
c.Config = config.(*PluginConfig)

‎router/router.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,8 @@ func Create(db *database.GormDatabase, vInfo *model.VersionInfo, conf *config.Co
6666
})
6767
}
6868
streamHandler := stream.New(
69-
time.Duration(conf.Server.Stream.PingPeriodSeconds)*time.Second, 15*time.Second, conf.Server.Stream.AllowedOrigins)
69+
time.Duration(conf.Server.Stream.PingPeriodSeconds)*time.Second, 15*time.Second, conf.Server.Stream.AllowedOrigins,
70+
)
7071
go func() {
7172
ticker := time.NewTicker(5 * time.Minute)
7273
for range ticker.C {

0 commit comments

Comments
 (0)