🛡️ Sentinel: [CRITICAL] Fix insecure password storage

.jules/sentinel.md

@@ -17,3 +17,10 @@
 **Vulnerability:** The WebUI server was missing standard HTTP security headers (X-Frame-Options, X-Content-Type-Options, Content-Security-Policy, etc.), potentially exposing it to clickjacking, MIME-sniffing, and XSS attacks.
 **Learning:** Express.js does not include security headers by default. Explicit middleware is required to set them.
 **Prevention:** Always include security headers middleware in the Express app setup pipeline to set X-Content-Type-Options, X-Frame-Options, Referrer-Policy, and Content-Security-Policy headers.
+
+## 2026-02-05 - Plaintext Password Migration Pattern
+**Vulnerability:** The application stored the WebUI password in plaintext in `config.json`, allowing anyone with file access to read it.
+**Learning:** Security upgrades often require migration strategies for existing data. A dual-strategy works best:
+1. **Startup Migration:** Check and migrate data when the service initializes.
+2. **Opportunistic Migration:** If startup migration is bypassed (e.g., config hot-reload), migrate when the legacy data is successfully used (e.g., on successful login).
+**Prevention:** Always store passwords hashed (e.g., PBKDF2, Argon2). When upgrading, ensure backward compatibility by detecting the data format (plaintext vs hash) and upgrading it transparently.

src/main/webui/server/AuthManager.ts

@@ -58,6 +58,74 @@ export class AuthManager {
     this.startSessionCleanup();
   }
 
+  /**
+   * Initialize authentication manager and migrate password if needed
+   */
+  public initialize(): void {
+    const config = this.configManager.getConfig();
+    const currentPassword = config.WebUIPassword;
+
+    // Check if password needs migration (is not hashed)
+    if (currentPassword && !currentPassword.startsWith('pbkdf2:')) {
+      console.log('Migrating WebUI password to secure hash...');
+      const hashedPassword = this.hashPassword(currentPassword);
+      this.configManager.set('WebUIPassword', hashedPassword);
+    }
+  }
+
+  /**
+   * Hash a password using PBKDF2
+   */
+  private hashPassword(password: string): string {
+    const salt = crypto.randomBytes(16).toString('hex');
+    const iterations = 10000;
+    const keylen = 64;
+    const digest = 'sha512';
+
+    const hash = crypto.pbkdf2Sync(password, salt, iterations, keylen, digest).toString('hex');
+    return `pbkdf2:${digest}:${iterations}:${salt}:${hash}`;
+  }
+
+  /**
+   * Verify a password against a stored hash (or plaintext for legacy)
+   */
+  private verifyPassword(password: string, storedPassword: string): boolean {
+    // Handle legacy plaintext passwords
+    if (!storedPassword.startsWith('pbkdf2:')) {
+      const passwordBuffer = Buffer.from(password);
+      const storedBuffer = Buffer.from(storedPassword);
+      if (passwordBuffer.length !== storedBuffer.length) {
+        return false;
+      }
+      return crypto.timingSafeEqual(passwordBuffer, storedBuffer);
+    }
+
+    try {
+      const parts = storedPassword.split(':');
+      if (parts.length !== 5) {
+        return false;
+      }
+
+      const [scheme, digest, iterationsStr, salt, expectedHash] = parts;
+      if (scheme !== 'pbkdf2') {
+        return false;
+      }
+
+      const iterations = parseInt(iterationsStr, 10);
+      const keylen = Buffer.from(expectedHash, 'hex').length;
+
+      const hash = crypto.pbkdf2Sync(password, salt, iterations, keylen, digest).toString('hex');
+
+      const hashBuffer = Buffer.from(hash, 'hex');
+      const expectedHashBuffer = Buffer.from(expectedHash, 'hex');
+
+      return crypto.timingSafeEqual(hashBuffer, expectedHashBuffer);
+    } catch (error) {
+      console.error('Password verification error:', error);
+      return false;
+    }
+  }
+
   /**
    * Validate login credentials and generate token
    */
@@ -75,19 +143,21 @@ export class AuthManager {
     // Check if password matches
     // Ensure password is a string to prevent Buffer.from throwing on undefined/null
     const passwordInput = typeof request.password === 'string' ? request.password : '';
-    const passwordBuffer = Buffer.from(passwordInput);
-    const serverPasswordBuffer = Buffer.from(serverPassword);
 
-    if (
-      passwordBuffer.length !== serverPasswordBuffer.length ||
-      !crypto.timingSafeEqual(passwordBuffer, serverPasswordBuffer)
-    ) {
+    if (!this.verifyPassword(passwordInput, serverPassword)) {
       return {
         success: false,
         message: 'Invalid password',
       };
     }
 
+    // Opportunistic migration: if we just successfully logged in with a plaintext password, hash it now
+    if (!serverPassword.startsWith('pbkdf2:')) {
+      console.log('Migrating WebUI password to secure hash after successful login');
+      const hashedPassword = this.hashPassword(serverPassword);
+      this.configManager.set('WebUIPassword', hashedPassword);
+    }
+
     // Generate session token
     const token = this.generateToken(request.rememberMe || false);
 
@@ -281,7 +351,7 @@ export class AuthManager {
 
     return {
       hasPassword: config.WebUIPasswordRequired && !!config.WebUIPassword,
-      defaultPassword: config.WebUIPassword === 'changeme',
+      defaultPassword: this.verifyPassword('changeme', config.WebUIPassword),
       authRequired: config.WebUIPasswordRequired,
     };
   }

src/main/webui/server/WebUIManager.ts

@@ -309,6 +309,9 @@ export class WebUIManager extends EventEmitter {
     }
 
     try {
+      // Ensure authentication system is initialized (and passwords migrated)
+      this.authManager.initialize();
+
       const config = this.configManager.getConfig();
 
       // Check if WebUI is enabled

src/main/webui/server/__tests__/AuthManager.test.ts

@@ -90,16 +90,52 @@ describe('AuthManager', () => {
         rememberMe: false,
       });
 
+      // Expect calls for both WebUIPassword (migration) and WebUISecret
       expect(mockConfigManager.set).toHaveBeenCalledWith('WebUISecret', expect.any(String));
     });
 
     it('should use existing WebUISecret if present', async () => {
       mockConfig.WebUISecret = 'existing-secret';
+      // Use a hashed password to prevent opportunistic migration from triggering 'set'
+      mockConfig.WebUIPassword = 'pbkdf2:sha512:10000:somesalt:somehash';
+      // We need to mock verifyPassword internal logic or just rely on it returning false?
+      // Wait, verifyPassword calculates hash. If I put a fake hash, verification fails.
+
+      // Easier: Let migration happen, but expect 'WebUISecret' NOT to be set.
+      mockConfig.WebUIPassword = 'securepassword123';
+
       await authManager.validateLogin({
         password: 'securepassword123',
         rememberMe: false,
       });
 
+      expect(mockConfigManager.set).toHaveBeenCalledWith('WebUIPassword', expect.stringMatching(/^pbkdf2:/));
+      expect(mockConfigManager.set).not.toHaveBeenCalledWith('WebUISecret', expect.any(String));
+    });
+
+    it('should migrate plaintext password on login', async () => {
+      mockConfig.WebUIPassword = 'securepassword123';
+
+      const result = await authManager.validateLogin({
+        password: 'securepassword123',
+        rememberMe: false,
+      });
+
+      expect(result.success).toBe(true);
+      expect(mockConfigManager.set).toHaveBeenCalledWith('WebUIPassword', expect.stringMatching(/^pbkdf2:/));
+    });
+  });
+
+  describe('initialize', () => {
+    it('should migrate plaintext password', () => {
+      mockConfig.WebUIPassword = 'securepassword123';
+      authManager.initialize();
+      expect(mockConfigManager.set).toHaveBeenCalledWith('WebUIPassword', expect.stringMatching(/^pbkdf2:/));
+    });
+
+    it('should not migrate already hashed password', () => {
+      mockConfig.WebUIPassword = 'pbkdf2:sha512:10000:salt:hash';
+      authManager.initialize();
       expect(mockConfigManager.set).not.toHaveBeenCalled();
     });
   });