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

Fix "e is undefined" masking the original error in MegolmDecryption #847

Merged
merged 2 commits into from
Feb 26, 2019

Conversation

turt2live
Copy link
Member

Someone with a bit of knowledge around the state machine for encryption probably wants to review this.

@turt2live turt2live requested a review from a team February 26, 2019 20:15
Copy link
Member

@uhoreg uhoreg left a comment

Choose a reason for hiding this comment

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

Why is anything throwing undefined? 😮

Looks reasonable, though I don't know how much good it will do, given that if e is undefined, then there isn't really an "original error" that we can get any useful information from. On the other hand, it surely can't hurt, so 👍 .

@@ -758,15 +758,15 @@ MegolmDecryption.prototype.decryptEvent = async function(event) {
} catch (e) {
let errorCode = "OLM_DECRYPT_GROUP_MESSAGE_ERROR";

if (e.message === 'OLM.UNKNOWN_MESSAGE_INDEX') {
if (e && e.message && e.message === 'OLM.UNKNOWN_MESSAGE_INDEX') {
Copy link
Member

Choose a reason for hiding this comment

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

I think only e && e.message === 'OLM.UNKNOWN_MESSAGE_INDEX' is required, since if e.message is falsy, then it won't equal to OLM.UNKNOWN_MESSAGE_INDEX.

@turt2live
Copy link
Member Author

Good point - if we're getting undefined then something else is horribly wrong. At least this should (hopefully) make it a bit clearer though, even if it is just reducing the noise in the logs.

@bwindels
Copy link
Contributor

bwindels commented Feb 27, 2019

Why is anything throwing undefined? open_mouth

Looks reasonable, though I don't know how much good it will do, given that if e is undefined, then there isn't really an "original error" that we can get any useful information from. On the other hand, it surely can't hurt, so +1 .

Could be https://github.com/matrix-org/matrix-js-sdk/blob/master/src/crypto/store/indexeddb-crypto-store-backend.js#L697 We should probably take the same approach as in the other idb store?

@uhoreg
Copy link
Member

uhoreg commented Feb 27, 2019

@bwindels good catch. I don't know why it different, but certainly the reject() is incorrect.

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.

3 participants