Skip to content

Commit 1ad762d

Browse files
morrilltimmorrilltim
committed
fix(push): address CodeRabbit findings on open_url PR
Addresses findings from #518: - KlaviyoTrampolineActivity.onCreate now catches exceptions from handleTrampolineIntent instead of letting them propagate past the finally block, matching the must-not-throw pattern for notification tap entry points. - Centralized the open_url scheme-allowlist check into a shared Uri.hasAllowedOpenUrlScheme() extension in core, used by both KlaviyoRemoteMessage (push-fcm) and KlaviyoNativeBridge (forms) instead of each module reimplementing the check separately. - Moved KlaviyoRemoteMessageTest's mockkStatic(Uri::class)/ unmockkStatic(Uri::class) into class-level setup()/cleanup(), matching KlaviyoNotificationTest's pattern, so a failed assertion can't leak the static mock into later tests. - Replaced inline fully-qualified android.net.Uri references in KlaviyoRemoteMessageTest with an import, per repo style. - Documented the SDK 4.5.0 requirement for external URL CTAs in the README, and corrected "opens in the default browser" wording in NativeBridgeMessage/KlaviyoNativeBridge/FormLifecycleEvent docs — openExternally also routes tel:/mailto:/sms:/smsto: to the dialer or messaging apps, not just a browser.
1 parent 47de223 commit 1ad762d

8 files changed

Lines changed: 81 additions & 67 deletions

File tree

‎README.md‎

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -703,6 +703,7 @@ See the table below to understand available features by SDK version.
703703
| Audience Targeting | 4.0.0 |
704704
| Event Triggers | 4.1.0 |
705705
| Form Lifecycle Hooks | 4.4.0 |
706+
| External URL CTAs | 4.5.0 |
706707

707708
### Setup
708709
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
797798

798799
### Monitoring Form Lifecycle Events
799800

800-
> Form lifecycle events are available in SDK version 4.4.0 and higher.
801+
> Form lifecycle events are available in SDK version 4.4.0 and higher. External URL CTA
802+
> routing and `event.deepLinkUrl` support for external URLs require SDK version 4.5.0 and higher.
801803

