Resend a 401 that refuses only the DPoP proof, with no refresh (#268)

* Test: a 401 refusing the DPoP proof or asking a nonce is resent once with no refresh (red)

Six of the seven cases fail on main: a refused proof refreshes the token,
a redirected write whose hop's proof was refused is sent again, and no
nonce HEY hands out ever reaches the strategy. The seventh (a nonce
another origin handed out never signs for this one) passes vacuously
until nonces are read, and guards the fix.

HttpRequestBuilder.dpopNonce is added here so the cases compile; nothing
sets it yet.

* Resend a 401 that refuses only the DPoP proof, with no refresh

A DPoP-bound token signs each request with a proof of its own. When HEY
refused the proof and not the token -- its time or jti (RFC 9449, 7.1,
invalid_dpop_proof), or a nonce it did not name (9, use_dpop_nonce) --
the Kotlin client spent a refresh on a token that was fine, and had no
way to hand the strategy the nonce HEY asked for.

Now such a 401 is sent once more with a fresh proof, and the token is
not refreshed; a second refusal is the caller's. The client keeps the
DPoP-Nonce each origin (scheme, host, port) hands out with any answer,
a redirect's included, and exposes it to the strategy as
HttpRequestBuilder.dpopNonce while it signs; a nonce is never offered to
another origin. A write HEY answered with a redirect is not sent again
for its hop's refused proof: the write went through.

The challenge's error is read by bearerError, quoted or bare, never from
error_description.

Verified: the seven new CredentialRefreshTest cases (six red before
this), a parser case, and make kt-check kt-check-drift.

Not done: no conformance case. DPoP signing exists only in Kotlin; the
other runners have no strategy hook to hand a nonce to, so a shared case
would hold them to a surface they do not have.

* Hear a proof refusal in any challenge header, and say a redirected write may be made

Review findings:

- A server that takes Bearer and DPoP may send each challenge in a
  WWW-Authenticate header of its own. Ktor's Headers.get answers the
  first only, so `Bearer realm="hey"` hid a second header's
  `DPoP error="invalid_dpop_proof"` and the client refreshed a good
  token. Every value is read now. Shown red on the first-only read:
  refreshes expected 0, was 1.
- A 302 or 303 answers a write that went through; a 307 or 308 repeats
  it at the hop, where a 401 refused it. The client holds both back,
  which is safe for either; the comment and README now say "may already
  be made" rather than "went through".
- Two cases the suite left open: a POST whose own first send had its
  proof refused is resent (flipping the guard's || to && passed before),
  and a read whose redirect hop had its proof refused is read again.

* Read every challenge in a WWW-Authenticate field, and no parameter inside a quoted value

Review: the error was taken from the first error= the field held, by a
regex that did not know challenges or quoted strings. Two cases read
wrong:

- `Bearer error="invalid_token", DPoP error="use_dpop_nonce"`, one
  field listing both challenges (RFC 9110, 11.6.1), was heard for the
  Bearer error, and refreshed a good token instead of answering the
  nonce.
- `error_description="prior error=invalid_dpop_proof", error=...`
  found an error inside the quoted description and suppressed a refresh
  the token needed.

With the second-header case before them, these are three ways one regex
misread the header, so it is replaced rather than patched again:
challengeErrors reads the field as RFC 9110 auth-params, each quoted
value whole, and answers every error it names. A 401 is a proof refusal
when any challenge in any field names one.

Shown red on the old reading: the combined field refreshed (expected 0,
was 1); the quoted description did not (expected 1, was 0).

* Take a 401 for a refused proof only when the request sent one

Review: reading every challenge in a field let a Bearer request answered
with `Bearer error="invalid_token", DPoP error="use_dpop_nonce"` be
taken for a refused proof. It was resent with the same stale token and
never refreshed. A proof refusal now needs the request to have gone out
under a DPoP credential (Authorization: DPoP ...); a Bearer request
meets any 401 with the refresh it always had. Shown red without the
gate: the resend carried the stale token again.

* Resend a proof refused after a redirect only when no hop changed the method

What was wrong: the proof-refusal resend keyed on "redirected" and on the
operation being idempotent. A DELETE answered with a 303 then had its GET
hop's refused proof resend the DELETE, which answers a deletion already
made with a 404; and a POST sent on by a 307 or 308, refused at the
destination and so not made, surfaced the 401 instead of going again.

What it is now: the redirect loop records whether any hop changed the
method (a 303, or a 301 or 302 to a POST). Only such a hop answered the
request first sent; a refused proof after it is the caller's, idempotent
or not. A hop that kept the method asks for the same request elsewhere,
and the operation goes again with a fresh proof.

Verified: two new CredentialRefreshTest cases, a 303'd DELETE not resent
and a 307'd POST resent, fail against the old condition and pass now;
make kt-check passes.

Rejected: limiting the resend to GET operations, as one review
suggested: it would also refuse the 307/308 write the other asked for.

* Hold a refused proof after a redirect to the rule the refresh path keeps

The previous commit resent a proof refused after a redirect only when
no hop changed the method. That disagreed with the refresh path, which
resends a redirected operation when it is idempotent (as main always
has) and holds back a redirected non-idempotent write. Under it, a PUT
answered with a 303 whose hop's proof was refused came back a 401 for a
write that succeeded, where a refreshed token resends it and succeeds.

One rule now serves both paths: resend when the operation is
idempotent or no redirect was followed. The hop-method tracking goes;
it only ever decided the 307/308 case, and HEY answers no write with a
307 or 308: its write redirects are all 302. Repeating an idempotent
write is safe (RFC 9110, 9.2.2), though a DELETE already made may then
answer 404.

The previous commit's two cases stay, with the rule's expectations: a
303'd DELETE whose hop's proof was refused is sent again; a 307'd POST
whose hop's proof was refused is the caller's 401, not resent. The
README says non-idempotent.

Verified: CredentialRefreshTest 32/32, make kt-check.

* Say a redirect can answer a write that went through, not that it does

Outside review: some of HEY's 302s answer a write that was not made (a
shared topic's trash asks for confirmation; a refused send redirects).
The comments, READMEs and the conformance case said a 302 or 303
answers a write that went through, or that the write has been made. They
now say it can: the write may already have taken effect, and the
redirect does not say whether it did. That uncertainty is the reason a
non-idempotent write is held back; behaviour is unchanged.

Author: jeremy

kotlin/README.md

@@ -32,6 +32,15 @@ accord — handing over a new token ahead of expiry — is not asked to refresh
 old one; the request is simply resent. Anything that wants the request headers
 outright implements `AuthStrategy` and passes it with `auth(strategy)`.
 
+A strategy that signs DPoP proofs (RFC 9449) names `request.dpopNonce` in each proof's `nonce`
+claim: the nonce the request's own origin handed out last, from any answer including a redirect,
+and never another origin's. A 401 whose challenge refuses only the proof (`invalid_dpop_proof`)
+or asks for a nonce (`use_dpop_nonce`) is sent once more with a fresh proof and never refreshes
+the token; a second such refusal is the caller's. A non-idempotent write HEY answered with a
+redirect is not sent again for its hop's refused proof, since it may already be made (a 302 or
+303 can answer a write that went through); an idempotent operation is sent again, as it is after a
+refreshed token.
+
 ```kotlin
 val client = HeyClient {
     accessToken(object : TokenProvider {

kotlin/sdk/src/commonMain/kotlin/com/basecamp/hey/Auth.kt

@@ -3,6 +3,7 @@ package com.basecamp.hey
 import io.ktor.client.request.HttpRequestBuilder
 import io.ktor.client.request.header
 import io.ktor.http.HttpHeaders
+import io.ktor.util.AttributeKey
 
 /**
  * Provides access tokens for authenticating with the HEY API.
@@ -90,3 +91,35 @@ class BearerAuth(private val tokenProvider: TokenProvider) : AuthStrategy {
     /** What this would sign with now, as the header value: asked before a refresh, so a token the provider has already renewed is not renewed again. */
     internal suspend fun bearer(): String = "Bearer ${tokenProvider.accessToken()}"
 }
+
+/**
+ * The DPoP nonce (RFC 9449, 8 and 9) the server this request goes to handed out last, for a
+ * strategy that signs DPoP proofs to name in its proof; null where that origin handed out none. A
+ * nonce is kept for the origin that handed it out, and is never another origin's. A strategy that
+ * signs DPoP proofs must put it in the proof's `nonce` claim: one that does not is refused again
+ * by a server that asks for a nonce, and the request fails with that 401.
+ */
+val HttpRequestBuilder.dpopNonce: String? get() = attributes.getOrNull(DpopNonce)
+
+internal val DpopNonce = AttributeKey<String>("HeyDpopNonce")
+
+/**
+ * One auth-param (RFC 9110, 11.2): a name, `=`, and a token or a quoted string read whole, so text
+ * inside a quoted value that looks like a parameter is never taken for one.
+ */
+private val AuthParam = Regex("""([!#$%&'*+.^_`|~0-9A-Za-z-]+)\s*=\s*("(?:[^"\\]|\\.)*"|[^\s,"]*)""")
+
+/**
+ * Every `error` the challenges of one `WWW-Authenticate` field name (RFC 6750, 3; RFC 9449, 7.1),
+ * in order: a field may list several challenges, `Bearer error="invalid_token", DPoP
+ * error="use_dpop_nonce"` naming both. Empty for no field, or one that names none.
+ */
+internal fun challengeErrors(field: String?): List<String> =
+    field?.let { AuthParam.findAll(it) }.orEmpty()
+        .filter { it.groupValues[1].equals("error", ignoreCase = true) }
+        .map { param ->
+            val value = param.groupValues[2]
+            if (value.startsWith('"')) value.substring(1, value.length - 1).replace(Regex("""\\(.)"""), "$1") else value
+        }
+        .filter { it.isNotEmpty() }
+        .toList()

kotlin/sdk/src/commonMain/kotlin/com/basecamp/hey/HeyClient.kt

@@ -260,6 +260,18 @@ internal class Shared(
     /** The bearer the SDK's own strategy last put on a request, so a token the provider rotates of its own accord is seen for the renewal it is. Null until a signing, and again after a refresh. */
     var lastBearer: String? = null
 
+    /** The DPoP nonce each origin handed out last (RFC 9449, 8.2 and 9), by [origin]: a proof for one origin names its nonce alone. */
+    private val nonces = mutableMapOf<String, String>()
+
+    /** Guards [nonces]: held only for a read or a write. */
+    private val nonceGate = Mutex()
+
+    suspend fun nonce(url: Url): String? = nonceGate.withLock { nonces[origin(url)] }
+
+    suspend fun heardNonce(url: Url, nonce: String) = nonceGate.withLock { nonces[origin(url)] = nonce }
+
+    private fun origin(url: Url) = "${url.protocol.name.lowercase()}://${url.host.lowercase()}:${url.port}"
+
     /** The credentials a request is signed under: the state of the counters at its signing, read under [refreshing]. */
     fun generation(): Generation = Generation(refreshes, refreshRuns)
 
@@ -564,6 +576,7 @@ class HeyClient internal constructor(
         var attempt = 1
         var delay = budget.delay
         var refreshed = false
+        var reproved = false
         var cached: Pair<String, CachedResponse>? = null
 
         while (true) {
@@ -608,9 +621,31 @@ class HeyClient internal constructor(
             val received = sent.getOrThrow()
             val status = received.status
             val retryable = status in budget.retryOn
+            // A 401 that refuses the request's DPoP proof and not its token: its time or its jti
+            // (RFC 9449, 7.1), or a nonce it did not name (9). Sent once more with a fresh proof,
+            // naming the nonce just heard, and never a refresh, which would spend a good token. A
+            // mutation the server answered with a redirect may already be made (a 302 or 303
+            // can answer a write that went through), and only its hop was refused: sending the
+            // operation again could make it twice, so the 401 is the caller's. A server that
+            // takes Bearer and DPoP may send a challenge for each, in one field or in fields of
+            // their own: the proof's refusal is heard in whichever carries it. Only a request that
+            // went out under a DPoP credential sent a proof to refuse; a Bearer request answered
+            // with a DPoP challenge beside its own is refreshed as any other.
+            val proofRefused = status == 401 && received.authenticated &&
+                received.bearer?.substringBefore(' ')?.equals("DPoP", ignoreCase = true) == true &&
+                received.headers.getAll(HttpHeaders.WWWAuthenticate).orEmpty().any { field -> challengeErrors(field).any { it in PROOF_REFUSALS } }
+            if (proofRefused && !reproved && (operation.idempotent || !received.redirected)) {
+                val cause = HeyException.Auth("DPoP proof refused")
+                hooks.safeRequestEnd(info, RequestResult(status, duration, error = cause))
+                hooks.safeRetry(info, attempt + 1, cause, 0)
+                reproved = true
+                attempt += 1
+                attempts = maxOf(attempts, attempt)
+                continue
+            }
             // A 401 from a hop that carried no credentials rejected none of HEY's: there is
             // nothing to refresh, and nothing a resend would change.
-            val renewed = status == 401 && received.authenticated && !refreshed && try {
+            val renewed = status == 401 && received.authenticated && !proofRefused && !refreshed && try {
                 refreshCredentials(received.signedUnder, received.bearer)
             } catch (error: Throwable) {
                 hooks.safeRequestEnd(info, RequestResult(status, duration, error = reported(error, "request cancelled")))
@@ -802,6 +837,8 @@ class HeyClient internal constructor(
      */
     private suspend fun sign(request: HttpRequestBuilder): Pair<Credentials, Generation> {
         val before = request.headers.build()
+        val nonce = shared.nonce(request.url.build())
+        if (nonce != null) request.attributes.put(DpopNonce, nonce) else request.attributes.remove(DpopNonce)
         val signedUnder = shared.refreshing.withLock {
             try {
                 shared.auth.authenticate(request)
@@ -893,6 +930,10 @@ class HeyClient internal constructor(
         var authenticated = !operation.unsigned
         while (true) {
             val outcome = shared.http.prepareRequest(request).execute { response ->
+                // A server hands out its next DPoP nonce with any answer, a redirect's included
+                // (RFC 9449, 8.2), and the proofs for that origin name it from then on: the hop
+                // after it among them.
+                response.headers[DPOP_NONCE]?.let { shared.heardNonce(url, it) }
                 val next = if (operation.captureRedirects) null else redirectTarget(url, response)
                 if (next != null) Outcome.Redirect(next, response.status.value) else Outcome.Answer(receive(operation, url, response, hops > 0, authenticated, signedUnder, credentials.headers["authorization"]?.firstOrNull()))
             }
@@ -1223,3 +1264,8 @@ internal fun isParsed(accept: String): Boolean =
  * credentials already ran and failed, in which case its answer is this request's too.
  */
 internal class Generation(val refreshes: Long, val runs: Long)
+
+/** The errors a DPoP challenge names for a proof refused, not a token (RFC 9449, 7.1 and 9). */
+private val PROOF_REFUSALS = setOf("invalid_dpop_proof", "use_dpop_nonce")
+
+private const val DPOP_NONCE = "DPoP-Nonce"