Changes to enable Firebase cloud messaging (FCM)#191
Conversation
Change-Id: I5d0e9105de0d413714c613a5701688a8a02c2112
alabiaga
requested review from
SigmanZero,
donovanfm and
madebymozart
as code owners
alabiaga
requested review from
ashnohe
and removed request for
SigmanZero,
donovanfm and
madebymozart
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 introduces the foundational changes required to integrate Firebase Cloud Messaging (FCM) into the application. The primary goal is to enable the app to receive push notifications by setting up the necessary Firebase dependencies, configuring the Android manifest for FCM services, and implementing a custom service to process incoming messages and manage device registration tokens. This initial integration follows best practices for FCM setup, focusing on the interplay with the notification framework rather than just basic message delivery. 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
Activity
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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request integrates Firebase Cloud Messaging (FCM) into the application, including necessary dependencies, AndroidManifest configuration, and basic logic for token handling and message receiving. However, it introduces a security vulnerability by logging sensitive information such as FCM registration tokens, message data payloads, and notification bodies to the system log, which can expose device identifiers and user data. It is recommended to remove these logging statements or ensure they are only active in non-production environments. Additionally, there is a suggestion in MainActivity.kt to improve the idiomatic and readability of the token retrieval logic.
| } // Get new FCM registration token | ||
| val token = task.result | ||
| // Log token | ||
| Log.d("FCM", "FCM message token $token") |
There was a problem hiding this comment.
This line logs the Firebase Cloud Messaging (FCM) registration token, which is a sensitive identifier. Logging FCM tokens can lead to unauthorized access if logs are intercepted. It is critical to remove this logging statement or ensure it only executes in debug builds. Additionally, the broader block (lines 68-78) could be simplified for more idiomatic Kotlin, making the OnCompleteListener explicit redundant and improving readability by restructuring success/failure handling.
| // Token used for device targeting. | ||
| // See https://firebase.google.com/docs/cloud-messaging/android/get-started#access-fcm-registration-token | ||
| super.onNewToken(token) | ||
| Log.d("FCM", "Refreshed token: $token") |
There was a problem hiding this comment.
The application logs the refreshed FCM registration token in the onNewToken callback. This exposes the token in the system logs, which is a security risk. It is recommended to remove this logging statement or restrict it to non-production environments.
|
|
||
| // Handle data payload | ||
| if (remoteMessage.data.isNotEmpty()) { | ||
| Log.d("FCM", "Message data payload: ${remoteMessage.data}") |
There was a problem hiding this comment.
The application logs the entire data payload of incoming FCM messages. This payload can contain sensitive application data or PII, which should not be exposed in system logs. It is recommended to avoid logging the entire data payload. If logging is necessary for debugging, log only non-sensitive fields or ensure logging is disabled in production.
|
|
||
| // Handle notification payload | ||
| remoteMessage.notification?.let { | ||
| Log.d("FCM", "Message Notification Body: ${it.body}") |
There was a problem hiding this comment.
The application logs the body of incoming FCM notifications. Notification bodies often contain sensitive user content or PII. It is recommended to remove the logging of the notification body to protect user privacy.
Change-Id: I7d942068896b3b9419554e2f75079dd584167487
Change-Id: Id33b31250bdb92d02665d0cf1adf6034fa320581
Change-Id: Ia66dc2932c8e90699bd12692fe68c6a56c6ec1c3
2 participants
Add changes to integrate usage of FCM.
These initial changes demonstrate how to integrate FCM while following best practices. While Firebase provides its own quickstart-android app, this implementation focuses specifically on the interplay between FCM and the notification framework, best practices, vs basic message facilitation.