Skip to content

Commit d945ca6

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

11 files changed

Lines changed: 107 additions & 32 deletions

File tree

‎api/session_test.go‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,9 +39,14 @@ func (s *SessionSuite) BeforeTest(suiteName, testName string) {
3939
s.notified = false
4040
s.a = &SessionAPI{DB: s.db, NotifyDeleted: s.notify}
4141

42+
pw, err := password.CreatePassword("testpass", 5)
43+
if err != nil {
44+
s.T().Fatalf("Failed to create password: %v", err)
45+
}
46+
4247
s.db.CreateUser(&model.User{
4348
Name: "testuser",
44-
Pass: password.CreatePassword("testpass", 5),
49+
Pass: pw,
4550
})
4651
}
4752

‎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: 20 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -359,7 +359,11 @@ func (s *UserSuite) Test_UpdateUserByID_UnknownUser() {
359359
}
360360

361361
func (s *UserSuite) Test_UpdateUserByID_UpdateNotPassword() {
362-
s.db.CreateUser(&model.User{ID: 2, Name: "nico", Pass: password.CreatePassword("old", 5)})
362+
pw, err := password.CreatePassword("old", 5)
363+
if err != nil {
364+
s.T().Fatalf("Failed to create password: %v", err)
365+
}
366+
s.db.CreateUser(&model.User{ID: 2, Name: "nico", Pass: pw})
363367

364368
s.ctx.Params = gin.Params{{Key: "id", Value: "2"}}
365369

@@ -376,7 +380,11 @@ func (s *UserSuite) Test_UpdateUserByID_UpdateNotPassword() {
376380
}
377381

378382
func (s *UserSuite) Test_UpdateUserByID_UpdatePassword() {
379-
s.db.CreateUser(&model.User{ID: 2, Name: "tom", Pass: password.CreatePassword("old", 5)})
383+
pw, err := password.CreatePassword("old", 5)
384+
if err != nil {
385+
s.T().Fatalf("Failed to create password: %v", err)
386+
}
387+
s.db.CreateUser(&model.User{ID: 2, Name: "tom", Pass: pw})
380388

381389
s.ctx.Params = gin.Params{{Key: "id", Value: "2"}}
382390

@@ -413,7 +421,11 @@ func (s *UserSuite) Test_UpdateUserByID_PreservesOIDCID() {
413421
}
414422

415423
func (s *UserSuite) Test_UpdatePassword() {
416-
s.db.CreateUser(&model.User{ID: 1, Name: "jmattheis", Pass: password.CreatePassword("old", 5)})
424+
pw, err := password.CreatePassword("old", 5)
425+
if err != nil {
426+
s.T().Fatalf("Failed to create password: %v", err)
427+
}
428+
s.db.CreateUser(&model.User{ID: 1, Name: "jmattheis", Pass: pw})
417429

418430
test.WithUser(s.ctx, 1)
419431
s.ctx.Request = httptest.NewRequest("POST", "/user/current/password", strings.NewReader(`{"pass": "new"}`))
@@ -429,7 +441,11 @@ func (s *UserSuite) Test_UpdatePassword() {
429441
}
430442

431443
func (s *UserSuite) Test_UpdatePassword_EmptyPassword() {
432-
s.db.CreateUser(&model.User{ID: 1, Name: "jmattheis", Pass: password.CreatePassword("old", 5)})
444+
pw, err := password.CreatePassword("old", 5)
445+
if err != nil {
446+
s.T().Fatalf("Failed to create password: %v", err)
447+
}
448+
s.db.CreateUser(&model.User{ID: 1, Name: "jmattheis", Pass: pw})
433449

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

‎auth/authentication_test.go‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -37,9 +37,14 @@ func (s *AuthenticationSuite) SetupSuite() {
3737
elevated := now.Add(time.Hour)
3838
expired := now.Add(-time.Hour)
3939

40+
pw, err := password.CreatePassword("pw", 5)
41+
if err != nil {
42+
s.T().Fatalf("Failed to create password: %v", err)
43+
}
44+
4045
s.DB.CreateUser(&model.User{
4146
Name: "existing",
42-
Pass: password.CreatePassword("pw", 5),
47+
Pass: pw,
4348
Admin: false,
4449
Applications: []model.Application{{Token: "apptoken", Name: "backup server1", Description: "irrelevant"}},
4550
Clients: []model.Client{
@@ -51,7 +56,7 @@ func (s *AuthenticationSuite) SetupSuite() {
5156

5257
s.DB.CreateUser(&model.User{
5358
Name: "admin",
54-
Pass: password.CreatePassword("pw", 5),
59+
Pass: pw,
5560
Admin: true,
5661
Applications: []model.Application{{Token: "apptoken_admin", Name: "backup server2", Description: "irrelevant"}},
5762
Clients: []model.Client{

‎auth/password/password.go‎

Lines changed: 12 additions & 6 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 {
7-
hashedPassword, err := bcrypt.GenerateFromPassword([]byte(pw), strength)
8-
if err != nil {
9-
panic(err)
12+
func CreatePassword(pw string, strength int) (hashedPassword []byte, err error) {
13+
hashedPassword, err = bcrypt.GenerateFromPassword([]byte(pw), strength)
14+
if err != nil && err != bcrypt.ErrPasswordTooLong {
15+
err = ErrUnexpectedError
1016
}
11-
return hashedPassword
17+
return
1218
}
1319

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

‎auth/password/password_test.go‎

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

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

67
"github.com/stretchr/testify/assert"
8+
"golang.org/x/crypto/bcrypt"
79
)
810

911
func TestPasswordSuccess(t *testing.T) {
10-
password := CreatePassword("secret", 5)
12+
password, err := CreatePassword("secret", 5)
13+
if err != nil {
14+
t.Fatalf("Failed to create password: %v", err)
15+
}
1116
assert.Equal(t, true, ComparePassword(password, []byte("secret")))
1217
}
1318

1419
func TestPasswordFailure(t *testing.T) {
15-
password := CreatePassword("secret", 5)
20+
password, err := CreatePassword("secret", 5)
21+
if err != nil {
22+
t.Fatalf("Failed to create password: %v", err)
23+
}
1624
assert.Equal(t, false, ComparePassword(password, []byte("secretx")))
1725
}
1826

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

‎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)