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

chore: Forbid remote users from joining local public rooms #63

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jason-famedly
Copy link
Contributor

This is an experiment, and while it appears to work it does need more extensive auditing of the other code paths used by this third party API callback. I chose to use the one that was associated with on_make_join_request() which should be early enough in the join process cycle to avoid leaking information and forbid joining rooms

requires #59

@jason-famedly jason-famedly force-pushed the jason/exp-forbid-make-join branch 2 times, most recently from 82f1c94 to c85c9f2 Compare March 12, 2025 15:00
@jason-famedly jason-famedly force-pushed the jason/block-out-invites-to-local-pub-room branch 3 times, most recently from d26c8b7 to 409764c Compare March 13, 2025 17:16
Base automatically changed from jason/block-out-invites-to-local-pub-room to main March 13, 2025 17:37
@jason-famedly jason-famedly force-pushed the jason/exp-forbid-make-join branch from c85c9f2 to 6108427 Compare March 13, 2025 17:47
Copy link

codecov bot commented Mar 13, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.96%. Comparing base (409764c) to head (6108427).

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #63      +/-   ##
==========================================
+ Coverage   97.94%   97.96%   +0.02%     
==========================================
  Files           9        9              
  Lines         778      786       +8     
  Branches      109      112       +3     
==========================================
+ Hits          762      770       +8     
  Misses         16       16              
Files with missing lines Coverage Δ
synapse_invite_checker/invite_checker.py 96.99% <100.00%> (+0.05%) ⬆️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 409764c...6108427. Read the comment docs.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jason-famedly
Copy link
Contributor Author

jason-famedly commented Mar 13, 2025

check_event_allowed auditing(in context of EventCreationHandler.create_new_client_event() as the other two uses are knocking related and therefore irrelevant):
handlers/federation.py(6 results):

  • on_make_join_request(): line 915 <- this one is where we use it
  • on_make_leave_request(): line 1188
  • on_make_knock_request(): line 1234 (this one is dumb. it calls both create_new_client_event() and then check_event_allowed() right after)
  • exchange_third_party_invite(): line 1436
  • on_exchange_third_party_invite_request(): line 1520
  • add_display_name_to_third_party_invite(): line 1586

handlers/message.py(1(real) result, other is definition):

  • create_event(): line 565
    • from message.py:
      • _create_and_send_nonmember_event_locked(): line 1041
      • _send_dummy_event_for_room(): line 2026
    • from room.py
      • RoomCreationHandler.upgrade_room(): line 185
      • in a subroutine as part of sending events into a cloned room, these should be ok to be ignored(check if an old room existed that would have been denied now is a thing?)
    • from room_member.py
      • RoomMemberHandler._local_membership_update(): line 385 <- this one looks interesting
      • RoomMemberMasterHandler._generate_local_out_of_band_leave(): line 2033

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.

1 participant