Skip to content

Commit 12a4ffb

Browse files
bmc08gtbmc08gt
authored
fix(blobs): recover a group-chat avatar whose download URL expired (#1478)
A sender's avatar in a group transcript stays on its BlurHash. The blob's download_url is a short-lived signed URL, so a persisted profile picture routinely carries a dead one, and the three seams that are meant to re-mint it were all broken: - GetBlobs set the access context after signing, so the signature covered {blobIds} while the server verified {blobIds, context}. Every re-mint of a blob the caller doesn't own — which is every other member's avatar — came back UNAUTHENTICATED. Owned blobs were unaffected, so this only showed up in group chat, the one surface that passes BlobAccessContext.profile. - NetworkFetcher writes a response to the disk cache before it throws on the status code, and ImmutableBlobCacheStrategy served any cached response without a request. One fetch through an expired URL pinned the blob's key to 403 for good; no re-mint could reach it. A response NetworkFetcher would throw on is now neither served nor stored, which also un-poisons an entry an older build wrote. - BlobId was a value class over ByteArray, so it compared by array identity. Each read of a persisted MediaItem decodes fresh arrays, making every emission a different MediaItem; remember(media) in rememberMediaUrl then dropped its state and cancelled the in-flight re-mint. On device this showed as GetBlobs being sent and its result never applied. BlobId carries a serializer that keeps encoding it as bare bytes. MediaItems persisted while it was a value class hold the id inlined, and the class-shaped default silently failed to decode them — which drops the whole MediaItem and leaves the avatar with no image at all.
1 parent fd77034 commit 12a4ffb

8 files changed

Lines changed: 391 additions & 34 deletions

File tree

‎apps/flipcash/app/build.gradle.kts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -362,4 +362,5 @@ dependencies {
362362
testImplementation(libs.junit)
363363
testImplementation(libs.kotlin.test.junit)
364364
testImplementation(libs.kotlinx.coroutines.test)
365+
testImplementation(libs.mockk)
365366
}

‎apps/flipcash/app/src/main/kotlin/com/flipcash/app/ImmutableBlobCacheStrategy.kt‎

Lines changed: 31 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -14,26 +14,48 @@ import coil3.request.Options
1414
* revalidate or re-download once a stored response looks stale, so images we already have on disk
1515
* still get re-fetched across launches — flashing the BlurHash before the same bytes reappear.
1616
*
17-
* Since the bytes are static there is nothing to revalidate: if we have a cached response we serve
18-
* it unconditionally and never hit the network; on a miss we download once and always persist. This
19-
* is paired with keying the Coil disk/memory cache on the durable blob id rather than the rotating
20-
* URL (see `MediaItem.cacheKeyForSize`); without a stable key this strategy would have nothing to
21-
* hit.
17+
* Since the bytes are static there is nothing to revalidate: if we have the bytes we serve them
18+
* unconditionally and never hit the network; on a miss we download once and persist. This is paired
19+
* with keying the Coil disk/memory cache on the durable blob id rather than the rotating URL (see
20+
* `MediaItem.cacheKeyForSize`); without a stable key this strategy would have nothing to hit.
21+
*
22+
* "Immutable" covers the bytes, not the failures. `NetworkFetcher` writes a response to the disk
23+
* cache *before* it throws on the status code, so a fetch through an expired URL leaves a cached 403
24+
* under the blob's key — and a strategy that serves any cached response replays that 403 on every
25+
* later load without a request, which no amount of re-minting can recover from. So a response that
26+
* would make `NetworkFetcher` throw is neither served nor stored.
2227
*/
2328
internal object ImmutableBlobCacheStrategy : CacheStrategy {
2429

25-
// Only invoked when a cached response exists — serve it as-is, no revalidation, no network.
30+
// Only invoked when a cached response exists — serve the bytes as-is, no revalidation, no
31+
// network. A cached failure isn't bytes: fall through to the network so a re-minted URL gets
32+
// its chance, which is also what un-poisons an entry an older build already wrote.
2633
override suspend fun read(
2734
cacheResponse: NetworkResponse,
2835
networkRequest: NetworkRequest,
2936
options: Options,
30-
): CacheStrategy.ReadResult = CacheStrategy.ReadResult(cacheResponse)
37+
): CacheStrategy.ReadResult = if (cacheResponse.isUsable) {
38+
CacheStrategy.ReadResult(cacheResponse)
39+
} else {
40+
CacheStrategy.ReadResult(networkRequest)
41+
}
3142

32-
// Always persist freshly downloaded bytes so the next load is a hit.
43+
// Always persist freshly downloaded bytes so the next load is a hit. Declining a failure also
44+
// leaves any entry already on disk intact, since the write is what would replace it.
3345
override suspend fun write(
3446
cacheResponse: NetworkResponse?,
3547
networkRequest: NetworkRequest,
3648
networkResponse: NetworkResponse,
3749
options: Options,
38-
): CacheStrategy.WriteResult = CacheStrategy.WriteResult(networkResponse)
50+
): CacheStrategy.WriteResult = if (networkResponse.isUsable) {
51+
CacheStrategy.WriteResult(networkResponse)
52+
} else {
53+
CacheStrategy.WriteResult.DISABLED
54+
}
55+
56+
/** Mirrors the status codes `NetworkFetcher` accepts instead of throwing `HttpException`. */
57+
private val NetworkResponse.isUsable: Boolean
58+
get() = code in 200 until 300 || code == HTTP_NOT_MODIFIED
59+
60+
private const val HTTP_NOT_MODIFIED = 304
3961
}

