Skip to content

Commit 6c6749f

Browse files
nan-licursoragent
nan-li
and
cursoragent
authored
fix: [SDK-4818] resolve TOCTOU race on the current user (#1695)
Co-authored-by: Cursor <cursoragent@cursor.com>
1 parent 6a37a52 commit 6c6749f

7 files changed

Lines changed: 112 additions & 47 deletions

File tree

‎iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSIdentityOperationExecutor.swift‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -223,8 +223,8 @@ class OSIdentityOperationExecutor: OSOperationExecutor {
223223
// Remove from cache and queue
224224
self.addRequestQueue.removeAll(where: { $0 == request})
225225
OneSignalUserDefaults.initShared().saveCodeableData(forKey: OS_IDENTITY_EXECUTOR_ADD_REQUEST_QUEUE_KEY, withValue: self.addRequestQueue)
226-
// Logout if the user in the SDK is the same
227-
guard OneSignalUserManagerImpl.sharedInstance.isCurrentUser(request.identityModel)
226+
// Logout only if this request's user is still current, so a concurrent login can't log out the wrong user.
227+
guard OneSignalUserManagerImpl.sharedInstance.currentUser(matching: request.identityModel.modelId) != nil
228228
else {
229229
if inBackground {
230230
OSBackgroundTaskManager.endBackgroundTask(backgroundTaskIdentifier)

‎iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSPropertyOperationExecutor.swift‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -257,11 +257,11 @@ class OSPropertyOperationExecutor: OSOperationExecutor {
257257

258258
// Re-assert the tags the server just confirmed by merging them back into the local model,
259259
// to remedy a concurrent FetchUser whose response is missing the just-written tags
260-
if OneSignalUserManagerImpl.sharedInstance.isCurrentUser(request.identityModel),
260+
if let user = OneSignalUserManagerImpl.sharedInstance.currentUser(matching: request.identityModel.modelId),
261261
let properties = response?["properties"] as? [String: Any],
262262
let confirmedTags = properties["tags"] as? [String: String],
263263
!confirmedTags.isEmpty {
264-
OneSignalUserManagerImpl.sharedInstance._user?.propertiesModel.mergeConfirmedTags(confirmedTags)
264+
user.propertiesModel.mergeConfirmedTags(confirmedTags)
265265
}
266266

267267
if let onesignalId = request.identityModel.onesignalId {
@@ -287,8 +287,8 @@ class OSPropertyOperationExecutor: OSOperationExecutor {
287287
// remove from cache and queue
288288
self.updateRequestQueue.removeAll(where: { $0 == request})
289289
OneSignalUserDefaults.initShared().saveCodeableData(forKey: OS_PROPERTIES_EXECUTOR_UPDATE_REQUEST_QUEUE_KEY, withValue: self.updateRequestQueue)
290-
// Logout if the user in the SDK is the same
291-
guard OneSignalUserManagerImpl.sharedInstance.isCurrentUser(request.identityModel)
290+
// Logout only if this request's user is still current, so a concurrent login can't log out the wrong user.
291+
guard OneSignalUserManagerImpl.sharedInstance.currentUser(matching: request.identityModel.modelId) != nil
292292
else {
293293
if inBackground {
294294
OSBackgroundTaskManager.endBackgroundTask(backgroundTaskIdentifier)

‎iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSSubscriptionOperationExecutor.swift‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -327,8 +327,8 @@ class OSSubscriptionOperationExecutor: OSOperationExecutor {
327327
if responseType == .missing {
328328
self.addRequestQueue.removeAll(where: { $0 == request})
329329
OneSignalUserDefaults.initShared().saveCodeableData(forKey: OS_SUBSCRIPTION_EXECUTOR_ADD_REQUEST_QUEUE_KEY, withValue: self.addRequestQueue)
330-
// Logout if the user in the SDK is the same
331-
guard OneSignalUserManagerImpl.sharedInstance.isCurrentUser(request.identityModel)
330+
// Logout only if this request's user is still current, so a concurrent login can't log out the wrong user.
331+
guard OneSignalUserManagerImpl.sharedInstance.currentUser(matching: request.identityModel.modelId) != nil
332332
else {
333333
if inBackground {
334334
OSBackgroundTaskManager.endBackgroundTask(backgroundTaskIdentifier)

‎iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSUserExecutor.swift‎

Lines changed: 17 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -120,7 +120,7 @@ class OSUserExecutor {
120120
// Translate the last request into a Create User request, if the current user is the same
121121
if let request = transferSubscriptionRequestQueue.last,
122122
let userInstance = OneSignalUserManagerImpl.sharedInstance._user,
123-
OneSignalUserManagerImpl.sharedInstance.isCurrentUser(request.aliasId) {
123+
userInstance.identityModel.externalId == request.aliasId {
124124
createUser(userInstance)
125125
}
126126
}
@@ -251,7 +251,7 @@ extension OSUserExecutor {
251251

252252
// If this user already exists and we logged into an external_id, fetch the user data
253253
// Fetch the user only if its the current user and non-anonymous
254-
if OneSignalUserManagerImpl.sharedInstance.isCurrentUser(request.identityModel),
254+
if OneSignalUserManagerImpl.sharedInstance.currentUser(matching: request.identityModel.modelId) != nil,
255255
let identity = request.parameters?["identity"] as? [String: String],
256256
let onesignalId = request.identityModel.onesignalId,
257257
identity[OS_EXTERNAL_ID] != nil {
@@ -320,7 +320,7 @@ extension OSUserExecutor {
320320
request.identityModel.hydrate(identityObject)
321321

322322
// Fetch this user's data if it is the current user
323-
guard OneSignalUserManagerImpl.sharedInstance.isCurrentUser(request.identityModel)
323+
guard OneSignalUserManagerImpl.sharedInstance.currentUser(matching: request.identityModel.modelId) != nil
324324
else {
325325
self.executePendingRequests()
326326
return
@@ -382,7 +382,7 @@ extension OSUserExecutor {
382382
request.identityModelToUpdate.hydrate(aliases)
383383

384384
// the anonymous user has been identified, still need to Fetch User as we cleared local data
385-
if OneSignalUserManagerImpl.sharedInstance.isCurrentUser(request.identityModelToUpdate) {
385+
if OneSignalUserManagerImpl.sharedInstance.currentUser(matching: request.identityModelToUpdate.modelId) != nil {
386386
// Add onesignal ID to new records because an immediate fetch may not return the newly-applied external ID
387387
self.newRecordsState.add(onesignalId, true)
388388
self.fetchUser(aliasLabel: OS_ONESIGNAL_ID, aliasId: onesignalId, identityModel: request.identityModelToUpdate)
@@ -397,8 +397,7 @@ extension OSUserExecutor {
397397

398398
self.removeFromQueue(request)
399399

400-
if let userInstance = OneSignalUserManagerImpl.sharedInstance._user,
401-
OneSignalUserManagerImpl.sharedInstance.isCurrentUser(request.identityModelToUpdate) {
400+
if let userInstance = OneSignalUserManagerImpl.sharedInstance.currentUser(matching: request.identityModelToUpdate.modelId) {
402401
// Generate a Create User request, if it's still the current user
403402
self.createUser(userInstance)
404403
} else {
@@ -412,8 +411,8 @@ extension OSUserExecutor {
412411
} else if responseType == .missing {
413412
self.removeFromQueue(request)
414413
self.executePendingRequests()
415-
// Logout if the user in the SDK is the same
416-
guard OneSignalUserManagerImpl.sharedInstance.isCurrentUser(request.identityModelToUpdate)
414+
// Logout only if this request's user is still current, so a concurrent login can't log out the wrong user.
415+
guard OneSignalUserManagerImpl.sharedInstance.currentUser(matching: request.identityModelToUpdate.modelId) != nil
417416
else {
418417
return
419418
}
@@ -448,15 +447,12 @@ extension OSUserExecutor {
448447
OneSignalCoreImpl.sharedClient().execute(request) { response in
449448
self.removeFromQueue(request)
450449

451-
// A fetch for a user that is no longer current is stale
452-
guard OneSignalUserManagerImpl.sharedInstance.isCurrentUser(request.identityModel) else {
453-
self.executePendingRequests()
454-
return
455-
}
456-
457-
if let response = response {
450+
// A fetch for a user that is no longer current is stale. A login can land while this
451+
// response is in flight, so the clear must apply to the user the response is for.
452+
if let user = OneSignalUserManagerImpl.sharedInstance.currentUser(matching: request.identityModel.modelId),
453+
let response = response {
458454
// Clear local data in preparation for hydration
459-
OneSignalUserManagerImpl.sharedInstance.clearUserData()
455+
OneSignalUserManagerImpl.sharedInstance.clearUserData(user)
460456
self.parseFetchUserResponse(response: response, identityModel: request.identityModel, originalPushToken: OneSignalUserManagerImpl.sharedInstance.pushSubscriptionImpl.token)
461457

462458
// If this is a on-new-session's fetch user call, check that the subscription still exists
@@ -485,8 +481,8 @@ extension OSUserExecutor {
485481
let responseType = OSNetworkingUtils.getResponseStatusType(error.code)
486482
if responseType == .missing {
487483
self.removeFromQueue(request)
488-
// Logout if the user in the SDK is the same
489-
guard OneSignalUserManagerImpl.sharedInstance.isCurrentUser(request.identityModel)
484+
// Logout only if this request's user is still current, so a concurrent login can't log out the wrong user.
485+
guard OneSignalUserManagerImpl.sharedInstance.currentUser(matching: request.identityModel.modelId) != nil
490486
else {
491487
return
492488
}
@@ -541,14 +537,14 @@ extension OSUserExecutor {
541537
}
542538
}
543539

544-
// Check if the current user is the same as the one in the request
540+
// Hydrate onto the user this response is for
545541
// If user has changed, don't hydrate, except for push subscription above
546-
guard OneSignalUserManagerImpl.sharedInstance.isCurrentUser(identityModel) else {
542+
guard let user = OneSignalUserManagerImpl.sharedInstance.currentUser(matching: identityModel.modelId) else {
547543
return
548544
}
549545

550546
if let propertiesObject = parsePropertiesObjectResponse(response) {
551-
OneSignalUserManagerImpl.sharedInstance._user?.propertiesModel.hydrate(propertiesObject)
547+
user.propertiesModel.hydrate(propertiesObject)
552548
}
553549

554550
// Now parse email and sms subscriptions

‎iOS_SDK/OneSignalSDK/OneSignalUser/Source/OneSignalUserManagerImpl.swift‎

Lines changed: 24 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -149,7 +149,16 @@ public class OneSignalUserManagerImpl: NSObject, OneSignalUserManager {
149149
return createNewUser(externalId: nil, token: nil)
150150
}
151151

152-
var _user: OSUserInternal?
152+
/// Guards `_user`. Held only across a single read or write; holding it while callers mutate
153+
/// models would re-enter the model stores and operation repo, and could deadlock.
154+
private let userLock = NSLock()
155+
156+
private var _userStorage: OSUserInternal?
157+
158+
var _user: OSUserInternal? {
159+
get { userLock.withLock { _userStorage } }
160+
set { userLock.withLock { _userStorage = newValue } }
161+
}
153162

154163
// This is a user instance to operate on when there is no app_id and/or privacy consent yet, effectively no-op.
155164
// The models are not added to any model stores.
@@ -413,27 +422,26 @@ public class OneSignalUserManagerImpl: NSObject, OneSignalUserManager {
413422
}
414423

415424
/**
416-
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.
425+
Act on the instance returned: the current user is read once here, so a concurrent
426+
`login()`/`logout()` can't land between the check and the use. Its identity and properties
427+
models are safe to mutate; the shared model stores are not scoped to a user.
417428
*/
418-
func isCurrentUser(_ identityModel: OSIdentityModel) -> Bool {
419-
return self.identityModelStore.getModel(modelId: identityModel.modelId) != nil
420-
}
421-
422-
func isCurrentUser(_ externalId: String) -> Bool {
423-
guard let userInstance = _user, !externalId.isEmpty else {
424-
OneSignalLog.onesignalLog(.LL_ERROR, message: "isCurrentUser called with empty externalId or no user instance")
425-
return false
429+
func currentUser(matching modelId: String) -> OSUserInternal? {
430+
guard let user = _user, user.identityModel.modelId == modelId else {
431+
return nil
426432
}
427-
428-
return userInstance.identityModel.externalId == externalId
433+
return user
429434
}
435+
430436
/**
431-
Clears the existing user's data in preparation for hydration via a fetch user call.
437+
Clears the passed-in user's data in preparation for hydration via a fetch user call.
438+
439+
Operates on the given user so a concurrent login can't redirect the clear onto a different one.
432440
*/
433-
func clearUserData() {
441+
func clearUserData(_ user: OSUserInternal) {
434442
// Identity and property models should still be the same instances, but with data cleared
435-
_user?.identityModel.clearData()
436-
_user?.propertiesModel.clearData()
443+
user.identityModel.clearData()
444+
user.propertiesModel.clearData()
437445

438446
// Subscription model store should be cleared completely
439447
OneSignalUserManagerImpl.sharedInstance.subscriptionModelStore.clearModelsFromStore()

‎iOS_SDK/OneSignalSDK/OneSignalUserTests/Executors/UserExecutorTests.swift‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -222,8 +222,8 @@ final class UserExecutorTests: XCTestCase {
222222
}
223223

224224
/**
225-
The normal new-session Fetch User for the *current* user must still clear stale local data before hydrating
226-
from the response, so the `isCurrentUser` guard added for the race above does not regress the common path.
225+
A Fetch User for the *current* user must still clear stale local data before hydrating from the
226+
response, so guarding against the race above does not regress the common path.
227227
*/
228228
func testFetchUser_forCurrentUser_stillClearsStaleData() {
229229
/* Setup */

‎iOS_SDK/OneSignalSDK/OneSignalUserTests/OneSignalUserTests.swift‎

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -248,4 +248,65 @@ final class OneSignalUserTests: XCTestCase {
248248
// The confirmed tags from the 202 response are merged back into the local model
249249
XCTAssertEqual(OneSignalUserManagerImpl.sharedInstance.getTags(), tags)
250250
}
251+
252+
// MARK: - Atomic current user access
253+
254+
/**
255+
A callback that acts on the current user must keep acting on the user it checked, even if a
256+
`login()` makes a different user current right afterwards. Swapping the user between the check
257+
and the mutation reproduces that interleaving.
258+
*/
259+
func testCurrentUser_matching_isTheUserMutated_whenTheUserChangesRightAfterTheCheck() throws {
260+
/* Setup */
261+
OneSignalCoreImpl.setSharedClient(MockOneSignalClient())
262+
let manager = OneSignalUserManagerImpl.sharedInstance
263+
let userA = OneSignalUserMocks.setUserManagerInternalUser(externalId: userA_EUID, onesignalId: userA_OSID)
264+
265+
/* When */
266+
let checkedUser = manager.currentUser(matching: userA.identityModel.modelId)
267+
// A concurrent login switches the current user before the response is applied
268+
let userB = OneSignalUserMocks.setUserManagerInternalUser(externalId: userB_EUID, onesignalId: userB_OSID)
269+
checkedUser?.propertiesModel.hydrate(["language": "language-for-user-a"])
270+
271+
/* Then */
272+
// The response's data went to the user it was for, and the new current user is untouched
273+
XCTAssertEqual(userA.propertiesModel.language, "language-for-user-a")
274+
XCTAssertNil(userB.propertiesModel.language)
275+
XCTAssertEqual(manager._user?.identityModel.externalId, userB_EUID)
276+
}
277+
278+
/// The common path: the request's user is still current, so it is returned to be mutated.
279+
func testCurrentUser_matching_returnsTheCurrentUser() throws {
280+
/* Setup */
281+
OneSignalCoreImpl.setSharedClient(MockOneSignalClient())
282+
let manager = OneSignalUserManagerImpl.sharedInstance
283+
let user = OneSignalUserMocks.setUserManagerInternalUser(externalId: userA_EUID, onesignalId: userA_OSID)
284+
285+
/* Then */
286+
XCTAssertEqual(manager.currentUser(matching: user.identityModel.modelId)?.identityModel.externalId, userA_EUID)
287+
}
288+
289+
/// A response for a user that is no longer current must not be applied at all.
290+
func testCurrentUser_matching_isNilWhenTheUserIsNoLongerCurrent() throws {
291+
/* Setup */
292+
OneSignalCoreImpl.setSharedClient(MockOneSignalClient())
293+
let manager = OneSignalUserManagerImpl.sharedInstance
294+
let userA = OneSignalUserMocks.setUserManagerInternalUser(externalId: userA_EUID, onesignalId: userA_OSID)
295+
let userB = OneSignalUserMocks.setUserManagerInternalUser(externalId: userB_EUID, onesignalId: userB_OSID)
296+
297+
/* Then */
298+
XCTAssertNil(manager.currentUser(matching: userA.identityModel.modelId))
299+
XCTAssertNotNil(manager.currentUser(matching: userB.identityModel.modelId))
300+
}
301+
302+
/// With no current user, there is nothing for a late response to act on.
303+
func testCurrentUser_matching_isNilWhenThereIsNoUser() throws {
304+
/* Setup */
305+
let manager = OneSignalUserManagerImpl.sharedInstance
306+
let identityModel = OSIdentityModel(aliases: nil, changeNotifier: OSEventProducer())
307+
308+
/* Then */
309+
XCTAssertNil(manager._user)
310+
XCTAssertNil(manager.currentUser(matching: identityModel.modelId))
311+
}
251312
}

0 commit comments

Comments
 (0)