feat: adds canAccessMedia middleware and upload repo #185
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughRefactors core auth signature verification; replaces fetch with richer content-type and multipart handling; adds Upload and MatrixBridgedRoom collections and repositories; extends federation request flow to return binary/multipart results and adds requestBinaryData; updates media download to use requestBinaryData; extends EventAuthorizationService with media-access checks and auth-header entrypoint. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant EventAuth as EventAuthorizationService
participant SigVerify as SignatureVerificationService
participant UploadRepo as UploadRepository
participant BridgeRepo as MatrixBridgedRoomRepository
participant State as StateService
participant Events as EventService
participant FedReq as FederationRequestService
participant Remote as RemoteServer
rect rgb(237,246,255)
note right of Client: Client requests media with Authorization header
Client->>EventAuth: canAccessMediaFromAuthorizationHeader(mediaId, auth, method, uri, body?)
EventAuth->>SigVerify: verifyRequestSignature(auth, method, uri, body, destination=serverName)
SigVerify-->>EventAuth: { origin } / error
end
alt Signature invalid
EventAuth-->>Client: { authorized: false, errorCode: M_UNAUTHORIZED }
else Signature valid
EventAuth->>UploadRepo: findRocketChatRoomIdByMediaId(mediaId)
UploadRepo-->>EventAuth: rid|null
alt rid found
EventAuth->>BridgeRepo: findMatrixRoomId(rid)
BridgeRepo-->>EventAuth: mri|null
alt mri found
EventAuth->>State: checkServerAcl(mri)
State-->>EventAuth: acl
EventAuth->>Events: isServerInRoom(origin, mri)?
Events-->>EventAuth: true/false
alt allowed
EventAuth-->>Client: { authorized: true }
Client->>FedReq: requestBinaryData(GET, serverName, endpoint)
FedReq->>Remote: signed HTTP request
Remote-->>FedReq: multipart/ok/json or redirect or binary
FedReq-->>Client: Buffer | JSON | null
else forbidden
EventAuth-->>Client: { authorized: false, errorCode: M_FORBIDDEN }
end
else mri missing
EventAuth-->>Client: { authorized: false, errorCode: M_FORBIDDEN }
end
else rid missing
EventAuth-->>Client: { authorized: false, errorCode: M_FORBIDDEN }
end
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## refactor/media-service #185 +/- ##
==========================================================
- Coverage 44.53% 44.03% -0.50%
==========================================================
Files 80 82 +2
Lines 9720 9862 +142
==========================================================
+ Hits 4329 4343 +14
- Misses 5391 5519 +128 ☔ 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/core/src/utils/authentication.ts (1)
37-39: Destination must be optional per Matrix spec; don’t reject headers missing it
extractSignaturesFromHeaderthrows whendestinationis absent, butverifyRequestSignaturetreats it as optional. Makedestinationoptional here to avoid spurious 401s.Apply:
- if ([origin, destination, key, signature].some((value) => !value)) { + if ([origin, key, signature].some((value) => !value)) { throw new Error('Invalid authorization header'); }
🧹 Nitpick comments (8)
packages/core/src/utils/authentication.ts (1)
72-80: Rename parameterhashtosignatureBase64for clarityIt carries the
sigfrom the header, not a hash. Improves readability across callers.packages/federation-sdk/src/repositories/upload.repository.ts (2)
19-25: Project only needed fields and prepare for scale (add index externally)Avoid pulling full upload docs; fetch only
rid. Also ensure an index onfederation.mediaIdexists (migration/init).Apply:
- const upload = await this.collection.findOne({ - 'federation.mediaId': mediaId, - }); + const upload = await this.collection.findOne( + { 'federation.mediaId': mediaId }, + { projection: { rid: 1 } }, + );Migration/index suggestion (outside this file):
await db.collection('rocketchat_uploads') .createIndex({ 'federation.mediaId': 1 }, { name: 'idx_uploads_fed_mediaId', unique: true });
19-25: Optional: support lookup by MXC URI as fallbackIf historical records lack
mediaIdbut havefederation.mxcUri, consider a fallback query.packages/federation-sdk/src/repositories/matrix-bridged-room.repository.ts (1)
17-23: Project onlymriand indexridTrim the payload and ensure a supporting index (
rid:1) via migration.Apply:
- const bridgedRoom = await this.collection.findOne({ - rid: rocketChatRoomId, - }); + const bridgedRoom = await this.collection.findOne( + { rid: rocketChatRoomId }, + { projection: { mri: 1 } }, + );Migration/index suggestion (outside this file):
await db.collection('rocketchat_matrix_bridged_rooms') .createIndex({ rid: 1 }, { name: 'idx_mbr_rid' });packages/federation-sdk/src/container.ts (1)
89-101: Create required indexes at startup/migration timeRegistering collections is fine; add migration/initialization to create:
- rocketchat_uploads: {'federation.mediaId': 1} unique
- rocketchat_matrix_bridged_rooms: {rid: 1}
Would you like me to open a follow-up PR with a small migration to create these indexes?
packages/federation-sdk/src/services/event-authorization.service.ts (3)
154-167: Remove dead code:validateAuthorizationHeaderthrows on invalid; the!isValidpath is unreachableSimplify and rely on exceptions.
Apply:
- const isValid = await validateAuthorizationHeader( + await validateAuthorizationHeader( origin, publicKey.verify_keys[key].key, - actualDestination, + actualDestination, method, uri, signature, body, ); - if (!isValid) { - this.logger.warn(`Invalid signature from ${origin}`); - return; - } return origin;
237-285: Improve patch coverage with focused tests for auth header and ACL pathsAdd unit tests for:
- accept header without
destination(using actual server name)- reject non-ed25519 keys
- invalid signature → M_UNAUTHORIZED
- canAccessMedia: room not found, bridged room missing, ACL deny, server in room, world_readable
I can scaffold tests with DI mocks for StateService/EventService/UploadRepository/MatrixBridgedRoomRepository.
369-475: Minor: dedupe “not found in any room” logs and include room IDs for quicker tracingCurrent messages for missing rcRoomId/matrixRoomId are identical; include which lookup failed and the IDs when available.
- this.logger.debug(`Media ${mediaId} not found in any room`); + this.logger.debug(`Media ${mediaId}: no Upload.rid found for mediaId`); ... - this.logger.debug(`Media ${mediaId} not found in any room`); + this.logger.debug(`Media ${mediaId}: no Matrix room mapping found for rid ${rcRoomId}`);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/core/src/utils/authentication.ts(1 hunks)packages/federation-sdk/src/container.ts(2 hunks)packages/federation-sdk/src/repositories/matrix-bridged-room.repository.ts(1 hunks)packages/federation-sdk/src/repositories/upload.repository.ts(1 hunks)packages/federation-sdk/src/services/event-authorization.service.ts(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
packages/core/src/utils/authentication.ts (1)
packages/room/src/manager/event-wrapper.ts (1)
origin(82-84)
packages/federation-sdk/src/repositories/matrix-bridged-room.repository.ts (2)
packages/federation-sdk/src/repositories/upload.repository.ts (1)
singleton(13-26)packages/federation-sdk/src/services/event-authorization.service.ts (1)
singleton(16-476)
packages/federation-sdk/src/repositories/upload.repository.ts (2)
packages/federation-sdk/src/repositories/matrix-bridged-room.repository.ts (1)
singleton(10-24)packages/federation-sdk/src/services/event-authorization.service.ts (1)
singleton(16-476)
packages/federation-sdk/src/container.ts (2)
packages/federation-sdk/src/repositories/upload.repository.ts (1)
Upload(4-11)packages/federation-sdk/src/repositories/matrix-bridged-room.repository.ts (1)
MatrixBridgedRoom(4-8)
packages/federation-sdk/src/services/event-authorization.service.ts (3)
packages/core/src/utils/authentication.ts (1)
validateAuthorizationHeader(72-109)packages/room/src/manager/event-wrapper.ts (2)
origin(82-84)roomId(72-74)packages/federation-sdk/src/services/config.service.ts (1)
serverName(92-94)
🔇 Additional comments (2)
packages/federation-sdk/src/services/event-authorization.service.ts (2)
292-346: ACL: empty allow list -> deny (spec-compliant)Matrix spec's m.room.server_acl decision order (no ACL → allow; check allow_ip_literals; deny list; allow list; otherwise deny) means a present ACL with an empty allow list denies all — your implementation (deny when allow.length === 0) matches the spec. (spec.matrix.org)
116-136: Upstream bug dependency: header parser currently rejects missing destinationextractSignaturesFromHeader (upstream/core) requires a destination while this code treats it as optional — repo search shows the only call is packages/federation-sdk/src/services/event-authorization.service.ts:129; this will work once the core parser accepts headers without destination.
| const signature = Uint8Array.from(atob(hash as string), (c) => | ||
| c.charCodeAt(0), | ||
| ); | ||
| const signingKeyBytes = Uint8Array.from(atob(signingKey as string), (c) => | ||
| c.charCodeAt(0), | ||
| ); | ||
| const messageBytes = new TextEncoder().encode(canonicalJson); | ||
| const isValid = nacl.sign.detached.verify( | ||
| messageBytes, | ||
| signature, | ||
| signingKeyBytes, | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use Node-safe base64 decoding and support base64url (Matrix uses unpadded/base64url)
atob isn’t guaranteed in Node and doesn’t handle base64url. Normalize to base64 and decode with Buffer.
Apply:
- const signature = Uint8Array.from(atob(hash as string), (c) =>
- c.charCodeAt(0),
- );
- const signingKeyBytes = Uint8Array.from(atob(signingKey as string), (c) =>
- c.charCodeAt(0),
- );
- const messageBytes = new TextEncoder().encode(canonicalJson);
- const isValid = nacl.sign.detached.verify(
- messageBytes,
- signature,
- signingKeyBytes,
- );
+ const toB64 = (s: string) =>
+ s.replace(/-/g, '+').replace(/_/g, '/').padEnd(Math.ceil(s.length / 4) * 4, '=');
+ const signature = Uint8Array.from(Buffer.from(toB64(hash), 'base64'));
+ const signingKeyBytes = Uint8Array.from(Buffer.from(toB64(signingKey), 'base64'));
+ const messageBytes = new TextEncoder().encode(canonicalJson);
+ const isValid = nacl.sign.detached.verify(messageBytes, signature, signingKeyBytes);📝 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.
| const signature = Uint8Array.from(atob(hash as string), (c) => | |
| c.charCodeAt(0), | |
| ); | |
| const signingKeyBytes = Uint8Array.from(atob(signingKey as string), (c) => | |
| c.charCodeAt(0), | |
| ); | |
| const messageBytes = new TextEncoder().encode(canonicalJson); | |
| const isValid = nacl.sign.detached.verify( | |
| messageBytes, | |
| signature, | |
| signingKeyBytes, | |
| ); | |
| const toB64 = (s: string) => | |
| s.replace(/-/g, '+').replace(/_/g, '/').padEnd(Math.ceil(s.length / 4) * 4, '='); | |
| const signature = Uint8Array.from(Buffer.from(toB64(hash), 'base64')); | |
| const signingKeyBytes = Uint8Array.from(Buffer.from(toB64(signingKey), 'base64')); | |
| const messageBytes = new TextEncoder().encode(canonicalJson); | |
| const isValid = nacl.sign.detached.verify(messageBytes, signature, signingKeyBytes); |
🤖 Prompt for AI Agents
In packages/core/src/utils/authentication.ts around lines 89 to 100, the code
uses atob for base64 decoding which is not Node-safe and does not support
base64url; normalize the incoming hash and signingKey strings to standard base64
by replacing '-' with '+' and '_' with '/' and padding with '=' to a multiple of
4, then decode using Buffer.from(..., 'base64') and convert the resulting Buffer
to Uint8Array for signature and signing key bytes before calling
nacl.sign.detached.verify; update both decodes (signature and signingKey) this
way so Node and unpadded base64url inputs are handled correctly.
Works along with RocketChat/Rocket.Chat#36926.
For anyone testing with Synapse as HS1, remember to add these two properties to the YAML spec file:
Summary by CodeRabbit
New Features
Improvements