Skip to content

Commit 0ff8cc8

Browse files
committed
fix: don't re-stamp bearer token on a cross-origin-redirect 401
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 <token> 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.
1 parent aea1b88 commit 0ff8cc8

3 files changed

Lines changed: 76 additions & 10 deletions

File tree

sdk-core/api/sdk-core.api

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -756,6 +756,7 @@ public class org/dexpace/sdk/core/http/pipeline/steps/BearerTokenAuthStep : org/
756756
public synthetic fun <init> (Lorg/dexpace/sdk/core/http/auth/BearerTokenProvider;Ljava/util/List;Ljava/time/Duration;Lorg/dexpace/sdk/core/util/Clock;ILkotlin/jvm/internal/DefaultConstructorMarker;)V
757757
protected fun authorizeRequest (Lorg/dexpace/sdk/core/http/request/Request;)Lorg/dexpace/sdk/core/http/request/Request;
758758
protected fun authorizeRequestOnChallenge (Lorg/dexpace/sdk/core/http/request/Request;Lorg/dexpace/sdk/core/http/response/Response;)Lorg/dexpace/sdk/core/http/request/Request;
759+
protected fun bearerHeaderValue (Ljava/lang/String;)Ljava/lang/String;
759760
}
760761

761762
public final class org/dexpace/sdk/core/http/pipeline/steps/DefaultAsyncInstrumentationStep : org/dexpace/sdk/core/http/pipeline/AsyncHttpStep {

sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/pipeline/steps/BearerTokenAuthStep.kt

Lines changed: 37 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,11 @@ import kotlin.concurrent.withLock
5151
* method (the in-step hook is not gated by retry-safety), so a non-idempotent POST is
5252
* re-authenticated too.
5353
*
54+
* A request that reaches the challenge hook carrying **no** `Authorization` header is a
55+
* cross-origin redirect re-issue whose credential the AUTH stage deliberately suppressed
56+
* (see [CrossOriginRedirectMarker]). In that case the hook re-stamps nothing and surfaces the
57+
* 401 unchanged, so the caller's token is never attached to a server-chosen foreign host.
58+
*
5459
* ## Errors from [provider]
5560
*
5661
* - Throws → propagated. The exception is **not** cached, so a subsequent request
@@ -62,7 +67,9 @@ import kotlin.concurrent.withLock
6267
*
6368
* Eviction-on-401 is built in. Users wanting to customise challenge handling further (e.g.
6469
* inspect the `WWW-Authenticate` scheme before deciding to refresh) can override
65-
* [authorizeRequestOnChallenge].
70+
* [authorizeRequestOnChallenge]. A subclass that changes the stamped header format by
71+
* overriding [authorizeRequest] must also override [bearerHeaderValue] so the eviction match
72+
* keeps recognising the rejected token.
6673
*
6774
* Thread-safety: see Caching above.
6875
* Cancellation: token fetch may block; the [provider] is expected to respect interrupts.
@@ -83,16 +90,24 @@ public open class BearerTokenAuthStep
8390
override fun authorizeRequest(request: Request): Request {
8491
val token = currentToken()
8592
return request.newBuilder()
86-
.setHeader(HttpHeaderName.AUTHORIZATION.caseSensitiveName, "Bearer ${token.token}")
93+
.setHeader(HttpHeaderName.AUTHORIZATION.caseSensitiveName, bearerHeaderValue(token.token))
8794
.build()
8895
}
8996

9097
/**
9198
* On a 401 challenge, evict the token that was just rejected and re-stamp [request]
9299
* with a freshly fetched one so [AuthStep]'s single retry carries a new credential.
93100
*
94-
* Eviction is scoped to the exact token that produced the 401 (matched against the
95-
* `Authorization` header [request] carried): if a concurrent request already refreshed
101+
* Returns `null` — surfacing the 401 unchanged with no retry — when [request] carried no
102+
* `Authorization` header. A request reaches this hook credential-free only when the AUTH
103+
* stage deliberately suppressed stamping, i.e. a cross-origin redirect re-issue (see
104+
* [CrossOriginRedirectMarker]). Re-stamping there would attach the caller's bearer token
105+
* to a server-chosen foreign host and re-drive it through the chain, leaking the token
106+
* cross-origin and bypassing the HTTPS guard that only [AuthStep.process]'s first pass
107+
* enforces. Refusing to re-stamp preserves that suppression.
108+
*
109+
* Otherwise eviction is scoped to the exact token that produced the 401 (matched against
110+
* the `Authorization` header [request] carried): if a concurrent request already refreshed
96111
* the cache, that newer token is preserved and reused for the retry rather than being
97112
* discarded. The subsequent [currentToken] call then re-fetches only when the cache is
98113
* actually empty, so a 401 storm across threads still funnels through the same
@@ -101,26 +116,38 @@ public open class BearerTokenAuthStep
101116
override fun authorizeRequestOnChallenge(
102117
request: Request,
103118
response: Response,
104-
): Request {
105-
evictRejectedToken(request.headers.get(HttpHeaderName.AUTHORIZATION))
119+
): Request? {
120+
// No credential on the rejected request means stamping was suppressed (cross-origin
121+
// redirect). Do not re-attach one: surface the 401 unchanged.
122+
val rejectedHeader = request.headers.get(HttpHeaderName.AUTHORIZATION) ?: return null
123+
evictRejectedToken(rejectedHeader)
106124
return authorizeRequest(request)
107125
}
108126

109127
/**
110128
* Clears [cachedToken] iff it is still the token whose stamped header is [rejectedHeader].
111129
* Guarded by the same [lock] as the refresh path so the read-compare-clear is atomic
112-
* against a concurrent refresh.
130+
* against a concurrent refresh. Routes the comparison through [bearerHeaderValue] so it
131+
* stays in lock-step with the value [authorizeRequest] stamps.
113132
*/
114-
private fun evictRejectedToken(rejectedHeader: String?) {
115-
if (rejectedHeader == null) return
133+
private fun evictRejectedToken(rejectedHeader: String) {
116134
lock.withLock {
117135
val current = cachedToken ?: return
118-
if ("Bearer ${current.token}" == rejectedHeader) {
136+
if (bearerHeaderValue(current.token) == rejectedHeader) {
119137
cachedToken = null
120138
}
121139
}
122140
}
123141

142+
/**
143+
* The `Authorization` header value for [token]. Single source of truth shared by
144+
* [authorizeRequest] (stamping) and [evictRejectedToken] (the eviction match), so the two
145+
* never drift. A subclass that overrides [authorizeRequest] to emit a different header
146+
* format must override this too, or eviction will stop matching and a rejected token will
147+
* be re-fetched but never cleared.
148+
*/
149+
protected open fun bearerHeaderValue(token: String): String = "Bearer $token"
150+
124151
private fun currentToken(): BearerToken {
125152
// Fast path: lock-free volatile read; return the cached token if it's still valid.
126153
cachedToken?.takeIf { !it.isExpiredAt(clock.now(), refreshMargin) }?.let { return it }

sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/pipeline/steps/BearerTokenAuthStepTest.kt

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ import kotlin.test.Test
2626
import kotlin.test.assertEquals
2727
import kotlin.test.assertFailsWith
2828
import kotlin.test.assertNotNull
29+
import kotlin.test.assertNull
2930
import kotlin.test.assertTrue
3031

3132
class BearerTokenAuthStepTest {
@@ -293,6 +294,43 @@ class BearerTokenAuthStepTest {
293294
assertEquals(2, fetches.get())
294295
}
295296

297+
@Test
298+
fun `cross-origin-marked request that 401s is surfaced unchanged without stamping a token`() {
299+
// A cross-origin redirect re-issue is stripped of its Authorization header and tagged
300+
// with the internal marker, so it reaches the foreign host with no credential. If that
301+
// host 401s, the challenge path must NOT stamp the caller's bearer token onto the
302+
// foreign host (that would leak the token cross-origin and bypass the HTTPS/cross-origin
303+
// suppression). The 401 must surface unchanged with no token fetch.
304+
val fetches = AtomicInteger(0)
305+
val provider =
306+
BearerTokenProvider { _, _ ->
307+
BearerToken("token-${fetches.incrementAndGet()}", futureExpiry)
308+
}
309+
val fake =
310+
FakeHttpClient()
311+
.enqueue { status(401).header("WWW-Authenticate", "Bearer realm=\"x\"") }
312+
val pipeline =
313+
HttpPipelineBuilder(fake)
314+
.append(BearerTokenAuthStep(provider, listOf("scope"), clock = clock))
315+
.build()
316+
317+
// Mimic DefaultRedirectStep's cross-origin re-issue: no Authorization, marker present.
318+
val markedRequest =
319+
Request.builder()
320+
.method(Method.GET)
321+
.url("https://foreign.example.org/resource")
322+
.addHeader(CrossOriginRedirectMarker.MARKER_HEADER, CrossOriginRedirectMarker.MARKER_VALUE)
323+
.build()
324+
325+
val response = pipeline.send(markedRequest)
326+
327+
assertEquals(401, response.status.code, "the cross-origin 401 must surface unchanged")
328+
assertEquals(1, fake.callCount, "no retry: the credential-free request must not be re-stamped")
329+
assertEquals(0, fetches.get(), "no token must be fetched for a credential-free cross-origin request")
330+
// The (single) request that reached the foreign host carried no Authorization header.
331+
assertNull(fake.requests.single().headers.get(HttpHeaderName.AUTHORIZATION))
332+
}
333+
296334
// ---- Helpers ----
297335

298336
private fun getRequest(): Request =

0 commit comments

Comments
 (0)