Add boilerplate code option for handling cross device dismissal#192
Add boilerplate code option for handling cross device dismissal#192alabiaga merged 2 commits intoandroid:mainfrom
Conversation
Change-Id: Ib64421fe875781c518cccaf5866b019b00371be5
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request lays the groundwork for implementing cross-device notification dismissal within the Socialite application. It establishes a mechanism where dismissing a notification on one device can initiate its dismissal on other linked devices, enhancing the user experience in a multi-device environment. This is achieved through the introduction of a dedicated broadcast receiver for dismissal events and the integration of logic within the FCM messaging service to process and propagate these events. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces boilerplate code for handling cross-device notification dismissal using Firebase Cloud Messaging (FCM), including registering a new MessagingBroadcastReceiver, defining constants, modifying NotificationHelper, and updating MessagingService to process FCM messages. However, a security audit identified two medium-severity issues in MessagingService.kt: potential logging of sensitive information (PII) from the FCM data payload and a potential Denial of Service (DoS) vulnerability due to unvalidated input conversion. Additionally, there are general areas for improvement related to message handling and unnecessary imports. It is recommended to sanitize logs and use safer conversion methods like toIntOrNull() to address the security concerns.
| Log.d( | ||
| MessagingService::class::simpleName.toString(), | ||
| "Message data payload: ${remoteMessage.data}", | ||
| ) |
There was a problem hiding this comment.
The Log.d call on line 57 logs the entire FCM data payload, which can contain sensitive Personally Identifiable Information (PII). Logging PII to system logs is a security risk as it can be accessed by other applications or adb logcat. It is crucial to sanitize logs to prevent data leakage. Additionally, the overall message processing logic (lines 45-68) should be reviewed to ensure remoteMessage.notification payloads are handled independently of remoteMessage.data to avoid missed notifications, as currently, notification payloads might not be processed if the data payload is empty.
| MessagingService::class::simpleName.toString(), | ||
| "Message data payload: ${remoteMessage.data}", | ||
| ) | ||
| notificationManager.cancel(remoteMessage.data[NOTIFICATION_ID]!!.toInt()) |
There was a problem hiding this comment.
The code calls toInt() on a value retrieved from the FCM data payload (remoteMessage.data[NOTIFICATION_ID]) without any error handling. If an attacker can send a malformed FCM message where this value is not a valid integer (e.g., a string, a very large number, or an empty string), the toInt() call will throw a NumberFormatException, causing the MessagingService to crash. While the service may be restarted by the system, this still represents a potential Denial of Service.
| notificationManager.cancel(remoteMessage.data[NOTIFICATION_ID]!!.toInt()) | |
| remoteMessage.data[NOTIFICATION_ID]?.toIntOrNull()?.let { notificationManager.cancel(it) } |
| import android.os.Build | ||
| import androidx.annotation.RequiresApi | ||
| import androidx.annotation.WorkerThread | ||
| import androidx.compose.runtime.snapshots.toInt |
There was a problem hiding this comment.
The import androidx.compose.runtime.snapshots.toInt is likely unnecessary. The toInt() extension function used later in the code (e.g., contact.id.toInt()) is a standard Kotlin function available for various number types and Strings, and does not require this specific Compose-related import. Importing unused or incorrect dependencies can lead to larger binary sizes and potential confusion.
Change-Id: Ib64421fe875781c518cccaf5866b019b00371be5