Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(message): fix in-message embedded link validator (WPB-3554) #2088

Merged
merged 2 commits into from
Aug 11, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ fun InvalidLinkDialog(dialogState: InvalidLinkDialogState, hideDialog: () -> Uni
if (dialogState is InvalidLinkDialogState.Visible) {
WireDialog(
title = stringResource(R.string.label_invalid_link_title),
text = stringResource(R.string.invalid_link),
text = stringResource(R.string.invalid_link_dialog_body),
buttonsHorizontalAlignment = false,
onDismiss = hideDialog,
optionButton1Properties = WireDialogButtonProperties(
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
/*
* Wire
* Copyright (C) 2023 Wire Swiss GmbH
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation, either version 3 of the License, or
* (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program. If not, see http://www.gnu.org/licenses/.
*/

package com.wire.android.ui.common.dialogs

import androidx.compose.runtime.Composable
import androidx.compose.ui.res.stringResource
import com.wire.android.R
import com.wire.android.ui.common.WireDialog
import com.wire.android.ui.common.WireDialogButtonProperties
import com.wire.android.ui.common.WireDialogButtonType
import com.wire.android.ui.home.conversations.VisitLinkDialogState

@Composable
fun VisitLinkDialog(dialogState: VisitLinkDialogState, hideDialog: () -> Unit) {
if (dialogState is VisitLinkDialogState.Visible) {
WireDialog(
title = stringResource(R.string.label_visit_link_title),
text = stringResource(R.string.visit_link_dialog_body, dialogState.link),
buttonsHorizontalAlignment = false,
onDismiss = hideDialog,
optionButton1Properties = WireDialogButtonProperties(
text = stringResource(R.string.label_open),
type = WireDialogButtonType.Primary,
onClick = dialogState.openLink
),
optionButton2Properties = WireDialogButtonProperties(
text = stringResource(R.string.label_cancel),
type = WireDialogButtonType.Primary,
onClick = hideDialog
)
)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ import com.wire.android.navigation.hiltSavedStateViewModel
import com.wire.android.ui.common.bottomsheet.MenuModalSheetHeader
import com.wire.android.ui.common.bottomsheet.MenuModalSheetLayout
import com.wire.android.ui.common.dialogs.InvalidLinkDialog
import com.wire.android.ui.common.dialogs.VisitLinkDialog
import com.wire.android.ui.common.dialogs.calling.CallingFeatureUnavailableDialog
import com.wire.android.ui.common.dialogs.calling.JoinAnywayDialog
import com.wire.android.ui.common.dialogs.calling.OngoingActiveCallDialog
Expand All @@ -82,6 +83,7 @@ import com.wire.android.ui.home.messagecomposer.MessageComposer
import com.wire.android.ui.home.messagecomposer.state.MessageBundle
import com.wire.android.ui.home.messagecomposer.state.MessageComposerStateHolder
import com.wire.android.ui.home.messagecomposer.state.rememberMessageComposerStateHolder
import com.wire.android.util.normalizeLink
import com.wire.android.util.permission.CallingAudioRequestFlow
import com.wire.android.util.permission.rememberCallingRecordAudioBluetoothRequestFlow
import com.wire.android.util.ui.UIText
Expand Down Expand Up @@ -224,10 +226,15 @@ fun ConversationScreen(
onClearMentionSearchResult = messageComposerViewModel::clearMentionSearchResult,
onLinkClick = { link ->
with(messageComposerViewModel) {
if (isLinkValid(link)) {
uriHandler.openUri(link)
} else {
invalidLinkDialogState = InvalidLinkDialogState.Visible
val normalizedLink = normalizeLink(link)
visitLinkDialogState = VisitLinkDialogState.Visible(normalizedLink) {
try {
uriHandler.openUri(normalizedLink)
visitLinkDialogState = VisitLinkDialogState.Hidden
} catch (_: Exception) {
visitLinkDialogState = VisitLinkDialogState.Hidden
invalidLinkDialogState = InvalidLinkDialogState.Visible
}
}
}
},
Expand All @@ -246,6 +253,11 @@ fun ConversationScreen(
dialogState = messageComposerViewModel.assetTooLargeDialogState,
hideDialog = messageComposerViewModel::hideAssetTooLargeError
)
VisitLinkDialog(
dialogState = messageComposerViewModel.visitLinkDialogState,
hideDialog = messageComposerViewModel::hideVisitLinkDialog
)

InvalidLinkDialog(
dialogState = messageComposerViewModel.invalidLinkDialogState,
hideDialog = messageComposerViewModel::hideInvalidLinkError
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,10 @@ class MessageComposerViewModel @Inject constructor(
AssetTooLargeDialogState.Hidden
)

var visitLinkDialogState: VisitLinkDialogState by mutableStateOf(
VisitLinkDialogState.Hidden
)

var invalidLinkDialogState: InvalidLinkDialogState by mutableStateOf(
InvalidLinkDialogState.Hidden
)
Expand Down Expand Up @@ -499,6 +503,10 @@ class MessageComposerViewModel @Inject constructor(
assetTooLargeDialogState = AssetTooLargeDialogState.Hidden
}

fun hideVisitLinkDialog() {
visitLinkDialogState = VisitLinkDialogState.Hidden
}

fun hideInvalidLinkError() {
invalidLinkDialogState = InvalidLinkDialogState.Hidden
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,11 @@ sealed class AssetTooLargeDialogState {
data class Visible(val assetType: AttachmentType, val maxLimitInMB: Int, val savedToDevice: Boolean) : AssetTooLargeDialogState()
}

sealed class VisitLinkDialogState {
object Hidden : VisitLinkDialogState()
data class Visible(val link: String, val openLink: () -> Unit) : VisitLinkDialogState()
}

sealed class InvalidLinkDialogState {
object Hidden : InvalidLinkDialogState()
object Visible : InvalidLinkDialogState()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ fun MarkdownText(
start = offset,
end = offset,
).firstOrNull()?.let { result ->
onClickLink?.invoke(annotatedString.substring(result.start, result.end))
onClickLink?.invoke(result.item)
}

annotatedString.getStringAnnotations(
Expand Down
36 changes: 36 additions & 0 deletions app/src/main/kotlin/com/wire/android/util/UriUtil.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
/*
* Wire
* Copyright (C) 2023 Wire Swiss GmbH
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation, either version 3 of the License, or
* (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program. If not, see http://www.gnu.org/licenses/.
*/
package com.wire.android.util

import java.net.URLDecoder

fun containsSchema(url: String): Boolean {
val regexPattern = Regex(".+://.+")

return regexPattern.matches(url)
Comment on lines +23 to +25
Copy link
Member

@vitorhugods vitorhugods Aug 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: leverage already existing functions in the JVM.

Suggested change
val regexPattern = Regex(".+://.+")
return regexPattern.matches(url)
return try {
URI.create(string).schema != null
} catch (iae: IllegalArgumentException) {
false // invalid URI
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this only validates against valid and known schemes, it's not passing against unknown schemes.

Copy link
Member

@vitorhugods vitorhugods Aug 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mchenani how so?

image image

}

fun normalizeLink(url: String): String {
val normalizedUrl = URLDecoder.decode(url, "UTF-8") // Decode URL to human-readable format

return if (containsSchema(normalizedUrl)) {
normalizedUrl
} else {
"https://$normalizedUrl"
Copy link
Member

@MohamadJaara MohamadJaara Aug 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: if the link already have something like, "/google.com" then it will turn into "https:///google.com" and it will fail to open, do we need to care about those edge cases ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/google.com is not a URL

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but what if we send [google](/google.com) then it will go https:///google.com as @MohamadJaara mentioned, no? 🤔

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd blame whoever pasted the invalid URI there to be honest 😄.

Would be a nice to have for the rare cases where people mistype that hard, but not sure if worth the effort

}
}
7 changes: 6 additions & 1 deletion app/src/main/res/values/strings.xml
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
<string name="label_new">New</string>
<string name="label_login">Login</string>
<string name="label_ok">OK</string>
<string name="label_open">Open</string>
<string name="label_cancel">Cancel</string>
<string name="label_confirm">Confirm</string>
<string name="label_continue">Continue</string>
Expand Down Expand Up @@ -979,9 +980,13 @@
<string name="self_deleting_messages_option_description">When this is on, all messages in this group will disappear after a certain time. This applies to all group participants.</string>
<string name="self_deleting_messages_folder_timer">Timer</string>

<!-- visit link -->
<string name="label_visit_link_title">Visit Link</string>
<string name="visit_link_dialog_body">This will take you to %s</string>

<!-- invalid link -->
<string name="label_invalid_link_title">Invalid Link</string>
<string name="invalid_link">The schema of the embedded link is not valid.</string>
<string name="invalid_link_dialog_body">Link could not be opened</string>

<!-- guest room link -->
<string name="folder_label_guest_link">Guest link</string>
Expand Down
26 changes: 26 additions & 0 deletions app/src/test/kotlin/com/wire/android/TestUtil.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
/*
* Wire
* Copyright (C) 2023 Wire Swiss GmbH
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation, either version 3 of the License, or
* (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program. If not, see http://www.gnu.org/licenses/.
*/
package com.wire.android

import kotlin.random.Random

val charPool: List<Char> = ('a'..'z') + ('A'..'Z') + ('0'..'9')

fun Random.string(length: Int) = (1..length)
.map { Random.nextInt(0, charPool.size).let { charPool[it] } }
.joinToString("")
57 changes: 57 additions & 0 deletions app/src/test/kotlin/com/wire/android/util/UriUtilTest.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
/*
* Wire
* Copyright (C) 2023 Wire Swiss GmbH
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation, either version 3 of the License, or
* (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program. If not, see http://www.gnu.org/licenses/.
*/
package com.wire.android.util

import com.wire.android.string
import org.junit.jupiter.api.Test
import kotlin.random.Random

class UriUtilTest {
@Test
fun givenLink_whenTheLinkStartsWithHttps_thenReturnsTheSameLink() {
val input = "https://google.com"
val expected = "https://google.com"
val actual = normalizeLink(input)
assert(expected == actual)
}

@Test
fun givenLink_whenTheLinkStartsWithHttp_thenReturnsTheSameLink() {
val input = "http://google.com"
val expected = "http://google.com"
val actual = normalizeLink(input)
assert(expected == actual)
}

@Test
fun givenLink_whenTheLinkStartsWithRandomSchema_thenReturnsTheSameLink() {
val randomString = Random.string(Random.nextInt(5))
val input = "$randomString://google.com"
val expected = "$randomString://google.com"
val actual = normalizeLink(input)
assert(expected == actual)
}

@Test
fun givenLink_whenTheLinkWithoutSchema_thenReturnsTheLinkWithHttps() {
val input = Random.string(Random.nextInt(20))
val expected = "https://$input"
val actual = normalizeLink(input)
assert(expected == actual)
}
}
Loading