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

Don't cancel SAS verifications if ready is received after start #2250

Merged
merged 2 commits into from
May 23, 2022

Conversation

ara4n
Copy link
Member

@ara4n ara4n commented Mar 20, 2022

it's completely valid to receive a ready event after having received a
start event as messages may be received or decrypted in any order.

partial (but possibly sufficient?) fix for element-hq/element-web#21488


Here's what your changelog entry will look like:

🐛 Bug Fixes

  • Don't cancel SAS verifications if ready is received after start (#2250).

it's completely valid to receive a `ready` event after having received a
`start` event as messages may be received or decrypted in any order.

partial (but possibly sufficient?) fix for element-hq/element-web#21488
@codecov-commenter
Copy link

codecov-commenter commented Mar 20, 2022

Codecov Report

Merging #2250 (d9f0704) into develop (75674d9) will not change coverage.
The diff coverage is 50.00%.

@@           Coverage Diff            @@
##           develop    #2250   +/-   ##
========================================
  Coverage    58.11%   58.11%           
========================================
  Files           92       92           
  Lines        16427    16427           
  Branches      3782     3782           
========================================
  Hits          9546     9546           
  Misses        6881     6881           
Impacted Files Coverage Δ
src/webrtc/callEventHandler.ts 7.80% <0.00%> (ø)
...crypto/verification/request/VerificationRequest.ts 80.09% <100.00%> (ø)

Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

The spec wants us to validate the order, but it also doesn't say how to deal with the ordering or if order is guaranteed, so I suppose this is fine.

Given the risk of touching verification though, I'm tempted to merge this post-RC rather than now for absolute safety (though I also have zero hope of it being tested while on develop/staging anyways).

@ara4n
Copy link
Member Author

ara4n commented May 23, 2022

this is still a valid fix. the fact the spec asks us to validate ordering on something which has no ordering is a spec bug to be fixed.

@ara4n ara4n enabled auto-merge May 23, 2022 19:54
@ara4n ara4n merged commit db4a6fa into develop May 23, 2022
@ara4n ara4n deleted the matthew/fix-flaky-verif-test branch May 23, 2022 19:59
su-ex added a commit to SchildiChat/matrix-js-sdk that referenced this pull request Jun 7, 2022
* Convert `getLocalAliases` to a stable API call ([\matrix-org#2402](matrix-org#2402)).
* Fix request, crypto, and bs58 imports ([\matrix-org#2414](matrix-org#2414)). Fixes matrix-org#2415.
* Update relations after every decryption attempt ([\matrix-org#2387](matrix-org#2387)). Fixes element-hq/element-web#22258. Contributed by @weeman1337.
* Fix degraded mode for the IDBStore and test it ([\matrix-org#2400](matrix-org#2400)). Fixes matrix-org/element-web-rageshakes#13170.
* Don't cancel SAS verifications if `ready` is received after `start` ([\matrix-org#2250](matrix-org#2250)).
* Prevent overlapping sync accumulator persists ([\matrix-org#2392](matrix-org#2392)). Fixes vector-im/element-web#21541.
* Fix behaviour of isRelation with relation m.replace for state events ([\matrix-org#2389](matrix-org#2389)). Fixes element-hq/element-web#22280.
* Fixes matrix-org#2384 ([\matrix-org#2385](matrix-org#2385)). Fixes undefined/matrix-js-sdk#2384. Contributed by @schmop.
* Ensure rooms are recalculated on re-invites ([\matrix-org#2374](matrix-org#2374)). Fixes element-hq/element-web#22106.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants