4844 fix url elision (#6588)
* For #4844: add test cases for url elision * For 4844: implement toShortUrl to pass test cases * For 4844: update plumbing to use toShortUrl * For 4844: adds/handles suggested url elision test casemaster
Severin Rudie
GitHub
parent
c6562bff98
commit
8d68317388
|
|
@ -27,7 +27,7 @@ import org.mozilla.fenix.R
|
|||
import org.mozilla.fenix.components.metrics.Event
|
||||
import org.mozilla.fenix.ext.components
|
||||
import org.mozilla.fenix.ext.increaseTapArea
|
||||
import org.mozilla.fenix.ext.urlToTrimmedHost
|
||||
import org.mozilla.fenix.ext.toShortUrl
|
||||
import org.mozilla.fenix.home.sessioncontrol.Tab
|
||||
import org.mozilla.fenix.home.sessioncontrol.TabCollection
|
||||
|
||||
|
|
@ -268,7 +268,7 @@ class CollectionCreationView(
|
|||
Tab(
|
||||
tab.id.toString(),
|
||||
tab.url,
|
||||
tab.url.urlToTrimmedHost(view.context),
|
||||
tab.url.toShortUrl(view.context.components.publicSuffixList),
|
||||
tab.title
|
||||
)
|
||||
}.let { tabs ->
|
||||
|
|
|
|||
|
|
@ -14,7 +14,8 @@ import mozilla.components.feature.tab.collections.TabCollection
|
|||
import mozilla.components.feature.tab.collections.TabCollectionStorage
|
||||
import mozilla.components.support.base.observer.Observable
|
||||
import mozilla.components.support.base.observer.ObserverRegistry
|
||||
import org.mozilla.fenix.ext.urlToTrimmedHost
|
||||
import org.mozilla.fenix.ext.components
|
||||
import org.mozilla.fenix.ext.toShortUrl
|
||||
import org.mozilla.fenix.home.sessioncontrol.viewholders.CollectionViewHolder
|
||||
import org.mozilla.fenix.test.Mockable
|
||||
|
||||
|
|
@ -93,7 +94,7 @@ class TabCollectionStorage(
|
|||
|
||||
fun TabCollection.description(context: Context): String {
|
||||
return this.tabs
|
||||
.map { it.url.urlToTrimmedHost(context) }
|
||||
.map { it.url.toShortUrl(context.components.publicSuffixList) }
|
||||
.map {
|
||||
if (it.length > CollectionViewHolder.maxTitleLength) {
|
||||
it.substring(
|
||||
|
|
|
|||
|
|
@ -15,12 +15,12 @@ fun Session.toTab(context: Context, selected: Boolean? = null, mediaState: Media
|
|||
|
||||
fun Session.toTab(publicSuffixList: PublicSuffixList, selected: Boolean? = null, mediaState: MediaState? = null): Tab {
|
||||
return Tab(
|
||||
this.id,
|
||||
this.url,
|
||||
this.url.urlToTrimmedHost(publicSuffixList),
|
||||
this.title,
|
||||
selected,
|
||||
mediaState,
|
||||
this.icon
|
||||
sessionId = this.id,
|
||||
url = this.url,
|
||||
hostname = this.url.toShortUrl(publicSuffixList),
|
||||
title = this.title,
|
||||
selected = selected,
|
||||
mediaState = mediaState,
|
||||
icon = this.icon
|
||||
)
|
||||
}
|
||||
|
|
|
|||
|
|
@ -4,13 +4,20 @@
|
|||
|
||||
package org.mozilla.fenix.ext
|
||||
|
||||
import android.content.Context
|
||||
import android.net.Uri
|
||||
import android.util.Patterns
|
||||
import android.webkit.URLUtil
|
||||
import androidx.core.net.toUri
|
||||
import kotlinx.coroutines.runBlocking
|
||||
import mozilla.components.lib.publicsuffixlist.PublicSuffixList
|
||||
import mozilla.components.support.ktx.android.net.hostWithoutCommonPrefixes
|
||||
import java.net.IDN
|
||||
import java.net.MalformedURLException
|
||||
import java.net.URL
|
||||
import java.util.Locale
|
||||
|
||||
const val FILE_PREFIX = "file://"
|
||||
const val MAX_VALID_PORT = 65_535
|
||||
|
||||
/**
|
||||
* Replaces the keys with the values with the map provided.
|
||||
|
|
@ -32,10 +39,73 @@ fun String.tryGetHostFromUrl(): String = try {
|
|||
}
|
||||
|
||||
/**
|
||||
* Trim a host's prefix and suffix
|
||||
* Shortens URLs to be more user friendly.
|
||||
*
|
||||
* The algorithm used to generate these strings is a combination of FF desktop 'top sites',
|
||||
* feedback from the security team, and documentation regarding url elision. See
|
||||
* StringTest.kt for details.
|
||||
*
|
||||
* This method is complex because URLs have a lot of edge cases. Be sure to thoroughly unit
|
||||
* test any changes you make to it.
|
||||
*/
|
||||
fun String.urlToTrimmedHost(context: Context): String =
|
||||
this.urlToTrimmedHost(context.components.publicSuffixList)
|
||||
@Suppress("UNUSED_PARAMETER", "ReturnCount", "ComplexCondition")
|
||||
// Unused Parameter: We may resume stripping eTLD, depending on conversations between security and UX
|
||||
// Return count: This is a complex method, but it would not be more understandable if broken up
|
||||
// ComplexCondition: Breaking out the complex condition would make this logic harder to follow
|
||||
fun String.toShortUrl(publicSuffixList: PublicSuffixList): String {
|
||||
val inputString = this
|
||||
val uri = inputString.toUri()
|
||||
|
||||
if (
|
||||
inputString.isEmpty() ||
|
||||
!URLUtil.isValidUrl(inputString) ||
|
||||
inputString == FILE_PREFIX ||
|
||||
uri.port !in -1..MAX_VALID_PORT
|
||||
) {
|
||||
return inputString
|
||||
}
|
||||
|
||||
if (inputString.startsWith(FILE_PREFIX)) {
|
||||
// Strip file prefix and return the remainder
|
||||
return inputString.substring(FILE_PREFIX.length)
|
||||
}
|
||||
|
||||
if (uri.host?.isIpv4() == true ||
|
||||
uri.isIpv6() ||
|
||||
// If inputString is just a hostname and not a FQDN, use the entire hostname.
|
||||
uri.host?.contains(".") == false
|
||||
) {
|
||||
return uri.host ?: inputString
|
||||
}
|
||||
|
||||
fun String.stripUserInfo(): String {
|
||||
val userInfo = this.toUri().encodedUserInfo
|
||||
return if (userInfo != null) {
|
||||
val infoIndex = this.indexOf(userInfo)
|
||||
this.removeRange(infoIndex..infoIndex + userInfo.length)
|
||||
} else {
|
||||
this
|
||||
}
|
||||
}
|
||||
fun String.stripPrefixes(): String = this.toUri().hostWithoutCommonPrefixes ?: this
|
||||
fun String.toUnicode() = IDN.toUnicode(this)
|
||||
|
||||
return inputString
|
||||
.stripUserInfo()
|
||||
.toLowerCase(Locale.getDefault())
|
||||
.stripPrefixes()
|
||||
.toUnicode()
|
||||
}
|
||||
|
||||
// impl via FFTV https://searchfox.org/mozilla-mobile/source/firefox-echo-show/app/src/main/java/org/mozilla/focus/utils/FormattedDomain.java#129
|
||||
fun String.isIpv4(): Boolean = Patterns.IP_ADDRESS.matcher(this).matches()
|
||||
|
||||
// impl via FFiOS: https://github.com/mozilla-mobile/firefox-ios/blob/deb9736c905cdf06822ecc4a20152df7b342925d/Shared/Extensions/NSURLExtensions.swift#L292
|
||||
// True IPv6 validation is difficult. This is slightly better than nothing
|
||||
private fun Uri.isIpv6(): Boolean {
|
||||
val host = this.host ?: return false
|
||||
return host.isNotEmpty() && host.contains(":")
|
||||
}
|
||||
|
||||
/**
|
||||
* Trim a host's prefix and suffix
|
||||
|
|
|
|||
|
|
@ -19,7 +19,7 @@ import org.mozilla.fenix.ext.components
|
|||
import org.mozilla.fenix.ext.getColorFromAttr
|
||||
import org.mozilla.fenix.ext.increaseTapArea
|
||||
import org.mozilla.fenix.ext.loadIntoView
|
||||
import org.mozilla.fenix.ext.urlToTrimmedHost
|
||||
import org.mozilla.fenix.ext.toShortUrl
|
||||
import org.mozilla.fenix.home.sessioncontrol.CollectionAction
|
||||
import org.mozilla.fenix.home.sessioncontrol.SessionControlAction
|
||||
import org.mozilla.fenix.home.sessioncontrol.TabCollection
|
||||
|
|
@ -70,7 +70,7 @@ class TabInCollectionViewHolder(
|
|||
}
|
||||
|
||||
private fun updateTabUI() {
|
||||
collection_tab_hostname.text = tab.url.urlToTrimmedHost(view.context)
|
||||
collection_tab_hostname.text = tab.url.toShortUrl(view.context.components.publicSuffixList)
|
||||
|
||||
collection_tab_title.text = tab.title
|
||||
collection_tab_icon.context.components.core.icons.loadIntoView(collection_tab_icon, tab.url)
|
||||
|
|
|
|||
|
|
@ -44,7 +44,7 @@ import org.mozilla.fenix.ext.bookmarkStorage
|
|||
import org.mozilla.fenix.ext.components
|
||||
import org.mozilla.fenix.ext.minus
|
||||
import org.mozilla.fenix.ext.nav
|
||||
import org.mozilla.fenix.ext.urlToTrimmedHost
|
||||
import org.mozilla.fenix.ext.toShortUrl
|
||||
import org.mozilla.fenix.library.LibraryPageFragment
|
||||
import org.mozilla.fenix.share.ShareTab
|
||||
import org.mozilla.fenix.utils.allowUndo
|
||||
|
|
@ -279,7 +279,7 @@ class BookmarkFragment : LibraryPageFragment<BookmarkNode>(), BackHandler {
|
|||
val bookmarkNode = selected.first()
|
||||
getString(
|
||||
R.string.bookmark_deletion_snackbar_message,
|
||||
bookmarkNode.url?.urlToTrimmedHost(context!!) ?: bookmarkNode.title
|
||||
bookmarkNode.url?.toShortUrl(context!!.components.publicSuffixList) ?: bookmarkNode.title
|
||||
)
|
||||
}
|
||||
else -> throw IllegalStateException("Illegal event type in onDeleteSome")
|
||||
|
|
|
|||
|
|
@ -44,7 +44,7 @@ import org.mozilla.fenix.ext.getRootView
|
|||
import org.mozilla.fenix.ext.nav
|
||||
import org.mozilla.fenix.ext.requireComponents
|
||||
import org.mozilla.fenix.ext.setToolbarColors
|
||||
import org.mozilla.fenix.ext.urlToTrimmedHost
|
||||
import org.mozilla.fenix.ext.toShortUrl
|
||||
import org.mozilla.fenix.library.bookmarks.BookmarksSharedViewModel
|
||||
import org.mozilla.fenix.library.bookmarks.DesktopFolders
|
||||
import java.util.concurrent.TimeUnit
|
||||
|
|
@ -191,7 +191,7 @@ class EditBookmarkFragment : Fragment(R.layout.fragment_edit_bookmark) {
|
|||
.setText(
|
||||
getString(
|
||||
R.string.bookmark_deletion_snackbar_message,
|
||||
it.url?.urlToTrimmedHost(activity) ?: it.title
|
||||
it.url?.toShortUrl(context.components.publicSuffixList) ?: it.title
|
||||
)
|
||||
)
|
||||
.show()
|
||||
|
|
|
|||
|
|
@ -1,18 +1,29 @@
|
|||
package org.mozilla.fenix.ext
|
||||
|
||||
import assertk.assertThat
|
||||
import assertk.assertions.contains
|
||||
import assertk.assertions.doesNotContain
|
||||
import assertk.assertions.isEqualTo
|
||||
import assertk.assertions.isFalse
|
||||
import assertk.assertions.isTrue
|
||||
import mozilla.components.support.test.robolectric.testContext
|
||||
import org.junit.Assert.assertEquals
|
||||
import org.junit.Ignore
|
||||
import org.junit.Test
|
||||
import org.junit.runner.RunWith
|
||||
import org.mozilla.fenix.TestApplication
|
||||
import org.robolectric.RobolectricTestRunner
|
||||
import org.robolectric.annotation.Config
|
||||
|
||||
const val PUNYCODE = "xn--kpry57d"
|
||||
const val IDN = "台灣"
|
||||
|
||||
@RunWith(RobolectricTestRunner::class)
|
||||
@Config(application = TestApplication::class)
|
||||
|
||||
class StringTest {
|
||||
|
||||
private val publicSuffixList = testContext.components.publicSuffixList
|
||||
|
||||
@Test
|
||||
fun replace() {
|
||||
val chars = mapOf("Mozilla Corporation" to "moco", "Mozilla Foundation" to "mofo")
|
||||
|
|
@ -31,7 +42,7 @@ class StringTest {
|
|||
@Test
|
||||
fun `Url To Trimmed Host`() {
|
||||
val urlTest = "http://www.example.com:1080/docs/resource1.html"
|
||||
val new = urlTest.urlToTrimmedHost(testContext)
|
||||
val new = urlTest.urlToTrimmedHost(testContext.components.publicSuffixList)
|
||||
assertEquals(new, "example")
|
||||
}
|
||||
|
||||
|
|
@ -41,4 +52,209 @@ class StringTest {
|
|||
val new = urlTest.simplifiedUrl()
|
||||
assertEquals(new, "amazon.com")
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `when the full hostname cannot be displayed, elide labels starting from the front`() {
|
||||
// See https://url.spec.whatwg.org/#url-rendering-elision
|
||||
// See https://chromium.googlesource.com/chromium/src/+/master/docs/security/url_display_guidelines/url_display_guidelines.md#eliding-urls
|
||||
|
||||
val display = "http://1.2.3.4.5.6.7.8.9.10.11.12.13.14.15.16.17.18.19.20.21.22.23.24.25.com"
|
||||
.shortened()
|
||||
|
||||
val split = display.split(".")
|
||||
|
||||
// If the list ends with 25.com...
|
||||
assertThat(split.dropLast(1).last()).isEqualTo("25")
|
||||
// ...and each value is 1 larger than the last...
|
||||
split.dropLast(1)
|
||||
.map { it.toInt() }
|
||||
.windowed(2, 1)
|
||||
.forEach { (prev, next) ->
|
||||
assertThat(prev + 1).isEqualTo(next)
|
||||
}
|
||||
// ...that means that all removed values came from the front of the list
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `the registrable domain is always displayed`() {
|
||||
// https://url.spec.whatwg.org/#url-rendering-elision
|
||||
// See https://chromium.googlesource.com/chromium/src/+/master/docs/security/url_display_guidelines/url_display_guidelines.md#eliding-urls
|
||||
|
||||
val bigRegistrableDomain = "evil-but-also-shockingly-long-registrable-domain.com"
|
||||
assertThat("https://login.your-bank.com.$bigRegistrableDomain/enter/your/password".shortened()).contains(bigRegistrableDomain)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `url username and password fields should not be displayed`() {
|
||||
// See https://url.spec.whatwg.org/#url-rendering-simplification
|
||||
// See https://chromium.googlesource.com/chromium/src/+/master/docs/security/url_display_guidelines/url_display_guidelines.md#simplify
|
||||
|
||||
assertThat("https://examplecorp.com@attacker.example/".shortened()).doesNotContain("examplecorp")
|
||||
assertThat("https://examplecorp.com@attacker.example/".shortened()).doesNotContain("com")
|
||||
assertThat("https://user:password@example.com/".shortened()).doesNotContain("user")
|
||||
assertThat("https://user:password@example.com/".shortened()).doesNotContain("password")
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `eTLDs should not be dropped`() {
|
||||
// See https://bugzilla.mozilla.org/show_bug.cgi?id=1554984#c11
|
||||
"http://mozfreddyb.github.io/" shortenedShouldBecome "mozfreddyb.github.io"
|
||||
"http://web.security.plumbing/" shortenedShouldBecome "web.security.plumbing"
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `ipv4 addresses should be returned as is`() {
|
||||
// See https://bugzilla.mozilla.org/show_bug.cgi?id=1554984#c11
|
||||
"192.168.85.1" shortenedShouldBecome "192.168.85.1"
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `about buildconfig should not be modified`() {
|
||||
// See https://bugzilla.mozilla.org/show_bug.cgi?id=1554984#c11
|
||||
"about:buildconfig" shortenedShouldBecome "about:buildconfig"
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `encoded userinfo should still be considered userinfo`() {
|
||||
"https://user:password%40really.evil.domain%2F@mail.google.com" shortenedShouldBecome "mail.google.com"
|
||||
}
|
||||
|
||||
@Test
|
||||
@Ignore("This would be more correct, but does not appear to be an attack vector")
|
||||
fun `should decode DWORD IP addresses`() {
|
||||
"https://16843009" shortenedShouldBecome "1.1.1.1"
|
||||
}
|
||||
|
||||
@Test
|
||||
@Ignore("This would be more correct, but does not appear to be an attack vector")
|
||||
fun `should decode octal IP addresses`() {
|
||||
"https://000010.000010.000010.000010" shortenedShouldBecome "8.8.8.8"
|
||||
}
|
||||
|
||||
@Test
|
||||
@Ignore("This would be more correct, but does not appear to be an attack vector")
|
||||
fun `should decode hex IP addresses`() {
|
||||
"http://0x01010101" shortenedShouldBecome "1.1.1.1"
|
||||
}
|
||||
|
||||
// BEGIN test cases borrowed from desktop (shortUrl is used for Top Sites on new tab)
|
||||
// Test cases are modified, as we show the eTLD
|
||||
// (https://searchfox.org/mozilla-central/source/browser/components/newtab/test/unit/lib/ShortUrl.test.js)
|
||||
@Test
|
||||
fun `should return a blank string if url is blank`() {
|
||||
"" shortenedShouldBecome ""
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `should return the 'url' if not a valid url`() {
|
||||
"something" shortenedShouldBecome "something"
|
||||
"http:" shortenedShouldBecome "http:"
|
||||
"http::double-colon" shortenedShouldBecome "http::double-colon"
|
||||
// The largest allowed port is 65,535
|
||||
"http://badport:65536/" shortenedShouldBecome "http://badport:65536/"
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `should convert host to idn when calling shortURL`() {
|
||||
"http://$PUNYCODE.blah.com" shortenedShouldBecome "$IDN.blah.com"
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `should get the hostname from url`() {
|
||||
"http://bar.com" shortenedShouldBecome "bar.com"
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `should not strip out www if not first subdomain`() {
|
||||
"http://foo.www.com" shortenedShouldBecome "foo.www.com"
|
||||
"http://www.foo.www.com" shortenedShouldBecome "foo.www.com"
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `should convert to lowercase`() {
|
||||
"HTTP://FOO.COM" shortenedShouldBecome "foo.com"
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `should not include the port`() {
|
||||
"http://foo.com:8888" shortenedShouldBecome "foo.com"
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `should return hostname for localhost`() {
|
||||
"http://localhost:8000/" shortenedShouldBecome "localhost"
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `should return hostname for ip address`() {
|
||||
"http://127.0.0.1/foo" shortenedShouldBecome "127.0.0.1"
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `should return etld for www gov uk (www-only non-etld)`() {
|
||||
"https://www.gov.uk/countersigning" shortenedShouldBecome "gov.uk"
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `should return idn etld for www-only non-etld`() {
|
||||
"https://www.$PUNYCODE/foo" shortenedShouldBecome IDN
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `should return not the protocol for file`() {
|
||||
"file:///foo/bar.txt" shortenedShouldBecome "/foo/bar.txt"
|
||||
}
|
||||
|
||||
@Test
|
||||
@Ignore("This behavior conflicts with https://bugzilla.mozilla.org/show_bug.cgi?id=1554984#c11")
|
||||
fun `should return not the protocol for about`() {
|
||||
"about:newtab" shortenedShouldBecome "newtab"
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `should fall back to full url as a last resort`() {
|
||||
"about:" shortenedShouldBecome "about:"
|
||||
}
|
||||
// END test cases borrowed from desktop
|
||||
|
||||
// BEGIN test cases borrowed from FFTV
|
||||
// (https://searchfox.org/mozilla-mobile/source/firefox-echo-show/app/src/test/java/org/mozilla/focus/utils/TestFormattedDomain.java#228)
|
||||
@Test
|
||||
fun testIsIPv4RealAddress() {
|
||||
assertThat("192.168.1.1".isIpv4()).isTrue()
|
||||
assertThat("8.8.8.8".isIpv4()).isTrue()
|
||||
assertThat("63.245.215.20".isIpv4()).isTrue()
|
||||
}
|
||||
|
||||
@Test
|
||||
fun testIsIPv4WithProtocol() {
|
||||
assertThat("http://8.8.8.8".isIpv4()).isFalse()
|
||||
assertThat("https://8.8.8.8".isIpv4()).isFalse()
|
||||
}
|
||||
|
||||
@Test
|
||||
fun testIsIPv4WithPort() {
|
||||
assertThat("8.8.8.8:400".isIpv4()).isFalse()
|
||||
assertThat("8.8.8.8:1337".isIpv4()).isFalse()
|
||||
}
|
||||
|
||||
@Test
|
||||
fun testIsIPv4WithPath() {
|
||||
assertThat("8.8.8.8/index.html".isIpv4()).isFalse()
|
||||
assertThat("8.8.8.8/".isIpv4()).isFalse()
|
||||
}
|
||||
|
||||
@Test
|
||||
fun testIsIPv4WithIPv6() {
|
||||
assertThat("2001:db8::1 ".isIpv4()).isFalse()
|
||||
assertThat("2001:db8:0:1:1:1:1:1".isIpv4()).isFalse()
|
||||
assertThat("[2001:db8:a0b:12f0::1]".isIpv4()).isFalse()
|
||||
}
|
||||
// END test cases borrowed from FFTV
|
||||
|
||||
private infix fun String.shortenedShouldBecome(expect: String) {
|
||||
assertThat(this.shortened()).isEqualTo(expect)
|
||||
}
|
||||
|
||||
private fun String.shortened() = this.toShortUrl(publicSuffixList)
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in New Issue