[lib] Restart session when any Olm error is thrown
AcceptedPublic
Actions

Authored by angelika on Mon, Oct 21, 6:20 AM.

Details

Reviewers

kamil
tomek
ashoat

Summary

This diff restarts session when any Olm error is thrown by throwing an exception with OLM_ERROR flag set if error occured when decrypting the message.
If flag is included in the exception then the session is reset.

https://linear.app/comm/issue/ENG-9729/session-not-reset-after-getting-bad-message-mac

Test Plan

Simulate BAD_MESSAGE_MAC by changing the last 3 characters of the message passed to session.decrypt() in the code
Verify in the logs that BAD_MESSAGE_MAC is thrown and session is reset.

Diff Detail

Repository

rCOMM Comm

Branch

graszka22/ENG-9729

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

angelika created this revision.Mon, Oct 21, 6:20 AM

Herald added subscribers: tomek, ashoat. · View Herald TranscriptMon, Oct 21, 6:20 AM

angelika added reviewers: kamil, ashoat.Mon, Oct 21, 6:21 AM

Harbormaster completed remote builds in B32315: Diff 45289.Mon, Oct 21, 6:38 AM

angelika requested review of this revision.Mon, Oct 21, 6:38 AM

angelika edited reviewers, added: tomek; removed: ashoat.Mon, Oct 21, 7:12 AM

ashoat added inline comments.Mon, Oct 21, 9:05 AM

lib/tunnelbroker/use-peer-to-peer-message-handler.js
307 ↗	(On Diff #45289)	I realize that this issue existed before, but there is a risk here... if the code throws a primitive (eg. `throw 5`), then we will get an error when trying to dereference `5.message` This can be mitigated by using `getMessageForException`. Given this pattern exists in the DMs code in more places than just here, perhaps it should be handled in a separate diff.
native/cpp/CommonCpp/CryptoTools/Session.cpp
177 ↗	(On Diff #45289)	Given that `OLM_ERROR` has some magical importance, I think we should do more to make sure that somebody doesn't change it. When we have two constants that need to match between two parts of the codebase, usually we add a code comment referring to the other location, and telling the reader to not change one location without changing the other. Could you: Factor this out into a constant Add a code comment to the constant's definition, as well as the definition in `olm-utils.js`. Each comment should point to the other location, and should caution the reader to not change one without changing the other