Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix sending a sync message to the same device twice. #108

Closed
wants to merge 1 commit into from

Conversation

boxdot
Copy link
Contributor

@boxdot boxdot commented Aug 2, 2021

This happens when the session store contains overlapping sessions for
the uuid and the e164 phone number of the same account. A device
which has both session receives and shows a sync message twice.

This happens when the session store contains overlapping sessions for
the uuid and the e164 phone number of the same account. A device
which has both session receives and shows a sync message twice.
@rubdos
Copy link
Member

rubdos commented Aug 3, 2021

You should never have overlapping sessions. The remote end cannot distinguish them, and this leads to undecryptable messages.

You should instead have logic like this merge_and_fetch_recipient, which decides when a phone number is moved or tied to a UUID. Better yet, Android's RecipientDatabase::getAndPossiblyMerge. An e164 and UUID that are "together" should never have separate sessions.

@rubdos
Copy link
Member

rubdos commented Sep 4, 2021

@boxdot , are you okay with closing this? Maybe we should document these requirements somehow, but it'd quite a challenge to document how to "correctly" implement the whole of Signal.

Maybe another approach here could be to give an error when overlapping sessions are retrieved, à la "This is a bug in the application and should be reported. The session with XYZ is now probably corrupt.".

@boxdot
Copy link
Contributor Author

boxdot commented Sep 4, 2021

I still don't have a working solution in my client. But I will close it.

@boxdot boxdot closed this Sep 4, 2021
@rubdos
Copy link
Member

rubdos commented Sep 4, 2021

I still don't have a working solution in my client. But I will close it.

If you want, you can open an issue about session/uuid/e164 management and merging; it should be relatively easy to port the (currently incomplete) session merging logic from Whisperfish into here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants