Sensitive clipboard handling for Android private key copying #522
Conversation
WalkthroughAdds an Android MethodChannel to mark clipboard content as sensitive, a new Dart utility to copy text via that channel with fallback to standard clipboard, and updates the nsec copy flow in the profile keys screen to use the sensitive copy path asynchronously. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User as User
participant UI as ProfileKeysScreen
participant Utils as ClipboardUtils
participant Channel as MethodChannel("clipboard_sensitive")
participant Android as MainActivity
participant CB as ClipboardManager
User->>UI: Tap "Copy nsec"
UI->>Utils: copySensitiveWithToast(text)
alt Android channel available
Utils->>Channel: invokeMethod("setSensitive", text)
alt Channel handles setSensitive
Channel->>Android: setSensitive(text)
Android->>CB: setPrimaryClip(ClipData + EXTRA_IS_SENSITIVE)
Android-->>Channel: success
Channel-->>Utils: success
Utils-->>UI: show success toast
else Channel error / notImplemented
Channel-->>Utils: error
Utils->>CB: setPrimaryClip(ClipData) -- fallback
Utils-->>UI: show success/error toast
end
else No channel / non-Android
Utils->>CB: setPrimaryClip(ClipData)
Utils-->>UI: show success/error toast
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![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/utils/clipboard_utils.dart (2)
46-60: Add parameter validation and improve documentation.The method signature has inconsistent nullability between
textToCopyparameters in both methods, and the documentation could be clearer about the Android-specific behavior.Apply this diff to improve consistency and documentation:
/// Copies text via platform-specific sensitive path (Android) with a success toast. - /// Falls back to regular copy on non-Android platforms. + /// Falls back to regular copy on non-Android platforms or if the sensitive channel fails. + /// On Android 12+, marks clipboard content as sensitive to prevent preview display. /// /// [ref] - WidgetRef for accessing providers - /// [textToCopy] - The text to copy to clipboard + /// [textToCopy] - The text to copy to clipboard (must not be null or empty) /// [successMessage] - Optional custom message to show (defaults to "Copied to clipboard") /// [noTextMessage] - Optional custom error message to show when there is no text to copy (defaults to "Nothing to copy") /// [errorMessage] - Optional custom error message to show when clipboard operation fails (defaults to "Failed to copy to clipboard") static Future<void> copySensitiveWithToast({ required WidgetRef ref, - required String? textToCopy, + required String textToCopy, String? successMessage, String? noTextMessage, String? errorMessage, }) async { - if (textToCopy == null || textToCopy.isEmpty) { + if (textToCopy.isEmpty) { ref.showErrorToast(noTextMessage ?? 'Nothing to copy'); return; }
66-88: Improve error handling granularity and logging.The current implementation catches all exceptions from the sensitive channel and falls back silently. Consider differentiating between platform unavailability and actual errors for better debugging.
Apply this diff to improve error handling:
try { // Try Android sensitive channel first; if it throws, fall back to Clipboard.setData await _sensitiveChannel.invokeMethod('setSensitive', {'text': textToCopy}); ref.showSuccessToast( successMessage ?? 'Copied to clipboard', autoDismiss: true, ); - } catch (_) { + } catch (e) { + // Log the channel error for debugging (optional) + // print('Sensitive clipboard channel failed: $e'); // Fallback for non-Android or if channel not available try { await Clipboard.setData(ClipboardData(text: textToCopy)); ref.showSuccessToast( successMessage ?? 'Copied to clipboard', autoDismiss: true, ); } catch (e) { ref.showErrorToast( errorMessage ?? 'Failed to copy to clipboard', autoDismiss: true, ); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
android/app/src/main/kotlin/com/example/whitenoise/MainActivity.kt(1 hunks)lib/ui/settings/profile_keys/profile_keys_screen.dart(1 hunks)lib/utils/clipboard_utils.dart(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.dart
📄 CodeRabbit Inference Engine (.cursor/rules/flutter.mdc)
**/*.dart: Always declare the type of each variable and function (parameters and return value). Avoid using 'any'. Create necessary types.
Don't leave blank lines within a function.
One export per file.
Use PascalCase for classes.
Use camelCase for variables, functions, and methods.
Use underscores_case for file and directory names.
Use UPPERCASE for environment variables. Avoid magic numbers and define constants.
Start each function with a verb.
Use verbs for boolean variables. Example: isLoading, hasError, canDelete, etc.
Use complete words instead of abbreviations and correct spelling, except for standard and well-known abbreviations (API, URL, i, j, err, ctx, req, res, next).
Write short functions with a single purpose. Less than 20 instructions.
Name functions with a verb and something else. If it returns a boolean, use isX or hasX, canX, etc. If it doesn't return anything, use executeX or saveX, etc.
Avoid nesting blocks by early checks and returns, or extraction to utility functions.
Use higher-order functions (map, filter, reduce, etc.) to avoid function nesting. Use arrow functions for simple functions (less than 3 instructions). Use named functions for non-simple functions.
Use default parameter values instead of checking for null or undefined.
Reduce function parameters using RO-RO: use an object to pass multiple parameters and to return results. Declare necessary types for input arguments and output.
Use a single level of abstraction in functions.
Don't abuse primitive types and encapsulate data in composite types.
Avoid data validations in functions and use classes with internal validation.
Prefer immutability for data. Use readonly for data that doesn't change. Use 'as const' for literals that don't change.
Declare interfaces to define contracts.
Write small classes with a single purpose. Less than 200 instructions, less than 10 public methods, less than 10 properties.
Use exceptions to handle errors you don't expect. If you catch an exception, it sh...
Files:
lib/ui/settings/profile_keys/profile_keys_screen.dartlib/utils/clipboard_utils.dart
🔇 Additional comments (6)
lib/utils/clipboard_utils.dart (1)
10-11: LGTM! Correctly defined Android-specific channel.The channel name matches the Android implementation and follows Flutter's method channel conventions.
lib/ui/settings/profile_keys/profile_keys_screen.dart (1)
47-56: LGTM! Correctly implemented async private key copying with sensitive handling.The method signature change to async and the switch to
copySensitiveWithToastproperly implements the security enhancement for private key copying. The null check ensures safe operation.android/app/src/main/kotlin/com/example/whitenoise/MainActivity.kt (4)
3-7: LGTM! All necessary imports added.The imports correctly include all Android clipboard and bundle classes needed for the sensitive clipboard implementation.
15-31: LGTM! Proper method channel implementation.The method channel handler correctly:
- Validates the method name
- Extracts and validates the text argument
- Calls the sensitive clipboard function
- Returns appropriate success/error responses
33-44: LGTM! Correct sensitive clipboard implementation.The method properly:
- Retrieves the system clipboard manager
- Creates clip data with the provided text
- Adds the sensitive bundle for supported API levels
- Sets the clip to clipboard
Once the API level condition is fixed, this implementation will work correctly.
12-14: Channel name consistency verifiedThe Dart implementation (
MethodChannel('clipboard_sensitive')) and the Kotlin implementation (private val CHANNEL = "clipboard_sensitive") already match exactly. No changes are necessary.Likely an incorrect or invalid review comment.
| val clipboard = getSystemService(CLIPBOARD_SERVICE) as ClipboardManager | ||
| val clip = ClipData.newPlainText("label", text) | ||
|
|
||
| if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISU || Build.VERSION.SDK_INT >= Build.VERSION_CODES.S) { |
There was a problem hiding this comment.
Fix API level condition logic.
The condition uses OR (||) when it should use AND (&&) or be simplified, since TIRAMISU (33) is higher than S (31).
Apply this diff to fix the API level check:
- if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISU || Build.VERSION.SDK_INT >= Build.VERSION_CODES.S) {
+ if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.S) {The EXTRA_IS_SENSITIVE feature was introduced in API level 31 (Android S), so checking for S covers both S and TIRAMISU.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISU || Build.VERSION.SDK_INT >= Build.VERSION_CODES.S) { | |
| if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.S) { |
🤖 Prompt for AI Agents
In android/app/src/main/kotlin/com/example/whitenoise/MainActivity.kt around
line 37, the API level check uses an OR between TIRAMISU and S which is
incorrect; replace the condition with a single check for Build.VERSION.SDK_INT
>= Build.VERSION_CODES.S (or use AND if you intended to require both, but here S
alone suffices) so the code correctly detects API level 31+ where
EXTRA_IS_SENSITIVE exists.
Quwaysim
requested review from
codeswot,
erskingardner,
josefinalliende and
untreu2
and removed request for
erskingardner
…ps://github.com/parres-hq/whitenoise_flutter into 503-mask-nsec-copy-clipboard-preview-on-android
|
Amazing 🙌. Other than the CodeRabbit comments I think this looks good. Also this makes me wonder what happens on other platforms like iOS and eventually desktop? Do we create method channels for them too ? |
Thank you @codeswot. I'll check coderabbit's comment now. For desktop, I guess we'll have to explicitly test on desktops before we know if this issue occurs there too. AFAIK, it doesn't happen on Windows and Mac. Unless you are suggesting something different? Like maybe auto-clearing the OS' clipboard after a specific time? I thought of it but I'm not sure if we have to do that. |
Ohk , I get the full context now. |
The problem for me now is, how soon do we clear it. We don't want to clear too early to avoid frustrating users and we shouldn't clear too late to avoid exposing it too long. Also, something crossed my mind. Even if we clear the clipboard, for apps (like Swiftkey keyboard) that maintain their own clipboard, will we be able to clear theirs too? I'll check on my end, please check too. Maybe we move this discussion to the issue itself? |
By all means please handle this on a separate issue, I was only curious and asking. Let's create an issue to handle the case of clearing clipboard after copying sensitive data. If we need to . Not sure if we do but it's worth researching |
|
Mmm, I guess clearing the clipboard is outside the scope of what the app can safely do 🤔 . It’s hard to know whether the clipboard content came from our app or from somewhere else… and it could be really annoying if we end up deleting something the user copied intentionally. For example, imagine I copy my nsec in our app, then switch to another app and copy a different message. If a scheduled task later clears the clipboard, that new copied content would also be lost. |
Description
Fixes #503
Added a method channel to let Android 13+ know that what the user is copying is sensitive, thereby masking it in the copy preview.
Benefit
Added security.
Notes
New method in ClipboardUtils now called
copySensitiveWithToastto be used in place ofcopyWithToastwhen copying sensitive data like nsec.Type of Change
Checklist
just precommitto ensure that formatting and linting are correctjust check-flutter-coverageto ensure that flutter coverage rules are passingCHANGELOG.mdfile with your changes (if they affect the user experience)Summary by CodeRabbit
New Features
Chores