From b9b5cab7722a8b648b80b62fffc19cf27b92c375 Mon Sep 17 00:00:00 2001 From: cketti Date: Tue, 29 Mar 2022 16:12:12 +0200 Subject: [PATCH 1/2] Use truncate mode to replace the contents of existing files `ContentResolver.openOutputStream(Uri)` does not truncate existing files. If the amount of data written is smaller than the file size, you end up with new data at the beginning of the file followed by old data at the end of the file. --- changelog.d/5663.bugfix | 1 + .../im/vector/app/features/crypto/keys/KeysExporter.kt | 2 +- .../keysbackup/setup/KeysBackupSetupStep3Fragment.kt | 2 +- .../crypto/recover/BootstrapSaveRecoveryKeyFragment.kt | 2 +- .../app/features/settings/devtools/KeyRequestsFragment.kt | 2 +- .../vector/app/features/crypto/keys/KeysExporterTest.kt | 8 ++++---- .../src/test/java/im/vector/app/test/fakes/FakeContext.kt | 8 ++++---- 7 files changed, 13 insertions(+), 12 deletions(-) create mode 100644 changelog.d/5663.bugfix diff --git a/changelog.d/5663.bugfix b/changelog.d/5663.bugfix new file mode 100644 index 0000000000..5086407e8e --- /dev/null +++ b/changelog.d/5663.bugfix @@ -0,0 +1 @@ +Fixed key export when overwriting existing files diff --git a/vector/src/main/java/im/vector/app/features/crypto/keys/KeysExporter.kt b/vector/src/main/java/im/vector/app/features/crypto/keys/KeysExporter.kt index 3db67df8e1..27d8d4842a 100644 --- a/vector/src/main/java/im/vector/app/features/crypto/keys/KeysExporter.kt +++ b/vector/src/main/java/im/vector/app/features/crypto/keys/KeysExporter.kt @@ -34,7 +34,7 @@ class KeysExporter @Inject constructor( suspend fun export(password: String, uri: Uri) { withContext(dispatchers.io) { val data = session.cryptoService().exportRoomKeys(password) - context.contentResolver.openOutputStream(uri) + context.contentResolver.openOutputStream(uri, "wt") ?.use { it.write(data) } ?: throw IllegalStateException("Unable to open file for writing") verifyExportedKeysOutputFileSize(uri, expectedSize = data.size.toLong()) diff --git a/vector/src/main/java/im/vector/app/features/crypto/keysbackup/setup/KeysBackupSetupStep3Fragment.kt b/vector/src/main/java/im/vector/app/features/crypto/keysbackup/setup/KeysBackupSetupStep3Fragment.kt index c1cd87b4c8..42ff4ac183 100644 --- a/vector/src/main/java/im/vector/app/features/crypto/keysbackup/setup/KeysBackupSetupStep3Fragment.kt +++ b/vector/src/main/java/im/vector/app/features/crypto/keysbackup/setup/KeysBackupSetupStep3Fragment.kt @@ -165,7 +165,7 @@ class KeysBackupSetupStep3Fragment @Inject constructor() : VectorBaseFragment os.write(data.toByteArray()) os.flush() diff --git a/vector/src/main/java/im/vector/app/features/crypto/recover/BootstrapSaveRecoveryKeyFragment.kt b/vector/src/main/java/im/vector/app/features/crypto/recover/BootstrapSaveRecoveryKeyFragment.kt index 8a41c7ce4d..746ed48c94 100644 --- a/vector/src/main/java/im/vector/app/features/crypto/recover/BootstrapSaveRecoveryKeyFragment.kt +++ b/vector/src/main/java/im/vector/app/features/crypto/recover/BootstrapSaveRecoveryKeyFragment.kt @@ -81,7 +81,7 @@ class BootstrapSaveRecoveryKeyFragment @Inject constructor( val uri = activityResult.data?.data ?: return@registerStartForActivityResult lifecycleScope.launch(Dispatchers.IO) { try { - sharedViewModel.handle(BootstrapActions.SaveKeyToUri(requireContext().contentResolver!!.openOutputStream(uri)!!)) + sharedViewModel.handle(BootstrapActions.SaveKeyToUri(requireContext().contentResolver!!.openOutputStream(uri, "wt")!!)) } catch (failure: Throwable) { sharedViewModel.handle(BootstrapActions.SaveReqFailed) } diff --git a/vector/src/main/java/im/vector/app/features/settings/devtools/KeyRequestsFragment.kt b/vector/src/main/java/im/vector/app/features/settings/devtools/KeyRequestsFragment.kt index db2d07feef..6748fec1bc 100644 --- a/vector/src/main/java/im/vector/app/features/settings/devtools/KeyRequestsFragment.kt +++ b/vector/src/main/java/im/vector/app/features/settings/devtools/KeyRequestsFragment.kt @@ -106,7 +106,7 @@ class KeyRequestsFragment @Inject constructor() : VectorBaseFragment { tryOrNull { - requireContext().contentResolver?.openOutputStream(it.uri) + requireContext().contentResolver?.openOutputStream(it.uri, "wt") ?.use { os -> os.write(it.raw.toByteArray()) } } } diff --git a/vector/src/test/java/im/vector/app/features/crypto/keys/KeysExporterTest.kt b/vector/src/test/java/im/vector/app/features/crypto/keys/KeysExporterTest.kt index 5d0317592d..3bec3fad88 100644 --- a/vector/src/test/java/im/vector/app/features/crypto/keys/KeysExporterTest.kt +++ b/vector/src/test/java/im/vector/app/features/crypto/keys/KeysExporterTest.kt @@ -53,7 +53,7 @@ class KeysExporterTest { @Test fun `when exporting then writes exported keys to context output stream`() { givenFileDescriptorWithSize(size = A_ROOM_KEYS_EXPORT.size.toLong()) - val outputStream = context.givenOutputStreamFor(A_URI) + val outputStream = context.givenOutputStreamFor(A_URI, mode = "wt") runTest { keysExporter.export(A_PASSWORD, A_URI) } @@ -63,7 +63,7 @@ class KeysExporterTest { @Test fun `given different file size returned for export when exporting then throws UnexpectedExportKeysFileSizeException`() { givenFileDescriptorWithSize(size = 110) - context.givenOutputStreamFor(A_URI) + context.givenOutputStreamFor(A_URI, mode = "wt") assertFailsWith { runTest { keysExporter.export(A_PASSWORD, A_URI) } @@ -72,7 +72,7 @@ class KeysExporterTest { @Test fun `given output stream is unavailable for exporting to when exporting then throws IllegalStateException`() { - context.givenMissingOutputStreamFor(A_URI) + context.givenMissingOutputStreamFor(A_URI, mode = "wt") assertFailsWith(message = "Unable to open file for writing") { runTest { keysExporter.export(A_PASSWORD, A_URI) } @@ -82,7 +82,7 @@ class KeysExporterTest { @Test fun `given exported file is missing after export when exporting then throws IllegalStateException`() { context.givenFileDescriptor(A_URI, mode = "r") { null } - context.givenOutputStreamFor(A_URI) + context.givenOutputStreamFor(A_URI, mode = "wt") assertFailsWith(message = "Exported file not found") { runTest { keysExporter.export(A_PASSWORD, A_URI) } diff --git a/vector/src/test/java/im/vector/app/test/fakes/FakeContext.kt b/vector/src/test/java/im/vector/app/test/fakes/FakeContext.kt index 8382e54253..de1a7956b8 100644 --- a/vector/src/test/java/im/vector/app/test/fakes/FakeContext.kt +++ b/vector/src/test/java/im/vector/app/test/fakes/FakeContext.kt @@ -39,13 +39,13 @@ class FakeContext( every { contentResolver.openFileDescriptor(uri, mode, null) } returns fileDescriptor } - fun givenOutputStreamFor(uri: Uri): OutputStream { + fun givenOutputStreamFor(uri: Uri, mode: String): OutputStream { val outputStream = mockk(relaxed = true) - every { contentResolver.openOutputStream(uri) } returns outputStream + every { contentResolver.openOutputStream(uri, mode) } returns outputStream return outputStream } - fun givenMissingOutputStreamFor(uri: Uri) { - every { contentResolver.openOutputStream(uri) } returns null + fun givenMissingOutputStreamFor(uri: Uri, mode: String) { + every { contentResolver.openOutputStream(uri, mode) } returns null } } From 29c7ea11bd799ab1021131c88048bbcb654e5de3 Mon Sep 17 00:00:00 2001 From: cketti Date: Wed, 30 Mar 2022 15:38:40 +0200 Subject: [PATCH 2/2] Create extension function `Context.safeOpenOutputStream` --- .../main/java/im/vector/app/core/extensions/Context.kt | 9 +++++++++ .../im/vector/app/features/crypto/keys/KeysExporter.kt | 3 ++- .../keysbackup/setup/KeysBackupSetupStep3Fragment.kt | 3 ++- .../crypto/recover/BootstrapSaveRecoveryKeyFragment.kt | 3 ++- .../features/settings/devtools/KeyRequestsFragment.kt | 3 ++- .../vector/app/features/crypto/keys/KeysExporterTest.kt | 8 ++++---- .../test/java/im/vector/app/test/fakes/FakeContext.kt | 8 ++++---- 7 files changed, 25 insertions(+), 12 deletions(-) diff --git a/vector/src/main/java/im/vector/app/core/extensions/Context.kt b/vector/src/main/java/im/vector/app/core/extensions/Context.kt index b1e24c9502..0f785e43a3 100644 --- a/vector/src/main/java/im/vector/app/core/extensions/Context.kt +++ b/vector/src/main/java/im/vector/app/core/extensions/Context.kt @@ -18,6 +18,7 @@ package im.vector.app.core.extensions import android.content.Context import android.graphics.drawable.Drawable +import android.net.Uri import android.text.Spannable import android.text.SpannableString import android.text.style.ImageSpan @@ -31,6 +32,7 @@ import androidx.datastore.preferences.core.Preferences import dagger.hilt.EntryPoints import im.vector.app.core.datastore.dataStoreProvider import im.vector.app.core.di.SingletonEntryPoint +import java.io.OutputStream import kotlin.math.roundToInt fun Context.singletonEntryPoint(): SingletonEntryPoint { @@ -68,3 +70,10 @@ private fun Float.toAndroidAlpha(): Int { } val Context.dataStoreProvider: (String) -> DataStore by dataStoreProvider() + +/** + * Open Uri in truncate mode to make sure we don't partially overwrite content when we get passed a Uri to an existing file. + */ +fun Context.safeOpenOutputStream(uri: Uri): OutputStream? { + return contentResolver.openOutputStream(uri, "wt") +} diff --git a/vector/src/main/java/im/vector/app/features/crypto/keys/KeysExporter.kt b/vector/src/main/java/im/vector/app/features/crypto/keys/KeysExporter.kt index 27d8d4842a..f40f126d2c 100644 --- a/vector/src/main/java/im/vector/app/features/crypto/keys/KeysExporter.kt +++ b/vector/src/main/java/im/vector/app/features/crypto/keys/KeysExporter.kt @@ -19,6 +19,7 @@ package im.vector.app.features.crypto.keys import android.content.Context import android.net.Uri import im.vector.app.core.dispatchers.CoroutineDispatchers +import im.vector.app.core.extensions.safeOpenOutputStream import kotlinx.coroutines.withContext import org.matrix.android.sdk.api.session.Session import javax.inject.Inject @@ -34,7 +35,7 @@ class KeysExporter @Inject constructor( suspend fun export(password: String, uri: Uri) { withContext(dispatchers.io) { val data = session.cryptoService().exportRoomKeys(password) - context.contentResolver.openOutputStream(uri, "wt") + context.safeOpenOutputStream(uri) ?.use { it.write(data) } ?: throw IllegalStateException("Unable to open file for writing") verifyExportedKeysOutputFileSize(uri, expectedSize = data.size.toLong()) diff --git a/vector/src/main/java/im/vector/app/features/crypto/keysbackup/setup/KeysBackupSetupStep3Fragment.kt b/vector/src/main/java/im/vector/app/features/crypto/keysbackup/setup/KeysBackupSetupStep3Fragment.kt index 42ff4ac183..e5d7ade3ce 100644 --- a/vector/src/main/java/im/vector/app/features/crypto/keysbackup/setup/KeysBackupSetupStep3Fragment.kt +++ b/vector/src/main/java/im/vector/app/features/crypto/keysbackup/setup/KeysBackupSetupStep3Fragment.kt @@ -30,6 +30,7 @@ import com.google.android.material.bottomsheet.BottomSheetDialog import com.google.android.material.dialog.MaterialAlertDialogBuilder import im.vector.app.R import im.vector.app.core.extensions.registerStartForActivityResult +import im.vector.app.core.extensions.safeOpenOutputStream import im.vector.app.core.platform.VectorBaseFragment import im.vector.app.core.utils.LiveEvent import im.vector.app.core.utils.copyToClipboard @@ -165,7 +166,7 @@ class KeysBackupSetupStep3Fragment @Inject constructor() : VectorBaseFragment os.write(data.toByteArray()) os.flush() diff --git a/vector/src/main/java/im/vector/app/features/crypto/recover/BootstrapSaveRecoveryKeyFragment.kt b/vector/src/main/java/im/vector/app/features/crypto/recover/BootstrapSaveRecoveryKeyFragment.kt index 746ed48c94..efabae2f3a 100644 --- a/vector/src/main/java/im/vector/app/features/crypto/recover/BootstrapSaveRecoveryKeyFragment.kt +++ b/vector/src/main/java/im/vector/app/features/crypto/recover/BootstrapSaveRecoveryKeyFragment.kt @@ -29,6 +29,7 @@ import com.airbnb.mvrx.parentFragmentViewModel import com.airbnb.mvrx.withState import im.vector.app.R import im.vector.app.core.extensions.registerStartForActivityResult +import im.vector.app.core.extensions.safeOpenOutputStream import im.vector.app.core.platform.VectorBaseFragment import im.vector.app.core.resources.ColorProvider import im.vector.app.core.utils.startSharePlainTextIntent @@ -81,7 +82,7 @@ class BootstrapSaveRecoveryKeyFragment @Inject constructor( val uri = activityResult.data?.data ?: return@registerStartForActivityResult lifecycleScope.launch(Dispatchers.IO) { try { - sharedViewModel.handle(BootstrapActions.SaveKeyToUri(requireContext().contentResolver!!.openOutputStream(uri, "wt")!!)) + sharedViewModel.handle(BootstrapActions.SaveKeyToUri(requireContext().safeOpenOutputStream(uri)!!)) } catch (failure: Throwable) { sharedViewModel.handle(BootstrapActions.SaveReqFailed) } diff --git a/vector/src/main/java/im/vector/app/features/settings/devtools/KeyRequestsFragment.kt b/vector/src/main/java/im/vector/app/features/settings/devtools/KeyRequestsFragment.kt index 6748fec1bc..cef68c01c1 100644 --- a/vector/src/main/java/im/vector/app/features/settings/devtools/KeyRequestsFragment.kt +++ b/vector/src/main/java/im/vector/app/features/settings/devtools/KeyRequestsFragment.kt @@ -34,6 +34,7 @@ import com.airbnb.mvrx.withState import com.google.android.material.tabs.TabLayoutMediator import im.vector.app.R import im.vector.app.core.extensions.registerStartForActivityResult +import im.vector.app.core.extensions.safeOpenOutputStream import im.vector.app.core.platform.VectorBaseFragment import im.vector.app.core.utils.selectTxtFileToWrite import im.vector.app.databinding.FragmentDevtoolKeyrequestsBinding @@ -106,7 +107,7 @@ class KeyRequestsFragment @Inject constructor() : VectorBaseFragment { tryOrNull { - requireContext().contentResolver?.openOutputStream(it.uri, "wt") + requireContext().safeOpenOutputStream(it.uri) ?.use { os -> os.write(it.raw.toByteArray()) } } } diff --git a/vector/src/test/java/im/vector/app/features/crypto/keys/KeysExporterTest.kt b/vector/src/test/java/im/vector/app/features/crypto/keys/KeysExporterTest.kt index 3bec3fad88..3cd797a7b1 100644 --- a/vector/src/test/java/im/vector/app/features/crypto/keys/KeysExporterTest.kt +++ b/vector/src/test/java/im/vector/app/features/crypto/keys/KeysExporterTest.kt @@ -53,7 +53,7 @@ class KeysExporterTest { @Test fun `when exporting then writes exported keys to context output stream`() { givenFileDescriptorWithSize(size = A_ROOM_KEYS_EXPORT.size.toLong()) - val outputStream = context.givenOutputStreamFor(A_URI, mode = "wt") + val outputStream = context.givenSafeOutputStreamFor(A_URI) runTest { keysExporter.export(A_PASSWORD, A_URI) } @@ -63,7 +63,7 @@ class KeysExporterTest { @Test fun `given different file size returned for export when exporting then throws UnexpectedExportKeysFileSizeException`() { givenFileDescriptorWithSize(size = 110) - context.givenOutputStreamFor(A_URI, mode = "wt") + context.givenSafeOutputStreamFor(A_URI) assertFailsWith { runTest { keysExporter.export(A_PASSWORD, A_URI) } @@ -72,7 +72,7 @@ class KeysExporterTest { @Test fun `given output stream is unavailable for exporting to when exporting then throws IllegalStateException`() { - context.givenMissingOutputStreamFor(A_URI, mode = "wt") + context.givenMissingSafeOutputStreamFor(A_URI) assertFailsWith(message = "Unable to open file for writing") { runTest { keysExporter.export(A_PASSWORD, A_URI) } @@ -82,7 +82,7 @@ class KeysExporterTest { @Test fun `given exported file is missing after export when exporting then throws IllegalStateException`() { context.givenFileDescriptor(A_URI, mode = "r") { null } - context.givenOutputStreamFor(A_URI, mode = "wt") + context.givenSafeOutputStreamFor(A_URI) assertFailsWith(message = "Exported file not found") { runTest { keysExporter.export(A_PASSWORD, A_URI) } diff --git a/vector/src/test/java/im/vector/app/test/fakes/FakeContext.kt b/vector/src/test/java/im/vector/app/test/fakes/FakeContext.kt index de1a7956b8..2a50c34ca3 100644 --- a/vector/src/test/java/im/vector/app/test/fakes/FakeContext.kt +++ b/vector/src/test/java/im/vector/app/test/fakes/FakeContext.kt @@ -39,13 +39,13 @@ class FakeContext( every { contentResolver.openFileDescriptor(uri, mode, null) } returns fileDescriptor } - fun givenOutputStreamFor(uri: Uri, mode: String): OutputStream { + fun givenSafeOutputStreamFor(uri: Uri): OutputStream { val outputStream = mockk(relaxed = true) - every { contentResolver.openOutputStream(uri, mode) } returns outputStream + every { contentResolver.openOutputStream(uri, "wt") } returns outputStream return outputStream } - fun givenMissingOutputStreamFor(uri: Uri, mode: String) { - every { contentResolver.openOutputStream(uri, mode) } returns null + fun givenMissingSafeOutputStreamFor(uri: Uri) { + every { contentResolver.openOutputStream(uri, "wt") } returns null } }