From 49a295f962d2e0c2fc677bef247fd74c60e5d8ed Mon Sep 17 00:00:00 2001 From: Nan Date: Thu, 25 Jun 2026 17:12:14 -0700 Subject: [PATCH 1/2] fix: capture the current user atomically in user executor callbacks Network-response callbacks across the user executors checked the current user via isCurrentUser(...) and then separately dereferenced _user (or called _logout()). A concurrent login/logout could swap _user in that window, so the callback applied one user's data or action to a different user (cross-user contamination or an erroneous logout / data loss). Add withCurrentUser(matching:) and currentUser(matching:) to OneSignalUserManagerImpl, which read _user once and confirm it still owns the request's identity model before operating on that single captured instance. Route through them: - parseFetchUserResponse properties + email/SMS hydration (the shared fetch and create-user contamination point) - the clearUserData + re-hydrate in the FetchUser success path - the _logout() failure paths in FetchUser, IdentifyUser, and the property, subscription, and identity update executors No lock is held across the closure: the model mutations there fire model-store listeners into the operation repo, so holding one could deadlock. Co-authored-by: Cursor --- .../OSIdentityOperationExecutor.swift | 5 +- .../OSPropertyOperationExecutor.swift | 5 +- .../OSSubscriptionOperationExecutor.swift | 5 +- .../Source/Executors/OSUserExecutor.swift | 91 ++++++++++--------- .../Source/OneSignalUserManagerImpl.swift | 47 +++++++++- 5 files changed, 101 insertions(+), 52 deletions(-) diff --git a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSIdentityOperationExecutor.swift b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSIdentityOperationExecutor.swift index 1a3c3e839..f0667428a 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSIdentityOperationExecutor.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSIdentityOperationExecutor.swift @@ -223,8 +223,9 @@ class OSIdentityOperationExecutor: OSOperationExecutor { // Remove from cache and queue self.addRequestQueue.removeAll(where: { $0 == request}) OneSignalUserDefaults.initShared().saveCodeableData(forKey: OS_IDENTITY_EXECUTOR_ADD_REQUEST_QUEUE_KEY, withValue: self.addRequestQueue) - // Logout if the user in the SDK is the same - guard OneSignalUserManagerImpl.sharedInstance.isCurrentUser(request.identityModel) + // Logout only if the request still owns the current user (captured once), so a + // concurrent login/logout can't escalate this into an erroneous logout of another user + guard OneSignalUserManagerImpl.sharedInstance.currentUser(matching: request.identityModel.modelId) != nil else { if inBackground { OSBackgroundTaskManager.endBackgroundTask(backgroundTaskIdentifier) diff --git a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSPropertyOperationExecutor.swift b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSPropertyOperationExecutor.swift index e4220e0be..62f401cab 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSPropertyOperationExecutor.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSPropertyOperationExecutor.swift @@ -278,8 +278,9 @@ class OSPropertyOperationExecutor: OSOperationExecutor { // remove from cache and queue self.updateRequestQueue.removeAll(where: { $0 == request}) OneSignalUserDefaults.initShared().saveCodeableData(forKey: OS_PROPERTIES_EXECUTOR_UPDATE_REQUEST_QUEUE_KEY, withValue: self.updateRequestQueue) - // Logout if the user in the SDK is the same - guard OneSignalUserManagerImpl.sharedInstance.isCurrentUser(request.identityModel) + // Logout only if the request still owns the current user (captured once), so a + // concurrent login/logout can't escalate this into an erroneous logout of another user + guard OneSignalUserManagerImpl.sharedInstance.currentUser(matching: request.identityModel.modelId) != nil else { if inBackground { OSBackgroundTaskManager.endBackgroundTask(backgroundTaskIdentifier) diff --git a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSSubscriptionOperationExecutor.swift b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSSubscriptionOperationExecutor.swift index 18e66de80..a06e0d0f7 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSSubscriptionOperationExecutor.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSSubscriptionOperationExecutor.swift @@ -325,8 +325,9 @@ class OSSubscriptionOperationExecutor: OSOperationExecutor { if responseType == .missing { self.addRequestQueue.removeAll(where: { $0 == request}) OneSignalUserDefaults.initShared().saveCodeableData(forKey: OS_SUBSCRIPTION_EXECUTOR_ADD_REQUEST_QUEUE_KEY, withValue: self.addRequestQueue) - // Logout if the user in the SDK is the same - guard OneSignalUserManagerImpl.sharedInstance.isCurrentUser(request.identityModel) + // Logout only if the request still owns the current user (captured once), so a + // concurrent login/logout can't escalate this into an erroneous logout of another user + guard OneSignalUserManagerImpl.sharedInstance.currentUser(matching: request.identityModel.modelId) != nil else { if inBackground { OSBackgroundTaskManager.endBackgroundTask(backgroundTaskIdentifier) diff --git a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSUserExecutor.swift b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSUserExecutor.swift index 4fb6094a4..cd43feef6 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSUserExecutor.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSUserExecutor.swift @@ -412,8 +412,9 @@ extension OSUserExecutor { } else if responseType == .missing { self.removeFromQueue(request) self.executePendingRequests() - // Logout if the user in the SDK is the same - guard OneSignalUserManagerImpl.sharedInstance.isCurrentUser(request.identityModelToUpdate) + // Logout only if the request still owns the current user (captured once), so a + // concurrent login/logout can't escalate this into an erroneous logout of another user + guard OneSignalUserManagerImpl.sharedInstance.currentUser(matching: request.identityModelToUpdate.modelId) != nil else { return } @@ -448,15 +449,15 @@ extension OSUserExecutor { OneSignalCoreImpl.sharedClient().execute(request) { response in self.removeFromQueue(request) - // A fetch for a user that is no longer current is stale - guard OneSignalUserManagerImpl.sharedInstance.isCurrentUser(request.identityModel) else { - self.executePendingRequests() - return - } - - if let response = response { + // A fetch for a user that is no longer current is stale. Capture `_user` once and + // clear/hydrate that same instance, so a concurrent login/logout can't wipe or hydrate + // a different user's data. + OneSignalUserManagerImpl.sharedInstance.withCurrentUser(matching: request.identityModel.modelId) { user in + guard let response = response else { + return + } // Clear local data in preparation for hydration - OneSignalUserManagerImpl.sharedInstance.clearUserData() + OneSignalUserManagerImpl.sharedInstance.clearUserData(user) self.parseFetchUserResponse(response: response, identityModel: request.identityModel, originalPushToken: OneSignalUserManagerImpl.sharedInstance.pushSubscriptionImpl.token) // If this is a on-new-session's fetch user call, check that the subscription still exists @@ -485,8 +486,9 @@ extension OSUserExecutor { let responseType = OSNetworkingUtils.getResponseStatusType(error.code) if responseType == .missing { self.removeFromQueue(request) - // Logout if the user in the SDK is the same - guard OneSignalUserManagerImpl.sharedInstance.isCurrentUser(request.identityModel) + // Logout only if the request still owns the current user (captured once), so a + // concurrent login/logout can't escalate this into an erroneous logout of another user + guard OneSignalUserManagerImpl.sharedInstance.currentUser(matching: request.identityModel.modelId) != nil else { return } @@ -541,39 +543,40 @@ extension OSUserExecutor { } } - // Check if the current user is the same as the one in the request - // If user has changed, don't hydrate, except for push subscription above - guard OneSignalUserManagerImpl.sharedInstance.isCurrentUser(identityModel) else { - return - } - - if let propertiesObject = parsePropertiesObjectResponse(response) { - OneSignalUserManagerImpl.sharedInstance._user?.propertiesModel.hydrate(propertiesObject) - } - - // Now parse email and sms subscriptions - if let subscriptionObject = parseSubscriptionObjectResponse(response) { - let models = OneSignalUserManagerImpl.sharedInstance.subscriptionModelStore.getModels() - for subModel in subscriptionObject { - if let address = subModel["token"] as? String, - let rawType = subModel["type"] as? String, - rawType != "iOSPush", - let type = OSSubscriptionType(rawValue: rawType) - { - if let model = models[address] { - // This subscription exists in the store, hydrate - model.hydrate(subModel) + // Hydrate the properties and email/SMS subscriptions only if the request still owns the + // current user; if the user has changed, don't hydrate (except for the push subscription + // above). Capture `_user` once and hydrate that same instance, so a concurrent login/logout + // can't land this response's data on a different user. This is reached from both the fetch + // user and create user paths, so the guard must live here. + OneSignalUserManagerImpl.sharedInstance.withCurrentUser(matching: identityModel.modelId) { user in + if let propertiesObject = parsePropertiesObjectResponse(response) { + user.propertiesModel.hydrate(propertiesObject) + } - } else { - // This subscription does not exist in the store, add - OneSignalUserManagerImpl.sharedInstance.subscriptionModelStore.add(id: address, model: OSSubscriptionModel( - type: type, - address: address, - subscriptionId: subModel["id"] as? String, - reachable: true, - isDisabled: false, - changeNotifier: OSEventProducer()), hydrating: true - ) + // Now parse email and sms subscriptions + if let subscriptionObject = parseSubscriptionObjectResponse(response) { + let models = OneSignalUserManagerImpl.sharedInstance.subscriptionModelStore.getModels() + for subModel in subscriptionObject { + if let address = subModel["token"] as? String, + let rawType = subModel["type"] as? String, + rawType != "iOSPush", + let type = OSSubscriptionType(rawValue: rawType) + { + if let model = models[address] { + // This subscription exists in the store, hydrate + model.hydrate(subModel) + + } else { + // This subscription does not exist in the store, add + OneSignalUserManagerImpl.sharedInstance.subscriptionModelStore.add(id: address, model: OSSubscriptionModel( + type: type, + address: address, + subscriptionId: subModel["id"] as? String, + reachable: true, + isDisabled: false, + changeNotifier: OSEventProducer()), hydrating: true + ) + } } } } diff --git a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OneSignalUserManagerImpl.swift b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OneSignalUserManagerImpl.swift index 5b9e0ce35..0237b4a7c 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OneSignalUserManagerImpl.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OneSignalUserManagerImpl.swift @@ -427,13 +427,56 @@ public class OneSignalUserManagerImpl: NSObject, OneSignalUserManager { return userInstance.identityModel.externalId == externalId } + + /** + Returns the current user only if it still owns `modelId`, otherwise `nil`. + + `_user` is read exactly once, so callers operate on a single confirmed instance instead of + checking the current user and then separately re-reading `_user`. That check-then-deref is a + TOCTOU: a concurrent login/logout can swap `_user` in the window between the two, landing one + user's data or action on a different user. + */ + func currentUser(matching modelId: String) -> OSUserInternal? { + guard let user = _user, user.identityModel.modelId == modelId else { + return nil + } + return user + } + + /** + Runs `body` with the current user, but only if it still owns `modelId`, capturing `_user` once. + + The same captured instance is both checked and passed to `body`, so a concurrent login/logout + can't divert a network-response callback's mutation onto a different user. No lock is held + across `body`: the model mutations callers perform here (e.g. `hydrate`, `clearData`) fire + model-store listeners into the operation repo, so holding a lock across them could deadlock. + */ + func withCurrentUser(matching modelId: String, _ body: (OSUserInternal) -> Void) { + guard let user = currentUser(matching: modelId) else { + return + } + body(user) + } + /** Clears the existing user's data in preparation for hydration via a fetch user call. */ func clearUserData() { + guard let user = _user else { + return + } + clearUserData(user) + } + + /** + Clears the passed-in (already-confirmed) user's data in preparation for hydration via a fetch + user call. Operating on the captured `user` rather than re-reading `_user` avoids clearing a + different user's data if a concurrent login/logout swaps `_user`. + */ + func clearUserData(_ user: OSUserInternal) { // Identity and property models should still be the same instances, but with data cleared - _user?.identityModel.clearData() - _user?.propertiesModel.clearData() + user.identityModel.clearData() + user.propertiesModel.clearData() // Subscription model store should be cleared completely OneSignalUserManagerImpl.sharedInstance.subscriptionModelStore.clearModelsFromStore() From 407fac14590459506910cc744b5acbd18ee0e024 Mon Sep 17 00:00:00 2001 From: Nan Date: Thu, 25 Jun 2026 17:12:20 -0700 Subject: [PATCH 2/2] test: cover the atomic current-user accessor Add deterministic unit tests for withCurrentUser/currentUser: the closure runs on the captured current user when the model ID matches, and is skipped (the accessor returns nil) when there is no current user or a concurrent swap has changed the current user's model ID. Co-authored-by: Cursor --- .../OneSignalUserTests.swift | 94 +++++++++++++++++++ 1 file changed, 94 insertions(+) diff --git a/iOS_SDK/OneSignalSDK/OneSignalUserTests/OneSignalUserTests.swift b/iOS_SDK/OneSignalSDK/OneSignalUserTests/OneSignalUserTests.swift index 5b4a9d2c7..58bde22b6 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUserTests/OneSignalUserTests.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUserTests/OneSignalUserTests.swift @@ -189,4 +189,98 @@ final class OneSignalUserTests: XCTestCase { contains: expectedPayload) ) } + + // MARK: - Atomic current-user accessor (TOCTOU) + + /** + The user executors guard a network-response callback with the current user and then separately + dereference `_user`. A concurrent login/logout can swap `_user` between the two, applying one + user's data or action to a different user. `withCurrentUser(matching:)` closes that window by + reading `_user` once and passing that same captured instance to the closure. + + This is the safe path: when the current user still owns the model ID, the closure runs on the + captured current user. + */ + func testWithCurrentUser_runsClosureOnCapturedCurrentUser_whenModelIdMatches() throws { + /* Setup */ + let user = OneSignalUserMocks.setUserManagerInternalUser(externalId: userA_EUID, onesignalId: userA_OSID) + let modelId = user.identityModel.modelId + + /* When */ + var runCount = 0 + var capturedIsCurrentUser = false + OneSignalUserManagerImpl.sharedInstance.withCurrentUser(matching: modelId) { capturedUser in + runCount += 1 + // The closure must receive the very instance that is currently `_user`. + capturedIsCurrentUser = capturedUser.identityModel === OneSignalUserManagerImpl.sharedInstance._user?.identityModel + } + + /* Then */ + XCTAssertEqual(runCount, 1) + XCTAssertTrue(capturedIsCurrentUser) + } + + /** + Simulates the TOCTOU race: a concurrent login swaps `_user` to a different user before the + callback runs. The accessor must NOT run its closure for the now-stale model ID, so the response + can't be applied to the new current user (cross-user contamination). + */ + func testWithCurrentUser_doesNotRunClosure_whenUserSwappedToDifferentModelId() throws { + /* Setup */ + let previousUser = OneSignalUserMocks.setUserManagerInternalUser(externalId: userA_EUID, onesignalId: userA_OSID) + let staleModelId = previousUser.identityModel.modelId + + // A concurrent login swaps the current user out from under the in-flight request. + let currentUser = OneSignalUserMocks.setUserManagerInternalUser(externalId: userB_EUID, onesignalId: userB_OSID) + XCTAssertNotEqual(staleModelId, currentUser.identityModel.modelId) + + /* When */ + var runCount = 0 + OneSignalUserManagerImpl.sharedInstance.withCurrentUser(matching: staleModelId) { _ in + runCount += 1 + } + + /* Then */ + XCTAssertEqual(runCount, 0) + } + + /** + When there is no current user (e.g. `_user == nil` during the logout window), the accessor must + not run its closure rather than dereferencing a nil or recreated user. + */ + func testWithCurrentUser_doesNotRunClosure_whenNoCurrentUser() throws { + /* Setup */ + // `OneSignalUserMocks.reset()` (called in setUp) leaves `_user == nil`. + XCTAssertNil(OneSignalUserManagerImpl.sharedInstance._user) + + /* When */ + var runCount = 0 + OneSignalUserManagerImpl.sharedInstance.withCurrentUser(matching: UUID().uuidString) { _ in + runCount += 1 + } + + /* Then */ + XCTAssertEqual(runCount, 0) + } + + /** + `currentUser(matching:)` is the non-closure variant used to gate `_logout()`. It must return the + current user only when the model ID matches, and `nil` when there is no user or the user was + swapped, so a stale failure callback can't log out a different user. + */ + func testCurrentUserMatching_returnsUserOnlyWhenModelIdMatches() throws { + /* No current user → nil */ + XCTAssertNil(OneSignalUserManagerImpl.sharedInstance.currentUser(matching: UUID().uuidString)) + + /* Matching current user → the captured instance */ + let user = OneSignalUserMocks.setUserManagerInternalUser(externalId: userA_EUID, onesignalId: userA_OSID) + let staleModelId = user.identityModel.modelId + let matched = OneSignalUserManagerImpl.sharedInstance.currentUser(matching: staleModelId) + XCTAssertNotNil(matched) + XCTAssertTrue(matched?.identityModel === user.identityModel) + + /* Concurrent swap → the previous model ID no longer matches → nil */ + _ = OneSignalUserMocks.setUserManagerInternalUser(externalId: userB_EUID, onesignalId: userB_OSID) + XCTAssertNil(OneSignalUserManagerImpl.sharedInstance.currentUser(matching: staleModelId)) + } }