Skip to content

Commit 5166fe7

Browse files
mahibimahibi
authored
Merge pull request #6635 from nextcloud/bugfix/noid/wrongImagesShown
Bugfix/noid/wrong images shown
2 parents 84ef344 + 704697b commit 5166fe7

5 files changed

Lines changed: 91 additions & 26 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/activities/MediaViewerScreen.kt‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -120,13 +120,13 @@ fun MediaViewerScreen(
120120
}
121121
val coroutineScope = rememberCoroutineScope()
122122

123-
// See MediaViewerViewModel.indexShiftEvents: prepending older groups shifts every existing
124-
// index, so the pager must silently jump to keep the same item on screen.
125-
LaunchedEffect(viewModel) {
126-
viewModel.indexShiftEvents.collect { shift ->
127-
if (shift != 0) {
128-
pagerState.scrollToPage((pagerState.currentPage + shift).coerceIn(0, items.size - 1))
123+
LaunchedEffect(uiState.pendingShift?.id) {
124+
val shift = uiState.pendingShift
125+
if (shift != null) {
126+
if (shift.amount != 0) {
127+
pagerState.scrollToPage((pagerState.currentPage + shift.amount).coerceIn(0, items.size - 1))
129128
}
129+
viewModel.consumePendingShift(shift.id)
130130
}
131131
}
132132

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

Lines changed: 18 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,6 @@ import com.nextcloud.talk.shareditems.repositories.SharedItemsRepository
3030
import com.nextcloud.talk.utils.CapabilitiesUtil
3131
import com.nextcloud.talk.utils.FileUtils
3232
import io.reactivex.Observable
33-
import kotlinx.coroutines.flow.MutableSharedFlow
3433
import kotlinx.coroutines.flow.MutableStateFlow
3534
import kotlinx.coroutines.flow.StateFlow
3635
import kotlinx.coroutines.flow.collect
@@ -52,13 +51,16 @@ import kotlin.coroutines.resumeWithException
5251
class MediaViewerViewModel @Inject constructor(private val sharedItemsRepository: SharedItemsRepository) :
5352
ViewModel() {
5453

54+
data class PendingShift(val id: Long, val amount: Int)
55+
5556
data class UiState(
5657
val groups: List<MediaViewerGroup> = emptyList(),
5758
val currentGlobalIndex: Int = 0,
5859
val loadingOlder: Boolean = false,
5960
val canLoadOlder: Boolean = true,
6061
val cachedFilePaths: Map<Long, String> = emptyMap(),
61-
val downloadingMessageIds: Set<Long> = emptySet()
62+
val downloadingMessageIds: Set<Long> = emptySet(),
63+
val pendingShift: PendingShift? = null
6264
) {
6365
val flattenedItems: List<MediaViewerItem> get() = groups.flatMap { it.items }
6466
val currentItem: MediaViewerItem? get() = flattenedItems.getOrNull(currentGlobalIndex)
@@ -71,14 +73,7 @@ class MediaViewerViewModel @Inject constructor(private val sharedItemsRepository
7173

7274
private val _uiState = MutableStateFlow(UiState())
7375
val uiState: StateFlow<UiState> = _uiState
74-
75-
// HorizontalPager indexes into the flattened item list; prepending older groups shifts every
76-
// existing index by the number of items prepended. This is a one-shot event (not part of
77-
// UiState) so the pager can silently jump to the shifted position - via
78-
// pagerState.scrollToPage(pagerState.currentPage + shift) - the moment it fires, keeping the
79-
// same item on screen instead of visually jumping to whatever is now at the old index.
80-
private val _indexShiftEvents = MutableSharedFlow<Int>(extraBufferCapacity = 1)
81-
val indexShiftEvents = _indexShiftEvents
76+
private var nextShiftId = 0L
8277

8378
private lateinit var user: User
8479
private lateinit var repositoryParameters: SharedItemsRepository.Parameters
@@ -112,6 +107,10 @@ class MediaViewerViewModel @Inject constructor(private val sharedItemsRepository
112107
onPageSettled(globalIndex)
113108
}
114109

110+
fun consumePendingShift(id: Long) {
111+
_uiState.update { if (it.pendingShift?.id == id) it.copy(pendingShift = null) else it }
112+
}
113+
115114
private fun ensureCachedAround(globalIndex: Int) {
116115
val items = _uiState.value.flattenedItems
117116
for (index in (globalIndex - PREFETCH_RADIUS)..(globalIndex + PREFETCH_RADIUS)) {
@@ -127,7 +126,7 @@ class MediaViewerViewModel @Inject constructor(private val sharedItemsRepository
127126
}
128127

129128
val context = NextcloudTalkApplication.sharedApplication!!
130-
val existing = FileUtils.resolveSharedAttachmentFile(context.cacheDir, item.fileName)
129+
val existing = FileUtils.resolveSharedAttachmentFile(context.cacheDir, item.fileId, item.fileName)
131130
if (existing != null && existing.exists()) {
132131
_uiState.update {
133132
it.copy(cachedFilePaths = it.cachedFilePaths + (item.messageId to existing.absolutePath))
@@ -144,6 +143,7 @@ class MediaViewerViewModel @Inject constructor(private val sharedItemsRepository
144143
DownloadFileToCacheWorker.KEY_ATTACHMENT_FOLDER,
145144
CapabilitiesUtil.getAttachmentFolder(user.capabilities!!.spreedCapability!!)
146145
)
146+
.putString(DownloadFileToCacheWorker.KEY_FILE_ID, item.fileId)
147147
.putString(DownloadFileToCacheWorker.KEY_FILE_NAME, item.fileName)
148148
.putString(DownloadFileToCacheWorker.KEY_FILE_PATH, item.path)
149149
.putLong(DownloadFileToCacheWorker.KEY_FILE_SIZE, item.fileSize)
@@ -161,7 +161,11 @@ class MediaViewerViewModel @Inject constructor(private val sharedItemsRepository
161161
if (workInfo == null) return@collect
162162
when (workInfo.state) {
163163
WorkInfo.State.SUCCEEDED -> {
164-
val downloaded = FileUtils.resolveSharedAttachmentFile(context.cacheDir, item.fileName)
164+
val downloaded = FileUtils.resolveSharedAttachmentFile(
165+
context.cacheDir,
166+
item.fileId,
167+
item.fileName
168+
)
165169
_uiState.update {
166170
it.copy(
167171
downloadingMessageIds = it.downloadingMessageIds - item.messageId,
@@ -210,17 +214,16 @@ class MediaViewerViewModel @Inject constructor(private val sharedItemsRepository
210214
}
211215

212216
oldestKnownMessageId = olderItems.first().messageId
213-
val previousCount = _uiState.value.flattenedItems.size
214217
_uiState.update { current ->
215218
val combined = (olderItems + current.flattenedItems).toMediaViewerGroups()
216219
current.copy(
217220
groups = combined,
218221
currentGlobalIndex = current.currentGlobalIndex + olderItems.size,
219222
loadingOlder = false,
220-
canLoadOlder = sharedItems?.moreItemsExisting == true
223+
canLoadOlder = sharedItems?.moreItemsExisting == true,
224+
pendingShift = PendingShift(id = nextShiftId++, amount = olderItems.size)
221225
)
222226
}
223-
_indexShiftEvents.tryEmit(_uiState.value.flattenedItems.size - previousCount)
224227
}
225228
}
226229

‎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)