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

refactor: Observers/ignored peers can now send and receive custom packets #2728

Merged
merged 1 commit into from
Mar 7, 2024

Conversation

JFreegman
Copy link
Member

@JFreegman JFreegman commented Mar 6, 2024

The Observer role was intended to prevent peers from being disruptive and/or interacting with other peers in the group. It wasn't intended to cripple custom protocols running on-top of the groups such as file sharing and message syncing.

In cases where it might be undesirable for observers to use custom packets (e.g. starting a file transfer) the client will still have the ability to decide whether or not to allow it.


This change is Reviewable

@JFreegman JFreegman added the refactor Refactoring production code, eg. renaming a variable, not affecting semantics label Mar 6, 2024
@JFreegman JFreegman added this to the v0.2.19 milestone Mar 6, 2024
Copy link

codecov bot commented Mar 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.05%. Comparing base (b3c3c49) to head (99e0bcc).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2728      +/-   ##
==========================================
- Coverage   73.11%   73.05%   -0.06%     
==========================================
  Files         149      149              
  Lines       30517    30499      -18     
==========================================
- Hits        22313    22282      -31     
- Misses       8204     8217      +13     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@JFreegman JFreegman force-pushed the group_custom_packet_observers branch from c6e2920 to 993d192 Compare March 6, 2024 17:59
@zoff99
Copy link

zoff99 commented Mar 6, 2024

this allows a peer thats is muted (observer) to send packets. i don't think this is a good thing. spam blocking on the receiver side is worse than blocking on the sender side.
i do not like this.

@JFreegman
Copy link
Member Author

JFreegman commented Mar 6, 2024

this allows a peer thats is muted (observer) to send packets. i don't think this is a good thing. spam blocking on the receiver side is worse than blocking on the sender side. i do not like this.

Nothing stops a malicious peer from modifying their source code and spamming illegitimate custom packets to a group, regardless of their permissions (apart from removing them from the group). The only thing this PR changes in that respect is that the spammer no longer needs to remove those client-side checks themselves. However, in order to spam illegitimate custom packets, they will still need to modify the source code, and removing the client-side checks at the same time is trivial.

@JFreegman JFreegman marked this pull request as ready for review March 7, 2024 13:03
…kets

The Observer role was intended to prevent peers from being
disruptive and/or interacting with other peers in the group.
It wasn't intended to cripple custom protocols running on-top
of the groups such as file sharing and message syncing.

In cases where it might be undesirable for observers to use
custom packets (e.g. starting a file transfer) the client will
have the ability to decide whether or not to allow it.
@JFreegman JFreegman force-pushed the group_custom_packet_observers branch from 993d192 to 99e0bcc Compare March 7, 2024 13:22
@JFreegman JFreegman merged commit 99e0bcc into TokTok:master Mar 7, 2024
62 of 63 checks passed
@JFreegman JFreegman deleted the group_custom_packet_observers branch March 7, 2024 14:09
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Refactoring production code, eg. renaming a variable, not affecting semantics
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants