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

Encrypted events seem to get into handleCallEvent() #18540

Open
SimonBrandner opened this issue Aug 13, 2021 · 8 comments
Open

Encrypted events seem to get into handleCallEvent() #18540

SimonBrandner opened this issue Aug 13, 2021 · 8 comments
Labels
A-E2EE A-VoIP O-Frequent Affects or can be seen by most users regularly or impacts most users' first experience S-Tolerable Low/no impact on users T-Defect X-Needs-Investigation

Comments

@SimonBrandner
Copy link
Contributor

SimonBrandner commented Aug 13, 2021

Screenshot_20210813_213936

@SimonBrandner SimonBrandner added A-E2EE A-VoIP O-Frequent Affects or can be seen by most users regularly or impacts most users' first experience S-Major Severely degrades major functionality or product features, with no satisfactory workaround T-Defect X-Regression labels Aug 13, 2021
@Palid
Copy link
Contributor

Palid commented Aug 14, 2021

Related: matrix-org/matrix-js-sdk#1834
Looks like this PR has shown us additional problems we didn't know about before.

@SimonBrandner SimonBrandner self-assigned this Aug 15, 2021
@SimonBrandner
Copy link
Contributor Author

It's quite unclear to me how can this happen since we have this line: https://github.com/matrix-org/matrix-js-sdk/blob/977720cee47e49371db46dca291868b78b2c29ca/src/webrtc/callEventHandler.ts#L61

@dbkr
Copy link
Member

dbkr commented Aug 16, 2021

Interesting. How do we know this is an encrypted event? Does this visibly cause any badness? How did you repro this?

@SimonBrandner
Copy link
Contributor Author

Interesting. How do we know this is an encrypted event?

The log says, so - event.getType() === "m.room.encrypted"

Does this visibly cause any badness?

No, it just unnecessarily goes through handleCallEvent wasting some tiny amount of time

How did you repro this?

Open an E2EE and receive a message

@SimonBrandner SimonBrandner added S-Tolerable Low/no impact on users and removed S-Major Severely degrades major functionality or product features, with no satisfactory workaround labels Aug 16, 2021
@SimonBrandner
Copy link
Contributor Author

I think I've run out of ideas here, if someone wants to take a look please go ahead

@SimonBrandner SimonBrandner removed their assignment Aug 16, 2021
@dkasak
Copy link
Member

dkasak commented Aug 18, 2021

I just stumbled across this while debugging something else. These are m.bad.encrypted messages that we're injecting when we can't decrypt a message:

content: Object {
    msgtype: "m.bad.encrypted",
    body: "** Unable to decrypt: The sender's device has not sent us the keys for this message. **"
}

I think the reason we're reporting m.room.encrypted instead of m.room.message (with msgtype: m.bad.encrypted) is that we're recording the type at the beginning of the handleCallEvent function and it gets decrypted between that location and the logger call.

It's quite unclear to me how can this happen since we have this line: https://github.com/matrix-org/matrix-js-sdk/blob/977720cee47e49371db46dca291868b78b2c29ca/src/webrtc/callEventHandler.ts#L61

That's not the only place from which can reach handleCallEvent, since it's also called directly here so maybe that has something to do with it?

No, it just unnecessarily goes through handleCallEvent wasting some tiny amount of time

I think log pollution is also quite counterproductive when it comes to debugging, especially when we're producing spurious warnings/errors.

To reproduce, just join an encrypted room with some existing history for which you won't receive keys (e.g. the Megolm test room) and watch the console.

@SimonBrandner
Copy link
Contributor Author

SimonBrandner commented Aug 18, 2021

I think the reason we're reporting m.room.encrypted instead of m.room.message (with msgtype: m.bad.encrypted) is that we're recording the type at the beginning of the handleCallEvent function and it gets decrypted between that location and the logger call.

It's quite unclear to me how can this happen since we have this line: https://github.com/matrix-org/matrix-js-sdk/blob/977720cee47e49371db46dca291868b78b2c29ca/src/webrtc/callEventHandler.ts#L61

@dkasak
Copy link
Member

dkasak commented Aug 18, 2021

@SimonBrandner Did you perhaps miss this later part of my comment?

That's not the only place from which can reach handleCallEvent, since it's also called directly here so maybe that has something to do with it?

Maybe the link in that was not obvious; I was linking to this direct handleCallEvent which doesn't go through evaluateEventBuffer (which contains the line you're quoting): https://github.com/matrix-org/matrix-js-sdk/blob/c2a0c02898f6bd1d4e508559b67880494005e1de/src/webrtc/callEventHandler.ts#L113

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-E2EE A-VoIP O-Frequent Affects or can be seen by most users regularly or impacts most users' first experience S-Tolerable Low/no impact on users T-Defect X-Needs-Investigation
Projects
None yet
Development

No branches or pull requests

4 participants