matrix-js-sdk icon indicating copy to clipboard operation
matrix-js-sdk copied to clipboard

We don't appear to be retrying `/sendToDevice` requests

Open dkasak opened this issue 4 years ago • 8 comments

I don't think we're retrying failed /sendToDevice requests. This comment seems to agree.

This is especially bad when e.g. sharing room keys in encryptAndSendKeysToDevices since we might end up marking a key as sent to a device when in reality the request failed. Though I'm not sure whether the then(...) is executed if the request fails or not.

All call sites:

  • https://github.com/matrix-org/matrix-js-sdk/blob/96e8f65af7429339dbed4bc8063b893b4a0966ce/src/crypto/algorithms/megolm.ts#L634
  • https://github.com/matrix-org/matrix-js-sdk/blob/96e8f65af7429339dbed4bc8063b893b4a0966ce/src/crypto/algorithms/megolm.ts#L686
  • https://github.com/matrix-org/matrix-js-sdk/blob/96e8f65af7429339dbed4bc8063b893b4a0966ce/src/crypto/algorithms/megolm.ts#L780
  • https://github.com/matrix-org/matrix-js-sdk/blob/96e8f65af7429339dbed4bc8063b893b4a0966ce/src/crypto/algorithms/megolm.ts#L1552
  • https://github.com/matrix-org/matrix-js-sdk/blob/96e8f65af7429339dbed4bc8063b893b4a0966ce/src/crypto/algorithms/megolm.ts#L1635
  • https://github.com/matrix-org/matrix-js-sdk/blob/96e8f65af7429339dbed4bc8063b893b4a0966ce/src/crypto/algorithms/megolm.ts#L1823
  • https://github.com/matrix-org/matrix-js-sdk/blob/96e8f65af7429339dbed4bc8063b893b4a0966ce/src/crypto/index.ts#L3313
  • https://github.com/matrix-org/matrix-js-sdk/blob/96e8f65af7429339dbed4bc8063b893b4a0966ce/src/crypto/OutgoingRoomKeyRequestManager.ts#L481
  • https://github.com/matrix-org/matrix-js-sdk/blob/96e8f65af7429339dbed4bc8063b893b4a0966ce/src/crypto/SecretStorage.ts#L402
  • https://github.com/matrix-org/matrix-js-sdk/blob/96e8f65af7429339dbed4bc8063b893b4a0966ce/src/crypto/SecretStorage.ts#L423
  • https://github.com/matrix-org/matrix-js-sdk/blob/96e8f65af7429339dbed4bc8063b893b4a0966ce/src/crypto/SecretStorage.ts#L525

dkasak avatar Aug 19 '21 14:08 dkasak

what are the consequences of this for the user?

novocaine avatar Aug 19 '21 14:08 novocaine

to-device messages are used for setting up 1:1 olm channels; sharing new megolm sessions; requesting keyshares; and in future VoIP signalling. so if one goes missing, it could wedge the ability to send to a single device in a room (until that device spots and renegotiates, if correctly implemented), or stop the megolm session being shared with all devices in the room, killing the next ~100 msgs sent by that device (until the recipients request the sender to resend, if the sender is still online), or cause a subsequent keyshare req/response to fail.

In other words, it causes UISIs to one or more devices, which should in theory be eventually be recoverable, but may hang around for ages if the receiver tries to read the msgs after the sender has gone offline.

ara4n avatar Aug 19 '21 14:08 ara4n

so the user impact of this is something like: a temporary network failure when sharing E2EE keys may stop them from ever being shared

novocaine avatar Aug 19 '21 14:08 novocaine

hrm, sendToDevice returns the promise from this.http.authRequest (https://github.com/matrix-org/matrix-js-sdk/blob/96e8f65af7429339dbed4bc8063b893b4a0966ce/src/client.ts#L7867) - I think .then(...) for that object would only execute if that network request was successful..

So .. I guess the consequence might not that be that we mark the keys as sent when they weren't, but we might never run that code again in the current session?

novocaine avatar Aug 19 '21 16:08 novocaine

This looks related to https://github.com/vector-im/element-web/issues/18666

MadLittleMods avatar Aug 24 '21 16:08 MadLittleMods

This looks related to vector-im/element-web#18666

Is it related? That issue doesn't read to me like a bug report about not retrying failed requests, but more like a feature request to share keys with new verified devices even though keys were never shared with them in the first place.

dkasak avatar Aug 24 '21 18:08 dkasak

But it is related to https://github.com/vector-im/element-web/issues/12851.

dkasak avatar Apr 19 '22 08:04 dkasak

This is now partially fixed by https://github.com/matrix-org/matrix-js-sdk/pull/2549 for room key sending. Other to-device messages are not retried yet.

dkasak avatar Aug 29 '22 13:08 dkasak