guard non-string payload fields in FIRMessagingContextManagerService - #16520
guard non-string payload fields in FIRMessagingContextManagerService#16520isl-Ramzi wants to merge 4 commits into
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];
});
|
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. |
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.