matrix-js-sdk
matrix-js-sdk copied to clipboard
matrix-org
utils: Fix bug in deepCompare which would incorrectly return objects with disjoint keys as equal
Checklist
- [x] Tests written for new code (and old code if feasible)
- [x] Linter and other CI checks pass
- [x] Sign-off given on the changes (see CONTRIBUTING.md)
Type: Defect
Here's what your changelog entry will look like:
🐛 Bug Fixes
- utils: Fix bug in deepCompare which would incorrectly return objects with disjoint keys as equal (#2586). Contributed by @3nprob.
This function is used in these places in this repo. While it's involved in cryptographic key management I don't immediately see how this can become an actual security issue.
https://github.com/matrix-org/matrix-js-sdk/blob/9eb72908a73fc17af3f5c78845affa3c6865cc66/src/crypto/store/memory-crypto-store.ts#L149
https://github.com/matrix-org/matrix-js-sdk/blob/9eb72908a73fc17af3f5c78845affa3c6865cc66/src/crypto/store/indexeddb-crypto-store-backend.ts#L171
https://github.com/matrix-org/matrix-js-sdk/blob/9eb72908a73fc17af3f5c78845affa3c6865cc66/src/client.ts#L6265
@turt2live @richvdh (Since I see you as the users of this function when looking at the blame)
hey @3nprob - thanks for the PR. Would it be possible to get a de-anonymized signoff on your PRs, or a form filled out with [email protected] (email) please? Thanks
hey @3nprob - thanks for the PR. Would it be possible to get a de-anonymized signoff on your PRs, or a form filled out with [email protected] (email) please? Thanks
I'd rather stay pseudonymous for the time being. Invited you for DM on Matrix just now. I could take a look at the form - e-mail in the signoff should work.
If you've invited me to a DM, I did not get it 😅 - I'm @travis:t2l.io on Matrix
So we did have a brief chat just now.
Unfortunately I had missed this before starting work: https://github.com/matrix-org/matrix-js-sdk/blob/develop/CONTRIBUTING.md#sign-off
We accept contributions under a legally identifiable name, such as your name on government documentation or common-law names (names claimed by legitimate usage or repute). Unfortunately, we cannot accept anonymous contributions at this time.
I submit and make my patches available under the Apache License and have agreed to the DCO, which I have signed off on. Element is free to use these patches under the license, all I ask is that the license terms including attribution are followed.
However, requiring contributors to doxx themselves (in a way legally recognized in the UK, I assume here) is not something I find comfortable with for a project such as this (which is kind of OT here; just explaining my rationale for any onlookers wondering why this is hanging).
not sure I see the actual code change which matches the title here - it looks like a general refactoring rather than a bug fix?
The existing code is asymmetric and checks one direction twice and never the other. Check out and run just the added regression cases and you should see them fail.
For anyone wondering what the status is re the sign-off conversation above: Got put in touch with someone else from the Matrix Foundation and they came up with something that was acceptable so this PR should be good to go and they told me that this is now unblocked in that regard as of Wednesday last week.
Also just rebased on current develop.
@3nprob my PR was related to the test failure, but not due to the code being buggy, but the test making the wrong assertions
@3nprob my PR was related to the test failure, but not due to the code being buggy, but the test making the wrong assertions
Or more precisely: The bug existed before and after your PR but some assertions were missing in the tests,making it go undetected. This PR added those assertions.
Just curious; Why did e808e0ff18922030c74d5ead33d9e7fda3f8f600 become part of this PR and not opened as its own? Looks like a completely separate issue and only tangentially related through the tests.
The resulting commit e87ce873b028c508b3217a9c511ecf65ba05c337 makes it look like a single change.
EDIT: Realized I prematurely deleted the branch on GH before fetching it... web archive mirror is not pretty but should work to distinguish in case the GH links stop.
@3nprob it became part of this PR to get your tests passing, we squash merge by policy hence it becoming a single commit