refactor: state resolution error propagation #229
Conversation
WalkthroughAligns rejection handling across services to use rejectCode/rejectReason/rejectedBy on PersistentEventBase. Introduces structured authorization errors (RejectCodes, StateResolverAuthorizationError), updates room authorization flow to throw/catch them, and propagates rejectReason usage throughout federation SDK and homeserver. Adds toStrippedJson(), adjusts factory RoomVersion typing, and re-exports errors. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Service as Federation/Room Services
participant Rules as Authorization Rules
participant StateRes as State Resolver
participant Event as PersistentEventBase
Client->>Service: Submit event (join/message/invite)
Service->>Rules: checkEventAuth*(event, authState)
alt Auth passes
Rules-->>Service: ok
Service->>StateRes: iterativeAuthChecks(event,...)
StateRes-->>Service: ok
else Auth fails
Rules-->>Service: throw StateResolverAuthorizationError(code, {rejectedEvent, reason, rejectedBy?})
Service->>StateRes: iterativeAuthChecks(event,...)
StateRes->>Event: reject(code, reason, rejectedBy)
StateRes-->>Service: continue processing
Service->>Client: Error(event.rejectReason)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #229 +/- ##
==========================================
- Coverage 81.74% 81.39% -0.36%
==========================================
Files 63 63
Lines 4695 4773 +78
==========================================
+ Hits 3838 3885 +47
- Misses 857 888 +31 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/room/src/authorizartion-rules/rules.ts (1)
102-111: Check for membership property correctly.The spec requires rejecting when the membership property is missing. The current check uses isMembershipEvent() instead of verifying content.membership presence, which can lead to a generic “unknown membership state undefined” later.
Apply this diff:
- if ( - !membershipEventToCheck.stateKey || - !membershipEventToCheck.isMembershipEvent() - ) { + if ( + !membershipEventToCheck.stateKey || + !('membership' in membershipEventToCheck.getContent()) + ) { throw new StateResolverAuthorizationError(RejectCodes.AuthError, { rejectedEvent: membershipEventToCheck, reason: 'm.room.member event has no state_key or membership property', }); }packages/room/src/manager/event-wrapper.ts (1)
1-6: Convert these toimport type
• packages/federation-sdk/src/services/state.service.ts:21 –import { RejectCodes } from '@rocket.chat/federation-room';
• packages/room/src/authorizartion-rules/rules.ts:12 –import { RejectCodes, StateResolverAuthorizationError } from './errors';
• packages/room/src/manager/event-wrapper.ts:6 –import { RejectCode } from '../authorizartion-rules/errors';
• packages/room/src/state_resolution/definitions/definitions.ts:8 –import { PersistentEventBase } from '../../manager/event-wrapper';
• packages/room/src/state_resolution/definitions/algorithm/v2.ts:61 –import { PersistentEventBase } from '../../../manager/event-wrapper';
• packages/room/src/authorizartion-rules/rules.spec.ts:2 –import { PersistentEventBase } from '../manager/event-wrapper';
• packages/room/src/authorizartion-rules/errors.ts:1 –import { PersistentEventBase } from '../manager/event-wrapper';
• packages/room/src/manager/power-level-event-wrapper.ts:6 –import { PersistentEventBase } from './event-wrapper';
🧹 Nitpick comments (12)
packages/federation-sdk/src/services/room.service.ts (1)
985-987: Use HttpException for rejected remote joins; prefer checking persisted event
- Use HttpException with a fallback message.
- Note: You’re checking
joinEventFinal.rejected(the local instance) after callingprocessIncomingPDUs. Consider verifying the persisted event’s rejection instead to avoid stale status.Apply:
- if (joinEventFinal.rejected) { - throw new Error(joinEventFinal.rejectReason); - } + if (joinEventFinal.rejected) { + throw new HttpException( + joinEventFinal.rejectReason || 'Join rejected', + HttpStatus.FORBIDDEN, + ); + }Optional follow-up: fetch the stored event by ID after processing and check its rejection fields before throwing.
packages/federation-sdk/src/services/send-join.service.ts (1)
53-54: Add a fallback message for safety
rejectReasoncan be empty/undefined. Provide a default.Apply:
- throw new Error(joinEvent.rejectReason); + throw new Error(joinEvent.rejectReason || 'Join rejected');packages/homeserver/src/controllers/internal/invite.controller.ts (1)
63-65: Prefer a safe fallback and consider returning a 400 response
- Add a default message for
rejectReason.- Optional: Instead of throwing, return an
ErrorResponse(400) to match the declared route responses.Apply minimal safety:
- throw new Error(membershipEvent.rejectReason); + throw new Error(membershipEvent.rejectReason || 'Invite rejected');Optional route-shape refactor (if you want 400 instead of exception bubbling):
- if (membershipEvent.rejected) { - throw new Error(membershipEvent.rejectReason || 'Invite rejected'); - } + if (membershipEvent.rejected) { + return { + error: membershipEvent.rejectReason || 'Invite rejected', + details: {}, + }; + }packages/room/src/manager/power-level-event-wrapper.ts (1)
71-74: LGTM; optionally add explicit return typeSignature tightening is good. Consider annotating the return type for clarity.
Apply:
- getPowerLevelForUser( + getPowerLevelForUser( userId: string, createEvent?: PersistentEventBase<RoomVersion, 'm.room.create'>, - ) { + ): number {packages/federation-sdk/src/services/message.service.ts (1)
85-86: Add a default message for thrown rejectionsProtect against undefined/empty
rejectReasonto avoid throwingError(undefined).Apply pattern:
- throw new Error(event.rejectReason); + throw new Error(event.rejectReason || 'Event rejected');Repeat at each occurrence within this file.
Also applies to: 133-134, 169-170, 225-226, 276-277
packages/room/src/authorizartion-rules/errors.ts (1)
26-29: Constructor payload uses event instance for rejectedBy, but class property stores EventID.This is fine, but it’s worth documenting in the JSDoc/tsdoc to prevent misuse. Alternatively, consider exposing rejectedEventId alongside rejectedBy to make the semantics explicit to consumers.
packages/room/src/manager/event-wrapper.ts (3)
48-53: Model rejectedBy as optional instead of sentinel empty string.Using '' as a sentinel for EventID is leaky. Prefer optional to express absence.
Apply this diff:
- public rejectCode = ''; - - public rejectReason = ''; - - public rejectedBy = '' as EventID; + public rejectCode = ''; + public rejectReason = ''; + public rejectedBy?: EventID;No further changes are required because reject() already guards assignment with a truthy check.
23-25: Duplicate extractDomain implementation (also in rules.ts).Consider centralizing in a shared util to avoid divergence.
439-447: toStrippedJson returns canonical JSON string; name suggests object.No issue, but a small rename to toCanonicalStrippedJson or a brief docstring can prevent confusion.
packages/room/src/authorizartion-rules/rules.ts (3)
6-6: Import RoomVersion as a type.Only used for typing; avoid runtime import.
Apply this diff:
-import { RoomVersion } from '../manager/type'; +import type { RoomVersion } from '../manager/type';
833-837: Remove console.warn in auth path.Logging here adds noise; the structured NotImplemented error suffices.
Apply this diff:
- console.warn('third_party_invite not implemented'); throw new StateResolverAuthorizationError(RejectCodes.NotImplemented, { rejectedEvent: event, reason: 'third_party_invite not implemented', });
387-695: Power level change validation is comprehensive.Comparisons use “greater than sender’s current level” across defaults, per-event, and per-user entries with accurate rejectedBy. Minor nit: truthy checks (if (value && value > level)) could be made explicit (value !== undefined && value > level) for clarity, but current logic is functionally fine for non-negative levels.
📜 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.
📒 Files selected for processing (18)
packages/federation-sdk/src/services/invite.service.ts(3 hunks)packages/federation-sdk/src/services/message.service.ts(5 hunks)packages/federation-sdk/src/services/room.service.ts(2 hunks)packages/federation-sdk/src/services/send-join.service.ts(1 hunks)packages/federation-sdk/src/services/state.service.ts(5 hunks)packages/homeserver/src/controllers/internal/invite.controller.ts(1 hunks)packages/room/src/authorizartion-rules/errors.ts(1 hunks)packages/room/src/authorizartion-rules/rules.ts(30 hunks)packages/room/src/index.ts(1 hunks)packages/room/src/manager/event-wrapper.ts(4 hunks)packages/room/src/manager/factory.ts(2 hunks)packages/room/src/manager/power-level-event-wrapper.ts(1 hunks)packages/room/src/manager/v11.ts(1 hunks)packages/room/src/manager/v3.ts(2 hunks)packages/room/src/manager/v6.ts(1 hunks)packages/room/src/manager/v8.ts(1 hunks)packages/room/src/manager/v9.ts(1 hunks)packages/room/src/state_resolution/definitions/definitions.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (12)
packages/room/src/manager/factory.ts (6)
packages/room/src/manager/v3.ts (1)
PersistentEventV3(12-79)packages/room/src/manager/event-wrapper.ts (1)
event(110-119)packages/room/src/manager/v6.ts (1)
PersistentEventV6(4-15)packages/room/src/manager/v8.ts (1)
PersistentEventV8(4-14)packages/room/src/manager/v9.ts (1)
PersistentEventV9(4-16)packages/room/src/manager/v11.ts (1)
PersistentEventV11(5-46)
packages/room/src/authorizartion-rules/errors.ts (1)
packages/room/src/types/_common.ts (1)
EventID(8-8)
packages/room/src/manager/v9.ts (2)
packages/room/src/types/v3-11.ts (1)
PduType(47-47)packages/room/src/manager/v8.ts (1)
PersistentEventV8(4-14)
packages/room/src/manager/power-level-event-wrapper.ts (2)
packages/federation-sdk/src/index.ts (1)
PersistentEventBase(21-21)packages/room/src/manager/type.ts (1)
RoomVersion(14-14)
packages/room/src/state_resolution/definitions/definitions.ts (3)
packages/room/src/manager/type.ts (1)
RoomVersion(14-14)packages/room/src/authorizartion-rules/errors.ts (1)
StateResolverAuthorizationError(48-48)packages/room/src/manager/event-wrapper.ts (1)
event(110-119)
packages/room/src/manager/v11.ts (2)
packages/room/src/types/v3-11.ts (1)
PduType(47-47)packages/room/src/manager/v9.ts (1)
PersistentEventV9(4-16)
packages/room/src/manager/v6.ts (2)
packages/room/src/types/v3-11.ts (1)
PduType(47-47)packages/room/src/manager/v3.ts (1)
PersistentEventV3(12-79)
packages/room/src/manager/event-wrapper.ts (4)
packages/room/src/manager/type.ts (1)
RoomVersion(14-14)packages/room/src/types/_common.ts (1)
EventID(8-8)packages/room/src/authorizartion-rules/errors.ts (1)
RejectCode(10-10)packages/core/src/utils/signJson.ts (1)
encodeCanonicalJson(175-195)
packages/room/src/manager/v3.ts (3)
packages/room/src/types/v3-11.ts (1)
PduType(47-47)packages/federation-sdk/src/index.ts (1)
PersistentEventBase(21-21)packages/room/src/manager/type.ts (1)
RoomVersion3To11(3-12)
packages/room/src/authorizartion-rules/rules.ts (3)
packages/room/src/manager/type.ts (1)
RoomVersion(14-14)packages/room/src/authorizartion-rules/errors.ts (2)
StateResolverAuthorizationError(48-48)RejectCodes(4-8)packages/room/src/manager/event-wrapper.ts (2)
event(110-119)stateKey(98-102)
packages/room/src/manager/v8.ts (2)
packages/room/src/types/v3-11.ts (1)
PduType(47-47)packages/room/src/manager/v6.ts (1)
PersistentEventV6(4-15)
packages/federation-sdk/src/services/state.service.ts (3)
packages/room/src/types/_common.ts (1)
EventID(8-8)packages/room/src/authorizartion-rules/errors.ts (1)
RejectCodes(4-8)packages/room/src/authorizartion-rules/rules.ts (1)
checkEventAuthWithState(774-886)
🔇 Additional comments (28)
packages/room/src/index.ts (1)
5-5: LGTM: public re-export of errorsRe-exporting errors at the barrel looks good and matches the new rejection model exposure needs.
packages/room/src/manager/v11.ts (1)
1-7: LGTM: generic parameterization on PersistentEventV11Aligns with V3/V6/V8/V9; improves typing across event handling.
packages/federation-sdk/src/services/invite.service.ts (3)
88-89: LGTM! Consistent error property naming.The change from
rejectedReasontorejectReasonaligns with the new rejection model used throughout the codebase.
159-160: LGTM! Error property naming consistency maintained.The update to use
rejectReasoninstead ofrejectedReasonis consistent with the refactoring pattern.
176-177: LGTM! Error handling standardized.The change maintains consistency with the new
rejectReasonproperty across all error handling paths.packages/room/src/manager/v9.ts (1)
1-6: LGTM! Generic type parameter properly propagated.The addition of the generic type parameter
Type extends PduType = PduTypeand its propagation to the base class enables more precise typing of PDU payloads across the event hierarchy.packages/room/src/manager/v6.ts (1)
1-6: LGTM! Type safety enhanced with generics.The introduction of the generic type parameter follows the established pattern and properly extends the base class with the type constraint.
packages/room/src/manager/v8.ts (1)
1-6: LGTM! Consistent generic typing pattern.The generic type parameter addition maintains consistency with the refactoring across all version managers.
packages/federation-sdk/src/services/state.service.ts (7)
57-58: LGTM! Error property naming updated.The change from
rejectedReasontorejectReasonis consistent with the new rejection model.
62-63: LGTM! Consistent error handling.The error property update maintains consistency across all rejection paths.
516-517: LGTM! State event error handling updated.The error property change aligns with the unified error model.
767-769: Improved type safety with EventID.Good change from string to EventID type for the Map key, ensuring type consistency with event identifiers throughout the codebase.
793-797: Structured rejection with error codes.The rejection now includes a proper error code (
RejectCodes.AuthError), making error handling more deterministic and easier to process downstream.
803-807: LGTM! Enhanced rejection metadata.Good addition of the
rejectedByparameter (Line 806) to track which event caused the rejection, providing better traceability for debugging authorization failures.
814-815: LGTM! Consistent error property usage.The error property change maintains consistency with the new rejection model.
packages/room/src/manager/v3.ts (1)
3-3: LGTM! Generic type parameter properly implemented.The addition of the generic type parameter
Type extends PduType = PduTypeand its propagation toPersistentEventBase<RoomVersion3To11, Type>enables more precise typing throughout the event hierarchy. This is a foundational change that improves type safety across all room versions.Also applies to: 12-14
packages/room/src/state_resolution/definitions/definitions.ts (3)
25-29: Improved type safety with optional return.Good change to explicitly include
undefinedin the return type. This makes the API contract clearer and forces callers to handle the case where a state event might not exist.
432-432: Enhanced type specificity for power level events.The parameter type refinement from generic
PersistentEventBasetoPersistentEventBase<RoomVersion, 'm.room.power_levels'>improves type safety and makes the function's expectations explicit.
594-603: Well-structured error handling with typed exceptions.Excellent implementation of typed error handling. The code now properly catches
StateResolverAuthorizationErrorinstances and extracts structured error information (code, reason, rejectedBy) for proper event rejection. Unknown errors are re-thrown to prevent silent failures. This is a significant improvement over generic error handling.packages/room/src/manager/event-wrapper.ts (1)
408-416: LGTM on reject() shape.The new reject(code, reason, rejectedBy?) aligns with the propagated error model and downstream usage.
packages/room/src/authorizartion-rules/rules.ts (8)
30-71: Create-event checks look correct and aligned with spec.Prev events, domain match, known room_version, and creator presence are handled with structured errors.
171-224: Join flow auth checks look solid.Sender/state_key match, ban guard, public/invite join rules, and precise rejectedBy context are correctly enforced.
Consider adding tests for:
- join with invite join_rule when invitee has no invite state (should reject with code=auth_error and rejectedBy=join_rules event).
- join as creator with only the create event as prev (should allow).
257-293: Invite flow: power-level and state guards are correctly enforced.Good use of rejectedBy to point at power_levels and membership auth events.
295-344: Leave flow: kick/ban semantics are implemented correctly.Checks for self-leave, sender join requirement, and kick threshold are in place.
346-376: Ban flow: aligns with spec with proper rejectedBy context.
697-772: Auth without state: good validation of auth_events set (duplicates, unexpected, missing create).
784-872: Auth with state: general checks look good.m.federate enforcement, sender join requirement, power-level threshold, and user-state_key rules are handled consistently.
1-1: Codecov shows low coverage for this file; add targeted tests.Patch coverage is ~44% with many new branches. Please add tests for:
- alias validations (state_key missing and domain mismatch),
- membership branches (join/public, join/invite mismatch, invite sender not join, leave self and kick/bans),
- power level change rejections (user_default/events_default/state_default and per-user/per-event deltas).
Would you like help drafting these tests?
| throw new Error(membershipEvent.rejectReason); | ||
| } |
There was a problem hiding this comment.
Surface a proper HTTP error and add a safe fallback message
rejectReason may be undefined. Also, other methods in this service use HttpException/HttpStatus. Align and carry a default message.
Apply:
- throw new Error(membershipEvent.rejectReason);
+ throw new HttpException(
+ membershipEvent.rejectReason || 'Join rejected',
+ HttpStatus.FORBIDDEN,
+ );🤖 Prompt for AI Agents
In packages/federation-sdk/src/services/room.service.ts around lines 803-804,
replace the raw throw new Error(membershipEvent.rejectReason) with throwing an
HttpException using Nest's HttpException/HttpStatus and a safe default message;
construct a message like membershipEvent.rejectReason ?? 'Membership request
rejected' and throw new HttpException(message, HttpStatus.BAD_REQUEST) (add the
HttpException and HttpStatus imports if missing) so the error is an HTTP error
and never undefined.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/room/src/authorizartion-rules/errors.ts (1)
32-36: Serialise events before interpolating into the error message
PersistentEventBase#toStrippedJson()returns a plain object, so the current template string will emit[object Object]for both the rejected event and the auth event, throwing away the useful details we rely on in logs. Please stringify the objects before concatenating them into the message.- let message = `${code}: ${rejectedEvent.toStrippedJson()} failed authorization check`; + const rejectedEventJson = JSON.stringify(rejectedEvent.toStrippedJson()); + let message = `${code}: ${rejectedEventJson} failed authorization check`; if (rejectedBy) { - message += ` against auth event ${rejectedBy.toStrippedJson()}`; + message += ` against auth event ${JSON.stringify(rejectedBy.toStrippedJson())}`; }
📜 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.
📒 Files selected for processing (2)
packages/room/src/authorizartion-rules/errors.ts(1 hunks)packages/room/src/manager/event-wrapper.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/room/src/manager/event-wrapper.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/room/src/authorizartion-rules/errors.ts (1)
packages/room/src/types/_common.ts (1)
EventID(8-8)
ggazzo
force-pushed
the
correct-state-resolver-error
branch
from
494c581 to
39746db
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (13)
packages/room/src/manager/v6.ts (1)
1-1: Use type‑only import to avoid runtime cost.Mark
PduTypeimport as type‑only.-import { PduType } from '../types/v3-11'; +import { type PduType } from '../types/v3-11';packages/room/src/manager/v9.ts (1)
1-1: Use type‑only import to avoid runtime cost.Mark
PduTypeimport as type‑only.-import { PduType } from '../types/v3-11'; +import { type PduType } from '../types/v3-11';packages/federation-sdk/src/services/state.service.ts (1)
767-786: Unify Map key typing to EventID across both auth maps.You updated
requiredAuthEventsWeHaveSeenMaptoMap<EventID, ...>; mirror that forauthEventsReferencedMapto avoid string/EventID drift and get better type safety.- const authEventsReferencedMap = new Map<string, PersistentEventBase>(); + const authEventsReferencedMap = new Map<EventID, PersistentEventBase>();packages/room/src/manager/v3.ts (1)
3-3: Import PduType as a type-only import to avoid a runtime require.Prevents unnecessary JS emission and reduces the risk of cycles.
Apply this diff:
-import { PduType } from '../types/v3-11'; +import type { PduType } from '../types/v3-11';packages/room/src/authorizartion-rules/errors.ts (2)
15-21: Make error fields readonly.They’re immutable after construction; marking readonly communicates intent and prevents accidental mutation.
Apply this diff:
- reason: string; + readonly reason: string; - rejectedBy?: EventID; + readonly rejectedBy?: EventID; constructor( - public code: RejectCode, + public readonly code: RejectCode,
31-39: Consider slimming the error message payload.Embedding canonical JSON of events in Error.message can bloat logs. Consider logging only IDs in the message, and use structured logging for rich context where needed.
Example alternative:
- let message = `${code}: ${rejectedEvent.toStrippedJson()} failed authorization check`; + let message = `${code}: event ${rejectedEvent.eventId} failed authorization check`; - if (rejectedBy) { - message += ` against auth event ${rejectedBy.toStrippedJson()}`; - } + if (rejectedBy) { + message += ` against auth event ${rejectedBy.eventId}`; + }packages/room/src/state_resolution/definitions/definitions.ts (3)
8-8: Import PersistentEventBase as type-only.Only used in type positions; drops a runtime dependency edge.
Apply this diff:
-import { PersistentEventBase } from '../../manager/event-wrapper'; +import type { PersistentEventBase } from '../../manager/event-wrapper';
25-29: Annotate the return type to avoid relying on casts.Improves readability and type inference.
Apply this diff:
-export function getStateByMapKey<T extends PduType>( +export function getStateByMapKey<T extends PduType>( map: Map<StateMapKey, PersistentEventBase>, filter: { type: T; state_key?: string; }, -) { - return map.get(getStateMapKey(filter)) as - | PersistentEventBase<RoomVersion, T> - | undefined; +) : PersistentEventBase<RoomVersion, T> | undefined { + return map.get(getStateMapKey(filter)) as + | PersistentEventBase<RoomVersion, T> + | undefined; }
595-603: Log after rejection with structured fields; avoid dumping full Error.Sets rejection first, then logs minimal context. Keeps unknown errors fatal.
Apply this diff:
- } catch (error) { - console.warn('event not allowed', error); - if (error instanceof StateResolverAuthorizationError) { - event.reject(error.code, error.reason, error.rejectedBy); - continue; - } - - // if unknown error we halt building new state - throw error; - } + } catch (error) { + if (error instanceof StateResolverAuthorizationError) { + event.reject(error.code, error.reason, error.rejectedBy); + console.warn('event rejected', { + eventId: event.eventId, + code: error.code, + reason: error.reason, + rejectedBy: error.rejectedBy, + }); + continue; + } + throw error; // unknown errors halt building new state + }packages/room/src/manager/event-wrapper.ts (3)
48-53: Prefer undefined over empty-string sentinels for rejection fields.Removes sentinel coupling and simplifies truthiness checks.
Apply this diff:
- public rejectCode = ''; - - public rejectReason = ''; - - public rejectedBy = '' as EventID; + public rejectCode?: RejectCode; + + public rejectReason?: string; + + public rejectedBy?: EventID;
409-416: Align rejected getter and reject() with optional fields.Avoid string comparisons for state.
Apply this diff:
- get rejected() { - return this.rejectCode !== ''; - } + get rejected() { + return this.rejectCode !== undefined; + } - reject(code: RejectCode, reason: string, rejectedBy?: EventID) { - this.rejectCode = code; - this.rejectReason = reason; - if (rejectedBy) this.rejectedBy = rejectedBy; - } + reject(code: RejectCode, reason: string, rejectedBy?: EventID) { + this.rejectCode = code; + this.rejectReason = reason; + this.rejectedBy = rejectedBy; + }
439-447: Optionally return a POJO and let callers choose serialization.Returning a string reduces flexibility (e.g., structured logs). Consider returning an object and letting callers call encodeCanonicalJson.
Example:
- toStrippedJson() { - return encodeCanonicalJson({ + toStrippedJson() { + return encodeCanonicalJson(this.toStrippedObject()); + } + + toStrippedObject() { + return { eventId: this.eventId, type: this.type, roomId: this.roomId, sender: this.sender, stateKey: this.stateKey, - }); + }; }packages/room/src/authorizartion-rules/rules.ts (1)
103-111: Check for membership property presence, not just event type.Current check can pass when content.membership is missing; the thrown reason promises to catch that earlier.
Apply this diff:
- if ( - !membershipEventToCheck.stateKey || - !membershipEventToCheck.isMembershipEvent() - ) { + if ( + !membershipEventToCheck.stateKey || + membershipEventToCheck.getContent().membership === undefined + ) { throw new StateResolverAuthorizationError(RejectCodes.AuthError, { rejectedEvent: membershipEventToCheck, reason: 'm.room.member event has no state_key or membership property', }); }
📜 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.
📒 Files selected for processing (18)
packages/federation-sdk/src/services/invite.service.ts(3 hunks)packages/federation-sdk/src/services/message.service.ts(5 hunks)packages/federation-sdk/src/services/room.service.ts(2 hunks)packages/federation-sdk/src/services/send-join.service.ts(1 hunks)packages/federation-sdk/src/services/state.service.ts(5 hunks)packages/homeserver/src/controllers/internal/invite.controller.ts(1 hunks)packages/room/src/authorizartion-rules/errors.ts(1 hunks)packages/room/src/authorizartion-rules/rules.ts(30 hunks)packages/room/src/index.ts(1 hunks)packages/room/src/manager/event-wrapper.ts(4 hunks)packages/room/src/manager/factory.ts(2 hunks)packages/room/src/manager/power-level-event-wrapper.ts(1 hunks)packages/room/src/manager/v11.ts(1 hunks)packages/room/src/manager/v3.ts(2 hunks)packages/room/src/manager/v6.ts(1 hunks)packages/room/src/manager/v8.ts(1 hunks)packages/room/src/manager/v9.ts(1 hunks)packages/room/src/state_resolution/definitions/definitions.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/room/src/manager/power-level-event-wrapper.ts
- packages/federation-sdk/src/services/room.service.ts
- packages/federation-sdk/src/services/message.service.ts
- packages/room/src/manager/factory.ts
- packages/federation-sdk/src/services/invite.service.ts
- packages/room/src/manager/v8.ts
- packages/homeserver/src/controllers/internal/invite.controller.ts
- packages/federation-sdk/src/services/send-join.service.ts
🧰 Additional context used
🧬 Code graph analysis (9)
packages/room/src/manager/v6.ts (2)
packages/room/src/types/v3-11.ts (1)
PduType(47-47)packages/room/src/manager/v3.ts (1)
PersistentEventV3(12-79)
packages/room/src/manager/v3.ts (2)
packages/room/src/types/v3-11.ts (1)
PduType(47-47)packages/room/src/manager/type.ts (1)
RoomVersion3To11(3-12)
packages/room/src/manager/v11.ts (2)
packages/room/src/types/v3-11.ts (1)
PduType(47-47)packages/room/src/manager/v9.ts (1)
PersistentEventV9(4-16)
packages/federation-sdk/src/services/state.service.ts (3)
packages/room/src/types/_common.ts (1)
EventID(8-8)packages/room/src/authorizartion-rules/errors.ts (1)
RejectCodes(4-8)packages/room/src/authorizartion-rules/rules.ts (1)
checkEventAuthWithState(774-886)
packages/room/src/authorizartion-rules/errors.ts (1)
packages/room/src/types/_common.ts (1)
EventID(8-8)
packages/room/src/manager/event-wrapper.ts (5)
packages/room/src/manager/type.ts (1)
RoomVersion(14-14)packages/room/src/types/v3-11.ts (1)
PduType(47-47)packages/room/src/types/_common.ts (1)
EventID(8-8)packages/room/src/authorizartion-rules/errors.ts (1)
RejectCode(10-10)packages/core/src/utils/signJson.ts (1)
encodeCanonicalJson(175-195)
packages/room/src/authorizartion-rules/rules.ts (6)
packages/federation-sdk/src/index.ts (2)
PersistentEventBase(21-21)RoomVersion(22-22)packages/room/src/manager/type.ts (1)
RoomVersion(14-14)packages/room/src/authorizartion-rules/errors.ts (2)
StateResolverAuthorizationError(48-48)RejectCodes(4-8)packages/room/src/manager/power-level-event-wrapper.ts (1)
PowerLevelEvent(199-199)packages/core/src/events/m.room.create.ts (1)
roomCreateEvent(26-43)packages/room/src/manager/event-wrapper.ts (2)
event(110-119)stateKey(98-102)
packages/room/src/manager/v9.ts (2)
packages/room/src/types/v3-11.ts (1)
PduType(47-47)packages/room/src/manager/v8.ts (1)
PersistentEventV8(4-14)
packages/room/src/state_resolution/definitions/definitions.ts (3)
packages/room/src/manager/type.ts (1)
RoomVersion(14-14)packages/room/src/authorizartion-rules/errors.ts (1)
StateResolverAuthorizationError(48-48)packages/room/src/manager/event-wrapper.ts (1)
event(110-119)
🔇 Additional comments (15)
packages/room/src/index.ts (1)
5-5: Public re‑export of errors is fine; confirm API surface change.Exposing
errorsvia the barrel looks intentional and matches the new error model. Please confirm this is part of the public API and won’t require a semver bump or deprecation notes.packages/room/src/manager/v6.ts (1)
4-6: Generic propagation LGTM.Making
PersistentEventV6generic and forwarding the type to the base aligns with the rest of the hierarchy.packages/room/src/manager/v9.ts (1)
4-6: Generic propagation LGTM.Matches v6/v11 patterns and keeps typings consistent.
packages/room/src/manager/v11.ts (2)
1-1: Type‑only import LGTM.Keeps the import erased at runtime.
5-7: Generic propagation LGTM.Correctly extends with
Type extends PduType.packages/federation-sdk/src/services/state.service.ts (3)
803-806: ValidaterejectedBysemantics.You pass
eventId(the expected auth event) asrejectedBy. Confirm this matches the contract (ID of the auth event that caused the rejection). If the message references a wrong auth event (present but incorrect), consider passing that actual referenced ID instead.
21-21: Import of RejectCodes LGTM.Consistent with the new error model.
57-57: Preserve structured rejection in state.service.ts
Replace allthrow new Error(...rejectReason)calls (lines 57, 62, 516, 797, 807, 814) with athrowRejection(event)helper that throwsStateResolverAuthorizationErrorcarryingrejectCode,rejectReason, andrejectedBy.At the top of packages/federation-sdk/src/services/state.service.ts add:
import { StateResolverAuthorizationError, RejectCodes } from '@rocket.chat/federation-room'; function throwRejection(event: { rejectReason?: string; rejectCode?: RejectCodes; rejectedBy?: string }): never { const code = event.rejectCode ?? RejectCodes.ValidationError; const reason = event.rejectReason ?? 'event rejected'; throw new StateResolverAuthorizationError(code, { rejectedEvent: event, rejectedBy: event.rejectedBy, reason, }); }Confirm upstream callers still expect and correctly handle this typed error.
packages/room/src/manager/v3.ts (1)
12-14: LGTM: generic typing on PersistentEventV3.The generic constraint and inheritance align with PersistentEventBase<RoomVersion3To11, Type>.
packages/room/src/authorizartion-rules/errors.ts (1)
4-11: LGTM: standardized reject codes and type.Clear, extensible code system for downstream consumers.
packages/room/src/manager/event-wrapper.ts (2)
45-47: LGTM: default RoomVersion generic broadened.Aligns with multi-version usage.
48-53: No stale uses ofrejectedReason; all call sites useevent.reject(code, reason, rejectedBy).packages/room/src/authorizartion-rules/rules.ts (3)
30-33: LGTM: typed create event guard.Signature restriction to 'm.room.create' improves safety.
75-92: Fix event type in error messages (aliases vs canonical_alias).Messages should reference m.room.aliases.
Apply this diff:
- reason: 'm.room.canonical_alias event has no state_key', + reason: 'm.room.aliases event has no state_key', ... - reason: - 'm.room.canonical_alias event sender domain does not match state_key', + reason: + 'm.room.aliases event sender domain does not match state_key',
697-706: Add tests for new rejection paths (Codecov −0.26%)
Cover at least:
- join: non-matching sender/state_key; invite join_rule without invite; banned sender
- invite: sender not in room; invitee already join/ban; insufficient power
- leave/ban: insufficient power; sender not in room
- create: prev_events present; domain mismatch; unrecognized version; missing creator
Use
rg -n "throw new StateResolverAuthorizationError" packages/room/src/authorization-rules/rules.tsto locate all branches andnpm test -- --coverageto identify untested paths.
ggazzo
force-pushed
the
correct-state-resolver-error
branch
from
39746db to
42c1974
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/federation-sdk/src/services/invite.service.ts (3)
83-93: Propagate structured invite rejection info
checkEventAuthWithStatenow raisesStateResolverAuthorizationError(and tags the event). Catching that as a plainErrorhere drops the newrejectCode/rejectReason/rejectedBydetails that the follow-up work relies on. Please forward the structured error (or at least its fields) instead of constructing a genericError, so consumers can distinguish auth errors from other failures.
150-177: Don’t swallow the rejection metadata on remote invitesSame concern as above: throwing a fresh
Errorwith onlyrejectReasonthrows awayrejectCodeandrejectedBy. Propagate theStateResolverAuthorizationError(or clone its fields) so upstream logic can persist the rejection accurately.
169-188: Persisted rejection loses structured contextWhen the persisted invite is rejected after we fetch room state, we again collapse the structured rejection into a bare
Error. Please return/throw an error that retainsrejectCode,rejectReason, andrejectedByso callers can handle it appropriately.packages/federation-sdk/src/services/state.service.ts (1)
53-58: Catch and rethrow structured auth failures
checkEventAuthWithStatenow raisesStateResolverAuthorizationError. Without a try/catch here, that exception bubbles out, bypassing theevent.rejectedcheck and losing the metadata. Wrap the auth call, interceptStateResolverAuthorizationError, callevent.reject(...), and rethrow (or return) the structured error instead ofnew Error(event.rejectReason).
🧹 Nitpick comments (1)
packages/room/src/state_resolution/definitions/definitions.ts (1)
595-603: Avoid losing rejection contextGood call catching
StateResolverAuthorizationError, but we still end up droppingerrorwhen weConsole.warnand continue. Consider returning/propagating the error (or at least the metadata) to the caller so it can persist the rejection instead of silently skipping the event.
📜 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.
📒 Files selected for processing (12)
packages/federation-sdk/src/services/invite.service.ts(3 hunks)packages/federation-sdk/src/services/message.service.ts(5 hunks)packages/federation-sdk/src/services/room.service.ts(2 hunks)packages/federation-sdk/src/services/send-join.service.ts(1 hunks)packages/federation-sdk/src/services/state.service.ts(5 hunks)packages/homeserver/src/controllers/internal/invite.controller.ts(1 hunks)packages/room/src/authorizartion-rules/errors.ts(1 hunks)packages/room/src/authorizartion-rules/rules.ts(30 hunks)packages/room/src/index.ts(1 hunks)packages/room/src/manager/event-wrapper.ts(4 hunks)packages/room/src/manager/factory.ts(1 hunks)packages/room/src/state_resolution/definitions/definitions.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/federation-sdk/src/services/send-join.service.ts
- packages/room/src/manager/factory.ts
🧰 Additional context used
🧬 Code graph analysis (5)
packages/federation-sdk/src/services/state.service.ts (3)
packages/room/src/types/_common.ts (1)
EventID(8-8)packages/room/src/authorizartion-rules/errors.ts (1)
RejectCodes(4-8)packages/room/src/authorizartion-rules/rules.ts (1)
checkEventAuthWithState(776-888)
packages/room/src/authorizartion-rules/errors.ts (1)
packages/room/src/types/_common.ts (1)
EventID(8-8)
packages/room/src/authorizartion-rules/rules.ts (3)
packages/federation-sdk/src/index.ts (2)
PersistentEventBase(21-21)RoomVersion(22-22)packages/room/src/manager/type.ts (1)
RoomVersion(14-14)packages/room/src/authorizartion-rules/errors.ts (2)
StateResolverAuthorizationError(48-48)RejectCodes(4-8)
packages/room/src/state_resolution/definitions/definitions.ts (2)
packages/room/src/authorizartion-rules/errors.ts (1)
StateResolverAuthorizationError(48-48)packages/room/src/manager/event-wrapper.ts (1)
event(113-122)
packages/room/src/manager/event-wrapper.ts (3)
packages/room/src/types/_common.ts (1)
EventID(8-8)packages/room/src/authorizartion-rules/errors.ts (1)
RejectCode(10-10)packages/core/src/utils/signJson.ts (1)
encodeCanonicalJson(175-195)
🔇 Additional comments (4)
packages/federation-sdk/src/services/message.service.ts (1)
85-85: Consistent rejection messaging propagatedUsing
rejectReasonmatches the updatedPersistentEventBasecontract, so callers will now see the same rejection text the state resolver emits. Looks good to me.Also applies to: 134-134, 170-170, 226-226, 277-277
packages/federation-sdk/src/services/room.service.ts (1)
803-804: Throw an HttpException with a fallback message
new Errorbubbles up as a 500 with anundefinedmessage whenrejectReasonis missing. Surface a proper 4xx so clients get a stable error contract.- throw new Error(membershipEvent.rejectReason); + throw new HttpException( + membershipEvent.rejectReason || 'Join rejected', + HttpStatus.FORBIDDEN, + );packages/room/src/index.ts (1)
5-5: Exporting the new error surface looks goodBarrel now re-exports the authorization errors, letting SDK consumers import
RejectCodesandStateResolverAuthorizationErrorfrom the package root. 👍packages/room/src/authorizartion-rules/rules.ts (1)
80-91: Fix alias error strings to referencem.room.aliasesLine 82 / Line 90: these rejection reasons still mention
m.room.canonical_aliaseven though the validator operates onm.room.aliases. Please align the wording (this was called out in the prior review round).- reason: 'm.room.canonical_alias event has no state_key', + reason: 'm.room.aliases event has no state_key', … - reason: - 'm.room.canonical_alias event sender domain does not match state_key', + reason: + 'm.room.aliases event sender domain does not match state_key',
| throw new Error(joinEventFinal.rejectReason); | ||
| } |
There was a problem hiding this comment.
Return a structured HTTP error
Same issue on the remote join path: a plain Error becomes a 500 and leaks an "undefined" message when the join was legitimately rejected. Emit an HttpException with a safe fallback instead.
- if (joinEventFinal.rejected) {
- throw new Error(joinEventFinal.rejectReason);
+ if (joinEventFinal.rejected) {
+ throw new HttpException(
+ joinEventFinal.rejectReason || 'Join rejected',
+ HttpStatus.FORBIDDEN,
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| throw new Error(joinEventFinal.rejectReason); | |
| } | |
| if (joinEventFinal.rejected) { | |
| throw new HttpException( | |
| joinEventFinal.rejectReason || 'Join rejected', | |
| HttpStatus.FORBIDDEN, | |
| ); | |
| } |
🤖 Prompt for AI Agents
In packages/federation-sdk/src/services/room.service.ts around lines 986-987,
replace throwing a plain Error with throwing a structured HttpException that
uses the joinEventFinal.rejectReason if present or a safe fallback message like
"Join request rejected", and set an appropriate HTTP status (e.g.,
HttpStatus.FORBIDDEN); also add the necessary imports (HttpException and
HttpStatus from @nestjs/common) and apply the same change to the remote join
path to avoid leaking "undefined" and to return a proper HTTP error.
| RejectCodes.AuthError, | ||
| `Auth events referenced in message do not match, expected ${requiredAuthEventsWeHaveSeenMap.size} but got ${authEventsReferencedMap.size}`, | ||
| ); | ||
| throw new Error(event.rejectedReason); | ||
| throw new Error(event.rejectReason); | ||
| } |
There was a problem hiding this comment.
Preserve RejectCodes when failing auth prechecks
After calling event.reject(RejectCodes.AuthError, …), throwing new Error(event.rejectReason) discards the code and rejectedBy. Please throw/return the StateResolverAuthorizationError (or include its fields) so higher layers can differentiate auth errors from other failures.
| for (const [eventId] of requiredAuthEventsWeHaveSeenMap) { | ||
| if (!authEventsReferencedMap.has(eventId)) { | ||
| event.reject( | ||
| RejectCodes.AuthError, | ||
| `wrong auth event in message, expected ${eventId} but not found in event`, | ||
| eventId, | ||
| ); | ||
| throw new Error(event.rejectedReason); | ||
| throw new Error(event.rejectReason); | ||
| } |
There was a problem hiding this comment.
Propagate full rejection context
Same as above: we reject the event with RejectCodes.AuthError, but then throw away the structured metadata by emitting a plain Error. Please surface the structured error object instead.
🤖 Prompt for AI Agents
In packages/federation-sdk/src/services/state.service.ts around lines 787 to
795, the code calls event.reject(RejectCodes.AuthError, ...) but then throws a
plain Error using event.rejectReason, losing the structured rejection metadata;
change this to capture and rethrow the structured rejection object returned by
event.reject (or construct and throw an object containing the RejectCode,
rejectReason and relevant metadata) instead of throwing new
Error(event.rejectReason) so callers receive the full rejection context.
| // now we validate against auth rules | ||
| await checkEventAuthWithState(event, room, store); | ||
| if (event.rejected) { | ||
| throw new Error(event.rejectedReason); | ||
| throw new Error(event.rejectReason); | ||
| } |
There was a problem hiding this comment.
Unhandled StateResolverAuthorizationError
checkEventAuthWithState can now throw StateResolverAuthorizationError. Catch it here, ensure event.reject is called, and propagate the structured error so callers can handle authorization failures deterministically.
🤖 Prompt for AI Agents
In packages/federation-sdk/src/services/state.service.ts around lines 798 to
802, the call to checkEventAuthWithState can throw
StateResolverAuthorizationError but the code only checks event.rejected
afterwards; wrap the await checkEventAuthWithState(event, room, store) in a
try/catch, catch StateResolverAuthorizationError specifically, call
event.reject(...) (providing the error message/reason and setting
event.rejectReason as needed), then rethrow the caught
StateResolverAuthorizationError so callers receive the structured error; for any
other exceptions rethrow them unchanged.
| if (membershipEvent.rejected) { | ||
| throw new Error(membershipEvent.rejectedReason); | ||
| throw new Error(membershipEvent.rejectReason); | ||
| } |
There was a problem hiding this comment.
Surface the rejection details instead of masking them
We now tag state-resolver rejections with rejectCode, rejectReason, and rejectedBy. Re-throwing a generic Error discards that context, making it impossible for callers to tell why the invite was rejected or which auth event caused it. Please propagate the structured rejection (e.g., by rethrowing the original StateResolverAuthorizationError or wrapping the code/reason in a richer error type) so downstream logic can act on it.
🤖 Prompt for AI Agents
In packages/homeserver/src/controllers/internal/invite.controller.ts around
lines 63 to 65, the code throws a generic Error when membershipEvent.rejected is
true, which discards the structured rejection details (rejectCode, rejectReason,
rejectedBy); instead rethrow the original structured rejection (e.g. the
StateResolverAuthorizationError instance) or construct and throw a new error
that preserves those fields (include rejectCode, rejectReason, rejectedBy and
original error as cause) so callers can inspect the rejection details for
downstream handling.
| toStrippedJson() { | ||
| return encodeCanonicalJson({ | ||
| eventId: this.eventId, | ||
| type: this.type, | ||
| roomId: this.roomId, | ||
| sender: this.sender, | ||
| stateKey: this.stateKey, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Prevent toStrippedJson from emitting invalid JSON when stateKey is missing
Line 452: when stateKey is undefined (timeline events), encodeCanonicalJson renders it as stateKey:undefined, which is not valid JSON and breaks downstream parsing. Please omit the property when it is absent.
- return encodeCanonicalJson({
- eventId: this.eventId,
- type: this.type,
- roomId: this.roomId,
- sender: this.sender,
- stateKey: this.stateKey,
- });
+ const stripped: Record<string, string> = {
+ eventId: this.eventId,
+ type: this.type,
+ roomId: this.roomId,
+ sender: this.sender,
+ };
+
+ if (this.stateKey !== undefined) {
+ stripped.stateKey = this.stateKey;
+ }
+
+ return encodeCanonicalJson(stripped);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| toStrippedJson() { | |
| return encodeCanonicalJson({ | |
| eventId: this.eventId, | |
| type: this.type, | |
| roomId: this.roomId, | |
| sender: this.sender, | |
| stateKey: this.stateKey, | |
| }); | |
| } | |
| toStrippedJson() { | |
| const stripped: Record<string, string> = { | |
| eventId: this.eventId, | |
| type: this.type, | |
| roomId: this.roomId, | |
| sender: this.sender, | |
| }; | |
| if (this.stateKey !== undefined) { | |
| stripped.stateKey = this.stateKey; | |
| } | |
| return encodeCanonicalJson(stripped); | |
| } |
🤖 Prompt for AI Agents
In packages/room/src/manager/event-wrapper.ts around lines 447 to 455,
toStrippedJson currently always includes stateKey which results in invalid JSON
when stateKey is undefined; change the construction of the object passed to
encodeCanonicalJson so stateKey is only included when it is not undefined (e.g.
build a base object with eventId,type,roomId,sender and conditionally add
stateKey when !== undefined) and then pass that cleaned object to
encodeCanonicalJson so timeline events omit the property.
required for further prs where we use these to save rejections
example log
Summary by CodeRabbit
New Features
Bug Fixes
Refactor