802804
You can register a handler to receive callbacks whenever a form is shown, dismissed, or a CTA button is tapped.
803805
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
824826
// e.g. myAnalytics.track("Form Dismissed", mapOf("formId" to event.formId, "formName" to event.formName))
825827
}
826828
is FormCtaClicked -> {
827-
// Fires for both in-app deep-link CTAs and CTAs that open an external URL;
828-
// event.deepLinkUrl carries whichever URL the CTA navigates to.
829+
// Fires for both in-app deep-link CTAs and CTAs that open an external URL
830+
// (SDK 4.5.0+); event.deepLinkUrl carries whichever URL the CTA navigates to.
829831
// e.g. myAnalytics.track("Form CTA Clicked", mapOf(
830832
// "formId" to event.formId,
831833
// "formName" to event.formName,
@@ -854,8 +856,8 @@ The handler is invoked on the **main thread**, so avoid performing long-running
854856
} else if (event instanceof FormLifecycleEvent.FormDismissed dismissed) {
855857
// e.g. myAnalytics.track("Form Dismissed", ...)
856858
} else if (event instanceof FormLifecycleEvent.FormCtaClicked ctaClicked) {
857-
// Fires for both deep-link and external-URL CTAs; ctaClicked.getDeepLinkUrl()
858-
// carries whichever URL the CTA navigates to.
859+
// Fires for both deep-link and external-URL CTAs (SDK 4.5.0+);
860+
// ctaClicked.getDeepLinkUrl() carries whichever URL the CTA navigates to.
859861
// e.g. myAnalytics.track("Form CTA Clicked", ...)
860862
}
861863
});

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

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
package com.klaviyo.core.utils
2+
3+
import android.net.Uri
4+
import com.klaviyo.core.Constants.ALLOWED_OPEN_URL_SCHEMES
5+
6+
/**
7+
* True if this URI's scheme is in [ALLOWED_OPEN_URL_SCHEMES]. Shared by the `push-fcm` and
8+
* `forms` modules so the `web_url`/`open_url`/form-CTA allowlist gate can never diverge
9+
* between them.
10+
*/
11+
fun Uri.hasAllowedOpenUrlScheme(): Boolean = scheme?.lowercase() in ALLOWED_OPEN_URL_SCHEMES

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

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -47,14 +47,16 @@ sealed interface FormLifecycleEvent {
4747
/**
4848
* Triggered when a user taps a call-to-action (CTA) button in a form
4949
* that has a URL configured, whether it deep links within the host app
50-
* or opens an external URL in the default browser.
50+
* or opens externally (a browser, dialer, or messaging app, depending
51+
* on the URL's scheme).
5152
*
5253
* Fired after the SDK has initiated navigation. Not emitted if no URL is
5354
* configured for the CTA.
5455
*
5556
* @property buttonLabel The text label of the CTA button.
5657
* @property deepLinkUrl The URI the CTA navigates to. This is an in-app deep
57-
* link for deep-link CTAs, or the external URL for CTAs that open the browser.
58+
* link for deep-link CTAs, or the external URL (opened in a browser, dialer,
59+
* or messaging app depending on its scheme) for external CTAs.
5860
*/
5961
data class FormCtaClicked(
6062
override val formId: String,

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

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -10,8 +10,8 @@ import androidx.webkit.WebViewFeature.WEB_MESSAGE_LISTENER
1010
import com.klaviyo.analytics.Klaviyo
1111
import com.klaviyo.analytics.linking.DeepLinking
1212
import com.klaviyo.analytics.networking.ApiClient
13-
import com.klaviyo.core.Constants.ALLOWED_OPEN_URL_SCHEMES
1413
import com.klaviyo.core.Registry
14+
import com.klaviyo.core.utils.hasAllowedOpenUrlScheme
1515
import com.klaviyo.core.utils.startActivityIfResolved
1616
import com.klaviyo.forms.FormLifecycleEvent
1717
import com.klaviyo.forms.FormLifecycleHandler
@@ -133,14 +133,16 @@ internal class KlaviyoNativeBridge : NativeBridge {
133133
* next activity resuming; [DeepLinking.handleDeepLink] alleviates that race by postponing until
134134
* the next activity resumes if the current activity is null.
135135
*
136-
* When true, the URL is opened in the default browser via a non-package-scoped intent, bypassing
137-
* any registered deep link handler — mirroring
136+
* When true, the URL is routed to its external handler via a non-package-scoped intent —
137+
* a browser for `http`/`https`, or the mail, dialer, or SMS app for
138+
* `mailto:`/`tel:`/`sms:`/`smsto:` — bypassing any registered deep link handler and mirroring
138139
* [com.klaviyo.forms.webview.KlaviyoWebViewClient.shouldOverrideUrlLoading]. The `NEW_TASK` intent
139140
* launches independently of the overlay activity, so no grace period is needed. The scheme is
140-
* checked against [ALLOWED_OPEN_URL_SCHEMES] first — the same allowlist gate applied to push's
141-
* `open_url`/`web_url` fields (see [com.klaviyo.pushFcm.KlaviyoRemoteMessage], PUSH-834) — to
142-
* avoid routing dangerous or unintended URIs (e.g. `intent:`, `javascript:`, `file:`). The intent
143-
* is built by [DeepLinking.makeExternalIntent], shared with the push `open_url` path.
141+
* checked via [com.klaviyo.core.utils.hasAllowedOpenUrlScheme] first — the same allowlist gate
142+
* applied to push's `open_url`/`web_url` fields (see [com.klaviyo.pushFcm.KlaviyoRemoteMessage],
143+
* PUSH-834) — to avoid routing dangerous or unintended URIs (e.g. `intent:`, `javascript:`,
144+
* `file:`). The intent is built by [DeepLinking.makeExternalIntent], shared with the push
145+
* `open_url` path.
144146
*
145147
* Fires [FormLifecycleEvent.FormCtaClicked] after dispatch, with the URL carried in
146148
* [FormLifecycleEvent.FormCtaClicked.deepLinkUrl].
@@ -154,7 +156,7 @@ internal class KlaviyoNativeBridge : NativeBridge {
154156
}
155157

156158
if (message.openExternally) {
157-
if (uri.scheme?.lowercase() !in ALLOWED_OPEN_URL_SCHEMES) {
159+
if (!uri.hasAllowedOpenUrlScheme()) {
158160
Registry.log.warning(
159161
"Form CTA external url has a scheme not in the allowed list " +
160162
"('${uri.scheme}'); ignoring."

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

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -66,8 +66,10 @@ internal sealed class NativeBridgeMessage {
6666
* @param formId The form ID of the form that triggered the CTA
6767
* @param formName The name of the form that triggered the CTA
6868
* @param buttonLabel The text label of the CTA button that was clicked
69-
* @param openExternally When true, open the URL in the default browser (bypassing any registered
70-
* deep link handler), gated by the scheme allowlist. When false, route it as an in-app deep link.
69+
* @param openExternally When true, route the URL to its external handler (a browser for
70+
* `http`/`https`, or the mail, dialer, or SMS app for `mailto:`/`tel:`/`sms:`/`smsto:`),
71+
* bypassing any registered deep link handler, gated by the scheme allowlist. When false,
72+
* route it as an in-app deep link.
7173
*/
7274
data class OpenDeepLink(
7375
val route: String?,

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

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -15,12 +15,12 @@ import androidx.core.graphics.toColorInt
1515
import androidx.core.net.toUri
1616
import com.google.firebase.messaging.CommonNotificationBuilder
1717
import com.google.firebase.messaging.RemoteMessage
18-
import com.klaviyo.core.Constants.ALLOWED_OPEN_URL_SCHEMES
1918
import com.klaviyo.core.Constants.PACKAGE_PREFIX
2019
import com.klaviyo.core.Constants.TRACKING_PARAMETER
2120
import com.klaviyo.core.Registry
2221
import com.klaviyo.core.config.getApplicationInfoCompat
2322
import com.klaviyo.core.config.getManifestInt
23+
import com.klaviyo.core.utils.hasAllowedOpenUrlScheme
2424
import java.net.URL
2525
import org.json.JSONArray
2626
import org.json.JSONObject
@@ -133,19 +133,20 @@ object KlaviyoRemoteMessage {
133133
val RemoteMessage.body: String? get() = this.data[KlaviyoNotification.BODY_KEY]
134134

135135
/**
136-
* True if the string parses as a Uri whose scheme is in [ALLOWED_OPEN_URL_SCHEMES].
136+
* True if the string parses as a Uri whose scheme is allowed — see
137+
* [com.klaviyo.core.utils.hasAllowedOpenUrlScheme], shared with the forms module so the
138+
* two call sites can never diverge.
137139
*/
138-
internal fun String.hasAllowedOpenUrlScheme(): Boolean =
139-
this.toUri().scheme?.lowercase() in ALLOWED_OPEN_URL_SCHEMES
140+
internal fun String.hasAllowedOpenUrlScheme(): Boolean = this.toUri().hasAllowedOpenUrlScheme()
140141

141142
/**
142143
* Parse the external URL from the payload, if present.
143144
*
144145
* Reads the `web_url` field. The presence of this field indicates the tap should open
145146
* the URL externally rather than route through the app's deep link handling.
146-
* Returns null if the field is absent, blank, or the URL's scheme is not in
147-
* [ALLOWED_OPEN_URL_SCHEMES] — disallowed schemes are rejected to prevent routing
148-
* dangerous URIs (e.g. intent:, javascript:, file:) through the SDK.
147+
* Returns null if the field is absent, blank, or the URL's scheme is not allowed —
148+
* disallowed schemes are rejected to prevent routing dangerous URIs (e.g. intent:,
149+
* javascript:, file:) through the SDK.
149150
*/
150151
val RemoteMessage.webUrl: String?
151152
get() {

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

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,11 @@ internal class KlaviyoTrampolineActivity : Activity() {
3333
super.onCreate(savedInstanceState)
3434
try {
3535
handleTrampolineIntent(intent, this)
36+
} catch (e: Exception) {
37+
// Must not throw: this is an invisible entry point for notification taps —
38+
// an uncaught exception here would crash the host app, which is worse than
39+
// the stuck-screen risk the `finally` below already guards against.
40+
Registry.log.error("KlaviyoTrampolineActivity failed to dispatch", e)
3641
} finally {
3742
// Always finish — leaving a translucent activity onscreen after an exception
3843
// would look like a stuck blank screen to the user.

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

Lines changed: 32 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package com.klaviyo.pushFcm
22

33
import android.content.Intent
4+
import android.net.Uri
45
import com.google.firebase.messaging.RemoteMessage
56
import com.klaviyo.fixtures.BaseTest
67
import com.klaviyo.pushFcm.KlaviyoNotification.Companion.ACTION_BUTTONS_KEY
@@ -22,9 +23,23 @@ import io.mockk.unmockkStatic
2223
import io.mockk.verify
2324
import org.json.JSONArray
2425
import org.json.JSONObject
26+
import org.junit.After
27+
import org.junit.Before
2528
import org.junit.Test
2629

2730
class KlaviyoRemoteMessageTest : BaseTest() {
31+
@Before
32+
override fun setup() {
33+
super.setup()
34+
mockkStatic(Uri::class)
35+
}
36+
37+
@After
38+
override fun cleanup() {
39+
unmockkStatic(Uri::class)
40+
super.cleanup()
41+
}
42+
2843
private val stubKeyValuePairs = mapOf(
2944
"test_key_1" to "test_value_1",
3045
"test_key_2" to "test_value_2",
@@ -527,10 +542,9 @@ class KlaviyoRemoteMessageTest : BaseTest() {
527542

528543
@Test
529544
fun `webUrl returns Uri when web_url is an https URL`() {
530-
val mockUri = mockk<android.net.Uri>(relaxed = true)
545+
val mockUri = mockk<Uri>(relaxed = true)
531546
every { mockUri.scheme } returns "https"
532-
mockkStatic(android.net.Uri::class)
533-
every { android.net.Uri.parse("https://example.com") } returns mockUri
547+
every { Uri.parse("https://example.com") } returns mockUri
534548

535549
val msg = mockk<RemoteMessage>()
536550
every { msg.data } returns stubMessage.toMutableMap().apply {
@@ -539,26 +553,21 @@ class KlaviyoRemoteMessageTest : BaseTest() {
539553

540554
val webUrl = msg.webUrl
541555
assert(webUrl != null)
542-
verify { android.net.Uri.parse("https://example.com") }
543-
544-
unmockkStatic(android.net.Uri::class)
556+
verify { Uri.parse("https://example.com") }
545557
}
546558

547559
@Test
548560
fun `webUrl returns Uri when web_url is an http URL`() {
549-
val mockUri = mockk<android.net.Uri>(relaxed = true)
561+
val mockUri = mockk<Uri>(relaxed = true)
550562
every { mockUri.scheme } returns "http"
551-
mockkStatic(android.net.Uri::class)
552-
every { android.net.Uri.parse("http://example.com") } returns mockUri
563+
every { Uri.parse("http://example.com") } returns mockUri
553564

554565
val msg = mockk<RemoteMessage>()
555566
every { msg.data } returns stubMessage.toMutableMap().apply {
556567
put(KlaviyoNotification.WEB_URL_KEY, "http://example.com")
557568
}
558569

559570
assert(msg.webUrl != null)
560-
561-
unmockkStatic(android.net.Uri::class)
562571
}
563572

564573
@Test
@@ -581,25 +590,20 @@ class KlaviyoRemoteMessageTest : BaseTest() {
581590

582591
@Test
583592
fun `webUrl returns null when web_url has a blocked scheme`() {
584-
val mockUri = mockk<android.net.Uri>(relaxed = true)
593+
val mockUri = mockk<Uri>(relaxed = true)
585594
every { mockUri.scheme } returns "javascript"
586-
mockkStatic(android.net.Uri::class)
587-
every { android.net.Uri.parse("javascript:alert(1)") } returns mockUri
595+
every { Uri.parse("javascript:alert(1)") } returns mockUri
588596

589597
val msg = mockk<RemoteMessage>()
590598
every { msg.data } returns stubMessage.toMutableMap().apply {
591599
put(KlaviyoNotification.WEB_URL_KEY, "javascript:alert(1)")
592600
}
593601

594602
assert(msg.webUrl == null)
595-
596-
unmockkStatic(android.net.Uri::class)
597603
}
598604

599605
@Test
600606
fun `webUrl returns url when web_url has an allowlisted communication scheme`() {
601-
mockkStatic(android.net.Uri::class)
602-
603607
val schemes = listOf(
604608
"mailto" to "mailto:user@example.com",
605609
"tel" to "tel:+15555550100",
@@ -608,9 +612,9 @@ class KlaviyoRemoteMessageTest : BaseTest() {
608612
)
609613

610614
for ((scheme, url) in schemes) {
611-
val mockUri = mockk<android.net.Uri>(relaxed = true)
615+
val mockUri = mockk<Uri>(relaxed = true)
612616
every { mockUri.scheme } returns scheme
613-
every { android.net.Uri.parse(url) } returns mockUri
617+
every { Uri.parse(url) } returns mockUri
614618

615619
val msg = mockk<RemoteMessage>()
616620
every { msg.data } returns stubMessage.toMutableMap().apply {
@@ -621,16 +625,13 @@ class KlaviyoRemoteMessageTest : BaseTest() {
621625
"Expected webUrl to return '$url' for scheme $scheme"
622626
}
623627
}
624-
625-
unmockkStatic(android.net.Uri::class)
626628
}
627629

628630
@Test
629631
fun `webUrl returns parsed URL even when url field is also present`() {
630-
val mockUri = mockk<android.net.Uri>(relaxed = true)
632+
val mockUri = mockk<Uri>(relaxed = true)
631633
every { mockUri.scheme } returns "https"
632-
mockkStatic(android.net.Uri::class)
633-
every { android.net.Uri.parse("https://example.com") } returns mockUri
634+
every { Uri.parse("https://example.com") } returns mockUri
634635

635636
val msg = mockk<RemoteMessage>()
636637
every { msg.data } returns stubMessage.toMutableMap().apply {
@@ -639,16 +640,13 @@ class KlaviyoRemoteMessageTest : BaseTest() {
639640
}
640641

641642
assert(msg.webUrl != null)
642-
643-
unmockkStatic(android.net.Uri::class)
644643
}
645644

646645
@Test
647646
fun `actionButtons parses open_url variant with url`() {
648-
val mockUri = mockk<android.net.Uri>(relaxed = true)
647+
val mockUri = mockk<Uri>(relaxed = true)
649648
every { mockUri.scheme } returns "https"
650-
mockkStatic(android.net.Uri::class)
651-
every { android.net.Uri.parse("https://example.com") } returns mockUri
649+
every { Uri.parse("https://example.com") } returns mockUri
652650

653651
val actionButtonsData = listOf(
654652
mapOf(
@@ -674,16 +672,13 @@ class KlaviyoRemoteMessageTest : BaseTest() {
674672
assert(button?.id == "open.url")
675673
assert(button?.label == "Open Website")
676674
assert((button as? ActionButton.OpenUrl)?.url == "https://example.com")
677-
678-
unmockkStatic(android.net.Uri::class)
679675
}
680676

681677
@Test
682678
fun `actionButtons skips open_url with blocked scheme`() {
683-
val mockUri = mockk<android.net.Uri>(relaxed = true)
679+
val mockUri = mockk<Uri>(relaxed = true)
684680
every { mockUri.scheme } returns "intent"
685-
mockkStatic(android.net.Uri::class)
686-
every { android.net.Uri.parse("intent://evil") } returns mockUri
681+
every { Uri.parse("intent://evil") } returns mockUri
687682

688683
val actionButtonsData = listOf(
689684
mapOf(
@@ -701,14 +696,10 @@ class KlaviyoRemoteMessageTest : BaseTest() {
701696
every { msg.data } returns messageWithActions
702697

703698
assert(msg.actionButtons == null)
704-
705-
unmockkStatic(android.net.Uri::class)
706699
}
707700

708701
@Test
709702
fun `actionButtons accepts open_url with allowlisted communication schemes`() {
710-
mockkStatic(android.net.Uri::class)
711-
712703
val schemes = listOf(
713704
"mailto" to "mailto:user@example.com",
714705
"tel" to "tel:+15555550100",
@@ -717,9 +708,9 @@ class KlaviyoRemoteMessageTest : BaseTest() {
717708
)
718709

719710
for ((scheme, url) in schemes) {
720-
val mockUri = mockk<android.net.Uri>(relaxed = true)
711+
val mockUri = mockk<Uri>(relaxed = true)
721712
every { mockUri.scheme } returns scheme
722-
every { android.net.Uri.parse(url) } returns mockUri
713+
every { Uri.parse(url) } returns mockUri
723714

724715
val actionButtonsData = listOf(
725716
mapOf(
@@ -746,8 +737,6 @@ class KlaviyoRemoteMessageTest : BaseTest() {
746737
"Expected url $url for scheme $scheme"
747738
}
748739
}
749-
750-
unmockkStatic(android.net.Uri::class)
751740
}
752741

753742
@Test

0 commit comments

Comments
 (0)