From e0e4f860b6340364bc8ae3863e9fe22b078390bb Mon Sep 17 00:00:00 2001 From: Nan Date: Mon, 27 Jul 2026 18:06:49 -0700 Subject: [PATCH 1/3] fix: [SDK-4818] resolve TOCTOU race on the current user in executor callbacks User executor network callbacks asked `isCurrentUser(...)` and then separately dereferenced `_user`. A concurrent login/logout landing between those two reads made the first answer stale, so a response could hydrate one user's properties and subscriptions onto another, clear a just-switched user's data, or log out a user the failed request had nothing to do with. Guard `_user` with a lock and add `currentUser(matching:)`, which reads it once and returns the instance only if it still owns the request's identity model. Callers use that returned instance instead of re-reading `_user`, so there is no window between the check and the mutation. The lock is not held across caller work, which fires model-store listeners into the operation repo and could deadlock; handing back the checked instance is what makes this safe. `clearUserData` now takes the user to clear for the same reason. Co-Authored-By: Cursor --- .../OSIdentityOperationExecutor.swift | 4 +- .../OSPropertyOperationExecutor.swift | 8 +-- .../OSSubscriptionOperationExecutor.swift | 4 +- .../Source/Executors/OSUserExecutor.swift | 30 ++++----- .../Source/OneSignalUserManagerImpl.swift | 36 +++++++++-- .../OneSignalUserTests.swift | 61 +++++++++++++++++++ 6 files changed, 113 insertions(+), 30 deletions(-) diff --git a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSIdentityOperationExecutor.swift b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSIdentityOperationExecutor.swift index 1a3c3e839..516ca0c02 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSIdentityOperationExecutor.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSIdentityOperationExecutor.swift @@ -223,8 +223,8 @@ 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 this request's user is still current, so a concurrent login can't log out the wrong 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 107254fd2..88f29af73 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSPropertyOperationExecutor.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSPropertyOperationExecutor.swift @@ -257,11 +257,11 @@ class OSPropertyOperationExecutor: OSOperationExecutor { // Re-assert the tags the server just confirmed by merging them back into the local model, // to remedy a concurrent FetchUser whose response is missing the just-written tags - if OneSignalUserManagerImpl.sharedInstance.isCurrentUser(request.identityModel), + if let user = OneSignalUserManagerImpl.sharedInstance.currentUser(matching: request.identityModel.modelId), let properties = response?["properties"] as? [String: Any], let confirmedTags = properties["tags"] as? [String: String], !confirmedTags.isEmpty { - OneSignalUserManagerImpl.sharedInstance._user?.propertiesModel.mergeConfirmedTags(confirmedTags) + user.propertiesModel.mergeConfirmedTags(confirmedTags) } if let onesignalId = request.identityModel.onesignalId { @@ -287,8 +287,8 @@ 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 this request's user is still current, so a concurrent login can't log out the wrong 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 3f823038c..f100bbfd9 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSSubscriptionOperationExecutor.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSSubscriptionOperationExecutor.swift @@ -327,8 +327,8 @@ 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 this request's user is still current, so a concurrent login can't log out the wrong 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..0795452c6 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSUserExecutor.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSUserExecutor.swift @@ -397,8 +397,7 @@ extension OSUserExecutor { self.removeFromQueue(request) - if let userInstance = OneSignalUserManagerImpl.sharedInstance._user, - OneSignalUserManagerImpl.sharedInstance.isCurrentUser(request.identityModelToUpdate) { + if let userInstance = OneSignalUserManagerImpl.sharedInstance.currentUser(matching: request.identityModelToUpdate.modelId) { // Generate a Create User request, if it's still the current user self.createUser(userInstance) } else { @@ -412,8 +411,8 @@ 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 this request's user is still current, so a concurrent login can't log out the wrong user. + guard OneSignalUserManagerImpl.sharedInstance.currentUser(matching: request.identityModelToUpdate.modelId) != nil else { return } @@ -448,15 +447,12 @@ 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. A login can land while this + // response is in flight, so the clear must apply to the user the response is for. + if let user = OneSignalUserManagerImpl.sharedInstance.currentUser(matching: request.identityModel.modelId), + let response = response { // 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 +481,8 @@ 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 this request's user is still current, so a concurrent login can't log out the wrong user. + guard OneSignalUserManagerImpl.sharedInstance.currentUser(matching: request.identityModel.modelId) != nil else { return } @@ -541,14 +537,14 @@ extension OSUserExecutor { } } - // Check if the current user is the same as the one in the request + // Hydrate onto the user this response is for // If user has changed, don't hydrate, except for push subscription above - guard OneSignalUserManagerImpl.sharedInstance.isCurrentUser(identityModel) else { + guard let user = OneSignalUserManagerImpl.sharedInstance.currentUser(matching: identityModel.modelId) else { return } if let propertiesObject = parsePropertiesObjectResponse(response) { - OneSignalUserManagerImpl.sharedInstance._user?.propertiesModel.hydrate(propertiesObject) + user.propertiesModel.hydrate(propertiesObject) } // Now parse email and sms subscriptions diff --git a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OneSignalUserManagerImpl.swift b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OneSignalUserManagerImpl.swift index 5b9e0ce35..87e00a3af 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OneSignalUserManagerImpl.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OneSignalUserManagerImpl.swift @@ -149,7 +149,16 @@ public class OneSignalUserManagerImpl: NSObject, OneSignalUserManager { return createNewUser(externalId: nil, token: nil) } - var _user: OSUserInternal? + /// Guards `_user`. Held only across a single read or write; holding it while callers mutate + /// models would re-enter the model stores and operation repo, and could deadlock. + private let userLock = NSLock() + + private var _userStorage: OSUserInternal? + + var _user: OSUserInternal? { + get { userLock.withLock { _userStorage } } + set { userLock.withLock { _userStorage = newValue } } + } // This is a user instance to operate on when there is no app_id and/or privacy consent yet, effectively no-op. // The models are not added to any model stores. @@ -427,13 +436,30 @@ public class OneSignalUserManagerImpl: NSObject, OneSignalUserManager { return userInstance.identityModel.externalId == externalId } + + /** + Returns the current user only if it is still the user that `modelId` identifies. + + Use this, not `isCurrentUser`, when the caller will then act on that user, and act on the + instance returned. It is read once here, so a concurrent `login()`/`logout()` cannot land + between the check and the use and apply one user's data to another. + */ + func currentUser(matching modelId: String) -> OSUserInternal? { + guard let user = _user, user.identityModel.modelId == modelId else { + return nil + } + return user + } + /** - Clears the existing user's data in preparation for hydration via a fetch user call. + Clears the passed-in user's data in preparation for hydration via a fetch user call. + + Operates on the given user so a concurrent login can't redirect the clear onto a different one. */ - func clearUserData() { + 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() diff --git a/iOS_SDK/OneSignalSDK/OneSignalUserTests/OneSignalUserTests.swift b/iOS_SDK/OneSignalSDK/OneSignalUserTests/OneSignalUserTests.swift index 9c55ec928..bfe416b46 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUserTests/OneSignalUserTests.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUserTests/OneSignalUserTests.swift @@ -248,4 +248,65 @@ final class OneSignalUserTests: XCTestCase { // The confirmed tags from the 202 response are merged back into the local model XCTAssertEqual(OneSignalUserManagerImpl.sharedInstance.getTags(), tags) } + + // MARK: - Atomic current user access + + /** + A callback that acts on the current user must keep acting on the user it checked, even if a + `login()` makes a different user current right afterwards. Swapping the user between the check + and the mutation reproduces that interleaving. + */ + func testCurrentUser_matching_isTheUserMutated_whenTheUserChangesRightAfterTheCheck() throws { + /* Setup */ + OneSignalCoreImpl.setSharedClient(MockOneSignalClient()) + let manager = OneSignalUserManagerImpl.sharedInstance + let userA = OneSignalUserMocks.setUserManagerInternalUser(externalId: userA_EUID, onesignalId: userA_OSID) + + /* When */ + let checkedUser = manager.currentUser(matching: userA.identityModel.modelId) + // A concurrent login switches the current user before the response is applied + let userB = OneSignalUserMocks.setUserManagerInternalUser(externalId: userB_EUID, onesignalId: userB_OSID) + checkedUser?.propertiesModel.hydrate(["language": "language-for-user-a"]) + + /* Then */ + // The response's data went to the user it was for, and the new current user is untouched + XCTAssertEqual(userA.propertiesModel.language, "language-for-user-a") + XCTAssertNil(userB.propertiesModel.language) + XCTAssertEqual(manager._user?.identityModel.externalId, userB_EUID) + } + + /// The common path: the request's user is still current, so it is returned to be mutated. + func testCurrentUser_matching_returnsTheCurrentUser() throws { + /* Setup */ + OneSignalCoreImpl.setSharedClient(MockOneSignalClient()) + let manager = OneSignalUserManagerImpl.sharedInstance + let user = OneSignalUserMocks.setUserManagerInternalUser(externalId: userA_EUID, onesignalId: userA_OSID) + + /* Then */ + XCTAssertEqual(manager.currentUser(matching: user.identityModel.modelId)?.identityModel.externalId, userA_EUID) + } + + /// A response for a user that is no longer current must not be applied at all. + func testCurrentUser_matching_isNilWhenTheUserIsNoLongerCurrent() throws { + /* Setup */ + OneSignalCoreImpl.setSharedClient(MockOneSignalClient()) + let manager = OneSignalUserManagerImpl.sharedInstance + let userA = OneSignalUserMocks.setUserManagerInternalUser(externalId: userA_EUID, onesignalId: userA_OSID) + let userB = OneSignalUserMocks.setUserManagerInternalUser(externalId: userB_EUID, onesignalId: userB_OSID) + + /* Then */ + XCTAssertNil(manager.currentUser(matching: userA.identityModel.modelId)) + XCTAssertNotNil(manager.currentUser(matching: userB.identityModel.modelId)) + } + + /// With no current user, there is nothing for a late response to act on. + func testCurrentUser_matching_isNilWhenThereIsNoUser() throws { + /* Setup */ + let manager = OneSignalUserManagerImpl.sharedInstance + let identityModel = OSIdentityModel(aliases: nil, changeNotifier: OSEventProducer()) + + /* Then */ + XCTAssertNil(manager._user) + XCTAssertNil(manager.currentUser(matching: identityModel.modelId)) + } } From 19d01a1926c2cb8f5884310789f380d3b2c9b8be Mon Sep 17 00:00:00 2001 From: Nan Date: Mon, 27 Jul 2026 18:40:15 -0700 Subject: [PATCH 2/3] refactor: [SDK-4818] remove isCurrentUser in favor of the atomic accessor isCurrentUser answered "is this the current user?" from the identity model store while currentUser(matching:) answers it from the _user instance. The two disagree while a login or logout is in flight, which is the same split source of truth behind the TOCTOU race, so collapse every caller onto the accessor. Co-authored-by: Cursor --- .../Source/Executors/OSUserExecutor.swift | 8 +++---- .../Source/OneSignalUserManagerImpl.swift | 21 ++----------------- .../Executors/UserExecutorTests.swift | 4 ++-- 3 files changed, 8 insertions(+), 25 deletions(-) diff --git a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSUserExecutor.swift b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSUserExecutor.swift index 0795452c6..1b8d0e5b3 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSUserExecutor.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSUserExecutor.swift @@ -120,7 +120,7 @@ class OSUserExecutor { // Translate the last request into a Create User request, if the current user is the same if let request = transferSubscriptionRequestQueue.last, let userInstance = OneSignalUserManagerImpl.sharedInstance._user, - OneSignalUserManagerImpl.sharedInstance.isCurrentUser(request.aliasId) { + userInstance.identityModel.externalId == request.aliasId { createUser(userInstance) } } @@ -251,7 +251,7 @@ extension OSUserExecutor { // If this user already exists and we logged into an external_id, fetch the user data // Fetch the user only if its the current user and non-anonymous - if OneSignalUserManagerImpl.sharedInstance.isCurrentUser(request.identityModel), + if OneSignalUserManagerImpl.sharedInstance.currentUser(matching: request.identityModel.modelId) != nil, let identity = request.parameters?["identity"] as? [String: String], let onesignalId = request.identityModel.onesignalId, identity[OS_EXTERNAL_ID] != nil { @@ -320,7 +320,7 @@ extension OSUserExecutor { request.identityModel.hydrate(identityObject) // Fetch this user's data if it is the current user - guard OneSignalUserManagerImpl.sharedInstance.isCurrentUser(request.identityModel) + guard OneSignalUserManagerImpl.sharedInstance.currentUser(matching: request.identityModel.modelId) != nil else { self.executePendingRequests() return @@ -382,7 +382,7 @@ extension OSUserExecutor { request.identityModelToUpdate.hydrate(aliases) // the anonymous user has been identified, still need to Fetch User as we cleared local data - if OneSignalUserManagerImpl.sharedInstance.isCurrentUser(request.identityModelToUpdate) { + if OneSignalUserManagerImpl.sharedInstance.currentUser(matching: request.identityModelToUpdate.modelId) != nil { // Add onesignal ID to new records because an immediate fetch may not return the newly-applied external ID self.newRecordsState.add(onesignalId, true) self.fetchUser(aliasLabel: OS_ONESIGNAL_ID, aliasId: onesignalId, identityModel: request.identityModelToUpdate) diff --git a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OneSignalUserManagerImpl.swift b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OneSignalUserManagerImpl.swift index 87e00a3af..6057226f9 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OneSignalUserManagerImpl.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OneSignalUserManagerImpl.swift @@ -421,28 +421,11 @@ public class OneSignalUserManagerImpl: NSObject, OneSignalUserManager { ) } - /** - Returns if the OSIdentityModel passed in belongs to the current user. This method is used in deciding whether or not to hydrate via a server response, for example. - */ - func isCurrentUser(_ identityModel: OSIdentityModel) -> Bool { - return self.identityModelStore.getModel(modelId: identityModel.modelId) != nil - } - - func isCurrentUser(_ externalId: String) -> Bool { - guard let userInstance = _user, !externalId.isEmpty else { - OneSignalLog.onesignalLog(.LL_ERROR, message: "isCurrentUser called with empty externalId or no user instance") - return false - } - - return userInstance.identityModel.externalId == externalId - } - /** Returns the current user only if it is still the user that `modelId` identifies. - Use this, not `isCurrentUser`, when the caller will then act on that user, and act on the - instance returned. It is read once here, so a concurrent `login()`/`logout()` cannot land - between the check and the use and apply one user's data to another. + Act on the instance returned. It is read once here, so a concurrent `login()`/`logout()` cannot + land between the check and the use and apply one user's data to another. */ func currentUser(matching modelId: String) -> OSUserInternal? { guard let user = _user, user.identityModel.modelId == modelId else { diff --git a/iOS_SDK/OneSignalSDK/OneSignalUserTests/Executors/UserExecutorTests.swift b/iOS_SDK/OneSignalSDK/OneSignalUserTests/Executors/UserExecutorTests.swift index f1eb5bbb3..703079453 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUserTests/Executors/UserExecutorTests.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUserTests/Executors/UserExecutorTests.swift @@ -222,8 +222,8 @@ final class UserExecutorTests: XCTestCase { } /** - The normal new-session Fetch User for the *current* user must still clear stale local data before hydrating - from the response, so the `isCurrentUser` guard added for the race above does not regress the common path. + A Fetch User for the *current* user must still clear stale local data before hydrating from the + response, so guarding against the race above does not regress the common path. */ func testFetchUser_forCurrentUser_stillClearsStaleData() { /* Setup */ From d210076261bb93bd14cf8d3344f3d13179f4ed14 Mon Sep 17 00:00:00 2001 From: Nan Date: Thu, 30 Jul 2026 23:47:39 -0700 Subject: [PATCH 3/3] docs: [SDK-4818] scope currentUser(matching:) comment to what the accessor actually guarantees The docstring overclaimed that concurrent login/logout cannot apply one user's data to another; that holds for the returned instance's models, not the shared stores. Co-authored-by: Cursor --- .../OneSignalUser/Source/OneSignalUserManagerImpl.swift | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OneSignalUserManagerImpl.swift b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OneSignalUserManagerImpl.swift index 6057226f9..eb7608b2d 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OneSignalUserManagerImpl.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OneSignalUserManagerImpl.swift @@ -422,10 +422,9 @@ public class OneSignalUserManagerImpl: NSObject, OneSignalUserManager { } /** - Returns the current user only if it is still the user that `modelId` identifies. - - Act on the instance returned. It is read once here, so a concurrent `login()`/`logout()` cannot - land between the check and the use and apply one user's data to another. + Act on the instance returned: the current user is read once here, so a concurrent + `login()`/`logout()` can't land between the check and the use. Its identity and properties + models are safe to mutate; the shared model stores are not scoped to a user. */ func currentUser(matching modelId: String) -> OSUserInternal? { guard let user = _user, user.identityModel.modelId == modelId else {