fix: handling non-public room join #290

debdutdeb · 2025-10-27T12:37:06Z

Summary by CodeRabbit

Bug Fixes
- Improved room invite processing with enhanced validation to prevent errors from missing room state data.
- Refined room privacy detection for more accurate invite handling.

coderabbitai · 2025-10-27T12:37:24Z

Walkthrough

This PR modifies the invite service to improve room privacy detection and add input validation. It changes from scanning all state events for a public join rule to locating the first m.room.join_rules event specifically, and adds validation to ensure strippedStateEvents is not empty before processing invites.

Changes

Cohort / File(s)	Summary
Room privacy detection and invite validation `packages/federation-sdk/src/services/invite.service.ts`	Refactored room privacy check to use m.room.join_rules event instead of scanning all stripped state events; added input validation throwing NotAllowedError when strippedStateEvents is empty; added documentation and SPEC references

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

The changes are focused in a single file with clear logic improvements
Room privacy detection refactoring is straightforward (find first join_rules event vs. scan all events)
Input validation addition is a simple guard clause with error handling
No complex cross-file interactions or architectural changes

Possibly related PRs

fix(federation): allow to join non-private or encrypted rooms based on settings #280: Introduces/uses NotAllowedError in the same file for invite processing validation
fix: receive invite_room_state as invite param #286: Directly related—also modifies invite.service.ts to handle room privacy detection from strippedStateEvents
refactor: standardize event creation in RoomService and PersistentEve… #188: Related—modifies the same service file and affects the invite processing flow

Suggested reviewers

ggazzo
sampaiodiego

Poem

🐰 A rabbit hops through privacy gates,
Join rules now found, no more delays,
Empty states caught with gentle care,
Invites flow free through Federation's air!
🌟

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The title "fix: handling non-public room join" is partially related to the changeset. It clearly refers to a real and significant aspect of the changes—the reworked room privacy check logic that now properly handles the m.room.join_rules event when determining room privacy. The title is concise, uses clear language, and avoids vague terms. However, the title doesn't capture the full scope of changes, as it omits mention of the input validation that was added to the processInvite function. Despite being focused on only one of the changes, the title does adequately represent the primary change in the pull request.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch fix-private-room-join-thing

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

codecov-commenter · 2025-10-27T12:38:36Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 60.49%. Comparing base (e131461) to head (5b5540b).

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #290   +/-   ##
=======================================
  Coverage   60.49%   60.49%           
=======================================
  Files          67       67           
  Lines        6675     6675           
=======================================
  Hits         4038     4038           
  Misses       2637     2637

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

coderabbitai

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between e131461 and 5b5540b.

📒 Files selected for processing (1)

