fix: address remaining code review findings (C-3, H-5, H-6, M-2 through M-16)

- C-3: inject setupLimiter into NewAdminAPI instead of package-level global
- H-5: generateRandomKey returns error instead of panicking
- H-6: replace init() bcrypt with sync.Once lazy initialization
- M-2: remove unsafe-inline from admin CSP
- M-3: sanitize upload filenames (strip control chars, truncate to 255)
- M-5: truncate User-Agent to 512 bytes before storing as device
- M-10: MaxBodySizeUnless uses prefix matching instead of exact path
- M-12: wrap seedExistingDatabase in a single transaction
- M-13: use errors.Is for EOF check in upload handler
- M-14: log writeJSON encoding errors instead of discarding
- M-16: standardize error codes to INTERNAL_ERROR across all handlers
This commit is contained in:
jevb
2026-03-31 19:08:02 +02:00
17 changed files with 234 additions and 101 deletions
+23 -12
View File
@@ -2,6 +2,7 @@ package auth
import (
"errors"
"sync"
"golang.org/x/crypto/bcrypt"
)
@@ -27,18 +28,28 @@ func HashPassword(password string) (string, error) {
return string(hash), nil
}
// dummyHash is a pre-computed bcrypt hash used to prevent timing side-channels
// when the user does not exist. Comparing against this dummy ensures that
// CheckPassword takes roughly constant time regardless of whether a valid hash
// was supplied.
var dummyHash []byte
// dummyHash is a lazily-computed bcrypt hash used to prevent timing
// side-channels when the user does not exist. Comparing against this dummy
// ensures that CheckPassword takes roughly constant time regardless of
// whether a valid hash was supplied.
var (
dummyHash []byte
dummyHashOnce sync.Once
)
func init() {
h, err := bcrypt.GenerateFromPassword([]byte("dummy-timing-pad"), bcryptCost)
if err != nil {
panic("auth: failed to generate dummy bcrypt hash: " + err.Error())
}
dummyHash = h
// getDummyHash returns the pre-computed dummy bcrypt hash, initialising it
// on first call via sync.Once.
func getDummyHash() []byte {
dummyHashOnce.Do(func() {
h, err := bcrypt.GenerateFromPassword([]byte("dummy-timing-pad"), bcryptCost)
if err != nil {
// crypto/rand is required for the server to function; panic is
// appropriate here as there is no recovery path.
panic("auth: failed to generate dummy bcrypt hash: " + err.Error())
}
dummyHash = h
})
return dummyHash
}
// CheckPassword reports whether password matches hash. Returns false on any
@@ -52,7 +63,7 @@ func CheckPassword(hash, password string) bool {
// The error is intentionally discarded: we always return false here.
// The comparison is performed only to consume time and prevent
// timing-based username enumeration.
_ = bcrypt.CompareHashAndPassword(dummyHash, []byte(password))
_ = bcrypt.CompareHashAndPassword(getDummyHash(), []byte(password))
return false
}
err := bcrypt.CompareHashAndPassword([]byte(hash), []byte(password))
+48
View File
@@ -161,6 +161,54 @@ func (s *PendingTOTPStore) cleanupExpiredLocked() {
}
}
// UsedTOTPCodeStore tracks recently verified TOTP codes to prevent replay
// attacks within the ±1 period validity window (~90 seconds).
type UsedTOTPCodeStore struct {
mu sync.Mutex
entries map[string]time.Time // key: "userID:code" → expiry
}
func NewUsedTOTPCodeStore() *UsedTOTPCodeStore {
return &UsedTOTPCodeStore{
entries: make(map[string]time.Time),
}
}
// MarkUsed records a TOTP code as used for the given user. Returns false if
// the code was already used (replay detected).
func (s *UsedTOTPCodeStore) MarkUsed(userID int64, code string) bool {
key := fmt.Sprintf("%d:%s", userID, code)
s.mu.Lock()
defer s.mu.Unlock()
s.cleanupExpiredLocked()
if _, exists := s.entries[key]; exists {
return false
}
s.entries[key] = time.Now().Add(90 * time.Second)
return true
}
func (s *UsedTOTPCodeStore) cleanupExpiredLocked() {
now := time.Now()
for key, expiry := range s.entries {
if now.After(expiry) {
delete(s.entries, key)
}
}
}
// VerifyTOTPCodeOnce verifies a TOTP code and marks it as used to prevent
// replay attacks. Returns false if the code is invalid or was already used.
func VerifyTOTPCodeOnce(secret, code string, at time.Time, userID int64, usedStore *UsedTOTPCodeStore) bool {
if !VerifyTOTPCode(secret, code, at) {
return false
}
if usedStore == nil {
return true
}
return usedStore.MarkUsed(userID, code)
}
func GenerateTOTPSecret() (string, error) {
bytes := make([]byte, 20)
if _, err := rand.Read(bytes); err != nil {