Code review:

- fix(media): stop ogg bypassing the ambiguity guard it is listed in
- fix(media): don't guess a family for an ambiguous bare subtype
- refactor(media): normalize the mime at the chokepoints, not one call site
This commit is contained in:
davotoula
2026-08-07 09:39:05 +02:00
parent 6472099d3a
commit ee4a5e5b89
8 changed files with 164 additions and 135 deletions
@@ -39,6 +39,7 @@ object ShareHelper {
private const val DEFAULT_IMAGE_EXTENSION = "jpg"
private const val DEFAULT_VIDEO_EXTENSION = "mp4"
private const val SHARED_FILE_PREFIX = "shared_media"
private const val GENERIC_BINARY_MIME_TYPE = "application/octet-stream"
data class SharableFile(
val uri: Uri,
@@ -70,23 +71,21 @@ object ShareHelper {
/**
* Picks the MIME type to put on an `ACTION_SEND` intent.
*
* `Intent.type` has to be a real `type/subtype`: an `IntentFilter` matches the two halves
* separately, so a slash-less value like `jpeg` matches nothing and the chooser opens empty —
* the share silently does nothing. Events can absolutely carry such a value, because NIP-92
* `imeta`/NIP-94 `m` tags are author-supplied and some clients write the bare subtype (Primal
* iOS emits `m jpeg`). Treat the declared type as a hint, not as truth.
*
* [fileExtension] is the safer signal — [getMediaExtension] sniffs it from the file's magic
* numbers rather than trusting the event — so it backs up an unusable declaration.
* `Intent.type` has to be a real `type/subtype` — an `IntentFilter` matches the two halves
* separately, so a slash-less value matches nothing and the chooser opens empty. The declared
* type is author-supplied and may be unusable (see [normalizeMimeType]), so it is treated as a
* hint; [fileExtension] is the safer signal because [getMediaExtension] sniffs it from the
* file's magic numbers rather than trusting the event.
*/
internal fun resolveShareMimeType(
declaredMimeType: String?,
fileExtension: String,
defaultTypePrefix: String,
): String =
normalizeMimeType(declaredMimeType)
?: mimeTypeMap[fileExtension.lowercase()]
?: "$defaultTypePrefix/$fileExtension"
// Unreachable today: getMediaExtension only ever returns keys of mimeTypeMap. Kept so
// the return type stays a well-formed MIME if that ever stops holding.
?: GENERIC_BINARY_MIME_TYPE
suspend fun getSharableUriFromUrl(
context: Context,
@@ -1103,7 +1103,7 @@ private suspend fun shareImageFile(
val (uri, fileExtension) = ShareHelper.getSharableUriFromUrl(context, videoUri)
// Determine mime type, use provided or derive from extension
val determinedMimeType = ShareHelper.resolveShareMimeType(mimeType, fileExtension, "image")
val determinedMimeType = ShareHelper.resolveShareMimeType(mimeType, fileExtension)
// Create share intent
val shareIntent =
@@ -1161,7 +1161,7 @@ private suspend fun shareVideoFile(
sharedFile = sharableFile
// Determine mime type
val determinedMimeType = ShareHelper.resolveShareMimeType(mimeType, extension, "video")
val determinedMimeType = ShareHelper.resolveShareMimeType(mimeType, extension)
// Create share intent
val shareIntent =
@@ -1227,7 +1227,7 @@ private suspend fun shareLocalVideoFile(
val (uri, extension) = ShareHelper.getSharableUriForLocalVideo(context, localFile)
// Determine mime type
val determinedMimeType = ShareHelper.resolveShareMimeType(mimeType, extension, "video")
val determinedMimeType = ShareHelper.resolveShareMimeType(mimeType, extension)
// Create share intent
val shareIntent =
@@ -168,35 +168,38 @@ class ShareHelperTest {
// `m jpeg`), so a bare subtype must be repaired rather than forwarded.
@Test
fun resolveShareMimeType_bareSubtype_isExpandedToFullMimeType() {
assertEquals("image/jpeg", ShareHelper.resolveShareMimeType("jpeg", "jpg", "image"))
assertEquals("video/mp4", ShareHelper.resolveShareMimeType("mp4", "mp4", "video"))
assertEquals("image/jpeg", ShareHelper.resolveShareMimeType("jpeg", "jpg"))
assertEquals("video/mp4", ShareHelper.resolveShareMimeType("mp4", "mp4"))
assertEquals("image/jpeg", ShareHelper.resolveShareMimeType("JPEG", "jpg"))
}
@Test
fun resolveShareMimeType_wellFormedDeclaration_isPreserved() {
assertEquals("image/png", ShareHelper.resolveShareMimeType("image/png", "png", "image"))
assertEquals("image/png", ShareHelper.resolveShareMimeType("image/png", "png"))
}
// An unusable declaration falls back to the extension, which is sniffed from the file's magic
// numbers and so is not attacker-controlled.
@Test
fun resolveShareMimeType_unrecognizableDeclaration_fallsBackToSniffedExtension() {
assertEquals("image/png", ShareHelper.resolveShareMimeType("notatype", "png", "image"))
assertEquals("image/png", ShareHelper.resolveShareMimeType("notatype", "png"))
assertEquals("image/png", ShareHelper.resolveShareMimeType("", "png"))
}
@Test
fun resolveShareMimeType_noDeclaration_usesCanonicalTypeForExtension() {
// Not "image/jpg" -- jpg is not a registered subtype.
assertEquals("image/jpeg", ShareHelper.resolveShareMimeType(null, "jpg", "image"))
assertEquals("video/quicktime", ShareHelper.resolveShareMimeType(null, "mov", "video"))
assertEquals("image/jpeg", ShareHelper.resolveShareMimeType(null, "jpg"))
assertEquals("video/quicktime", ShareHelper.resolveShareMimeType(null, "mov"))
}
// The invariant the share sheet depends on, pinned across the whole set of extensions
// getMediaExtension can sniff: whatever the event declared, Intent.type stays matchable.
@Test
fun resolveShareMimeType_alwaysProducesASlashSeparatedType() {
val cases = listOf(null, "", "jpeg", "image/jpeg", "notatype", "JPEG")
cases.forEach { declared ->
val resolved = ShareHelper.resolveShareMimeType(declared, "jpg", "image")
assertTrue("`$declared` resolved to un-matchable type `$resolved`", resolved.contains("/"))
listOf("jpg", "png", "gif", "webp", "webm", "avi", "mp4", "mov").forEach { extension ->
val resolved = ShareHelper.resolveShareMimeType("notatype", extension)
assertTrue("`$extension` resolved to un-matchable type `$resolved`", resolved.contains("/"))
}
}
}
@@ -44,10 +44,18 @@ abstract class MediaUrlContent(
dim: DimensionTag? = null,
blurhash: String? = null,
val uri: String? = null,
val mimeType: String? = null,
mimeType: String? = null,
thumbhash: String? = null,
val authorPubKey: String? = null,
) : BaseMediaContent(description, dim, blurhash, thumbhash)
) : BaseMediaContent(description, dim, blurhash, thumbhash) {
/**
* Repaired at construction rather than at each of the eight call sites that build a model from
* an author-supplied `m` tag — a bare subtype reaching this field is what puts `Intent.type =
* "jpeg"` on the share sheet (matching no `IntentFilter`) and what republishes the malformed
* tag under the user's own key when media is added to a gallery. See [normalizeMimeType].
*/
val mimeType: String? = normalizeMimeType(mimeType)
}
@Immutable
open class MediaUrlImage(
@@ -59,7 +59,7 @@ class RichTextParser {
val tags = eventTags.get(fullUrl)?.properties ?: emptyMap()
val contentType = normalizeMimeType(frags[MimeTypeTag.TAG_NAME] ?: tags[MimeTypeTag.TAG_NAME]?.firstOrNull())
val contentType = frags[MimeTypeTag.TAG_NAME] ?: tags[MimeTypeTag.TAG_NAME]?.firstOrNull()
// Returning null here drops the URL to a plain link, discarding the imeta's `dim`/blurhash
// and forcing a URL-preview round-trip to rediscover a type the imeta already declared —
@@ -557,10 +557,10 @@ class RichTextParser {
* Resolves which renderer can display a declared blob — the single decision every media
* renderer must make, from a NIP-94 `m` tag, a NIP-92 imeta, or a bare URL.
*
* A declared MIME type wins; the URL extension is the fallback both for the no-MIME case
* and for a *malformed* MIME (Primal iOS emits `m jpeg` rather than `m image/jpeg`, which
* matches no prefix below). `data:` URIs carry their type in the prefix, so a miss there is
* genuine and the base64 payload is never extension-probed.
* A declared MIME type wins, after [normalizeMimeType] repairs the bare-subtype form some
* clients emit; the URL extension is the fallback both for the no-MIME case and for a
* declaration too mangled to repair. `data:` URIs carry their type in the prefix, so a miss
* there is genuine and the base64 payload is never extension-probed.
*
* Returns **null** when nothing can render the file. Callers must not substitute a media
* kind for that null: handing an arbitrary blob — a webxdc app, a zip, an APK — to the
@@ -570,8 +570,9 @@ class RichTextParser {
*/
fun classifyMedia(
url: String,
mimeType: String?,
rawMimeType: String?,
): MediaContentKind? {
val mimeType = normalizeMimeType(rawMimeType)
if (mimeType != null) {
if (mimeType.startsWith("image/")) return MediaContentKind.IMAGE
// HLS playlists are advertised with a non-`video/*` MIME; see [isHlsMimeType].
@@ -666,6 +667,9 @@ val mimeTypeMap: Map<String, String> =
// Video
"mp4" to "video/mp4",
"webm" to "video/webm",
// Dead entry: "ogg" is re-keyed under Audio below and mapOf keeps the last, so every
// lookup of it yields audio/ogg. Kept only to show the extension is genuinely ambiguous —
// see [ambiguousMimeSubtypes]. Don't read this line as reachable.
"ogg" to "video/ogg",
"mov" to "video/quicktime",
"avi" to "video/x-msvideo",
@@ -682,6 +686,22 @@ val mimeTypeMap: Map<String, String> =
"pdf" to "application/pdf",
)
/**
* Subtypes that name more than one top-level type: `mpeg`, `mp4`, `ogg`, `webm` and `3gpp` all exist
* as both `audio/` and `video/`, so a bare token spelling one of them identifies no family on its
* own. See [normalizeMimeType] for what that costs them.
*/
private val ambiguousMimeSubtypes = setOf("mpeg", "mp4", "ogg", "webm", "3gpp")
/**
* The subtype half of every MIME in [mimeTypeMap], so a bare token can be looked up as what it
* actually is. [mimeTypeMap] is keyed by *extension*, which only doubles as a subtype index where
* the two spellings coincide — `quicktime`, `x-matroska` and `svg+xml` are subtypes no extension
* spells. Consulted after [mimeTypeMap] so the extension spelling keeps priority where they
* disagree (`mp4` stays `video/mp4` rather than the later `audio/mp4` entry).
*/
private val mimeSubtypeMap: Map<String, String> = mimeTypeMap.values.associateBy { it.substringAfter('/') }
/**
* NIP-92's `m` property is meant to carry a full `type/subtype`, but several clients emit the
* bare subtype instead — Primal iOS writes `m jpeg` rather than `m image/jpeg`. That value is
@@ -690,14 +710,28 @@ val mimeTypeMap: Map<String, String> =
* `Intent.type = "jpeg"`. No `<data android:mimeType>` filter matches a type without a slash,
* so the share sheet opens with zero targets and the image cannot be shared at all.
*
* Map a bare subtype back onto its canonical MIME so every downstream consumer (the share
* intent, the gallery entry's published `m` tag, the player's type hint) sees a well-formed
* value. Anything already containing a `/` is passed through untouched, and an unrecognised
* bare token is dropped to null rather than propagated — that leaves the caller's
* extension-based detection to decide, which is strictly better than carrying garbage forward.
* Map a bare subtype back onto its canonical MIME. This is called from the two chokepoints every
* `m` value passes through — [RichTextParser.classifyMedia] for the render decision and
* [MediaUrlContent] for the value the share intent and the gallery entry's republished `m` tag
* read — so consumers see a well-formed type without each having to remember to repair it.
*
* Anything already containing a `/` is passed through untouched, and an unrecognised bare token is
* dropped to null rather than propagated — that leaves the caller's extension-based detection to
* decide, which is strictly better than carrying garbage forward.
*
* The same refusal covers a token in [ambiguousMimeSubtypes] that would land in `audio/`. `audio/`
* is the one destructive family: it is what [RichTextParser.isAudioContent] reads to drop the
* picture, so guessing it for what may be a video loses content, while guessing `video/` for what
* may be audio only costs some chrome. That asymmetry is why the guard is one-sided rather than a
* blanket refusal — `mp4` and `webm` resolve to `video/` and keep their rescue, so an extensionless
* Blossom URL declaring `m mp4` still renders, whereas `ogg` (whose only live [mimeTypeMap] entry
* is `audio/ogg`, its `video/ogg` one being a dead duplicate key) and `mpeg` decline.
*/
fun normalizeMimeType(rawMimeType: String?): String? {
if (rawMimeType == null) return null
if (rawMimeType.contains('/')) return rawMimeType
return mimeTypeMap[rawMimeType.lowercase()]
val token = rawMimeType.lowercase()
val resolved = mimeTypeMap[token] ?: mimeSubtypeMap[token] ?: return null
if (token in ambiguousMimeSubtypes && resolved.startsWith("audio/")) return null
return resolved
}
@@ -23,7 +23,9 @@ package com.vitorpamplona.amethyst.commons.richtext
import com.vitorpamplona.quartz.nip92IMeta.IMetaTag
import kotlin.test.Test
import kotlin.test.assertEquals
import kotlin.test.assertFalse
import kotlin.test.assertNull
import kotlin.test.assertTrue
class ClassifyMediaTest {
@Test
@@ -97,10 +99,67 @@ class ClassifyMediaTest {
}
@Test
fun extensionRescuesAMalformedMime() {
// Primal iOS emits `m jpeg` instead of `m image/jpeg`; the extension must still win
// over "unknown". Preserves the behaviour createMediaContent already documented.
fun aMalformedMimeIsRepairedRatherThanIgnored() {
// Primal iOS emits `m jpeg` instead of `m image/jpeg`. The bare subtype is mapped back to
// its canonical MIME here, so it classifies even on the extensionless URLs Blossom hands
// out, where the imeta is the only type signal there is.
assertEquals(MediaContentKind.IMAGE, RichTextParser.classifyMedia("https://x.com/a.jpg", "jpeg"))
assertEquals(MediaContentKind.IMAGE, RichTextParser.classifyMedia("https://x.com/abcd1234", "jpeg"))
assertEquals(MediaContentKind.VIDEO, RichTextParser.classifyMedia("https://x.com/abcd1234", "quicktime"))
// A token that maps to no known type has nothing to recover from; the extension decides.
assertNull(RichTextParser.classifyMedia("https://x.com/abcd1234", "notarealtype"))
assertEquals(MediaContentKind.IMAGE, RichTextParser.classifyMedia("https://x.com/a.jpg", "notarealtype"))
}
// A subtype that names more than one top-level type identifies no family on its own. Guessing
// one is worse than not repairing: `audio/mpeg` classifies as VIDEO either way, but it also
// satisfies isAudioContent, which strips the picture and renders an MPEG video as a bare audio
// track. Leaving it unresolved hands the decision back to the extension, which knows.
@Test
fun anAmbiguousBareSubtypeIsLeftForTheExtensionToDecide() {
assertNull(normalizeMimeType("mpeg"), "`mpeg` names both audio/mpeg and video/mpeg")
// `ogg` reaches the same guard even though mimeTypeMap answers it: the extension table
// holds audio/ogg (its video/ogg entry is a dead duplicate key), so short-circuiting there
// is exactly the audio mis-flag this guard exists to stop.
assertNull(normalizeMimeType("ogg"), "`ogg` names both audio/ogg and video/ogg")
val url = "https://x.com/clip.mpg"
val media = RichTextParser().createMediaContent(url, mapOf(url to imeta(url, "mpeg")), null)
assertTrue(media is MediaUrlVideo, "the .mpg extension still classifies it")
assertFalse(RichTextParser.isAudioContent(media.mimeType, url), "an MPEG video is not an audio track")
// The case that actually reached a user: an OGG video must not be flagged as an audio track.
val ogv = "https://x.com/clip.ogv"
assertFalse(
RichTextParser.isAudioContent(normalizeMimeType("ogg"), ogv),
"an OGG video must not be rendered as a pictureless audio track",
)
}
// Declining is reserved for the destructive direction. `audio/` is the one family that strips
// the picture, so an ambiguous token whose extension spelling already resolves to `video/`
// keeps its rescue — an extensionless Blossom URL with `m mp4` still renders.
@Test
fun anAmbiguousSubtypeThatResolvesToVideoKeepsItsRescue() {
assertEquals("video/mp4", normalizeMimeType("mp4"))
assertEquals("video/webm", normalizeMimeType("webm"))
assertEquals(MediaContentKind.VIDEO, RichTextParser.classifyMedia("https://x.com/abcd1234", "mp4"))
// Unambiguously-audio tokens are untouched by the guard.
assertEquals("audio/mpeg", normalizeMimeType("mp3"))
assertEquals("audio/flac", normalizeMimeType("flac"))
}
// The unambiguous half must keep working: these are subtypes no extension in the table spells,
// so they resolve only through the subtype index.
@Test
fun anUnambiguousBareSubtypeStillResolves() {
assertEquals("video/quicktime", normalizeMimeType("quicktime"))
assertEquals("video/x-matroska", normalizeMimeType("x-matroska"))
assertEquals("image/svg+xml", normalizeMimeType("svg+xml"))
// An extension spelling that is also a subtype keeps the extension's family.
assertEquals("video/mp4", normalizeMimeType("mp4"))
}
@Test
@@ -135,6 +194,9 @@ class ClassifyMediaTest {
"https://x.com/a.xdc" to "application/x-webxdc",
"https://x.com/a.zip" to "application/zip",
"https://x.com/a.jpg" to "jpeg",
"https://x.com/abcd1234" to "jpeg",
"https://x.com/abcd1234" to "notarealtype",
"https://x.com/clip.mpg" to "mpeg",
"data:image/png;base64,AAAA" to null,
"data:application/zip;base64,AAAAmp4" to null,
)
@@ -67,11 +67,10 @@ class PdfParserTest {
assertTrue(RichTextParser.isPdfUrl("https://example.com/doc.pdf?sig=abc"))
}
// Primal iOS writes a bare subtype (`m jpeg`) instead of a full MIME (`m image/jpeg`).
// The bare subtype matches none of the `image/`/`video/`/`application/pdf` prefixes, so before
// the extension fallback the whole imeta was dropped: the URL rendered as a plain link, losing
// the `dim` needed to reserve the image's height (feed jump) and forcing a URL-preview fetch.
// The `.jpg` extension must still route it to a MediaUrlImage that carries `dim`.
// Primal iOS writes a bare subtype (`m jpeg`) instead of a full MIME (`m image/jpeg`); see
// normalizeMimeType. End-to-end here because the failure was never just the render decision:
// dropping the imeta also lost the `dim` that reserves the image's height (feed jump) and
// forced a URL-preview fetch to rediscover a type the imeta had already declared.
@Test
fun detectsImageFromMalformedImetaMimeWithImageExtension() {
val url = "https://blossom.primal.net/33e7c01afbea894a64e1db44dece460b09a2426108f47143754e1cf4bfdf747c.jpg"
@@ -88,6 +87,9 @@ class PdfParserTest {
val imageMedia = state.mediaForPager[url]
assertTrue(imageMedia is MediaUrlImage, "Expected MediaUrlImage despite the malformed `m jpeg` mime")
// Repaired on the model too, not just for the render decision: this field becomes
// Intent.type when the image is shared, and a slash-less one matches no IntentFilter.
assertEquals("image/jpeg", imageMedia.mimeType, "The bare subtype must not survive onto the model")
assertEquals("1009x680", imageMedia.dim?.toString(), "The imeta dim must survive so the loader can reserve space")
assertEquals(1009f / 680f, imageMedia.dim?.aspectRatio())
@@ -112,37 +114,20 @@ class PdfParserTest {
assertTrue(videoMedia is MediaUrlVideo, "Expected MediaUrlVideo despite the malformed `m mp4` mime")
}
// A bare-subtype mime is recovered from the imeta itself, so an extensionless URL — the shape
// Blossom hands out, where the imeta is the only type signal there is — still renders.
// An extensionless URL is the shape Blossom hands out, where the imeta is the only type signal
// there is: a recognizable bare subtype now carries it, and an unrecognizable one leaves
// nothing to recover from. The second half documents the limit so it isn't read as a
// regression.
@Test
fun malformedImetaMimeWithoutExtensionIsRecoveredFromTheMime() {
fun malformedImetaMimeWithoutExtensionIsRecoveredFromTheMimeAlone() {
val url = "https://files.example.com/abcd1234"
val tags =
ImmutableListOfLists(
arrayOf(
arrayOf("imeta", "url $url", "m jpeg"),
),
)
val state = RichTextParser().parseText(url, tags, null)
fun parse(mime: String) =
RichTextParser()
.parseText(url, ImmutableListOfLists(arrayOf(arrayOf("imeta", "url $url", "m $mime"))), null)
.mediaForPager[url]
assertTrue(state.mediaForPager[url] is MediaUrlImage, "`m jpeg` alone is enough to classify the media")
}
// The boundary: a bare token that maps to no known type, on a URL with no extension, has
// nothing left to recover from. This documents the limit so it isn't mistaken for a regression.
@Test
fun unrecognizableImetaMimeWithoutExtensionStaysUnclassified() {
val url = "https://files.example.com/abcd1234"
val tags =
ImmutableListOfLists(
arrayOf(
arrayOf("imeta", "url $url", "m notarealtype"),
),
)
val state = RichTextParser().parseText(url, tags, null)
assertEquals(null, state.mediaForPager[url], "Nothing to recover from -> not treated as media")
assertTrue(parse("jpeg") is MediaUrlImage, "`m jpeg` alone is enough to classify the media")
assertEquals(null, parse("notarealtype"), "Nothing to recover from -> not treated as media")
}
}
@@ -1,62 +0,0 @@
/*
* Copyright (c) 2025 Vitor Pamplona
*
* Permission is hereby granted, free of charge, to any person obtaining a copy of
* this software and associated documentation files (the "Software"), to deal in
* the Software without restriction, including without limitation the rights to use,
* copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the
* Software, and to permit persons to whom the Software is furnished to do so,
* subject to the following conditions:
*
* The above copyright notice and this permission notice shall be included in all
* copies or substantial portions of the Software.
*
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS
* FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR
* COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN
* AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION
* WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
*/
package com.vitorpamplona.amethyst.commons.richtext
import com.vitorpamplona.quartz.nip92IMeta.IMetaTag
import kotlin.test.Test
import kotlin.test.assertEquals
import kotlin.test.assertTrue
class RichTextParserMalformedMimeTest {
private val url = "https://blossom.primal.net/c27d7b7be6e58d69b29006cc275d29d67967760b1772e50a79d5b24f60d62fc5.jpg"
private fun parse(mime: String): MediaUrlContent? =
RichTextParser().createMediaContent(
fullUrl = url,
eventTags =
mapOf(
url to
IMetaTag(
url = url,
properties = mapOf("m" to listOf(mime), "dim" to listOf("960.0x1358.0")),
),
),
description = null,
)
@Test
fun malformedImetaMimeStillRendersAsImage() {
val content = parse("jpeg")
assertTrue(content is MediaUrlImage, "bare `m jpeg` must still route to MediaUrlImage")
}
@Test
fun malformedImetaMimeIsNormalizedForSharing() {
val content = parse("jpeg") as MediaUrlImage
assertEquals("image/jpeg", content.mimeType)
}
@Test
fun wellFormedImetaMimeIsUntouched() {
val content = parse("image/jpeg") as MediaUrlImage
assertEquals("image/jpeg", content.mimeType)
}
}