Conversation
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 the 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 counterproductive. 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. |
|
/gemini review |
![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 introduces type-safety checks for fields extracted from untrusted push payloads in FIRMessagingContextManagerService to prevent crashes caused by unexpected data types, and adds corresponding unit tests. The review feedback correctly identifies additional locations in the same file where untrusted payload fields (such as kFIRMessagingContextManagerLocalTimeStart in handleContextManagerMessage: and kFIRMessagingContextManagerLocalTimeEnd) are still accessed without validation, presenting further opportunities for crashes that should be addressed.
| id startTime = message[kFIRMessagingContextManagerLocalTimeStart]; | ||
| if (![startTime isKindOfClass:[NSString class]] || ![(NSString *)startTime length]) { |
There was a problem hiding this comment.
While guarding kFIRMessagingContextManagerLocalTimeStart here is a great step, there are two other places in this file where untrusted payload fields can still cause crashes:
-
handleContextManagerMessage:: This is a public method that directly accessesmessage[kFIRMessagingContextManagerLocalTimeStart]and calls.lengthon it without type validation. If called directly with a non-string start time, it will crash. -
kFIRMessagingContextManagerLocalTimeEnd: InhandleContextManagerLocalTimeMessage:, the end time is read from the untrusted payload and passed directly to[dateFormatter dateFromString:]without type validation. IfendTimeStringis a number or other non-string type, it will crash.
Consider updating these methods as well to ensure complete safety against malformed payloads.
There was a problem hiding this comment.
Good call, both were still exposed. Pushed a follow-up: handleContextManagerMessage: now type-checks the start time before -length, and handleContextManagerLocalTimeMessage: guards both the start and end times before handing them to dateFromString:. Added a regression test that runs a non-string start and a non-string end through the public handler.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request hardens the FIRMessagingContextManagerService against untrusted push payloads by adding type checks for fields like start/end times, badge, body, title, sound, and category, preventing potential crashes when unexpected JSON types are received. Unit tests have also been added to verify these safeguards. The review feedback highlights a critical crash risk in handleContextManagerLocalTimeMessage where a nil startDate (resulting from an invalid date string) could be passed to compare:, as well as a logic bug where endTimeString is checked instead of endDate after parsing.
| @"Invalid local start date format %@. Message dropped", startTime); | ||
| return NO; | ||
| } | ||
| NSString *startTimeString = startTime; |
There was a problem hiding this comment.
The subsequent check if (!startTimeString) on line 112 is redundant because startTime is already guaranteed to be a non-nil NSString due to the isKindOfClass: check on line 106.\n\nMore importantly, there is a critical crash risk here: if startTimeString is an invalid date string (e.g., "invalid-date"), [dateFormatter dateFromString:startTimeString] on line 121 will return nil. Passing nil to [currentDate compare:] on line 124 will raise an NSInvalidArgumentException and crash the app because compare: does not accept nil.\n\nTo fix this, you should remove the redundant if (!startTimeString) check and instead add a check for if (!startDate) after it is parsed on line 121:\n\nobjc\n NSDate *startDate = [dateFormatter dateFromString:startTimeString];\n if (!startDate) {\n FIRMessagingLoggerError(kFIRMessagingMessageCodeContextManagerService002,\n @"Invalid local start date format %@. Message dropped",\n startTimeString);\n return NO;\n }\n
There was a problem hiding this comment.
Fixed. Dropped the redundant startTimeString nil check and now bail out when dateFromString: returns nil, so the nil startDate never reaches compare:.
| @"Invalid local end date format %@. Message dropped", endTime); | ||
| return NO; | ||
| } | ||
| NSString *endTimeString = endTime; |
There was a problem hiding this comment.
The check on line 143 is if (!endTimeString), but it should be if (!endDate).\n\nSince endTimeString is already guaranteed to be a non-nil NSString (due to the checks on lines 129 and 135), if (!endTimeString) will always evaluate to false. If dateFormatter fails to parse the string (returning nil for endDate), the code will proceed with a nil endDate.\n\nWhile sending a message to nil in Objective-C does not crash (it returns 0/NSOrderedSame), it will cause a logic bug on line 149 where [endDate compare:currentDate] evaluates to NSOrderedSame, preventing the message from being dropped when it has an invalid end date format.\n\nPlease update the check on line 143 to verify endDate instead:\n\nobjc\n NSDate *endDate = [dateFormatter dateFromString:endTimeString];\n if (!endDate) {\n FIRMessagingLoggerError(kFIRMessagingMessageCodeContextManagerService004,\n @"Invalid local end date format %@. Message dropped", endTimeString);\n return NO;\n }\n
There was a problem hiding this comment.
Good catch, that check was dead. Switched it to test endDate, so an unparseable end time is dropped instead of falling through to compare: as NSOrderedSame.
|
Pushed the nil-date handling for both start and end, and extended the handler test to cover string times that don't parse. |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request improves the robustness of FIRMessagingContextManagerService by adding type and nil checks on fields extracted from untrusted push payloads (such as start/end times, badge, body, title, sound, and category) to prevent crashes from unexpected types. Unit tests have been added to verify these safety checks. The review feedback suggests optimizing performance by caching the NSDateFormatter instance using dispatch_once instead of instantiating it on every method call.
| NSDateFormatter *dateFormatter = [[NSDateFormatter alloc] init]; | ||
| dateFormatter.locale = [NSLocale localeWithLocaleIdentifier:@"en_US_POSIX"]; | ||
| [dateFormatter setDateFormat:kLocalTimeFormatString]; |
There was a problem hiding this comment.
Creating an NSDateFormatter is an expensive operation. Since NSDateFormatter is thread-safe on iOS 7+ / macOS 10.9+, we can initialize it once using dispatch_once and reuse it to improve performance, especially since it is used multiple times within this method.
static NSDateFormatter *dateFormatter;
static dispatch_once_t onceToken;
dispatch_once(&onceToken, ^{
dateFormatter = [[NSDateFormatter alloc] init];
dateFormatter.locale = [NSLocale localeWithLocaleIdentifier:@"en_US_POSIX"];
[dateFormatter setDateFormat:kLocalTimeFormatString];
});There was a problem hiding this comment.
Done, moved the formatter to a dispatch_once static so it is built once and reused.
|
PTAL at the CI issues |
|
The CI failure was my test's fault, not the code. The non-string body/title fields get left unset, and the UNMutableNotificationContent default for those is nil on macOS/Catalyst here rather than an empty string, so the equal-to-@"" asserts failed. Switched those to check the field length is 0 instead, which holds either way. Pushed. |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces robust type-safety checks on untrusted push payload fields within FIRMessagingContextManagerService to prevent crashes caused by unexpected data types. It ensures that start/end times, badges, bodies, titles, sounds, and categories are verified before invoking type-specific selectors, and adds comprehensive unit tests to validate this behavior. The review feedback points out a redundant type check in handleContextManagerLocalTimeMessage: that can be safely removed because the caller already performs the necessary validation.
| id startTime = message[kFIRMessagingContextManagerLocalTimeStart]; | ||
| if (![startTime isKindOfClass:[NSString class]]) { | ||
| FIRMessagingLoggerError(kFIRMessagingMessageCodeContextManagerService002, | ||
| @"Invalid local start date format %@. Message dropped", | ||
| startTimeString); | ||
| @"Invalid local start date format %@. Message dropped", startTime); | ||
| return NO; | ||
| } | ||
| NSString *startTimeString = startTime; |
There was a problem hiding this comment.
Since handleContextManagerLocalTimeMessage: is a private helper method only called from handleContextManagerMessage:, and handleContextManagerMessage: already validates that startTime is a valid, non-empty NSString (via isContextManagerMessage:), this redundant type check can be safely removed.
NSString *startTimeString = message[kFIRMessagingContextManagerLocalTimeStart];There was a problem hiding this comment.
Good point, it is fully covered by the caller's check. Dropped it, so startTimeString is just read straight from the payload now.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request hardens the FIRMessagingContextManagerService against untrusted push payloads by adding type validation for start/end times, badges, and other notification fields, and optimizes NSDateFormatter creation using dispatch_once. Feedback is provided to improve the cached NSDateFormatter by setting its timezone to [NSTimeZone localTimeZone] to dynamically handle timezone changes, and to support string-based badge counts in addition to NSNumber values.
| static NSDateFormatter *dateFormatter; | ||
| static dispatch_once_t onceToken; | ||
| dispatch_once(&onceToken, ^{ | ||
| dateFormatter = [[NSDateFormatter alloc] init]; | ||
| dateFormatter.locale = [NSLocale localeWithLocaleIdentifier:@"en_US_POSIX"]; | ||
| [dateFormatter setDateFormat:kLocalTimeFormatString]; | ||
| }); |
There was a problem hiding this comment.
When caching an NSDateFormatter using dispatch_once, it defaults to the system's current timezone at the time of creation ([NSTimeZone defaultTimeZone]). If the user subsequently changes their device's timezone (e.g., due to travel), the cached formatter will continue to use the stale timezone.\n\nTo ensure the cached formatter always uses the active local timezone, explicitly set its timeZone property to [NSTimeZone localTimeZone], which dynamically tracks system timezone changes.
static NSDateFormatter *dateFormatter;
static dispatch_once_t onceToken;
dispatch_once(&onceToken, ^{
dateFormatter = [[NSDateFormatter alloc] init];
dateFormatter.locale = [NSLocale localeWithLocaleIdentifier:@"en_US_POSIX"];
dateFormatter.timeZone = [NSTimeZone localTimeZone];
[dateFormatter setDateFormat:kLocalTimeFormatString];
});| id badge = apsDictionary[kFIRMessagingContextManagerBadgeKey]; | ||
| if ([badge isKindOfClass:[NSNumber class]]) { | ||
| content.badge = badge; | ||
| } |
There was a problem hiding this comment.
Push notification payloads (especially those generated by various backend systems) sometimes represent the badge count as a string (e.g., "5") instead of a number.\n\nTo prevent valid badge values from being silently ignored, we can support both NSNumber and numeric NSString values by using NSScanner to safely parse string-based badges without crashing on non-numeric strings.
id badge = apsDictionary[kFIRMessagingContextManagerBadgeKey];
if ([badge isKindOfClass:[NSNumber class]]) {
content.badge = badge;
} else if ([badge isKindOfClass:[NSString class]]) {
NSInteger val;
if ([[NSScanner scannerWithString:badge] scanInteger:&val]) {
content.badge = @(val);
}
}This branch has not been deployed
2 participants
Repro: send a push whose
google.c.cm.lt_startis a number rather than a date string. On receiptisContextManagerMessage:calls-lengthon that value and the app hits-[__NSCFNumber length]: unrecognized selector, so a single message crashes the receiver. The same holds for the body, title, sound, category and badge fields incontentFromContextualMessage:.Cause: those fields are read straight from the untrusted push payload and assumed to be a specific type before a type-specific selector is sent to them.
Fix: check each field with
isKindOfClass:before use, the way the rest of the module already guards payload dictionaries. Valid payloads behave the same, and a regression test covers a non-string start time and non-string content fields.