fix: address review feedback on password hashing

- Return { valid, needsUpgrade } from validPassword() instead of
  setting mutable _needsUpgrade state on the instance — eliminates
  coupling between validPassword() and authenticate() via shared state.

- Validate PBKDF2_ITERATIONS env var at module load: warn and fall
  back to 220000 if the value is not a valid positive integer.

- Use strict regex (/^\d+\$[0-9a-f]{64}$/) to validate self-describing
  hash format before parsing — rejects malformed stored hashes like
  "220000$abc$def" instead of silently misinterpreting them.

- Add comment on raw SQL test helper explaining why User.create() is
  bypassed (need to insert specific legacy password formats).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Author: aberoham

lib/user/mysql.js

@@ -36,8 +36,9 @@ class UserRepoMySQL extends UserBase {
       AND g.name = ?`
 
     for (const u of await Mysql.execute(query, [username, groupName])) {
-      if (await this.validPassword(authTry.password, u.password, authTry.username, u.pass_salt)) {
-        if (this._needsUpgrade) {
+      const { valid, needsUpgrade } = await this.validPassword(authTry.password, u.password, authTry.username, u.pass_salt)
+      if (valid) {
+        if (needsUpgrade) {
           const salt = this.generateSalt()
           const hash = await this.hashForStorage(authTry.password, salt)
           await Mysql.execute(

lib/user/test.js

@@ -79,35 +79,35 @@ describe('user', function () {
   describe('validPassword', function () {
     it('auths user with plain text password', async () => {
       const r = await User.validPassword('test', 'test', 'demo', '')
-      assert.equal(r, true)
+      assert.deepEqual(r, { valid: true, needsUpgrade: true })
     })
 
     it('auths valid self-describing PBKDF2 password', async () => {
       const salt = '(ICzAm2.QfCa6.MN'
       const hash = await User.hashForStorage('YouGuessedIt!', salt)
       const r = await User.validPassword('YouGuessedIt!', hash, 'unit-test', salt)
-      assert.equal(r, true)
+      assert.deepEqual(r, { valid: true, needsUpgrade: false })
     })
 
     it('rejects invalid self-describing PBKDF2 password', async () => {
       const salt = '(ICzAm2.QfCa6.MN'
       const hash = await User.hashForStorage('YouGuessedIt!', salt)
       const r = await User.validPassword('YouMissedIt!', hash, 'unit-test', salt)
-      assert.equal(r, false)
+      assert.deepEqual(r, { valid: false, needsUpgrade: false })
     })
 
     it('auths valid legacy PBKDF2-5000 password', async () => {
       const salt = '(ICzAm2.QfCa6.MN'
       const hash = await User.hashAuthPbkdf2('YouGuessedIt!', salt, 5000)
       const r = await User.validPassword('YouGuessedIt!', hash, 'unit-test', salt)
-      assert.equal(r, true)
+      assert.deepEqual(r, { valid: true, needsUpgrade: true })
     })
 
     it('rejects invalid legacy PBKDF2-5000 password', async () => {
       const salt = '(ICzAm2.QfCa6.MN'
       const hash = await User.hashAuthPbkdf2('YouGuessedIt!', salt, 5000)
       const r = await User.validPassword('YouMissedIt!', hash, 'unit-test', salt)
-      assert.equal(r, false)
+      assert.deepEqual(r, { valid: false, needsUpgrade: false })
     })
 
     it('auths valid SHA1 password', async () => {
@@ -116,7 +116,7 @@ describe('user', function () {
         '083007777a5241d01abba70c938c60d80be60027',
         'unit-test',
       )
-      assert.equal(r, true)
+      assert.deepEqual(r, { valid: true, needsUpgrade: true })
     })
 
     it('rejects invalid SHA1 password', async () => {
@@ -125,7 +125,7 @@ describe('user', function () {
         '083007777a5241d01abba7Oc938c60d80be60027',
         'unit-test',
       )
-      assert.equal(r, false)
+      assert.deepEqual(r, { valid: false, needsUpgrade: false })
     })
   })
 
@@ -179,6 +179,8 @@ describe('user', function () {
       return rows[0]
     }
 
+    // Raw SQL so we can insert specific legacy password formats (plain text,
+    // SHA-1, PBKDF2-5000) that User.create() would hash on the way in.
     async function insertUser(password, passSalt) {
       await Mysql.execute(
         'INSERT INTO nt_user (nt_user_id, nt_group_id, username, email, first_name, last_name, password, pass_salt) VALUES (?, ?, ?, ?, ?, ?, ?, ?)',

lib/user/userBase.js

@@ -1,7 +1,17 @@
 import crypto from 'node:crypto'
 
-const PBKDF2_ITERATIONS = parseInt(process.env.PBKDF2_ITERATIONS) || 220000
+const PBKDF2_ITERATIONS = (() => {
+  const raw = process.env.PBKDF2_ITERATIONS
+  if (raw === undefined) return 220000
+  const parsed = parseInt(raw, 10)
+  if (Number.isNaN(parsed) || parsed < 1) {
+    console.warn(`PBKDF2_ITERATIONS="${raw}" is not a valid positive integer, using 220000`)
+    return 220000
+  }
+  return parsed
+})()
 const LEGACY_ITERATIONS = 5000
+const SELF_DESCRIBING_RE = /^(\d+)\$([0-9a-f]{64})$/
 
 /**
  * User domain class – pure attributes and password business logic.
@@ -78,48 +88,43 @@ class UserBase {
   }
 
   async validPassword(passTry, passDb, username, salt) {
-    this._needsUpgrade = false
-
     if (!salt && passTry === passDb) {
-      this._needsUpgrade = true
-      return true
+      return { valid: true, needsUpgrade: true }
     }
 
     if (salt) {
       // Self-describing format: "iterations$hexHash" — single hash, no fallback
-      if (passDb.includes('$')) {
-        const [storedItersStr, storedHashHex] = passDb.split('$')
-        const storedIters = parseInt(storedItersStr, 10)
+      const m = SELF_DESCRIBING_RE.exec(passDb)
+      if (m) {
+        const storedIters = parseInt(m[1], 10)
+        const storedHashHex = m[2]
         const hashed = await this.hashAuthPbkdf2(passTry, salt, storedIters)
         if (this.debug) console.log(`self-describing: (${hashed === storedHashHex}) ${hashed}`)
         if (hashed === storedHashHex) {
-          if (storedIters < PBKDF2_ITERATIONS) this._needsUpgrade = true
-          return true
+          return { valid: true, needsUpgrade: storedIters < PBKDF2_ITERATIONS }
         }
-        return false
+        return { valid: false, needsUpgrade: false }
       }
 
       // Raw hex (legacy NicTool 2 format, implicitly 5000 iterations)
       const legacy = await this.hashAuthPbkdf2(passTry, salt, LEGACY_ITERATIONS)
       if (this.debug) console.log(`legacy: (${legacy === passDb}) ${legacy}`)
       if (legacy === passDb) {
-        this._needsUpgrade = true
-        return true
+        return { valid: true, needsUpgrade: true }
       }
-      return false
+      return { valid: false, needsUpgrade: false }
     }
 
     // Check for HMAC SHA-1 password
     if (/^[0-9a-f]{40}$/.test(passDb)) {
       const digest = crypto.createHmac('sha1', username.toLowerCase()).update(passTry).digest('hex')
       if (this.debug) console.log(`digest: (${digest === passDb}) ${digest}`)
       if (digest === passDb) {
-        this._needsUpgrade = true
-        return true
+        return { valid: true, needsUpgrade: true }
       }
     }
 
-    return false
+    return { valid: false, needsUpgrade: false }
   }
 }