fix: delete message #240
fix: delete message #240
Conversation
WalkthroughRedaction events now carry Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client as Internal Caller
participant C as MessageController
participant S as MessageService
participant ES as EventStore
participant EB as RedactionFactory
participant SA as StagingArea
participant Fed as Federation
Client->>C: POST /internal/messages/:id/redact
C->>S: redactMessage(roomId, eventIdToRedact)
S->>ES: getEvent(eventIdToRedact)
ES-->>S: event { sender, ... }
Note over S,ES: Sender is derived from the target event
S->>EB: create redaction(redacts: eventIdToRedact, reason?)
EB-->>S: redaction PDU { type: m.room.redaction, redacts, content{reason?}, ... }
S->>SA: stage(redaction PDU)
SA->>Fed: emit m.room.redaction (reads event.redacts)
Fed-->>SA: ack
SA-->>S: staged id
S-->>C: redaction event id
C-->>Client: 200 OK
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ 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 #240 +/- ##
=======================================
Coverage 81.86% 81.86%
=======================================
Files 63 63
Lines 4709 4709
=======================================
Hits 3855 3855
Misses 854 854 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ricardogarim
force-pushed
the
fix/delete-message
branch
from
6fa2154 to
e124380
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/core/src/events/m.room.redaction.ts (1)
37-88: Refactor correctly moves redacts to top-level parameter.The change properly relocates
redactsfrom thecontentobject to a top-level function parameter, aligning with the event structure whereredactsis a top-level property.The comment at lines 62-63 is now misleading:
// Extract redacts from content - it must be at top level only const { reason } = content;This comment suggests
redactsis being extracted fromcontent, but it's now a top-level parameter. Consider updating or removing this comment:- // Extract redacts from content - it must be at top level only + // Extract reason from content (redacts is now a top-level parameter) const { reason } = content;
📜 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 (8)
packages/core/src/events/eventBase.ts(1 hunks)packages/core/src/events/m.room.redaction.spec.ts(1 hunks)packages/core/src/events/m.room.redaction.ts(2 hunks)packages/federation-sdk/src/services/message.service.ts(2 hunks)packages/federation-sdk/src/services/staging-area.service.ts(1 hunks)packages/homeserver/src/controllers/internal/message.controller.ts(1 hunks)packages/homeserver/src/dtos/internal/message.dto.ts(0 hunks)packages/room/src/types/v3-11.ts(0 hunks)
💤 Files with no reviewable changes (2)
- packages/room/src/types/v3-11.ts
- packages/homeserver/src/dtos/internal/message.dto.ts
🧰 Additional context used
🧬 Code graph analysis (4)
packages/federation-sdk/src/services/staging-area.service.ts (1)
packages/room/src/manager/event-wrapper.ts (1)
event(108-117)
packages/core/src/events/m.room.redaction.ts (2)
packages/federation-sdk/src/index.ts (1)
EventID(23-23)packages/room/src/types/_common.ts (1)
EventID(8-8)
packages/core/src/events/eventBase.ts (2)
packages/room/src/manager/event-wrapper.ts (1)
event(108-117)packages/room/src/types/_common.ts (2)
PduForType(14-14)EventID(8-8)
packages/federation-sdk/src/services/message.service.ts (1)
packages/core/src/events/m.room.redaction.ts (1)
redactionEvent(37-88)
🔇 Additional comments (3)
packages/federation-sdk/src/services/staging-area.service.ts (1)
226-237: LGTM! Redaction payload correctly reads from top-level redacts.The change aligns with the PR's refactor to move
redactsfromcontentto the top-level event property. TheisRedactedEventtype guard ensures theredactsfield exists before this code executes.packages/core/src/events/m.room.redaction.spec.ts (1)
57-94: LGTM! Test fixture updated to reflect top-level redacts.The test correctly validates that
redactsis present as a top-level property of the redaction event (line 81-82). The reordering of theredactsfield in the fixture aligns with the new event structure.packages/homeserver/src/controllers/internal/message.controller.ts (1)
149-179: Approve simplified redactMessage controller updateController now calls
redactMessage(roomId, messageId), matching the updated service signature inMessageService. The service internally derives the sender viaeventService.getEventById, so nosenderUserIdneeds to be passed. LGTM.
ricardogarim
force-pushed
the
fix/delete-message
branch
from
f684355 to
512b3bb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/federation-sdk/src/services/message.service.ts (1)
422-425: Pass redactingUserId to redactMessage in the homeserver controller
In packages/homeserver/src/controllers/internal/message.controller.ts (line 156), the callawait messageService.redactMessage(roomId, params.messageId as EventID)must include the ID of the user performing the deletion as the third argument.
♻️ Duplicate comments (2)
packages/core/src/events/eventBase.ts (1)
43-43: Test coverage needed for runtime check.The new runtime check
'redacts' in eventis missing test coverage, as indicated by Codecov (50% patch coverage) and a previous review comment.packages/federation-sdk/src/services/message.service.ts (1)
436-439: CRITICAL: Redaction sender must be the acting user, not the message author.Lines 436-439 fetch the event being redacted and extract its sender, which is then used as the redaction sender (line 454). This violates the Matrix specification and creates several critical problems:
- Spec violation: The redaction
sendermust be the user performing the redaction, not the author of the original message.- Breaks admin deletions: An admin cannot delete another user's message because the redaction would be signed by the original author.
- Cross-domain failure: Cannot redact events authored on foreign domains (you cannot legitimately sign PDUs as
@alice:remote.netfrom your homeserver).The
redactMessagefunction must accept aredactingUserIdparameter representing the user performing the deletion. Fetch the target event only for validation, then use the redacting user as the sender:async redactMessage( roomId: string, eventIdToRedact: EventID, + redactingUserId: string, ): Promise<string> { const isTombstoned = await this.roomService.isRoomTombstoned(roomId); if (isTombstoned) { this.logger.warn( `Attempted to delete a message in a tombstoned room: ${roomId}`, ); throw new ForbiddenError('Cannot delete a message in a tombstoned room'); } const roomInfo = await this.stateService.getRoomInformation(roomId); - const senderUserId = await this.eventService.getEventById(eventIdToRedact); - if (!senderUserId?.event.sender) { - throw new Error(`Sender user ID not found for event ${eventIdToRedact}`); - } + const eventToRedact = await this.eventService.getEventById(eventIdToRedact); + if (!eventToRedact?.event) { + throw new Error(`Event not found: ${eventIdToRedact}`); + } const redactionEvent = await this.stateService.buildEvent<'m.room.redaction'>( { type: 'm.room.redaction', content: { reason: `Deleting message: ${eventIdToRedact}`, }, redacts: eventIdToRedact, room_id: roomId, auth_events: [], depth: 0, prev_events: [], origin_server_ts: Date.now(), - sender: senderUserId.event.sender, + sender: redactingUserId, }, roomInfo.room_version, );
🧹 Nitpick comments (1)
packages/core/src/events/m.room.redaction.ts (1)
62-63: Update misleading comment.The comment states "Extract redacts from content" but the code no longer extracts
redactsfromcontent. Instead,redactsis now a top-level parameter.Apply this diff to clarify the comment:
- // Extract redacts from content - it must be at top level only - const { reason } = content; + // Extract reason from content (redacts is handled as a top-level parameter) + const { reason } = content;
📜 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 (6)
packages/core/src/events/eventBase.ts(1 hunks)packages/core/src/events/m.room.redaction.spec.ts(1 hunks)packages/core/src/events/m.room.redaction.ts(2 hunks)packages/federation-sdk/src/services/message.service.ts(2 hunks)packages/federation-sdk/src/services/staging-area.service.ts(1 hunks)packages/room/src/types/v3-11.ts(0 hunks)
💤 Files with no reviewable changes (1)
- packages/room/src/types/v3-11.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/core/src/events/m.room.redaction.spec.ts
- packages/federation-sdk/src/services/staging-area.service.ts
🧰 Additional context used
🧬 Code graph analysis (3)
packages/core/src/events/m.room.redaction.ts (2)
packages/federation-sdk/src/index.ts (1)
EventID(23-23)packages/room/src/types/_common.ts (1)
EventID(8-8)
packages/core/src/events/eventBase.ts (2)
packages/room/src/manager/event-wrapper.ts (1)
event(108-117)packages/room/src/types/_common.ts (1)
PduForType(14-14)
packages/federation-sdk/src/services/message.service.ts (1)
packages/core/src/events/m.room.redaction.ts (1)
redactionEvent(37-88)
🔇 Additional comments (2)
packages/core/src/events/m.room.redaction.ts (1)
37-61: LGTM! Function signature correctly reflects top-levelredacts.The
redactionEventfunction signature has been correctly updated to acceptredactsas a top-level parameter rather than withincontent. This aligns with the Matrix specification and the PR's objective.packages/federation-sdk/src/services/message.service.ts (1)
352-375: LGTM! Correct top-levelredactsfield inunsetReaction.The addition of
redacts: eventIdReactedToas a top-level field (line 359) correctly aligns with the PR's objective to moveredactsout ofcontent.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/federation-sdk/src/services/room.service.ts (1)
930-936: Use logger utility instead of console.log.This debug output uses
console.lograther than the structuredloggerutility used elsewhere in the codebase.Apply this diff to use the logger:
if (!authEvent) { for (const stateEvent of eventMap.keys()) { - console.log( + logger.debug( `${stateEvent} -> ${JSON.stringify( eventMap.get(stateEvent)?.event, null, 2, )}`, ); } throw new Error(`Auth event ${authEventId} not found`); }
📜 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 (3)
packages/federation-sdk/src/services/room.service.ts(9 hunks)packages/federation-sdk/src/services/staging-area.service.ts(1 hunks)packages/room/src/types/v3-11.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/federation-sdk/src/services/staging-area.service.ts
🧰 Additional context used
🧬 Code graph analysis (2)
packages/room/src/types/v3-11.ts (1)
packages/room/src/types/_common.ts (1)
eventIdSchema(6-6)
packages/federation-sdk/src/services/room.service.ts (2)
packages/federation-sdk/src/index.ts (1)
EventEmitterService(69-69)packages/room/src/manager/event-wrapper.ts (2)
roomId(78-80)event(108-117)
🔇 Additional comments (9)
packages/room/src/types/v3-11.ts (2)
198-200: Correct removal ofredactsfrom content schema.The
PduRoomRedactionContentSchemanow correctly contains onlyreason, withredactsmoved to the top-level event structure (line 694). This aligns with the Matrix specification where redaction events carryredactsas a top-level field rather than withincontent.
694-694: Approveredactstop‐level and refine its descriptionIn packages/room/src/types/v3-11.ts at line 694, update to:
- redacts: eventIdSchema.describe('event id'), + redacts: eventIdSchema.describe('The ID of the event being redacted'),Legacy
content.redactsusages remain intentional for migration and test coverage—no further consumer updates required.Likely an incorrect or invalid review comment.
packages/federation-sdk/src/services/room.service.ts (7)
42-42: LGTM!The import of
EventEmitterServiceis correct and aligns with the new functionality added to emit membership events.
48-57: LGTM!The
EventEmitterServicedependency injection is correctly implemented and follows the established pattern for constructor injection in this service.
474-478: LGTM!The multi-line formatting improves readability of the error log message. The logic remains unchanged and properly handles missing auth events.
626-628: LGTM!The multi-line formatting improves readability of the kick action log message.
707-709: LGTM!The multi-line formatting improves readability of the ban action log message.
962-964: LGTM!The multi-line formatting improves readability of the log message.
1007-1010: LGTM!The multi-line formatting improves readability of the state logging message.
Summary by CodeRabbit
Bug Fixes
Refactor
New Features
Tests