From 1a04f16d1a923df780a29ed73189eb248ac169ad Mon Sep 17 00:00:00 2001 From: davotoula Date: Sun, 18 Jan 2026 22:17:44 +0100 Subject: [PATCH] Safer temp video handling Cleanup deletes the actual shared file (and on error) instead of the old temp path in Guard against empty response.body Gate media-type detection so image sharing only checks image headers Added unit test for rename-failure fallback --- .../amethyst/ui/components/ShareHelper.kt | 97 ++++++++++++------- .../ui/components/ZoomableContentView.kt | 14 ++- .../amethyst/ui/components/ShareHelperTest.kt | 52 ++++++++++ 3 files changed, 125 insertions(+), 38 deletions(-) create mode 100644 amethyst/src/test/java/com/vitorpamplona/amethyst/ui/components/ShareHelperTest.kt diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/components/ShareHelper.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/components/ShareHelper.kt index 39d7c28ba..7e6a1cf7a 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/components/ShareHelper.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/components/ShareHelper.kt @@ -37,6 +37,12 @@ object ShareHelper { private const val DEFAULT_VIDEO_EXTENSION = "mp4" private const val SHARED_FILE_PREFIX = "shared_media" + data class SharableFile( + val uri: Uri, + val extension: String, + val file: File, + ) + // Image type magic numbers private val JPEG_MAGIC = byteArrayOf(0xFF.toByte(), 0xD8.toByte()) private val PNG_MAGIC = byteArrayOf(0x89.toByte(), 0x50.toByte(), 0x4E.toByte(), 0x47.toByte()) @@ -81,7 +87,7 @@ object ShareHelper { private fun getImageExtension(file: File): String = getMediaExtension(file, isVideo = false) - fun getVideoExtension(file: File): String = getMediaExtension(file, isVideo = true) + private fun getVideoExtension(file: File): String = getMediaExtension(file, isVideo = true) private fun getMediaExtension( file: File, @@ -97,43 +103,49 @@ object ShareHelper { return defaultExtension } - when { - // Image formats - // JPEG: Check first 2 bytes - matchesMagicNumbers(header, 0, JPEG_MAGIC) -> "jpg" + if (isVideo) { + when { + // Video formats + // WebM/MKV: Check first 4 bytes (EBML header) + // Both use Matroska container; default to webm as it's more common on web + matchesMagicNumbers(header, 0, WEBM_MAGIC) -> "webm" - // PNG: Check first 4 bytes - matchesMagicNumbers(header, 0, PNG_MAGIC) -> "png" + // AVI: Check "RIFF" (bytes 0-3) and "AVI " (bytes 8-11) + matchesMagicNumbers(header, 0, AVI_HEADER_START) && + bytesRead >= 12 && + matchesMagicNumbers(header, 8, AVI_HEADER_END) -> "avi" - // GIF: Check first 4 bytes for "GIF8" - matchesMagicNumbers(header, 0, GIF_MAGIC) -> "gif" + // MP4/MOV: Check for ftyp box (bytes 4-7 should be "ftyp") + bytesRead >= 12 && matchesMagicNumbers(header, 4, MOV_FTYP) -> detectMp4OrMov(header) - // WEBP: Check "RIFF" (bytes 0-3) and "WEBP" (bytes 8-11) - matchesMagicNumbers(header, 0, WEBP_HEADER_START) && - bytesRead >= 12 && - matchesMagicNumbers(header, 8, WEBP_HEADER_END) -> "webp" + // MP4/MOV alternative: moov, mdat, or free at offset 4 + bytesRead >= 8 && ( + matchesMagicNumbers(header, 4, MOV_MOOV) || + matchesMagicNumbers(header, 4, MOV_MDAT) || + matchesMagicNumbers(header, 4, MOV_FREE) + ) -> "mp4" - // Video formats - // WebM/MKV: Check first 4 bytes (EBML header) - // Both use Matroska container; default to webm as it's more common on web - matchesMagicNumbers(header, 0, WEBM_MAGIC) -> "webm" + else -> defaultExtension + } + } else { + when { + // Image formats + // JPEG: Check first 2 bytes + matchesMagicNumbers(header, 0, JPEG_MAGIC) -> "jpg" - // AVI: Check "RIFF" (bytes 0-3) and "AVI " (bytes 8-11) - matchesMagicNumbers(header, 0, AVI_HEADER_START) && - bytesRead >= 12 && - matchesMagicNumbers(header, 8, AVI_HEADER_END) -> "avi" + // PNG: Check first 4 bytes + matchesMagicNumbers(header, 0, PNG_MAGIC) -> "png" - // MP4/MOV: Check for ftyp box (bytes 4-7 should be "ftyp") - bytesRead >= 12 && matchesMagicNumbers(header, 4, MOV_FTYP) -> detectMp4OrMov(header) + // GIF: Check first 4 bytes for "GIF8" + matchesMagicNumbers(header, 0, GIF_MAGIC) -> "gif" - // MP4/MOV alternative: moov, mdat, or free at offset 4 - bytesRead >= 8 && ( - matchesMagicNumbers(header, 4, MOV_MOOV) || - matchesMagicNumbers(header, 4, MOV_MDAT) || - matchesMagicNumbers(header, 4, MOV_FREE) - ) -> "mp4" + // WEBP: Check "RIFF" (bytes 0-3) and "WEBP" (bytes 8-11) + matchesMagicNumbers(header, 0, WEBP_HEADER_START) && + bytesRead >= 12 && + matchesMagicNumbers(header, 8, WEBP_HEADER_END) -> "webp" - else -> defaultExtension + else -> defaultExtension + } } } } catch (e: IOException) { @@ -209,21 +221,36 @@ object ShareHelper { fun prepareTempVideoForSharing( context: Context, tempFile: File, - ): Pair { + ): SharableFile { val extension = getVideoExtension(tempFile) val timestamp = System.currentTimeMillis() val sharableFile = File(context.cacheDir, "${SHARED_FILE_PREFIX}_$timestamp.$extension") try { - tempFile.renameTo(sharableFile) + moveTempFileForSharing(tempFile, sharableFile) } catch (e: Exception) { Log.e(TAG, "Failed to rename temp video file for sharing", e) throw e } - return Pair( - FileProvider.getUriForFile(context, "${context.packageName}.provider", sharableFile), - extension, + return SharableFile( + uri = FileProvider.getUriForFile(context, "${context.packageName}.provider", sharableFile), + extension = extension, + file = sharableFile, ) } + + internal fun moveTempFileForSharing( + tempFile: File, + sharableFile: File, + rename: (File, File) -> Boolean = { source, dest -> source.renameTo(dest) }, + ) { + val renamed = rename(tempFile, sharableFile) + if (!renamed) { + tempFile.copyTo(sharableFile, overwrite = true) + if (!tempFile.delete()) { + Log.w(TAG, "Failed to delete temp file ${tempFile.path} after copy") + } + } + } } diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/components/ZoomableContentView.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/components/ZoomableContentView.kt index 0127af4e3..48e0a2cfe 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/components/ZoomableContentView.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/components/ZoomableContentView.kt @@ -121,6 +121,7 @@ import okhttp3.Request import okhttp3.coroutines.executeAsync import okio.sink import java.io.File +import java.io.IOException import kotlin.time.Duration.Companion.seconds @Composable @@ -884,6 +885,7 @@ private suspend fun shareVideoFile( onError: () -> Unit, ) { val tempFile = ShareHelper.createTempVideoFile(context) + var sharedFile: File? = null try { withContext(Dispatchers.IO) { // Download video using streaming @@ -897,15 +899,20 @@ private suspend fun shareVideoFile( client.newCall(request).executeAsync().use { response -> check(response.isSuccessful) { "Download failed: ${response.code}" } + val responseBody = response.body // Stream the response to the temp file tempFile.outputStream().use { outputStream -> - response.body.source().readAll(outputStream.sink()) + val bytesCopied = responseBody.source().readAll(outputStream.sink()) + if (bytesCopied == 0L) { + throw IOException("Download failed: empty response body") + } } } // Prepare the temp file for sharing (determines extension and creates sharable URI) - val (uri, extension) = ShareHelper.prepareTempVideoForSharing(context, tempFile) + val (uri, extension, sharableFile) = ShareHelper.prepareTempVideoForSharing(context, tempFile) + sharedFile = sharableFile // Determine mime type val determinedMimeType = mimeType ?: "video/$extension" @@ -924,7 +931,7 @@ private suspend fun shareVideoFile( // Schedule cleanup after 60 seconds to allow the receiving app time to copy the file Handler(Looper.getMainLooper()).postDelayed({ - tempFile.delete() + sharableFile.delete() }, 60_000) } @@ -937,6 +944,7 @@ private suspend fun shareVideoFile( // Clean up temp file on error tempFile.delete() + sharedFile?.delete() withContext(Dispatchers.Main) { Toast diff --git a/amethyst/src/test/java/com/vitorpamplona/amethyst/ui/components/ShareHelperTest.kt b/amethyst/src/test/java/com/vitorpamplona/amethyst/ui/components/ShareHelperTest.kt new file mode 100644 index 000000000..fbc1a29da --- /dev/null +++ b/amethyst/src/test/java/com/vitorpamplona/amethyst/ui/components/ShareHelperTest.kt @@ -0,0 +1,52 @@ +/** + * 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.ui.components + +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Test +import java.io.File +import java.nio.file.Files + +class ShareHelperTest { + @Test + fun moveTempFileForSharing_renameFails_copiesAndDeletesSource() { + val tempDir = Files.createTempDirectory("sharehelpertest").toFile() + val tempFile = File(tempDir, "video.tmp") + val content = "test-content" + tempFile.writeText(content) + val targetFile = File(tempDir, "shared.mp4") + + ShareHelper.moveTempFileForSharing( + tempFile, + targetFile, + rename = { _, _ -> false }, + ) + + assertTrue(targetFile.exists()) + assertEquals(content, targetFile.readText()) + assertFalse(tempFile.exists()) + + targetFile.delete() + tempDir.delete() + } +}