Skip to content

Commit 650e96f

Browse files
committed
Fix PR review issues: fallback 401 handling, volatile fields, TOCTOU race
- Add UNAUTHORIZED check to fetchInAppMessagesWithoutRywToken so the JWT retry mechanism works when the fallback (no RYW token) path hits a 401/403. - Add @volatile to pendingJwtRetryExternalId and pendingJwtRetryRywData for cross-thread memory visibility. - Fix TOCTOU race in UserManager between fireJwtInvalidated and addJwtInvalidatedListener using a synchronized lock, preventing a lost JWT invalidated event when the two methods interleave. Made-with: Cursor
1 parent 60c8d9a commit 650e96f

4 files changed

Lines changed: 51 additions & 13 deletions

File tree

OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/UserManager.kt

Lines changed: 19 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -55,14 +55,18 @@ internal open class UserManager(
5555
private val jwtInvalidatedAppCallbackScope =
5656
CoroutineScope(SupervisorJob() + Dispatchers.Default)
5757

58-
@Volatile
58+
private val jwtInvalidatedLock = Any()
5959
private var pendingJwtInvalidatedExternalId: String? = null
6060

6161
fun addJwtInvalidatedListener(listener: IUserJwtInvalidatedListener) {
62-
jwtInvalidatedNotifier.subscribe(listener)
63-
pendingJwtInvalidatedExternalId?.let { externalId ->
62+
val pendingExternalId: String?
63+
synchronized(jwtInvalidatedLock) {
64+
jwtInvalidatedNotifier.subscribe(listener)
65+
pendingExternalId = pendingJwtInvalidatedExternalId
6466
pendingJwtInvalidatedExternalId = null
65-
listener.onUserJwtInvalidated(UserJwtInvalidatedEvent(externalId))
67+
}
68+
pendingExternalId?.let {
69+
listener.onUserJwtInvalidated(UserJwtInvalidatedEvent(it))
6670
}
6771
}
6872

@@ -77,18 +81,20 @@ internal open class UserManager(
7781
* listener is added via [addJwtInvalidatedListener].
7882
*/
7983
fun fireJwtInvalidated(externalId: String) {
80-
if (jwtInvalidatedNotifier.hasSubscribers) {
81-
jwtInvalidatedAppCallbackScope.launch {
82-
runCatching {
83-
jwtInvalidatedNotifier.fire { listener ->
84-
listener.onUserJwtInvalidated(UserJwtInvalidatedEvent(externalId))
84+
synchronized(jwtInvalidatedLock) {
85+
if (jwtInvalidatedNotifier.hasSubscribers) {
86+
jwtInvalidatedAppCallbackScope.launch {
87+
runCatching {
88+
jwtInvalidatedNotifier.fire { listener ->
89+
listener.onUserJwtInvalidated(UserJwtInvalidatedEvent(externalId))
90+
}
91+
}.onFailure {
92+
Logging.warn("Failed to deliver JWT invalidated event for externalId=$externalId", it)
8593
}
86-
}.onFailure {
87-
Logging.warn("Failed to deliver JWT invalidated event for externalId=$externalId", it)
8894
}
95+
} else {
96+
pendingJwtInvalidatedExternalId = externalId
8997
}
90-
} else {
91-
pendingJwtInvalidatedExternalId = externalId
9298
}
9399
}
94100

OneSignalSDK/onesignal/in-app-messages/src/main/java/com/onesignal/inAppMessages/internal/InAppMessagesManager.kt

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -129,7 +129,10 @@ internal class InAppMessagesManager(
129129

130130
// Pending IAM retry state for 401 (expired JWT) responses.
131131
// Stores the externalId and rywData from the failed fetch so we can retry after JWT refresh.
132+
@Volatile
132133
private var pendingJwtRetryExternalId: String? = null
134+
135+
@Volatile
133136
private var pendingJwtRetryRywData: RywData? = null
134137

135138
private val identityModelChangeHandler =

OneSignalSDK/onesignal/in-app-messages/src/main/java/com/onesignal/inAppMessages/internal/backend/impl/InAppBackendService.kt

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -271,6 +271,8 @@ internal class InAppBackendService(
271271
if (response.isSuccess) {
272272
val jsonResponse = response.payload?.let { JSONObject(it) }
273273
return jsonResponse?.let { hydrateInAppMessages(it) }
274+
} else if (NetworkUtils.getResponseStatusType(response.statusCode) == NetworkUtils.ResponseStatusType.UNAUTHORIZED) {
275+
throw BackendException(response.statusCode, response.payload, response.retryAfterSeconds)
274276
} else {
275277
return null
276278
}

OneSignalSDK/onesignal/in-app-messages/src/test/java/com/onesignal/inAppMessages/internal/backend/InAppBackendServiceTests.kt

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -105,6 +105,33 @@ class InAppBackendServiceTests :
105105
coVerify(exactly = 1) { mockHttpClient.get("apps/appId/users/by/onesignal_id/user123/subscriptions/subscriptionId/iams", any()) }
106106
}
107107

108+
test("listInAppMessages throws BackendException on 401 from fallback (no RYW token) path") {
109+
// Given
110+
val mockHydrator = InAppHydrator(MockHelper.time(1000), MockHelper.propertiesModelStore())
111+
val mockHttpClient = mockk<IHttpClient>()
112+
113+
// Exhaust retries with 425 then return 401 on the fallback request (without RYW token)
114+
coEvery {
115+
mockHttpClient.get(any(), any())
116+
} returnsMany
117+
listOf(
118+
HttpResponse(425, null, retryAfterSeconds = 0, retryLimit = 0),
119+
HttpResponse(401, "{\"errors\":[\"Invalid token\"]}"),
120+
)
121+
122+
val inAppBackendService = InAppBackendService(mockHttpClient, MockHelper.deviceService(), mockHydrator)
123+
124+
// When / Then
125+
val exception =
126+
shouldThrowUnit<BackendException> {
127+
inAppBackendService.listInAppMessages("appId", "onesignal_id", "user123", "subscriptionId", RywData("123", 500L), mockSessionDurationProvider, "expired-jwt")
128+
}
129+
130+
exception.statusCode shouldBe 401
131+
// First call is the retry attempt (with RYW), second is the fallback (without RYW)
132+
coVerify(exactly = 2) { mockHttpClient.get(any(), any()) }
133+
}
134+
108135
test("listInAppMessages returns null when non-success response") {
109136
// Given
110137
val mockHydrator = InAppHydrator(MockHelper.time(1000), MockHelper.propertiesModelStore())

0 commit comments

Comments
 (0)