‎apps/flipcash/app/src/test/kotlin/com/flipcash/app/ImmutableBlobCacheStrategyTest.kt‎

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,69 @@
1+
package com.flipcash.app
2+
3+
import android.content.Context
4+
import coil3.network.NetworkRequest
5+
import coil3.network.NetworkResponse
6+
import coil3.request.Options
7+
import io.mockk.mockk
8+
import kotlinx.coroutines.test.runTest
9+
import kotlin.test.Test
10+
import kotlin.test.assertEquals
11+
import kotlin.test.assertNull
12+
import kotlin.test.assertSame
13+
14+
/**
15+
* A cached failure is the one thing this strategy must not treat as immutable: `NetworkFetcher`
16+
* writes the response to the disk cache before it throws on the status code, and it re-throws
17+
* whatever the strategy hands back from the cache without making a request. Serve a stored 403 and
18+
* that blob is unloadable for the life of the cache entry, however fresh a download URL it is given.
19+
*/
20+
class ImmutableBlobCacheStrategyTest {
21+
22+
private val request = NetworkRequest(url = "https://cdn.example/images/blob/thumbnail_160x160.webp")
23+
private val options = Options(context = mockk<Context>())
24+
25+
@Test
26+
fun `a cached success is served without a request`() = runTest {
27+
val cached = NetworkResponse(code = 200)
28+
29+
val result = ImmutableBlobCacheStrategy.read(cached, request, options)
30+
31+
assertSame(cached, result.response)
32+
assertNull(result.request)
33+
}
34+
35+
@Test
36+
fun `a cached failure falls through to the network`() = runTest {
37+
for (code in listOf(403, 404, 500)) {
38+
val result = ImmutableBlobCacheStrategy.read(NetworkResponse(code = code), request, options)
39+
40+
assertNull(result.response, "$code was served from the cache")
41+
assertSame(request, result.request, "$code did not issue a request")
42+
}
43+
}
44+
45+
@Test
46+
fun `a success is persisted`() = runTest {
47+
val response = NetworkResponse(code = 200)
48+
49+
val result = ImmutableBlobCacheStrategy.write(null, request, response, options)
50+
51+
assertSame(response, result.response)
52+
}
53+
54+
@Test
55+
fun `a failure is not persisted`() = runTest {
56+
for (code in listOf(403, 404, 500)) {
57+
val result = ImmutableBlobCacheStrategy.write(null, request, NetworkResponse(code = code), options)
58+
59+
assertNull(result.response, "$code was written to the cache")
60+
}
61+
}
62+
63+
@Test
64+
fun `a not modified response is served, matching what the fetcher accepts`() = runTest {
65+
val cached = NetworkResponse(code = 304)
66+
67+
assertEquals(cached, ImmutableBlobCacheStrategy.read(cached, request, options).response)
68+
}
69+
}

