feat: add PreventedToFetchSelfServer error handling in FederationRequ… #328
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a new exported error class Changes
Sequence Diagram(s)(omitted — change is a local guard and does not introduce multi-component sequential flows) Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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 #328 +/- ##
==========================================
+ Coverage 52.29% 52.32% +0.03%
==========================================
Files 97 97
Lines 13153 13169 +16
==========================================
+ Hits 6878 6891 +13
- Misses 6275 6278 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR adds safeguard logic to prevent the federation service from making requests to its own server. It introduces a new error class PreventedToFetchSelfServer and implements a validation check in the request method to throw this error when attempting to fetch from the self server.
Changes:
- Added
PreventedToFetchSelfServererror class for self-server fetch prevention - Implemented validation in the
requestmethod to check and prevent self-server requests
Comments suppressed due to low confidence (1)
packages/federation-sdk/src/services/federation-request.service.ts:217
- The self-server check is missing in the 'requestBinaryData' method. This method also accepts a targetServer parameter and makes requests, so it should have the same validation to prevent fetching from the self server for consistency and completeness.
async requestBinaryData(
method: string,
targetServer: string,
endpoint: string,
queryParams?: Record<string, string>,
): Promise<MultipartResult> {
const response = await this.makeSignedRequest({
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/federation-sdk/src/services/federation-request.service.ts
Outdated
Show resolved
Hide resolved
packages/federation-sdk/src/services/federation-request.service.ts
Outdated
Show resolved
Hide resolved
packages/federation-sdk/src/services/federation-request.service.ts
Outdated
Show resolved
Hide resolved
| if (targetServer === this.configService.getConfig('serverName')) { | ||
| throw new PreventedToFetchSelfServer( | ||
| 'Not possible to fetch the self server', | ||
| ); | ||
| } |
There was a problem hiding this comment.
The new self-server validation logic lacks test coverage. The test file 'federation-request.service.spec.ts' contains comprehensive tests for other methods, but no tests verify this validation behavior. Add tests to ensure the error is thrown when targetServer equals the configured serverName.
![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
🤖 Fix all issues with AI agents
In `@packages/federation-sdk/src/services/federation-request.service.ts`:
- Around line 148-152: The self-server prevention guard is only in request() and
is bypassed by requestBinaryData() which calls makeSignedRequest() directly;
move the guard into makeSignedRequest() so all outgoing federation calls are
blocked from targeting the local server (use the same PreventedToFetchSelfServer
exception and compare targetServer to
this.configService.getConfig('serverName')), then remove the duplicate guard
from request(); alternatively, if you prefer less-wide changes, add the
identical check from request() into requestBinaryData() to throw
PreventedToFetchSelfServer when targetServer equals the configured server name.
📜 Review details
Configuration used: Organization 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/federation-request.service.ts
🔇 Additional comments (1)
packages/federation-sdk/src/services/federation-request.service.ts (1)
44-49: LGTM!The custom error class follows the standard pattern for extending
Errorin TypeScript, correctly setting thenameproperty for proper error identification.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
…e.ts Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…e.ts Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/federation-sdk/src/services/federation-request.service.ts`:
- Line 51: The module exports an alias "PreventedToFetchSelfServer" via "export
{ SelfServerFetchError as PreventedToFetchSelfServer }" but later code tries to
reference PreventedToFetchSelfServer as a local binding—create a real local
binding or stop using the alias: either replace internal references to
PreventedToFetchSelfServer with SelfServerFetchError, or add a local alias
(e.g., const PreventedToFetchSelfServer = SelfServerFetchError;) and keep the
export; update usages accordingly so the symbol referenced in this module is a
real local binding.
♻️ Duplicate comments (1)
packages/federation-sdk/src/services/federation-request.service.ts (1)
213-229: Self-server check is still bypassed byrequestBinaryData.This method calls
makeSignedRequest()directly, bypassing the guard inrequest(). This concern was previously raised and remains unaddressed.
📜 Review details
Configuration used: Organization 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/federation-request.service.ts
🧰 Additional context used
🪛 GitHub Actions: my-workflow
packages/federation-sdk/src/services/federation-request.service.ts
[error] 151-151: TS2304: Cannot find name 'PreventedToFetchSelfServer'.
🔇 Additional comments (1)
packages/federation-sdk/src/services/federation-request.service.ts (1)
44-49: LGTM!The error class follows best practices for custom errors in TypeScript—extending
Errorand settingthis.namefor proper stack trace identification.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
packages/federation-sdk/src/services/federation-request.service.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/federation-sdk/src/services/federation-request.service.ts`:
- Around line 44-49: SelfServerFetchError is not re-exported from the package
public API; update the module's index.ts to add SelfServerFetchError to the
exported errors (alongside FederationRequestError and the other error classes)
so consumers can import and catch SelfServerFetchError by type—i.e., add
SelfServerFetchError to the existing export list that re-exports from
services/federation-request.service.
📜 Review details
Configuration used: Organization 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/federation-request.service.ts
🔇 Additional comments (2)
packages/federation-sdk/src/services/federation-request.service.ts (2)
149-153: LGTM! Self-server guard correctly prevents self-targeted requests.The guard is properly placed early in the method (before query string construction), providing a fast-fail behavior. The error message is clear and actionable.
218-222: LGTM! Addresses the previous bypass concern.This guard ensures
requestBinaryData()also prevents self-targeted requests, addressing the previously identified gap where this method bypassed the check by callingmakeSignedRequest()directly.Note: Per the Codecov report, the patch coverage shows 3 lines missing—likely these guard blocks. Consider adding tests that verify
SelfServerFetchErroris thrown whentargetServermatchesserverNamein bothrequest()andrequestBinaryData().
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| export class SelfServerFetchError extends Error { | ||
| constructor(message: string) { | ||
| super(message); | ||
| this.name = 'SelfServerFetchError'; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify if SelfServerFetchError is exported in the package's public API
# Search for SelfServerFetchError export in index.ts files
echo "=== Checking index.ts exports for SelfServerFetchError ==="
rg -n "SelfServerFetchError" --type ts -g '**/index.ts'
echo ""
echo "=== Checking how FederationRequestError is exported (for comparison) ==="
rg -n "FederationRequestError" --type ts -g '**/index.ts'
echo ""
echo "=== Listing all index.ts files in federation-sdk package ==="
fd -t f 'index.ts' packages/federation-sdk/Repository: RocketChat/homeserver
Length of output: 416
🏁 Script executed:
cat -n packages/federation-sdk/src/index.tsRepository: RocketChat/homeserver
Length of output: 7078
Export SelfServerFetchError in the public API
SelfServerFetchError is not exported in packages/federation-sdk/src/index.ts. Add it alongside FederationRequestError and other error classes so consumers can catch it by type:
export { SelfServerFetchError, FederationRequestError } from './services/federation-request.service';
🤖 Prompt for AI Agents
In `@packages/federation-sdk/src/services/federation-request.service.ts` around
lines 44 - 49, SelfServerFetchError is not re-exported from the package public
API; update the module's index.ts to add SelfServerFetchError to the exported
errors (alongside FederationRequestError and the other error classes) so
consumers can import and catch SelfServerFetchError by type—i.e., add
SelfServerFetchError to the existing export list that re-exports from
services/federation-request.service.
…estService
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.