Page MenuHomePhabricator

Fix BAD_MESSAGE_FORMAT issue with e2ee web notifs
ClosedPublic

Authored by marcin on Sep 16 2024, 10:55 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Nov 20, 7:02 AM
Unknown Object (File)
Wed, Nov 20, 6:39 AM
Unknown Object (File)
Sat, Nov 16, 2:51 PM
Unknown Object (File)
Sat, Nov 9, 7:07 PM
Unknown Object (File)
Oct 22 2024, 10:41 AM
Unknown Object (File)
Oct 22 2024, 10:20 AM
Unknown Object (File)
Oct 22 2024, 10:20 AM
Unknown Object (File)
Oct 22 2024, 10:20 AM
Subscribers
None

Details

Summary

This differential fixes this bug. All decryption methods take message type as argument. The only exception are the top level methods that decrypt notifs from the keyserver

Test Plan

Execute steps here. Ensure the issue does not reproduce.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Would be good for somebody more familiar with notif-crypto-utils.js to review

web/push-notif/notif-crypto-utils.js
418 ↗(On Diff #44233)

How do we know this will always be TEXT?

549 ↗(On Diff #44233)

How do we know this will always be TEXT?

786 ↗(On Diff #44233)

What's the context on this change? Seems like it should be a separate diff

tomek added 1 blocking reviewer(s): kamil.
tomek added inline comments.
web/push-notif/notif-crypto-utils.js
716–719 ↗(On Diff #44233)

It will be easy to forget about updating this place while introducing a new message type. We should probably introduce a function that takes a string, returns OlmEncryptedMessageTypes, and asserts.

web/push-notif/notif-crypto-utils.js
418 ↗(On Diff #44233)

Keyserver notifications are one-way, ane the keyserver is the inbound. Therefore the only prekey message is the initial encrypted send by the client. Not that this is not new hardcoding but rather moving hardcoding to higher level place.

549 ↗(On Diff #44233)

Same as above

716–719 ↗(On Diff #44233)

Okay, but it is very unlikely that we will introduce new type since those types come from olm protocol.

786 ↗(On Diff #44233)

I've just realised that we don't delete one time key after creating inbound session. Okay - I will create new diff. This change is also necessary on native.

This revision is now accepted and ready to land.Sep 18 2024, 12:52 AM
This revision was landed with ongoing or failed builds.Sep 18 2024, 7:27 AM
This revision was automatically updated to reflect the committed changes.