(chore): Refactor generation to reflect handler pattern #3376
Conversation
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. |
|
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 Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
packages/agent/src/api.ts
Outdated
| router.get('/storage', async (_req, res) => { | ||
| try { | ||
| const uploadDir = path.join(process.cwd(), "data", "characters"); | ||
| const files = await fs.promises.readdir(uploadDir); | ||
| res.json({ files }); | ||
| } catch (error) { | ||
| res.status(500).json({ error: error.message }); | ||
| } | ||
| }); |
Check failure
Code scanning / CodeQL
Missing rate limiting High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix AI 6 days ago
To fix the problem, we will introduce a rate-limiting middleware to the Express application. We will use the express-rate-limit package to limit the number of requests that can be made to the /storage route within a specified time window. This will help prevent potential DoS attacks by limiting the rate at which requests are accepted.
- Install the
express-rate-limitpackage. - Import the
express-rate-limitpackage in thepackages/agent/src/api.tsfile. - Set up a rate limiter with appropriate configuration (e.g., maximum of 100 requests per 15 minutes).
- Apply the rate limiter to the
/storageroute.
| @@ -14,2 +14,3 @@ | ||
| import path from "node:path"; | ||
| import rateLimit from "express-rate-limit"; | ||
| import type { CharacterServer } from "./server"; | ||
| @@ -53,2 +54,7 @@ | ||
|
|
||
| const storageRateLimiter = rateLimit({ | ||
| windowMs: 15 * 60 * 1000, // 15 minutes | ||
| max: 100, // limit each IP to 100 requests per windowMs | ||
| }); | ||
|
|
||
| router.use(cors()); | ||
| @@ -79,3 +85,3 @@ | ||
|
|
||
| router.get('/storage', async (_req, res) => { | ||
| router.get('/storage', storageRateLimiter, async (_req, res) => { | ||
| try { |
| @@ -31,3 +31,4 @@ | ||
| "yargs": "17.7.2", | ||
| "minipass": "7.1.2" | ||
| "minipass": "7.1.2", | ||
| "express-rate-limit": "^7.5.0" | ||
| }, |
| Package | Version | Security advisories |
| express-rate-limit (npm) | 7.5.0 | None |
packages/agent/src/api.ts
Outdated
| router.post("/agents/:agentId/set", async (req, res) => { | ||
| const { agentId } = validateUUIDParams(req.params, res) ?? { | ||
| agentId: null, | ||
| }; | ||
| if (!agentId) return; | ||
|
|
||
| let agent: AgentRuntime = agents.get(agentId); | ||
|
|
||
| // update character | ||
| if (agent) { | ||
| // stop agent | ||
| agent.stop(); | ||
| directClient.unregisterAgent(agent); | ||
| // if it has a different name, the agentId will change | ||
| } | ||
|
|
||
| // stores the json data before it is modified with added data | ||
| const characterJson = { ...req.body }; | ||
|
|
||
| // load character from body | ||
| const character = req.body; | ||
| try { | ||
| validateCharacterConfig(character); | ||
| } catch (e) { | ||
| logger.error(`Error parsing character: ${e}`); | ||
| res.status(400).json({ | ||
| success: false, | ||
| message: e.message, | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| // start it up (and register it) | ||
| try { | ||
| agent = await directClient.startAgent(character); | ||
| logger.log(`${character.name} started`); | ||
| } catch (e) { | ||
| logger.error(`Error starting agent: ${e}`); | ||
| res.status(500).json({ | ||
| success: false, | ||
| message: e.message, | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| if (process.env.USE_CHARACTER_STORAGE === "true") { | ||
| try { | ||
| const filename = `${agent.agentId}.json`; | ||
| const uploadDir = path.join( | ||
| process.cwd(), | ||
| "data", | ||
| "characters" | ||
| ); | ||
| const filepath = path.join(uploadDir, filename); | ||
| await fs.promises.mkdir(uploadDir, { recursive: true }); | ||
| await fs.promises.writeFile( | ||
| filepath, | ||
| JSON.stringify( | ||
| { ...characterJson, id: agent.agentId }, | ||
| null, | ||
| 2 | ||
| ) | ||
| ); | ||
| logger.info( | ||
| `Character stored successfully at ${filepath}` | ||
| ); | ||
| } catch (error) { | ||
| logger.error( | ||
| `Failed to store character: ${error.message}` | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| res.json({ | ||
| id: character.id, | ||
| character: character, | ||
| }); | ||
| }); |
Check failure
Code scanning / CodeQL
Missing rate limiting High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix AI 6 days ago
To fix the problem, we will introduce rate limiting to the Express application using the express-rate-limit package. This will help prevent denial-of-service attacks by limiting the number of requests a client can make within a specified time window. We will set up a rate limiter and apply it to the routes that perform expensive operations.
- Install the
express-rate-limitpackage. - Import the
express-rate-limitpackage in the file. - Set up a rate limiter with appropriate configuration.
- Apply the rate limiter to the routes that perform expensive operations.
| @@ -14,2 +14,3 @@ | ||
| import path from "node:path"; | ||
| import rateLimit from "express-rate-limit"; | ||
| import type { CharacterServer } from "./server"; | ||
| @@ -88,2 +89,9 @@ | ||
| }); | ||
| // set up rate limiter: maximum of 100 requests per 15 minutes | ||
| const limiter = rateLimit({ | ||
| windowMs: 15 * 60 * 1000, // 15 minutes | ||
| max: 100, // max 100 requests per windowMs | ||
| }); | ||
|
|
||
| router.use(limiter); | ||
|
|
| @@ -31,3 +31,4 @@ | ||
| "yargs": "17.7.2", | ||
| "minipass": "7.1.2" | ||
| "minipass": "7.1.2", | ||
| "express-rate-limit": "^7.5.0" | ||
| }, |
| Package | Version | Security advisories |
| express-rate-limit (npm) | 7.5.0 | None |
packages/agent/src/server.ts
Outdated
| async (req: CustomRequest, res: express.Response) => { | ||
| const audioFile = req.file; // Access the uploaded file using req.file | ||
| const agentId = req.params.agentId; | ||
|
|
||
| if (!audioFile) { | ||
| res.status(400).send("No audio file provided"); | ||
| return; | ||
| } | ||
|
|
||
| let runtime = this.agents.get(agentId); | ||
|
|
||
| // if runtime is null, look for runtime with the same name | ||
| if (!runtime) { | ||
| runtime = Array.from(this.agents.values()).find( | ||
| (a) => | ||
| a.character.name.toLowerCase() === | ||
| agentId.toLowerCase() | ||
| ); | ||
| } | ||
|
|
||
| if (!runtime) { | ||
| res.status(404).send("Agent not found"); | ||
| return; | ||
| } | ||
|
|
||
| const transcription = await runtime.getModelProviderManager().call(ModelType.AUDIO_TRANSCRIPTION, { | ||
| file: fs.createReadStream(audioFile.path), | ||
| model: "whisper-1", | ||
| }); | ||
|
|
||
| res.json(transcription); | ||
| } |
Check failure
Code scanning / CodeQL
Missing rate limiting High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix AI 6 days ago
To fix the problem, we need to introduce a rate-limiting middleware to the Express application. The express-rate-limit package is a well-known library that can be used to achieve this. We will configure the rate limiter to allow a maximum of 100 requests per 15 minutes and apply it to the route handlers that perform file system operations.
- Install the
express-rate-limitpackage. - Import the
express-rate-limitpackage in the file. - Configure the rate limiter with appropriate settings.
- Apply the rate limiter to the relevant route handlers.
| @@ -22,2 +22,3 @@ | ||
| import { z } from "zod"; | ||
| import rateLimit from "express-rate-limit"; | ||
| import { createApiRouter } from "./api.ts"; | ||
| @@ -115,2 +116,9 @@ | ||
| this.app = express(); | ||
|
|
||
| // Configure rate limiter: maximum of 100 requests per 15 minutes | ||
| const limiter = rateLimit({ | ||
| windowMs: 15 * 60 * 1000, // 15 minutes | ||
| max: 100, // limit each IP to 100 requests per windowMs | ||
| }); | ||
|
|
||
| this.app.use(cors()); | ||
| @@ -142,2 +150,3 @@ | ||
| "/:agentId/whisper", | ||
| limiter, | ||
| upload.single("file"), | ||
| @@ -179,2 +188,3 @@ | ||
| "/:agentId/message", | ||
| limiter, | ||
| upload.single("file"), |
| @@ -31,3 +31,4 @@ | ||
| "yargs": "17.7.2", | ||
| "minipass": "7.1.2" | ||
| "minipass": "7.1.2", | ||
| "express-rate-limit": "^7.5.0" | ||
| }, |
| Package | Version | Security advisories |
| express-rate-limit (npm) | 7.5.0 | None |
| } | ||
|
|
||
| const transcription = await runtime.getModelProviderManager().call(ModelType.AUDIO_TRANSCRIPTION, { | ||
| file: fs.createReadStream(audioFile.path), |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix AI 6 days ago
To fix the problem, we need to ensure that the file path used in fs.createReadStream is validated to prevent directory traversal attacks. We can achieve this by normalizing the path and ensuring it is within the expected upload directory.
- Normalize the
audioFile.pathusingpath.resolveto remove any ".." segments. - Check that the normalized path starts with the upload directory path.
- If the path is not valid, return an error response.
| @@ -168,4 +168,10 @@ | ||
|
|
||
| const uploadDir = path.join(process.cwd(), "data", "uploads"); | ||
| const normalizedPath = path.resolve(audioFile.path); | ||
| if (!normalizedPath.startsWith(uploadDir)) { | ||
| res.status(400).send("Invalid file path"); | ||
| return; | ||
| } | ||
| const transcription = await runtime.call(ModelType.TRANSCRIPTION, { | ||
| file: fs.createReadStream(audioFile.path), | ||
| file: fs.createReadStream(normalizedPath), | ||
| model: "whisper-1", |
packages/agent/src/server.ts
Outdated
| async (req: express.Request, res: express.Response) => { | ||
| const assetId = req.params.assetId; | ||
| const downloadDir = path.join( | ||
| process.cwd(), | ||
| "downloads", | ||
| assetId | ||
| ); | ||
|
|
||
| logger.log("Download directory:", downloadDir); | ||
|
|
||
| try { | ||
| logger.log("Creating directory..."); | ||
| await fs.promises.mkdir(downloadDir, { recursive: true }); | ||
|
|
||
| logger.log("Fetching file..."); | ||
| const fileResponse = await fetch( | ||
| `https://api.bageldb.ai/api/v1/asset/${assetId}/download`, | ||
| { | ||
| headers: { | ||
| "X-API-KEY": `${process.env.BAGEL_API_KEY}`, | ||
| }, | ||
| } | ||
| ); | ||
|
|
||
| if (!fileResponse.ok) { | ||
| throw new Error( | ||
| `API responded with status ${fileResponse.status}: ${await fileResponse.text()}` | ||
| ); | ||
| } | ||
|
|
||
| logger.log("Response headers:", fileResponse.headers); | ||
|
|
||
| const fileName = | ||
| fileResponse.headers | ||
| .get("content-disposition") | ||
| ?.split("filename=")[1] | ||
| ?.replace(/"/g, /* " */ "") || "default_name.txt"; | ||
|
|
||
| logger.log("Saving as:", fileName); | ||
|
|
||
| const arrayBuffer = await fileResponse.arrayBuffer(); | ||
| const buffer = Buffer.from(arrayBuffer); | ||
|
|
||
| const filePath = path.join(downloadDir, fileName); | ||
| logger.log("Full file path:", filePath); | ||
|
|
||
| await fs.promises.writeFile(filePath, buffer); | ||
|
|
||
| // Verify file was written | ||
| const stats = await fs.promises.stat(filePath); | ||
| logger.log( | ||
| "File written successfully. Size:", | ||
| stats.size, | ||
| "bytes" | ||
| ); | ||
|
|
||
| res.json({ | ||
| success: true, | ||
| message: "Single file downloaded successfully", | ||
| downloadPath: downloadDir, | ||
| fileCount: 1, | ||
| fileName: fileName, | ||
| fileSize: stats.size, | ||
| }); | ||
| } catch (error) { | ||
| logger.error("Detailed error:", error); | ||
| res.status(500).json({ | ||
| error: "Failed to download files from BagelDB", | ||
| details: error.message, | ||
| stack: error.stack, | ||
| }); | ||
| } | ||
| } |
Check failure
Code scanning / CodeQL
Missing rate limiting High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix AI 6 days ago
To fix the problem, we need to introduce rate limiting to the route handler that performs file system operations. The best way to achieve this is by using the express-rate-limit middleware. This middleware allows us to limit the number of requests a client can make to the server within a specified time window.
We will:
- Install the
express-rate-limitpackage. - Import the
express-rate-limitpackage in thepackages/agent/src/server.tsfile. - Create a rate limiter with appropriate settings.
- Apply the rate limiter to the specific route handler that performs file system operations.
| @@ -23,2 +23,8 @@ | ||
| import { createApiRouter } from "./api.ts"; | ||
| import rateLimit from "express-rate-limit"; | ||
|
|
||
| const limiter = rateLimit({ | ||
| windowMs: 15 * 60 * 1000, // 15 minutes | ||
| max: 100, // limit each IP to 100 requests per windowMs | ||
| }); | ||
|
|
||
| @@ -643,2 +649,3 @@ | ||
| "/fine-tune/:assetId", | ||
| limiter, | ||
| async (req: express.Request, res: express.Response) => { |
| @@ -31,3 +31,4 @@ | ||
| "yargs": "17.7.2", | ||
| "minipass": "7.1.2" | ||
| "minipass": "7.1.2", | ||
| "express-rate-limit": "^7.5.0" | ||
| }, |
| Package | Version | Security advisories |
| express-rate-limit (npm) | 7.5.0 | None |
packages/agent/src/server.ts
Outdated
| const fileResponse = await fetch( | ||
| `https://api.bageldb.ai/api/v1/asset/${assetId}/download`, | ||
| { | ||
| headers: { | ||
| "X-API-KEY": `${process.env.BAGEL_API_KEY}`, | ||
| }, | ||
| } | ||
| ); |
Check failure
Code scanning / CodeQL
Server-side request forgery Critical
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix AI 6 days ago
To fix the problem, we need to validate and sanitize the assetId parameter before using it to construct the URL. One way to do this is to ensure that the assetId only contains valid characters (e.g., alphanumeric characters) and does not include any special characters that could be used for path traversal or other malicious purposes. We can use a regular expression to validate the assetId.
| @@ -645,2 +645,5 @@ | ||
| const assetId = req.params.assetId; | ||
| if (!/^[a-zA-Z0-9]+$/.test(assetId)) { | ||
| return res.status(400).json({ error: "Invalid assetId" }); | ||
| } | ||
| const downloadDir = path.join( |
| const filePath = path.join(downloadDir, fileName); | ||
| logger.log("Full file path:", filePath); | ||
|
|
||
| await fs.promises.writeFile(filePath, buffer); |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix AI 6 days ago
To fix the problem, we need to ensure that the constructed file path is contained within a safe root folder. We can achieve this by normalizing the path using path.resolve and then checking that the normalized path starts with the root folder. This will prevent directory traversal attacks by ensuring that the file path does not escape the intended directory.
- Normalize the
downloadDirandfilePathusingpath.resolve. - Check that the normalized paths start with the intended root directory.
- If the check fails, return an error response.
| @@ -645,7 +645,9 @@ | ||
| const assetId = req.params.assetId; | ||
| const downloadDir = path.join( | ||
| process.cwd(), | ||
| "downloads", | ||
| assetId | ||
| ); | ||
| const rootDir = path.resolve(process.cwd(), "downloads"); | ||
| const downloadDir = path.resolve(rootDir, assetId); | ||
|
|
||
| if (!downloadDir.startsWith(rootDir)) { | ||
| res.status(403).json({ error: "Invalid assetId" }); | ||
| return; | ||
| } | ||
|
|
||
| @@ -686,3 +688,9 @@ | ||
|
|
||
| const filePath = path.join(downloadDir, fileName); | ||
| const filePath = path.resolve(downloadDir, fileName); | ||
|
|
||
| if (!filePath.startsWith(downloadDir)) { | ||
| res.status(403).json({ error: "Invalid file path" }); | ||
| return; | ||
| } | ||
|
|
||
| logger.log("Full file path:", filePath); |
| await fs.promises.writeFile(filePath, buffer); | ||
|
|
||
| // Verify file was written | ||
| const stats = await fs.promises.stat(filePath); |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix AI 6 days ago
To fix the problem, we need to ensure that the constructed file path is contained within a safe root directory. This can be achieved by normalizing the path using path.resolve and then verifying that the normalized path starts with the intended root directory. This approach will prevent directory traversal attacks by ensuring that any .. segments are resolved and the final path is within the allowed directory.
| @@ -645,7 +645,7 @@ | ||
| const assetId = req.params.assetId; | ||
| const downloadDir = path.join( | ||
| process.cwd(), | ||
| "downloads", | ||
| assetId | ||
| ); | ||
| const rootDir = path.join(process.cwd(), "downloads"); | ||
| const downloadDir = path.resolve(rootDir, assetId); | ||
| if (!downloadDir.startsWith(rootDir)) { | ||
| throw new Error("Invalid assetId, directory traversal attempt detected."); | ||
| } | ||
|
|
||
| @@ -686,3 +686,6 @@ | ||
|
|
||
| const filePath = path.join(downloadDir, fileName); | ||
| const filePath = path.resolve(downloadDir, fileName); | ||
| if (!filePath.startsWith(downloadDir)) { | ||
| throw new Error("Invalid fileName, directory traversal attempt detected."); | ||
| } | ||
| logger.log("Full file path:", filePath); |
packages/cli/src/tee/phala/docker.ts
Outdated
| try { | ||
| console.log(`Starting local environment using compose file: ${composeFile}...`); | ||
| // Pass the env file to docker-compose | ||
| await execAsync(`docker-compose --env-file ${path.resolve(envFile)} -f ${composeFile} up -d`); |
Check warning
Code scanning / CodeQL
Shell command built from environment values Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix AI 6 days ago
To fix the problem, we should avoid constructing the shell command directly with environment values. Instead, we can use the execFileSync method from the child_process module, which allows us to pass the command and its arguments separately. This approach ensures that the arguments are not interpreted by the shell, mitigating the risk of command injection.
- Replace the
execAsynccall withexecFileSyncto separate the command and its arguments. - Update the
runLocalComposemethod to useexecFileSyncwith the appropriate arguments.
| @@ -1,2 +1,2 @@ | ||
| import { exec } from 'child_process'; | ||
| import { exec, execFileSync } from 'child_process'; | ||
| import { promisify } from 'util'; | ||
| @@ -254,3 +254,3 @@ | ||
| // Pass the env file to docker-compose | ||
| await execAsync(`docker-compose --env-file ${path.resolve(envFile)} -f ${composeFile} up -d`); | ||
| await execFileSync('docker-compose', ['--env-file', path.resolve(envFile), '-f', composeFile, 'up', '-d']); | ||
|
|
| return state[key] ?? ""; | ||
| }); | ||
| return out; | ||
| const templateFunction = handlebars.compile(templateStr); |
Check failure
Code scanning / CodeQL
Code injection Critical
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix AI 6 days ago
To fix the problem, we need to ensure that the user-provided input is properly sanitized before being used in the handlebars.compile function. This can be achieved by escaping any potentially dangerous characters in the template string. The handlebars library provides a way to escape HTML, but we should also ensure that any other potentially dangerous content is handled appropriately.
The best way to fix this issue is to use the handlebars.escapeExpression function to escape the template string before compiling it. This will ensure that any user-provided input is safely escaped and cannot be used to inject malicious code.
| @@ -43,3 +43,4 @@ | ||
|
|
||
| const templateFunction = handlebars.compile(templateStr); | ||
| const escapedTemplateStr = handlebars.escapeExpression(templateStr); | ||
| const templateFunction = handlebars.compile(escapedTemplateStr); | ||
| return templateFunction(state); |
| @@ -86,7 +85,7 @@ | |||
| let jsonData = null; | |||
|
|
|||
| // First try to parse with the original JSON format | |||
| const jsonBlockMatch = text.match(jsonBlockPattern); | |||
| const jsonBlockMatch = text?.match(jsonBlockPattern); | |||
Check failure
Code scanning / CodeQL
Polynomial regular expression used on uncontrolled data High
This PR implements a 'handlers' which can be used to hook in services