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

crypto: Don't deep copy the OlmMachine when creating a notification client #3992

Merged
merged 2 commits into from
Sep 16, 2024

Conversation

poljar
Copy link
Contributor

@poljar poljar commented Sep 13, 2024

Opening as a draft since I will want to make test for this. This closes: #1415.

  • Public API changes documented in changelogs (optional)

Copy link

codecov bot commented Sep 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.25%. Comparing base (b7ce2dc) to head (f590543).
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3992      +/-   ##
==========================================
- Coverage   84.26%   84.25%   -0.02%     
==========================================
  Files         266      266              
  Lines       28332    28336       +4     
==========================================
- Hits        23875    23874       -1     
- Misses       4457     4462       +5     

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

@poljar poljar force-pushed the poljar/notification-duplicate-olm-machine-fix branch from 2e96459 to d4c51d6 Compare September 16, 2024 14:33
@poljar poljar marked this pull request as ready for review September 16, 2024 14:33
@poljar poljar requested a review from a team as a code owner September 16, 2024 14:33
@poljar poljar requested review from bnjbvr and removed request for a team September 16, 2024 14:33
Copy link
Member

@bnjbvr bnjbvr left a comment

Choose a reason for hiding this comment

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

Amazing catch, thanks for writing a test too. I'll send you my best baguette over mail.

@poljar poljar force-pushed the poljar/notification-duplicate-olm-machine-fix branch from 3635fe4 to f32a795 Compare September 16, 2024 16:01
@poljar poljar enabled auto-merge (rebase) September 16, 2024 16:01
@poljar poljar force-pushed the poljar/notification-duplicate-olm-machine-fix branch from f32a795 to 5db87b5 Compare September 16, 2024 16:10
…nClient

The NotificationClient, responsible for handling, fetching, and
potentially decrypting events received via push notifications, creates a
copy of the main Client object.

During this process, the Client object is adjusted to use an in-memory
state store to prevent concurrency issues from multiple sync loops
attempting to write to the same database.

This copying unintentionally recreated the OlmMachine with fresh data
loaded from the database. If both Client instances were used for syncing
without proper cross-process locking, forks of the vodozemac Account and
Olm Sessions could be created and later persisted to the database.

This behavior can lead to the duplication of one-time keys, cause
sessions to lose their ability to decrypt messages, and result in the
generation of undecryptable messages on the recipient’s side.
@poljar poljar force-pushed the poljar/notification-duplicate-olm-machine-fix branch from 5db87b5 to f590543 Compare September 16, 2024 16:13
@poljar poljar merged commit f576c72 into main Sep 16, 2024
40 checks passed
@poljar poljar deleted the poljar/notification-duplicate-olm-machine-fix branch September 16, 2024 16:27
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.

Lost OTK, leading to "OneTime key already exists" error and later UTDs
2 participants