Skip to content

fix : Handling DPoP enabled WebAuth flow after process death#977

Open
pmathew92 wants to merge 6 commits into
mainfrom
fix_dpop_process_death
Open

fix : Handling DPoP enabled WebAuth flow after process death#977
pmathew92 wants to merge 6 commits into
mainfrom
fix_dpop_process_death

Conversation

@pmathew92

@pmathew92 pmathew92 commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Changes

Fixes a bug where the OAuth token exchange fails after process death during a
DPoP-enabled Universal Login flow.

When the Android host process is killed while Chrome Custom Tabs is open, the SDK
persists OAuthManagerState to savedInstanceState and rebuilds the manager via
OAuthManager.fromState on restart. Previously, the DPoP configuration was lost on
this restore path:

  • OAuthManager.toState() did not persist the DPoP-enabled flag.
  • OAuthManager.fromState() reconstructed the manager without DPoP, and the restored
    PKCE's AuthenticationAPIClient was never DPoP-enabled.

As a result, the /oauth/token exchange after process restart was sent without a
DPoP proof header
, and the server rejected it with HTTP 400 invalid_request
("Authorization Code is bound to a DPoP key, but DPoP header is missing or invalid").

This change persists a dPoPEnabled boolean in OAuthManagerState (the DPoP
object holds a Context and cannot be serialized directly), and on restore re-creates
the DPoP instance and re-enables DPoP on the restored API client so the token
exchange carries the proof header.

References

Files changed

  • OAuthManager.kttoState persists dPoPEnabled; fromState takes a Context,
    re-enables DPoP on the restored API client, and rebuilds the DPoP instance.
  • OAuthManagerState.kt — replaced the non-serializable DPoP? field with
    dPoPEnabled: Boolean.
  • WebAuthProvider.ktonRestoreInstanceState accepts a Context and forwards it
    to fromState. PAR restore path is unchanged.
  • AuthenticationActivity.kt — passes the activity Context into
    onRestoreInstanceState.

Tests

  • OAuthManagerStateTest — serialization of dPoPEnabled (true / default false /
    legacy JSON missing the field defaults to false, preserving back-compat with
    previously persisted state).
  • WebAuthProviderTest — added a behavioral test that simulates the process
    death → restore path and asserts DPoP is re-enabled on the restored manager.

Testing

Please describe how this can be tested by reviewers. Be specific about anything not tested and reasons why. Since this library has unit testing, tests should be added for new functionality and existing tests should complete without errors.

  • This change adds unit test coverage

  • This change adds integration test coverage

  • This change has been tested on the latest version of the platform/language or why not

Checklist

Summary by CodeRabbit

  • Bug Fixes
    • Enhanced recovery of authentication state after app process death, ensuring DPoP-enabled secure authentication flows remain intact when the app is restored from the background.
    • Improved handling of pending OAuth results by buffering them until callbacks are registered, preventing authentication responses from being lost during app restoration.
    • Better preservation of authentication configuration during activity recreation.

@pmathew92 pmathew92 requested a review from a team as a code owner June 8, 2026 07:18
@coderabbitai

coderabbitai Bot commented Jun 8, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

This PR persists DPoP enablement as a boolean across process death, threads Android Context through the OAuth restoration path, and implements buffering for results that arrive before callbacks are registered. The state model, OAuth manager restore logic, WebAuthProvider restore flow, and activity integration are updated together with comprehensive test coverage.

Changes

DPoP State Persistence Across Process Death

Layer / File(s) Summary
DPoP State Model and Serialization
auth0/src/main/java/com/auth0/android/provider/OAuthManagerState.kt
Replace stored DPoP object with dPoPEnabled: Boolean in OAuthManagerState; update internal OAuthManagerJson model; modify serializeToJson() and deserializeState() to persist/restore boolean flag instead of object.
OAuth Manager DPoP Re-enablement on Restore
auth0/src/main/java/com/auth0/android/provider/OAuthManager.kt
OAuthManager.toState() now persists dPoPEnabled flag. OAuthManager.Companion.fromState() accepts a Context parameter and conditionally enables DPoP on the restored AuthenticationAPIClient when the state flag is true, then restores headers, PKCE, and id-token verification settings.
WebAuthProvider Restore with Result Buffering and Context Threading
auth0/src/main/java/com/auth0/android/provider/WebAuthProvider.kt
onRestoreInstanceState() signature now accepts Context. Added RecoveredResult sealed type and pendingRecovered state (guarded by recoveryLock) to buffer results until callbacks are registered. OAuth restore path passes Context to OAuthManager.fromState().
AuthenticationActivity Context Wiring
auth0/src/main/java/com/auth0/android/provider/AuthenticationActivity.kt
AuthenticationActivity.onCreate() now passes the Activity instance (this) as Context when calling WebAuthProvider.onRestoreInstanceState().
State Persistence, Restore, and Process Death Recovery Tests
auth0/src/test/java/com/auth0/android/provider/OAuthManagerStateTest.kt, auth0/src/test/java/com/auth0/android/provider/WebAuthProviderTest.kt
Unit tests verify dPoPEnabled serialization (true when set, false by default) and deserialization with backward compatibility for legacy JSON. Regression tests confirm OAuthManager.fromState() restores AuthenticationAPIClient.isDPoPEnabled matching the state flag. Integration test simulates process death by saving/resetting/restoring WebAuthProvider state and asserts dPoP is reinitialized.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 A bunny hops through bundles,
DPoP flags persist through the death,
Recovering flows with Context in tow,
Tests guard the auth path.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 5.26% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and specifically describes the main fix: handling the DPoP-enabled WebAuth flow after process death, which is the core issue being resolved in this PR.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix_dpop_process_death

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (2)
auth0/src/test/java/com/auth0/android/provider/WebAuthProviderTest.kt (1)

2979-2980: 🏗️ Heavy lift

Validate restored behavior at the token exchange boundary, not only object presence.

A non-null restoredManager.dPoP doesn’t prove /oauth/token uses DPoP proof. Add an assertion that the resumed token request carries the DPoP header to directly cover the original regression.

Suggested test direction
-        val restoredManager = WebAuthProvider.managerInstance as OAuthManager
-        assertThat(restoredManager.dPoP, `is`(notNullValue()))
+        val restoredManager = WebAuthProvider.managerInstance as OAuthManager
+        assertThat(restoredManager.dPoP, `is`(notNullValue()))
+        // Then resume the flow and assert the outgoing /oauth/token request includes a DPoP header.
+        // (Use existing request-capture patterns in this test suite to verify headers.)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@auth0/src/test/java/com/auth0/android/provider/WebAuthProviderTest.kt` around
lines 2979 - 2980, The test currently only checks restoredManager.dPoP is
non-null; update it to assert the resumed token-exchange request actually
includes the DPoP header so the /oauth/token boundary is covered. After
obtaining restoredManager from WebAuthProvider.managerInstance (cast to
OAuthManager), simulate or capture the resumed token request the manager sends
during resume (the same place the original regression occurred) and assert that
the HTTP request contains the DPoP header (e.g., "DPoP" or the library's header
constant) and/or the expected proof value; ensure you reference
restoredManager.dPoP and the resumed token request object when adding the
assertion.
auth0/src/test/java/com/auth0/android/provider/OAuthManagerStateTest.kt (1)

124-126: ⚡ Quick win

Use JSON field removal instead of raw string replacement for legacy payload simulation.

The current replacement is formatting/order-sensitive and can become flaky. Remove dPoPEnabled via a parsed JSON object instead.

