AB#292768 fix(push): chain and harden the UNUserNotificationCenter delegate install - #148
Open
eligutovsky wants to merge 1 commit into
Open
AB#292768 fix(push): chain and harden the UNUserNotificationCenter delegate install#148eligutovsky wants to merge 1 commit into
eligutovsky wants to merge 1 commit into
Conversation
…tall AB#292768 The delegate we install does forward foreign notifications to the one the host app had set, but that rule lived inline in two protocol methods that no test can reach: UNUserNotificationCenter.current() raises bundleProxyForCurrentProcess in a test bundle, and neither UNNotification nor UNNotificationResponse can be constructed. Extract the routing rules into NotificationDelegateRouting, as functions of the payload alone, and cover them. Also, in the same install path: - Never chain to another delegate of ours. Nesting would handle each notification once per layer and forward it to the host app as many times over. The delegate the install reads is now injected, which is what makes the class constructible in a test at all. - Drop UIApplication.shared.delegate! and object_getClass(delegate)!. Initializing the SDK before the application delegate is set crashed; it now logs and skips. - Run the install on the main thread. UIApplication.shared is main-thread only and the swizzles write file-scope IMP globals. - Guard against installing twice, so class_replaceMethod cannot hand back our own IMP and make the swizzled method call itself. - Warn when the delegate is replaced after the SDK was initialized, which takes push handling away from the SDK with no trace today.
There was a problem hiding this comment.
Pull request overview
This PR hardens the SDK’s push-notification integration by making UNUserNotificationCenter delegate installation safer (no self-wrapping, no force-unwrap crashes, main-thread confinement, and a diagnostic warning if replaced later), while extracting the core “is this notification ours?” routing logic into a testable unit with dedicated coverage.
Changes:
- Extract notification routing rules into
NotificationDelegateRoutingand add unit tests to lock down forwarding/handling behavior. - Refactor
OptimoveUserNotificationCenterDelegateto inject and de-nest the prior delegate and centralize routing decisions. - Harden push/swizzle installation: main-thread confinement, install-once guard, nil-safe delegate/class discovery, and warn if the notification center delegate is replaced after initialization; bump version to 6.8.3 and update changelog.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| OptimoveSDK/Tests/Sources/Optimobile/NotificationDelegateRoutingTests.swift | Adds focused tests that pin down routing/forwarding and self-wrapping prevention. |
| OptimoveSDK/Sources/Classes/Optimobile/OptimoveUserNotificationCenterDelegate.swift | Refactors delegate chaining + routing and removes untestable UNUserNotificationCenter.current() dependency from init. |
| OptimoveSDK/Sources/Classes/Optimobile/Optimobile+Push.swift | Makes push install main-thread-only, crash-safe, install-once, and adds “delegate replaced” warning. |
| OptimoveSDK/Sources/Classes/Optimobile/NotificationDelegateRouting.swift | Introduces testable routing logic based on payload message id and action identifier. |
| OptimoveSDK.podspec | Bumps SDK pod version to 6.8.3. |
| OptimoveNotificationServiceExtension.podspec | Bumps NSE pod version to 6.8.3. |
| OptimoveCore/Sources/Classes/Constants/SDKVersion.swift | Updates SDKVersion constant to 6.8.3. |
| OptimoveCore.podspec | Bumps core pod version to 6.8.3. |
| CHANGELOG.md | Documents the delegate-install hardening and swizzling crash fix in 6.8.3. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
16 tasks
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#292768
Description of Changes
The premise of AB#292768 is partly wrong, and I want to state that before the fix.
The bug says the SDK "silently drops" the host app's
UNUserNotificationCenterdelegate.It does not.
OptimoveUserNotificationCenterDelegatehas captured the previous delegate andforwarded to it since 2022, and every notification that is not ours reaches it:
setPushReceivedInForegroundHandler/setPushOpenedHandlerare the sanctioned hooks)So the headline defect from the handoff is not real. What is real, and what this PR fixes:
1. The forwarding rule was untestable, and therefore unprotected. It lived inline in two
protocol methods that a test cannot reach.
UNUserNotificationCenter.current()raises in atest bundle — measured, not assumed:
and neither
UNNotificationnorUNNotificationResponsehas an initializer. So the one rulethat keeps the host app working had zero coverage.
NotificationDelegateRoutingnow holds itas functions of the payload alone, and 11 tests pin it down — including that registering a
foreground handler must not start swallowing other people's notifications.
The extraction is behaviour-preserving. The old code decided by a mix of
push.id == 0,userInfo["aps"] == nil, and theBoolreturned bypushHandleOpen/pushHandleDismissed.All three reduce to the same predicate — an Optimove message id is present — because both
pushHandle*methods returnid != 0, and a payload with noapscannot parse an id. Theif handledfallback to chaining is kept as a safety net rather than replaced by the router'sdecision.
2. Re-installing over our own delegate nested the chain. Each layer would handle the same
notification and forward it to the host app once per layer. Not reachable through
Optimove.initializetoday, which is guarded byinstance == nil, but the constructor nowrefuses to wrap itself so it cannot become reachable.
3.
UIApplication.shared.delegate!crashed the app when the SDK was initialized beforethe application delegate was set. Now logged and skipped. Same for
object_getClass(delegate)!and the duplicate force-unwrap that shadowed the
klassparameter insideswizzleDidFailRegister.4. The install ran on whichever thread initialized the SDK.
UIApplication.sharedismain-thread only, and the swizzles write file-scope IMP globals. It is now confined to the
main thread, which also removes any race on those globals.
5. Installing twice was unguarded.
class_replaceMethodwould return our own IMP, and theswizzled method would then call itself until the stack ran out. Unreachable today for the same
reason as (2) — I checked, rather than repeating the handoff's claim that it was live — but the
invariant is now explicit instead of accidental.
6. The genuinely silent failure, now diagnosable. Chaining only covers a delegate installed
before the SDK. One installed after replaces ours outright, and the SDK stops seeing
notifications entirely — no push opens, no in-app tickles, no log line. The install now checks
once the current run loop turn has drained and warns, which catches the common case of the
assignment happening later in
application(_:didFinishLaunchingWithOptions:). It does notcatch an assignment in a later turn (a
viewDidLoad, say).Verification
Full suite: 89 tests, 0 failures (75 before, plus the 14 added here).
Mutation-checked — the tests are not vacuous. Two mutations,
isOursforced totrueand theself-wrapping guard disabled, kill 7 of the 14:
Deliberately not changed
existingDelegatestays a strong reference, althoughUNUserNotificationCenter.delegateis weak and holding it strongly can keep a delegate alive that the host app has released.
Making it
weakis the more correct ownership, but it can only reduce what the host appreceives, which is the opposite of this bug, and there is no way to test the lifetime change
here. Worth doing on its own.
Optimove notifications still do not reach the host app's delegate. Forwarding them too
would double-handle in every app that already uses
setPushOpenedHandler, so it is a productdecision, not a bug fix.
PushNotification.initkeeps its force casts (msg["data"] as!,msgData["id"] as!,Optimobile+Push.swift:47-49). A payload carryingcustom.a.k.messagewithout a well-formeddata.idcrashes, and this init now runs on every notification the app receives, ours or not.Out of scope here, but it should be picked up.
Breaking Changes
No public API changes. Behaviour changes are confined to cases that previously crashed or
silently did nothing.
Release Checklist
Prepare:
pod lib lintpassesintegration guide is worth a note that the notification-center delegate must be set
before
Optimove.initialize, which is what the new warning tells people at runtimeBump versions in:
Bumped to 6.8.3. 6.8.1 and 6.8.2 are claimed by #144 and #147, both still open, so this
takes the next free patch number. Whichever of the three merges first should keep its number
and the others renumbered — none of them collide in
CHANGELOG.mdas they stand.OptimoveCore.podspecOptimoveNotificationServiceExtension.podspecOptimoveSDK.podspecOptimoveCore/Sources/Classes/Constants/SDKVersion.swiftREADME.md— n/a, contains no version referenceCHANGELOG.mdIntegration tests
Unit tests pass (89, 0 failures). The routing rules are covered, but the parts that only exist
at runtime need a device:
UNUserNotificationCenter.current().delegatebeforeOptimove.initialize,then: an Optimove push opens and is tracked; the host app's own push still reaches their
delegate; a local notification still reaches their delegate.
Optimove.initialize— expect the new warning in the log.T&T Only
Mobile Only
Release: