AB#292839 fix(push): stop force-casting the notification message payload - #149
Open
eligutovsky wants to merge 1 commit into
Open
AB#292839 fix(push): stop force-casting the notification message payload#149eligutovsky wants to merge 1 commit into
eligutovsky wants to merge 1 commit into
Conversation
AB#292839 PushNotification.init(userInfo:) force-cast msg["data"] and its "id". A payload carrying custom.a.k.message without a well-formed data.id took the host app down, and this initialiser runs on every notification the app receives, ours or not, so a malformed payload from any sender was enough. Both casts are now checked. Leaving id at 0 marks the notification as not ours, which is already how the delegate decides to forward it to the host app, so an unreadable payload is treated like any other notification that is not ours. Both branches log, because a malformed id used to produce a crash report and would otherwise now fail silently. Verified by running the new tests against master first: four distinct crashes at the two lines, one per malformed shape.
There was a problem hiding this comment.
Pull request overview
This PR hardens PushNotification.init(userInfo:) so malformed notification payloads can no longer crash the host app during push parsing, and adds unit tests to lock in the new non-crashing behavior. It also bumps the SDK patch version and documents the fix in the changelog.
Changes:
- Replace two unsafe force-casts in push payload parsing with guarded casts plus warning logs, treating unreadable payloads as “not ours” (
id == 0). - Add a dedicated unit test suite covering well-formed, foreign, and previously-crashing malformed payload shapes.
- Bump SDK version to 6.8.4 across podspecs /
SDKVersion.swiftand add aCHANGELOGentry.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| OptimoveSDK/Sources/Classes/Optimobile/Optimobile+Push.swift | Removes force-casts in PushNotification.init(userInfo:), adds guarded parsing with warnings and preserves id == 0 behavior for non-Optimove/malformed payloads. |
| OptimoveSDK/Tests/Sources/Optimobile/PushNotificationTests.swift | Adds comprehensive tests for valid/invalid payload shapes, including the previously crashing cases. |
| OptimoveSDK.podspec | Bumps pod version to 6.8.4. |
| OptimoveNotificationServiceExtension.podspec | Bumps pod version to 6.8.4. |
| OptimoveCore.podspec | Bumps pod version to 6.8.4. |
| OptimoveCore/Sources/Classes/Constants/SDKVersion.swift | Updates SDKVersion constant to 6.8.4. |
| CHANGELOG.md | Adds 6.8.4 entry describing the crash fix and new behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
AB#292839
Description of Changes
PushNotification.init(userInfo:)force-cast two values it had not checked:A payload carrying
custom.a.k.messagewithout a well-formeddata.idtook the host app down.This initialiser runs on every notification the app receives, ours or not — both
UNUserNotificationCenterdelegate methods build aPushNotificationbefore deciding whosenotification it is — so a malformed payload from any sender was enough.
Both casts are now checked. Leaving
idat 0 marks the notification as not ours, which isalready how the delegate decides to forward it to the host app's delegate, so a payload we
cannot read is treated the same as any other notification that is not ours.
Verification
The crash reproduces. Running the new tests against
mastergives four distinct crashes,one per malformed shape, at exactly the two lines above:
Each one kills the test process, which is what the crash does to a host app. With the fix, the
same 11 tests pass and nothing crashes.
Full suite: 86 tests, 0 failures (75 before, plus the 11 added here).
The 11 tests cover the well-formed payload (message id, deep link URL,
aps/datapassthrough),the payloads that were already handled (no payload, no
aps, nocustom, nok.message), andthe four that crashed (no
data,datanot a dictionary, noid,idnot a number).One consequence worth naming
A malformed id used to crash loudly. It now logs a warning and the notification is treated as
not ours. If the push service ever changed the id's type, the old behaviour would have produced
a crash report, whereas this will silently stop attributing opens. That is why both branches log
rather than return quietly — the log line is what makes it diagnosable:
Accepting a string id as well would paper over exactly that kind of change, so the check stays
strict.
Breaking Changes
No public API change. The only behaviour change is in cases that previously crashed.
Release Checklist
Prepare:
pod lib lintpassesBump versions in:
Bumped to 6.8.4, the next number not already claimed by an open PR (6.8.1 → #144,
6.8.2 → #147, 6.8.3 → #148). This is not sustainable: with several PRs open at once, a
per-PR bump means whichever merges first is right and the rest have to be renumbered. Worth
deciding to bump once at release time instead, and dropping the version rows from the per-PR
checklist.
OptimoveCore.podspecOptimoveNotificationServiceExtension.podspecOptimoveSDK.podspecOptimoveCore/Sources/Classes/Constants/SDKVersion.swiftREADME.md— n/a, contains no version referenceCHANGELOG.mdIntegration tests
Unit tests pass (86, 0 failures). Parsing is fully covered by them; what needs a device is only
that a normal Optimove push is unaffected:
Release: