Skip to content

Commit ca2e285

Browse files
mahibimahibi
committed
fix(mediaviewer): Key attachment cache files by fileId to avoid collisions
Two different attachments sharing a display name (e.g. a generically named pasted screenshot) resolved to the same file inside the shared attachment cache directory. Since the media viewer prefetches the tapped image's neighbors, a colliding neighbor's download could overwrite the cache file the viewer had just shown, making the open image flip to a different one moments later. Cache files for the media viewer are now nested under a per-fileId subdirectory so same-named attachments can no longer collide. Assisted-by: Claude Code:claude-sonnet-5 Signed-off-by: Marcel Hibbe <dev@mhibbe.de>
1 parent 84ef344 commit ca2e285

4 files changed

Lines changed: 74 additions & 7 deletions

File tree

‎app/src/main/java/com/nextcloud/talk/jobs/DownloadFileToCacheWorker.kt‎

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,7 @@ class DownloadFileToCacheWorker(val context: Context, workerParameters: WorkerPa
5959
val baseUrl = inputData.getString(KEY_BASE_URL)
6060
val userId = inputData.getString(KEY_USER_ID)
6161
val attachmentFolder = inputData.getString(KEY_ATTACHMENT_FOLDER)
62+
val fileId = inputData.getString(KEY_FILE_ID)
6263
val fileName = inputData.getString(KEY_FILE_NAME)
6364
val remotePath = inputData.getString(KEY_FILE_PATH)
6465
totalFileSize = (inputData.getLong(KEY_FILE_SIZE, -1))
@@ -72,24 +73,28 @@ class DownloadFileToCacheWorker(val context: Context, workerParameters: WorkerPa
7273

7374
val url = ApiUtils.getUrlForFileDownload(baseUrl, userId, remotePath)
7475

75-
return downloadFile(currentUser, url, fileName)
76+
return downloadFile(currentUser, url, fileId, fileName)
7677
} catch (e: IllegalStateException) {
7778
Log.e(javaClass.simpleName, "Something went wrong when trying to download file", e)
7879
return Result.failure()
7980
}
8081
}
8182

82-
private fun downloadFile(currentUser: User, url: String, fileName: String): Result {
83+
private fun downloadFile(currentUser: User, url: String, fileId: String?, fileName: String): Result {
8384
val downloadCall = ncApi.downloadFile(
8485
ApiUtils.getCredentials(currentUser.username, currentUser.token),
8586
url
8687
)
8788

88-
return executeDownload(downloadCall.execute().body(), fileName)
89+
return executeDownload(downloadCall.execute().body(), fileId, fileName)
8990
}
9091

91-
private fun executeDownload(body: ResponseBody?, fileName: String): Result {
92-
val targetFile = FileUtils.resolveSharedAttachmentFile(context.cacheDir, fileName)
92+
private fun executeDownload(body: ResponseBody?, fileId: String?, fileName: String): Result {
93+
val targetFile = if (fileId.isNullOrEmpty()) {
94+
FileUtils.resolveSharedAttachmentFile(context.cacheDir, fileName)
95+
} else {
96+
FileUtils.resolveSharedAttachmentFile(context.cacheDir, fileId, fileName)
97+
}
9398
if (body == null || targetFile == null) {
9499
if (body == null) {
95100
Log.e(TAG, "Response body when downloading $fileName is null!")
@@ -145,6 +150,7 @@ class DownloadFileToCacheWorker(val context: Context, workerParameters: WorkerPa
145150
const val KEY_BASE_URL = "KEY_BASE_URL"
146151
const val KEY_USER_ID = "KEY_USER_ID"
147152
const val KEY_ATTACHMENT_FOLDER = "KEY_ATTACHMENT_FOLDER"
153+
const val KEY_FILE_ID = "KEY_FILE_ID"
148154
const val KEY_FILE_NAME = "KEY_FILE_NAME"
149155
const val KEY_FILE_PATH = "KEY_FILE_PATH"
150156
const val KEY_FILE_SIZE = "KEY_FILE_SIZE"

‎app/src/main/java/com/nextcloud/talk/mediaviewer/viewmodels/MediaViewerViewModel.kt‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -127,7 +127,7 @@ class MediaViewerViewModel @Inject constructor(private val sharedItemsRepository
127127
}
128128

129129
val context = NextcloudTalkApplication.sharedApplication!!
130-
val existing = FileUtils.resolveSharedAttachmentFile(context.cacheDir, item.fileName)
130+
val existing = FileUtils.resolveSharedAttachmentFile(context.cacheDir, item.fileId, item.fileName)
131131
if (existing != null && existing.exists()) {
132132
_uiState.update {
133133
it.copy(cachedFilePaths = it.cachedFilePaths + (item.messageId to existing.absolutePath))
@@ -144,6 +144,7 @@ class MediaViewerViewModel @Inject constructor(private val sharedItemsRepository
144144
DownloadFileToCacheWorker.KEY_ATTACHMENT_FOLDER,
145145
CapabilitiesUtil.getAttachmentFolder(user.capabilities!!.spreedCapability!!)
146146
)
147+
.putString(DownloadFileToCacheWorker.KEY_FILE_ID, item.fileId)
147148
.putString(DownloadFileToCacheWorker.KEY_FILE_NAME, item.fileName)
148149
.putString(DownloadFileToCacheWorker.KEY_FILE_PATH, item.path)
149150
.putLong(DownloadFileToCacheWorker.KEY_FILE_SIZE, item.fileSize)
@@ -161,7 +162,11 @@ class MediaViewerViewModel @Inject constructor(private val sharedItemsRepository
161162
if (workInfo == null) return@collect
162163
when (workInfo.state) {
163164
WorkInfo.State.SUCCEEDED -> {
164-
val downloaded = FileUtils.resolveSharedAttachmentFile(context.cacheDir, item.fileName)
165+
val downloaded = FileUtils.resolveSharedAttachmentFile(
166+
context.cacheDir,
167+
item.fileId,
168+
item.fileName
169+
)
165170
_uiState.update {
166171
it.copy(
167172
downloadingMessageIds = it.downloadingMessageIds - item.messageId,

‎app/src/main/java/com/nextcloud/talk/utils/FileUtils.kt‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,21 @@ object FileUtils {
4848
return resolveFileInDirectory(sharedDirectory, untrustedFileName)
4949
}
5050

51+
/**
52+
* Resolves a shared attachment file inside a per-[fileId] subdirectory of the dedicated cache
53+
* directory. [untrustedFileName] alone is sender-controlled and not unique - two different
54+
* attachments (e.g. two pasted screenshots both named "image.png") can share it, which would
55+
* otherwise let concurrent downloads (see MediaViewerViewModel's prefetch) resolve to, and
56+
* overwrite, the very same cache file. Nesting under the server-assigned [fileId] keeps every
57+
* attachment's cache slot unique regardless of what its sender named it.
58+
*/
59+
fun resolveSharedAttachmentFile(cacheDir: File, fileId: String, untrustedFileName: String?): File? {
60+
val sharedDirectory = getSharedAttachmentsDirectory(cacheDir) ?: return null
61+
val fileIdDirectory = resolveFileInDirectory(sharedDirectory, fileId) ?: return null
62+
if (!fileIdDirectory.exists() && !fileIdDirectory.mkdirs()) return null
63+
return resolveFileInDirectory(fileIdDirectory, untrustedFileName)
64+
}
65+
5166
/**
5267
* Resolves an untrusted file name inside [baseDirectory] and rejects traversal attempts.
5368
*/

‎app/src/test/java/com/nextcloud/talk/utils/FileUtilsTest.kt‎

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -117,6 +117,47 @@ class FileUtilsTest {
117117
}
118118
}
119119

120+
@Test
121+
fun resolveSharedAttachmentFile_withFileId_resolvesInsidePerFileIdSubDirectory() {
122+
val baseDir = createTempDir()
123+
try {
124+
val result = FileUtils.resolveSharedAttachmentFile(baseDir, "42", "example.pdf")
125+
126+
assertNotNull(result)
127+
val expected = File(baseDir, "shared_attachments/42/example.pdf").canonicalPath
128+
assertEquals(expected, result?.canonicalPath)
129+
} finally {
130+
baseDir.deleteRecursively()
131+
}
132+
}
133+
134+
@Test
135+
fun resolveSharedAttachmentFile_withFileId_keepsDifferentFileIdsFromColliding() {
136+
val baseDir = createTempDir()
137+
try {
138+
val first = FileUtils.resolveSharedAttachmentFile(baseDir, "1", "image.jpg")
139+
val second = FileUtils.resolveSharedAttachmentFile(baseDir, "2", "image.jpg")
140+
141+
assertNotNull(first)
142+
assertNotNull(second)
143+
assertTrue(first?.canonicalPath != second?.canonicalPath)
144+
} finally {
145+
baseDir.deleteRecursively()
146+
}
147+
}
148+
149+
@Test
150+
fun resolveSharedAttachmentFile_withFileId_rejectsTraversalInFileId() {
151+
val baseDir = createTempDir()
152+
try {
153+
val result = FileUtils.resolveSharedAttachmentFile(baseDir, "../evil", "example.pdf")
154+
155+
assertNull(result)
156+
} finally {
157+
baseDir.deleteRecursively()
158+
}
159+
}
160+
120161
@Test
121162
fun getSharedAttachmentsDirectory_returnsNullWhenPathIsFile() {
122163
val baseDir = createTempDir()

0 commit comments

Comments
 (0)