packages/federation-sdk/src/services/invite.service.ts (2 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

packages/federation-sdk/src/services/invite.service.ts (1)

packages/federation-sdk/src/index.ts (1)

NotAllowedError (93-93)

🔇 Additional comments (1)

packages/federation-sdk/src/services/invite.service.ts (1)

203-209: No changes needed; validation is spec-compliant.

The Matrix specification distinguishes between the invite_room_state field being optional (can be omitted entirely) and the array being empty when present. Per the v1.16 specification, if stripped state events are provided, they must include valid events—notably, m.room.create is required in invite contexts. Throwing an error when the array is empty is correct behavior and prevents malformed invites. The comment accurately references the spec, and the validation appropriately guards against empty state.

coderabbitai · 2025-10-27T12:40:53Z

packages/federation-sdk/src/services/invite.service.ts

+		const joinRuleEvent = strippedStateEvents.find(
+			(stateEvent) => stateEvent.type === 'm.room.join_rules',
 		);
+		const isRoomNonPrivate = joinRuleEvent
+			? joinRuleEvent.content.join_rule === 'public'
+			: true /* default, if no event */;


⚠️ Potential issue | 🟠 Major

Reconsider the default value for isRoomNonPrivate.

When no m.room.join_rules event is found, the code defaults isRoomNonPrivate to true, treating the room as public/non-private. This appears inconsistent with the Matrix specification, which typically defaults join_rules to "invite" (private) when not explicitly set. Defaulting to true here could bypass privacy restrictions configured via allowedNonPrivateRooms, potentially allowing invites to private rooms that should be rejected.

Consider this safer default:

const joinRuleEvent = strippedStateEvents.find( (stateEvent) => stateEvent.type === 'm.room.join_rules', ); const isRoomNonPrivate = joinRuleEvent ? joinRuleEvent.content.join_rule === 'public' - : true /* default, if no event */; + : false /* default to private/invite per Matrix spec */;

🤖 Prompt for AI Agents

In packages/federation-sdk/src/services/invite.service.ts around lines 163 to 168, the code currently treats a missing m.room.join_rules event as public (isRoomNonPrivate = true); change this to treat a missing join_rules as private (default to invite) by setting the default to false so that absence of the event is handled as "invite"/private; update the ternary (or conditional) and any accompanying comment to reflect the safer default and ensure downstream checks (e.g., allowedNonPrivateRooms) continue to behave correctly.

@debdutdeb what do you think about this one?

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

https://github.com/element-hq/synapse/blob/f56670515bc402e13ee0bb2dd99ceb6e2ea8ba7d/synapse/event_auth.py#L565

sampaiodiego · 2025-10-27T19:05:06Z

packages/federation-sdk/src/services/invite.service.ts

 	) {
+		// SPEC:  An optional list of stripped state events to help the receiver of the invite identify the room.
+		// https://spec.matrix.org/v1.12/server-server-api/#put_matrixfederationv1inviteroomideventid
+		if (strippedStateEvents.length === 0) {


should we validate this only if we have configured restrictions? like if we don't restrict anything we won't use the stripped state anyways

sampaiodiego · 2025-11-05T12:28:45Z

not required for now

debdutdeb · 2025-11-05T12:29:17Z

https://github.com/element-hq/synapse/blob/f56670515bc402e13ee0bb2dd99ceb6e2ea8ba7d/synapse/event_auth.py#L565

existing check covers the case sionce no join_rule === non public.

this pr shouldn't be needed anymore.

debdutdeb added 2 commits October 27, 2025 18:02

fix: disallow invite if no state to begin with

4d0e2ff

fix: handle missing join rule event

5b5540b

coderabbitai bot reviewed Oct 27, 2025

View reviewed changes

sampaiodiego reviewed Oct 27, 2025

View reviewed changes

sampaiodiego closed this Nov 5, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: handling non-public room join #290

fix: handling non-public room join #290

Uh oh!

debdutdeb commented Oct 27, 2025 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Oct 27, 2025 •

edited

Loading

Uh oh!

codecov-commenter commented Oct 27, 2025

Uh oh!

coderabbitai bot left a comment

Uh oh!

coderabbitai bot Oct 27, 2025 •

edited

Loading

Uh oh!

ggazzo Oct 30, 2025

Uh oh!

coderabbitai bot Oct 30, 2025

Uh oh!

debdutdeb Nov 5, 2025

Uh oh!

sampaiodiego Oct 27, 2025

Uh oh!

sampaiodiego commented Nov 5, 2025

Uh oh!

debdutdeb commented Nov 5, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

fix: handling non-public room join #290

fix: handling non-public room join #290

Uh oh!

Conversation

debdutdeb commented Oct 27, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Oct 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Suggested reviewers

Poem

Pre-merge checks and finishing touches

Uh oh!

codecov-commenter commented Oct 27, 2025

Codecov Report

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot Oct 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ggazzo Oct 30, 2025

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot Oct 30, 2025

Choose a reason for hiding this comment

Uh oh!

debdutdeb Nov 5, 2025

Choose a reason for hiding this comment

Uh oh!

sampaiodiego Oct 27, 2025

Choose a reason for hiding this comment

Uh oh!

sampaiodiego commented Nov 5, 2025

Uh oh!

debdutdeb commented Nov 5, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

debdutdeb commented Oct 27, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Oct 27, 2025 •

edited

Loading

coderabbitai bot Oct 27, 2025 •

edited

Loading