feat: implement server factory pattern and enhance server structure #5288
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| app.use('/api', (req, res, next) => { | ||
| apiKeyAuthMiddleware(req, res, next); | ||
| }); |
Check failure
Code scanning / CodeQL
Missing rate limiting High
Copilot Autofix
AI 6 months ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
| (req: express.Request, res: express.Response) => { | ||
| const agentId = req.params.agentId as string; | ||
| const filename = req.params.filename as string; | ||
| const uuidRegex = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i; | ||
|
|
||
| if (!uuidRegex.test(agentId)) { | ||
| return res.status(400).json({ error: 'Invalid agent ID format' }); | ||
| } | ||
|
|
||
| const sanitizedFilename = basename(filename); | ||
| const agentUploadsPath = join(uploadsBasePath, agentId); | ||
| const filePath = join(agentUploadsPath, sanitizedFilename); | ||
|
|
||
| if (!filePath.startsWith(agentUploadsPath)) { | ||
| return res.status(403).json({ error: 'Access denied' }); | ||
| } | ||
|
|
||
| if (!fs.existsSync(filePath)) { | ||
| return res.status(404).json({ error: 'File does not exist!!!!!!!' }); | ||
| } | ||
|
|
||
| res.sendFile(sanitizedFilename, { root: agentUploadsPath }, (err) => { | ||
| if (err) { | ||
| if (err.message === 'Request aborted') { | ||
| logger.warn(`[MEDIA] Download aborted: ${req.originalUrl}`); | ||
| } else if (!res.headersSent) { | ||
| logger.warn(`[MEDIA] File not found: ${agentUploadsPath}/${sanitizedFilename}`); | ||
| res.status(404).json({ error: 'File not found' }); | ||
| } | ||
| } else { | ||
| logger.debug(`[MEDIA] Successfully served: ${sanitizedFilename}`); | ||
| } | ||
| }); | ||
| } |
Check failure
Code scanning / CodeQL
Missing rate limiting High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 6 months ago
To address the issue, we will introduce rate limiting to the affected route handler using the express-rate-limit package. This package allows us to define a rate-limiting policy, such as limiting the number of requests per minute for the route. The fix involves:
- Importing the
express-rate-limitpackage. - Creating a rate limiter instance with appropriate configuration (e.g., maximum requests per minute).
- Applying the rate limiter middleware to the affected route handler.
This ensures that the route is protected against DoS attacks while maintaining its functionality.
| @@ -230,4 +230,12 @@ | ||
| private setupAgentMediaRoute(app: express.Application, uploadsBasePath: string): void { | ||
| const rateLimit = require('express-rate-limit'); | ||
| const agentMediaRateLimiter = rateLimit({ | ||
| windowMs: 15 * 60 * 1000, // 15 minutes | ||
| max: 100, // Limit each IP to 100 requests per windowMs | ||
| message: { error: 'Too many requests, please try again later.' }, | ||
| }); | ||
|
|
||
| app.get( | ||
| '/media/uploads/agents/:agentId/:filename', | ||
| agentMediaRateLimiter, | ||
| // @ts-expect-error - this is a valid express route |
| return res.status(404).json({ error: 'File does not exist!!!!!!!' }); | ||
| } | ||
|
|
||
| res.sendFile(sanitizedFilename, { root: agentUploadsPath }, (err) => { |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 6 months ago
To fix the issue, we need to ensure that the constructed file path is properly normalized and validated to prevent path traversal attacks. This involves:
- Using
path.resolveto normalize the constructed path (agentUploadsPathandfilePath) and remove any..segments. - Using
fs.realpathSyncto resolve symbolic links and ensure the path is contained within the intended directory. - Adding a strict check to verify that the normalized path starts with the intended base directory (
uploadsBasePath).
The changes will be made in the setupAgentMediaRoute method, specifically around the construction and validation of agentUploadsPath and filePath.
| @@ -243,4 +243,4 @@ | ||
| const sanitizedFilename = basename(filename); | ||
| const agentUploadsPath = join(uploadsBasePath, agentId); | ||
| const filePath = join(agentUploadsPath, sanitizedFilename); | ||
| const agentUploadsPath = fs.realpathSync(path.resolve(uploadsBasePath, agentId)); | ||
| const filePath = fs.realpathSync(path.resolve(agentUploadsPath, sanitizedFilename)); | ||
|
|
| (req: express.Request<{ agentId: string; filename: string }>, res: express.Response) => { | ||
| const agentId = req.params.agentId; | ||
| const filename = req.params.filename; | ||
| const uuidRegex = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i; | ||
|
|
||
| if (!uuidRegex.test(agentId)) { | ||
| return res.status(400).json({ error: 'Invalid agent ID format' }); | ||
| } | ||
|
|
||
| const sanitizedFilename = basename(filename); | ||
| const agentGeneratedPath = join(generatedBasePath, agentId); | ||
| const filePath = join(agentGeneratedPath, sanitizedFilename); | ||
|
|
||
| if (!filePath.startsWith(agentGeneratedPath)) { | ||
| return res.status(403).json({ error: 'Access denied' }); | ||
| } | ||
|
|
||
| res.sendFile(filePath, (err) => { | ||
| if (err) { | ||
| res.status(404).json({ error: 'File not found' }); | ||
| } | ||
| }); | ||
| } |
Check failure
Code scanning / CodeQL
Missing rate limiting High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 6 months ago
To fix the issue, we will introduce rate limiting to the /media/generated/:agentId/:filename route using the express-rate-limit package. This package allows us to define a maximum number of requests per time window, effectively mitigating the risk of DoS attacks. The rate limiter will be applied specifically to the affected route.
Steps:
- Install the
express-rate-limitpackage if not already installed. - Import the package in the file.
- Define a rate limiter configuration with appropriate limits (e.g., 100 requests per 15 minutes).
- Apply the rate limiter middleware to the
/media/generated/:agentId/:filenameroute.
| @@ -7,2 +7,3 @@ | ||
| import { apiKeyAuthMiddleware } from './middleware/auth.js'; | ||
| import rateLimit from 'express-rate-limit'; | ||
| import type { ServerOptions, ServerMiddleware } from '../types/server.js'; | ||
| @@ -271,4 +272,10 @@ | ||
| private setupGeneratedMediaRoute(app: express.Application, generatedBasePath: string): void { | ||
| const generatedMediaRateLimiter = rateLimit({ | ||
| windowMs: 15 * 60 * 1000, // 15 minutes | ||
| max: 100, // max 100 requests per windowMs | ||
| }); | ||
|
|
||
| app.get( | ||
| '/media/generated/:agentId/:filename', | ||
| generatedMediaRateLimiter, | ||
| // @ts-expect-error - this is a valid express route |
| (req: express.Request<{ channelId: string; filename: string }>, res: express.Response) => { | ||
| const channelId = req.params.channelId as string; | ||
| const filename = req.params.filename as string; | ||
| const uuidRegex = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i; | ||
|
|
||
| if (!uuidRegex.test(channelId)) { | ||
| res.status(400).json({ error: 'Invalid channel ID format' }); | ||
| return; | ||
| } | ||
|
|
||
| const sanitizedFilename = basename(filename); | ||
| const channelUploadsPath = join(uploadsBasePath, 'channels', channelId); | ||
| const filePath = join(channelUploadsPath, sanitizedFilename); | ||
|
|
||
| if (!filePath.startsWith(channelUploadsPath)) { | ||
| res.status(403).json({ error: 'Access denied' }); | ||
| return; | ||
| } | ||
|
|
||
| res.sendFile(filePath, (err) => { | ||
| if (err) { | ||
| logger.warn(`[STATIC] Channel media file not found: ${filePath}`, err); | ||
| if (!res.headersSent) { | ||
| res.status(404).json({ error: 'File not found' }); | ||
| } | ||
| } else { | ||
| logger.debug(`[STATIC] Served channel media file: ${filePath}`); | ||
| } | ||
| }); | ||
| } |
Check failure
Code scanning / CodeQL
Missing rate limiting High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 6 months ago
To address the issue, we will introduce rate limiting to the /media/uploads/channels/:channelId/:filename route using the express-rate-limit package. This package allows us to define a maximum number of requests per time window, effectively mitigating the risk of DoS attacks.
Steps to fix:
- Install the
express-rate-limitpackage if not already installed. - Import the package in the file.
- Define a rate limiter configuration with appropriate limits (e.g., 100 requests per 15 minutes).
- Apply the rate limiter middleware specifically to the
/media/uploads/channels/:channelId/:filenameroute.
| @@ -8,2 +8,3 @@ | ||
| import type { ServerOptions, ServerMiddleware } from '../types/server.js'; | ||
| import rateLimit from 'express-rate-limit'; | ||
|
|
||
| @@ -301,4 +302,11 @@ | ||
| private setupChannelMediaRoute(app: express.Application, uploadsBasePath: string): void { | ||
| const channelMediaRateLimiter = rateLimit({ | ||
| windowMs: 15 * 60 * 1000, // 15 minutes | ||
| max: 100, // max 100 requests per windowMs | ||
| message: { error: 'Too many requests, please try again later.' }, | ||
| }); | ||
|
|
||
| app.get( | ||
| '/media/uploads/channels/:channelId/:filename', | ||
| channelMediaRateLimiter, | ||
| (req: express.Request<{ channelId: string; filename: string }>, res: express.Response) => { |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Bug: TypeScript Type Safety Violation
The getServersForAgent method uses an incorrect as never type cast when pushing server.id to the serverIds array. This cast is illogical as never represents unreachable values, effectively bypassing TypeScript's type safety. The serverIds array is implicitly typed as any[] while the method returns Promise<UUID[]>, and server.id is expected to be a UUID. This can hide type errors and lead to runtime issues.
packages/server/src/services/database.ts#L177-L178
eliza/packages/server/src/services/database.ts
Lines 177 to 178 in 170a0ed
Was this report helpful? Give feedback by reacting with 👍 or 👎
|
Aborting this because Next will replace most of this. |
Server Architecture Refactoring - Factory Pattern Implementation
Risks
Medium
What could be affected:
Background
What does this PR do?
This PR implements a comprehensive refactoring of the ElizaOS server architecture:
Layer Pattern Implementation: Breaks down the 1000+ line
AgentServerclass into focused services:DatabaseService- Database operations and migrationsAgentService- Agent lifecycle and plugin managementMiddlewareService- Express middleware configurationHttpService- HTTP server, routing, and Socket.IOFactory Pattern: Introduces
createElizaServer()factory for clean server instantiation with dependency injectionPlugin Loading: Extracts and implements complete plugin loading logic from CLI to server, enabling dynamic plugin loading based on character configuration
Architecture Cleanup: Removes inappropriate cross-package dependencies (server no longer depends on CLI)
API-Only Server: Converts server to pure API backend, removing static file serving for better separation of concerns
What kind of change is this?
Features (non-breaking change which adds functionality)
Improvements (misc. changes to existing features)
Documentation changes needed?
My changes require a change to the project documentation.
Documentation updates needed:
Testing
Where should a reviewer start?
packages/serverbuilds successfullycreateElizaServer()/api/*routes work correctlyDetailed testing steps
Build and Start Tests
API Testing
Plugin Loading Verification
character.pluginsarrayelizaos start --character test-char.jsonKey Files Changed
New Architecture Files
packages/server/src/server/factory.ts- Factory pattern implementationpackages/server/src/server/server.ts- Main server class using compositionpackages/server/src/services/database.ts- Database service layerpackages/server/src/services/agent.ts- Agent management servicepackages/server/src/services/middleware.ts- Express middleware servicepackages/server/src/services/http.ts- HTTP and Socket.IO servicepackages/server/src/utils/plugin-loader.ts- Extracted plugin loading logicFile Organization
packages/server/src/utils/character-loader.ts(renamed fromloader.ts)services/middleware/directoryutils/structureUpdated Entry Points
packages/server/src/index.ts- Clean public API exportspackages/cli/src/commands/start/actions/server-start.ts- Uses factory patternBreaking Changes
Server Package API:
AgentServerinstantiationcreateElizaServer()factory functionFrontend Serving:
//Plugin Loading:
Backward Compatibility
AgentServerclass still exported for compatibilityTesting Results
Successful Tests:
Architecture Validation: