From 913c6b0f140d39e20d7596b4dcbcf7b3a9a83eb2 Mon Sep 17 00:00:00 2001 From: Adam Brown Date: Thu, 26 May 2022 11:38:55 +0100 Subject: [PATCH 1/3] warning the user when urls contain directional overrides and allowing them to confirm the url --- changelog.d/6163.feature | 1 + .../im/vector/app/core/extensions/String.kt | 26 ++++++++ .../home/room/detail/TimelineFragment.kt | 43 ++++++++------ .../core/extensions/StringExtensionsTest.kt | 59 +++++++++++++++++++ 4 files changed, 112 insertions(+), 17 deletions(-) create mode 100644 changelog.d/6163.feature create mode 100644 vector/src/main/java/im/vector/app/core/extensions/String.kt create mode 100644 vector/src/test/java/im/vector/app/core/extensions/StringExtensionsTest.kt diff --git a/changelog.d/6163.feature b/changelog.d/6163.feature new file mode 100644 index 0000000000..7c64f89b12 --- /dev/null +++ b/changelog.d/6163.feature @@ -0,0 +1 @@ +Security - Asking for user confirmation when tapping URLs which contain unicode directional overrides diff --git a/vector/src/main/java/im/vector/app/core/extensions/String.kt b/vector/src/main/java/im/vector/app/core/extensions/String.kt new file mode 100644 index 0000000000..f035de469c --- /dev/null +++ b/vector/src/main/java/im/vector/app/core/extensions/String.kt @@ -0,0 +1,26 @@ +/* + * Copyright (c) 2022 New Vector Ltd + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package im.vector.app.core.extensions + +private const val RTL_OVERRIDE_CHAR = '\u202E' +private const val LTR_OVERRIDE_CHAR = '\u202D' + +fun String.ensureEndsLeftToRight() = if (containsRtLOverride()) "$this$LTR_OVERRIDE_CHAR" else this + +fun String.containsRtLOverride() = contains(RTL_OVERRIDE_CHAR) + +fun String.filterDirectionOverrides() = filterNot { it == RTL_OVERRIDE_CHAR || it == LTR_OVERRIDE_CHAR } diff --git a/vector/src/main/java/im/vector/app/features/home/room/detail/TimelineFragment.kt b/vector/src/main/java/im/vector/app/features/home/room/detail/TimelineFragment.kt index f0cfff4cf8..98818aba92 100644 --- a/vector/src/main/java/im/vector/app/features/home/room/detail/TimelineFragment.kt +++ b/vector/src/main/java/im/vector/app/features/home/room/detail/TimelineFragment.kt @@ -74,6 +74,9 @@ import im.vector.app.core.dialogs.ConfirmationDialogBuilder import im.vector.app.core.dialogs.GalleryOrCameraDialogHelper import im.vector.app.core.epoxy.LayoutManagerStateRestorer import im.vector.app.core.extensions.cleanup +import im.vector.app.core.extensions.containsRtLOverride +import im.vector.app.core.extensions.ensureEndsLeftToRight +import im.vector.app.core.extensions.filterDirectionOverrides import im.vector.app.core.extensions.hideKeyboard import im.vector.app.core.extensions.registerStartForActivityResult import im.vector.app.core.extensions.setTextOrHide @@ -1922,23 +1925,13 @@ class TimelineFragment @Inject constructor( } }) if (!isManaged) { - if (title.isValidUrl() && url.isValidUrl() && URL(title).host != URL(url).host) { - MaterialAlertDialogBuilder(requireActivity(), R.style.ThemeOverlay_Vector_MaterialAlertDialog_NegativeDestructive) - .setTitle(R.string.external_link_confirmation_title) - .setMessage( - getString(R.string.external_link_confirmation_message, title, url) - .toSpannable() - .colorizeMatchingText(url, colorProvider.getColorFromAttribute(R.attr.vctr_content_tertiary)) - .colorizeMatchingText(title, colorProvider.getColorFromAttribute(R.attr.vctr_content_tertiary)) - ) - .setPositiveButton(R.string._continue) { _, _ -> - openUrlInExternalBrowser(requireContext(), url) - } - .setNegativeButton(R.string.action_cancel, null) - .show() - } else { - // Open in external browser, in a new Tab - openUrlInExternalBrowser(requireContext(), url) + when { + url.containsRtLOverride() || (title.isValidUrl() && url.isValidUrl() && URL(title).host != URL(url).host) -> { + displayUrlConfirmationDialog(title.ensureEndsLeftToRight(), url.filterDirectionOverrides()) + } + else -> { + openUrlInExternalBrowser(requireContext(), url) + } } } } @@ -1946,6 +1939,22 @@ class TimelineFragment @Inject constructor( return true } + private fun displayUrlConfirmationDialog(title: String, url: String) { + MaterialAlertDialogBuilder(requireActivity(), R.style.ThemeOverlay_Vector_MaterialAlertDialog_NegativeDestructive) + .setTitle(R.string.external_link_confirmation_title) + .setMessage( + getString(R.string.external_link_confirmation_message, title, url) + .toSpannable() + .colorizeMatchingText(url, colorProvider.getColorFromAttribute(R.attr.vctr_content_tertiary)) + .colorizeMatchingText(title, colorProvider.getColorFromAttribute(R.attr.vctr_content_tertiary)) + ) + .setPositiveButton(R.string._continue) { _, _ -> + openUrlInExternalBrowser(requireContext(), url) + } + .setNegativeButton(R.string.action_cancel, null) + .show() + } + override fun onUrlLongClicked(url: String): Boolean { if (url != getString(R.string.edited_suffix) && url.isValidUrl()) { // Copy the url to the clipboard diff --git a/vector/src/test/java/im/vector/app/core/extensions/StringExtensionsTest.kt b/vector/src/test/java/im/vector/app/core/extensions/StringExtensionsTest.kt new file mode 100644 index 0000000000..c4c7a2d38a --- /dev/null +++ b/vector/src/test/java/im/vector/app/core/extensions/StringExtensionsTest.kt @@ -0,0 +1,59 @@ +/* + * Copyright (c) 2022 New Vector Ltd + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package im.vector.app.core.extensions + +import org.amshove.kluent.shouldBeEqualTo +import org.junit.Test + +class StringExtensionsTest { + + @Test + fun `given text with RtL unicode override, when checking contains RtL Override, then returns true`() { + val textWithRtlOverride = "hello\u202Eworld" + + val result = textWithRtlOverride.containsRtLOverride() + + result shouldBeEqualTo true + } + + @Test + fun `given text without RtL unicode override, when checking contains RtL Override, then returns false`() { + val textWithRtlOverride = "hello world" + + val result = textWithRtlOverride.containsRtLOverride() + + result shouldBeEqualTo false + } + + @Test + fun `given text with RtL unicode override, when ensuring ends LtR, then appends a LtR unicode override`() { + val textWithRtlOverride = "123\u202E456" + + val result = textWithRtlOverride.ensureEndsLeftToRight() + + result shouldBeEqualTo "$textWithRtlOverride\u202D" + } + + @Test + fun `given text with unicode direction overrides, when filtering direction overrides, then removes all overrides`() { + val textWithDirectionOverrides = "123\u202E456\u202d789" + + val result = textWithDirectionOverrides.filterDirectionOverrides() + + result shouldBeEqualTo "123456789" + } +} From e6198d7bf601aa73d955d42e3248b34f93af6075 Mon Sep 17 00:00:00 2001 From: Adam Brown Date: Mon, 30 May 2022 10:38:42 +0100 Subject: [PATCH 2/3] splitting url detection condition into separate branches --- .../app/features/home/room/detail/TimelineFragment.kt | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/vector/src/main/java/im/vector/app/features/home/room/detail/TimelineFragment.kt b/vector/src/main/java/im/vector/app/features/home/room/detail/TimelineFragment.kt index 98818aba92..0f4989e36d 100644 --- a/vector/src/main/java/im/vector/app/features/home/room/detail/TimelineFragment.kt +++ b/vector/src/main/java/im/vector/app/features/home/room/detail/TimelineFragment.kt @@ -1926,10 +1926,13 @@ class TimelineFragment @Inject constructor( }) if (!isManaged) { when { - url.containsRtLOverride() || (title.isValidUrl() && url.isValidUrl() && URL(title).host != URL(url).host) -> { + url.containsRtLOverride() -> { displayUrlConfirmationDialog(title.ensureEndsLeftToRight(), url.filterDirectionOverrides()) } - else -> { + title.isValidUrl() && url.isValidUrl() && URL(title).host != URL(url).host -> { + displayUrlConfirmationDialog(title.ensureEndsLeftToRight(), url) + } + else -> { openUrlInExternalBrowser(requireContext(), url) } } From 67f1929784a0554fdd90ce099d160fd4b9a6d98a Mon Sep 17 00:00:00 2001 From: Adam Brown Date: Mon, 30 May 2022 10:42:42 +0100 Subject: [PATCH 3/3] continuing to the originally supplied url when a rtl override character is detected --- .../home/room/detail/TimelineFragment.kt | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/vector/src/main/java/im/vector/app/features/home/room/detail/TimelineFragment.kt b/vector/src/main/java/im/vector/app/features/home/room/detail/TimelineFragment.kt index 0f4989e36d..3120decd0c 100644 --- a/vector/src/main/java/im/vector/app/features/home/room/detail/TimelineFragment.kt +++ b/vector/src/main/java/im/vector/app/features/home/room/detail/TimelineFragment.kt @@ -1927,10 +1927,14 @@ class TimelineFragment @Inject constructor( if (!isManaged) { when { url.containsRtLOverride() -> { - displayUrlConfirmationDialog(title.ensureEndsLeftToRight(), url.filterDirectionOverrides()) + displayUrlConfirmationDialog( + seenUrl = title.ensureEndsLeftToRight(), + actualUrl = url.filterDirectionOverrides(), + continueTo = url + ) } title.isValidUrl() && url.isValidUrl() && URL(title).host != URL(url).host -> { - displayUrlConfirmationDialog(title.ensureEndsLeftToRight(), url) + displayUrlConfirmationDialog(title, url) } else -> { openUrlInExternalBrowser(requireContext(), url) @@ -1942,17 +1946,17 @@ class TimelineFragment @Inject constructor( return true } - private fun displayUrlConfirmationDialog(title: String, url: String) { + private fun displayUrlConfirmationDialog(seenUrl: String, actualUrl: String, continueTo: String = actualUrl) { MaterialAlertDialogBuilder(requireActivity(), R.style.ThemeOverlay_Vector_MaterialAlertDialog_NegativeDestructive) .setTitle(R.string.external_link_confirmation_title) .setMessage( - getString(R.string.external_link_confirmation_message, title, url) + getString(R.string.external_link_confirmation_message, seenUrl, actualUrl) .toSpannable() - .colorizeMatchingText(url, colorProvider.getColorFromAttribute(R.attr.vctr_content_tertiary)) - .colorizeMatchingText(title, colorProvider.getColorFromAttribute(R.attr.vctr_content_tertiary)) + .colorizeMatchingText(actualUrl, colorProvider.getColorFromAttribute(R.attr.vctr_content_tertiary)) + .colorizeMatchingText(seenUrl, colorProvider.getColorFromAttribute(R.attr.vctr_content_tertiary)) ) .setPositiveButton(R.string._continue) { _, _ -> - openUrlInExternalBrowser(requireContext(), url) + openUrlInExternalBrowser(requireContext(), continueTo) } .setNegativeButton(R.string.action_cancel, null) .show()