Code review:

- align voice-file debug log with deleteOrWarn's is-gone contract
- Convert the delete-then-warn sites the sweep left hand-rolled in already
  touched files: ThumbnailDiskCache corrupt-file and temp-thumbnail cleanup,
  NappletBlobCache.put leftover temp, and SecureKeyStorage's bare delete of
  the fallback key file (the highest-stakes delete in that file).
- Drop the exists() guards left layered over deleteOrWarn — the helper
  already treats an absent file as silent success.
- Collapse AccountManager's legacy-file triple into a loop and drop the
  stale "silent" from its comment.
- Snapshot lastModified alongside length in NappletBlobCache.trimToSize so
  sortedBy compares in-memory values instead of stat-ing per comparison.
- Promote DesktopTorManager's private restrictToOwner into a shared
  File.restrictToOwner(tag) in commons (600 files / 700 dirs) — the repo's
  sixth private copy of this pattern was one too many; the remaining copies
  can migrate incrementally
This commit is contained in:
davotoula
2026-07-04 15:53:02 +02:00
parent 27650f2f77
commit 206c0979b1
9 changed files with 77 additions and 47 deletions
@@ -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
}
@@ -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)
@@ -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")
}
}
@@ -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)
}
}
@@ -261,10 +261,9 @@ class AccountManager internal constructor(
suspend fun loadSavedAccount(): Result<AccountState.LoggedIn> =
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 =
@@ -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")
}
}
@@ -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
@@ -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()
@@ -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