Conversation
… tokens - Transitions `NSKeyedArchiver` and `NSKeyedUnarchiver` to use `requiresSecureCoding = YES`. - Preserves fallback `requiresSecureCoding = NO` blocks for backward compatibility with 12.18.0 and older legacy data. - Ensures cross-version compatibility by maintaining `FIRInstanceID` legacy class aliases during secure decoding.
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 updates FirebaseMessaging to enforce NSSecureCoding when archiving and unarchiving registration tokens, ensuring better security, and adds extensive tests to verify backward compatibility. The review feedback points out critical issues where unarchiving failures could be handled silently or lead to incorrect migration states (potentially causing data loss), and suggests minor improvements like using more efficient NSSet initialization and explicitly initializing NSError variables to nil.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates FirebaseMessaging to enforce NSSecureCoding when archiving and unarchiving registration tokens from the keychain, replacing deprecated and insecure APIs. It also adds robust unit tests to ensure backward compatibility and smooth upgrade/downgrade transitions. The review feedback correctly identifies several instances of an Objective-C anti-pattern where success or failure is determined by checking the error pointer rather than the returned object (such as unarchiver or tokenInfoData). Adhering to Cocoa conventions by checking the returned values instead of the error pointer will prevent potential unexpected behavior.
…er success checks
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request enforces NSSecureCoding when archiving and unarchiving registration tokens from the keychain in FirebaseMessaging, updating both FIRMessagingTokenInfo and FIRMessagingTokenStore to use secure coding APIs. It also introduces comprehensive unit tests to verify legacy compatibility, upgrade/downgrade scenarios, and corrupted payload handling. The review feedback recommends adding error logging when decodeObjectOfClass:forKey: fails to decode token info or nested APNS info, which will help diagnose secure coding or decoding failures in production.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates FirebaseMessaging to enforce NSSecureCoding when archiving and unarchiving registration tokens and APNS info to and from the keychain. It replaces deprecated, insecure archiving/unarchiving methods with secure alternatives, adds robust error handling and logging during decoding, and introduces comprehensive unit tests and mocks to ensure backwards compatibility and smooth upgrade/downgrade migration paths. There are no review comments to address, so I have no feedback to provide.
|
Needs extra data compatibility testing and possible impl. cleanup since launching it in major version. |
…se 13 Remove the legacy pre-10.19.0 fallback decoding APNSInfo from raw NSData blobs in FIRMessagingTokenInfo as part of Firebase 13 major release cleanup.
The previous revision dropped the fallback that reads `apns_info` archived by FirebaseMessaging 10.18.0 and earlier, on the premise that the nested blob could not be read without `requiresSecureCoding = NO`. It can. FIRMessagingAPNSInfo conforms to NSSecureCoding, so the nested archive decodes under the strict decoder once `FIRInstanceIDAPNSInfo` is mapped to its current class, exactly as FIRMessagingTokenStore already maps `FIRInstanceIDTokenInfo` on the enclosing archive. Reading the blob rather than discarding it removes the only remaining insecure unarchive without changing behavior. Apps upgrading from 10.18.0 and earlier keep their cached APNS association, so their token is no longer invalidated and refetched on the next APNS registration, and `needsMigration` keeps the meaning it has on main. Declaring NSData and NSMutableData alongside FIRMessagingAPNSInfo is now load-bearing: the blob materializes as NSMutableData, and it has to be decoded as data before it can be unarchived. Also drop the @try/@catch, which `initForReadingFromData:error:` makes vestigial, and stop leaking process-global archiver class mappings out of the tests that install them.
The secure archiving added to FIRMessagingTokenStore introduced an early return whose handler ran synchronously on the calling thread, while the keychain write it replaces delivers its handler on the main queue via FIRMessagingKeychain. A caller therefore saw a different calling context depending on which step failed. Dispatch the failure to the main queue so there is one contract. These log sites also borrowed kFIRMessagingMessageCodeTokenManager001 from a different file; give FIRMessagingTokenStore its own code instead.
ncooke3
changed the title
ncooke3
changed the title
The PR enforced NSSecureCoding on both keychain unarchivers, but nothing in the suite failed if either was flipped back to requiresSecureCoding = NO. The security fix was untested. Add FIRMessagingArchiveGadget, a class that conforms to NSCoding but not NSSecureCoding and records whether the runtime ever ran its initWithCoder:, and plant it at the root of a keychain item and inside a legacy nested apns_info blob. Both paths must reject it without constructing it. Verified by mutation: reverting either requiresSecureCoding assignment fails exactly these two tests and nothing else. Move the legacy encoders and decoders into FIRMessagingLegacyArchiveFixtures, verified line-by-line against the 10.18.0 and 12.19.0 tags. The inline 12.x mock had silently dropped 12.19's nested-blob fallback branch while its doc comment claimed exact parity; the omission happened not to matter for the one test using it, but it would have made any 10.18 -> 12.x test quietly wrong. Close three coverage gaps, weighted toward 12.x as the realistic base version: - Extend the downgrade test into the full six-leg cycle, so the record a downgraded SDK writes on its way back out is proven readable when the user rolls forward again. - Add the untested matrix cell where Firebase 13 migrates a <= 10.18 record and a 12.x SDK then reads the migrated bytes. - Assert the V4 token_type default, which is the only thing standing between a 10.18 record and a nil token type. Make testLegacyTokenStoreReadsSecureToken run 12.19's decoder rather than mapping the legacy class name onto the current one, which had meant Firebase 13's own initWithCoder: was under test.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request enforces NSSecureCoding when archiving and unarchiving registration tokens in the keychain to address potential deserialization vulnerabilities. It updates FIRMessagingTokenStore and FIRMessagingTokenInfo to require secure coding during serialization and deserialization, while maintaining backward compatibility with legacy token formats (including pre-10.19.0 nested archives). Comprehensive unit tests and compatibility fixtures are also introduced to verify upgrade/downgrade cycles and ensure that hostile payloads are rejected. The review feedback suggests renaming several local variables (such as APNSInfoClasses, APNSInfoError, and APNSInfoUnarchiver) to adhere to standard camelCase naming conventions starting with a lowercase letter, and optimizing set initialization.
The nested unarchiver guards against a blob that is NSData but not a valid archive, but nothing exercised that branch. Whether it holds matters: if a malformed blob raised instead of returning nil, the exception would unwind to the @catch in tokenInfoFromKeychainItem: and discard the whole record, costing the user their token over a damaged APNS association. Losing the APNSInfo is recoverable on the next APNS registration; losing the token is not. Test three flavours of corruption, chosen because they fail at different depths of NSKeyedUnarchiver and the shallow ones are the ones that can raise: bytes that are not a property list, a valid property list that is not a keyed archive, and a truncated real archive. All three return cleanly, so the record survives with only its APNSInfo missing and no production change is needed. Generalize the poisoned-archive fixture to carry any substitute apns_info payload so the hostile-gadget and corrupt-bytes cases share one builder. This touches only the synthetic attack fixtures added in the previous commit; the encoders and decoders reconstructed from the 10.18.0 and 12.19.0 tags are byte-identical, verified by extracting and diffing those regions rather than by inspection.
2 participants
Summary
Registration tokens read from the
com.google.iid-tokenskeychain servicewere unarchived with
requiresSecureCoding = NOin two places, allowing aplanted archive to instantiate an arbitrary
-initWithCoder:-implementingclass. This enables both layers to decode under secure coding.
See b/511903712
Fix
Both unarchivers now set
requiresSecureCoding = YESand decode withdecodeObjectOfClass:, constraining the root object toFIRMessagingTokenInfoandFIRMessagingAPNSInforespectively. The legacyFIRInstanceIDTokenInfo/FIRInstanceIDAPNSInfoclass-name remaps areretained — they work under secure coding, and because a remap can only
narrow an encoded name onto a named class, it is not an allowlist bypass.
Both use the instance-scoped
setClass:forClassName:, so no process-globalstate is mutated.
NSData/NSMutableDataappear in theapns_infoallowlist because thelegacy blob must be decoded as data before it can be unarchived; it
materializes as
NSMutableData, which secure coding will not accept via itssuperclass.
The 10.18 fallback is secured, not removed
An earlier revision deleted the legacy nested decode. That was unnecessary:
FIRMessagingAPNSInfoalready conforms toNSSecureCoding, so the blobreads fine under a strict decoder. Deleting it would have dropped cached
APNS metadata for anyone upgrading from ≤10.18, invalidating and refetching
their token — a behavior change unrelated to the vulnerability, and one that
would also be visible to a downgraded reader once migration rewrote the
record. Retiring the fallback remains tracked by
TODO(#12246)as astandalone deprecation.
Data compatibility
No persisted string changed: keychain service, all eight coding keys, and
both legacy class names are byte-identical to
main. Keychain queryattributes are untouched.
requiringSecureCoding:YESdoes not alter theserialized shape for conforming classes, so 13.0 writes structurally what
10.19–12.x wrote.
¹
apns_infohas been written as a direct object since 10.19 (#12110), sodowngrade below 10.19 predates and is unaffected by this change.
Testing
Full
MessagingUnitsuite green: 181 tests, 1 skipped, 0 failures, and noFoundation decoding warnings.
New coverage:
testTokenInfoFrom10_18ArchiveIsDecodedSecurely— a genuine ≤10.18archive, built by a mock of that version's encoder, with a fixture
self-check that fails loudly if the mock drifts from the real format
testReadingLegacyRecordMigratesItToTheModernFormat— end-to-end throughthe store, asserting the rewrite is lossless for APNS info
testModernArchiveRecordsAPNSInfoUnderItsCurrentClassName— documents whythe outer decoder needs no remap