Conversation
Validate types of values from campaign fetch responses, the on-disk campaign cache, persisted impressions and Clearcut responses instead of storing wrongly typed values that crash later outside the parser's @Try, e.g. a non-string analytics event name crashing in -isEqualToString: when the event fires, or nil being inserted into an array when reading impressions. Refs #16728
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 significantly improves the robustness of Firebase In-App Messaging by adding strict type checking and validation when parsing API fetch responses, cached campaigns, and persisted impressions. It introduces helper functions and type checks across multiple classes to prevent crashes from malformed or unexpected JSON structures, accompanied by comprehensive unit tests. The review feedback suggests further enhancing safety by introducing a FIRIAMDictionaryOrNil helper function to safely parse nested dictionary structures and avoid unrecognized selector exceptions.
The parser returns nil for malformed top-level responses, which the static analyzer flagged as nullability.NullReturnedFromNonnull. Declare the return as nullable and treat a nil result from the cached response as an empty message set.
Address review feedback: add FIRIAMDictionaryOrNil and use it for every nested content lookup (title, body, action, buttons) across banner, modal, image-only, and card messages. A wrong-typed intermediate node now drops only the fields beneath it instead of raising an unrecognized selector exception that discarded the whole message.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request improves the robustness of Firebase In-App Messaging when parsing malformed campaign fetch responses, cached campaigns, and persisted impressions. It introduces defensive type-checking helpers and validation checks across several components—including the response parser, HTTP request sender, bookkeeper, and client cache—to prevent crashes from unexpected JSON types. Additionally, comprehensive unit tests have been added to verify these robustness improvements. There are no review comments to address, and I have no feedback to provide.
This branch has not been deployed
2 participants
Context: #16728 — a malformed server payload was parsed into a nil dictionary key
and crashed apps on every launch. This PR audits FirebaseInAppMessaging's
handling of server-delivered campaign data (fetch responses, the on-disk
campaign cache re-read at launch, persisted impressions, and Clearcut
responses) for the same class of bug.
Most per-message parsing in
FIRIAMFetchResponseParseris already wrapped in@try/@catch, but that only covers exceptions thrown during parsing. Valuesof the wrong type were stored as-is and crashed later, outside the
@try.Hardened code paths
FIRIAMFetchResponseParsermessagesnode that is not an array: these hit-objectForKeyedSubscript:/ fast enumeration outside the
@tryand threw. The parser now returns nil,and callers already treat that as a failed fetch or an empty cache.
{"name": 7}ornullwas stored. It was then added to the watch set andlater sent
-isEqualToString:from-[FIRIAMMessageDefinition messageRenderedOnAnalyticsEvent:], which crasheswhen a matching Analytics event fires. Because the response is cached on
disk, the bad trigger came back on every launch.
campaignId(now must be a non-empty string),campaignName, title, body,button text, image and action URL strings, and
fiamTriggerare nowtype-checked. Wrong-typed optional fields are dropped instead of being
stored as
NSNumber/NSArrayinNSStringproperties that UI andbookkeeping code use later.
experimentPayloadis only passed toABTExperimentPayloadwhen it is a dictionary. Trigger entries that are notdictionaries are skipped.
UIColor+FIRIAMHexString: a non-string hex color returns nil, so thedefault color is used.
FIRIAMMsgFetcherUsingRestful: a 200 response with a nil body no longerpasses nil to
NSJSONSerialization, which throwsNSInvalidArgumentException.FIRIAMMessageClientCache: a nil analytics event name is never insertedinto
firebaseAnalyticEventsToWatch.FIRIAMBookKeeperViaUserDefaults: malformed persisted impression entriesare now skipped. Before this change,
getImpressionsadded nil to an array(
initWithStorageDictionary:returns nil for bad entries), and bothgetImpressionsandgetMessageIDsFromImpressionssubscripted non-dictionaryentries.
getImpressionsruns on every fetch check, so corrupt entriescrashed the app repeatedly.
FIRIAMClearcutHttpRequestSender: now handles a nil body, a JSON body thatis not a dictionary, and a
next_request_wait_millisthat is not anumber/string (e.g.
null,[],{}). Each of these used to throw.Wait-time range clamping already exists in
FIRIAMClearcutUploader.Valid payloads parse the same as before. The existing fixtures and assertions
are unchanged.
Tests
New fixture:
JsonDataWithMalformedMessagesFromFetch.txt. New/extended testsin
FIRIAMFetchResponseParserTests,FIRIAMMessageClientCacheTests,FIRIAMBookKeeperViaUserDefaultsTests,FIRIAMMsgFetcherUsingRestfulTests, anda new
FIRIAMClearcutHttpRequestSenderTests.These 12 tests fail on
main(with the source changes reverted):testParsingMalformedMessageNodes: wrong-typed ids, names, titles, and eventnames were accepted
testParsingNonDictionaryResponse,testParsingNonArrayMessagesNode:unrecognized selector exceptions
testLoadingCachedResponseWithMalformedEventNames:-[NSConstantIntegerNumber isEqualToString:]frommessageRenderedOnAnalyticsEvent:testLoadingCorruptCachedResponsetestReadingMalformedImpressionRecords,testRecordingImpressionWithMalformedImpressionRecordstestFetchWithNilResponseBody,testFetchWithNonDictionaryJSONResponseBody,testFetchWithNonArrayMessagesInResponseBodytestMalformedResponseBodies,testNilResponseBody(Clearcut)A few more tests (empty response, invalid expiration times, garbage JSON body,
non-array impressions, valid Clearcut wait times) already pass on
mainandare added as regression coverage.
Testing
pod gen FirebaseInAppMessaging.podspec --platforms=ios, then ranFirebaseInAppMessaging-Unit-uniton an iOS 26 simulator. The full unit suitepasses: 138 tests, 0 failures (121 before this change plus 17 new).