Page MenuHomePhabricator

Utilities to idempotently create content and notif sessions
AcceptedPublic

Authored by marcin on Thu, Jul 4, 9:33 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Jul 22, 8:32 AM
Unknown Object (File)
Fri, Jul 19, 3:46 AM
Unknown Object (File)
Thu, Jul 18, 7:16 AM
Unknown Object (File)
Mon, Jul 15, 12:10 AM
Unknown Object (File)
Sat, Jul 13, 6:58 PM
Unknown Object (File)
Fri, Jul 12, 11:45 PM
Unknown Object (File)
Fri, Jul 12, 12:40 PM
Unknown Object (File)
Wed, Jul 10, 4:00 PM
Subscribers

Details

Reviewers
tomek
inka
Summary

This differential modifies utility function that creates content session to create both content and notif sessions idempotently

Test Plan
  1. Apply this stash: https://gist.github.com/marcinwasowicz/047b0f1b00079562b2547ebe3a8c18ad. This stash does some of the work https://linear.app/comm/project/send-encrypted-notification-from-the-client-to-the-tunnelbroker-03fc019ed3ee/issues is supposed to do. Message reducer is modified to return MessageData array and db-ops-handler greedily consumes MessageData array to create and send encrypted notifications through TB.
  2. Send text message from a client (any platform).
  3. Ensure that the first time you send message there are logs indicating that session are created with particular device ids (thread members device ids).
  4. Ensure that each time you see in the console logged payloads of encrypted notifications. Examine payloads. Ensure that they match relevant flow types. No notification will be displayed on the receiving client since they can't decrypt the notif yet.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

marcin requested review of this revision.Thu, Jul 4, 9:54 AM

I am afraid that there is a vulnerability in this diff. Basically it is possible that two createOlmSessionWithPeer promises will execute concurrently. This might lead to corrupted state. In order to mitigate that I think it is best expose this method as a context. The context would internally keep a map like [(userID, deviceID)] : Promise mapping from particular deviceID and userID to currently executing promise (if any).

tomek requested changes to this revision.Wed, Jul 10, 5:01 AM

I am afraid that there is a vulnerability in this diff. Basically it is possible that two createOlmSessionWithPeer promises will execute concurrently. This might lead to corrupted state. In order to mitigate that I think it is best expose this method as a context. The context would internally keep a map like [(userID, deviceID)] : Promise mapping from particular deviceID and userID to currently executing promise (if any).

Sounds like something that should be fixed.

lib/utils/crypto-utils.js
161–171 ↗(On Diff #42043)

It is possible that hasNotifsSession is true here - should we create the notifs session regardless?

This revision now requires changes to proceed.Wed, Jul 10, 5:01 AM
lib/utils/crypto-utils.js
161–171 ↗(On Diff #42043)

Perhaps I am missing something but I think that if we reach these lines then we are in the case that there are neither notifs nor content session. If statement at liner 126 should guarantee this

lib/utils/crypto-utils.js
161–171 ↗(On Diff #42043)

If hasContentSession = false and hasNotifsSession = true, then we won't enter the if at 126 line. We also won't enter the if at 152. So it seems possible to reach this code with the session present.

Fix race condition by exposing olm session creation via context

This diff looks confusing to me. We're introducing a new context PeerOlmSessionCreatorContext but it doesn't look like we're using it.

lib/components/peer-olm-session-creator-provider.react.js
35–38 ↗(On Diff #42348)

This looks fragile. We have a function that accepts two parameters but keep promises in an object indexed by only one of them. What if someone calls this method twice with the same deviceID and with different userIDs? We can solve this issue in two ways: 1. we can index by two properties, or 2. we can remove userID parameter and get it from e.g. a selector. Not sure which is better, but in the second solution we should probably also clear the runningPromises ref.

Or maybe, userID here means the ID of a user possessing a device with deviceID. If that's correct, we're fine with the current solution.

This revision is now accepted and ready to land.Wed, Jul 17, 4:37 AM