fix: piggycards auth hardening#1496
Open
HashEngineering wants to merge 2 commits into
Open
Conversation
Apply the same auth hardening to PiggyCards that PR #1492 applied to CTX: - Move PiggyCardsAuthenticator.tokenMutex to the companion object so it is shared across every instance (the OkHttp retry path and the factory-built instance used by the repository), serializing concurrent re-logins against the single shared token store instead of racing on separate per-instance locks. - Funnel all re-logins through PiggyCardsAuthenticator.reLogin(): the repository's performAutoLogin() now delegates to it, so the repo and OkHttp paths share the same lock and token persistence. - Preserve the cached token on transient failures; only a genuine HTTP 401 (credential rejection, surfaced as CTXSpendException) clears it. Previously any exception wiped the access token. - Add a responseCount guard to authenticate() to avoid retry loops. Note: Problem 1 from PR #1492 (verifyEmail mis-reporting success as "invalid code") is already fixed for PiggyCards by that PR, since the routing lives in the shared DashSpendUserAuthFragment. Adds PiggyCardsAuthenticatorTest covering serialized re-login, token preservation on transient failure, and token clearing on a genuine 401. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthrough
ChangesPiggyCards Authentication Hardening
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
features/exploredash/src/test/java/org/dash/wallet/features/exploredash/PiggyCardsAuthenticatorTest.kt (1)
55-55: ⚡ Quick winUse an expression body for the no-op override.
This keeps the stub behavior while clearing the detekt
EmptyFunctionBlockwarning.Proposed fix
- override fun deleteKey(keyAlias: String) {} + override fun deleteKey(keyAlias: String) = Unit🤖 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 `@features/exploredash/src/test/java/org/dash/wallet/features/exploredash/PiggyCardsAuthenticatorTest.kt` at line 55, The deleteKey method override in PiggyCardsAuthenticatorTest has an empty function block body which triggers the detekt EmptyFunctionBlock warning. Convert the method from block body syntax with empty braces to an expression body using = Unit syntax, which maintains the no-op behavior while satisfying the detekt rule.Source: Linters/SAST tools
features/exploredash/src/main/java/org/dash/wallet/features/exploredash/network/authenticator/PiggyCardsAuthenticator.kt (1)
54-55: Add token coalescing to avoid redundant logins on burst 401s.The
reLogin()method always callstokenApi.login()after acquiring the mutex, even if another waiter already stored a fresh token. Under burst 401s, each waiting request sequentially invokes the login endpoint instead of reusing the refreshed token, causing unnecessary API churn that can trigger rate limits or invalidate earlier retries.Before calling
tokenApi.login(), capture the failed request'sAuthorizationheader token and compare it against the cached token. If they differ, another waiter already refreshed the token—reuse it instead. Only issue a new login call when the cached token still matches the failed request's token.Also applies to: 72-82
🤖 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 `@features/exploredash/src/main/java/org/dash/wallet/features/exploredash/network/authenticator/PiggyCardsAuthenticator.kt` around lines 54 - 55, In the reLogin() method (around lines 54-82), implement token coalescing to prevent redundant logins during burst 401 errors. Before calling tokenApi.login(), capture the Authorization header token from the failed request and compare it against the currently cached token. If the tokens differ, another waiting request has already refreshed the token, so reuse the cached token instead of invoking tokenApi.login(). Only proceed with a new tokenApi.login() call when the cached token still matches the token from the failed request.
🤖 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
`@features/exploredash/src/main/java/org/dash/wallet/features/exploredash/network/authenticator/PiggyCardsAuthenticator.kt`:
- Around line 81-83: The issue is that in the login method, persistLogin() is
being called before validating that response.accessToken is not blank, which
means invalid or blank tokens can overwrite cached credentials. Fix this by
reordering the logic so that the token is validated for being non-blank first,
and then persistLogin() is only called if the token passes that validation
check. The takeIf { it.isNotBlank() } check should be performed before the
persistLogin() call, not after it.
- Around line 84-94: The generic catch block catching Exception in the token
refresh logic is intercepting CancellationException, which breaks structured
concurrency by silencing coroutine cancellation. Add an explicit catch handler
for CancellationException before the existing generic Exception catch block in
this try-catch structure, and rethrow the CancellationException immediately to
preserve proper coroutine cancellation semantics.
In
`@features/exploredash/src/test/java/org/dash/wallet/features/exploredash/PiggyCardsAuthenticatorTest.kt`:
- Around line 95-98: The test creates threadA and threadB using thread {
authenticatorA.authenticate(...) } and thread { authenticatorB.authenticate(...)
} but any exceptions thrown within these threads are not captured or rethrown
after the join() calls, allowing failures to be silently ignored. Capture any
exceptions thrown by each thread (using try-catch or similar mechanism within
the thread creation), store them, and after both threadA.join() and
threadB.join() complete, rethrow any captured exceptions so that failures in
either authenticate() call will properly fail the test.
---
Nitpick comments:
In
`@features/exploredash/src/main/java/org/dash/wallet/features/exploredash/network/authenticator/PiggyCardsAuthenticator.kt`:
- Around line 54-55: In the reLogin() method (around lines 54-82), implement
token coalescing to prevent redundant logins during burst 401 errors. Before
calling tokenApi.login(), capture the Authorization header token from the failed
request and compare it against the currently cached token. If the tokens differ,
another waiting request has already refreshed the token, so reuse the cached
token instead of invoking tokenApi.login(). Only proceed with a new
tokenApi.login() call when the cached token still matches the token from the
failed request.
In
`@features/exploredash/src/test/java/org/dash/wallet/features/exploredash/PiggyCardsAuthenticatorTest.kt`:
- Line 55: The deleteKey method override in PiggyCardsAuthenticatorTest has an
empty function block body which triggers the detekt EmptyFunctionBlock warning.
Convert the method from block body syntax with empty braces to an expression
body using = Unit syntax, which maintains the no-op behavior while satisfying
the detekt rule.
🪄 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
Run ID: fde0c61e-358f-449d-8c87-e4d2fddfd2a7
📒 Files selected for processing (5)
features/exploredash/src/main/java/org/dash/wallet/features/exploredash/network/PiggyCardsRemoteDataSource.ktfeatures/exploredash/src/main/java/org/dash/wallet/features/exploredash/network/authenticator/PiggyCardsAuthenticator.ktfeatures/exploredash/src/main/java/org/dash/wallet/features/exploredash/repository/DashSpendRepositoryFactory.ktfeatures/exploredash/src/main/java/org/dash/wallet/features/exploredash/repository/PiggyCardsRepository.ktfeatures/exploredash/src/test/java/org/dash/wallet/features/exploredash/PiggyCardsAuthenticatorTest.kt
Comment on lines
+81
to
+83
| val response = tokenApi.login(LoginRequest(userId = userId, password = password)) | ||
| persistLogin(response.accessToken, response.expiresIn) | ||
| response.accessToken.takeIf { it.isNotBlank() } |
Contributor
There was a problem hiding this comment.
Validate the access token before persisting it.
persistLogin() runs even when response.accessToken is blank, so a malformed successful response can overwrite the cached token/expiry while reLogin() returns null. Persist only after the token passes the non-blank check.
Proposed fix
- val response = tokenApi.login(LoginRequest(userId = userId, password = password))
- persistLogin(response.accessToken, response.expiresIn)
- response.accessToken.takeIf { it.isNotBlank() }
+ val response = tokenApi.login(LoginRequest(userId = userId, password = password))
+ val accessToken = response.accessToken.takeIf { it.isNotBlank() } ?: return@withLock null
+ persistLogin(accessToken, response.expiresIn)
+ accessToken📝 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
| val response = tokenApi.login(LoginRequest(userId = userId, password = password)) | |
| persistLogin(response.accessToken, response.expiresIn) | |
| response.accessToken.takeIf { it.isNotBlank() } | |
| val response = tokenApi.login(LoginRequest(userId = userId, password = password)) | |
| val accessToken = response.accessToken.takeIf { it.isNotBlank() } ?: return@withLock null | |
| persistLogin(accessToken, response.expiresIn) | |
| accessToken |
🤖 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
`@features/exploredash/src/main/java/org/dash/wallet/features/exploredash/network/authenticator/PiggyCardsAuthenticator.kt`
around lines 81 - 83, The issue is that in the login method, persistLogin() is
being called before validating that response.accessToken is not blank, which
means invalid or blank tokens can overwrite cached credentials. Fix this by
reordering the logic so that the token is validated for being non-blank first,
and then persistLogin() is only called if the token passes that validation
check. The takeIf { it.isNotBlank() } check should be performed before the
persistLogin() call, not after it.
Comment on lines
+84
to
94
| } catch (e: Exception) { | ||
| log.error("Failed to refresh token: ${e.message}", e) | ||
| // Only a genuine credential rejection drops the cached token. The login endpoint | ||
| // returns HTTP 401 for that case, surfaced by ErrorHandlingInterceptor as a | ||
| // CTXSpendException; any other failure is transient, so the token is kept and the | ||
| // next request retries instead of forcing the user to sign in again. | ||
| if (e is CTXSpendException && e.errorCode == 401) { | ||
| config.setSecuredData(PiggyCardsConfig.PREFS_KEY_ACCESS_TOKEN, "") | ||
| } | ||
| null | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n features/exploredash/src/main/java/org/dash/wallet/features/exploredash/network/authenticator/PiggyCardsAuthenticator.kt | head -120Repository: dashpay/dash-wallet
Length of output: 5811
🏁 Script executed:
rg "CancellationException" features/exploredash/src/main/java/org/dash/wallet/features/exploredash/network/authenticator/PiggyCardsAuthenticator.ktRepository: dashpay/dash-wallet
Length of output: 45
🏁 Script executed:
rg "import.*CancellationException" features/exploredash/Repository: dashpay/dash-wallet
Length of output: 45
🏁 Script executed:
cd features/exploredash && find . -name "*.kt" | head -5 && cd - > /dev/nullRepository: dashpay/dash-wallet
Length of output: 470
🏁 Script executed:
find . -name ".editorconfig" -o -name "ktlint.yml" -o -name "ktlint.yaml" | head -10Repository: dashpay/dash-wallet
Length of output: 110
🏁 Script executed:
rg "CancellationException" --type kt | head -20Repository: dashpay/dash-wallet
Length of output: 90
🏁 Script executed:
rg "CancellationException" --type kotlin | head -20Repository: dashpay/dash-wallet
Length of output: 1572
🏁 Script executed:
rg "CancellationException" | head -20Repository: dashpay/dash-wallet
Length of output: 1572
🏁 Script executed:
cat wallet/src/de/schildbach/wallet/service/platform/PlatformSyncService.kt | grep -A 5 "catch (_: CancellationException)" | head -20Repository: dashpay/dash-wallet
Length of output: 541
🏁 Script executed:
cat .editorconfig | head -50Repository: dashpay/dash-wallet
Length of output: 569
Rethrow coroutine cancellation before generic failure handling.
The catch (e: Exception) block catches coroutine cancellation, silencing it and returning null instead of propagating the cancellation signal. This breaks structured concurrency semantics in a suspend function. Add an explicit CancellationException handler before the generic handler, following the pattern used elsewhere in the codebase.
Proposed fix
+import kotlinx.coroutines.CancellationException
+
...
- } catch (e: Exception) {
+ } catch (e: CancellationException) {
+ throw e
+ } catch (e: Exception) {
log.error("Failed to refresh token: ${e.message}", e)📝 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
| } catch (e: Exception) { | |
| log.error("Failed to refresh token: ${e.message}", e) | |
| // Only a genuine credential rejection drops the cached token. The login endpoint | |
| // returns HTTP 401 for that case, surfaced by ErrorHandlingInterceptor as a | |
| // CTXSpendException; any other failure is transient, so the token is kept and the | |
| // next request retries instead of forcing the user to sign in again. | |
| if (e is CTXSpendException && e.errorCode == 401) { | |
| config.setSecuredData(PiggyCardsConfig.PREFS_KEY_ACCESS_TOKEN, "") | |
| } | |
| null | |
| } | |
| } catch (e: CancellationException) { | |
| throw e | |
| } catch (e: Exception) { | |
| log.error("Failed to refresh token: ${e.message}", e) | |
| // Only a genuine credential rejection drops the cached token. The login endpoint | |
| // returns HTTP 401 for that case, surfaced by ErrorHandlingInterceptor as a | |
| // CTXSpendException; any other failure is transient, so the token is kept and the | |
| // next request retries instead of forcing the user to sign in again. | |
| if (e is CTXSpendException && e.errorCode == 401) { | |
| config.setSecuredData(PiggyCardsConfig.PREFS_KEY_ACCESS_TOKEN, "") | |
| } | |
| null | |
| } |
🤖 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
`@features/exploredash/src/main/java/org/dash/wallet/features/exploredash/network/authenticator/PiggyCardsAuthenticator.kt`
around lines 84 - 94, The generic catch block catching Exception in the token
refresh logic is intercepting CancellationException, which breaks structured
concurrency by silencing coroutine cancellation. Add an explicit catch handler
for CancellationException before the existing generic Exception catch block in
this try-catch structure, and rethrow the CancellationException immediately to
preserve proper coroutine cancellation semantics.
Comment on lines
+95
to
+98
| val threadA = thread { authenticatorA.authenticate(null, response) } | ||
| val threadB = thread { authenticatorB.authenticate(null, response) } | ||
| threadA.join() | ||
| threadB.join() |
Contributor
There was a problem hiding this comment.
Propagate failures from the worker threads.
Exceptions thrown inside thread { ... } do not fail this JUnit test; capture and rethrow them after join() so one broken authenticate() path cannot be hidden.
Proposed fix
import java.util.concurrent.atomic.AtomicInteger
+import java.util.concurrent.atomic.AtomicReference
...
val response = unauthorizedResponse()
+ val failure = AtomicReference<Throwable?>()
- val threadA = thread { authenticatorA.authenticate(null, response) }
- val threadB = thread { authenticatorB.authenticate(null, response) }
+ val threadA = thread {
+ runCatching { authenticatorA.authenticate(null, response) }
+ .exceptionOrNull()
+ ?.let(failure::set)
+ }
+ val threadB = thread {
+ runCatching { authenticatorB.authenticate(null, response) }
+ .exceptionOrNull()
+ ?.let(failure::set)
+ }
threadA.join()
threadB.join()
+ failure.get()?.let { throw it }📝 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
| val threadA = thread { authenticatorA.authenticate(null, response) } | |
| val threadB = thread { authenticatorB.authenticate(null, response) } | |
| threadA.join() | |
| threadB.join() | |
| val response = unauthorizedResponse() | |
| val failure = AtomicReference<Throwable?>() | |
| val threadA = thread { | |
| runCatching { authenticatorA.authenticate(null, response) } | |
| .exceptionOrNull() | |
| ?.let(failure::set) | |
| } | |
| val threadB = thread { | |
| runCatching { authenticatorB.authenticate(null, response) } | |
| .exceptionOrNull() | |
| ?.let(failure::set) | |
| } | |
| threadA.join() | |
| threadB.join() | |
| failure.get()?.let { throw it } |
🤖 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
`@features/exploredash/src/test/java/org/dash/wallet/features/exploredash/PiggyCardsAuthenticatorTest.kt`
around lines 95 - 98, The test creates threadA and threadB using thread {
authenticatorA.authenticate(...) } and thread { authenticatorB.authenticate(...)
} but any exceptions thrown within these threads are not captured or rethrown
after the join() calls, allowing failures to be silently ignored. Capture any
exceptions thrown by each thread (using try-catch or similar mechanism within
the thread creation), store them, and after both threadA.join() and
threadB.join() complete, rethrow any captured exceptions so that failures in
either authenticate() call will properly fail the test.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue being fixed or feature implemented
Related PR's and Dependencies
Screenshots / Videos
How Has This Been Tested?
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes