From 6ac88a9420de31c5408d47856765abda3f092d0c Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Tue, 16 Aug 2022 16:03:24 +0200 Subject: [PATCH 1/5] Add a check on suspicious string template. Especially we want to ensure that the app does not log unexpected content. --- tools/check/forbidden_strings_in_code.txt | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tools/check/forbidden_strings_in_code.txt b/tools/check/forbidden_strings_in_code.txt index b12f15fa5d..b4d7ebae1f 100755 --- a/tools/check/forbidden_strings_in_code.txt +++ b/tools/check/forbidden_strings_in_code.txt @@ -185,3 +185,6 @@ System\.currentTimeMillis\(\)===2 onCreateOptionsMenu onOptionsItemSelected onPrepareOptionsMenu + +### Suspicious String template. Please check that the string template will behave as expected, i.e. the class field and not the whole object will be used. For instance `Timber.d("$event.type")` is not correct, you should write `Timber.d("${event.type}")`. In the former the whole event content will be logged, since it's a data class. If this is expected (i.e. to fix false positive), please add explicit curly braces (`{` and `}`) around the variable, for instance `"elementLogs.${i}.txt"` +\$[a-zA-Z_]\w*\??\.[a-zA-Z_] From 6089d2440959e460ecda1ddb84fa3309b8662b52 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Tue, 16 Aug 2022 15:44:27 +0200 Subject: [PATCH 2/5] Fix some string template issue. --- .../verification/VerificationTransportToDevice.kt | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/verification/VerificationTransportToDevice.kt b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/verification/VerificationTransportToDevice.kt index 861a7a3a77..23a75d2bb3 100644 --- a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/verification/VerificationTransportToDevice.kt +++ b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/verification/VerificationTransportToDevice.kt @@ -76,12 +76,12 @@ internal class VerificationTransportToDevice( .configureWith(SendToDeviceTask.Params(MessageType.MSGTYPE_VERIFICATION_REQUEST, contentMap)) { this.callback = object : MatrixCallback { override fun onSuccess(data: Unit) { - Timber.v("## verification [$tx.transactionId] send toDevice request success") + Timber.v("## verification [${tx?.transactionId}] send toDevice request success") callback.invoke(localId, validKeyReq) } override fun onFailure(failure: Throwable) { - Timber.e("## verification [$tx.transactionId] failed to send toDevice request") + Timber.e("## verification [${tx?.transactionId}] failed to send toDevice request") } } } @@ -103,12 +103,12 @@ internal class VerificationTransportToDevice( .configureWith(SendToDeviceTask.Params(EventType.KEY_VERIFICATION_READY, contentMap)) { this.callback = object : MatrixCallback { override fun onSuccess(data: Unit) { - Timber.v("## verification [$tx.transactionId] send toDevice request success") + Timber.v("## verification [${tx?.transactionId}] send toDevice request success") callback?.invoke() } override fun onFailure(failure: Throwable) { - Timber.e("## verification [$tx.transactionId] failed to send toDevice request") + Timber.e("## verification [${tx?.transactionId}] failed to send toDevice request") } } } @@ -136,7 +136,7 @@ internal class VerificationTransportToDevice( .configureWith(SendToDeviceTask.Params(type, contentMap)) { this.callback = object : MatrixCallback { override fun onSuccess(data: Unit) { - Timber.v("## SAS verification [$tx.transactionId] toDevice type '$type' success.") + Timber.v("## SAS verification [${tx.transactionId}] toDevice type '$type' success.") if (onDone != null) { onDone() } else { @@ -149,7 +149,7 @@ internal class VerificationTransportToDevice( } override fun onFailure(failure: Throwable) { - Timber.e("## SAS verification [$tx.transactionId] failed to send toDevice in state : $tx.state") + Timber.e("## SAS verification [${tx.transactionId}] failed to send toDevice in state : ${tx.state}") tx.cancel(onErrorReason) } } From ea465a1b86f0cf36a04799de6cdfa125260f0f01 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Tue, 16 Aug 2022 16:03:54 +0200 Subject: [PATCH 3/5] Fix false positive on string template suspicious usage. --- .../sdk/internal/database/SessionRealmConfigurationFactory.kt | 2 +- .../im/vector/app/espresso/tools/ScreenshotFailureRule.kt | 4 ++-- .../src/main/java/im/vector/app/core/extensions/Fragment.kt | 4 ++-- .../crypto/keysbackup/setup/KeysBackupSetupStep3Fragment.kt | 2 +- .../java/im/vector/app/features/rageshake/VectorFileLogger.kt | 4 ++-- 5 files changed, 8 insertions(+), 8 deletions(-) diff --git a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/database/SessionRealmConfigurationFactory.kt b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/database/SessionRealmConfigurationFactory.kt index 949dd5daa1..5a0fd74bc0 100644 --- a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/database/SessionRealmConfigurationFactory.kt +++ b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/database/SessionRealmConfigurationFactory.kt @@ -93,7 +93,7 @@ internal class SessionRealmConfigurationFactory @Inject constructor( return } - listOf(REALM_NAME, "$REALM_NAME.lock", "$REALM_NAME.note", "$REALM_NAME.management").forEach { file -> + listOf(REALM_NAME, "${REALM_NAME}.lock", "${REALM_NAME}.note", "${REALM_NAME}.management").forEach { file -> try { File(directory, file).deleteRecursively() } catch (e: Exception) { diff --git a/vector/src/androidTest/java/im/vector/app/espresso/tools/ScreenshotFailureRule.kt b/vector/src/androidTest/java/im/vector/app/espresso/tools/ScreenshotFailureRule.kt index b01c1a895f..068c9fb646 100644 --- a/vector/src/androidTest/java/im/vector/app/espresso/tools/ScreenshotFailureRule.kt +++ b/vector/src/androidTest/java/im/vector/app/espresso/tools/ScreenshotFailureRule.kt @@ -83,7 +83,7 @@ private fun useMediaStoreScreenshotStorage( screenshotLocation: String, bitmap: Bitmap ) { - contentValues.put(MediaStore.MediaColumns.DISPLAY_NAME, "$screenshotName.jpeg") + contentValues.put(MediaStore.MediaColumns.DISPLAY_NAME, "${screenshotName}.jpeg") contentValues.put(MediaStore.Images.Media.RELATIVE_PATH, screenshotLocation) val uri: Uri? = contentResolver.insert(MediaStore.Images.Media.EXTERNAL_CONTENT_URI, contentValues) if (uri != null) { @@ -104,7 +104,7 @@ private fun usePublicExternalScreenshotStorage( if (!directory.exists()) { directory.mkdirs() } - val file = File(directory, "$screenshotName.jpeg") + val file = File(directory, "${screenshotName}.jpeg") saveScreenshotToStream(bitmap, FileOutputStream(file)) contentResolver.insert(MediaStore.Images.Media.EXTERNAL_CONTENT_URI, contentValues) } diff --git a/vector/src/main/java/im/vector/app/core/extensions/Fragment.kt b/vector/src/main/java/im/vector/app/core/extensions/Fragment.kt index 61c4fe2174..f3aef54062 100644 --- a/vector/src/main/java/im/vector/app/core/extensions/Fragment.kt +++ b/vector/src/main/java/im/vector/app/core/extensions/Fragment.kt @@ -175,7 +175,7 @@ fun Fragment.queryExportKeys(userId: String, activityResultLauncher: ActivityRes selectTxtFileToWrite( activity = requireActivity(), activityResultLauncher = activityResultLauncher, - defaultFileName = "$appName-megolm-export-$userId-$timestamp.txt", + defaultFileName = "$appName-megolm-export-$userId-${timestamp}.txt", chooserHint = getString(R.string.keys_backup_setup_step1_manual_export) ) } @@ -187,7 +187,7 @@ fun Activity.queryExportKeys(userId: String, activityResultLauncher: ActivityRes selectTxtFileToWrite( activity = this, activityResultLauncher = activityResultLauncher, - defaultFileName = "$appName-megolm-export-$userId-$timestamp.txt", + defaultFileName = "$appName-megolm-export-$userId-${timestamp}.txt", chooserHint = getString(R.string.keys_backup_setup_step1_manual_export) ) } 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 b8b84ea322..41ff8af04b 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 @@ -134,7 +134,7 @@ class KeysBackupSetupStep3Fragment @Inject constructor() : VectorBaseFragment - File(cacheDirectory, "$fileNamePrefix.$index.txt") + File(cacheDirectory, "$fileNamePrefix.${index}.txt") .takeIf { it.exists() } } } From 446bf7e0aa88b6981f14dd15b3b9b71efda56970 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Tue, 16 Aug 2022 16:10:15 +0200 Subject: [PATCH 4/5] changelog --- changelog.d/6843.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/6843.misc diff --git a/changelog.d/6843.misc b/changelog.d/6843.misc new file mode 100644 index 0000000000..5382e27082 --- /dev/null +++ b/changelog.d/6843.misc @@ -0,0 +1 @@ +Fix some string template From 27de9230b111bc392f777d6dc50212826261a8dc Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Tue, 16 Aug 2022 16:28:27 +0200 Subject: [PATCH 5/5] Ignore ktlint string-template. --- build.gradle | 2 ++ 1 file changed, 2 insertions(+) diff --git a/build.gradle b/build.gradle index afe51cc734..6ef6ef0a33 100644 --- a/build.gradle +++ b/build.gradle @@ -151,6 +151,8 @@ allprojects { "experimental:comment-wrapping", // - A KDoc comment after any other element on the same line must be separated by a new line "experimental:kdoc-wrapping", + // Ignore error "Redundant curly braces", since we use it to fix false positives, for instance in "elementLogs.${i}.txt" + "string-template", ] }