[lib] Tighten utils type to contain only thick threads
ClosedPublic
Actions

Authored by tomek on Thu, Oct 31, 7:59 AM.

Details

Reviewers

kamil
angelika

Commits

rCOMM580b6451cbab: [lib] Tighten utils type to contain only thick threads

Summary

It is inconvenient to have the assertions in a lot of places - the issue would become a lot worse after the rest of the stack.

This diff introduces a new risk - an attacker can create a new thread with the same ID as an existing thin thread. Not sure how useful this attack could be. Protecting against it is handled in https://linear.app/comm/issue/ENG-9468/introduce-additional-validation-of-operations.

https://linear.app/comm/issue/ENG-9823/fix-sending-notifs-about-leaving-a-thick-thread

Depends on D13698

Test Plan

Flow

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

tomek created this revision.Thu, Oct 31, 7:59 AM

Herald added a subscriber: ashoat. · View Herald TranscriptThu, Oct 31, 7:59 AM

tomek added a child revision: D13843: [lib] Merge generating notifs data with processing operations.Thu, Oct 31, 8:03 AM

Harbormaster completed remote builds in B32476: Diff 45509.Thu, Oct 31, 8:17 AM

tomek requested review of this revision.Thu, Oct 31, 8:17 AM

This diff introduces a new risk - an attacker can create a new thread with the same ID as an existing thin thread. Not sure how useful this attack could be. Protecting against it is handled in https://linear.app/comm/issue/ENG-9468/introduce-additional-validation-of-operations.

I'm a bit skeptical about introducing this regression. The task you linked is unlikely to be prioritized anytime soon.

In D13842#386643, @ashoat wrote:

This diff introduces a new risk - an attacker can create a new thread with the same ID as an existing thin thread. Not sure how useful this attack could be. Protecting against it is handled in https://linear.app/comm/issue/ENG-9468/introduce-additional-validation-of-operations.

I'm a bit skeptical about introducing this regression. The task you linked is unlikely to be prioritized anytime soon.

Introducing protection against this is a really quick fix (30 min?) to implement, and I think we can create a subtask out of that task and prioritize it.

Makes sense to me!

Created https://linear.app/comm/issue/ENG-9826/validate-the-ids-from-the-dm-operations to track the validation and assigned it to the cycle.

kamil accepted this revision.Tue, Nov 5, 1:44 AM

This revision is now accepted and ready to land.Tue, Nov 5, 1:44 AM

Closed by commit rCOMM580b6451cbab: [lib] Tighten utils type to contain only thick threads (authored by tomek). · Explain WhyFri, Nov 8, 4:29 AM

This revision was automatically updated to reflect the committed changes.

tomek added a commit: rCOMM580b6451cbab: [lib] Tighten utils type to contain only thick threads.