From ecb31a158b69222099caa361f26a0e8c89235c7e Mon Sep 17 00:00:00 2001 From: zhangwenjian Date: Tue, 1 Sep 2026 11:35:33 +0800 Subject: [PATCH] =?UTF-8?q?fix=F0=9F=90=9B:=20stop=20re-hashing=20a=20pass?= =?UTF-8?q?word=20that=20is=20already=20hashed?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit BeforeCreate and BeforeUpdate run Encrypt on whatever is in the struct, and a user read from the database carries the stored hash in Password. Hashing it again produces a hash of a hash: the password that user knows stops matching, they cannot log in, and nothing reports an error. Only the Omit("password") on SysUser.Update stood between that and the stored credential. Any other write to this model - a profile update written the way every other model here is written - destroys the password, permanently and silently. Encrypt now returns early when Password already parses as a bcrypt hash. That also removes the round SysUser.Update was paying and discarding: 306ns where it was 54.7ms, on a route reachable without the permission check, since PUT /api/v1/sys-user is in CasbinExclude. The cost of deciding from the value is that a password which is itself a well-formed bcrypt hash would be stored unchanged. That is a 60-character string beginning "$2a$", and it grants whoever set it no access they did not already have. --- app/admin/models/sys_user.go | 40 +++++--- app/admin/models/sys_user_password_test.go | 106 +++++++++++++++++++++ 2 files changed, 132 insertions(+), 14 deletions(-) create mode 100644 app/admin/models/sys_user_password_test.go diff --git a/app/admin/models/sys_user.go b/app/admin/models/sys_user.go index eab2cadf..44dd578d 100644 --- a/app/admin/models/sys_user.go +++ b/app/admin/models/sys_user.go @@ -42,19 +42,35 @@ func (e *SysUser) GetId() interface{} { return e.UserId } -// Encrypt 加密 -func (e *SysUser) Encrypt() (err error) { +// Encrypt hashes Password, unless it already holds a hash. +// +// The hooks below run on whatever is in the struct, and a user read from the +// database carries the stored hash in that field. Hashing it again produces a +// hash of a hash, and the password that user knows no longer matches anything: +// they cannot log in, and nothing reports an error. The only thing preventing +// that today is an Omit("password") on the one update that loads a user first, +// which makes every other write to this model one line away from destroying +// credentials. +// +// bcrypt.Cost parses a hash and fails on anything else, so it distinguishes +// the two cases without the call site having to say which it is. The cost is +// that a password which is itself a well-formed bcrypt hash would be stored +// unchanged - a 60-character string beginning "$2a$", not something a person +// types, and it grants whoever set it no access they did not already have. +func (e *SysUser) Encrypt() error { if e.Password == "" { - return + return nil + } + if _, err := bcrypt.Cost([]byte(e.Password)); err == nil { + return nil } - var hash []byte - if hash, err = bcrypt.GenerateFromPassword([]byte(e.Password), bcrypt.DefaultCost); err != nil { - return - } else { - e.Password = string(hash) - return + hash, err := bcrypt.GenerateFromPassword([]byte(e.Password), bcrypt.DefaultCost) + if err != nil { + return err } + e.Password = string(hash) + return nil } func (e *SysUser) BeforeCreate(_ *gorm.DB) error { @@ -62,11 +78,7 @@ func (e *SysUser) BeforeCreate(_ *gorm.DB) error { } func (e *SysUser) BeforeUpdate(_ *gorm.DB) error { - var err error - if e.Password != "" { - err = e.Encrypt() - } - return err + return e.Encrypt() } func (e *SysUser) AfterFind(_ *gorm.DB) error { diff --git a/app/admin/models/sys_user_password_test.go b/app/admin/models/sys_user_password_test.go new file mode 100644 index 00000000..cdfd5dca --- /dev/null +++ b/app/admin/models/sys_user_password_test.go @@ -0,0 +1,106 @@ +package models + +import ( + "testing" + + "golang.org/x/crypto/bcrypt" +) + +const knownPassword = "correct-horse-battery-staple" + +// A user loaded from the database carries the stored hash in Password, and the +// hooks run on whatever is in the struct. Hashing it a second time produces a +// hash of a hash: the password the user knows stops matching, they cannot log +// in, and nothing reports an error. +// +// Only an Omit("password") on one call site stood between this and every write +// to the model. This is the test that removes the need for it. +func TestEncryptLeavesAnAlreadyHashedPasswordAlone(t *testing.T) { + fresh := SysUser{Password: knownPassword} + if err := fresh.Encrypt(); err != nil { + t.Fatalf("Encrypt: %v", err) + } + stored := fresh.Password + if err := bcrypt.CompareHashAndPassword([]byte(stored), []byte(knownPassword)); err != nil { + t.Fatalf("setup failed: the password was not hashed: %v", err) + } + + // What a query puts in the struct, and what an update then hands the hook. + loaded := SysUser{Password: stored} + if err := loaded.Encrypt(); err != nil { + t.Fatalf("Encrypt on a loaded user: %v", err) + } + if loaded.Password != stored { + t.Error("Encrypt re-hashed a stored hash; the user can no longer log in") + } + if err := bcrypt.CompareHashAndPassword([]byte(loaded.Password), []byte(knownPassword)); err != nil { + t.Errorf("the user can no longer log in with their password: %v", err) + } +} + +// The other half: a password that is not a hash still gets hashed, on create +// and on update alike. +func TestEncryptHashesAPlaintextPassword(t *testing.T) { + for _, c := range []struct { + name string + hook func(*SysUser) error + }{ + {"BeforeCreate", func(u *SysUser) error { return u.BeforeCreate(nil) }}, + {"BeforeUpdate", func(u *SysUser) error { return u.BeforeUpdate(nil) }}, + } { + t.Run(c.name, func(t *testing.T) { + u := SysUser{Password: knownPassword} + if err := c.hook(&u); err != nil { + t.Fatal(err) + } + if u.Password == knownPassword { + t.Fatal("the password was stored as it was typed") + } + if err := bcrypt.CompareHashAndPassword([]byte(u.Password), []byte(knownPassword)); err != nil { + t.Errorf("the stored value does not verify the password: %v", err) + } + }) + } +} + +// An empty Password means "not being set", and must not become a hash of "". +func TestEncryptIgnoresAnEmptyPassword(t *testing.T) { + u := SysUser{} + if err := u.Encrypt(); err != nil { + t.Fatal(err) + } + if u.Password != "" { + t.Errorf("an unset password became %q", u.Password) + } +} + +// Encrypt runs on every update of this model, including the ones that change +// something else entirely. What it costs when there is nothing to do is the +// difference between a profile update and a bcrypt round; the correctness test +// above is what catches a regression, this reports the size of it. +func BenchmarkEncrypt(b *testing.B) { + fresh := SysUser{Password: knownPassword} + if err := fresh.Encrypt(); err != nil { + b.Fatal(err) + } + + b.Run("already hashed", func(b *testing.B) { + u := SysUser{Password: fresh.Password} + b.ReportAllocs() + for i := 0; i < b.N; i++ { + if err := u.Encrypt(); err != nil { + b.Fatal(err) + } + } + }) + + b.Run("plaintext", func(b *testing.B) { + b.ReportAllocs() + for i := 0; i < b.N; i++ { + u := SysUser{Password: knownPassword} + if err := u.Encrypt(); err != nil { + b.Fatal(err) + } + } + }) +}