fix(push): address CodeRabbit findings on open_url PR (#522)

Author: morrilltim

README.md

@@ -703,6 +703,7 @@ See the table below to understand available features by SDK version.
 | Audience Targeting   | 4.0.0               |
 | Event Triggers       | 4.1.0               |
 | Form Lifecycle Hooks | 4.4.0               |
+| External URL CTAs    | 4.5.0               |
 
 ### Setup
 To begin, call `Klaviyo.registerForInAppForms()` after initializing the SDK with your public API key.
@@ -797,7 +798,8 @@ object to the `registerForInAppForms()` method. For example, to set a session ti
 
 ### Monitoring Form Lifecycle Events
 
-> Form lifecycle events are available in SDK version 4.4.0 and higher.
+> Form lifecycle events are available in SDK version 4.4.0 and higher. External URL CTA
+> routing and `event.deepLinkUrl` support for external URLs require SDK version 4.5.0 and higher.
 
 You can register a handler to receive callbacks whenever a form is shown, dismissed, or a CTA button is tapped.
 This is useful for forwarding engagement data to a third-party analytics platform such as Amplitude, Segment, or Mixpanel.
@@ -824,8 +826,8 @@ The handler is invoked on the **main thread**, so avoid performing long-running
                // e.g. myAnalytics.track("Form Dismissed", mapOf("formId" to event.formId, "formName" to event.formName))
            }
            is FormCtaClicked -> {
-               // Fires for both in-app deep-link CTAs and CTAs that open an external URL;
-               // event.deepLinkUrl carries whichever URL the CTA navigates to.
+               // Fires for both in-app deep-link CTAs and CTAs that open an external URL
+               // (SDK 4.5.0+); event.deepLinkUrl carries whichever URL the CTA navigates to.
                // e.g. myAnalytics.track("Form CTA Clicked", mapOf(
                //     "formId" to event.formId,
                //     "formName" to event.formName,
@@ -854,8 +856,8 @@ The handler is invoked on the **main thread**, so avoid performing long-running
        } else if (event instanceof FormLifecycleEvent.FormDismissed dismissed) {
            // e.g. myAnalytics.track("Form Dismissed", ...)
        } else if (event instanceof FormLifecycleEvent.FormCtaClicked ctaClicked) {
-           // Fires for both deep-link and external-URL CTAs; ctaClicked.getDeepLinkUrl()
-           // carries whichever URL the CTA navigates to.
+           // Fires for both deep-link and external-URL CTAs (SDK 4.5.0+);
+           // ctaClicked.getDeepLinkUrl() carries whichever URL the CTA navigates to.
            // e.g. myAnalytics.track("Form CTA Clicked", ...)
        }
    });

sdk/core/src/main/java/com/klaviyo/core/utils/UriExtension.kt

@@ -0,0 +1,11 @@
+package com.klaviyo.core.utils
+
+import android.net.Uri
+import com.klaviyo.core.Constants.ALLOWED_OPEN_URL_SCHEMES
+
+/**
+ * True if this URI's scheme is in [ALLOWED_OPEN_URL_SCHEMES]. Shared by the `push-fcm` and
+ * `forms` modules so the `web_url`/`open_url`/form-CTA allowlist gate can never diverge
+ * between them.
+ */
+fun Uri.hasAllowedOpenUrlScheme(): Boolean = scheme?.lowercase() in ALLOWED_OPEN_URL_SCHEMES

sdk/forms-core/src/main/java/com/klaviyo/forms/FormLifecycleEvent.kt

