Skip to content

Commit 38eccee

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

11 files changed

Lines changed: 94 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: 18 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -188,10 +188,15 @@ 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+
pw, err := password.CreatePassword(user.Pass, a.PasswordStrength)
192+
if err != nil {
193+
ctx.AbortWithError(http.StatusInternalServerError, fmt.Errorf("failed to prepare password: %s", err))
194+
return
195+
}
191196
internal := &model.User{
192197
Name: user.Name,
193198
Admin: user.Admin,
194-
Pass: password.CreatePassword(user.Pass, a.PasswordStrength),
199+
Pass: pw,
195200
}
196201
existingUser, err := a.DB.GetUserByName(internal.Name)
197202
if success := successOrAbort(ctx, 500, err); !success {
@@ -393,7 +398,12 @@ func (a *UserAPI) ChangePassword(ctx *gin.Context) {
393398
if success := successOrAbort(ctx, 500, err); !success {
394399
return
395400
}
396-
user.Pass = password.CreatePassword(pw.Pass, a.PasswordStrength)
401+
pw, err := password.CreatePassword(pw.Pass, a.PasswordStrength)
402+
if err != nil {
403+
ctx.AbortWithError(http.StatusInternalServerError, fmt.Errorf("failed to prepare password: %s", err))
404+
return
405+
}
406+
user.Pass = pw
397407
successOrAbort(ctx, 500, a.DB.UpdateUser(user))
398408
}
399409
}
@@ -465,7 +475,12 @@ func (a *UserAPI) UpdateUserByID(ctx *gin.Context) {
465475
dbUser.Admin = updatedUser.Admin
466476

467477
if updatedUser.Pass != "" {
468-
dbUser.Pass = password.CreatePassword(updatedUser.Pass, a.PasswordStrength)
478+
pw, err := password.CreatePassword(updatedUser.Pass, a.PasswordStrength)
479+
if err != nil {
480+
ctx.AbortWithError(http.StatusInternalServerError, fmt.Errorf("failed to prepare password: %s", err))
481+
return
482+
}
483+
dbUser.Pass = pw
469484
}
470485
if success := successOrAbort(ctx, 500, a.DB.UpdateUser(dbUser)); !success {
471486
return

‎api/user_test.go‎

Lines changed: 13 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

@@ -359,7 +360,9 @@ func (s *UserSuite) Test_UpdateUserByID_UnknownUser() {
359360
}
360361

361362
func (s *UserSuite) Test_UpdateUserByID_UpdateNotPassword() {
362-
s.db.CreateUser(&model.User{ID: 2, Name: "nico", Pass: password.CreatePassword("old", 5)})
363+
pw, err := password.CreatePassword("old", 5)
364+
require.NoError(s.T(), err)
365+
s.db.CreateUser(&model.User{ID: 2, Name: "nico", Pass: pw})
363366

364367
s.ctx.Params = gin.Params{{Key: "id", Value: "2"}}
365368

@@ -376,7 +379,9 @@ func (s *UserSuite) Test_UpdateUserByID_UpdateNotPassword() {
376379
}
377380

378381
func (s *UserSuite) Test_UpdateUserByID_UpdatePassword() {
379-
s.db.CreateUser(&model.User{ID: 2, Name: "tom", Pass: password.CreatePassword("old", 5)})
382+
pw, err := password.CreatePassword("old", 5)
383+
require.NoError(s.T(), err)
384+
s.db.CreateUser(&model.User{ID: 2, Name: "tom", Pass: pw})
380385

381386
s.ctx.Params = gin.Params{{Key: "id", Value: "2"}}
382387

@@ -413,7 +418,9 @@ func (s *UserSuite) Test_UpdateUserByID_PreservesOIDCID() {
413418
}
414419

415420
func (s *UserSuite) Test_UpdatePassword() {
416-
s.db.CreateUser(&model.User{ID: 1, Name: "jmattheis", Pass: password.CreatePassword("old", 5)})
421+
pw, err := password.CreatePassword("old", 5)
422+
require.NoError(s.T(), err)
423+
s.db.CreateUser(&model.User{ID: 1, Name: "jmattheis", Pass: pw})
417424

418425
test.WithUser(s.ctx, 1)
419426
s.ctx.Request = httptest.NewRequest("POST", "/user/current/password", strings.NewReader(`{"pass": "new"}`))
@@ -429,7 +436,9 @@ func (s *UserSuite) Test_UpdatePassword() {
429436
}
430437

431438
func (s *UserSuite) Test_UpdatePassword_EmptyPassword() {
432-
s.db.CreateUser(&model.User{ID: 1, Name: "jmattheis", Pass: password.CreatePassword("old", 5)})
439+
pw, err := password.CreatePassword("old", 5)
440+
require.NoError(s.T(), err)
441+
s.db.CreateUser(&model.User{ID: 1, Name: "jmattheis", Pass: pw})
433442

434443
test.WithUser(s.ctx, 1)
435444
s.ctx.Request = httptest.NewRequest("POST", "/user/current/password", strings.NewReader(`{"pass":""}`))

‎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: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,20 @@
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")
410

511
// CreatePassword returns a hashed version of the given password.
6-
func CreatePassword(pw string, strength int) []byte {
12+
func CreatePassword(pw string, strength int) ([]byte, error) {
713
hashedPassword, err := bcrypt.GenerateFromPassword([]byte(pw), strength)
8-
if err != nil {
9-
panic(err)
14+
if err != nil && err != bcrypt.ErrPasswordTooLong {
15+
err = ErrUnexpectedError
1016
}
11-
return hashedPassword
17+
return hashedPassword, err
1218
}
1319

1420
// 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)