From aea1b88000905280f1a83e71dfcfdeef8cfb775d Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Tue, 16 Jun 2026 16:13:39 +0300 Subject: [PATCH 1/3] feat: evict bearer token on a 401 challenge BearerTokenAuthStep only refreshed its cached token on the local expiry margin. When a server revokes a token before its advertised expiry, the step kept stamping the rejected token until the margin elapsed, so every request in that window failed with a 401. Override authorizeRequestOnChallenge so a 401 carrying WWW-Authenticate evicts the rejected token and re-stamps the request with a freshly fetched one, driving AuthStep's existing single retry with a new credential. Using the in-step hook (rather than throwing a retryable exception) means the retry is not gated by request idempotency, so a non-idempotent POST is re-authenticated too. Eviction is scoped to the token that actually produced the 401 (matched against the request's Authorization header) and runs under the same lock as the refresh path, so a token a concurrent request already refreshed is preserved rather than clobbered. This is an additional eviction trigger layered on top of the expiry-margin check, which is unchanged. --- sdk-core/api/sdk-core.api | 1 + .../pipeline/steps/BearerTokenAuthStep.kt | 53 ++++++- .../core/http/pipeline/steps/AuthStepTest.kt | 7 +- .../pipeline/steps/BearerTokenAuthStepTest.kt | 129 ++++++++++++++++++ 4 files changed, 185 insertions(+), 5 deletions(-) diff --git a/sdk-core/api/sdk-core.api b/sdk-core/api/sdk-core.api index de3a5d14..ddd7cf00 100644 --- a/sdk-core/api/sdk-core.api +++ b/sdk-core/api/sdk-core.api @@ -755,6 +755,7 @@ public class org/dexpace/sdk/core/http/pipeline/steps/BearerTokenAuthStep : org/ public fun (Lorg/dexpace/sdk/core/http/auth/BearerTokenProvider;Ljava/util/List;Ljava/time/Duration;Lorg/dexpace/sdk/core/util/Clock;)V public synthetic fun (Lorg/dexpace/sdk/core/http/auth/BearerTokenProvider;Ljava/util/List;Ljava/time/Duration;Lorg/dexpace/sdk/core/util/Clock;ILkotlin/jvm/internal/DefaultConstructorMarker;)V protected fun authorizeRequest (Lorg/dexpace/sdk/core/http/request/Request;)Lorg/dexpace/sdk/core/http/request/Request; + protected fun authorizeRequestOnChallenge (Lorg/dexpace/sdk/core/http/request/Request;Lorg/dexpace/sdk/core/http/response/Response;)Lorg/dexpace/sdk/core/http/request/Request; } public final class org/dexpace/sdk/core/http/pipeline/steps/DefaultAsyncInstrumentationStep : org/dexpace/sdk/core/http/pipeline/AsyncHttpStep { diff --git a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/pipeline/steps/BearerTokenAuthStep.kt b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/pipeline/steps/BearerTokenAuthStep.kt index f514f200..23c8e64f 100644 --- a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/pipeline/steps/BearerTokenAuthStep.kt +++ b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/pipeline/steps/BearerTokenAuthStep.kt @@ -11,6 +11,7 @@ import org.dexpace.sdk.core.http.auth.BearerToken import org.dexpace.sdk.core.http.auth.BearerTokenProvider import org.dexpace.sdk.core.http.common.HttpHeaderName import org.dexpace.sdk.core.http.request.Request +import org.dexpace.sdk.core.http.response.Response import org.dexpace.sdk.core.util.Clock import java.time.Duration import java.util.concurrent.locks.ReentrantLock @@ -37,6 +38,19 @@ import kotlin.concurrent.withLock * to decide when to refresh pre-emptively (default 30 seconds). A token whose `expiresAt` * is `now + 29s` with the default margin is considered expired and is refreshed. * + * ## Eviction on 401 + * + * The refresh margin only covers *local* expiry. A server may revoke a token before its + * advertised expiry, and the cached token would otherwise keep being stamped until the + * margin elapsed. To handle this, [authorizeRequestOnChallenge] evicts the rejected token + * on a 401 + `WWW-Authenticate` response and re-stamps the request with a freshly fetched + * one, so the single retry driven by [AuthStep] carries a new credential. Eviction is an + * *additional* trigger layered on top of the margin check — it does not change pre-emptive + * refresh. Only the token that produced the 401 is evicted; a token a concurrent request + * already refreshed in the meantime is left in place. The retry runs regardless of HTTP + * method (the in-step hook is not gated by retry-safety), so a non-idempotent POST is + * re-authenticated too. + * * ## Errors from [provider] * * - Throws → propagated. The exception is **not** cached, so a subsequent request @@ -46,8 +60,9 @@ import kotlin.concurrent.withLock * * ## Open for subclassing * - * Users wanting to override [authorizeRequestOnChallenge] (e.g. force a token refresh on - * 401) can extend this class. + * Eviction-on-401 is built in. Users wanting to customise challenge handling further (e.g. + * inspect the `WWW-Authenticate` scheme before deciding to refresh) can override + * [authorizeRequestOnChallenge]. * * Thread-safety: see Caching above. * Cancellation: token fetch may block; the [provider] is expected to respect interrupts. @@ -72,6 +87,40 @@ public open class BearerTokenAuthStep .build() } + /** + * On a 401 challenge, evict the token that was just rejected and re-stamp [request] + * with a freshly fetched one so [AuthStep]'s single retry carries a new credential. + * + * Eviction is scoped to the exact token that produced the 401 (matched against the + * `Authorization` header [request] carried): if a concurrent request already refreshed + * the cache, that newer token is preserved and reused for the retry rather than being + * discarded. The subsequent [currentToken] call then re-fetches only when the cache is + * actually empty, so a 401 storm across threads still funnels through the same + * double-checked-locking fetch as the expiry-margin path. + */ + override fun authorizeRequestOnChallenge( + request: Request, + response: Response, + ): Request { + evictRejectedToken(request.headers.get(HttpHeaderName.AUTHORIZATION)) + return authorizeRequest(request) + } + + /** + * Clears [cachedToken] iff it is still the token whose stamped header is [rejectedHeader]. + * Guarded by the same [lock] as the refresh path so the read-compare-clear is atomic + * against a concurrent refresh. + */ + private fun evictRejectedToken(rejectedHeader: String?) { + if (rejectedHeader == null) return + lock.withLock { + val current = cachedToken ?: return + if ("Bearer ${current.token}" == rejectedHeader) { + cachedToken = null + } + } + } + private fun currentToken(): BearerToken { // Fast path: lock-free volatile read; return the cached token if it's still valid. cachedToken?.takeIf { !it.isExpiredAt(clock.now(), refreshMargin) }?.let { return it } diff --git a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/pipeline/steps/AuthStepTest.kt b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/pipeline/steps/AuthStepTest.kt index 44443f59..b2fa28da 100644 --- a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/pipeline/steps/AuthStepTest.kt +++ b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/pipeline/steps/AuthStepTest.kt @@ -519,15 +519,16 @@ class AuthStepTest { // ----------------- 401 + WWW-Authenticate handling ----------------- @Test - fun `401 with WWW-Authenticate returns the 401 unchanged by default`() { - val provider = BearerTokenProvider { _, _ -> BearerToken("tk", null) } + fun `401 with WWW-Authenticate returns the 401 unchanged when the step does not override the challenge hook`() { + // KeyCredentialAuthStep does not override authorizeRequestOnChallenge, so it exercises + // the AuthStep base default (returns null → no retry). The 401 surfaces unchanged. val fake = FakeHttpClient() .enqueue { status(401).header("WWW-Authenticate", "Bearer realm=\"x\"") } val pipeline = HttpPipelineBuilder(fake) - .append(BearerTokenAuthStep(provider, listOf("scope"))) + .append(KeyCredentialAuthStep(KeyCredential("k"))) .build() val response = pipeline.send(getHttpsRequest()) diff --git a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/pipeline/steps/BearerTokenAuthStepTest.kt b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/pipeline/steps/BearerTokenAuthStepTest.kt index 8c07139d..cce0ed1d 100644 --- a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/pipeline/steps/BearerTokenAuthStepTest.kt +++ b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/pipeline/steps/BearerTokenAuthStepTest.kt @@ -13,10 +13,15 @@ import org.dexpace.sdk.core.http.common.HttpHeaderName import org.dexpace.sdk.core.http.pipeline.HttpPipelineBuilder import org.dexpace.sdk.core.http.request.Method import org.dexpace.sdk.core.http.request.Request +import org.dexpace.sdk.core.http.request.RequestBody +import org.dexpace.sdk.core.io.Io import org.dexpace.sdk.core.testing.FakeHttpClient import org.dexpace.sdk.core.testing.FixedClock +import org.dexpace.sdk.io.OkioIoProvider import java.time.Duration import java.time.Instant +import java.util.concurrent.atomic.AtomicInteger +import kotlin.test.BeforeTest import kotlin.test.Test import kotlin.test.assertEquals import kotlin.test.assertFailsWith @@ -28,6 +33,13 @@ class BearerTokenAuthStepTest { private val nowInstant: Instant = Instant.parse("2026-01-01T12:00:00Z") private val clock = FixedClock(nowInstant) + @BeforeTest + fun setUp() { + // The 401-challenge retry path closes the 401 response body; tests that exercise it + // need an installed IoProvider so body materialization succeeds. + Io.installProvider(OkioIoProvider) + } + // ---- B14: fresh-token validation does NOT apply refreshMargin ---- /** @@ -172,6 +184,115 @@ class BearerTokenAuthStepTest { assertEquals(2, calls, "provider must have been called again after prior exception") } + // ---- Evict-on-401 ---- + + @Test + fun `401 challenge evicts the cached token and the retry carries a freshly fetched one`() { + // A non-expiring token would normally be cached forever (no expiry margin ever fires). + // The server rejects the first attempt with a 401 + WWW-Authenticate; the step must + // evict the stale token and re-fetch so the single retry carries the fresh credential. + val fetches = AtomicInteger(0) + val provider = + BearerTokenProvider { _, _ -> + BearerToken("token-${fetches.incrementAndGet()}", futureExpiry) + } + val fake = + FakeHttpClient() + .enqueue { status(401).header("WWW-Authenticate", "Bearer realm=\"x\"") } + .enqueue { status(200) } + val pipeline = + HttpPipelineBuilder(fake) + .append(BearerTokenAuthStep(provider, listOf("scope"), clock = clock)) + .build() + + val response = pipeline.send(getRequest()) + + assertEquals(200, response.status.code) + assertEquals(2, fake.callCount, "exactly one retry after the 401") + assertEquals(2, fetches.get(), "the 401 must trigger a re-fetch (eviction), not reuse the cached token") + // First attempt carries the stale token; the retry carries the freshly fetched one. + assertEquals("Bearer token-1", fake.requests[0].headers.get(HttpHeaderName.AUTHORIZATION)) + assertEquals("Bearer token-2", fake.requests[1].headers.get(HttpHeaderName.AUTHORIZATION)) + } + + @Test + fun `401 eviction works for a non-idempotent POST`() { + // The in-step challenge hook is NOT gated by retry-safety, so a POST (non-idempotent) + // still re-authenticates after a 401. A replayable body lets the retry re-send. + val fetches = AtomicInteger(0) + val provider = + BearerTokenProvider { _, _ -> + BearerToken("token-${fetches.incrementAndGet()}", futureExpiry) + } + val fake = + FakeHttpClient() + .enqueue { status(401).header("WWW-Authenticate", "Bearer realm=\"x\"") } + .enqueue { status(200) } + val pipeline = + HttpPipelineBuilder(fake) + .append(BearerTokenAuthStep(provider, listOf("scope"), clock = clock)) + .build() + + val response = pipeline.send(postRequest()) + + assertEquals(200, response.status.code) + assertEquals(2, fake.callCount, "the POST must be retried once after re-authentication") + assertEquals(2, fetches.get()) + assertEquals("Bearer token-2", fake.requests[1].headers.get(HttpHeaderName.AUTHORIZATION)) + } + + @Test + fun `after a 401 eviction the next independent request still sees the refreshed token cached`() { + // The retry re-fetches and re-caches; a subsequent unrelated request must reuse that + // refreshed token rather than fetching a third time. + val fetches = AtomicInteger(0) + val provider = + BearerTokenProvider { _, _ -> + BearerToken("token-${fetches.incrementAndGet()}", futureExpiry) + } + val fake = + FakeHttpClient() + .enqueue { status(401).header("WWW-Authenticate", "Bearer realm=\"x\"") } + .enqueue { status(200) } + .enqueue { status(200) } + val pipeline = + HttpPipelineBuilder(fake) + .append(BearerTokenAuthStep(provider, listOf("scope"), clock = clock)) + .build() + + pipeline.send(getRequest()) // 401 → evict → retry with token-2 + pipeline.send(getRequest()) // reuses cached token-2 + + assertEquals(2, fetches.get(), "the refreshed token is cached; no third fetch") + assertEquals("Bearer token-2", fake.requests[2].headers.get(HttpHeaderName.AUTHORIZATION)) + } + + @Test + fun `repeated 401s surface the second 401 without a second retry`() { + // The base AuthStep retries exactly once. If the freshly fetched token is ALSO rejected, + // the second 401 is surfaced as the operation result (no infinite refresh loop), and the + // provider is consulted once per attempt (twice total). + val fetches = AtomicInteger(0) + val provider = + BearerTokenProvider { _, _ -> + BearerToken("token-${fetches.incrementAndGet()}", futureExpiry) + } + val fake = + FakeHttpClient() + .enqueue { status(401).header("WWW-Authenticate", "Bearer realm=\"x\"") } + .enqueue { status(401).header("WWW-Authenticate", "Bearer realm=\"x\"") } + val pipeline = + HttpPipelineBuilder(fake) + .append(BearerTokenAuthStep(provider, listOf("scope"), clock = clock)) + .build() + + val response = pipeline.send(getRequest()) + + assertEquals(401, response.status.code, "the second 401 is surfaced, not retried again") + assertEquals(2, fake.callCount) + assertEquals(2, fetches.get()) + } + // ---- Helpers ---- private fun getRequest(): Request = @@ -179,4 +300,12 @@ class BearerTokenAuthStepTest { .method(Method.GET) .url("https://api.example.com/resource") .build() + + private fun postRequest(): Request = + Request.builder() + .method(Method.POST) + .url("https://api.example.com/resource") + // RequestBody.create(String) is replayable, so the non-idempotent retry can re-send. + .body(RequestBody.create("payload")) + .build() } From 0ff8cc839c71597a7da38c78e1616d7db6d1c3e4 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Tue, 16 Jun 2026 22:29:28 +0300 Subject: [PATCH 2/3] fix: don't re-stamp bearer token on a cross-origin-redirect 401 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A cross-origin redirect re-issue reaches the AUTH stage stripped of its Authorization header and tagged with the internal cross-origin marker, so the stage forwards it credential-free. If the foreign host answered that request with a 401 + WWW-Authenticate, the challenge hook unconditionally re-stamped Authorization: Bearer onto the server-chosen host and re-drove it through the chain — leaking the caller's token cross-origin and bypassing the HTTPS guard, which only the first pass through process() enforces. The challenge hook now returns null (surfacing the 401 unchanged) when the rejected request carried no Authorization header, since that only happens when the AUTH stage deliberately suppressed stamping. Route the bearer header value through a single bearerHeaderValue() helper shared by stamping and the eviction match so the two cannot drift, and document that a subclass customizing the header format must override it. Add a test for a cross-origin-marked request that 401s. --- sdk-core/api/sdk-core.api | 1 + .../pipeline/steps/BearerTokenAuthStep.kt | 47 +++++++++++++++---- .../pipeline/steps/BearerTokenAuthStepTest.kt | 38 +++++++++++++++ 3 files changed, 76 insertions(+), 10 deletions(-) diff --git a/sdk-core/api/sdk-core.api b/sdk-core/api/sdk-core.api index ddd7cf00..7a199045 100644 --- a/sdk-core/api/sdk-core.api +++ b/sdk-core/api/sdk-core.api @@ -756,6 +756,7 @@ public class org/dexpace/sdk/core/http/pipeline/steps/BearerTokenAuthStep : org/ public synthetic fun (Lorg/dexpace/sdk/core/http/auth/BearerTokenProvider;Ljava/util/List;Ljava/time/Duration;Lorg/dexpace/sdk/core/util/Clock;ILkotlin/jvm/internal/DefaultConstructorMarker;)V protected fun authorizeRequest (Lorg/dexpace/sdk/core/http/request/Request;)Lorg/dexpace/sdk/core/http/request/Request; protected fun authorizeRequestOnChallenge (Lorg/dexpace/sdk/core/http/request/Request;Lorg/dexpace/sdk/core/http/response/Response;)Lorg/dexpace/sdk/core/http/request/Request; + protected fun bearerHeaderValue (Ljava/lang/String;)Ljava/lang/String; } public final class org/dexpace/sdk/core/http/pipeline/steps/DefaultAsyncInstrumentationStep : org/dexpace/sdk/core/http/pipeline/AsyncHttpStep { diff --git a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/pipeline/steps/BearerTokenAuthStep.kt b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/pipeline/steps/BearerTokenAuthStep.kt index 23c8e64f..7258c59e 100644 --- a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/pipeline/steps/BearerTokenAuthStep.kt +++ b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/pipeline/steps/BearerTokenAuthStep.kt @@ -51,6 +51,11 @@ import kotlin.concurrent.withLock * method (the in-step hook is not gated by retry-safety), so a non-idempotent POST is * re-authenticated too. * + * A request that reaches the challenge hook carrying **no** `Authorization` header is a + * cross-origin redirect re-issue whose credential the AUTH stage deliberately suppressed + * (see [CrossOriginRedirectMarker]). In that case the hook re-stamps nothing and surfaces the + * 401 unchanged, so the caller's token is never attached to a server-chosen foreign host. + * * ## Errors from [provider] * * - Throws → propagated. The exception is **not** cached, so a subsequent request @@ -62,7 +67,9 @@ import kotlin.concurrent.withLock * * Eviction-on-401 is built in. Users wanting to customise challenge handling further (e.g. * inspect the `WWW-Authenticate` scheme before deciding to refresh) can override - * [authorizeRequestOnChallenge]. + * [authorizeRequestOnChallenge]. A subclass that changes the stamped header format by + * overriding [authorizeRequest] must also override [bearerHeaderValue] so the eviction match + * keeps recognising the rejected token. * * Thread-safety: see Caching above. * Cancellation: token fetch may block; the [provider] is expected to respect interrupts. @@ -83,7 +90,7 @@ public open class BearerTokenAuthStep override fun authorizeRequest(request: Request): Request { val token = currentToken() return request.newBuilder() - .setHeader(HttpHeaderName.AUTHORIZATION.caseSensitiveName, "Bearer ${token.token}") + .setHeader(HttpHeaderName.AUTHORIZATION.caseSensitiveName, bearerHeaderValue(token.token)) .build() } @@ -91,8 +98,16 @@ public open class BearerTokenAuthStep * On a 401 challenge, evict the token that was just rejected and re-stamp [request] * with a freshly fetched one so [AuthStep]'s single retry carries a new credential. * - * Eviction is scoped to the exact token that produced the 401 (matched against the - * `Authorization` header [request] carried): if a concurrent request already refreshed + * Returns `null` — surfacing the 401 unchanged with no retry — when [request] carried no + * `Authorization` header. A request reaches this hook credential-free only when the AUTH + * stage deliberately suppressed stamping, i.e. a cross-origin redirect re-issue (see + * [CrossOriginRedirectMarker]). Re-stamping there would attach the caller's bearer token + * to a server-chosen foreign host and re-drive it through the chain, leaking the token + * cross-origin and bypassing the HTTPS guard that only [AuthStep.process]'s first pass + * enforces. Refusing to re-stamp preserves that suppression. + * + * Otherwise eviction is scoped to the exact token that produced the 401 (matched against + * the `Authorization` header [request] carried): if a concurrent request already refreshed * the cache, that newer token is preserved and reused for the retry rather than being * discarded. The subsequent [currentToken] call then re-fetches only when the cache is * actually empty, so a 401 storm across threads still funnels through the same @@ -101,26 +116,38 @@ public open class BearerTokenAuthStep override fun authorizeRequestOnChallenge( request: Request, response: Response, - ): Request { - evictRejectedToken(request.headers.get(HttpHeaderName.AUTHORIZATION)) + ): Request? { + // No credential on the rejected request means stamping was suppressed (cross-origin + // redirect). Do not re-attach one: surface the 401 unchanged. + val rejectedHeader = request.headers.get(HttpHeaderName.AUTHORIZATION) ?: return null + evictRejectedToken(rejectedHeader) return authorizeRequest(request) } /** * Clears [cachedToken] iff it is still the token whose stamped header is [rejectedHeader]. * Guarded by the same [lock] as the refresh path so the read-compare-clear is atomic - * against a concurrent refresh. + * against a concurrent refresh. Routes the comparison through [bearerHeaderValue] so it + * stays in lock-step with the value [authorizeRequest] stamps. */ - private fun evictRejectedToken(rejectedHeader: String?) { - if (rejectedHeader == null) return + private fun evictRejectedToken(rejectedHeader: String) { lock.withLock { val current = cachedToken ?: return - if ("Bearer ${current.token}" == rejectedHeader) { + if (bearerHeaderValue(current.token) == rejectedHeader) { cachedToken = null } } } + /** + * The `Authorization` header value for [token]. Single source of truth shared by + * [authorizeRequest] (stamping) and [evictRejectedToken] (the eviction match), so the two + * never drift. A subclass that overrides [authorizeRequest] to emit a different header + * format must override this too, or eviction will stop matching and a rejected token will + * be re-fetched but never cleared. + */ + protected open fun bearerHeaderValue(token: String): String = "Bearer $token" + private fun currentToken(): BearerToken { // Fast path: lock-free volatile read; return the cached token if it's still valid. cachedToken?.takeIf { !it.isExpiredAt(clock.now(), refreshMargin) }?.let { return it } diff --git a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/pipeline/steps/BearerTokenAuthStepTest.kt b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/pipeline/steps/BearerTokenAuthStepTest.kt index cce0ed1d..91ae824c 100644 --- a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/pipeline/steps/BearerTokenAuthStepTest.kt +++ b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/pipeline/steps/BearerTokenAuthStepTest.kt @@ -26,6 +26,7 @@ import kotlin.test.Test import kotlin.test.assertEquals import kotlin.test.assertFailsWith import kotlin.test.assertNotNull +import kotlin.test.assertNull import kotlin.test.assertTrue class BearerTokenAuthStepTest { @@ -293,6 +294,43 @@ class BearerTokenAuthStepTest { assertEquals(2, fetches.get()) } + @Test + fun `cross-origin-marked request that 401s is surfaced unchanged without stamping a token`() { + // A cross-origin redirect re-issue is stripped of its Authorization header and tagged + // with the internal marker, so it reaches the foreign host with no credential. If that + // host 401s, the challenge path must NOT stamp the caller's bearer token onto the + // foreign host (that would leak the token cross-origin and bypass the HTTPS/cross-origin + // suppression). The 401 must surface unchanged with no token fetch. + val fetches = AtomicInteger(0) + val provider = + BearerTokenProvider { _, _ -> + BearerToken("token-${fetches.incrementAndGet()}", futureExpiry) + } + val fake = + FakeHttpClient() + .enqueue { status(401).header("WWW-Authenticate", "Bearer realm=\"x\"") } + val pipeline = + HttpPipelineBuilder(fake) + .append(BearerTokenAuthStep(provider, listOf("scope"), clock = clock)) + .build() + + // Mimic DefaultRedirectStep's cross-origin re-issue: no Authorization, marker present. + val markedRequest = + Request.builder() + .method(Method.GET) + .url("https://foreign.example.org/resource") + .addHeader(CrossOriginRedirectMarker.MARKER_HEADER, CrossOriginRedirectMarker.MARKER_VALUE) + .build() + + val response = pipeline.send(markedRequest) + + assertEquals(401, response.status.code, "the cross-origin 401 must surface unchanged") + assertEquals(1, fake.callCount, "no retry: the credential-free request must not be re-stamped") + assertEquals(0, fetches.get(), "no token must be fetched for a credential-free cross-origin request") + // The (single) request that reached the foreign host carried no Authorization header. + assertNull(fake.requests.single().headers.get(HttpHeaderName.AUTHORIZATION)) + } + // ---- Helpers ---- private fun getRequest(): Request = From 62d2d8a85a718964e0e11bc9bd04f63e7fe2a00e Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Wed, 17 Jun 2026 00:03:21 +0300 Subject: [PATCH 3/3] feat: gate bearer eviction on a Bearer WWW-Authenticate challenge Only evict and refetch the cached token when the 401's WWW-Authenticate header actually advertises a Bearer challenge. A 401 carrying only a non-bearer challenge (e.g. Basic), or an unparseable one, is surfaced unchanged: refreshing the bearer token cannot satisfy it, so retrying would only earn a second 401 after a wasted token fetch and round trip. Reuses AuthChallengeParser for RFC 7235 parsing, which also matches a Bearer challenge listed alongside others in a multi-challenge header. --- .../pipeline/steps/BearerTokenAuthStep.kt | 44 +++++++++++++--- .../pipeline/steps/BearerTokenAuthStepTest.kt | 51 +++++++++++++++++++ 2 files changed, 88 insertions(+), 7 deletions(-) diff --git a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/pipeline/steps/BearerTokenAuthStep.kt b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/pipeline/steps/BearerTokenAuthStep.kt index 7258c59e..ab9c1500 100644 --- a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/pipeline/steps/BearerTokenAuthStep.kt +++ b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/pipeline/steps/BearerTokenAuthStep.kt @@ -7,6 +7,7 @@ package org.dexpace.sdk.core.http.pipeline.steps +import org.dexpace.sdk.core.http.auth.AuthChallengeParser import org.dexpace.sdk.core.http.auth.BearerToken import org.dexpace.sdk.core.http.auth.BearerTokenProvider import org.dexpace.sdk.core.http.common.HttpHeaderName @@ -51,6 +52,11 @@ import kotlin.concurrent.withLock * method (the in-step hook is not gated by retry-safety), so a non-idempotent POST is * re-authenticated too. * + * Eviction only fires when the `WWW-Authenticate` header actually advertises a `Bearer` + * challenge. A 401 carrying only a non-bearer challenge (e.g. `Basic`) — or an unparseable + * one — is surfaced unchanged, because refreshing the bearer token would not satisfy it: the + * retry would just be rejected again, at the cost of a wasted token fetch and round trip. + * * A request that reaches the challenge hook carrying **no** `Authorization` header is a * cross-origin redirect re-issue whose credential the AUTH stage deliberately suppressed * (see [CrossOriginRedirectMarker]). In that case the hook re-stamps nothing and surfaces the @@ -98,13 +104,19 @@ public open class BearerTokenAuthStep * On a 401 challenge, evict the token that was just rejected and re-stamp [request] * with a freshly fetched one so [AuthStep]'s single retry carries a new credential. * - * Returns `null` — surfacing the 401 unchanged with no retry — when [request] carried no - * `Authorization` header. A request reaches this hook credential-free only when the AUTH - * stage deliberately suppressed stamping, i.e. a cross-origin redirect re-issue (see - * [CrossOriginRedirectMarker]). Re-stamping there would attach the caller's bearer token - * to a server-chosen foreign host and re-drive it through the chain, leaking the token - * cross-origin and bypassing the HTTPS guard that only [AuthStep.process]'s first pass - * enforces. Refusing to re-stamp preserves that suppression. + * Returns `null` — surfacing the 401 unchanged with no retry — in two cases: + * + * - [request] carried no `Authorization` header. A request reaches this hook + * credential-free only when the AUTH stage deliberately suppressed stamping, i.e. a + * cross-origin redirect re-issue (see [CrossOriginRedirectMarker]). Re-stamping there + * would attach the caller's bearer token to a server-chosen foreign host and re-drive it + * through the chain, leaking the token cross-origin and bypassing the HTTPS guard that + * only [AuthStep.process]'s first pass enforces. Refusing to re-stamp preserves that + * suppression. + * - [response]'s `WWW-Authenticate` header does not advertise a `Bearer` challenge. + * Refreshing the bearer token cannot satisfy a `Basic`/`Digest`-only (or unparseable) + * challenge, so the retry would only earn a second 401 after a wasted fetch and round + * trip; the original 401 is surfaced instead. * * Otherwise eviction is scoped to the exact token that produced the 401 (matched against * the `Authorization` header [request] carried): if a concurrent request already refreshed @@ -120,10 +132,24 @@ public open class BearerTokenAuthStep // No credential on the rejected request means stamping was suppressed (cross-origin // redirect). Do not re-attach one: surface the 401 unchanged. val rejectedHeader = request.headers.get(HttpHeaderName.AUTHORIZATION) ?: return null + // A token refresh can only satisfy a Bearer challenge; surface anything else unchanged + // rather than burning a fetch + retry that the server will just reject again. + if (!offersBearerChallenge(response)) return null evictRejectedToken(rejectedHeader) return authorizeRequest(request) } + /** + * Returns `true` when [response]'s `WWW-Authenticate` header advertises a `Bearer` + * challenge. A header with only non-bearer challenges (or one that does not parse) returns + * `false`. [AuthStep] guarantees the header is present before this hook runs; the explicit + * null-guard keeps the method correct if called from elsewhere. + */ + private fun offersBearerChallenge(response: Response): Boolean { + val header = response.headers.get(HttpHeaderName.WWW_AUTHENTICATE) ?: return false + return AuthChallengeParser.parse(header).any { it.scheme == BEARER_SCHEME } + } + /** * Clears [cachedToken] iff it is still the token whose stamped header is [rejectedHeader]. * Guarded by the same [lock] as the refresh path so the read-compare-clear is atomic @@ -198,5 +224,9 @@ public open class BearerTokenAuthStep // Default refresh margin: refresh the bearer token 30 seconds before its expiry // so an in-flight request never carries a near-expired credential. private const val DEFAULT_REFRESH_MARGIN_SECONDS = 30L + + // Lower-cased `Bearer` scheme name; AuthChallengeParser normalises schemes to lower + // case, so the eviction gate compares against this constant. + private const val BEARER_SCHEME = "bearer" } } diff --git a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/pipeline/steps/BearerTokenAuthStepTest.kt b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/pipeline/steps/BearerTokenAuthStepTest.kt index c1be41d0..ad0254f3 100644 --- a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/pipeline/steps/BearerTokenAuthStepTest.kt +++ b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/pipeline/steps/BearerTokenAuthStepTest.kt @@ -331,6 +331,57 @@ class BearerTokenAuthStepTest { assertNull(fake.requests.single().headers.get(HttpHeaderName.AUTHORIZATION)) } + @Test + fun `401 advertising only a non-bearer challenge is surfaced unchanged without eviction or refetch`() { + // A token refresh cannot satisfy a Basic-only challenge, so the step must NOT evict, + // refetch, or retry: the 401 surfaces unchanged after the single initial fetch. + val fetches = AtomicInteger(0) + val provider = + BearerTokenProvider { _, _ -> + BearerToken("token-${fetches.incrementAndGet()}", futureExpiry) + } + val fake = + FakeHttpClient() + .enqueue { status(401).header("WWW-Authenticate", "Basic realm=\"x\"") } + val pipeline = + HttpPipelineBuilder(fake) + .append(BearerTokenAuthStep(provider, listOf("scope"), clock = clock)) + .build() + + val response = pipeline.send(getRequest()) + + assertEquals(401, response.status.code, "a non-bearer 401 must surface unchanged") + assertEquals(1, fake.callCount, "no retry for a challenge a token refresh cannot satisfy") + assertEquals(1, fetches.get(), "only the initial stamp fetched; the 401 must not trigger a refetch") + assertEquals("Bearer token-1", fake.requests.single().headers.get(HttpHeaderName.AUTHORIZATION)) + } + + @Test + fun `401 advertising both a non-bearer and a bearer challenge still evicts and retries`() { + // The eviction gate matches the Bearer challenge anywhere in a multi-challenge header, + // so a `Basic, Bearer` 401 still refreshes the token and retries. + val fetches = AtomicInteger(0) + val provider = + BearerTokenProvider { _, _ -> + BearerToken("token-${fetches.incrementAndGet()}", futureExpiry) + } + val fake = + FakeHttpClient() + .enqueue { status(401).header("WWW-Authenticate", "Basic realm=\"x\", Bearer realm=\"y\"") } + .enqueue { status(200) } + val pipeline = + HttpPipelineBuilder(fake) + .append(BearerTokenAuthStep(provider, listOf("scope"), clock = clock)) + .build() + + val response = pipeline.send(getRequest()) + + assertEquals(200, response.status.code) + assertEquals(2, fake.callCount, "the bearer challenge in the multi-challenge header triggers one retry") + assertEquals(2, fetches.get()) + assertEquals("Bearer token-2", fake.requests[1].headers.get(HttpHeaderName.AUTHORIZATION)) + } + // ---- Helpers ---- private fun getRequest(): Request =