fix: address reviewer feedback (#54)

- Add automatic re-hashing of legacy SHA-256 PINs to Argon2id on
  successful verification, returning new hash to frontend for persistence
- Use constant-time comparison (subtle::ConstantTimeEq) for both
  Argon2id and legacy SHA-256 hash verification
- Add unit tests for hash_pin, verify_pin (Argon2id and legacy paths),
  re-hashing flow, error cases, and hex encoding roundtrip
- Update frontend to handle VerifyPinResult struct and save rehashed
  PIN hash via profile update

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

src-tauri/Cargo.toml

@@ -34,6 +34,7 @@ encoding_rs = "0.8"
 walkdir = "2"
 aes-gcm = "0.10"
 argon2 = "0.5"
+subtle = "2"
 rand = "0.8"
 jsonwebtoken = "9"
 machine-uid = "0.5"

src-tauri/src/commands/profile_commands.rs

@@ -3,6 +3,7 @@ use rand::RngCore;
 use serde::{Deserialize, Serialize};
 use sha2::{Digest, Sha256, Sha384};
 use std::fs;
+use subtle::ConstantTimeEq;
 use tauri::Manager;
 
 use crate::database;
@@ -150,8 +151,15 @@ pub fn hash_pin(pin: String) -> Result<String, String> {
     Ok(format!("argon2id:{}:{}", salt_hex, hash_hex))
 }
 
+#[derive(Debug, Clone, Serialize, Deserialize)]
+pub struct VerifyPinResult {
+    pub valid: bool,
+    /// New Argon2id hash when a legacy SHA-256 PIN was successfully verified and re-hashed
+    pub rehashed: Option<String>,
+}
+
 #[tauri::command]
-pub fn verify_pin(pin: String, stored_hash: String) -> Result<bool, String> {
+pub fn verify_pin(pin: String, stored_hash: String) -> Result<VerifyPinResult, String> {
     // Argon2id format: "argon2id:salt_hex:hash_hex"
     if let Some(rest) = stored_hash.strip_prefix("argon2id:") {
         let parts: Vec<&str> = rest.split(':').collect();
@@ -159,12 +167,12 @@ pub fn verify_pin(pin: String, stored_hash: String) -> Result<bool, String> {
             return Err("Invalid Argon2id hash format".to_string());
         }
         let salt = hex_decode(parts[0])?;
-        let expected_hash = parts[1];
+        let expected_hash = hex_decode(parts[1])?;
 
         let computed = argon2_hash(&pin, &salt)?;
-        let computed_hex = hex_encode(&computed);
 
-        return Ok(computed_hex == expected_hash);
+        let valid = computed.ct_eq(&expected_hash).into();
+        return Ok(VerifyPinResult { valid, rehashed: None });
     }
 
     // Legacy SHA-256 format: "salt_hex:hash_hex"
@@ -173,15 +181,22 @@ pub fn verify_pin(pin: String, stored_hash: String) -> Result<bool, String> {
         return Err("Invalid stored hash format".to_string());
     }
     let salt_hex = parts[0];
-    let expected_hash = parts[1];
+    let expected_hash = hex_decode(parts[1])?;
 
     let mut hasher = Sha256::new();
     hasher.update(salt_hex.as_bytes());
     hasher.update(pin.as_bytes());
     let result = hasher.finalize();
-    let computed_hash = hex_encode(&result);
 
-    Ok(computed_hash == expected_hash)
+    let valid: bool = result.as_slice().ct_eq(&expected_hash).into();
+
+    if valid {
+        // Re-hash with Argon2id so this legacy PIN is upgraded
+        let new_hash = hash_pin(pin)?;
+        Ok(VerifyPinResult { valid: true, rehashed: Some(new_hash) })
+    } else {
+        Ok(VerifyPinResult { valid: false, rehashed: None })
+    }
 }
 
 fn hex_encode(bytes: &[u8]) -> String {
@@ -262,3 +277,98 @@ pub fn repair_migrations(app: tauri::AppHandle, db_filename: String) -> Result<b
 
     Ok(repaired)
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn test_hash_pin_produces_argon2id_format() {
+        let hash = hash_pin("1234".to_string()).unwrap();
+        assert!(hash.starts_with("argon2id:"), "Hash should start with 'argon2id:' prefix");
+        let parts: Vec<&str> = hash.split(':').collect();
+        assert_eq!(parts.len(), 3, "Hash should have 3 parts: prefix:salt:hash");
+        assert_eq!(parts[1].len(), ARGON2_SALT_LEN * 2, "Salt should be {} hex chars", ARGON2_SALT_LEN * 2);
+        assert_eq!(parts[2].len(), ARGON2_OUTPUT_LEN * 2, "Hash should be {} hex chars", ARGON2_OUTPUT_LEN * 2);
+    }
+
+    #[test]
+    fn test_hash_pin_different_salts() {
+        let h1 = hash_pin("1234".to_string()).unwrap();
+        let h2 = hash_pin("1234".to_string()).unwrap();
+        assert_ne!(h1, h2, "Two hashes of the same PIN should use different salts");
+    }
+
+    #[test]
+    fn test_verify_argon2id_pin_correct() {
+        let hash = hash_pin("5678".to_string()).unwrap();
+        let result = verify_pin("5678".to_string(), hash).unwrap();
+        assert!(result.valid, "Correct PIN should verify");
+        assert!(result.rehashed.is_none(), "Argon2id PIN should not be rehashed");
+    }
+
+    #[test]
+    fn test_verify_argon2id_pin_wrong() {
+        let hash = hash_pin("5678".to_string()).unwrap();
+        let result = verify_pin("0000".to_string(), hash).unwrap();
+        assert!(!result.valid, "Wrong PIN should not verify");
+        assert!(result.rehashed.is_none());
+    }
+
+    #[test]
+    fn test_verify_legacy_sha256_correct_and_rehash() {
+        // Create a legacy SHA-256 hash: "salt_hex:sha256(salt_hex + pin)"
+        let salt_hex = "abcdef0123456789";
+        let mut hasher = Sha256::new();
+        hasher.update(salt_hex.as_bytes());
+        hasher.update(b"4321");
+        let hash_bytes = hasher.finalize();
+        let hash_hex = hex_encode(&hash_bytes);
+        let stored = format!("{}:{}", salt_hex, hash_hex);
+
+        let result = verify_pin("4321".to_string(), stored).unwrap();
+        assert!(result.valid, "Correct legacy PIN should verify");
+        assert!(result.rehashed.is_some(), "Legacy PIN should be rehashed to Argon2id");
+
+        // Verify the rehashed value is a valid Argon2id hash
+        let new_hash = result.rehashed.unwrap();
+        assert!(new_hash.starts_with("argon2id:"));
+
+        // Verify the rehashed value works for future verification
+        let result2 = verify_pin("4321".to_string(), new_hash).unwrap();
+        assert!(result2.valid, "Rehashed PIN should verify");
+        assert!(result2.rehashed.is_none(), "Already Argon2id, no rehash needed");
+    }
+
+    #[test]
+    fn test_verify_legacy_sha256_wrong() {
+        let salt_hex = "abcdef0123456789";
+        let mut hasher = Sha256::new();
+        hasher.update(salt_hex.as_bytes());
+        hasher.update(b"4321");
+        let hash_bytes = hasher.finalize();
+        let hash_hex = hex_encode(&hash_bytes);
+        let stored = format!("{}:{}", salt_hex, hash_hex);
+
+        let result = verify_pin("9999".to_string(), stored).unwrap();
+        assert!(!result.valid, "Wrong legacy PIN should not verify");
+        assert!(result.rehashed.is_none(), "Failed verification should not rehash");
+    }
+
+    #[test]
+    fn test_verify_invalid_format() {
+        let result = verify_pin("1234".to_string(), "invalid".to_string());
+        assert!(result.is_err(), "Single-part hash should fail");
+
+        let result = verify_pin("1234".to_string(), "argon2id:bad".to_string());
+        assert!(result.is_err(), "Argon2id with wrong part count should fail");
+    }
+
+    #[test]
+    fn test_hex_roundtrip() {
+        let original = vec![0u8, 127, 255, 1, 16];
+        let encoded = hex_encode(&original);
+        let decoded = hex_decode(&encoded).unwrap();
+        assert_eq!(original, decoded);
+    }
+}

src/components/profile/PinDialog.tsx

@@ -6,7 +6,7 @@ import { verifyPin } from "../../services/profileService";
 interface Props {
   profileName: string;
   storedHash: string;
-  onSuccess: () => void;
+  onSuccess: (rehashed?: string | null) => void;
   onCancel: () => void;
 }
 
@@ -41,9 +41,9 @@ export default function PinDialog({ profileName, storedHash, onSuccess, onCancel
       if (value && filledCount === index + 1) {
         setChecking(true);
         try {
-          const valid = await verifyPin(pin.replace(/\s/g, ""), storedHash);
+          const result = await verifyPin(pin.replace(/\s/g, ""), storedHash);
-          if (valid) {
+          if (result.valid) {
-            onSuccess();
+            onSuccess(result.rehashed);
           } else if (filledCount >= 6 || (filledCount >= 4 && index === filledCount - 1 && !value)) {
             setError(true);
             setDigits(["", "", "", "", "", ""]);
@@ -67,10 +67,10 @@ export default function PinDialog({ profileName, storedHash, onSuccess, onCancel
       const pin = digits.join("");
       if (pin.length >= 4) {
         setChecking(true);
-        verifyPin(pin, storedHash).then((valid) => {
+        verifyPin(pin, storedHash).then((result) => {
           setChecking(false);
-          if (valid) {
+          if (result.valid) {
-            onSuccess();
+            onSuccess(result.rehashed);
           } else {
             setError(true);
             setDigits(["", "", "", "", "", ""]);

src/components/profile/ProfileSwitcher.tsx

@@ -8,7 +8,7 @@ import type { Profile } from "../../services/profileService";
 
 export default function ProfileSwitcher() {
   const { t } = useTranslation();
-  const { profiles, activeProfile, switchProfile } = useProfile();
+  const { profiles, activeProfile, switchProfile, updateProfile } = useProfile();
   const [open, setOpen] = useState(false);
   const [pinProfile, setPinProfile] = useState<Profile | null>(null);
   const [showManage, setShowManage] = useState(false);
@@ -36,8 +36,11 @@ export default function ProfileSwitcher() {
     }
   };
 
-  const handlePinSuccess = () => {
+  const handlePinSuccess = async (rehashed?: string | null) => {
     if (pinProfile) {
+      if (rehashed) {
+        await updateProfile(pinProfile.id, { pin_hash: rehashed });
+      }
       switchProfile(pinProfile.id);
       setPinProfile(null);
     }

src/contexts/ProfileContext.tsx

@@ -46,7 +46,7 @@ interface ProfileContextValue {
   error: string | null;
   switchProfile: (id: string) => Promise<void>;
   createProfile: (name: string, color: string, pin?: string) => Promise<void>;
-  updateProfile: (id: string, updates: Partial<Pick<Profile, "name" | "color">>) => Promise<void>;
+  updateProfile: (id: string, updates: Partial<Pick<Profile, "name" | "color" | "pin_hash">>) => Promise<void>;
   deleteProfile: (id: string) => Promise<void>;
   setPin: (id: string, pin: string | null) => Promise<void>;
   connectActiveProfile: () => Promise<void>;
@@ -151,7 +151,7 @@ export function ProfileProvider({ children }: { children: ReactNode }) {
     }
   }, [state.config]);
 
-  const updateProfile = useCallback(async (id: string, updates: Partial<Pick<Profile, "name" | "color">>) => {
+  const updateProfile = useCallback(async (id: string, updates: Partial<Pick<Profile, "name" | "color" | "pin_hash">>) => {
     if (!state.config) return;
 
     const newProfiles = state.config.profiles.map((p) =>

src/pages/ProfileSelectionPage.tsx

@@ -8,7 +8,7 @@ import ProfileFormModal from "../components/profile/ProfileFormModal";
 
 export default function ProfileSelectionPage() {
   const { t } = useTranslation();
-  const { profiles, switchProfile } = useProfile();
+  const { profiles, switchProfile, updateProfile } = useProfile();
   const [pinProfileId, setPinProfileId] = useState<string | null>(null);
   const [showCreate, setShowCreate] = useState(false);
 
@@ -23,8 +23,11 @@ export default function ProfileSelectionPage() {
     }
   };
 
-  const handlePinSuccess = () => {
+  const handlePinSuccess = async (rehashed?: string | null) => {
     if (pinProfileId) {
+      if (rehashed) {
+        await updateProfile(pinProfileId, { pin_hash: rehashed });
+      }
       switchProfile(pinProfileId);
       setPinProfileId(null);
     }

src/services/profileService.ts

@@ -34,6 +34,12 @@ export async function hashPin(pin: string): Promise<string> {
   return invoke<string>("hash_pin", { pin });
 }
 
-export async function verifyPin(pin: string, storedHash: string): Promise<boolean> {
+export interface VerifyPinResult {
-  return invoke<boolean>("verify_pin", { pin, storedHash });
+  valid: boolean;
+  /** New Argon2id hash when a legacy SHA-256 PIN was re-hashed on successful verification */
+  rehashed: string | null;
+}
+
+export async function verifyPin(pin: string, storedHash: string): Promise<VerifyPinResult> {
+  return invoke<VerifyPinResult>("verify_pin", { pin, storedHash });
 }