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

Various changes to src/crypto files for correctness #2137

Merged
merged 18 commits into from
Jun 13, 2022

Conversation

ShadowJonathan
Copy link
Contributor

@ShadowJonathan ShadowJonathan commented Jan 26, 2022

This is an experimental PR for figuring out how to add changes that's brought about by making the js-sdk codebase more strict and correct, less "lax".

Type: internal

Signed-off-by: Jonathan de Jong <jonathan@automatia.nl>


Here's what your changelog entry will look like:

✨ Features

Copy link
Contributor Author

@ShadowJonathan ShadowJonathan left a comment

Choose a reason for hiding this comment

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

Here are some further explanations on some decisions i took.

src/crypto/algorithms/base.ts Show resolved Hide resolved
src/crypto/algorithms/megolm.ts Show resolved Hide resolved
src/crypto/algorithms/megolm.ts Show resolved Hide resolved
src/crypto/algorithms/megolm.ts Outdated Show resolved Hide resolved
src/crypto/algorithms/megolm.ts Show resolved Hide resolved
src/crypto/algorithms/megolm.ts Outdated Show resolved Hide resolved
src/crypto/algorithms/megolm.ts Show resolved Hide resolved
Copy link
Member

@t3chguy t3chguy left a comment

Choose a reason for hiding this comment

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

LGTM otherwise, thanks

src/crypto/algorithms/megolm.ts Outdated Show resolved Hide resolved
src/crypto/algorithms/megolm.ts Outdated Show resolved Hide resolved
src/crypto/algorithms/megolm.ts Outdated Show resolved Hide resolved
src/crypto/algorithms/megolm.ts Show resolved Hide resolved
src/crypto/algorithms/megolm.ts Outdated Show resolved Hide resolved
src/crypto/algorithms/megolm.ts Outdated Show resolved Hide resolved
src/crypto/algorithms/megolm.ts Outdated Show resolved Hide resolved
src/crypto/algorithms/megolm.ts Outdated Show resolved Hide resolved
src/crypto/algorithms/megolm.ts Show resolved Hide resolved
src/crypto/algorithms/megolm.ts Outdated Show resolved Hide resolved
@t3chguy t3chguy self-requested a review May 11, 2022 10:58
Copy link
Member

@t3chguy t3chguy left a comment

Choose a reason for hiding this comment

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

Looks sane to me, though it will need some additional test coverage to make SonarQube happy

If you think Sonar's coverage is off, make sure your fork's develop branch is up to date with upstream

@MadLittleMods MadLittleMods added the Z-Community-PR Issue is solved by a community member's PR label Jun 2, 2022
@t3chguy t3chguy requested a review from a team June 10, 2022 13:43
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.

otherwise lgtm code-wise. I'm going to send this for crypto team review to ensure it's still behaving correctly, but I think it's generally fine.

spec/unit/crypto.spec.js Outdated Show resolved Hide resolved
spec/unit/crypto/algorithms/megolm.spec.js Outdated Show resolved Hide resolved
Co-authored-by: Travis Ralston <travpc@gmail.com>
@t3chguy t3chguy requested a review from turt2live June 10, 2022 21:40
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.

lgtm pending crypto review

Copy link
Contributor

@duxovni duxovni left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM, thank you!

spec/unit/crypto.spec.js Outdated Show resolved Hide resolved
@duxovni duxovni removed the X-Blocked label Jun 13, 2022
Co-authored-by: Faye Duxovni <duxovni@duxovni.org>
@t3chguy t3chguy enabled auto-merge (squash) June 13, 2022 18:59
@t3chguy t3chguy merged commit 78db74d into matrix-org:develop Jun 13, 2022
su-ex added a commit to SchildiChat/matrix-js-sdk that referenced this pull request Jul 7, 2022
* Remove unused sessionStore ([\matrix-org#2455](matrix-org#2455)).
* Implement MSC3827: Filtering of `/publicRooms` by room type ([\matrix-org#2469](matrix-org#2469)).
* expose latestLocationEvent on beacon model ([\matrix-org#2467](matrix-org#2467)). Contributed by @kerryarchibald.
* Live location share - add start time leniency ([\matrix-org#2465](matrix-org#2465)). Contributed by @kerryarchibald.
* Log real errors and not just their messages, traces are useful ([\matrix-org#2464](matrix-org#2464)).
* Various changes to `src/crypto` files for correctness ([\matrix-org#2137](matrix-org#2137)). Contributed by @ShadowJonathan.
* Update MSC3786 implementation: Check the `state_key` ([\matrix-org#2429](matrix-org#2429)).
* Timeline needs to refresh when we see a MSC2716 marker event  ([\matrix-org#2299](matrix-org#2299)). Contributed by @MadLittleMods.
* Try to load keys from key backup when a message fails to decrypt ([\matrix-org#2373](matrix-org#2373)). Fixes element-hq/element-web#21026. Contributed by @duxovni.
* Send call version `1` as a string ([\matrix-org#2471](matrix-org#2471)). Fixes element-hq/element-web#22629.
* Fix issue with `getEventTimeline` returning undefined for thread roots in main timeline ([\matrix-org#2454](matrix-org#2454)). Fixes element-hq/element-web#22539.
* Add missing `type` property on `IAuthData` ([\matrix-org#2463](matrix-org#2463)).
* Clearly indicate that `lastReply` on a Thread can return falsy ([\matrix-org#2462](matrix-org#2462)).
* Fix issues with getEventTimeline and thread roots ([\matrix-org#2444](matrix-org#2444)). Fixes element-hq/element-web#21613.
* Live location sharing - monitor liveness of beacons yet to start ([\matrix-org#2437](matrix-org#2437)). Contributed by @kerryarchibald.
* Refactor Relations to not be per-EventTimelineSet ([\matrix-org#2412](matrix-org#2412)). Fixes matrix-org#2399 and element-hq/element-web#22298.
* Add tests for sendEvent threadId handling ([\matrix-org#2435](matrix-org#2435)). Fixes element-hq/element-web#22433.
* Make sure `encryptAndSendKeysToDevices` assumes devices are unique per-user. ([\matrix-org#2136](matrix-org#2136)). Fixes matrix-org#2135. Contributed by @ShadowJonathan.
* Don't bug the user while re-checking key backups after decryption failures ([\matrix-org#2430](matrix-org#2430)). Fixes element-hq/element-web#22416. Contributed by @duxovni.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-Enhancement Z-Community-PR Issue is solved by a community member's PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants