Repository navigation
fix(blobs): recover a group-chat avatar whose download URL expired - #1478
Merged
Merged
Conversation
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A sender's avatar in a group transcript stays on its BlurHash. A blob's
download_urlis a short-lived signed URL, so a persisted profile picture routinely carries a dead one by the time a row is read, and all three seams meant to re-mint it were broken.The access context was signed out
getBlobsset the access context aftersetAuth(authenticate(owner)).authenticatesignsbuildPartial(), so the signature covered{blobIds}while the server verified{blobIds, context}, and every re-mint came backUNAUTHENTICATED.BlobAccessContext.profile(userId)is what authorizes re-minting a blob the caller doesn't own, and group chat is the only surface that passes it — owned blobs fall back toOwnedand kept working, which is why this looked like a group-chat bug. Introduced by 36e7cef.The request is now built through a helper that sets every field before
authenticateruns.A cached 403 was served forever
NetworkFetcherwrites a response to the disk cache at line 110 and only throws on its status code at line 112, so one fetch through an expired URL leaves a 403 stored under the blob's key.ImmutableBlobCacheStrategyserved any cached response without a request, so that 403 replayed on every later load and no re-mint could reach it.A response
NetworkFetcherwould throw on is now neither served nor stored. Declining the write also leaves an entry an older build already poisoned intact rather than removed, which costs one doomed request per blob and then heals — Coil'sCacheStrategyhas no eviction hook.The re-mint was cancelled before it landed
BlobIdwas a@JvmInline value classoverByteArray, so it compared by array identity. That propagates:MediaItemRenditionandMediaItemare data classes, so every read of a persistedMediaItemdecoded fresh arrays and produced a value unequal to the last one.remember(media, targetLongestSidePx)inrememberMediaUrlre-keyed on each Room emission, allocating a newMediaUrlStateand cancelling theLaunchedEffectholding the re-mint. On device theGetBlobsRPC was sent and its result never applied.BlobIdis now a data class comparing bytes by content.Why BlobId carries a serializer
Dropping the value class changes how
BlobIdencodes, and that encoding is a storage format:MediaItemis persisted as JSON inuser_profiles.profile_picture_jsonand in chat message contents. Rows written by earlier builds hold the id inlined as\"blobId\":[1,2,…]; the class-shaped default writes\"blobId\":{\"bytes\":[1,2,…]}and fails to decode them, which drops the wholeMediaItemand leaves the avatar with no image at all — worse than the bug being fixed.BlobIdSerializerpins the bare-bytes encoding, andBlobIdSerializationTestdecodes a row in the old shape so this can't regress quietly.Tests
GetBlobsRequestSigningTest— 3 of 6 fail against the old field ordering.ImmutableBlobCacheStrategyTest— 2 of 5 fail against the old strategy.BlobIdEqualityTest— 3 of 5 fail against the value class.BlobIdSerializationTest— pins the persisted encoding.