From 77263aad704844782a461b49a92b51a8bed31c16 Mon Sep 17 00:00:00 2001 From: Sebastian Kaspari Date: Wed, 5 Aug 2020 10:53:45 +0200 Subject: [PATCH] Validate deep links. --- .../java/org/mozilla/fenix/HomeActivity.kt | 24 ++++++++--- .../org/mozilla/fenix/components/Analytics.kt | 6 ++- .../metrics/LeanplumMetricsService.kt | 17 +++++++- .../home/intent/DeepLinkIntentProcessor.kt | 41 +++++++++++++++---- .../intent/DeepLinkIntentProcessorTest.kt | 40 ++++++++++++++++-- 5 files changed, 110 insertions(+), 18 deletions(-) diff --git a/app/src/main/java/org/mozilla/fenix/HomeActivity.kt b/app/src/main/java/org/mozilla/fenix/HomeActivity.kt index 7aaaa6eb3..479e168c9 100644 --- a/app/src/main/java/org/mozilla/fenix/HomeActivity.kt +++ b/app/src/main/java/org/mozilla/fenix/HomeActivity.kt @@ -41,6 +41,7 @@ import mozilla.components.browser.session.SessionManager import mozilla.components.browser.state.state.SessionState import mozilla.components.browser.state.state.WebExtensionState import mozilla.components.browser.state.store.BrowserStore +import mozilla.components.concept.engine.EngineSession import mozilla.components.concept.engine.EngineView import mozilla.components.feature.contextmenu.DefaultSelectionActionDelegate import mozilla.components.feature.search.BrowserStoreSearchAdapter @@ -136,7 +137,7 @@ open class HomeActivity : LocaleAwareAppCompatActivity(), NavHostActivity { listOf( SpeechProcessingIntentProcessor(this, components.analytics.metrics), StartSearchIntentProcessor(components.analytics.metrics), - DeepLinkIntentProcessor(this), + DeepLinkIntentProcessor(this, components.analytics.leanplumMetricsService), OpenBrowserIntentProcessor(this, ::getIntentSessionId), OpenSpecificTabIntentProcessor(this) ) @@ -525,6 +526,12 @@ open class HomeActivity : LocaleAwareAppCompatActivity(), NavHostActivity { protected open fun getIntentSessionId(intent: SafeIntent): String? = null + /** + * Navigates to the browser fragment and loads a URL or performs a search (depending on the + * value of [searchTermOrURL]). + * + * @param flags Flags that will be used when loading the URL (not applied to searches). + */ @Suppress("LongParameterList") fun openToBrowserAndLoad( searchTermOrURL: String, @@ -532,10 +539,11 @@ open class HomeActivity : LocaleAwareAppCompatActivity(), NavHostActivity { from: BrowserDirection, customTabSessionId: String? = null, engine: SearchEngine? = null, - forceSearch: Boolean = false + forceSearch: Boolean = false, + flags: EngineSession.LoadUrlFlags = EngineSession.LoadUrlFlags.none() ) { openToBrowser(from, customTabSessionId) - load(searchTermOrURL, newTab, engine, forceSearch) + load(searchTermOrURL, newTab, engine, forceSearch, flags) } fun openToBrowser(from: BrowserDirection, customTabSessionId: String? = null) { @@ -589,11 +597,17 @@ open class HomeActivity : LocaleAwareAppCompatActivity(), NavHostActivity { LoginDetailFragmentDirections.actionGlobalBrowser(customTabSessionId) } + /** + * Loads a URL or performs a search (depending on the value of [searchTermOrURL]). + * + * @param flags Flags that will be used when loading the URL (not applied to searches). + */ private fun load( searchTermOrURL: String, newTab: Boolean, engine: SearchEngine?, - forceSearch: Boolean + forceSearch: Boolean, + flags: EngineSession.LoadUrlFlags = EngineSession.LoadUrlFlags.none() ) { val startTime = components.core.engine.profiler?.getProfilerTime() val mode = browsingModeManager.mode @@ -619,7 +633,7 @@ open class HomeActivity : LocaleAwareAppCompatActivity(), NavHostActivity { } if (!forceSearch && searchTermOrURL.isUrl()) { - loadUrlUseCase.invoke(searchTermOrURL.toNormalizedUrl()) + loadUrlUseCase.invoke(searchTermOrURL.toNormalizedUrl(), flags) } else { searchUseCase.invoke(searchTermOrURL) } diff --git a/app/src/main/java/org/mozilla/fenix/components/Analytics.kt b/app/src/main/java/org/mozilla/fenix/components/Analytics.kt index d95125796..120a27ac1 100644 --- a/app/src/main/java/org/mozilla/fenix/components/Analytics.kt +++ b/app/src/main/java/org/mozilla/fenix/components/Analytics.kt @@ -84,12 +84,14 @@ class Analytics( ) } + val leanplumMetricsService by lazy { LeanplumMetricsService(context as Application) } + val metrics: MetricController by lazy { MetricController.create( listOf( GleanMetricsService(context), - LeanplumMetricsService(context as Application), - AdjustMetricsService(context) + leanplumMetricsService, + AdjustMetricsService(context as Application) ), isDataTelemetryEnabled = { context.settings().isTelemetryEnabled }, isMarketingDataTelemetryEnabled = { context.settings().isMarketingTelemetryEnabled } diff --git a/app/src/main/java/org/mozilla/fenix/components/metrics/LeanplumMetricsService.kt b/app/src/main/java/org/mozilla/fenix/components/metrics/LeanplumMetricsService.kt index 4d9ca393e..3fc636e4a 100644 --- a/app/src/main/java/org/mozilla/fenix/components/metrics/LeanplumMetricsService.kt +++ b/app/src/main/java/org/mozilla/fenix/components/metrics/LeanplumMetricsService.kt @@ -6,6 +6,7 @@ package org.mozilla.fenix.components.metrics import android.app.Application import android.content.Context.MODE_PRIVATE +import android.net.Uri import android.util.Log import androidx.annotation.VisibleForTesting import com.leanplum.Leanplum @@ -22,6 +23,7 @@ import mozilla.components.support.locale.LocaleManager import org.mozilla.fenix.BuildConfig import org.mozilla.fenix.components.metrics.MozillaProductDetector.MozillaProducts import org.mozilla.fenix.ext.settings +import org.mozilla.fenix.home.intent.DeepLinkIntentProcessor import java.util.Locale import java.util.MissingResourceException import java.util.UUID.randomUUID @@ -55,7 +57,7 @@ private val Event.name: String? class LeanplumMetricsService( private val application: Application, private val deviceIdGenerator: () -> String = { randomUUID().toString() } -) : MetricsService { +) : MetricsService, DeepLinkIntentProcessor.DeepLinkVerifier { val scope = CoroutineScope(Dispatchers.IO) var leanplumJob: Job? = null @@ -167,6 +169,19 @@ class LeanplumMetricsService( } } + /** + * Verifies a deep link and returns `true` for deep links that should be handled and `false` if + * a deep link should be rejected. + * + * @See DeepLinkIntentProcessor.verifier + */ + override fun verifyDeepLink(deepLink: Uri): Boolean { + // We compare the local Leanplum device ID against the "uid" query parameter and only + // accept deep links where both values match. + val uid = deepLink.getQueryParameter("uid") + return uid == deviceId + } + override fun stop() { if (application.settings().isMarketingTelemetryEnabled) return // As written in LeanPlum SDK documentation, "This prevents Leanplum from communicating with the server." diff --git a/app/src/main/java/org/mozilla/fenix/home/intent/DeepLinkIntentProcessor.kt b/app/src/main/java/org/mozilla/fenix/home/intent/DeepLinkIntentProcessor.kt index 740307cc4..5d1893db5 100644 --- a/app/src/main/java/org/mozilla/fenix/home/intent/DeepLinkIntentProcessor.kt +++ b/app/src/main/java/org/mozilla/fenix/home/intent/DeepLinkIntentProcessor.kt @@ -11,6 +11,8 @@ import android.os.Build import android.os.Build.VERSION.SDK_INT import android.provider.Settings import androidx.navigation.NavController +import mozilla.components.concept.engine.EngineSession +import mozilla.components.support.base.log.logger.Logger import org.mozilla.fenix.BrowserDirection import org.mozilla.fenix.BuildConfig import org.mozilla.fenix.GlobalDirections @@ -21,10 +23,14 @@ import org.mozilla.fenix.ext.alreadyOnDestination /** * Deep links in the form of `fenix://host` open different parts of the app. + * + * @param verifier [DeepLinkVerifier] that will be used to verify deep links before handling them. */ class DeepLinkIntentProcessor( - private val activity: HomeActivity + private val activity: HomeActivity, + private val verifier: DeepLinkVerifier ) : HomeIntentProcessor { + private val logger = Logger("DeepLinkIntentProcessor") override fun process(intent: Intent, navController: NavController, out: Intent): Boolean { val scheme = intent.scheme?.equals(BuildConfig.DEEP_LINK_SCHEME, ignoreCase = true) ?: return false @@ -38,6 +44,11 @@ class DeepLinkIntentProcessor( @Suppress("ComplexMethod") private fun handleDeepLink(deepLink: Uri, navController: NavController) { + if (!verifier.verifyDeepLink(deepLink)) { + logger.warn("Invalid deep link: $deepLink") + return + } + handleDeepLinkSideEffects(deepLink) val globalDirections = when (deepLink.host) { @@ -81,13 +92,18 @@ class DeepLinkIntentProcessor( } } "open" -> { - deepLink.getQueryParameter("url")?.let { searchTermOrUrl -> - activity.openToBrowserAndLoad( - searchTermOrUrl, - newTab = true, - from = BrowserDirection.FromGlobal - ) + val url = deepLink.getQueryParameter("url") + if (url == null || !url.startsWith("https://")) { + logger.info("Not opening deep link: $url") + return } + + activity.openToBrowserAndLoad( + url, + newTab = true, + from = BrowserDirection.FromGlobal, + flags = EngineSession.LoadUrlFlags.external() + ) } "settings_notifications" -> { val intent = notificationSettings(activity) @@ -123,4 +139,15 @@ class DeepLinkIntentProcessor( } } } + + /** + * Interface for a class that verifies deep links before they get handled. + */ + interface DeepLinkVerifier { + /** + * Verifies the given deep link and returns `true` for verified deep links or `false` for + * rejected deep links. + */ + fun verifyDeepLink(deepLink: Uri): Boolean + } } diff --git a/app/src/test/java/org/mozilla/fenix/home/intent/DeepLinkIntentProcessorTest.kt b/app/src/test/java/org/mozilla/fenix/home/intent/DeepLinkIntentProcessorTest.kt index 4e34aa574..19b959b2e 100644 --- a/app/src/test/java/org/mozilla/fenix/home/intent/DeepLinkIntentProcessorTest.kt +++ b/app/src/test/java/org/mozilla/fenix/home/intent/DeepLinkIntentProcessorTest.kt @@ -5,6 +5,7 @@ package org.mozilla.fenix.home.intent import android.content.Intent +import android.net.Uri import androidx.core.net.toUri import androidx.navigation.NavController import io.mockk.Called @@ -13,6 +14,7 @@ import io.mockk.mockk import io.mockk.mockkObject import io.mockk.verify import mozilla.appservices.places.BookmarkRoot +import mozilla.components.concept.engine.EngineSession import org.junit.Assert.assertFalse import org.junit.Assert.assertTrue import org.junit.Before @@ -39,7 +41,11 @@ class DeepLinkIntentProcessorTest { activity = mockk(relaxed = true) navController = mockk(relaxed = true) out = mockk() - processor = DeepLinkIntentProcessor(activity) + processor = DeepLinkIntentProcessor(activity, object : DeepLinkIntentProcessor.DeepLinkVerifier { + override fun verifyDeepLink(deepLink: Uri): Boolean { + return true + } + }) } @Test @@ -198,17 +204,45 @@ class DeepLinkIntentProcessorTest { assertTrue(processor.process(testIntent("open?url=test"), navController, out)) + verify { activity wasNot Called } + verify { navController wasNot Called } + verify { out wasNot Called } + + assertTrue(processor.process(testIntent("open?url=https%3A%2F%2Fwww.example.org%2F"), navController, out)) + verify { activity.openToBrowserAndLoad( - "test", + "https://www.example.org/", newTab = true, - from = BrowserDirection.FromGlobal + from = BrowserDirection.FromGlobal, + flags = EngineSession.LoadUrlFlags.external() ) } verify { navController wasNot Called } verify { out wasNot Called } } + @Test + fun `process invalid open deep link`() { + val invalidProcessor = DeepLinkIntentProcessor(activity, object : DeepLinkIntentProcessor.DeepLinkVerifier { + override fun verifyDeepLink(deepLink: Uri): Boolean { + return false + } + }) + + assertTrue(invalidProcessor.process(testIntent("open"), navController, out)) + + verify { activity wasNot Called } + verify { navController wasNot Called } + verify { out wasNot Called } + + assertTrue(invalidProcessor.process(testIntent("open?url=open?url=https%3A%2F%2Fwww.example.org%2F"), navController, out)) + + verify { activity wasNot Called } + verify { navController wasNot Called } + verify { out wasNot Called } + } + @Test fun `process make_default_browser deep link`() { assertTrue(processor.process(testIntent("make_default_browser"), navController, out))