Fix: Critical Crash Bugs and Security Vulnerabilities

CODE_AUDIT_REPORT.md

@@ -0,0 +1,339 @@
+# VoiceInk Code Audit Report
+**Date:** 2025-11-08  
+**Auditor:** AI Code Review  
+**Scope:** Comprehensive bug and security review
+
+---
+
+## Executive Summary
+
+Comprehensive audit of 220+ Swift files identified **11 issues**: 4 critical crashes, 2 critical security vulnerabilities, 3 medium-priority bugs, and 2 low-priority code quality items.
+
+**Critical Issues:** 6  
+**Medium Priority:** 3  
+**Low Priority:** 2  
+**Total:** 11 issues
+
+---
+
+## 🔴 CRITICAL ISSUES (Priority: HIGH)
+
+### 1. Implicitly Unwrapped Optional - Potential Crash
+**File:** `VoiceInk/Whisper/WhisperState.swift:72`  
+**Issue:** `private var localTranscriptionService: LocalTranscriptionService!`  
+**Risk:** App crash if accessed before initialization completes  
+**Fix:** Change to regular optional and use guard statements or optional chaining
+
+```swift
+// Current (unsafe):
+private var localTranscriptionService: LocalTranscriptionService!
+
+// Recommended:
+private var localTranscriptionService: LocalTranscriptionService?
+```
+
+---
+
+### 2. Force Cast - Potential Crash
+**File:** `VoiceInk/Services/PasteEligibilityService.swift:25`  
+**Issue:** `let axElement = (element as! AXUIElement)`  
+**Risk:** App crash if `element` is not an `AXUIElement`  
+**Fix:** Use safe cast with `as?`
+
+```swift
+// Current (unsafe):
+let axElement = (element as! AXUIElement)
+
+// Recommended:
+guard let axElement = element as? AXUIElement else {
+    return false
+}
+```
+
+---
+
+### 3. Force Unwraps in Transcription Manager - Potential Crash
+**File:** `VoiceInk/Services/AudioFileTranscriptionManager.swift:104,106`  
+**Issue:** `localTranscriptionService!` and `parakeetTranscriptionService!`  
+**Risk:** App crash if services are nil  
+**Fix:** Use guard statements or optional chaining
+
+```swift
+// Current (unsafe):
+text = try await localTranscriptionService!.transcribe(...)
+
+// Recommended:
+guard let service = localTranscriptionService else {
+    throw TranscriptionError.serviceNotAvailable
+}
+text = try await service.transcribe(...)
+```
+
+---
+
+### 4. Force Unwrap URL Construction - Potential Crash
+**File:** `VoiceInk/Services/PolarService.swift:13`  
+**Issue:** `let url = URL(string: "\(baseURL)\(endpoint)")!`  
+**Risk:** App crash if URL string is malformed  
+**Fix:** Use guard statement
+
+```swift
+// Current (unsafe):
+let url = URL(string: "\(baseURL)\(endpoint)")!
+
+// Recommended:
+guard let url = URL(string: "\(baseURL)\(endpoint)") else {
+    throw PolarError.invalidURL
+}
+```
+
+---
+
+### 5. 🔒 SECURITY: API Keys in UserDefaults (Cloud Transcription)
+**Files:** 
+- `Services/CloudTranscription/GroqTranscriptionService.swift:39`
+- `Services/CloudTranscription/ElevenLabsTranscriptionService.swift:36`
+- `Services/CloudTranscription/DeepgramTranscriptionService.swift:45`
+- `Services/CloudTranscription/MistralTranscriptionService.swift:9`
+- `Services/CloudTranscription/GeminiTranscriptionService.swift:78`
+- `Services/CloudTranscription/SonioxTranscriptionService.swift:21`
+
+**Issue:** API keys stored in UserDefaults (plain text plist file)  
+**Risk:** API keys readable by anyone with filesystem access  
+**Fix:** Migrate to KeychainManager (already implemented for TTS services)
+
+```swift
+// Current (INSECURE):
+let apiKey = UserDefaults.standard.string(forKey: "GROQAPIKey")
+
+// Recommended:
+let apiKey = KeychainManager().getAPIKey(for: "Groq")
+```
+
+**Migration Path:**
+1. Update all 6 cloud transcription services to use `KeychainManager`
+2. Add migration code to move existing keys from UserDefaults to Keychain
+3. Remove old UserDefaults keys after migration
+
+---
+
+### 6. 🔒 SECURITY: API Keys in UserDefaults (AI Enhancement)
+**Files:**
+- `Services/AIService.swift:170,202,235,293,523`
+- `Views/AI Models/CloudModelCardRowView.swift:23,263,302,320`
+
+**Issue:** AI enhancement API keys stored in UserDefaults  
+**Risk:** API keys readable by anyone with filesystem access  
+**Fix:** Same as #5 - use KeychainManager
+
+---
+
+## 🟡 MEDIUM PRIORITY ISSUES
+
+### 7. Force Unwrap in String Encoding
+**Files:** 5 cloud transcription services (multipart form data encoding)
+- `SonioxTranscriptionService.swift:163-168`
+- `GroqTranscriptionService.swift:60-95`
+- `ElevenLabsTranscriptionService.swift:55-86`
+- `OpenAICompatibleTranscriptionService.swift:57-92`
+- `MistralTranscriptionService.swift:28-43`
+
+**Issue:** `.data(using: .utf8)!` force unwraps  
+**Risk:** Crash if string contains invalid UTF-8 (very unlikely but possible)  
+**Fix:** Use guard statements with proper error handling
+
+```swift
+// Current (unsafe):
+body.append("--\(boundary)\(crlf)".data(using: .utf8)!)
+
+// Recommended:
+guard let data = "--\(boundary)\(crlf)".data(using: .utf8) else {
+    throw TranscriptionError.encodingFailed
+}
+body.append(data)
+```
+
+**Note:** This is low-risk in practice since hardcoded ASCII strings always encode to UTF-8, but it violates Swift safety principles.
+
+---
+
+### 8. Force Unwrap After isEmpty Check
+**File:** `VoiceInk/Services/LastTranscriptionService.swift:128`  
+**Issue:** `newTranscription.enhancedText!` after checking `isEmpty == false`  
+**Risk:** Logic error could cause crash  
+**Fix:** Use nil-coalescing or proper optional binding
+
+```swift
+// Current (risky):
+let textToCopy = newTranscription.enhancedText?.isEmpty == false ? newTranscription.enhancedText! : newTranscription.text
+
+// Recommended:
+let textToCopy = (newTranscription.enhancedText?.isEmpty == false ? newTranscription.enhancedText : nil) ?? newTranscription.text
+```
+
+---
+
+### 9. Unsafe Buffer Access
+**File:** `VoiceInk/Services/VoiceActivityDetector.swift:48`  
+**Issue:** `buffer.baseAddress!` force unwrap  
+**Risk:** Crash if buffer is empty  
+**Fix:** Use guard statement
+
+```swift
+// Current (unsafe):
+whisper_vad_detect_speech(vadContext, buffer.baseAddress!, Int32(audioSamples.count))
+
+// Recommended:
+guard let baseAddress = buffer.baseAddress else {
+    logger.error("Buffer has no base address")
+    return []
+}
+whisper_vad_detect_speech(vadContext, baseAddress, Int32(audioSamples.count))
+```
+
+---
+
+## 🟢 LOW PRIORITY (Code Quality)
+
+### 10. Non-Idiomatic Optional Handling
+**File:** `VoiceInk/Whisper/WhisperState+ModelQueries.swift:18-34`  
+**Issue:** Pattern `key != nil && !key!.isEmpty` (6 occurrences)  
+**Risk:** None (safe but verbose)  
+**Fix:** Use optional binding
+
+```swift
+// Current (safe but verbose):
+let key = UserDefaults.standard.string(forKey: "GROQAPIKey")
+return key != nil && !key!.isEmpty
+
+// Recommended (idiomatic):
+if let key = UserDefaults.standard.string(forKey: "GROQAPIKey"), !key.isEmpty {
+    return true
+}
+return false
+```
+
+---
+
+### 11. Force Try in Preview Code
+**File:** `VoiceInk/Views/KeyboardShortcutCheatSheet.swift:248`  
+**Issue:** `try! ModelContainer(for: Transcription.self)`  
+**Risk:** None (preview code only)  
+**Note:** Acceptable in SwiftUI preview contexts
+
+---
+
+## ✅ POSITIVE FINDINGS
+
+### Good Practices Observed:
+1. **Memory Management:** Extensive use of `[weak self]` in closures (60+ instances)
+2. **Concurrency:** Proper `@MainActor` annotations on UI classes
+3. **Security:** TTS services correctly use KeychainManager for API keys
+4. **Error Handling:** Most async operations properly wrapped in do-catch
+5. **No Debug Logging:** No `print()` statements logging sensitive data
+6. **Quick Rules Feature:** Correctly defaults to OFF, safe regex handling
+
+---
+
+## PRIORITY RECOMMENDATIONS
+
+### Immediate (Before Next Release):
+1. **Fix #1-4:** Eliminate crash-prone force unwraps and force casts
+2. **Fix #5-6:** Migrate API keys from UserDefaults to Keychain
+
+### Short Term (Next Sprint):
+3. **Fix #7-9:** Replace remaining force unwraps with safe unwrapping
+4. **Add Tests:** Unit tests for critical paths (transcription, API calls)
+
+### Long Term (Backlog):
+5. **Fix #10:** Refactor to idiomatic Swift optional handling
+6. **Security Audit:** Penetration test after Keychain migration
+7. **Code Signing:** Re-enable for test suite execution
+
+---
+
+## TESTING RECOMMENDATIONS
+
+Since you cannot run the test suite currently, prioritize manual testing of:
+
+1. **Audio File Transcription** (tests force unwraps in #3)
+2. **Paste Eligibility** (tests force cast in #2)
+3. **Voice Activity Detection** (tests buffer unwrap in #9)
+4. **Cloud Transcription** with all 6 providers (tests API key handling)
+5. **App Launch** without models downloaded (tests #1)
+
+---
+
+## MIGRATION GUIDE: API Keys to Keychain
+
+```swift
+// Step 1: Add migration function to AppDelegate or similar
+func migrateAPIKeysToKeychain() {
+    let keychain = KeychainManager()
+    let defaults = UserDefaults.standard
+
+    let providersToMigrate = [
+        ("GROQAPIKey", "Groq"),
+        ("ElevenLabsAPIKey", "ElevenLabs"),
+        ("DeepgramAPIKey", "Deepgram"),
+        ("MistralAPIKey", "Mistral"),
+        ("GeminiAPIKey", "Gemini"),
+        ("SonioxAPIKey", "Soniox"),
+        // AI enhancement providers
+        ("OpenAIAPIKey", "OpenAI"),
+        ("GroqAPIKey", "Groq"),
+        // ... etc
+    ]
+
+    for (oldKey, provider) in providersToMigrate {
+        if let apiKey = defaults.string(forKey: oldKey), !apiKey.isEmpty {
+            // Save to keychain
+            keychain.saveAPIKey(apiKey, for: provider)
+            // Remove from UserDefaults
+            defaults.removeObject(forKey: oldKey)
+            print("Migrated \(provider) API key to Keychain")
+        }
+    }
+
+    defaults.synchronize()
+}
+
+// Step 2: Call once on app launch
+// VoiceInk.swift - in init() or applicationDidFinishLaunching
+if !UserDefaults.standard.bool(forKey: "hasCompletedAPIKeyMigration") {
+    migrateAPIKeysToKeychain()
+    UserDefaults.standard.set(true, forKey: "hasCompletedAPIKeyMigration")
+}
+```
+
+---
+
+## CONCLUSION
+
+The codebase is generally well-structured with good memory management practices. The main concerns are:
+
+1. **4 crash-prone force unwraps/casts** that should be fixed immediately
+2. **API keys in UserDefaults** - significant security vulnerability requiring migration
+3. **Minor force unwraps** in string encoding (low practical risk)
+
+**Estimated Effort:**
+- Critical fixes (#1-4): 2-4 hours
+- Security fixes (#5-6): 4-8 hours (includes migration testing)
+- Medium priority (#7-9): 2-3 hours
+- Total: ~1-2 days of development time
+
+**Risk Assessment:** MEDIUM  
+Without fixes, users may experience crashes in specific scenarios (rare but possible). API keys are exposed to local filesystem access (higher risk if device is compromised).
+
+---
+
+**Next Steps:**
+1. Review this report with team
+2. Prioritize fixes based on release timeline
+3. Create tickets for each issue
+4. Implement fixes in order of priority
+5. Test thoroughly before release
+
+---
+
+*Generated by automated code audit - Review findings before implementing fixes*