From 11994247535d55db0d8ff2bd63a90d04f6fe3241 Mon Sep 17 00:00:00 2001 From: Severin Rudie Date: Fri, 15 Nov 2019 14:25:50 -0800 Subject: [PATCH] [fenix] 4844 fix url elision (https://github.com/mozilla-mobile/fenix/pull/6588) * For https://github.com/mozilla-mobile/fenix/issues/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 case --- .../collections/CollectionCreationView.kt | 4 +- .../fenix/components/TabCollectionStorage.kt | 5 +- .../java/org/mozilla/fenix/ext/Session.kt | 14 +- .../main/java/org/mozilla/fenix/ext/String.kt | 78 ++++++- .../viewholders/TabInCollectionViewHolder.kt | 4 +- .../library/bookmarks/BookmarkFragment.kt | 4 +- .../bookmarks/edit/EditBookmarkFragment.kt | 4 +- .../java/org/mozilla/fenix/ext/StringTest.kt | 220 +++++++++++++++++- 8 files changed, 310 insertions(+), 23 deletions(-) diff --git a/app/src/main/java/org/mozilla/fenix/collections/CollectionCreationView.kt b/app/src/main/java/org/mozilla/fenix/collections/CollectionCreationView.kt index 990f750081..cd1e36c426 100644 --- a/app/src/main/java/org/mozilla/fenix/collections/CollectionCreationView.kt +++ b/app/src/main/java/org/mozilla/fenix/collections/CollectionCreationView.kt @@ -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 -> diff --git a/app/src/main/java/org/mozilla/fenix/components/TabCollectionStorage.kt b/app/src/main/java/org/mozilla/fenix/components/TabCollectionStorage.kt index daa407f46e..44bbba6724 100644 --- a/app/src/main/java/org/mozilla/fenix/components/TabCollectionStorage.kt +++ b/app/src/main/java/org/mozilla/fenix/components/TabCollectionStorage.kt @@ -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( diff --git a/app/src/main/java/org/mozilla/fenix/ext/Session.kt b/app/src/main/java/org/mozilla/fenix/ext/Session.kt index ff6ebcc3c1..aa615e965d 100644 --- a/app/src/main/java/org/mozilla/fenix/ext/Session.kt +++ b/app/src/main/java/org/mozilla/fenix/ext/Session.kt @@ -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 ) } diff --git a/app/src/main/java/org/mozilla/fenix/ext/String.kt b/app/src/main/java/org/mozilla/fenix/ext/String.kt index ce22e79d7f..659c6e3265 100644 --- a/app/src/main/java/org/mozilla/fenix/ext/String.kt +++ b/app/src/main/java/org/mozilla/fenix/ext/String.kt @@ -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 diff --git a/app/src/main/java/org/mozilla/fenix/home/sessioncontrol/viewholders/TabInCollectionViewHolder.kt b/app/src/main/java/org/mozilla/fenix/home/sessioncontrol/viewholders/TabInCollectionViewHolder.kt index 758de0a310..94187b63a9 100644 --- a/app/src/main/java/org/mozilla/fenix/home/sessioncontrol/viewholders/TabInCollectionViewHolder.kt +++ b/app/src/main/java/org/mozilla/fenix/home/sessioncontrol/viewholders/TabInCollectionViewHolder.kt @@ -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) diff --git a/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkFragment.kt b/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkFragment.kt index c4b531786a..5b964a642f 100644 --- a/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkFragment.kt +++ b/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkFragment.kt @@ -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(), 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") diff --git a/app/src/main/java/org/mozilla/fenix/library/bookmarks/edit/EditBookmarkFragment.kt b/app/src/main/java/org/mozilla/fenix/library/bookmarks/edit/EditBookmarkFragment.kt index b2e2bbca4a..8dfa2d8744 100644 --- a/app/src/main/java/org/mozilla/fenix/library/bookmarks/edit/EditBookmarkFragment.kt +++ b/app/src/main/java/org/mozilla/fenix/library/bookmarks/edit/EditBookmarkFragment.kt @@ -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() diff --git a/app/src/test/java/org/mozilla/fenix/ext/StringTest.kt b/app/src/test/java/org/mozilla/fenix/ext/StringTest.kt index 8d1ba98dd7..3f222d5c9c 100644 --- a/app/src/test/java/org/mozilla/fenix/ext/StringTest.kt +++ b/app/src/test/java/org/mozilla/fenix/ext/StringTest.kt @@ -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) }