utils: Fix bug in deepCompare which would incorrectly return objects with disjoint keys as equal #2586

3nprob · 2022-08-11T10:53:53Z

Checklist

Tests written for new code (and old code if feasible)
Linter and other CI checks pass
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.

spec/unit/utils.spec.ts

3nprob · 2022-08-11T10:56:05Z

Signed-off-by: 3nprob <[email protected]>

3nprob · 2022-08-11T11:08:06Z

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.

matrix-js-sdk/src/crypto/store/memory-crypto-store.ts

Line 149 in 9eb7290

if (utils.deepCompare(existing.requestBody, requestBody)) {

matrix-js-sdk/src/crypto/store/indexeddb-crypto-store-backend.ts

Line 171 in 9eb7290

if (utils.deepCompare(existing.requestBody, requestBody)) {

matrix-js-sdk/src/client.ts

Line 6265 in 9eb7290

if (utils.deepCompare(oldDef, newDef)) {

3nprob · 2022-08-11T11:09:52Z

@turt2live @richvdh (Since I see you as the users of this function when looking at the blame)

turt2live · 2022-08-11T14:43:28Z

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

3nprob · 2022-08-11T14:53:32Z

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.

turt2live · 2022-08-11T16:15:33Z

If you've invited me to a DM, I did not get it 😅 - I'm @travis:t2l.io on Matrix

3nprob · 2022-08-11T22:20:30Z

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).

turt2live

not sure I see the actual code change which matches the title here - it looks like a general refactoring rather than a bug fix?

3nprob · 2022-08-24T04:02:37Z

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.

3nprob · 2022-08-28T22:47:21Z

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.

…s by target (#2623)

…with disjoint keys as equal

3nprob · 2022-08-30T11:03:24Z

Rebased on develop

This test wrongly asserted that `initialSyncLimit` would be used to make a filter It is used only for the initial sync inline filter, and not in POST /filter

t3chguy · 2022-09-01T15:31:15Z

@3nprob my PR was related to the test failure, but not due to the code being buggy, but the test making the wrong assertions

turt2live

Thanks! Apologies it took so long for this one to go through: it managed to hit just about every internal process unexpectedly :)

turt2live · 2022-09-01T21:26:30Z

src/utils.ts

            if (y.hasOwnProperty(p) !== x.hasOwnProperty(p)) {
                return false;
            }
        }

        // finally, compare each of x's keys with y
-        for (p in y) { // eslint-disable-line guard-for-in


right, it was pointed out to me that this is the bug I couldn't see: y is meant to be x.

3nprob · 2022-09-02T01:10:40Z

@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.

3nprob · 2022-09-02T01:18:34Z

Just curious; Why did e808e0f 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 e87ce87 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.

t3chguy · 2022-09-02T08:11:46Z

@3nprob it became part of this PR to get your tests passing, we squash merge by policy hence it becoming a single commit

@3nprob

* Fix bug in deepCompare which would incorrectly return objects with disjoint keys as equal ([\matrix-org#2586](matrix-org#2586)). Contributed by @3nprob. * Refactor Sync and fix `initialSyncLimit` ([\matrix-org#2587](matrix-org#2587)). * Use deep equality comparisons when searching for outgoing key requests by target ([\matrix-org#2623](matrix-org#2623)). Contributed by @duxovni. * Fix room membership race with PREPARED event ([\matrix-org#2613](matrix-org#2613)). Contributed by @jotto.

github-actions bot added the Z-Community-PR Issue is solved by a community member's PR label Aug 11, 2022

3nprob commented Aug 11, 2022

View reviewed changes

spec/unit/utils.spec.ts Show resolved Hide resolved

3nprob force-pushed the fix-deepcompare branch from 32f0f96 to d4115f3 Compare August 11, 2022 10:57

github-actions bot added the T-Defect label Aug 11, 2022

3nprob marked this pull request as ready for review August 11, 2022 11:01

3nprob requested a review from a team as a code owner August 11, 2022 11:01

3nprob requested review from t3chguy, kerryarchibald and justjanne August 11, 2022 11:01

turt2live reviewed Aug 24, 2022

View reviewed changes

turt2live removed request for t3chguy, kerryarchibald and justjanne August 24, 2022 03:26

turt2live added the X-Blocked label Aug 24, 2022

3nprob requested a review from turt2live August 28, 2022 22:47

3nprob force-pushed the fix-deepcompare branch 2 times, most recently from feb02a1 to 1f17973 Compare August 28, 2022 22:55

3nprob referenced this pull request Aug 28, 2022

Use deep equality comparisons when searching for outgoing key request…

1c77816

…s by target (#2623)

This was referenced Aug 28, 2022

Refactor Sync and fix initialSyncLimit #2587

Merged

"Ignoring duplicate event" is logged each time the sdk sends an event #2618

Closed

utils: Fix bug in deepCompare which would incorrectly return objects …

48cce65

…with disjoint keys as equal

3nprob force-pushed the fix-deepcompare branch from 1f17973 to 48cce65 Compare August 30, 2022 11:03

t3chguy added 2 commits September 1, 2022 16:05

Merge branch 'develop' into fix-deepcompare

faa8186

Fix bugs in sync test

e808e0f

This test wrongly asserted that `initialSyncLimit` would be used to make a filter It is used only for the initial sync inline filter, and not in POST /filter

turt2live approved these changes Sep 1, 2022

View reviewed changes

turt2live removed the X-Blocked label Sep 1, 2022

turt2live enabled auto-merge (squash) September 1, 2022 21:35

turt2live merged commit e87ce87 into matrix-org:develop Sep 1, 2022

3nprob deleted the fix-deepcompare branch September 2, 2022 01:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

utils: Fix bug in deepCompare which would incorrectly return objects with disjoint keys as equal #2586

utils: Fix bug in deepCompare which would incorrectly return objects with disjoint keys as equal #2586

3nprob commented Aug 11, 2022 •

edited

Loading

3nprob commented Aug 11, 2022

3nprob commented Aug 11, 2022

3nprob commented Aug 11, 2022

turt2live commented Aug 11, 2022

3nprob commented Aug 11, 2022 •

edited

Loading

turt2live commented Aug 11, 2022

3nprob commented Aug 11, 2022 •

edited

Loading

turt2live left a comment

3nprob commented Aug 24, 2022 •

edited

Loading

3nprob commented Aug 28, 2022 •

edited

Loading

3nprob commented Aug 30, 2022

t3chguy commented Sep 1, 2022

turt2live left a comment

turt2live Sep 1, 2022

3nprob commented Sep 2, 2022 •

edited

Loading

3nprob commented Sep 2, 2022 •

edited

Loading

t3chguy commented Sep 2, 2022

utils: Fix bug in deepCompare which would incorrectly return objects with disjoint keys as equal #2586

utils: Fix bug in deepCompare which would incorrectly return objects with disjoint keys as equal #2586

Conversation

3nprob commented Aug 11, 2022 • edited Loading

Checklist

🐛 Bug Fixes

3nprob commented Aug 11, 2022

3nprob commented Aug 11, 2022

3nprob commented Aug 11, 2022

turt2live commented Aug 11, 2022

3nprob commented Aug 11, 2022 • edited Loading

turt2live commented Aug 11, 2022

3nprob commented Aug 11, 2022 • edited Loading

turt2live left a comment

Choose a reason for hiding this comment

3nprob commented Aug 24, 2022 • edited Loading

3nprob commented Aug 28, 2022 • edited Loading

3nprob commented Aug 30, 2022

t3chguy commented Sep 1, 2022

turt2live left a comment

Choose a reason for hiding this comment

turt2live Sep 1, 2022

Choose a reason for hiding this comment

3nprob commented Sep 2, 2022 • edited Loading

3nprob commented Sep 2, 2022 • edited Loading

t3chguy commented Sep 2, 2022

3nprob commented Aug 11, 2022 •

edited

Loading

3nprob commented Aug 11, 2022 •

edited

Loading

3nprob commented Aug 11, 2022 •

edited

Loading

3nprob commented Aug 24, 2022 •

edited

Loading

3nprob commented Aug 28, 2022 •

edited

Loading

3nprob commented Sep 2, 2022 •

edited

Loading

3nprob commented Sep 2, 2022 •

edited

Loading