From 206c0979b1cf56c3d553517ca9850dab4ecb126d Mon Sep 17 00:00:00 2001 From: davotoula Date: Sat, 4 Jul 2026 14:34:05 +0200 Subject: [PATCH] =?UTF-8?q?Code=20review:=20-=20align=20voice-file=20debug?= =?UTF-8?q?=20log=20with=20deleteOrWarn's=20is-gone=20contract=20-=20Conve?= =?UTF-8?q?rt=20the=20delete-then-warn=20sites=20the=20sweep=20left=20hand?= =?UTF-8?q?-rolled=20in=20already=20=20=20touched=20files:=20ThumbnailDisk?= =?UTF-8?q?Cache=20corrupt-file=20and=20temp-thumbnail=20cleanup,=20=20=20?= =?UTF-8?q?NappletBlobCache.put=20leftover=20temp,=20and=20SecureKeyStorag?= =?UTF-8?q?e's=20bare=20delete=20of=20=20=20the=20fallback=20key=20file=20?= =?UTF-8?q?(the=20highest-stakes=20delete=20in=20that=20file).=20-=20Drop?= =?UTF-8?q?=20the=20exists()=20guards=20left=20layered=20over=20deleteOrWa?= =?UTF-8?q?rn=20=E2=80=94=20the=20helper=20=20=20already=20treats=20an=20a?= =?UTF-8?q?bsent=20file=20as=20silent=20success.=20-=20Collapse=20AccountM?= =?UTF-8?q?anager's=20legacy-file=20triple=20into=20a=20loop=20and=20drop?= =?UTF-8?q?=20the=20=20=20stale=20"silent"=20from=20its=20comment.=20-=20S?= =?UTF-8?q?napshot=20lastModified=20alongside=20length=20in=20NappletBlobC?= =?UTF-8?q?ache.trimToSize=20so=20=20=20sortedBy=20compares=20in-memory=20?= =?UTF-8?q?values=20instead=20of=20stat-ing=20per=20comparison.=20-=20Prom?= =?UTF-8?q?ote=20DesktopTorManager's=20private=20restrictToOwner=20into=20?= =?UTF-8?q?a=20shared=20=20=20File.restrictToOwner(tag)=20in=20commons=20(?= =?UTF-8?q?600=20files=20/=20700=20dirs)=20=E2=80=94=20the=20repo's=20=20?= =?UTF-8?q?=20sixth=20private=20copy=20of=20this=20pattern=20was=20one=20t?= =?UTF-8?q?oo=20many;=20the=20remaining=20copies=20=20=20can=20migrate=20i?= =?UTF-8?q?ncrementally?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../service/images/ThumbnailDiskCache.kt | 8 +-- .../loggedIn/home/VoiceReplyViewModel.kt | 6 +- .../commons/keystorage/SecureKeyStorage.kt | 8 +-- .../amethyst/commons/util/FilePermissions.kt | 58 +++++++++++++++++++ .../desktop/account/AccountManager.kt | 7 +-- .../service/drafts/DesktopDraftStore.kt | 2 +- .../service/media/VideoThumbnailCache.kt | 2 +- .../amethyst/desktop/tor/DesktopTorManager.kt | 24 +------- .../amethyst/napplethost/NappletBlobCache.kt | 9 ++- 9 files changed, 77 insertions(+), 47 deletions(-) create mode 100644 commons/src/jvmMain/kotlin/com/vitorpamplona/amethyst/commons/util/FilePermissions.kt diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/images/ThumbnailDiskCache.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/images/ThumbnailDiskCache.kt index 32b1c1fe70..67b86c2824 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/images/ThumbnailDiskCache.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/images/ThumbnailDiskCache.kt @@ -69,9 +69,7 @@ class ThumbnailDiskCache( BitmapFactory.decodeFile(file.absolutePath) } catch (e: Exception) { Log.w("ThumbnailDiskCache", "Failed to decode cached thumbnail, deleting: ${file.absolutePath}", e) - if (!file.delete()) { - Log.w("ThumbnailDiskCache") { "Failed to delete corrupt cache file: ${file.absolutePath}" } - } + file.deleteOrWarn("ThumbnailDiskCache", "corrupt cache file") null } } @@ -156,9 +154,7 @@ class ThumbnailDiskCache( scaled.recycle() if (!tempFile.renameTo(finalFile)) { Log.w("ThumbnailDiskCache") { "Failed to rename temp thumbnail to final: ${tempFile.absolutePath}" } - if (!tempFile.delete()) { - Log.w("ThumbnailDiskCache") { "Failed to delete temp thumbnail: ${tempFile.absolutePath}" } - } + tempFile.deleteOrWarn("ThumbnailDiskCache", "temp thumbnail") return false } diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/home/VoiceReplyViewModel.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/home/VoiceReplyViewModel.kt index ce3e28411b..40c5b3919a 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/home/VoiceReplyViewModel.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/home/VoiceReplyViewModel.kt @@ -156,10 +156,8 @@ class VoiceReplyViewModel : ViewModel() { private fun deleteVoiceLocalFile() { voiceLocalFile?.let { file -> try { - if (file.exists()) { - if (file.deleteOrWarn("VoiceReplyViewModel", "voice file")) { - Log.d("VoiceReplyViewModel") { "Deleted voice file: ${file.absolutePath}" } - } + if (file.deleteOrWarn("VoiceReplyViewModel", "voice file")) { + Log.d("VoiceReplyViewModel") { "Voice file removed or already gone: ${file.absolutePath}" } } } catch (e: Exception) { Log.w("VoiceReplyViewModel", "Failed to delete voice file: ${file.absolutePath}", e) diff --git a/commons/src/jvmMain/kotlin/com/vitorpamplona/amethyst/commons/keystorage/SecureKeyStorage.kt b/commons/src/jvmMain/kotlin/com/vitorpamplona/amethyst/commons/keystorage/SecureKeyStorage.kt index bcfe4a3509..00772efcec 100644 --- a/commons/src/jvmMain/kotlin/com/vitorpamplona/amethyst/commons/keystorage/SecureKeyStorage.kt +++ b/commons/src/jvmMain/kotlin/com/vitorpamplona/amethyst/commons/keystorage/SecureKeyStorage.kt @@ -229,7 +229,7 @@ actual class SecureKeyStorage private actual constructor() { if (existed) { if (data.isEmpty()) { - fallbackFile.delete() + fallbackFile.deleteOrWarn("SecureKeyStorage", "fallback key file") } else { atomicWriteFallbackData(fallbackFile, data) } @@ -276,10 +276,8 @@ actual class SecureKeyStorage private actual constructor() { StandardCopyOption.REPLACE_EXISTING, ) } finally { - // Clean up temp file if it still exists - if (tempFile.exists()) { - tempFile.deleteOrWarn("SecureKeyStorage", "temp key file") - } + // Clean up any leftover temp file + tempFile.deleteOrWarn("SecureKeyStorage", "temp key file") } } diff --git a/commons/src/jvmMain/kotlin/com/vitorpamplona/amethyst/commons/util/FilePermissions.kt b/commons/src/jvmMain/kotlin/com/vitorpamplona/amethyst/commons/util/FilePermissions.kt new file mode 100644 index 0000000000..0ebbd1861b --- /dev/null +++ b/commons/src/jvmMain/kotlin/com/vitorpamplona/amethyst/commons/util/FilePermissions.kt @@ -0,0 +1,58 @@ +/* + * 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.util + +import com.vitorpamplona.quartz.utils.Log +import java.io.File +import java.nio.file.Files +import java.nio.file.attribute.PosixFilePermission + +/** + * Restricts this file or directory to owner-only access (600 for files, + * 700 for directories), best-effort. + * + * Silent on filesystems without POSIX permissions (Windows), where the user + * profile's NTFS ACLs apply instead; warns when a POSIX filesystem refuses. + * + * @param tag the log tag of the calling component + */ +fun File.restrictToOwner(tag: String) { + try { + val permissions = + if (isDirectory) { + setOf( + PosixFilePermission.OWNER_READ, + PosixFilePermission.OWNER_WRITE, + PosixFilePermission.OWNER_EXECUTE, + ) + } else { + setOf( + PosixFilePermission.OWNER_READ, + PosixFilePermission.OWNER_WRITE, + ) + } + Files.setPosixFilePermissions(toPath(), permissions) + } catch (_: UnsupportedOperationException) { + // Windows: no POSIX permissions; the user profile's NTFS ACLs apply instead. + } catch (e: Exception) { + Log.w(tag, "Could not restrict permissions on $absolutePath", e) + } +} diff --git a/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/account/AccountManager.kt b/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/account/AccountManager.kt index 66b0924a06..068b5d1770 100644 --- a/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/account/AccountManager.kt +++ b/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/account/AccountManager.kt @@ -261,10 +261,9 @@ class AccountManager internal constructor( suspend fun loadSavedAccount(): Result = try { - // Clean up legacy files (one-time, silent) - File(amethystDir, "last_account.txt").deleteOrWarn("AccountManager", "legacy file") - File(amethystDir, "bunker_uri.txt").deleteOrWarn("AccountManager", "legacy file") - File(amethystDir, "nwc_connection.txt").deleteOrWarn("AccountManager", "legacy file") + // Clean up legacy files (one-time) + listOf("last_account.txt", "bunker_uri.txt", "nwc_connection.txt") + .forEach { File(amethystDir, it).deleteOrWarn("AccountManager", "legacy file") } // Single source of truth: accounts.json.enc val activeNpub = diff --git a/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/service/drafts/DesktopDraftStore.kt b/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/service/drafts/DesktopDraftStore.kt index 8271970177..536802c6be 100644 --- a/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/service/drafts/DesktopDraftStore.kt +++ b/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/service/drafts/DesktopDraftStore.kt @@ -250,7 +250,7 @@ class DesktopDraftStore( StandardCopyOption.REPLACE_EXISTING, ) } finally { - if (tempFile.exists()) tempFile.deleteOrWarn("DesktopDraftStore", "temp draft file") + tempFile.deleteOrWarn("DesktopDraftStore", "temp draft file") } } diff --git a/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/service/media/VideoThumbnailCache.kt b/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/service/media/VideoThumbnailCache.kt index be0e10bb3a..a7d28d7b9d 100644 --- a/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/service/media/VideoThumbnailCache.kt +++ b/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/service/media/VideoThumbnailCache.kt @@ -167,7 +167,7 @@ object VideoThumbnailCache { val hash = sha1Hex(url) val cached = File(downloadCacheDir, "$hash.mp4") if (cached.length() > 0L) return Download(cached, persistable = true) - if (cached.exists()) cached.deleteOrWarn("VideoThumbnailCache", "empty cached chunk") + cached.deleteOrWarn("VideoThumbnailCache", "empty cached chunk") var wrote = false var rangeHonored = false diff --git a/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/tor/DesktopTorManager.kt b/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/tor/DesktopTorManager.kt index e8881b6375..2742782a25 100644 --- a/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/tor/DesktopTorManager.kt +++ b/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/tor/DesktopTorManager.kt @@ -23,6 +23,7 @@ package com.vitorpamplona.amethyst.desktop.tor import com.vitorpamplona.amethyst.commons.tor.ITorManager import com.vitorpamplona.amethyst.commons.tor.TorServiceStatus import com.vitorpamplona.amethyst.commons.tor.TorType +import com.vitorpamplona.amethyst.commons.util.restrictToOwner import com.vitorpamplona.quartz.utils.Log import io.matthewnelson.kmp.tor.resource.exec.tor.ResourceLoaderTorExec import io.matthewnelson.kmp.tor.runtime.Action.Companion.startDaemonAsync @@ -46,8 +47,6 @@ import kotlinx.coroutines.flow.map import kotlinx.coroutines.flow.stateIn import kotlinx.coroutines.launch import java.io.File -import java.nio.file.Files -import java.nio.file.attribute.PosixFilePermission /** * Desktop Tor daemon manager using kmp-tor. @@ -192,7 +191,8 @@ class DesktopTorManager( private fun desktopEnvironment(): TorRuntime.Environment { val appDir = torDataDirectory() appDir.mkdirs() - restrictToOwner(appDir) + // Owner-only (700) — Tor state includes onion keys. + appDir.restrictToOwner("DesktopTorManager") return TorRuntime.Environment.Builder( workDirectory = appDir.resolve("work"), @@ -201,24 +201,6 @@ class DesktopTorManager( ) {} } - /** Restricts [dir] to owner only (700) — Tor state includes onion keys. */ - private fun restrictToOwner(dir: File) { - try { - Files.setPosixFilePermissions( - dir.toPath(), - setOf( - PosixFilePermission.OWNER_READ, - PosixFilePermission.OWNER_WRITE, - PosixFilePermission.OWNER_EXECUTE, - ), - ) - } catch (e: UnsupportedOperationException) { - // Windows: no POSIX permissions; the user profile's NTFS ACLs apply instead. - } catch (e: Exception) { - Log.w("DesktopTorManager", "Could not restrict permissions on ${dir.absolutePath}", e) - } - } - /** OS-specific data directory for Tor. */ internal fun torDataDirectory(): File { val osName = System.getProperty("os.name", "").lowercase() diff --git a/nappletHost/src/main/kotlin/com/vitorpamplona/amethyst/napplethost/NappletBlobCache.kt b/nappletHost/src/main/kotlin/com/vitorpamplona/amethyst/napplethost/NappletBlobCache.kt index baf1109759..3439f537dc 100644 --- a/nappletHost/src/main/kotlin/com/vitorpamplona/amethyst/napplethost/NappletBlobCache.kt +++ b/nappletHost/src/main/kotlin/com/vitorpamplona/amethyst/napplethost/NappletBlobCache.kt @@ -22,7 +22,6 @@ package com.vitorpamplona.amethyst.napplethost import com.vitorpamplona.amethyst.commons.util.deleteOrWarn import com.vitorpamplona.quartz.nip01Core.core.toHexKey -import com.vitorpamplona.quartz.utils.Log import com.vitorpamplona.quartz.utils.sha256.sha256 import java.io.File @@ -56,8 +55,8 @@ class NappletBlobCache( dir.mkdirs() val tmp = File(dir, "$sha256.tmp.${System.nanoTime()}") tmp.writeBytes(bytes) - if (!tmp.renameTo(target) && !tmp.delete()) { - Log.w("NappletBlobCache") { "Failed to delete leftover temp file ${tmp.absolutePath} after a failed rename" } + if (!tmp.renameTo(target)) { + tmp.deleteOrWarn("NappletBlobCache", "leftover temp file") } } } @@ -69,10 +68,10 @@ class NappletBlobCache( dir .listFiles() ?.filter { it.isFile && !it.name.contains(".tmp.") } - ?.map { it to it.length() } ?: return + ?.map { Triple(it, it.length(), it.lastModified()) } ?: return var total = files.sumOf { it.second } if (total <= maxBytes) return - files.sortedBy { it.first.lastModified() }.forEach { (f, length) -> + files.sortedBy { it.third }.forEach { (f, length, _) -> if (total <= maxBytes) return if (f.deleteOrWarn("NappletBlobCache", "blob")) { total -= length