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 SyncMessage to self #263

Merged
merged 5 commits into from
Nov 19, 2023
Merged

Fix sending SyncMessage to self #263

merged 5 commits into from
Nov 19, 2023

Conversation

gferon
Copy link
Collaborator

@gferon gferon commented Nov 19, 2023

Fixes sending SyncMessage and to self.

  • After looking at both Signal-Android and Signal-Desktop, we need to make sure any message sent to self is never send unidentified
  • Make sure we add padding to all SyncMessage
  • Remove some redundant session loading before creating encrypted messages

@gferon gferon requested a review from direc85 November 19, 2023 14:28
Copy link

codecov bot commented Nov 19, 2023

Codecov Report

Attention: 18 lines in your changes are missing coverage. Please review.

Comparison is base (afb5114) 3.66% compared to head (d53df2a) 3.65%.

❗ Current head d53df2a differs from pull request most recent head bc8f646. Consider uploading reports for the commit bc8f646 to get more accurate results

Files Patch % Lines
libsignal-service/src/proto.rs 0.00% 9 Missing ⚠️
libsignal-service/src/sender.rs 0.00% 6 Missing ⚠️
libsignal-service/src/websocket.rs 0.00% 3 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##            main    #263      +/-   ##
========================================
- Coverage   3.66%   3.65%   -0.02%     
========================================
  Files         38      38              
  Lines       2809    2818       +9     
========================================
  Hits         103     103              
- Misses      2706    2715       +9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@direc85 direc85 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, assuming some on-device testing has been made!

libsignal-service/src/sender.rs Outdated Show resolved Hide resolved
libsignal-service/src/sender.rs Outdated Show resolved Hide resolved
@gferon gferon changed the title Mark internally generated sync messages as such Fix sending SyncMessage to self Nov 19, 2023
@gferon gferon enabled auto-merge (squash) November 19, 2023 22:44
@gferon gferon merged commit 6fc62c8 into main Nov 19, 2023
11 checks passed
@gferon gferon deleted the is-sync-message branch November 19, 2023 22:46
Comment on lines +71 to +72
impl SyncMessage {
pub fn with_padding() -> Self {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gferon Shouldn't we make those stronger types, such that it's impossible to transmit without padding?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that was a ninja trick for the time being.

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.

3 participants