From 87e428776af22263234a72d57f502ced996351d4 Mon Sep 17 00:00:00 2001 From: Wolfgang Mathurin Date: Tue, 22 Sep 2026 15:09:58 -0700 Subject: [PATCH 1/2] @W-24269726: Reject incomplete Android DPoP credentials --- .../androidsdk/auth/dpop/DPoPKeyManager.kt | 11 +++++ .../auth/dpop/DPoPRequestDecorator.kt | 20 ++++++-- .../androidsdk/rest/ClientManager.java | 5 +- .../SalesforceSDKManagerClientManagerTest.kt | 22 +++++++++ .../auth/dpop/DPoPKeyManagerTest.kt | 12 +++++ .../auth/dpop/DPoPRequestDecoratorTest.kt | 39 +++++++++++++++ .../androidsdk/rest/ClientManagerTest.java | 44 +++++++++++++++++ .../rest/RestClientDPoPGateTests.kt | 47 ++++++++++++++++--- 8 files changed, 188 insertions(+), 12 deletions(-) diff --git a/libs/SalesforceSDK/src/com/salesforce/androidsdk/auth/dpop/DPoPKeyManager.kt b/libs/SalesforceSDK/src/com/salesforce/androidsdk/auth/dpop/DPoPKeyManager.kt index 1b17901360..35fd45936a 100644 --- a/libs/SalesforceSDK/src/com/salesforce/androidsdk/auth/dpop/DPoPKeyManager.kt +++ b/libs/SalesforceSDK/src/com/salesforce/androidsdk/auth/dpop/DPoPKeyManager.kt @@ -54,6 +54,17 @@ object DPoPKeyManager { fun isDPoPTokenType(tokenType: String?): Boolean = tokenType?.equals(DPOP_TOKEN_TYPE, ignoreCase = true) == true + /** + * Returns whether the persisted fields required to use [tokenType] are internally + * consistent. A DPoP-bound credential must identify the key pair used for its proof; + * Bearer and transitional credentials do not require that identifier. + */ + @JvmStatic + fun hasCompleteDPoPCredentials( + credentialsIdentifier: String?, + tokenType: String? + ): Boolean = !isDPoPTokenType(tokenType) || !credentialsIdentifier.isNullOrBlank() + private const val TAG = "DPoPKeyManager" fun generateOrLoadKeyPair(alias: String): KeyPair { diff --git a/libs/SalesforceSDK/src/com/salesforce/androidsdk/auth/dpop/DPoPRequestDecorator.kt b/libs/SalesforceSDK/src/com/salesforce/androidsdk/auth/dpop/DPoPRequestDecorator.kt index 683dd3b8a4..6789633a81 100644 --- a/libs/SalesforceSDK/src/com/salesforce/androidsdk/auth/dpop/DPoPRequestDecorator.kt +++ b/libs/SalesforceSDK/src/com/salesforce/androidsdk/auth/dpop/DPoPRequestDecorator.kt @@ -49,14 +49,16 @@ object DPoPRequestDecorator { /** * Stamps [Authorization] and, if the account is DPoP-bound, a [DPoP] proof header - * on [builder]. No-op when [UserAccount.getAuthToken] is null or empty. + * on [builder]. No-op when [UserAccount.getAuthToken] is null or empty. Rejects an + * incomplete DPoP credential before mutating [builder]. */ fun applyAuthHeaders(builder: Request.Builder, userAccount: UserAccount) { - val authToken = userAccount.authToken ?: return - if (authToken.isEmpty()) return - val tokenType = userAccount.tokenType val credentialsIdentifier = userAccount.credentialsIdentifier + requireCompleteDPoPCredentials(credentialsIdentifier, tokenType) + + val authToken = userAccount.authToken ?: return + if (authToken.isEmpty()) return OAuth2.addAuthorizationHeader(builder, authToken, tokenType) attachProof(builder, credentialsIdentifier, tokenType, authToken) @@ -100,6 +102,7 @@ object DPoPRequestDecorator { tokenType: String?, authToken: String? ) { + requireCompleteDPoPCredentials(credentialsIdentifier, tokenType) if (!DPoPKeyManager.shouldAttachDPoP(credentialsIdentifier, tokenType)) return try { val request = builder.build() @@ -116,4 +119,13 @@ object DPoPRequestDecorator { SalesforceSDKLogger.e(TAG, "Failed to attach DPoP proof", e) } } + + private fun requireCompleteDPoPCredentials( + credentialsIdentifier: String?, + tokenType: String? + ) { + check(DPoPKeyManager.hasCompleteDPoPCredentials(credentialsIdentifier, tokenType)) { + "DPoP credentials require a non-blank credentials identifier" + } + } } diff --git a/libs/SalesforceSDK/src/com/salesforce/androidsdk/rest/ClientManager.java b/libs/SalesforceSDK/src/com/salesforce/androidsdk/rest/ClientManager.java index dd6f5eac82..fe27cc9212 100644 --- a/libs/SalesforceSDK/src/com/salesforce/androidsdk/rest/ClientManager.java +++ b/libs/SalesforceSDK/src/com/salesforce/androidsdk/rest/ClientManager.java @@ -54,6 +54,7 @@ import com.salesforce.androidsdk.auth.OAuth2.OAuthFailedException; import com.salesforce.androidsdk.auth.OAuth2.TokenEndpointResponse; import com.salesforce.androidsdk.auth.OAuth2.TokenErrorResponse; +import com.salesforce.androidsdk.auth.dpop.DPoPKeyManager; import com.salesforce.androidsdk.rest.RestClient.ClientInfo; import com.salesforce.androidsdk.util.SalesforceSDKLogger; @@ -240,7 +241,9 @@ private UserAccount validateUser(boolean requireRefreshFields, @Nullable UserAcc } if (user == null || isMissing(user.getUserId()) - || isMissing(user.getOrgId())) { + || isMissing(user.getOrgId()) + || !DPoPKeyManager.hasCompleteDPoPCredentials( + user.getCredentialsIdentifier(), user.getTokenType())) { return null; } if (requireRefreshFields && (isMissing(user.getRefreshTokenForPersistence()) diff --git a/libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/app/SalesforceSDKManagerClientManagerTest.kt b/libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/app/SalesforceSDKManagerClientManagerTest.kt index 35d244d339..891c65c195 100644 --- a/libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/app/SalesforceSDKManagerClientManagerTest.kt +++ b/libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/app/SalesforceSDKManagerClientManagerTest.kt @@ -572,6 +572,28 @@ class SalesforceSDKManagerClientManagerTest { } } + @Test + fun getRestClient_withDPoPAccountMissingCredentialsIdentifier_removesItWithoutCallback() { + val corruptUser = UserAccountBuilder.getInstance() + .populateFromUserAccount(buildUser("corrupt-dpop")) + .tokenType("dPoP") + .allowUnset(true) + .credentialsIdentifier(null) + .build() + userAccountManager.createAccount(corruptUser) + val account = requireNotNull(userAccountManager.buildAccount(corruptUser)) + val activity = mockk(relaxed = true) + var callbackCount = 0 + + sdkManager.getRestClient(activity) { callbackCount++ } + + assertEquals(0, callbackCount) + assertFalse(accountManager.getAccountsByType(account.type).contains(account)) + verify(exactly = 0) { + activity.startActivityForResult(any(), any()) + } + } + @Test fun getRestClient_resolvesCurrentAccountExactlyOnce() { /* diff --git a/libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/auth/dpop/DPoPKeyManagerTest.kt b/libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/auth/dpop/DPoPKeyManagerTest.kt index 5210239605..7d4c163a31 100644 --- a/libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/auth/dpop/DPoPKeyManagerTest.kt +++ b/libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/auth/dpop/DPoPKeyManagerTest.kt @@ -177,6 +177,18 @@ class DPoPKeyManagerTest { assertFalse(DPoPKeyManager.isDPoPTokenType("bearer")) } + @Test + fun test_hasCompleteDPoPCredentials_requiresNonBlankIdentifierOnlyForDPoP() { + assertTrue(DPoPKeyManager.hasCompleteDPoPCredentials("credential-id", "DPoP")) + assertTrue(DPoPKeyManager.hasCompleteDPoPCredentials("credential-id", "dPoP")) + assertFalse(DPoPKeyManager.hasCompleteDPoPCredentials(null, "DPoP")) + assertFalse(DPoPKeyManager.hasCompleteDPoPCredentials("", "dpop")) + assertFalse(DPoPKeyManager.hasCompleteDPoPCredentials(" ", "DPOP")) + + assertTrue(DPoPKeyManager.hasCompleteDPoPCredentials(null, "Bearer")) + assertTrue(DPoPKeyManager.hasCompleteDPoPCredentials("", null)) + } + @Test fun test_shouldAttachDPoP_lowercaseDPoPTokenType_returnsTrue() { // Regression for W-24027018: server returns lowercase "dpop" in refresh responses. diff --git a/libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/auth/dpop/DPoPRequestDecoratorTest.kt b/libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/auth/dpop/DPoPRequestDecoratorTest.kt index aa7f1d9149..91342892fc 100644 --- a/libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/auth/dpop/DPoPRequestDecoratorTest.kt +++ b/libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/auth/dpop/DPoPRequestDecoratorTest.kt @@ -39,6 +39,7 @@ import org.junit.Assert.assertEquals import org.junit.Assert.assertFalse import org.junit.Assert.assertNotNull import org.junit.Assert.assertNull +import org.junit.Assert.assertThrows import org.junit.Assert.assertTrue import org.junit.Test import org.junit.runner.RunWith @@ -132,6 +133,44 @@ class DPoPRequestDecoratorTest { assertNull(request.header(DPoPRequestDecorator.DPOP_HEADER)) } + @Test + fun applyAuthHeaders_dpopAccountWithMissingIdentifier_failsBeforeAddingHeaders() { + listOf(null, "", " ").forEach { identifier -> + val builder = requestBuilder() + + assertThrows(IllegalStateException::class.java) { + DPoPRequestDecorator.applyAuthHeaders( + builder, + userAccount( + tokenType = "dPoP", + credentialsIdentifier = identifier, + ), + ) + } + + assertNull(builder.build().header("Authorization")) + assertNull(builder.build().header(DPoPRequestDecorator.DPOP_HEADER)) + } + } + + @Test + fun applyAuthHeaders_dpopAccountWithMissingToken_failsClosed() { + val builder = requestBuilder() + + assertThrows(IllegalStateException::class.java) { + DPoPRequestDecorator.applyAuthHeaders( + builder, + userAccount( + authToken = null, + tokenType = "DPoP", + credentialsIdentifier = null, + ), + ) + } + + assertNull(builder.build().header("Authorization")) + } + @Test fun applyAuthHeaders_dpopAccount_isUseDPoPFalse_proofStillAttached() { val sdkManager = com.salesforce.androidsdk.app.SalesforceSDKManager.getInstance() diff --git a/libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/rest/ClientManagerTest.java b/libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/rest/ClientManagerTest.java index a781111dc1..9412ded9e0 100644 --- a/libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/rest/ClientManagerTest.java +++ b/libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/rest/ClientManagerTest.java @@ -356,6 +356,50 @@ public void testPeekRestClientWithUserFailsClosedWhenSuppliedUserMissingUserId() Assert.assertNull(clientManager.peekRestClient(incompleteUser)); } + /** Persisted DPoP credentials without a key identifier are rejected as corrupt. */ + @Test + public void testPeekRestClientWithDPoPAndNullCredentialsIdentifierReturnsNull() { + final UserAccount corruptUser = UserAccountBuilder.getInstance() + .populateFromUserAccount(UserAccountTest.createTestAccount()) + .tokenType("DPoP") + .allowUnset(true) + .credentialsIdentifier(null) + .build(); + userAccountManager.createAccount(corruptUser); + clientManager = new ClientManager(targetContext, corruptUser); + + Assert.assertNull(clientManager.peekRestClient()); + } + + /** DPoP matching is case-insensitive and whitespace is not a usable key identifier. */ + @Test + public void testPeekRestClientWithMixedCaseDPoPAndBlankCredentialsIdentifierReturnsNull() { + final UserAccount corruptUser = UserAccountBuilder.getInstance() + .populateFromUserAccount(UserAccountTest.createTestAccount()) + .tokenType("dPoP") + .credentialsIdentifier(" ") + .build(); + userAccountManager.createAccount(corruptUser); + clientManager = new ClientManager(targetContext, corruptUser); + + Assert.assertNull(clientManager.peekRestClient()); + } + + /** Bearer accounts do not require a DPoP credentials identifier. */ + @Test + public void testPeekRestClientWithBearerAndNullCredentialsIdentifierReturnsClient() { + final UserAccount bearerUser = UserAccountBuilder.getInstance() + .populateFromUserAccount(UserAccountTest.createTestAccount()) + .tokenType("Bearer") + .allowUnset(true) + .credentialsIdentifier(null) + .build(); + userAccountManager.createAccount(bearerUser); + clientManager = new ClientManager(targetContext, bearerUser); + + Assert.assertNotNull(clientManager.peekRestClient()); + } + /** * Checks there are no test accounts */ diff --git a/libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/rest/RestClientDPoPGateTests.kt b/libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/rest/RestClientDPoPGateTests.kt index 38f68b5dd1..f92a6907d3 100644 --- a/libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/rest/RestClientDPoPGateTests.kt +++ b/libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/rest/RestClientDPoPGateTests.kt @@ -31,6 +31,7 @@ import com.salesforce.androidsdk.auth.dpop.DPoPKeyManager import io.mockk.every import io.mockk.mockk import io.mockk.slot +import io.mockk.verify import okhttp3.Interceptor import okhttp3.Protocol import okhttp3.Request @@ -39,6 +40,7 @@ import org.junit.After import org.junit.Assert.assertFalse import org.junit.Assert.assertNotNull import org.junit.Assert.assertNull +import org.junit.Assert.assertThrows import org.junit.Assert.assertTrue import org.junit.Test import org.junit.runner.RunWith @@ -166,22 +168,53 @@ class RestClientDPoPGateTests { ) } - // Null credentialsIdentifier short-circuits — no DPoP header regardless of tokenType. + // An explicit DPoP token type without its key identifier is corrupt. Fail before the + // OkHttp chain can transmit an Authorization header without a proof. @Test - fun test_givenNullCredentialsIdentifier_whenIntercept_thenNoDPoPHeader() { + fun test_givenDPoPWithNullCredentialsIdentifier_whenIntercept_thenFailsBeforeNetwork() { val interceptor = RestClient.OAuthRefreshInterceptor( null, "__ACCESS_TOKEN__", - "DPoP", + "dPoP", null, null, ) + val outbound = Request.Builder() + .url("https://instance.example.com/services/data/v65.0/query") + .get() + .build() + val chain = mockk(relaxed = true) { + every { request() } returns outbound + } - val captured = captureAuthenticatedRequest(interceptor) + assertThrows(IllegalStateException::class.java) { + interceptor.intercept(chain) + } - assertNull( - "Did not expect DPoP header when credentialsIdentifier is null", - captured.header("DPoP"), + verify(exactly = 0) { chain.proceed(any()) } + } + + @Test + fun test_givenDPoPWithBlankCredentialsIdentifier_whenIntercept_thenFailsBeforeNetwork() { + val interceptor = RestClient.OAuthRefreshInterceptor( + null, + "__ACCESS_TOKEN__", + "DPoP", + " ", + null, ) + val outbound = Request.Builder() + .url("https://instance.example.com/services/data/v65.0/query") + .get() + .build() + val chain = mockk(relaxed = true) { + every { request() } returns outbound + } + + assertThrows(IllegalStateException::class.java) { + interceptor.intercept(chain) + } + + verify(exactly = 0) { chain.proceed(any()) } } } From ea75f47fcb9babcbd5222c08736908fbf7ff24bb Mon Sep 17 00:00:00 2001 From: Wolfgang Mathurin Date: Tue, 22 Sep 2026 20:38:39 -0700 Subject: [PATCH 2/2] @W-24269726: Report incomplete DPoP credentials as I/O failures --- .../auth/dpop/DPoPRequestDecorator.kt | 14 +++-- .../auth/dpop/DPoPRequestDecoratorTest.kt | 5 +- .../rest/RestClientDPoPGateTests.kt | 61 ++++++++++++++++++- 3 files changed, 72 insertions(+), 8 deletions(-) diff --git a/libs/SalesforceSDK/src/com/salesforce/androidsdk/auth/dpop/DPoPRequestDecorator.kt b/libs/SalesforceSDK/src/com/salesforce/androidsdk/auth/dpop/DPoPRequestDecorator.kt index 6789633a81..a1804df293 100644 --- a/libs/SalesforceSDK/src/com/salesforce/androidsdk/auth/dpop/DPoPRequestDecorator.kt +++ b/libs/SalesforceSDK/src/com/salesforce/androidsdk/auth/dpop/DPoPRequestDecorator.kt @@ -31,6 +31,7 @@ import com.salesforce.androidsdk.auth.OAuth2 import com.salesforce.androidsdk.util.SalesforceSDKLogger import okhttp3.Request import okhttp3.Response +import java.io.IOException /** * Public convenience API for app developers stamping DPoP/Bearer authorization @@ -50,7 +51,7 @@ object DPoPRequestDecorator { /** * Stamps [Authorization] and, if the account is DPoP-bound, a [DPoP] proof header * on [builder]. No-op when [UserAccount.getAuthToken] is null or empty. Rejects an - * incomplete DPoP credential before mutating [builder]. + * incomplete DPoP credential with [IOException] before mutating [builder]. */ fun applyAuthHeaders(builder: Request.Builder, userAccount: UserAccount) { val tokenType = userAccount.tokenType @@ -93,7 +94,8 @@ object DPoPRequestDecorator { /** * Package-private helper used by RestClient.OAuthRefreshInterceptor to delegate - * the proof-building step without constructing a UserAccount. + * the proof-building step without constructing a UserAccount. An incomplete DPoP + * credential throws [IOException] so OkHttp reports it through normal request failure. */ @JvmName("attachProof") internal fun attachProof( @@ -124,8 +126,12 @@ object DPoPRequestDecorator { credentialsIdentifier: String?, tokenType: String? ) { - check(DPoPKeyManager.hasCompleteDPoPCredentials(credentialsIdentifier, tokenType)) { - "DPoP credentials require a non-blank credentials identifier" + if (!DPoPKeyManager.hasCompleteDPoPCredentials(credentialsIdentifier, tokenType)) { + throw IncompleteDPoPCredentialsException() } } + + private class IncompleteDPoPCredentialsException : IOException( + "DPoP credentials require a non-blank credentials identifier" + ) } diff --git a/libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/auth/dpop/DPoPRequestDecoratorTest.kt b/libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/auth/dpop/DPoPRequestDecoratorTest.kt index 91342892fc..d47a396609 100644 --- a/libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/auth/dpop/DPoPRequestDecoratorTest.kt +++ b/libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/auth/dpop/DPoPRequestDecoratorTest.kt @@ -43,6 +43,7 @@ import org.junit.Assert.assertThrows import org.junit.Assert.assertTrue import org.junit.Test import org.junit.runner.RunWith +import java.io.IOException /** * Unit tests for [DPoPRequestDecorator], the public convenience API for stamping @@ -138,7 +139,7 @@ class DPoPRequestDecoratorTest { listOf(null, "", " ").forEach { identifier -> val builder = requestBuilder() - assertThrows(IllegalStateException::class.java) { + assertThrows(IOException::class.java) { DPoPRequestDecorator.applyAuthHeaders( builder, userAccount( @@ -157,7 +158,7 @@ class DPoPRequestDecoratorTest { fun applyAuthHeaders_dpopAccountWithMissingToken_failsClosed() { val builder = requestBuilder() - assertThrows(IllegalStateException::class.java) { + assertThrows(IOException::class.java) { DPoPRequestDecorator.applyAuthHeaders( builder, userAccount( diff --git a/libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/rest/RestClientDPoPGateTests.kt b/libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/rest/RestClientDPoPGateTests.kt index f92a6907d3..6cac1d1afd 100644 --- a/libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/rest/RestClientDPoPGateTests.kt +++ b/libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/rest/RestClientDPoPGateTests.kt @@ -44,6 +44,8 @@ import org.junit.Assert.assertThrows import org.junit.Assert.assertTrue import org.junit.Test import org.junit.runner.RunWith +import java.io.IOException +import java.net.URI import java.util.UUID /** @@ -187,7 +189,7 @@ class RestClientDPoPGateTests { every { request() } returns outbound } - assertThrows(IllegalStateException::class.java) { + assertThrows(IOException::class.java) { interceptor.intercept(chain) } @@ -211,10 +213,65 @@ class RestClientDPoPGateTests { every { request() } returns outbound } - assertThrows(IllegalStateException::class.java) { + assertThrows(IOException::class.java) { interceptor.intercept(chain) } verify(exactly = 0) { chain.proceed(any()) } } + + @Test + fun test_givenCachedBearerInterceptorMigratesToIncompleteDPoP_whenReplay_thenReportsIoFailure() { + val provider = object : RestClient.AuthTokenProvider { + override fun getInstanceUrl() = "https://instance.example.com" + override fun getNewAuthToken() = "__DPOP_ACCESS_TOKEN__" + override fun getRefreshToken() = "__REFRESH_TOKEN__" + override fun getLastRefreshTime() = 0L + override fun getTokenType() = "DPoP" + } + val interceptor = RestClient.OAuthRefreshInterceptor( + clientInfo(), + "__BEARER_ACCESS_TOKEN__", + "Bearer", + null, + provider, + ) + val outbound = Request.Builder() + .url("https://instance.example.com/services/data/v65.0/query") + .get() + .build() + val attempts = mutableListOf() + val chain = mockk { + every { request() } returns outbound + every { proceed(any()) } answers { + val attemptedRequest = firstArg() + attempts += attemptedRequest + Response.Builder() + .request(attemptedRequest) + .protocol(Protocol.HTTP_1_1) + .code(401) + .message("Unauthorized") + .build() + } + } + + assertThrows(IOException::class.java) { + interceptor.intercept(chain) + } + + assertTrue(attempts.single().header("Authorization")!!.startsWith("Bearer ")) + verify(exactly = 1) { chain.proceed(any()) } + } + + private fun clientInfo() = RestClient.ClientInfo( + URI.create("https://instance.example.com"), + URI.create("https://login.example.com"), + URI.create("https://login.example.com/id/orgId/userId"), + "account", + "user@example.com", + "userId", + "orgId", + null, null, null, null, null, null, null, null, null, + null, null, null, null, null, null, null, + ) }