Suggested diff
+import com.google.gson.JsonObject
...
-        // Remove the dPoPEnabled field to simulate legacy JSON
-        val legacyJson = json.replace(",\"dPoPEnabled\":false", "")
+        // Remove the dPoPEnabled field to simulate legacy JSON
+        val legacyJsonObject = GsonProvider.gson.fromJson(json, JsonObject::class.java)
+        legacyJsonObject.remove("dPoPEnabled")
+        val legacyJson = legacyJsonObject.toString()
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@auth0/src/test/java/com/auth0/android/provider/OAuthManagerStateTest.kt`
around lines 124 - 126, The test currently generates legacyJson by doing a
fragile string replacement on json; instead parse the JSON into a JSON object,
remove the "dPoPEnabled" field programmatically, then serialize back to a string
so ordering/formatting won't break the test; locate the variables json and
legacyJson in OAuthManagerStateTest.kt and replace the raw string replace with
logic that parses json (e.g., using org.json.JSONObject or your project’s JSON
library), calls remove("dPoPEnabled") on the parsed object, and assigns
legacyJson to the serialized result.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@auth0/src/test/java/com/auth0/android/provider/OAuthManagerStateTest.kt`:
- Around line 124-126: The test currently generates legacyJson by doing a
fragile string replacement on json; instead parse the JSON into a JSON object,
remove the "dPoPEnabled" field programmatically, then serialize back to a string
so ordering/formatting won't break the test; locate the variables json and
legacyJson in OAuthManagerStateTest.kt and replace the raw string replace with
logic that parses json (e.g., using org.json.JSONObject or your project’s JSON
library), calls remove("dPoPEnabled") on the parsed object, and assigns
legacyJson to the serialized result.

In `@auth0/src/test/java/com/auth0/android/provider/WebAuthProviderTest.kt`:
- Around line 2979-2980: The test currently only checks restoredManager.dPoP is
non-null; update it to assert the resumed token-exchange request actually
includes the DPoP header so the /oauth/token boundary is covered. After
obtaining restoredManager from WebAuthProvider.managerInstance (cast to
OAuthManager), simulate or capture the resumed token request the manager sends
during resume (the same place the original regression occurred) and assert that
the HTTP request contains the DPoP header (e.g., "DPoP" or the library's header
constant) and/or the expected proof value; ensure you reference
restoredManager.dPoP and the resumed token request object when adding the
assertion.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 90c807f2-792c-4cc3-953f-8b8741d2cd1d

📥 Commits

Reviewing files that changed from the base of the PR and between 47f75ae and cc9983e.

📒 Files selected for processing (6)
  • auth0/src/main/java/com/auth0/android/provider/AuthenticationActivity.kt
  • auth0/src/main/java/com/auth0/android/provider/OAuthManager.kt
  • auth0/src/main/java/com/auth0/android/provider/OAuthManagerState.kt
  • auth0/src/main/java/com/auth0/android/provider/WebAuthProvider.kt
  • auth0/src/test/java/com/auth0/android/provider/OAuthManagerStateTest.kt
  • auth0/src/test/java/com/auth0/android/provider/WebAuthProviderTest.kt

@pmathew92

Copy link
Copy Markdown
Contributor Author

@CodeRabbit review

@coderabbitai

coderabbitai Bot commented Jun 8, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Comment thread auth0/src/main/java/com/auth0/android/provider/OAuthManager.kt Outdated
callback = callback,
customAuthorizeUrl = state.customAuthorizeUrl
customAuthorizeUrl = state.customAuthorizeUrl,
dPoP = if (state.dPoPEnabled ) DPoP(context) else null

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The real fix is the useDPoP(context) call above — this dPoP arg only gets read in startAuthentication(), which doesn't run on resume. Is it set here just to keep state consistent / for the test to assert on? Want to confirm it's intentional.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the dPoP arg is intentional — for state-consistency on toState(), round-trip + the test assertion.

Comment thread auth0/src/main/java/com/auth0/android/provider/OAuthManagerState.kt Outdated
@utkrishtsahu

utkrishtsahu commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

@pmathew92 The PR notes the PAR restore path (PARCodeManager.fromState) is left unchanged. If a DPoP + PAR Universal Login flow is supported in this SDK, wouldn't it hit the same process-death bug and not be covered by this fix? Could you confirm whether that combination is possible?

@pmathew92

Copy link
Copy Markdown
Contributor Author

@utkrishtsahu PAR flow doesn't support DPoP since in PAR the jwk exchange is done in the client backend. So the flow is not applicable

@pmathew92 pmathew92 closed this Jun 8, 2026
@pmathew92 pmathew92 reopened this Jun 8, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
auth0/src/test/java/com/auth0/android/provider/OAuthManagerStateTest.kt (1)

87-109: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Assert the serialized field explicitly in the default-false test.

Line 87 currently validates only round-trip behavior; it can still pass if serializeToJson() stops emitting dPoPEnabled and deserialization falls back to false.

Suggested patch
     val json = state.serializeToJson()
+    Assert.assertTrue(json.contains("\"dPoPEnabled\":false"))

     val deserializedState = OAuthManagerState.deserializeState(json)

     Assert.assertFalse(deserializedState.dPoPEnabled)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@auth0/src/test/java/com/auth0/android/provider/OAuthManagerStateTest.kt`
around lines 87 - 109, The test currently only round-trips state via
OAuthManagerState.serializeToJson() and deserializeState(), which can hide
missing serialization of dPoPEnabled; update the test to explicitly assert the
serialized JSON contains the dPoPEnabled field set to false (e.g. check the JSON
string includes the "dPoPEnabled":false key/value) before calling
OAuthManagerState.deserializeState(json), referencing serializeToJson() and
OAuthManagerState.deserializeState so the test fails if the serializer stops
emitting dPoPEnabled.
🧹 Nitpick comments (1)
auth0/src/test/java/com/auth0/android/provider/OAuthManagerStateTest.kt (1)

131-134: ⚡ Quick win

Use structured JSON mutation for the legacy-payload test.

Line 133 relies on exact JSON text formatting/order. Parsing to a JSON object and removing the key makes this test resilient.

Suggested patch
+import com.google.gson.JsonObject
...
-        val legacyJson = json.replace(",\"dPoPEnabled\":false", "")
+        val jsonObject = GsonProvider.gson.fromJson(json, JsonObject::class.java)
+        jsonObject.remove("dPoPEnabled")
+        val legacyJson = jsonObject.toString()
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@auth0/src/test/java/com/auth0/android/provider/OAuthManagerStateTest.kt`
around lines 131 - 134, The test in OAuthManagerStateTest currently mutates the
serialized JSON string using exact text replacement (the json and legacyJson
variables); instead, parse the serialized string returned by serializeToJson()
into a JSON object/node, remove the "dPoPEnabled" key from that object, then
re-serialize to a string to produce legacyJson so the test no longer depends on
property ordering/formatting; locate the serializeToJson() usage in the test and
replace the string replace call with JSON parsing, key removal, and
toString/serialize back to JSON.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@auth0/src/test/java/com/auth0/android/provider/OAuthManagerStateTest.kt`:
- Around line 87-109: The test currently only round-trips state via
OAuthManagerState.serializeToJson() and deserializeState(), which can hide
missing serialization of dPoPEnabled; update the test to explicitly assert the
serialized JSON contains the dPoPEnabled field set to false (e.g. check the JSON
string includes the "dPoPEnabled":false key/value) before calling
OAuthManagerState.deserializeState(json), referencing serializeToJson() and
OAuthManagerState.deserializeState so the test fails if the serializer stops
emitting dPoPEnabled.

---

Nitpick comments:
In `@auth0/src/test/java/com/auth0/android/provider/OAuthManagerStateTest.kt`:
- Around line 131-134: The test in OAuthManagerStateTest currently mutates the
serialized JSON string using exact text replacement (the json and legacyJson
variables); instead, parse the serialized string returned by serializeToJson()
into a JSON object/node, remove the "dPoPEnabled" key from that object, then
re-serialize to a string to produce legacyJson so the test no longer depends on
property ordering/formatting; locate the serializeToJson() usage in the test and
replace the string replace call with JSON parsing, key removal, and
toString/serialize back to JSON.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 6d5f7c3b-06f0-4fe2-af2d-1ce2d7876bba

📥 Commits

Reviewing files that changed from the base of the PR and between cc9983e and 65be2b4.

📒 Files selected for processing (5)
  • .github/workflows/test.yml
  • auth0/src/main/java/com/auth0/android/provider/OAuthManager.kt
  • auth0/src/main/java/com/auth0/android/provider/OAuthManagerState.kt
  • auth0/src/test/java/com/auth0/android/provider/OAuthManagerStateTest.kt
  • auth0/src/test/java/com/auth0/android/provider/WebAuthProviderTest.kt
🚧 Files skipped from review as they are similar to previous changes (2)
  • auth0/src/test/java/com/auth0/android/provider/WebAuthProviderTest.kt
  • auth0/src/main/java/com/auth0/android/provider/OAuthManager.kt

utkrishtsahu
utkrishtsahu previously approved these changes Jun 9, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@auth0/src/main/java/com/auth0/android/provider/WebAuthProvider.kt`:
- Around line 184-203: The restored callback dispatch is racy: while you check
callbacks.isEmpty() under recoveryLock in onRestoreInstanceState's
Callback.onSuccess/onFailure, you iterate callbacks outside the lock and
removeCallback mutates callbacks without locking, so recovered results may be
lost; fix by acquiring recoveryLock, check for empty and set pendingRecovered if
empty, otherwise make a shallow copy of callbacks (e.g., new list) while still
inside synchronized(recoveryLock), release the lock and then iterate that copied
list to call callback.onSuccess/onFailure; reference symbols: recoveryLock,
callbacks, pendingRecovered, RecoveredResult.Success/Failure,
onRestoreInstanceState and removeCallback.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 04df376a-a753-4bd2-85af-a411cc821911

