Skip to content

Conversation

iequidoo
Copy link
Collaborator

@iequidoo iequidoo commented Aug 29, 2025

No description provided.

@iequidoo iequidoo requested a review from Hocuri August 29, 2025 15:37
@iequidoo iequidoo changed the base branch from main to iequidoo/sync_create_group August 29, 2025 15:38
@iequidoo

This comment was marked as resolved.

@iequidoo iequidoo requested a review from link2xt August 29, 2025 15:42
@iequidoo iequidoo force-pushed the iequidoo/dont-reverify-by-self branch 2 times, most recently from 39d5c5c to 19b5126 Compare August 29, 2025 18:32
@iequidoo
Copy link
Collaborator Author

I've checked that logically this doesn't conflict with #7116, so can be merged independently. Not adding tests for the two last fixes yet because there may be other ideas on how to solve the problems described.

@iequidoo iequidoo mentioned this pull request Aug 31, 2025
11 tasks
@link2xt
Copy link
Collaborator

link2xt commented Aug 31, 2025

I've checked that logically this doesn't conflict with #7116, so can be merged independently.

It needs to be based on top of main for this. I also commented in #7116 (comment)

src/contact.rs Outdated
if verifier_verifier_id == verifier_id {
// Avoid introducing incorrect reverse chains: if the verifier itself has an
// unknown verifier, it may be `contact_id` actually (directly or indirectly) on
// the other device.
Copy link
Collaborator

Choose a reason for hiding this comment

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

You don't even need the other device for this except for getting "verified by unknown contact" in the first place. If you have Alice and Bob verified by unknown contact, then they start chatting in some group and claim that each other is verified (via Chat-Verified currently, later with _verified attribute of Autocrypt-Gossip), you will get a loop with Alice and Bob having each other as verifier.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You don't even need the other device for this except for getting "verified by unknown contact" in the first place.

This is what i meant exactly. Added this to the comment to make it more clear.

As for "verifier loops", they're already not possible before this commit. Only "reverse chains" are possible on different devices.

Copy link
Collaborator

@link2xt link2xt left a comment

Choose a reason for hiding this comment

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

All commits look good, but this should be merged into main instead of #7009
I'll rebase #7116 on top of main then.

…ther device

Also verify not yet verified contacts w/o setting a verifier for them (in the db it's stored as
`verifier_id=id` though) because we don't know who verified them for another device.
If this happens, mark the contact as verified by an unknown contact instead. This avoids introducing
incorrect reverse chains: if the verifier itself has an unknown verifier, it may be `contact_id`
actually (directly or indirectly) on the other device (which is needed for getting "verified by
unknown contact" in the first place).
@iequidoo iequidoo force-pushed the iequidoo/dont-reverify-by-self branch from d52fd61 to 6ea63c7 Compare September 1, 2025 07:46
@iequidoo iequidoo changed the base branch from iequidoo/sync_create_group to main September 1, 2025 07:46
…verifier

Now that the previous commit avoids creating incorrect reverse verification chains, we can do
this. Sure, existing users' dbs aready have verification chains ending with "unknown" roots, but at
least for new users updating `verifier_id` to a known verifier makes sense.
@iequidoo iequidoo force-pushed the iequidoo/dont-reverify-by-self branch from 6ea63c7 to c10965c Compare September 1, 2025 07:53
@iequidoo iequidoo merged commit e6ab1e3 into main Sep 1, 2025
29 checks passed
@iequidoo iequidoo deleted the iequidoo/dont-reverify-by-self branch September 1, 2025 08:09
@iequidoo
Copy link
Collaborator Author

iequidoo commented Sep 1, 2025

Not adding tests for the two last fixes yet because there may be other ideas on how to solve the problems described.

Going to do this in a follow-up PR.

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.

2 participants