Disable backup to prevent unintended data restore on Android#4393
Disable backup to prevent unintended data restore on Android#4393
Conversation
WalkthroughThis pull request introduces platform-specific encryption key management for Hive caching. On Android, a new 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
lib/features/caching/manager/app_security_manager.dart (1)
83-94: Consider logging additional context when wiping due to inconsistent state.When
hiveExists && storedKey == null, the app wipes all local data. This is a significant action triggered by backup restore detection. Consider:
- Adding telemetry/analytics to track how often this occurs
- Logging the number of
.hivefiles found for debugging purposesThis would help diagnose if legitimate users are unexpectedly hitting this path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/caching/manager/app_security_manager.dart` around lines 83 - 94, In _handleInconsistentState, before calling _wipeLocalStorageSafe(), add richer diagnostic logging and telemetry: compute/count the number of .hive files found (e.g., scan the Hive directory) and include that count and hiveExists/storedKey values in the logWarning message, and emit a telemetry/analytics event (e.g., TelemetryService.trackEvent or Analytics.logEvent) named something like "inconsistent_state_wipe" with metadata {hiveCount, hiveExists, storedKeyNull:true, timestamp}. Keep calling _wipeLocalStorageSafe() afterwards; update references to logWarning, _handleInconsistentState, and _wipeLocalStorageSafe so reviewers can find the change.android/app/src/main/AndroidManifest.xml (1)
39-40: Addandroid:dataExtractionRulesattribute and configuration file for Android 12+ compatibility.The
android:allowBackup="false"andandroid:fullBackupContent="false"attributes correctly disable backup for Android 6.0–11, but Android 12 (API 31) and above require theandroid:dataExtractionRulesattribute. Addandroid:dataExtractionRules="@xml/data_extraction_rules"to the<application>element and createres/xml/data_extraction_rules.xmlwith domain exclusions for both<cloud-backup>and<device-transfer>sections (excluding root, file, database, sharedpref, and external domains). Keep the existing allowBackup and fullBackupContent attributes for backward compatibility with older devices.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/AndroidManifest.xml` around lines 39 - 40, Add android:dataExtractionRules="@xml/data_extraction_rules" to the <application> element in AndroidManifest (alongside the existing android:allowBackup="false" and android:fullBackupContent="false") and create a new resource file res/xml/data_extraction_rules.xml that defines <data-extraction-rules> containing both <cloud-backup> and <device-transfer> sections; in each section include domain exclusions for root, file, database, sharedpref, and external so those domains are excluded from backup/transfer on Android 12+.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/features/caching/manager/app_security_manager.dart`:
- Around line 186-194: The current _wipeSharePreferenceSafe method calls
SharedPreferences.clear(), which removes all app-wide prefs (auth, OIDC config,
credentials, settings); instead, delete only the Hive/cache-related keys: obtain
SharedPreferences via SharedPreferences.getInstance() and call remove(key) for
each cache-specific key used by your codebase (the keys that track Hive cache
state and cache version), or, if a full app reset is truly intended for the
backup-restore path, document this behavior and keep clear() behind an explicit
flag; update the _wipeSharePreferenceSafe implementation to iterate and remove
only those cache keys rather than calling SharedPreferences.clear().
---
Nitpick comments:
In `@android/app/src/main/AndroidManifest.xml`:
- Around line 39-40: Add
android:dataExtractionRules="@xml/data_extraction_rules" to the <application>
element in AndroidManifest (alongside the existing android:allowBackup="false"
and android:fullBackupContent="false") and create a new resource file
res/xml/data_extraction_rules.xml that defines <data-extraction-rules>
containing both <cloud-backup> and <device-transfer> sections; in each section
include domain exclusions for root, file, database, sharedpref, and external so
those domains are excluded from backup/transfer on Android 12+.
In `@lib/features/caching/manager/app_security_manager.dart`:
- Around line 83-94: In _handleInconsistentState, before calling
_wipeLocalStorageSafe(), add richer diagnostic logging and telemetry:
compute/count the number of .hive files found (e.g., scan the Hive directory)
and include that count and hiveExists/storedKey values in the logWarning
message, and emit a telemetry/analytics event (e.g., TelemetryService.trackEvent
or Analytics.logEvent) named something like "inconsistent_state_wipe" with
metadata {hiveCount, hiveExists, storedKeyNull:true, timestamp}. Keep calling
_wipeLocalStorageSafe() afterwards; update references to logWarning,
_handleInconsistentState, and _wipeLocalStorageSafe so reviewers can find the
change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 42afa6f2-636a-4465-bfc9-a1b5915e751a
📒 Files selected for processing (7)
android/app/src/main/AndroidManifest.xmllib/features/caching/config/hive_cache_client.dartlib/features/caching/config/secure_storage_factory.dartlib/features/caching/config/secure_storage_keys.dartlib/features/caching/manager/app_security_manager.dartlib/main/bindings/core/core_bindings.dartlib/main/main_entry.dart
|
This PR has been deployed to https://linagora.github.io/tmail-flutter/4393. |
|
Cc @Crash-- |
|
Could I have a summary of:
|
Starting with Android 6.0 Marshmallow , allowBackup is enabled by default, allowing apps to automatically back up data such as shared preferences, databases, and internal files. When users enable device backup, Android uploads this data to their cloud (e.g., Google Drive). Upon reinstalling the app or setting up a new device, the system automatically restores the data before the app is first launched.
It was added to improve user experience by preserving app data across device changes or reinstalls. This reduces friction by avoiding data loss, keeping users logged in, and restoring app state automatically.
Yes, it is common practice. Auto Backup is enabled by default on Android and widely used. However, apps handling sensitive data (e.g., auth tokens, encryption keys) often disable it or exclude specific data for security reasons.
Yes, it introduces trade-offs. Disabling Auto Backup improves security and avoids data/key mismatch issues, but users will lose local data on reinstall or device change and need to log in again, resulting in a less seamless experience. |
Oh great, this is the part I didn't find on the documentation. But I can read it from your link! So yeah the only down side is when I transfer my phone to a new one, I need to log in again. Not a big deal! |
| logWarning('$runtimeType::_getEncryptionKey: Exception $e'); | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
I would have created an interface :
abstract class EncryptionKeyProvider {
Future<void> init();
Future<Uint8List?> getKey();
void clearKey();
}
then 2 implem:
class AndroidEncryptionKeyProvider implements EncryptionKeyProvider {
@override
Future<void> init() => AppSecurityManager.instance.init();
@override
Future<Uint8List?> getKey() => AppSecurityManager.instance.getKey();
@override
void clearKey() => AppSecurityManager.instance.clearKey();
}
class DefaultEncryptionKeyProvider implements EncryptionKeyProvider {
@OverRide
Future init() => HiveCacheConfig.instance.initializeEncryptionKey();
@override
Future<Uint8List?> getKey() => HiveCacheConfig.instance.getEncryptionKey();
@override
void clearKey() {
}
}
and a factory:
class EncryptionKeyProviderFactory {
const EncryptionKeyProviderFactory._();
static EncryptionKeyProvider create() {
if (PlatformInfo.isAndroid) {
return AndroidEncryptionKeyProvider();
}
return DefaultEncryptionKeyProvider();
}
}
Like that "no more" `if(Platform...)`. And maybe it'll be easier to add unit tests for this part. Then if tomorrow we want to use the Apple KeyStore, then we can easily add it and etc.
|
Thanks for the great writeup @dab246 That's ok to me! |
3 participants
Issue
https://github.com/linagora/james-project-private/issues/1177
Summary by CodeRabbit
Release Notes