From 0d9b2d8d29ee9dbf9bb67309d5cf5cf9425799b4 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 29 Aug 2026 15:00:56 +0000 Subject: [PATCH] fix: never hand a completed sign-job to a new Blossom token caller MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `signOnce` retired the in-flight entry from `invokeOnCompletion`, which runs when the job ends — after `fresh.complete()` has already resumed the awaiting caller. In that window the map still holds a *completed* deferred, so the next caller took the leader/follower branch and was handed the token that job had already signed instead of signing a new one. A caller whose token has just expired does exactly that: `header()` misses the cache, reaches `signOnce`, and gets the expired token straight back. `BlossomReadAuthTokenProviderTest.refreshesAfterExpiry` closes that window immediately, so it hit the bug on every run and has been failing on main. Remove the entry before completing it. `invokeOnCompletion` keeps its now-idempotent removal as the cancellation safety net. The test also asserted the re-signed header differed byte-for-byte from the first. That cannot hold: the injected `clock` only drives the cache TTL, while BlossomAuthorizationEvent takes `created_at` from `TimeUtils.now()`, so two signings in the same second produce identical events. Count signatures instead, which is what "must be re-signed" actually means. --- .../okhttp/BlossomReadAuthTokenProvider.kt | 9 ++++++++ .../BlossomReadAuthTokenProviderTest.kt | 23 +++++++++++++++---- 2 files changed, 28 insertions(+), 4 deletions(-) diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/okhttp/BlossomReadAuthTokenProvider.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/okhttp/BlossomReadAuthTokenProvider.kt index af62b37767..4ce346f6c9 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/okhttp/BlossomReadAuthTokenProvider.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/okhttp/BlossomReadAuthTokenProvider.kt @@ -139,6 +139,15 @@ class BlossomReadAuthTokenProvider( if (header != null) { cache[host] = CachedToken(header, clock() + CACHE_TTL_MS) } + + // Retire the entry *before* completing it. `invokeOnCompletion` fires when the + // job ends, which is after `complete()` resumes the awaiting caller — so a + // caller that returned from `header()` could come straight back, find this + // finished deferred still in the map, and be handed its already-signed token + // instead of signing a new one. A caller whose token has just expired does + // exactly that, and got the expired token back for as long as the window + // lasted — [refreshesAfterExpiry] closes it immediately and so hit it every run. + inFlight.remove(host, fresh) fresh.complete(header) }.invokeOnCompletion { inFlight.remove(host, fresh) diff --git a/amethyst/src/test/java/com/vitorpamplona/amethyst/service/okhttp/BlossomReadAuthTokenProviderTest.kt b/amethyst/src/test/java/com/vitorpamplona/amethyst/service/okhttp/BlossomReadAuthTokenProviderTest.kt index b209522a56..de8de985e2 100644 --- a/amethyst/src/test/java/com/vitorpamplona/amethyst/service/okhttp/BlossomReadAuthTokenProviderTest.kt +++ b/amethyst/src/test/java/com/vitorpamplona/amethyst/service/okhttp/BlossomReadAuthTokenProviderTest.kt @@ -35,6 +35,7 @@ import kotlinx.coroutines.withTimeout import org.junit.After import org.junit.Assert.assertEquals import org.junit.Assert.assertNotEquals +import org.junit.Assert.assertNotNull import org.junit.Assert.assertNull import org.junit.Assert.assertTrue import org.junit.Test @@ -114,14 +115,28 @@ class BlossomReadAuthTokenProviderTest { fun refreshesAfterExpiry() = runBlocking { var now = 0L - val provider = BlossomReadAuthTokenProvider({ signer }, scope, clock = { now }) + var lookups = 0 + val provider = + BlossomReadAuthTokenProvider({ + lookups++ + signer + }, scope, clock = { now }) + + assertNotNull(provider.header(host)) + assertEquals("the first call must sign", 1, lookups) - val first = provider.header(host) now += 56L * 60L * 1000L assertNull("token must be gone from the pure read once expired", provider.cachedHeader(host)) - val second = provider.header(host) - assertNotEquals("an expired token must be re-signed", first, second) + // Counts signatures rather than comparing the two headers: the injected [clock] only + // drives the cache TTL, while the signed BlossomAuthorizationEvent takes its + // `created_at` from the real wall clock (TimeUtils.now()). Both signings land in the + // same second on any quick machine, so the two events — and therefore their ids, sigs + // and headers — are byte-identical, and an assertNotEquals on them fails even though + // the token was correctly re-signed. The signer is only ever consulted on a real + // signing pass (a cache hit returns before it), so this counter says exactly that. + assertNotNull(provider.header(host)) + assertEquals("an expired token must be re-signed", 2, lookups) } @Test