📥 Commits

Reviewing files that changed from the base of the PR and between 65be2b4 and ff8bf8e.

📒 Files selected for processing (1)
  • auth0/src/main/java/com/auth0/android/provider/WebAuthProvider.kt

Comment on lines +184 to 203
synchronized(recoveryLock) {
if (callbacks.isEmpty()) {
pendingRecovered = RecoveredResult.Success(result)
return
}
}
for (callback in callbacks) {
callback.onSuccess(result)
}
}

override fun onFailure(error: AuthenticationException) {
synchronized(recoveryLock) {
if (callbacks.isEmpty()) {
pendingRecovered = RecoveredResult.Failure(error)
return
}
}
for (callback in callbacks) {
callback.onFailure(error)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

FILE="auth0/src/main/java/com/auth0/android/provider/WebAuthProvider.kt"

# Show the full relevant region around the reported lines
nl -ba "$FILE" | sed -n '140,260p'

# Locate the exact declarations/usages of recoveryLock and callbacks/pendingRecovered
rg -n "recoveryLock|pendingRecovered|callbacks|addCallback|onRestoreInstanceState|RecoveredResult" "$FILE"

# Also find where callbacks are added/removed (mutation points)
rg -n "callbacks\.|addCallback|removeCallback|clear\(" "$FILE"

# Print the callbacks field and recoveryLock initialization area
nl -ba "$FILE" | sed -n '1,140p'

Repository: auth0/Auth0.Android

Length of output: 106


🏁 Script executed:

#!/bin/bash
set -euo pipefail

FILE="auth0/src/main/java/com/auth0/android/provider/WebAuthProvider.kt"

if [ ! -f "$FILE" ]; then
  echo "Missing file: $FILE"
  exit 1
fi

# Relevant region around the reported lines
cat -n "$FILE" | sed -n '140,260p'

# Locate exact declarations/usages
rg -n "recoveryLock|pendingRecovered|callbacks|addCallback|onRestoreInstanceState|RecoveredResult" "$FILE"

# Find mutation points
rg -n "callbacks\.|addCallback|removeCallback|clear\(" "$FILE"

# Initialization / context area
cat -n "$FILE" | sed -n '1,140p'

Repository: auth0/Auth0.Android

Length of output: 14715


Prevent recovered OAuth result from being dropped due to non-atomic callbacks emptiness check/dispatch

In WebAuthProvider.onRestoreInstanceState’s restore Callback.onSuccess/onFailure, the code checks callbacks.isEmpty() under recoveryLock but then dispatches by iterating callbacks outside the lock. Since removeCallback() mutates callbacks without recoveryLock, callbacks can become empty between the check and the iteration, leaving pendingRecovered unset and the recovered result un-delivered.

Suggested fix
                         override fun onSuccess(result: Credentials) {
-                            synchronized(recoveryLock) {
-                                if (callbacks.isEmpty()) {
-                                    pendingRecovered = RecoveredResult.Success(result)
-                                    return
-                                }
-                            }
-                            for (callback in callbacks) {
+                            val snapshot = synchronized(recoveryLock) {
+                                if (callbacks.isEmpty()) {
+                                    pendingRecovered = RecoveredResult.Success(result)
+                                    return
+                                }
+                                callbacks.toList()
+                            }
+                            for (callback in snapshot) {
                                 callback.onSuccess(result)
                             }
                         }

                         override fun onFailure(error: AuthenticationException) {
-                            synchronized(recoveryLock) {
-                                if (callbacks.isEmpty()) {
-                                    pendingRecovered = RecoveredResult.Failure(error)
-                                    return
-                                }
-                            }
-                            for (callback in callbacks) {
+                            val snapshot = synchronized(recoveryLock) {
+                                if (callbacks.isEmpty()) {
+                                    pendingRecovered = RecoveredResult.Failure(error)
+                                    return
+                                }
+                                callbacks.toList()
+                            }
+                            for (callback in snapshot) {
                                 callback.onFailure(error)
                             }
                         }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
synchronized(recoveryLock) {
if (callbacks.isEmpty()) {
pendingRecovered = RecoveredResult.Success(result)
return
}
}
for (callback in callbacks) {
callback.onSuccess(result)
}
}
override fun onFailure(error: AuthenticationException) {
synchronized(recoveryLock) {
if (callbacks.isEmpty()) {
pendingRecovered = RecoveredResult.Failure(error)
return
}
}
for (callback in callbacks) {
callback.onFailure(error)
synchronized(recoveryLock) {
if (callbacks.isEmpty()) {
pendingRecovered = RecoveredResult.Success(result)
return
}
}
for (callback in callbacks) {
callback.onSuccess(result)
}
}
override fun onFailure(error: AuthenticationException) {
val snapshot = synchronized(recoveryLock) {
if (callbacks.isEmpty()) {
pendingRecovered = RecoveredResult.Failure(error)
return
}
callbacks.toList()
}
for (callback in snapshot) {
callback.onFailure(error)
}
}
Suggested change
synchronized(recoveryLock) {
if (callbacks.isEmpty()) {
pendingRecovered = RecoveredResult.Success(result)
return
}
}
for (callback in callbacks) {
callback.onSuccess(result)
}
}
override fun onFailure(error: AuthenticationException) {
synchronized(recoveryLock) {
if (callbacks.isEmpty()) {
pendingRecovered = RecoveredResult.Failure(error)
return
}
}
for (callback in callbacks) {
callback.onFailure(error)
val snapshot = synchronized(recoveryLock) {
if (callbacks.isEmpty()) {
pendingRecovered = RecoveredResult.Success(result)
return
}
callbacks.toList()
}
for (callback in snapshot) {
callback.onSuccess(result)
}
}
override fun onFailure(error: AuthenticationException) {
val snapshot = synchronized(recoveryLock) {
if (callbacks.isEmpty()) {
pendingRecovered = RecoveredResult.Failure(error)
return
}
callbacks.toList()
}
for (callback in snapshot) {
callback.onFailure(error)
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@auth0/src/main/java/com/auth0/android/provider/WebAuthProvider.kt` around
lines 184 - 203, The restored callback dispatch is racy: while you check
callbacks.isEmpty() under recoveryLock in onRestoreInstanceState's
Callback.onSuccess/onFailure, you iterate callbacks outside the lock and
removeCallback mutates callbacks without locking, so recovered results may be
lost; fix by acquiring recoveryLock, check for empty and set pendingRecovered if
empty, otherwise make a shallow copy of callbacks (e.g., new list) while still
inside synchronized(recoveryLock), release the lock and then iterate that copied
list to call callback.onSuccess/onFailure; reference symbols: recoveryLock,
callbacks, pendingRecovered, RecoveredResult.Success/Failure,
onRestoreInstanceState and removeCallback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants