Merge pull request #355 from hotwired/mb/deeplink_args_vulnerability_fix

Validate deep link args when resolving the start location

Author: mbarta

turbo/src/main/kotlin/dev/hotwire/turbo/session/TurboSessionNavHostFragment.kt

@@ -17,6 +17,7 @@ import dev.hotwire.turbo.views.TurboWebView
 import kotlin.reflect.KClass
 
 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"
 
 abstract class TurboSessionNavHostFragment : NavHostFragment() {
@@ -114,22 +115,30 @@ abstract class TurboSessionNavHostFragment : NavHostFragment() {
             ?: throw IllegalStateException("No current destination found in NavHostFragment")
 
     /**
-     * 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
     internal fun ensureDeeplinkStartLocationValid(activity: FragmentActivity) {
-        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<Bundle>(DEEPLINK_ARGS_KEY)?.let { args ->
+            intent.putParcelableArrayListExtra(DEEPLINK_ARGS_KEY, ArrayList(args.map { Bundle() }))
+        }
+
+        val extrasBundle = intent.extras?.getBundle(DEEPLINK_EXTRAS_KEY) ?: return
         val startLocationFromIntent = extrasBundle.getString(LOCATION_KEY) ?: return
 
         val deepLinkStartUri = startLocationFromIntent.toUri()
         val configStartUri = startLocation.toUri()
 
         if (deepLinkStartUri.host != configStartUri.host) {
             extrasBundle.putString(LOCATION_KEY, startLocation)
-            activity.intent.putExtra(DEEPLINK_EXTRAS_KEY, extrasBundle)
+            intent.putExtra(DEEPLINK_EXTRAS_KEY, extrasBundle)
         }
     }
 

turbo/src/test/kotlin/dev/hotwire/turbo/session/TurboSessionNavHostFragmentTest.kt

@@ -2,6 +2,7 @@ package dev.hotwire.turbo.session
 
 import android.content.Intent
 import android.os.Build
+import android.os.Bundle
 import androidx.appcompat.app.AppCompatActivity
 import androidx.core.os.bundleOf
 import androidx.fragment.app.Fragment
@@ -54,6 +55,28 @@ class TurboSessionNavHostFragmentTest : BaseUnitTest() {
         assertThat(resultBundle?.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).create().get()
+
+        host = TestNavHostFragment()
+        host.ensureDeeplinkStartLocationValid(activity)
+
+        val survivingArgs = activity.intent.getParcelableArrayListExtra<Bundle>(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"
+    }
+
 }
 
 class TestActivity : AppCompatActivity()