From 8ed7fc894e03d1618f3465a9cd7d60482c5ed32f Mon Sep 17 00:00:00 2001 From: Greyson Parrelli Date: Wed, 29 Jul 2020 00:55:20 -0400 Subject: [PATCH] Improve handling of partially bi-directional text. --- .../model/GroupsV2UpdateMessageProducer.java | 5 +- .../database/model/MessageRecord.java | 5 +- .../MessageRequestsBottomView.java | 4 +- .../securesms/recipients/Recipient.java | 25 +++--- .../securesms/util/GroupUtil.java | 2 +- .../securesms/util/StringUtil.java | 77 +++++++++++++++++++ .../GroupsV2UpdateMessageProducerTest.java | 14 ++-- 7 files changed, 108 insertions(+), 24 deletions(-) diff --git a/app/src/main/java/org/thoughtcrime/securesms/database/model/GroupsV2UpdateMessageProducer.java b/app/src/main/java/org/thoughtcrime/securesms/database/model/GroupsV2UpdateMessageProducer.java index 589f68efd4..7b4668a57f 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/database/model/GroupsV2UpdateMessageProducer.java +++ b/app/src/main/java/org/thoughtcrime/securesms/database/model/GroupsV2UpdateMessageProducer.java @@ -18,6 +18,7 @@ import org.signal.storageservice.protos.groups.local.DecryptedPendingMemberRemov import org.thoughtcrime.securesms.R; import org.thoughtcrime.securesms.groups.GV2AccessLevelUtil; import org.thoughtcrime.securesms.util.ExpirationUtil; +import org.thoughtcrime.securesms.util.StringUtil; import org.whispersystems.libsignal.util.guava.Optional; import org.whispersystems.signalservice.api.groupsv2.DecryptedGroupUtil; import org.whispersystems.signalservice.api.util.UuidUtil; @@ -396,7 +397,7 @@ final class GroupsV2UpdateMessageProducer { boolean editorIsYou = change.getEditor().equals(selfUuidBytes); if (change.hasNewTitle()) { - String newTitle = change.getNewTitle().getValue(); + String newTitle = StringUtil.isolateBidi(change.getNewTitle().getValue()); if (editorIsYou) { updates.add(updateDescription(context.getString(R.string.MessageRecord_you_changed_the_group_name_to_s, newTitle))); } else { @@ -407,7 +408,7 @@ final class GroupsV2UpdateMessageProducer { private void describeUnknownEditorNewTitle(@NonNull DecryptedGroupChange change, @NonNull List updates) { if (change.hasNewTitle()) { - updates.add(updateDescription(context.getString(R.string.MessageRecord_the_group_name_has_changed_to_s, change.getNewTitle().getValue()))); + updates.add(updateDescription(context.getString(R.string.MessageRecord_the_group_name_has_changed_to_s, StringUtil.isolateBidi(change.getNewTitle().getValue())))); } } diff --git a/app/src/main/java/org/thoughtcrime/securesms/database/model/MessageRecord.java b/app/src/main/java/org/thoughtcrime/securesms/database/model/MessageRecord.java index cb484abc29..6b2fbd807f 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/database/model/MessageRecord.java +++ b/app/src/main/java/org/thoughtcrime/securesms/database/model/MessageRecord.java @@ -39,6 +39,7 @@ import org.thoughtcrime.securesms.recipients.RecipientId; import org.thoughtcrime.securesms.util.Base64; import org.thoughtcrime.securesms.util.ExpirationUtil; import org.thoughtcrime.securesms.util.GroupUtil; +import org.thoughtcrime.securesms.util.StringUtil; import org.whispersystems.libsignal.util.guava.Function; import org.whispersystems.signalservice.api.util.UuidUtil; @@ -196,8 +197,8 @@ public abstract class MessageRecord extends DisplayRecord { if (profileChangeDetails.hasProfileNameChange()) { String displayName = getIndividualRecipient().getDisplayName(context); - String newName = ProfileName.fromSerialized(profileChangeDetails.getProfileNameChange().getNew()).toString(); - String previousName = ProfileName.fromSerialized(profileChangeDetails.getProfileNameChange().getPrevious()).toString(); + String newName = StringUtil.isolateBidi(ProfileName.fromSerialized(profileChangeDetails.getProfileNameChange().getNew()).toString()); + String previousName = StringUtil.isolateBidi(ProfileName.fromSerialized(profileChangeDetails.getProfileNameChange().getPrevious()).toString()); if (getIndividualRecipient().isSystemContact()) { return context.getString(R.string.MessageRecord_changed_their_profile_name_from_to, displayName, previousName, newName); diff --git a/app/src/main/java/org/thoughtcrime/securesms/messagerequests/MessageRequestsBottomView.java b/app/src/main/java/org/thoughtcrime/securesms/messagerequests/MessageRequestsBottomView.java index ebe6516206..b183cde972 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/messagerequests/MessageRequestsBottomView.java +++ b/app/src/main/java/org/thoughtcrime/securesms/messagerequests/MessageRequestsBottomView.java @@ -65,7 +65,7 @@ public class MessageRequestsBottomView extends ConstraintLayout { if (recipient.isGroup()) { question.setText(R.string.MessageRequestBottomView_unblock_this_group_and_share_your_name_and_photo_with_its_members); } else { - String name = recipient.getProfileName().isEmpty() ? recipient.getDisplayName(getContext()) : recipient.getProfileName().getGivenName(); + String name = recipient.getShortDisplayName(getContext()); question.setText(HtmlCompat.fromHtml(getContext().getString(R.string.MessageRequestBottomView_do_you_want_to_let_s_message_you_wont_receive_any_messages_until_you_unblock_them, HtmlUtil.bold(name)), 0)); } setActiveInactiveGroups(blockedButtons, normalButtons); @@ -77,7 +77,7 @@ public class MessageRequestsBottomView extends ConstraintLayout { question.setText(R.string.MessageRequestBottomView_do_you_want_to_join_this_group_they_wont_know_youve_seen_their_messages_until_you_accept); } } else { - String name = recipient.getProfileName().isEmpty() ? recipient.getDisplayName(getContext()) : recipient.getProfileName().getGivenName(); + String name = recipient.getShortDisplayName(getContext()); question.setText(HtmlCompat.fromHtml(getContext().getString(R.string.MessageRequestBottomView_do_you_want_to_let_s_message_you_they_wont_know_youve_seen_their_messages_until_you_accept, HtmlUtil.bold(name)), 0)); } setActiveInactiveGroups(normalButtons, blockedButtons); diff --git a/app/src/main/java/org/thoughtcrime/securesms/recipients/Recipient.java b/app/src/main/java/org/thoughtcrime/securesms/recipients/Recipient.java index fa894e2bab..6c0c6c5530 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/recipients/Recipient.java +++ b/app/src/main/java/org/thoughtcrime/securesms/recipients/Recipient.java @@ -26,19 +26,18 @@ import org.thoughtcrime.securesms.contacts.avatars.TransparentContactPhoto; import org.thoughtcrime.securesms.database.DatabaseFactory; import org.thoughtcrime.securesms.database.IdentityDatabase.VerifiedStatus; import org.thoughtcrime.securesms.database.RecipientDatabase; -import org.thoughtcrime.securesms.database.RecipientDatabase.RecipientIdResult; import org.thoughtcrime.securesms.database.RecipientDatabase.RegisteredState; import org.thoughtcrime.securesms.database.RecipientDatabase.UnidentifiedAccessMode; import org.thoughtcrime.securesms.database.RecipientDatabase.VibrateState; import org.thoughtcrime.securesms.dependencies.ApplicationDependencies; import org.thoughtcrime.securesms.groups.GroupId; -import org.thoughtcrime.securesms.jobs.DirectoryRefreshJob; import org.thoughtcrime.securesms.logging.Log; import org.thoughtcrime.securesms.notifications.NotificationChannels; import org.thoughtcrime.securesms.phonenumbers.NumberUtil; import org.thoughtcrime.securesms.phonenumbers.PhoneNumberFormatter; import org.thoughtcrime.securesms.profiles.ProfileName; import org.thoughtcrime.securesms.util.FeatureFlags; +import org.thoughtcrime.securesms.util.StringUtil; import org.thoughtcrime.securesms.util.Util; import org.thoughtcrime.securesms.util.concurrent.SignalExecutors; import org.whispersystems.libsignal.util.guava.Optional; @@ -400,18 +399,22 @@ public class Recipient { } public @NonNull String getDisplayName(@NonNull Context context) { - return Util.getFirstNonEmpty(getName(context), - getProfileName().toString(), - getDisplayUsername(), - e164, - email, - context.getString(R.string.Recipient_unknown)); + String name = Util.getFirstNonEmpty(getName(context), + getProfileName().toString(), + getDisplayUsername(), + e164, + email, + context.getString(R.string.Recipient_unknown)); + + return StringUtil.isolateBidi(name); } public @NonNull String getShortDisplayName(@NonNull Context context) { - return Util.getFirstNonEmpty(getName(context), - getProfileName().getGivenName(), - getDisplayName(context)); + String name = Util.getFirstNonEmpty(getName(context), + getProfileName().getGivenName(), + getDisplayName(context)); + + return StringUtil.isolateBidi(name); } public @NonNull MaterialColor getColor() { diff --git a/app/src/main/java/org/thoughtcrime/securesms/util/GroupUtil.java b/app/src/main/java/org/thoughtcrime/securesms/util/GroupUtil.java index eb90f45ce9..8cece8d996 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/util/GroupUtil.java +++ b/app/src/main/java/org/thoughtcrime/securesms/util/GroupUtil.java @@ -125,7 +125,7 @@ public final class GroupUtil { return description.toString(); } - String title = groupContext.getName(); + String title = StringUtil.isolateBidi(groupContext.getName()); if (members != null && members.size() > 0) { description.append("\n"); diff --git a/app/src/main/java/org/thoughtcrime/securesms/util/StringUtil.java b/app/src/main/java/org/thoughtcrime/securesms/util/StringUtil.java index a3ae093ce9..986b8da9e8 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/util/StringUtil.java +++ b/app/src/main/java/org/thoughtcrime/securesms/util/StringUtil.java @@ -2,10 +2,12 @@ package org.thoughtcrime.securesms.util; import androidx.annotation.NonNull; import androidx.annotation.Nullable; +import androidx.core.text.BidiFormatter; import com.google.android.collect.Sets; import java.nio.charset.StandardCharsets; +import java.util.Collections; import java.util.Set; public final class StringUtil { @@ -14,6 +16,27 @@ public final class StringUtil { '\u200F', // right-to-left mark '\u2007'); // figure space + private static final class Bidi { + /** Override text direction */ + private static final Set OVERRIDES = Sets.newHashSet("\u202a".codePointAt(0), /* LRE */ + "\u202b".codePointAt(0), /* RLE */ + "\u202d".codePointAt(0), /* LRO */ + "\u202e".codePointAt(0) /* RLO */); + + /** Set direction and isolate surrounding text */ + private static final Set ISOLATES = Sets.newHashSet("\u2066".codePointAt(0), /* LRI */ + "\u2067".codePointAt(0), /* RLI */ + "\u2068".codePointAt(0) /* FSI */); + /** Closes things in {@link #OVERRIDES} */ + private static final int PDF = "\u202c".codePointAt(0); + + /** Closes things in {@link #ISOLATES} */ + private static final int PDI = "\u2069".codePointAt(0); + + /** Auto-detecting isolate */ + private static final int FSI = "\u2068".codePointAt(0); + } + private StringUtil() { } @@ -99,4 +122,58 @@ public final class StringUtil { public static @NonNull String codePointToString(int codePoint) { return new String(Character.toChars(codePoint)); } + + /** + * Isolates bi-directional text from influencing surrounding text. You should use this whenever + * you're injecting user-generated text into a larger string. + * + * You'd think we'd be able to trust {@link BidiFormatter}, but unfortunately it just misses some + * corner cases, so here we are. + * + * The general idea is just to balance out the opening and closing codepoints, and then wrap the + * whole thing in FSI/PDI to isolate it. + * + * For more details, see: + * https://www.w3.org/International/questions/qa-bidi-unicode-controls + */ + public static @NonNull String isolateBidi(@NonNull String text) { + int overrideCount = 0; + int overrideCloseCount = 0; + int isolateCount = 0; + int isolateCloseCount = 0; + + for (int i = 0, len = text.codePointCount(0, text.length()); i < len; i++) { + int codePoint = text.codePointAt(i); + + if (Bidi.OVERRIDES.contains(codePoint)) { + overrideCount++; + } else if (codePoint == Bidi.PDF) { + overrideCloseCount++; + } else if (Bidi.ISOLATES.contains(codePoint)) { + isolateCount++; + } else if (codePoint == Bidi.PDI) { + isolateCloseCount++; + } + } + + StringBuilder suffix = new StringBuilder(); + + while (overrideCount > overrideCloseCount) { + suffix.appendCodePoint(Bidi.PDF); + overrideCloseCount++; + } + + while (isolateCount > isolateCloseCount) { + suffix.appendCodePoint(Bidi.FSI); + isolateCloseCount++; + } + + StringBuilder out = new StringBuilder(); + + return out.appendCodePoint(Bidi.FSI) + .append(text) + .append(suffix) + .appendCodePoint(Bidi.PDI) + .toString(); + } } diff --git a/app/src/test/java/org/thoughtcrime/securesms/database/model/GroupsV2UpdateMessageProducerTest.java b/app/src/test/java/org/thoughtcrime/securesms/database/model/GroupsV2UpdateMessageProducerTest.java index 70289a5aaf..4136951624 100644 --- a/app/src/test/java/org/thoughtcrime/securesms/database/model/GroupsV2UpdateMessageProducerTest.java +++ b/app/src/test/java/org/thoughtcrime/securesms/database/model/GroupsV2UpdateMessageProducerTest.java @@ -28,6 +28,7 @@ import org.signal.storageservice.protos.groups.local.DecryptedPendingMemberRemov import org.signal.storageservice.protos.groups.local.DecryptedString; import org.signal.storageservice.protos.groups.local.DecryptedTimer; import org.thoughtcrime.securesms.testutil.MainThreadUtil; +import org.thoughtcrime.securesms.util.StringUtil; import org.thoughtcrime.securesms.util.Util; import org.whispersystems.signalservice.api.util.UuidUtil; @@ -43,6 +44,7 @@ import static org.hamcrest.MatcherAssert.assertThat; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertTrue; +import static org.thoughtcrime.securesms.util.StringUtil.isolateBidi; @RunWith(RobolectricTestRunner.class) @Config(manifest = Config.NONE, application = Application.class) @@ -603,7 +605,7 @@ public final class GroupsV2UpdateMessageProducerTest { .title("New title") .build(); - assertThat(describeChange(change), is(singletonList("Alice changed the group name to \"New title\"."))); + assertThat(describeChange(change), is(singletonList("Alice changed the group name to \"" + isolateBidi("New title") + "\"."))); } @Test @@ -612,7 +614,7 @@ public final class GroupsV2UpdateMessageProducerTest { .title("Title 2") .build(); - assertThat(describeChange(change), is(singletonList("You changed the group name to \"Title 2\"."))); + assertThat(describeChange(change), is(singletonList("You changed the group name to \"" + isolateBidi("Title 2") + "\"."))); } @Test @@ -621,7 +623,7 @@ public final class GroupsV2UpdateMessageProducerTest { .title("Title 3") .build(); - assertThat(describeChange(change), is(singletonList("The group name has changed to \"Title 3\"."))); + assertThat(describeChange(change), is(singletonList("The group name has changed to \"" + isolateBidi("Title 3") + "\"."))); } // Avatar change @@ -762,7 +764,7 @@ public final class GroupsV2UpdateMessageProducerTest { assertThat(describeChange(change), is(Arrays.asList( "Alice added Bob.", - "Alice changed the group name to \"Title\".", + "Alice changed the group name to \"" + isolateBidi("Title") + "\".", "Alice set the disappearing message timer to 5 minutes.", "Alice changed who can edit group membership to \"All members\"."))); } @@ -803,7 +805,7 @@ public final class GroupsV2UpdateMessageProducerTest { assertThat(describeChange(change), is(Arrays.asList( "Bob joined the group.", - "The group name has changed to \"Title 2\".", + "The group name has changed to \"" + isolateBidi("Title 2") + "\".", "The group avatar has been changed.", "The disappearing message timer has been set to 10 minutes.", "Who can edit group membership has been changed to \"All members\"."))); @@ -821,7 +823,7 @@ public final class GroupsV2UpdateMessageProducerTest { assertThat(describeChange(change), is(Arrays.asList( "Alice joined the group.", "Alice is now an admin.", - "The group name has changed to \"Updated title\".", + "The group name has changed to \"" + isolateBidi("Updated title") + "\".", "Alice is no longer in the group."))); }