‎services/flipcash/src/main/kotlin/com/flipcash/services/internal/network/api/BlobStorageApi.kt‎

Lines changed: 41 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import com.codeinc.flipcash.gen.blob.v1.BlobStorageGrpcKt
44
import com.codeinc.flipcash.gen.blob.v1.BlobStorageService as RpcBlobStorageService
55
import com.codeinc.flipcash.gen.blob.v1.Model
66
import com.codeinc.flipcash.gen.blob.v1.validate
7+
import com.codeinc.flipcash.gen.common.v1.Common
78
import com.flipcash.services.internal.annotations.FlipcashManagedChannel
89
import com.flipcash.services.internal.network.extensions.asChatId
910
import com.flipcash.services.internal.network.extensions.asUserId
@@ -86,34 +87,52 @@ internal class BlobStorageApi @Inject constructor(
8687
owner: Ed25519.KeyPair,
8788
context: BlobAccessContext,
8889
): RpcBlobStorageService.GetBlobsResponse {
89-
val request = RpcBlobStorageService.GetBlobsRequest.newBuilder()
90-
.setBlobIds(
91-
Model.BlobIdBatch.newBuilder()
92-
.addAllBlobIds(blobIds.map { it.toProto() })
93-
)
94-
.apply { setAuth(authenticate(owner)) }
95-
// Omitted for Owned: the server resolves the caller's own blobs without one, and a
96-
// scope the caller can't claim would only narrow the read.
97-
.apply { context.toProto()?.let { setContext(it) } }
98-
.build()
90+
val request = getBlobsRequest(blobIds, context) { authenticate(owner) }
9991

10092
request.validate().orThrow()
10193

10294
return withContext(Dispatchers.IO) {
10395
api.getBlobs(request)
10496
}
10597
}
98+
}
10699

107-
private fun BlobId.toProto(): Model.BlobId =
108-
Model.BlobId.newBuilder().setValue(bytes.toByteString()).build()
109-
110-
private fun BlobAccessContext.toProto(): Model.AccessContext? = when (this) {
111-
BlobAccessContext.Owned -> null
112-
is BlobAccessContext.Profile ->
113-
Model.AccessContext.newBuilder().setUserProfile(userId.asUserId()).build()
114-
is BlobAccessContext.Chat ->
115-
Model.AccessContext.newBuilder().setChat(chatId.asChatId()).build()
116-
is BlobAccessContext.ChatProfile ->
117-
Model.AccessContext.newBuilder().setChatProfile(chatId.asChatId()).build()
118-
}
100+
/**
101+
* Builds a `GetBlobs` request with [context] already in place when [authenticate] runs.
102+
*
103+
* Order matters and isn't cosmetic: `authenticate` signs the message as built so far, while the
104+
* server verifies that signature against the whole request minus the auth field. Anything set
105+
* afterwards is in what the server checks and missing from what the client signed, so the call
106+
* comes back `UNAUTHENTICATED` — which for an access context means every re-mint of a blob the
107+
* caller doesn't own fails, and the avatar it was for stays on its BlurHash.
108+
*
109+
* [authenticate] is a parameter so the ordering can be asserted without a signing key.
110+
*/
111+
internal fun getBlobsRequest(
112+
blobIds: List<BlobId>,
113+
context: BlobAccessContext,
114+
authenticate: RpcBlobStorageService.GetBlobsRequest.Builder.() -> Common.Auth,
115+
): RpcBlobStorageService.GetBlobsRequest =
116+
RpcBlobStorageService.GetBlobsRequest.newBuilder()
117+
.setBlobIds(
118+
Model.BlobIdBatch.newBuilder()
119+
.addAllBlobIds(blobIds.map { it.toProto() })
120+
)
121+
// Omitted for Owned: the server resolves the caller's own blobs without one, and a
122+
// scope the caller can't claim would only narrow the read.
123+
.apply { context.toProto()?.let { setContext(it) } }
124+
.apply { setAuth(authenticate()) }
125+
.build()
126+
127+
private fun BlobId.toProto(): Model.BlobId =
128+
Model.BlobId.newBuilder().setValue(bytes.toByteString()).build()
129+
130+
private fun BlobAccessContext.toProto(): Model.AccessContext? = when (this) {
131+
BlobAccessContext.Owned -> null
132+
is BlobAccessContext.Profile ->
133+
Model.AccessContext.newBuilder().setUserProfile(userId.asUserId()).build()
134+
is BlobAccessContext.Chat ->
135+
Model.AccessContext.newBuilder().setChat(chatId.asChatId()).build()
136+
is BlobAccessContext.ChatProfile ->
137+
Model.AccessContext.newBuilder().setChatProfile(chatId.asChatId()).build()
119138
}

