From b8e5d14426b45123c0a1f0ab14cbaa7de14b675c Mon Sep 17 00:00:00 2001 From: xhon-pelushi Date: Thu, 13 Aug 2026 00:15:02 -0400 Subject: [PATCH 1/3] Read Intent.EXTRA_EMAIL as a String[] instead of a String Intent.EXTRA_EMAIL is documented as a String[] of e-mail addresses, but LaunchConversationActivity and ShareIntentActivity read it with getStringExtra(), which silently returns null for the array extras put by well-behaved senders (including this app's own VCardDetailEffectHandler, which already puts EXTRA_EMAIL as a String[]). Use getStringArrayExtra() so intents carrying the extra in its documented type are handled, and forward every address rather than only the first one. Fixes #164 --- .../ui/conversation/LaunchConversationActivity.java | 5 +++-- .../ui/conversationpicker/host/share/ShareIntentActivity.kt | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/com/android/messaging/ui/conversation/LaunchConversationActivity.java b/src/com/android/messaging/ui/conversation/LaunchConversationActivity.java index 5c9e10357..e05a01bb9 100644 --- a/src/com/android/messaging/ui/conversation/LaunchConversationActivity.java +++ b/src/com/android/messaging/ui/conversation/LaunchConversationActivity.java @@ -71,12 +71,13 @@ protected void onCreate(final Bundle savedInstanceState) { recipients = commaSeparatedRecipients.split(","); } final boolean haveAddress = !TextUtils.isEmpty(intent.getStringExtra(ADDRESS)); - final boolean haveEmail = !TextUtils.isEmpty(intent.getStringExtra(Intent.EXTRA_EMAIL)); + final String[] emails = intent.getStringArrayExtra(Intent.EXTRA_EMAIL); + final boolean haveEmail = emails != null && emails.length > 0; if (recipients == null && (haveAddress || haveEmail)) { if (haveAddress) { recipients = new String[] { intent.getStringExtra(ADDRESS) }; } else { - recipients = new String[] { intent.getStringExtra(Intent.EXTRA_EMAIL) }; + recipients = emails; } } if (recipients != null) { diff --git a/src/com/android/messaging/ui/conversationpicker/host/share/ShareIntentActivity.kt b/src/com/android/messaging/ui/conversationpicker/host/share/ShareIntentActivity.kt index bba460809..1112dbbd8 100644 --- a/src/com/android/messaging/ui/conversationpicker/host/share/ShareIntentActivity.kt +++ b/src/com/android/messaging/ui/conversationpicker/host/share/ShareIntentActivity.kt @@ -128,7 +128,7 @@ class ShareIntentActivity : BugleComponentActivity() { private fun redirectToSendToIfNeeded(): Boolean { val hasNoDestination = intent.getStringExtra(EXTRA_ADDRESS).isNullOrEmpty() && - intent.getStringExtra(Intent.EXTRA_EMAIL).isNullOrEmpty() + intent.getStringArrayExtra(Intent.EXTRA_EMAIL).isNullOrEmpty() if (Intent.ACTION_SEND != intent.action || hasNoDestination) { return false From b4b4b144914b6beae876f9d714ce2b94bc096e09 Mon Sep 17 00:00:00 2001 From: xhon-pelushi Date: Wed, 26 Aug 2026 14:26:24 -0400 Subject: [PATCH 2/3] Address review: accept both EXTRA_EMAIL forms, tolerate null entries - Read Intent.EXTRA_EMAIL as either String[] or String again. The previous commit replaced the String read with a String[] read, so senders that put a single String regressed even though the PR title claimed both. - trimInvalidRecipients() now uses TextUtils.isEmpty() instead of recipient.length(). LaunchConversationActivity is exported, so the array contents come from another app and may contain nulls; a String[]{null} extra crashed with an NPE. - ShareIntentActivity no longer redirects an ACTION_SEND intent that carries EXTRA_STREAM. LaunchConversationActivity only understands a destination plus a text body, so redirecting dropped the attachment. - Add unit tests for both, covering the String/String[] duality, null and empty entries, over-long recipients, and the EXTRA_STREAM case. --- ...aunchConversationActivityRecipientsTest.kt | 114 ++++++++++++++++++ .../share/ShareIntentActivityRedirectTest.kt | 78 ++++++++++++ .../LaunchConversationActivity.java | 30 ++++- .../host/share/ShareIntentActivity.kt | 34 +++++- 4 files changed, 248 insertions(+), 8 deletions(-) create mode 100644 app/src/test/kotlin/com/android/messaging/ui/conversation/LaunchConversationActivityRecipientsTest.kt create mode 100644 app/src/test/kotlin/com/android/messaging/ui/conversationpicker/host/share/ShareIntentActivityRedirectTest.kt diff --git a/app/src/test/kotlin/com/android/messaging/ui/conversation/LaunchConversationActivityRecipientsTest.kt b/app/src/test/kotlin/com/android/messaging/ui/conversation/LaunchConversationActivityRecipientsTest.kt new file mode 100644 index 000000000..35d8f3c17 --- /dev/null +++ b/app/src/test/kotlin/com/android/messaging/ui/conversation/LaunchConversationActivityRecipientsTest.kt @@ -0,0 +1,114 @@ +package com.android.messaging.ui.conversation + +import android.content.Intent +import org.junit.Assert.assertArrayEquals +import org.junit.Assert.assertNull +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner +import org.robolectric.annotation.Config + +/** + * Covers the EXTRA_EMAIL parsing and recipient filtering in LaunchConversationActivity. The + * extras arrive from other apps through an exported activity, so null and empty entries have to + * be tolerated rather than assumed away. + */ +@RunWith(RobolectricTestRunner::class) +@Config(sdk = [36]) +class LaunchConversationActivityRecipientsTest { + + private fun intentWithEmailArray(vararg emails: String?): Intent = + Intent(Intent.ACTION_SENDTO).putExtra(Intent.EXTRA_EMAIL, arrayOf(*emails)) + + private fun intentWithEmailString(email: String?): Intent = + Intent(Intent.ACTION_SENDTO).putExtra(Intent.EXTRA_EMAIL, email) + + @Test + fun emailArrayIsRead() { + assertArrayEquals( + arrayOf("a@example.com", "b@example.com"), + LaunchConversationActivity.getEmailRecipients( + intentWithEmailArray("a@example.com", "b@example.com"), + ), + ) + } + + @Test + fun singleEmailStringIsStillRead() { + assertArrayEquals( + arrayOf("a@example.com"), + LaunchConversationActivity.getEmailRecipients( + intentWithEmailString("a@example.com"), + ), + ) + } + + @Test + fun nullAndEmptyArrayEntriesAreDropped() { + assertArrayEquals( + arrayOf("a@example.com"), + LaunchConversationActivity.getEmailRecipients( + intentWithEmailArray(null, "", "a@example.com"), + ), + ) + } + + @Test + fun arrayOfOnlyNullsCountsAsNoRecipient() { + assertNull( + LaunchConversationActivity.getEmailRecipients(intentWithEmailArray(null, null)), + ) + } + + @Test + fun emptyArrayCountsAsNoRecipient() { + assertNull(LaunchConversationActivity.getEmailRecipients(intentWithEmailArray())) + } + + @Test + fun missingExtraCountsAsNoRecipient() { + assertNull(LaunchConversationActivity.getEmailRecipients(Intent(Intent.ACTION_SENDTO))) + } + + @Test + fun emptyEmailStringCountsAsNoRecipient() { + assertNull(LaunchConversationActivity.getEmailRecipients(intentWithEmailString(""))) + } + + @Test + fun trimInvalidRecipientsToleratesNullEntries() { + // Regression: a null entry used to NPE on recipient.length(). + assertArrayEquals( + arrayOf("a@example.com"), + LaunchConversationActivity.trimInvalidRecipients( + arrayOf(null, "a@example.com"), + ), + ) + } + + @Test + fun trimInvalidRecipientsDropsEmptyEntries() { + assertArrayEquals( + arrayOf("a@example.com"), + LaunchConversationActivity.trimInvalidRecipients( + arrayOf("", "a@example.com"), + ), + ) + } + + @Test + fun trimInvalidRecipientsDropsOverlongEntries() { + val tooLong = "x".repeat(1000) + assertArrayEquals( + arrayOf("a@example.com"), + LaunchConversationActivity.trimInvalidRecipients( + arrayOf(tooLong, "a@example.com"), + ), + ) + } + + @Test + fun trimInvalidRecipientsReturnsNullWhenNothingValid() { + assertNull(LaunchConversationActivity.trimInvalidRecipients(arrayOf(null, ""))) + } +} diff --git a/app/src/test/kotlin/com/android/messaging/ui/conversationpicker/host/share/ShareIntentActivityRedirectTest.kt b/app/src/test/kotlin/com/android/messaging/ui/conversationpicker/host/share/ShareIntentActivityRedirectTest.kt new file mode 100644 index 000000000..e215c7372 --- /dev/null +++ b/app/src/test/kotlin/com/android/messaging/ui/conversationpicker/host/share/ShareIntentActivityRedirectTest.kt @@ -0,0 +1,78 @@ +package com.android.messaging.ui.conversationpicker.host.share + +import android.content.Intent +import android.net.Uri +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner +import org.robolectric.annotation.Config + +/** + * Covers when a share intent may be redirected to LaunchConversationActivity. That activity only + * understands a destination plus a text body, so an intent carrying an EXTRA_STREAM attachment + * must stay in the conversation picker or the attachment is silently dropped. + */ +@RunWith(RobolectricTestRunner::class) +@Config(sdk = [36]) +class ShareIntentActivityRedirectTest { + + private val address = "address" + private val stream: Uri = Uri.parse("content://media/external/images/media/1") + + private fun sendIntent(): Intent = Intent(Intent.ACTION_SEND).setType("text/plain") + + @Test + fun redirectsWhenEmailArrayIsTheDestination() { + val intent = sendIntent().putExtra(Intent.EXTRA_EMAIL, arrayOf("a@example.com")) + assertTrue(ShareIntentActivity.shouldRedirectToSendTo(intent)) + } + + @Test + fun redirectsWhenEmailStringIsTheDestination() { + val intent = sendIntent().putExtra(Intent.EXTRA_EMAIL, "a@example.com") + assertTrue(ShareIntentActivity.shouldRedirectToSendTo(intent)) + } + + @Test + fun redirectsWhenAddressIsTheDestination() { + val intent = sendIntent().putExtra(address, "+15555550100") + assertTrue(ShareIntentActivity.shouldRedirectToSendTo(intent)) + } + + @Test + fun doesNotRedirectWithoutADestination() { + assertFalse(ShareIntentActivity.shouldRedirectToSendTo(sendIntent())) + } + + @Test + fun doesNotRedirectForOtherActions() { + val intent = Intent(Intent.ACTION_SEND_MULTIPLE) + .putExtra(Intent.EXTRA_EMAIL, arrayOf("a@example.com")) + assertFalse(ShareIntentActivity.shouldRedirectToSendTo(intent)) + } + + @Test + fun doesNotRedirectWhenAnAttachmentWouldBeLost() { + // Regression: EXTRA_STREAM + EXTRA_EMAIL used to redirect and drop the attachment. + val intent = sendIntent() + .putExtra(Intent.EXTRA_EMAIL, arrayOf("a@example.com")) + .putExtra(Intent.EXTRA_STREAM, stream) + assertFalse(ShareIntentActivity.shouldRedirectToSendTo(intent)) + } + + @Test + fun doesNotRedirectWhenAttachmentAccompaniesAnAddress() { + val intent = sendIntent() + .putExtra(address, "+15555550100") + .putExtra(Intent.EXTRA_STREAM, stream) + assertFalse(ShareIntentActivity.shouldRedirectToSendTo(intent)) + } + + @Test + fun emailArrayOfOnlyNullsIsNotADestination() { + val intent = sendIntent().putExtra(Intent.EXTRA_EMAIL, arrayOf(null, "")) + assertFalse(ShareIntentActivity.shouldRedirectToSendTo(intent)) + } +} diff --git a/src/com/android/messaging/ui/conversation/LaunchConversationActivity.java b/src/com/android/messaging/ui/conversation/LaunchConversationActivity.java index e05a01bb9..88651cea7 100644 --- a/src/com/android/messaging/ui/conversation/LaunchConversationActivity.java +++ b/src/com/android/messaging/ui/conversation/LaunchConversationActivity.java @@ -71,8 +71,8 @@ protected void onCreate(final Bundle savedInstanceState) { recipients = commaSeparatedRecipients.split(","); } final boolean haveAddress = !TextUtils.isEmpty(intent.getStringExtra(ADDRESS)); - final String[] emails = intent.getStringArrayExtra(Intent.EXTRA_EMAIL); - final boolean haveEmail = emails != null && emails.length > 0; + final String[] emails = getEmailRecipients(intent); + final boolean haveEmail = emails != null; if (recipients == null && (haveAddress || haveEmail)) { if (haveAddress) { recipients = new String[] { intent.getStringExtra(ADDRESS) }; @@ -110,10 +110,32 @@ protected void onCreate(final Bundle savedInstanceState) { finish(); } - private String[] trimInvalidRecipients(String[] recipients) { + /** + * {@link Intent#EXTRA_EMAIL} is documented as a String[], but senders also put a single + * String there, which is what this activity used to read. Accept both so neither kind of + * sender regresses. Returns null when no usable address is present. + */ + static String[] getEmailRecipients(final Intent intent) { + final String[] emails = intent.getStringArrayExtra(Intent.EXTRA_EMAIL); + if (emails != null) { + final List nonEmpty = new ArrayList<>(emails.length); + for (final String email : emails) { + if (!TextUtils.isEmpty(email)) { + nonEmpty.add(email); + } + } + return nonEmpty.isEmpty() ? null : nonEmpty.toArray(new String[0]); + } + final String email = intent.getStringExtra(Intent.EXTRA_EMAIL); + return TextUtils.isEmpty(email) ? null : new String[] { email }; + } + + static String[] trimInvalidRecipients(String[] recipients) { List trimmedRecipients = new ArrayList<>(); for (String recipient : recipients) { - if (recipient.length() < MAX_RECIPIENT_LENGTH) { + // The recipients come from another app's intent extras, so entries may be null or + // empty; TextUtils.isEmpty() is null-safe where recipient.length() is not. + if (!TextUtils.isEmpty(recipient) && recipient.length() < MAX_RECIPIENT_LENGTH) { trimmedRecipients.add(recipient); } } diff --git a/src/com/android/messaging/ui/conversationpicker/host/share/ShareIntentActivity.kt b/src/com/android/messaging/ui/conversationpicker/host/share/ShareIntentActivity.kt index 1112dbbd8..b6f07285f 100644 --- a/src/com/android/messaging/ui/conversationpicker/host/share/ShareIntentActivity.kt +++ b/src/com/android/messaging/ui/conversationpicker/host/share/ShareIntentActivity.kt @@ -127,10 +127,7 @@ class ShareIntentActivity : BugleComponentActivity() { } private fun redirectToSendToIfNeeded(): Boolean { - val hasNoDestination = intent.getStringExtra(EXTRA_ADDRESS).isNullOrEmpty() && - intent.getStringArrayExtra(Intent.EXTRA_EMAIL).isNullOrEmpty() - - if (Intent.ACTION_SEND != intent.action || hasNoDestination) { + if (!shouldRedirectToSendTo(intent)) { return false } @@ -158,6 +155,35 @@ class ShareIntentActivity : BugleComponentActivity() { ) companion object { + /** + * LaunchConversationActivity only understands a destination plus a text body, so + * redirecting an intent that carries an EXTRA_STREAM attachment would silently drop the + * attachment. Keep such intents in the conversation picker instead. + */ + internal fun shouldRedirectToSendTo(intent: Intent): Boolean { + if (Intent.ACTION_SEND != intent.action) { + return false + } + if (intent.hasExtra(Intent.EXTRA_STREAM)) { + return false + } + val hasDestination = !intent.getStringExtra(EXTRA_ADDRESS).isNullOrEmpty() || + hasEmailDestination(intent) + return hasDestination + } + + /** + * EXTRA_EMAIL is documented as a String[] but senders also put a single String there. + * Accept both, and ignore null or empty entries since they come from another app. + */ + internal fun hasEmailDestination(intent: Intent): Boolean { + val emails = intent.getStringArrayExtra(Intent.EXTRA_EMAIL) + if (emails != null) { + return emails.any { !it.isNullOrEmpty() } + } + return !intent.getStringExtra(Intent.EXTRA_EMAIL).isNullOrEmpty() + } + internal fun createForwardIntent( context: Context, uri: Uri, From 5b24d4af07e8e15a085a2462f7d5b47e6778919f Mon Sep 17 00:00:00 2001 From: xhon-pelushi Date: Wed, 26 Aug 2026 18:01:00 -0400 Subject: [PATCH 3/3] Satisfy detekt ReturnCount in shouldRedirectToSendTo detekt's ReturnCount rule caps a function at 2 returns; the new shouldRedirectToSendTo had 3. Express it as a single boolean instead, which also reads closer to the condition suggested in review, and collapse hasEmailDestination onto one return while there. --- .../host/share/ShareIntentActivity.kt | 24 +++++++------------ 1 file changed, 9 insertions(+), 15 deletions(-) diff --git a/src/com/android/messaging/ui/conversationpicker/host/share/ShareIntentActivity.kt b/src/com/android/messaging/ui/conversationpicker/host/share/ShareIntentActivity.kt index b6f07285f..b21861986 100644 --- a/src/com/android/messaging/ui/conversationpicker/host/share/ShareIntentActivity.kt +++ b/src/com/android/messaging/ui/conversationpicker/host/share/ShareIntentActivity.kt @@ -160,17 +160,13 @@ class ShareIntentActivity : BugleComponentActivity() { * redirecting an intent that carries an EXTRA_STREAM attachment would silently drop the * attachment. Keep such intents in the conversation picker instead. */ - internal fun shouldRedirectToSendTo(intent: Intent): Boolean { - if (Intent.ACTION_SEND != intent.action) { - return false - } - if (intent.hasExtra(Intent.EXTRA_STREAM)) { - return false - } - val hasDestination = !intent.getStringExtra(EXTRA_ADDRESS).isNullOrEmpty() || - hasEmailDestination(intent) - return hasDestination - } + internal fun shouldRedirectToSendTo(intent: Intent): Boolean = + Intent.ACTION_SEND == intent.action && + !intent.hasExtra(Intent.EXTRA_STREAM) && + ( + !intent.getStringExtra(EXTRA_ADDRESS).isNullOrEmpty() || + hasEmailDestination(intent) + ) /** * EXTRA_EMAIL is documented as a String[] but senders also put a single String there. @@ -178,10 +174,8 @@ class ShareIntentActivity : BugleComponentActivity() { */ internal fun hasEmailDestination(intent: Intent): Boolean { val emails = intent.getStringArrayExtra(Intent.EXTRA_EMAIL) - if (emails != null) { - return emails.any { !it.isNullOrEmpty() } - } - return !intent.getStringExtra(Intent.EXTRA_EMAIL).isNullOrEmpty() + return emails?.any { !it.isNullOrEmpty() } + ?: !intent.getStringExtra(Intent.EXTRA_EMAIL).isNullOrEmpty() } internal fun createForwardIntent(