fix: validate event from sender #227
Conversation
WalkthroughRefactors event validation to derive origin from event.sender, removes the origin parameter from validateEvent, adds early domain and room version checks, rejects third-party invites, updates call sites to pass only the event, and enhances error logging while continuing processing on validation failures. Changes
Sequence Diagram(s)sequenceDiagram
participant S as Sender HS
participant ES as EventService
participant VS as validateEvent
participant CH as checkSignAndHashes
participant L as Logger
S->>ES: processIncomingPDUs(events)
loop For each event
ES->>VS: validateEvent(event)
note over VS: Extract origin from event.sender<br/>Reject third-party invites<br/>Ensure room version present
VS->>CH: verify signatures & hashes(origin, event)
CH-->>VS: result
alt validation fails
VS-->>ES: throw Error
ES->>L: log failure with origin and event
ES-->>S: continue to next event
else validation passes
VS-->>ES: ok
ES-->>S: proceed with processing
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #227 +/- ##
=======================================
Coverage 81.67% 81.67%
=======================================
Files 63 63
Lines 4682 4682
=======================================
Hits 3824 3824
Misses 858 858 ☔ 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: 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/event.service.ts (1)
270-272: Fail fast on missing signature and unify server-name extraction
- In
packages/federation-sdk/src/services/event.service.tsbefore callingcheckSignAndHashes:+ if (!event.signatures?.[origin]) { + throw new Error(`M_MISSING_SIGNATURE_FOR_ORIGIN: ${origin}`); + }
- Replace
extractDomainin the same file with:-private extractDomain(id: string): string { - const parts = id.split(':'); - return parts.length > 1 ? parts[1] : ''; -} +private extractDomain(id: string): string { + const idx = id.indexOf(':'); + return idx > -1 ? id.slice(idx + 1) : ''; +}Apply the substring-based extraction (
id.slice(id.indexOf(':') + 1)) across allextractDomain/extractServerNameFromUserIdimplementations so the derived origin (including port) matches the keys inevent.signatures.
🧹 Nitpick comments (1)
packages/federation-sdk/src/services/event.service.ts (1)
178-186: Log both transaction and event origins on validation failures (and consider redacting payload).Include the origin derived from the event in the error log to quickly spot mismatches with the transaction origin. Also consider logging only event_id/type instead of the full event to avoid leaking PII on errors.
- this.logger.error({ + const eventOrigin = this.extractServerNameFromUserId(event.sender); + this.logger.error({ msg: 'Event validation failed', - origin, + transactionOrigin: origin, + eventOrigin, - event, + // event, // consider excluding or redacting content to avoid PII leakage err, });Add this helper once (see suggestion below) to avoid duplicating parsing logic.
📜 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 (1)
packages/federation-sdk/src/services/event.service.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/federation-sdk/src/services/event.service.ts (2)
packages/room/src/manager/event-wrapper.ts (2)
event(105-114)origin(85-91)packages/room/src/types/v3-11.ts (1)
Pdu(729-729)
🔇 Additional comments (1)
packages/federation-sdk/src/services/event.service.ts (1)
230-236: Good guard: explicitly reject third‑party invites.Early rejection here is correct and avoids deeper processing on unsupported flows.
| const origin = event.sender.split(':').pop(); | ||
| if (!origin) { | ||
| throw new Error('Event sender is missing domain'); | ||
| } | ||
|
|
There was a problem hiding this comment.
Bug: Incorrect server name extraction from sender (breaks signature verification, esp. with ports).
Using split(':').pop() returns only the last segment. For MXIDs like @alice:example.org:8448 this yields "8448", which is wrong and will cause signature lookups to fail or be bypassed incorrectly.
Use a single, canonical extractor that returns everything after the first colon. Prefer reusing one helper across the codebase.
Apply this diff:
- const origin = event.sender.split(':').pop();
- if (!origin) {
- throw new Error('Event sender is missing domain');
- }
+ const origin = this.extractServerNameFromUserId(event.sender);
+ if (!origin) {
+ throw new Error('Event sender is missing domain');
+ }Add this helper to the class (or adapt your existing extractDomain to keep the port):
// Helper: returns everything after the first ":" in the MXID, preserving optional ":port"
private extractServerNameFromUserId(userId: string): string {
const idx = userId.indexOf(':');
return idx > 0 ? userId.slice(idx + 1) : '';
}Note: event-wrapper.ts uses an extractDomain helper; align both to avoid inconsistencies and ensure ports are preserved for signature keys. Based on relevant code in event-wrapper.ts.
🤖 Prompt for AI Agents
In packages/federation-sdk/src/services/event.service.ts around lines 238 to
242, the current origin extraction using event.sender.split(':').pop()
incorrectly returns only the last segment (breaking MXIDs with ports); replace
this with a helper that returns everything after the first ":" (or adapt the
existing extractDomain to preserve ":port") and use that helper here so e.g.
"@alice:example.org:8448" yields "example.org:8448"; add the helper to the class
(or update the shared extractDomain) and change the code to call it instead of
split(':').pop() to ensure correct server name extraction for signature lookups.
Summary by CodeRabbit
Bug Fixes
Refactor
Chores