‎services/flipcash/src/main/kotlin/com/flipcash/services/models/chat/BlobId.kt‎

Lines changed: 47 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,9 +2,53 @@ package com.flipcash.services.models.chat
22

33
import android.os.Parcelable
44
import kotlinx.parcelize.Parcelize
5+
import kotlinx.serialization.ExperimentalSerializationApi
6+
import kotlinx.serialization.KSerializer
57
import kotlinx.serialization.Serializable
8+
import kotlinx.serialization.builtins.ByteArraySerializer
9+
import kotlinx.serialization.descriptors.SerialDescriptor
10+
import kotlinx.serialization.encoding.Decoder
11+
import kotlinx.serialization.encoding.Encoder
612

13+
/**
14+
* Identifies one immutable set of blob bytes.
15+
*
16+
* Not a value class, because a value class may not override [equals], and the default over a
17+
* `ByteArray` compares array identity. Every read of a persisted [MediaItem] decodes fresh arrays,
18+
* so identity equality makes each read a distinct [MediaItem] — and Compose keys off that, so
19+
* `remember(media)` drops its state and cancels the effects holding it on every emission, which is
20+
* enough to stop a re-minted download URL from ever reaching the image loader.
21+
*
22+
* [BlobIdSerializer] keeps the wrapper invisible on the wire, which a value class got for free.
23+
*/
724
@Parcelize
8-
@Serializable
9-
@JvmInline
10-
value class BlobId(val bytes: ByteArray): Parcelable
25+
@Serializable(with = BlobIdSerializer::class)
26+
data class BlobId(val bytes: ByteArray) : Parcelable {
27+
28+
override fun equals(other: Any?): Boolean =
29+
this === other || (other is BlobId && bytes.contentEquals(other.bytes))
30+
31+
override fun hashCode(): Int = bytes.contentHashCode()
32+
}
33+
34+
/**
35+
* Encodes a [BlobId] as its bare bytes.
36+
*
37+
* `MediaItem`s are persisted as JSON (`user_profiles.profile_picture_json`, chat message contents),
38+
* and every row written while [BlobId] was a value class holds the id inlined — `"blobId":[1,2,…]`.
39+
* The class-shaped default would write `"blobId":{"bytes":[1,2,…]}` and fail to decode those rows,
40+
* which drops the whole `MediaItem` and leaves the surface with no image at all.
41+
*/
42+
object BlobIdSerializer : KSerializer<BlobId> {
43+
private val delegate = ByteArraySerializer()
44+
45+
@OptIn(ExperimentalSerializationApi::class)
46+
override val descriptor: SerialDescriptor =
47+
SerialDescriptor("com.flipcash.services.models.chat.BlobId", delegate.descriptor)
48+
49+
override fun serialize(encoder: Encoder, value: BlobId) =
50+
encoder.encodeSerializableValue(delegate, value.bytes)
51+
52+
override fun deserialize(decoder: Decoder): BlobId =
53+
BlobId(decoder.decodeSerializableValue(delegate))
54+
}

‎services/flipcash/src/test/kotlin/com/flipcash/services/internal/network/api/GetBlobsRequestSigningTest.kt‎

Lines changed: 94 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,94 @@
1+
package com.flipcash.services.internal.network.api
2+
3+
import com.codeinc.flipcash.gen.blob.v1.BlobStorageService as RpcBlobStorageService
4+
import com.codeinc.flipcash.gen.common.v1.Common
5+
import com.flipcash.services.models.chat.BlobAccessContext
6+
import com.flipcash.services.models.chat.BlobId
7+
import com.flipcash.services.models.chat.ChatId
8+
import org.junit.Test
9+
import kotlin.test.assertEquals
10+
import kotlin.test.assertFalse
11+
import kotlin.test.assertTrue
12+
13+
/**
14+
* `authenticate` signs the request as built so far and the server verifies against the whole
15+
* request minus the auth, so every field has to be set before the signature is taken. `GetBlobs`
16+
* set its access context afterwards, which made the server reject the call as `UNAUTHENTICATED`:
17+
* re-minting another user's expired avatar URL always failed, and group-chat sender avatars stayed
18+
* on their BlurHash. These assert on the bytes handed to the signer, so no signing key is needed.
19+
*/
20+
class GetBlobsRequestSigningTest {
21+
22+
private val blobId = BlobId(ByteArray(16) { 7 })
23+
24+
/** The message the signature is taken over, as the signer sees it. */
25+
private fun signedBytes(
26+
context: BlobAccessContext,
27+
): RpcBlobStorageService.GetBlobsRequest {
28+
var signed: ByteArray? = null
29+
getBlobsRequest(listOf(blobId), context) {
30+
signed = buildPartial().toByteArray()
31+
Common.Auth.getDefaultInstance()
32+
}
33+
return RpcBlobStorageService.GetBlobsRequest.parseFrom(
34+
requireNonNull(signed)
35+
)
36+
}
37+
38+
private fun requireNonNull(bytes: ByteArray?): ByteArray =
39+
bytes ?: error("the authenticator was never invoked")
40+
41+
@Test
42+
fun `a profile context is signed`() {
43+
val userId = List<Byte>(16) { 2 }
44+
45+
val signed = signedBytes(BlobAccessContext.Profile(userId))
46+
47+
assertTrue(signed.hasContext())
48+
assertEquals(userId, signed.context.userProfile.value.toByteArray().toList())
49+
}
50+
51+
@Test
52+
fun `a chat context is signed`() {
53+
val chatId = ChatId(ByteArray(32) { 1 })
54+
55+
val signed = signedBytes(BlobAccessContext.Chat(chatId))
56+
57+
assertTrue(signed.hasContext())
58+
assertEquals(chatId.bytes.toList(), signed.context.chat.value.toByteArray().toList())
59+
}
60+
61+
@Test
62+
fun `a chat profile context is signed`() {
63+
val chatId = ChatId(ByteArray(32) { 1 })
64+
65+
val signed = signedBytes(BlobAccessContext.ChatProfile(chatId))
66+
67+
assertTrue(signed.hasContext())
68+
assertEquals(chatId.bytes.toList(), signed.context.chatProfile.value.toByteArray().toList())
69+
}
70+
71+
@Test
72+
fun `the blob ids are signed`() {
73+
val signed = signedBytes(BlobAccessContext.Owned)
74+
75+
assertEquals(
76+
listOf(blobId.bytes.toList()),
77+
signed.blobIds.blobIdsList.map { it.value.toByteArray().toList() },
78+
)
79+
}
80+
81+
@Test
82+
fun `an owned read signs no context`() {
83+
val signed = signedBytes(BlobAccessContext.Owned)
84+
85+
assertFalse(signed.hasContext())
86+
}
87+
88+
@Test
89+
fun `the auth is not part of what is signed`() {
90+
val signed = signedBytes(BlobAccessContext.Profile(List<Byte>(16) { 2 }))
91+
92+
assertFalse(signed.hasAuth())
93+
}
94+
}

0 commit comments

Comments
 (0)