6.4.1: Secure Storage on IOS Patch

[i5hi]

Fixes the secure storage issue on some iOS devices

[kumulynja]

Tested with existing app and with fresh install on a real iOS device. Both working and recipients working as well 👍

Migrations not tested though.

[leesalminen]

@claude please review this PR

[claude]

Claude finished @leesalminen's task in 1m 49s —— View job

---

Code Review: Secure Storage on iOS Patch

I've completed a thorough review of this PR. Here's my analysis:

---

Summary

This PR addresses iOS secure storage issues by refactoring how FlutterSecureStorage instances are created and shared across the application. The key change is removing the SecureStorage wrapper class and ensuring a single, properly configured instance is passed to all consumers.

---

✅ Positive Findings

1. iOS Entitlements Configuration

• Added proper keychain access group entitlements for both Debug/Profile and Release builds (ios/Runner/DebugProfile.entitlements:5-6, ios/Runner/Release.entitlements:5-6)
• Empty array configuration is correct for single-app keychain access
• This should resolve keychain access issues on iOS devices

2. Single Instance Pattern

• Critical fix: The refactoring eliminates multiple FlutterSecureStorage instances being created throughout the app
• In lib/core/storage/storage_locator.dart:21-24, a single FlutterSecureStorage instance is now created with consistent configuration
• This instance is passed to all consumers: SecureStorageDatasourceImpl, MigrationSecureStorageDatasource, and OldHiveDatasource.getBox()
• This prevents iOS keychain synchronization issues that can occur when multiple instances with different configurations access the same keys

3. Proper Android Configuration Maintained

• AndroidOptions(encryptedSharedPreferences: true) is correctly maintained (lib/core/storage/storage_locator.dart:22)

4. Migration Code Updated Correctly

• All three migration files properly updated to accept FlutterSecureStorage parameter
• lib/core/storage/migrations/004_legacy/migration01to02.dart:22, migration02to03.dart:15, migration03to04.dart:6
• Consistent pattern maintained across all migrations

5. Recipients Feature Refactored Cleanly

• BullbitcoinApiKeyProvider now receives storage via dependency injection (lib/features/recipients/frameworks/http/bullbitcoin_api_key_provider.dart:13-15)
• Proper use of the registered secure storage datasource instance (lib/features/recipients/recipients_locator.dart:30-32)

---

⚠️ Minor Observations

1. Commented iOS Options

// iOptions: IOSOptions(accessibility: KeychainAccessibility.first_unlock),

Location: lib/core/storage/storage_locator.dart:23

The iOS-specific options are commented out. This might be intentional to use default behavior, but worth noting:

• Default is KeychainAccessibility.unlocked_this_device
• If the app needs background keychain access, this might need to be uncommented
• Consider documenting why this was commented out if it's intentional

2. Migration Testing Gap
As noted by reviewer @kumulynja, migrations were not tested. While the code changes look correct, consider:

• Testing upgrade path from previous versions
• Verifying that seed data persists correctly after migration
• Ensuring no keychain data is lost during the upgrade

3. Error Handling in API Key Provider
In lib/features/recipients/frameworks/http/bullbitcoin_api_key_provider.dart:37-40:

} catch (e) {
  log.severe('Error retrieving API key: $e');
  return null;
}

The error is logged but swallowed. This is probably fine for this use case, but worth noting that API key retrieval failures will be silent to the user.

---

🔒 Security Review

✅ Good Security Practices:

• Proper use of Flutter Secure Storage for sensitive data (API keys, seeds)
• Android encrypted shared preferences enabled
• Keychain access properly scoped via entitlements
• No hardcoded secrets or credentials

No security issues identified.

---

📝 Code Quality

Strengths:

• Clean dependency injection pattern
• Consistent refactoring across all affected files
• Removal of unnecessary wrapper class reduces complexity
• Good separation of concerns maintained

No code quality issues identified.

---

✅ Recommendation

LGTM - Approve with minor notes

This is a well-executed fix for the iOS secure storage issue. The core problem (multiple FlutterSecureStorage instances) has been properly addressed by establishing a single instance pattern. The refactoring is clean and consistent across the codebase.

The commented iOS options line is the only item that might benefit from clarification, but it doesn't block approval.

Testing suggestion for users upgrading: Test with an existing app installation to ensure seed data and API keys migrate correctly.

---

Review Checklist

• Read and analyze changed files
• Review iOS entitlements files
• Review secure storage refactoring
• Review migration changes
• Review recipients feature changes
• Provide comprehensive feedback

---