Merge pull request #1270 from vector-im/feature/misleading_url_target

Show a warning dialog if the text of the clicked link does not match

CHANGES.md

@@ -23,6 +23,7 @@ Improvements 🙌:
  - Emoji Verification | It's not the same butterfly! (#1220)
  - Cross-Signing | Composer decoration: shields (#1077)
  - Cross-Signing | Migrate existing keybackup to cross signing with 4S from mobile (#1197)
+ - Show a warning dialog if the text of the clicked link does not match the link target (#922)
  - Cross-Signing | Consider not using a spinner on the 'complete security' prompt (#1271)
  - Restart broken Olm sessions ([MSC1719](https://github.com/matrix-org/matrix-doc/pull/1719))
  - Cross-Signing | Hide Use recovery key when 4S is not setup (#1007)

vector/src/main/java/im/vector/riotx/core/utils/EvenBetterLinkMovementMethod.kt

@@ -0,0 +1,48 @@
+/*
+ * Copyright (c) 2020 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.riotx.core.utils
+
+import android.text.Spanned
+import android.text.style.ClickableSpan
+import android.text.style.URLSpan
+import android.widget.TextView
+import me.saket.bettermovementmethod.BetterLinkMovementMethod
+
+class EvenBetterLinkMovementMethod(private val onLinkClickListener: OnLinkClickListener? = null) : BetterLinkMovementMethod() {
+
+    interface OnLinkClickListener {
+        /**
+         * @param textView   The TextView on which a click was registered.
+         * @param span       The ClickableSpan which is clicked on.
+         * @param url        The clicked URL.
+         * @param actualText The original text which is spanned. Can be used to compare actualText and target url to prevent misleading urls.
+         * @return true if this click was handled, false to let Android handle the URL.
+         */
+        fun onLinkClicked(textView: TextView, span: ClickableSpan, url: String, actualText: String): Boolean
+    }
+
+    override fun dispatchUrlClick(textView: TextView, clickableSpan: ClickableSpan) {
+        val spanned = textView.text as Spanned
+        val actualText = textView.text.subSequence(spanned.getSpanStart(clickableSpan), spanned.getSpanEnd(clickableSpan)).toString()
+        val url = (clickableSpan as? URLSpan)?.url ?: actualText
+
+        if (onLinkClickListener == null || !onLinkClickListener.onLinkClicked(textView, clickableSpan, url, actualText)) {
+            // Let Android handle this long click as a short-click.
+            clickableSpan.onClick(textView)
+        }
+    }
+}

vector/src/main/java/im/vector/riotx/features/home/room/detail/RoomDetailFragment.kt

@@ -39,6 +39,7 @@ import androidx.core.app.ActivityOptionsCompat
 import androidx.core.content.ContextCompat
 import androidx.core.net.toUri
 import androidx.core.text.buildSpannedString
+import androidx.core.text.toSpannable
 import androidx.core.util.Pair
 import androidx.core.view.ViewCompat
 import androidx.core.view.forEach
@@ -110,9 +111,11 @@ import im.vector.riotx.core.utils.PERMISSION_REQUEST_CODE_PICK_ATTACHMENT
 import im.vector.riotx.core.utils.TextUtils
 import im.vector.riotx.core.utils.allGranted
 import im.vector.riotx.core.utils.checkPermissions
+import im.vector.riotx.core.utils.colorizeMatchingText
 import im.vector.riotx.core.utils.copyToClipboard
 import im.vector.riotx.core.utils.createUIHandler
 import im.vector.riotx.core.utils.getColorFromUserId
+import im.vector.riotx.core.utils.isValidUrl
 import im.vector.riotx.core.utils.jsonViewerStyler
 import im.vector.riotx.core.utils.openUrlInExternalBrowser
 import im.vector.riotx.core.utils.saveMedia
@@ -167,6 +170,7 @@ import org.billcarsonfr.jsonviewer.JSonViewerDialog
 import org.commonmark.parser.Parser
 import timber.log.Timber
 import java.io.File
+import java.net.URL
 import java.util.concurrent.TimeUnit
 import javax.inject.Inject
 
@@ -919,7 +923,7 @@ class RoomDetailFragment @Inject constructor(
 
     // TimelineEventController.Callback ************************************************************
 
-    override fun onUrlClicked(url: String): Boolean {
+    override fun onUrlClicked(url: String, title: String): Boolean {
         permalinkHandler
                 .launch(requireActivity(), url, object : NavigationInterceptor {
                     override fun navToRoom(roomId: String?, eventId: String?): Boolean {
@@ -947,8 +951,25 @@ class RoomDetailFragment @Inject constructor(
                 .observeOn(AndroidSchedulers.mainThread())
                 .subscribe { managed ->
                     if (!managed) {
-                        // Open in external browser, in a new Tab
-                        openUrlInExternalBrowser(requireContext(), url)
+                        if (title.isValidUrl() && url.isValidUrl() && URL(title).host != URL(url).host) {
+                            AlertDialog.Builder(requireActivity())
+                                    .setTitle(R.string.external_link_confirmation_title)
+                                    .setMessage(
+                                            getString(R.string.external_link_confirmation_message, title, url)
+                                                    .toSpannable()
+                                                    .colorizeMatchingText(url, colorProvider.getColorFromAttribute(android.R.attr.textColorLink))
+                                                    .colorizeMatchingText(title, colorProvider.getColorFromAttribute(android.R.attr.textColorLink))
+                                    )
+                                    .setPositiveButton(R.string._continue) { _, _ ->
+                                        openUrlInExternalBrowser(requireContext(), url)
+                                    }
+                                    .setNegativeButton(R.string.cancel, null)
+                                    .show()
+                                    .withColoredButton(DialogInterface.BUTTON_NEGATIVE)
+                        } else {
+                            // Open in external browser, in a new Tab
+                            openUrlInExternalBrowser(requireContext(), url)
+                        }
                     }
                 }
                 .disposeOnDestroyView()
@@ -957,7 +978,7 @@ class RoomDetailFragment @Inject constructor(
     }
 
     override fun onUrlLongClicked(url: String): Boolean {
-        if (url != getString(R.string.edited_suffix)) {
+        if (url != getString(R.string.edited_suffix) && url.isValidUrl()) {
             // Copy the url to the clipboard
             copyToClipboard(requireContext(), url, true, R.string.link_copied_to_clipboard)
         }
@@ -1263,7 +1284,7 @@ class RoomDetailFragment @Inject constructor(
                 roomDetailViewModel.handle(RoomDetailAction.IgnoreUser(action.senderId))
             }
             is EventSharedAction.OnUrlClicked               -> {
-                onUrlClicked(action.url)
+                onUrlClicked(action.url, action.title)
             }
             is EventSharedAction.OnUrlLongClicked           -> {
                 onUrlLongClicked(action.url)

vector/src/main/java/im/vector/riotx/features/home/room/detail/timeline/TimelineEventController.kt

@@ -104,7 +104,7 @@ class TimelineEventController @Inject constructor(private val dateFormatter: Vec
     }
 
     interface UrlClickCallback {
-        fun onUrlClicked(url: String): Boolean
+        fun onUrlClicked(url: String, title: String): Boolean
         fun onUrlLongClicked(url: String): Boolean
     }
 

vector/src/main/java/im/vector/riotx/features/home/room/detail/timeline/action/EventSharedAction.kt

@@ -99,7 +99,7 @@ sealed class EventSharedAction(@StringRes val titleRes: Int,
             EventSharedAction(R.string.message_view_edit_history, R.drawable.ic_view_edit_history)
 
     // An url in the event preview has been clicked
-    data class OnUrlClicked(val url: String) :
+    data class OnUrlClicked(val url: String, val title: String) :
             EventSharedAction(0, 0)
 
     // An url in the event preview has been long clicked

vector/src/main/java/im/vector/riotx/features/home/room/detail/timeline/action/MessageActionsBottomSheet.kt

@@ -63,8 +63,8 @@ class MessageActionsBottomSheet : VectorBaseBottomSheetDialogFragment(), Message
         super.onDestroyView()
     }
 
-    override fun onUrlClicked(url: String): Boolean {
-        sharedActionViewModel.post(EventSharedAction.OnUrlClicked(url))
+    override fun onUrlClicked(url: String, title: String): Boolean {
+        sharedActionViewModel.post(EventSharedAction.OnUrlClicked(url, title))
         // Always consume
         return true
     }

vector/src/main/java/im/vector/riotx/features/home/room/detail/timeline/tools/EventRenderingTools.kt

@@ -17,19 +17,20 @@
 package im.vector.riotx.features.home.room.detail.timeline.tools
 
 import android.text.SpannableStringBuilder
+import android.text.style.ClickableSpan
 import android.view.MotionEvent
+import android.widget.TextView
 import androidx.core.text.toSpannable
 import im.vector.matrix.android.api.permalinks.MatrixLinkify
 import im.vector.matrix.android.api.permalinks.MatrixPermalinkSpan
 import im.vector.riotx.core.linkify.VectorLinkify
-import im.vector.riotx.core.utils.isValidUrl
+import im.vector.riotx.core.utils.EvenBetterLinkMovementMethod
 import im.vector.riotx.features.home.room.detail.timeline.TimelineEventController
 import im.vector.riotx.features.html.PillImageSpan
 import kotlinx.coroutines.CoroutineScope
 import kotlinx.coroutines.Dispatchers
 import kotlinx.coroutines.launch
 import kotlinx.coroutines.withContext
-import me.saket.bettermovementmethod.BetterLinkMovementMethod
 
 fun CharSequence.findPillsAndProcess(scope: CoroutineScope, processBlock: (PillImageSpan) -> Unit) {
     scope.launch(Dispatchers.Main) {
@@ -42,10 +43,11 @@ fun CharSequence.findPillsAndProcess(scope: CoroutineScope, processBlock: (PillI
 }
 
 fun CharSequence.linkify(callback: TimelineEventController.UrlClickCallback?): CharSequence {
+    val text = this.toString()
     val spannable = SpannableStringBuilder(this)
     MatrixLinkify.addLinks(spannable, object : MatrixPermalinkSpan.Callback {
         override fun onUrlClicked(url: String) {
-            callback?.onUrlClicked(url)
+            callback?.onUrlClicked(url, text)
         }
     })
     VectorLinkify.addLinks(spannable, true)
@@ -54,18 +56,17 @@ fun CharSequence.linkify(callback: TimelineEventController.UrlClickCallback?): C
 
 // Better link movement methods fixes the issue when
 // long pressing to open the context menu on a TextView also triggers an autoLink click.
-fun createLinkMovementMethod(urlClickCallback: TimelineEventController.UrlClickCallback?): BetterLinkMovementMethod {
-    return BetterLinkMovementMethod.newInstance()
+fun createLinkMovementMethod(urlClickCallback: TimelineEventController.UrlClickCallback?): EvenBetterLinkMovementMethod {
+    return EvenBetterLinkMovementMethod(object : EvenBetterLinkMovementMethod.OnLinkClickListener {
+        override fun onLinkClicked(textView: TextView, span: ClickableSpan, url: String, actualText: String): Boolean {
+            return urlClickCallback?.onUrlClicked(url, actualText) == true
+        }
+    })
             .apply {
-                setOnLinkClickListener { _, url ->
-                    // Return false to let android manage the click on the link, or true if the link is handled by the application
-                    url.isValidUrl() && urlClickCallback?.onUrlClicked(url) == true
-                }
-
                 // We need also to fix the case when long click on link will trigger long click on cell
                 setOnLinkLongClickListener { tv, url ->
                     // Long clicks are handled by parent, return true to block android to do something with url
-                    if (url.isValidUrl() && urlClickCallback?.onUrlLongClicked(url) == true) {
+                    if (urlClickCallback?.onUrlLongClicked(url) == true) {
                         tv.dispatchTouchEvent(MotionEvent.obtain(0, 0, MotionEvent.ACTION_CANCEL, 0f, 0f, 0))
                         true
                     } else {

vector/src/main/res/values/strings_riotX.xml

@@ -34,7 +34,8 @@
 
 
     <!-- BEGIN Strings added by Onuray -->
-
+    <string name="external_link_confirmation_title">Double-check this link</string>
+    <string name="external_link_confirmation_message">The link %1$s is taking you to another site: %2$s.\n\nAre you sure you want to continue?</string>
     <!-- END Strings added by Onuray -->