diff --git a/navigation-fragments/src/main/java/dev/hotwire/navigation/navigator/NavigatorHost.kt b/navigation-fragments/src/main/java/dev/hotwire/navigation/navigator/NavigatorHost.kt index 25291b7b..2fc433c6 100644 --- a/navigation-fragments/src/main/java/dev/hotwire/navigation/navigator/NavigatorHost.kt +++ b/navigation-fragments/src/main/java/dev/hotwire/navigation/navigator/NavigatorHost.kt @@ -15,6 +15,7 @@ import dev.hotwire.navigation.activities.HotwireActivity import dev.hotwire.navigation.config.HotwireNavigation internal const val DEEPLINK_EXTRAS_KEY = "android-support-nav:controller:deepLinkExtras" +internal const val DEEPLINK_ARGS_KEY = "android-support-nav:controller:deepLinkArgs" internal const val LOCATION_KEY = "location" open class NavigatorHost : NavHostFragment(), FragmentOnAttachListener { @@ -72,14 +73,22 @@ open class NavigatorHost : NavHostFragment(), FragmentOnAttachListener { } /** - * Google's Navigation library automatically navigates to deep links provided in the - * Activity's Intent. This exposes a vulnerability for malicious Intents to open an arbitrary - * webpage outside of the app's domain, allowing javascript injection on the page. Ensure - * that deep link intents always match the app's domain. + * Google's Navigation library automatically navigates to deep links provided in the launching + * Intent, which lets a malicious Intent open an arbitrary page in the WebView. Sanitize the + * Intent's attacker-controllable deep-link arguments so the start location stays within the + * app's domain. */ @VisibleForTesting(otherwise = PROTECTED) fun ensureDeeplinkStartLocationValid() { - val extrasBundle = activity.intent.extras?.getBundle(DEEPLINK_EXTRAS_KEY) ?: return + val intent = activity.intent + + // NavController merges deepLinkArgs over the validated deepLinkExtras (last write wins), so + // empty each per-destination bundle to stop it overriding the validated start location. + intent.extras?.getParcelableArrayList(DEEPLINK_ARGS_KEY)?.let { args -> + intent.putParcelableArrayListExtra(DEEPLINK_ARGS_KEY, ArrayList(args.map { Bundle() })) + } + + val extrasBundle = intent.extras?.getBundle(DEEPLINK_EXTRAS_KEY) ?: return val startLocation = extrasBundle.getString(LOCATION_KEY) ?: return val deepLinkStartUri = startLocation.toUri() @@ -87,7 +96,7 @@ open class NavigatorHost : NavHostFragment(), FragmentOnAttachListener { if (deepLinkStartUri.host != configStartUri.host) { extrasBundle.putString(LOCATION_KEY, configuration.startLocation) - activity.intent.putExtra(DEEPLINK_EXTRAS_KEY, extrasBundle) + intent.putExtra(DEEPLINK_EXTRAS_KEY, extrasBundle) } } diff --git a/navigation-fragments/src/test/kotlin/dev/hotwire/navigation/navigator/NavigatorHostTest.kt b/navigation-fragments/src/test/kotlin/dev/hotwire/navigation/navigator/NavigatorHostTest.kt index d6ddec32..6165ae6e 100644 --- a/navigation-fragments/src/test/kotlin/dev/hotwire/navigation/navigator/NavigatorHostTest.kt +++ b/navigation-fragments/src/test/kotlin/dev/hotwire/navigation/navigator/NavigatorHostTest.kt @@ -1,6 +1,5 @@ package dev.hotwire.navigation.navigator -import android.R.attr.host import android.content.Intent import android.os.Bundle import androidx.core.os.bundleOf @@ -25,28 +24,52 @@ class NavigatorHostTest { @Test fun `reverts to config start location when deep link host differs`() { - val extras = bundleOf(LOCATION_KEY to "https://other.com/path") - val intent = Intent().apply { putExtra(DEEPLINK_EXTRAS_KEY, extras) } + val intent = Intent().apply { + putExtra(DEEPLINK_EXTRAS_KEY, bundleOf(LOCATION_KEY to "https://other.com/path")) + } activity = Robolectric.buildActivity(TestActivity::class.java, intent).get() host.activity = activity host.ensureDeeplinkStartLocationValid() - val resultBundle = activity.intent.getBundleExtra(DEEPLINK_EXTRAS_KEY) - assertThat(resultBundle?.getString(LOCATION_KEY)).isEqualTo("https://example.com/start") + assertThat(activity.intent.getBundleExtra(DEEPLINK_EXTRAS_KEY)?.getString(LOCATION_KEY)) + .isEqualTo("https://example.com/start") } @Test fun `does not change start location when deep link host matches config`() { - val extras = bundleOf(LOCATION_KEY to "https://example.com/path") - val intent = Intent().apply { putExtra(DEEPLINK_EXTRAS_KEY, extras) } + val intent = Intent().apply { + putExtra(DEEPLINK_EXTRAS_KEY, bundleOf(LOCATION_KEY to "https://example.com/path")) + } activity = Robolectric.buildActivity(TestActivity::class.java, intent).get() host.activity = activity host.ensureDeeplinkStartLocationValid() - val resultBundle = activity.intent.getBundleExtra(DEEPLINK_EXTRAS_KEY) - assertThat(resultBundle?.getString(LOCATION_KEY)).isEqualTo("https://example.com/path") + assertThat(activity.intent.getBundleExtra(DEEPLINK_EXTRAS_KEY)?.getString(LOCATION_KEY)) + .isEqualTo("https://example.com/path") + } + + // NavController merges deepLinkArgs over deepLinkExtras (last write wins); the intent's args + // must not survive to override the validated start location. + @Test + fun `empties deepLinkArgs so they cannot override the start location`() { + val intent = Intent().apply { + putExtra(DEEPLINK_EXTRAS_KEY, bundleOf(LOCATION_KEY to "https://example.com/ok")) + putParcelableArrayListExtra(DEEPLINK_ARGS_KEY, arrayListOf(bundleOf(LOCATION_KEY to ATTACKER_URL))) + } + activity = Robolectric.buildActivity(TestActivity::class.java, intent).get() + + host.activity = activity + host.ensureDeeplinkStartLocationValid() + + val survivingArgs = activity.intent.getParcelableArrayListExtra(DEEPLINK_ARGS_KEY) + ?.mapNotNull { it.getString(LOCATION_KEY) }.orEmpty() + assertThat(survivingArgs).doesNotContain(ATTACKER_URL) + } + + companion object { + private const val ATTACKER_URL = "https://attacker.example/steal" } private class TestActivity : HotwireActivity() {