fix: defensive null-safety for ServiceId fields in MessageEnvelope#1944
Closed
strichter wants to merge 2 commits intoAsamK:masterfrom
Closed
fix: defensive null-safety for ServiceId fields in MessageEnvelope#1944strichter wants to merge 2 commits intoAsamK:masterfrom
strichter wants to merge 2 commits intoAsamK:masterfrom
Conversation
Signal Desktop v8.0.0 switched to binary ACI encoding (*AciBinary fields) in protobuf messages. While libsignal-service-java v2.15.3_unofficial_138 adds dual-format parsing, the MessageEnvelope construction layer in signal-cli can still encounter null ServiceId values in edge cases or with older library versions. This change adds comprehensive null-safety guards: - Quote: Remove overly strict .filter() that drops the entire quote when the author ServiceId is null or invalid. Instead, preserve quote content and fall back to UNKNOWN_UUID for the author. Quote text and attachments remain valuable context even without author attribution. - Reaction: Add null guard for targetAuthor before resolving recipient. A reaction without a target author cannot be meaningfully processed. - Mention: Filter out mentions with null ServiceId to prevent NPE during recipient resolution while preserving valid mentions in the same message. - StoryContext: Add null guard for authorServiceId before resolving. - PollVote: Add null guard for targetAuthor before resolving. - Quote text: Use Optional.ofNullable() instead of Optional.of() to handle null quote text defensively. Fixes AsamK#1935, AsamK#1940 Related: AsamK#1938
Author
|
I did test the fix (reaction, mention, reply) with Signal Desktop and Mobile to ensure both are working. |
Add tests for the defensive null-safety handling in MessageEnvelope.Data: - Quote with valid author: preserved with correct UUID - Quote with ACI.UNKNOWN author: preserved with UNKNOWN_UUID fallback (previously dropped entirely by the isValid() filter) - Quote with empty text: handled defensively - Reaction with valid author: preserved with emoji and timestamp - Reaction with ACI.UNKNOWN author: preserved (non-null, still useful) - Mention with valid ServiceId: preserved with correct position - Multiple mentions: all preserved - No mentions: returns empty list - Plain message: no optional fields present - Combined quote + mentions: both preserved Also adds TestRecipientIdFactory to create RecipientId instances in tests without requiring the package-private RecipientStore dependency.
2 tasks
strichter
added a commit
to strichter/signal-cli-rest-api
that referenced
this pull request
Feb 28, 2026
Signal Desktop v8.0.0 switched from string ACI fields to binary ACI encoding in protobuf messages. This breaks reactions, mentions, quotes, and other message features when the library cannot parse the new format. Two-part fix applied via patch to signal-cli v0.13.24 source build: 1. Bump signal-service-java from unofficial_137 to unofficial_138, which adds dual-format ACI parsing (string + binary fallback via ServiceId.parseOrNull). 2. Add defensive null guards in MessageEnvelope.java for cases where ServiceId resolution still fails (e.g. ACI.UNKNOWN). Preserves message content with UNKNOWN_UUID fallback rather than dropping entire message components (quotes, reactions, mentions, etc.). The patch is applied during the x86_64 source build. The source-built installDist output replaces the release tarball, so both the JVM and native (GraalVM) paths get the fix. Non-x86_64 architectures continue using the unpatched release tarball until signal-cli cuts a new release with unofficial_138. See: AsamK/signal-cli#1944 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
5 tasks
Owner
|
Did you test the latest master version without your fix as well? Because reaction, mention, reply should already work there. |
10 tasks
Author
|
Hi Sebastian, I just checked with master and it is indeed working. Though I think the changes are valid protections. That said, feel free to close this PR. |
3 tasks
Owner
|
Thanks for checking! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Signal Desktop v8.0.0 switched to binary ACI encoding (
*AciBinaryfields) in protobuf messages. Whilelibsignal-service-java:2.15.3_unofficial_138(already on master) adds dual-format parsing, theMessageEnvelopeconstruction layer can still encounter nullServiceIdvalues in edge cases, causing data loss or NPEs.This PR adds comprehensive null-safety guards to all
ServiceId-dependent fields inMessageEnvelope.java:Quote (the main fix): Remove the
.filter(q -> q.getAuthor() != null && q.getAuthor().isValid())that silently drops the entire quote when the author cannot be resolved. Instead, preserve quote content and fall back toUNKNOWN_UUIDfor the author — the quote text, mentions, and attachments remain valuable context even without author attribution. Also useOptional.ofNullable()for quote text.Reaction: Add null guard for
targetAuthorbefore recipient resolution to prevent NPE.Mention: Filter out individual mentions with null
ServiceIdwhile preserving valid mentions in the same message.StoryContext: Add null guard for
authorServiceId.PollVote: Add null guard for
targetAuthor.Testing
Built from master and verified end-to-end with a linked device. Tested all three primary failure modes from Signal Desktop v8.0.0:
dataMessage, reaction field missingreactionpresent withtargetAuthorUuid\uFFFCplaceholder, nomentionsarraymentionsarray with resolved UUIDsquotewithauthorUuidand textAll cases also verified working from Mobile (string ACI encoding) — backward compatible.
Build and tests pass:
Related