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

Use a more structured algorithm for determining decryption client for appservice events #41

Merged
merged 8 commits into from
Dec 4, 2023

Conversation

Half-Shot
Copy link
Member

This PR changes the way we pick a client for decrypting an event, and caches the result so that it can be reused. This is intended to ensure that when the sender user isn't used for decryption, Appservice doesn't make repeated calls to get_joined_rooms, and when it picks a client it prefers one with encryption already enabled to save time.

We also update to ES2022 so I can use Error.cause, one of my favourite new ES features.

Checklist

  • Tests written for all new code
  • Linter has been satisfied
  • Sign-off given on the changes (see CONTRIBUTING.md)

@Half-Shot Half-Shot requested a review from a team as a code owner November 28, 2023 10:18
src/appservice/Appservice.ts Outdated Show resolved Hide resolved
private async decryptAppserivceEvent(roomId: string, encrypted: EncryptedRoomEvent): ReturnType<Appservice["processEvent"]> {
const existingClient = this.cryptoClientForRoomId.get(roomId);
const decryptFn = async (client: MatrixClient) => {
let event = (await client.crypto.decryptRoomEvent(encrypted, roomId)).raw;
Copy link
Member

Choose a reason for hiding this comment

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

If the rust-sdk's OlmMachine doesn't discard keys for rooms that a user has left, then this should first check if client is currently in the target room. Otherwise, this would decrypt an event that client shouldn't be able to access.

On the other hand, it might not matter in practice whether client shouldn't see the event, because as long as any of the appservice's users can see it, there's no difference in terms of privacy/security which of its clients the appservices uses to decrypt an event.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think we materially care on the AS side. If we still have keys and can turn an encrypted body into unencrypted text then yay. It doesn't really matter which client did it.

@Half-Shot Half-Shot merged commit 545eb16 into element-hq:main Dec 4, 2023
7 checks passed
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.

2 participants