@@ -47,14 +47,16 @@ sealed interface FormLifecycleEvent {
     /**
      * Triggered when a user taps a call-to-action (CTA) button in a form
      * that has a URL configured, whether it deep links within the host app
-     * or opens an external URL in the default browser.
+     * or opens externally (a browser, dialer, or messaging app, depending
+     * on the URL's scheme).
      *
      * Fired after the SDK has initiated navigation. Not emitted if no URL is
      * configured for the CTA.
      *
      * @property buttonLabel The text label of the CTA button.
      * @property deepLinkUrl The URI the CTA navigates to. This is an in-app deep
-     * link for deep-link CTAs, or the external URL for CTAs that open the browser.
+     * link for deep-link CTAs, or the external URL (opened in a browser, dialer,
+     * or messaging app depending on its scheme) for external CTAs.
      */
     data class FormCtaClicked(
         override val formId: String,

sdk/forms/src/main/java/com/klaviyo/forms/bridge/KlaviyoNativeBridge.kt

@@ -10,8 +10,8 @@ import androidx.webkit.WebViewFeature.WEB_MESSAGE_LISTENER
 import com.klaviyo.analytics.Klaviyo
 import com.klaviyo.analytics.linking.DeepLinking
 import com.klaviyo.analytics.networking.ApiClient
-import com.klaviyo.core.Constants.ALLOWED_OPEN_URL_SCHEMES
 import com.klaviyo.core.Registry
+import com.klaviyo.core.utils.hasAllowedOpenUrlScheme
 import com.klaviyo.core.utils.startActivityIfResolved
 import com.klaviyo.forms.FormLifecycleEvent
 import com.klaviyo.forms.FormLifecycleHandler
@@ -133,14 +133,16 @@ internal class KlaviyoNativeBridge : NativeBridge {
      * next activity resuming; [DeepLinking.handleDeepLink] alleviates that race by postponing until
      * the next activity resumes if the current activity is null.
      *
-     * When true, the URL is opened in the default browser via a non-package-scoped intent, bypassing
-     * any registered deep link handler — mirroring
+     * When true, the URL is routed to its external handler via a non-package-scoped intent —
+     * a browser for `http`/`https`, or the mail, dialer, or SMS app for
+     * `mailto:`/`tel:`/`sms:`/`smsto:` — bypassing any registered deep link handler and mirroring
      * [com.klaviyo.forms.webview.KlaviyoWebViewClient.shouldOverrideUrlLoading]. The `NEW_TASK` intent
      * launches independently of the overlay activity, so no grace period is needed. The scheme is
-     * checked against [ALLOWED_OPEN_URL_SCHEMES] first — the same allowlist gate applied to push's
-     * `open_url`/`web_url` fields (see [com.klaviyo.pushFcm.KlaviyoRemoteMessage], PUSH-834) — to
-     * avoid routing dangerous or unintended URIs (e.g. `intent:`, `javascript:`, `file:`). The intent
-     * is built by [DeepLinking.makeExternalIntent], shared with the push `open_url` path.
+     * checked via [com.klaviyo.core.utils.hasAllowedOpenUrlScheme] first — the same allowlist gate
+     * applied to push's `open_url`/`web_url` fields (see [com.klaviyo.pushFcm.KlaviyoRemoteMessage],
+     * PUSH-834) — to avoid routing dangerous or unintended URIs (e.g. `intent:`, `javascript:`,
+     * `file:`). The intent is built by [DeepLinking.makeExternalIntent], shared with the push
+     * `open_url` path.
      *
      * Fires [FormLifecycleEvent.FormCtaClicked] after dispatch, with the URL carried in
      * [FormLifecycleEvent.FormCtaClicked.deepLinkUrl].
@@ -154,7 +156,7 @@ internal class KlaviyoNativeBridge : NativeBridge {
         }
 
         if (message.openExternally) {
-            if (uri.scheme?.lowercase() !in ALLOWED_OPEN_URL_SCHEMES) {
+            if (!uri.hasAllowedOpenUrlScheme()) {
                 Registry.log.warning(
                     "Form CTA external url has a scheme not in the allowed list " +
                         "('${uri.scheme}'); ignoring."

sdk/forms/src/main/java/com/klaviyo/forms/bridge/NativeBridgeMessage.kt

@@ -66,8 +66,10 @@ internal sealed class NativeBridgeMessage {
      * @param formId The form ID of the form that triggered the CTA
      * @param formName The name of the form that triggered the CTA
      * @param buttonLabel The text label of the CTA button that was clicked
-     * @param openExternally When true, open the URL in the default browser (bypassing any registered
-     * deep link handler), gated by the scheme allowlist. When false, route it as an in-app deep link.
+     * @param openExternally When true, route the URL to its external handler (a browser for
+     * `http`/`https`, or the mail, dialer, or SMS app for `mailto:`/`tel:`/`sms:`/`smsto:`),
+     * bypassing any registered deep link handler, gated by the scheme allowlist. When false,
+     * route it as an in-app deep link.
      */
     data class OpenDeepLink(
         val route: String?,

sdk/push-fcm/src/main/java/com/klaviyo/pushFcm/KlaviyoRemoteMessage.kt

@@ -15,12 +15,12 @@ import androidx.core.graphics.toColorInt
 import androidx.core.net.toUri
 import com.google.firebase.messaging.CommonNotificationBuilder
 import com.google.firebase.messaging.RemoteMessage
-import com.klaviyo.core.Constants.ALLOWED_OPEN_URL_SCHEMES
 import com.klaviyo.core.Constants.PACKAGE_PREFIX
 import com.klaviyo.core.Constants.TRACKING_PARAMETER
 import com.klaviyo.core.Registry
 import com.klaviyo.core.config.getApplicationInfoCompat
 import com.klaviyo.core.config.getManifestInt
+import com.klaviyo.core.utils.hasAllowedOpenUrlScheme
 import java.net.URL
 import org.json.JSONArray
 import org.json.JSONObject
@@ -133,19 +133,20 @@ object KlaviyoRemoteMessage {
     val RemoteMessage.body: String? get() = this.data[KlaviyoNotification.BODY_KEY]
 
     /**
-     * True if the string parses as a Uri whose scheme is in [ALLOWED_OPEN_URL_SCHEMES].
+     * True if the string parses as a Uri whose scheme is allowed — see
+     * [com.klaviyo.core.utils.hasAllowedOpenUrlScheme], shared with the forms module so the
+     * two call sites can never diverge.
      */
-    internal fun String.hasAllowedOpenUrlScheme(): Boolean =
-        this.toUri().scheme?.lowercase() in ALLOWED_OPEN_URL_SCHEMES
+    internal fun String.hasAllowedOpenUrlScheme(): Boolean = this.toUri().hasAllowedOpenUrlScheme()
 
     /**
      * Parse the external URL from the payload, if present.
      *
      * Reads the `web_url` field. The presence of this field indicates the tap should open
      * the URL externally rather than route through the app's deep link handling.
-     * Returns null if the field is absent, blank, or the URL's scheme is not in
-     * [ALLOWED_OPEN_URL_SCHEMES] — disallowed schemes are rejected to prevent routing
-     * dangerous URIs (e.g. intent:, javascript:, file:) through the SDK.
+     * Returns null if the field is absent, blank, or the URL's scheme is not allowed —
+     * disallowed schemes are rejected to prevent routing dangerous URIs (e.g. intent:,
+     * javascript:, file:) through the SDK.
      */
     val RemoteMessage.webUrl: String?
         get() {

sdk/push-fcm/src/main/java/com/klaviyo/pushFcm/KlaviyoTrampolineActivity.kt

@@ -33,6 +33,11 @@ internal class KlaviyoTrampolineActivity : Activity() {
         super.onCreate(savedInstanceState)
         try {
             handleTrampolineIntent(intent, this)
+        } catch (e: Exception) {
+            // Must not throw: this is an invisible entry point for notification taps —
+            // an uncaught exception here would crash the host app, which is worse than
+            // the stuck-screen risk the `finally` below already guards against.
+            Registry.log.error("KlaviyoTrampolineActivity failed to dispatch", e)
         } finally {
             // Always finish — leaving a translucent activity onscreen after an exception
             // would look like a stuck blank screen to the user.

sdk/push-fcm/src/test/java/com/klaviyo/pushFcm/KlaviyoRemoteMessageTest.kt

@@ -1,6 +1,7 @@
 package com.klaviyo.pushFcm
 
 import android.content.Intent
+import android.net.Uri
 import com.google.firebase.messaging.RemoteMessage
 import com.klaviyo.fixtures.BaseTest
 import com.klaviyo.pushFcm.KlaviyoNotification.Companion.ACTION_BUTTONS_KEY
@@ -22,9 +23,23 @@ import io.mockk.unmockkStatic
 import io.mockk.verify
 import org.json.JSONArray
 import org.json.JSONObject
+import org.junit.After
+import org.junit.Before
 import org.junit.Test
 
 class KlaviyoRemoteMessageTest : BaseTest() {
+    @Before
+    override fun setup() {
+        super.setup()
+        mockkStatic(Uri::class)
+    }
+
+    @After
+    override fun cleanup() {
+        unmockkStatic(Uri::class)
+        super.cleanup()
+    }
+
     private val stubKeyValuePairs = mapOf(
         "test_key_1" to "test_value_1",
         "test_key_2" to "test_value_2",
@@ -527,10 +542,9 @@ class KlaviyoRemoteMessageTest : BaseTest() {
 
     @Test
     fun `webUrl returns Uri when web_url is an https URL`() {
-        val mockUri = mockk<android.net.Uri>(relaxed = true)
+        val mockUri = mockk<Uri>(relaxed = true)
         every { mockUri.scheme } returns "https"
-        mockkStatic(android.net.Uri::class)
-        every { android.net.Uri.parse("https://example.com") } returns mockUri
+        every { Uri.parse("https://example.com") } returns mockUri
 
         val msg = mockk<RemoteMessage>()
         every { msg.data } returns stubMessage.toMutableMap().apply {
@@ -539,26 +553,21 @@ class KlaviyoRemoteMessageTest : BaseTest() {
 
         val webUrl = msg.webUrl
         assert(webUrl != null)
-        verify { android.net.Uri.parse("https://example.com") }
-
-        unmockkStatic(android.net.Uri::class)
+        verify { Uri.parse("https://example.com") }
     }
 
     @Test
     fun `webUrl returns Uri when web_url is an http URL`() {
-        val mockUri = mockk<android.net.Uri>(relaxed = true)
+        val mockUri = mockk<Uri>(relaxed = true)
         every { mockUri.scheme } returns "http"
-        mockkStatic(android.net.Uri::class)
-        every { android.net.Uri.parse("http://example.com") } returns mockUri
+        every { Uri.parse("http://example.com") } returns mockUri
 
         val msg = mockk<RemoteMessage>()
         every { msg.data } returns stubMessage.toMutableMap().apply {
             put(KlaviyoNotification.WEB_URL_KEY, "http://example.com")
         }
 
         assert(msg.webUrl != null)
-
-        unmockkStatic(android.net.Uri::class)
     }
 
     @Test
@@ -581,25 +590,20 @@ class KlaviyoRemoteMessageTest : BaseTest() {
 
     @Test
     fun `webUrl returns null when web_url has a blocked scheme`() {
-        val mockUri = mockk<android.net.Uri>(relaxed = true)
+        val mockUri = mockk<Uri>(relaxed = true)
         every { mockUri.scheme } returns "javascript"
-        mockkStatic(android.net.Uri::class)
-        every { android.net.Uri.parse("javascript:alert(1)") } returns mockUri
+        every { Uri.parse("javascript:alert(1)") } returns mockUri
 
         val msg = mockk<RemoteMessage>()
         every { msg.data } returns stubMessage.toMutableMap().apply {
             put(KlaviyoNotification.WEB_URL_KEY, "javascript:alert(1)")
         }
 
         assert(msg.webUrl == null)
-
-        unmockkStatic(android.net.Uri::class)
     }
 
     @Test
     fun `webUrl returns url when web_url has an allowlisted communication scheme`() {
-        mockkStatic(android.net.Uri::class)
-
         val schemes = listOf(
             "mailto" to "mailto:user@example.com",
             "tel" to "tel:+15555550100",
@@ -608,9 +612,9 @@ class KlaviyoRemoteMessageTest : BaseTest() {
         )
 
         for ((scheme, url) in schemes) {
-            val mockUri = mockk<android.net.Uri>(relaxed = true)
+            val mockUri = mockk<Uri>(relaxed = true)
             every { mockUri.scheme } returns scheme
-            every { android.net.Uri.parse(url) } returns mockUri
+            every { Uri.parse(url) } returns mockUri
 
             val msg = mockk<RemoteMessage>()
             every { msg.data } returns stubMessage.toMutableMap().apply {
@@ -621,16 +625,13 @@ class KlaviyoRemoteMessageTest : BaseTest() {
                 "Expected webUrl to return '$url' for scheme $scheme"
             }
         }
-
-        unmockkStatic(android.net.Uri::class)
     }
 
     @Test
     fun `webUrl returns parsed URL even when url field is also present`() {
-        val mockUri = mockk<android.net.Uri>(relaxed = true)
+        val mockUri = mockk<Uri>(relaxed = true)
         every { mockUri.scheme } returns "https"
-        mockkStatic(android.net.Uri::class)
-        every { android.net.Uri.parse("https://example.com") } returns mockUri
+        every { Uri.parse("https://example.com") } returns mockUri
 
         val msg = mockk<RemoteMessage>()
         every { msg.data } returns stubMessage.toMutableMap().apply {
@@ -639,16 +640,13 @@ class KlaviyoRemoteMessageTest : BaseTest() {
         }
 
         assert(msg.webUrl != null)
-
-        unmockkStatic(android.net.Uri::class)
     }
 
     @Test
     fun `actionButtons parses open_url variant with url`() {
-        val mockUri = mockk<android.net.Uri>(relaxed = true)
+        val mockUri = mockk<Uri>(relaxed = true)
         every { mockUri.scheme } returns "https"
-        mockkStatic(android.net.Uri::class)
-        every { android.net.Uri.parse("https://example.com") } returns mockUri
+        every { Uri.parse("https://example.com") } returns mockUri
 
         val actionButtonsData = listOf(
             mapOf(
@@ -674,16 +672,13 @@ class KlaviyoRemoteMessageTest : BaseTest() {
         assert(button?.id == "open.url")
         assert(button?.label == "Open Website")
         assert((button as? ActionButton.OpenUrl)?.url == "https://example.com")
-
-        unmockkStatic(android.net.Uri::class)
     }
 
     @Test
     fun `actionButtons skips open_url with blocked scheme`() {
-        val mockUri = mockk<android.net.Uri>(relaxed = true)
+        val mockUri = mockk<Uri>(relaxed = true)
         every { mockUri.scheme } returns "intent"
-        mockkStatic(android.net.Uri::class)
-        every { android.net.Uri.parse("intent://evil") } returns mockUri
+        every { Uri.parse("intent://evil") } returns mockUri
 
         val actionButtonsData = listOf(
             mapOf(
@@ -701,14 +696,10 @@ class KlaviyoRemoteMessageTest : BaseTest() {
         every { msg.data } returns messageWithActions
 
         assert(msg.actionButtons == null)
-
-        unmockkStatic(android.net.Uri::class)
     }
 
     @Test
     fun `actionButtons accepts open_url with allowlisted communication schemes`() {
-        mockkStatic(android.net.Uri::class)
-
         val schemes = listOf(
             "mailto" to "mailto:user@example.com",
             "tel" to "tel:+15555550100",
@@ -717,9 +708,9 @@ class KlaviyoRemoteMessageTest : BaseTest() {
         )
 
         for ((scheme, url) in schemes) {
-            val mockUri = mockk<android.net.Uri>(relaxed = true)
+            val mockUri = mockk<Uri>(relaxed = true)
             every { mockUri.scheme } returns scheme
-            every { android.net.Uri.parse(url) } returns mockUri
+            every { Uri.parse(url) } returns mockUri
 
             val actionButtonsData = listOf(
                 mapOf(
@@ -746,8 +737,6 @@ class KlaviyoRemoteMessageTest : BaseTest() {
                 "Expected url $url for scheme $scheme"
             }
         }
-
-        unmockkStatic(android.net.Uri::class)
     }
 
     @Test