+ "details": "The MCPB file upload handler extracts a ZIP file and reads `manifest.json` from it. The `name` field from the manifest is concatenated directly into the file path (line 107) without any sanitization or path traversal character validation. An attacker can craft a malicious MCPB file with `manifest.name` set to `'../../../etc/malicious'` or similar, causing files to be extracted to arbitrary locations on the file system. The `cleanupOldMcpbServer` function (line 110) also uses the unsanitized name, potentially allowing arbitrary directory deletion.\n\n## 1. Summary\n- **Vulnerability Type**: Path Traversal\n- **Flagged Location**: `src/controllers/mcpbController.ts:107`\n- **Vulnerability Description**: The `name` field in the uploaded MCPB manifest is used directly to construct file system paths for directory creation and move operations without sanitization or normalization, potentially leading to a path traversal attack.\n\n## 2. Analysis Logic\n\n### Step 1: Inspect the flagged sink (`src/controllers/mcpbController.ts:106-116`)\nI reviewed the upload handler and located the file system sink where `manifest.name` is used to construct the final extraction path and where files are written to that path.\n\n```ts\n// src/controllers/mcpbController.ts:106-116\n// Use server name as the final extract directory for automatic version management\nconst finalExtractDir = path.join(path.dirname(mcpbFilePath), `server-${manifest.name}`);\n\n// Clean up any existing version of this server\ncleanupOldMcpbServer(manifest.name);\nif (!fs.existsSync(finalExtractDir)) {\n fs.mkdirSync(finalExtractDir, { recursive: true });\n}\n\n// Move the temporary directory to the final location\nfs.renameSync(tempExtractDir, finalExtractDir);\n```\n\nAnalysis: `manifest.name` is used to build `finalExtractDir`, which is then operated on by `fs.mkdirSync` and `fs.renameSync`. These are file system write/move operations, so if `name` is user-controllable and unsanitized, this is a path traversal sink. Next, I traced the source of `manifest.name`.\n\n### Step 2: Trace the source of `manifest.name` in the upload handler (`src/controllers/mcpbController.ts:83-104`)\nI traced the data flow backward to see how the manifest is read and validated.\n\n```ts\n// src/controllers/mcpbController.ts:83-104\nconst manifestPath = path.join(tempExtractDir, 'manifest.json');\nif (!fs.existsSync(manifestPath)) {\n throw new Error('manifest.json not found in MCPB file');\n}\n\nconst manifestContent = fs.readFileSync(manifestPath, 'utf-8');\nconst manifest = JSON.parse(manifestContent);\n\n// Validate required fields in manifest\nif (!manifest.manifest_version) {\n throw new Error('Invalid manifest: missing manifest_version');\n}\nif (!manifest.name) {\n throw new Error('Invalid manifest: missing name');\n}\n```\n\nAnalysis: `manifest` is parsed directly from the `manifest.json` inside the uploaded archive. The only check on `manifest.name` is non-emptiness; there is no sanitization, normalization, or whitelist validation. Next, I confirmed the entry point for uploading MCPB files to verify user control.\n\n### Step 3: Trace the HTTP entry point in `src/routes/index.ts:297-299`\nI located the route that exposes the upload handler.\n\n```ts\n// src/routes/index.ts:297-299\n// MCPB upload routes\nrouter.post('/mcpb/upload', uploadMiddleware, uploadMcpbFile);\n```\n\nAnalysis: The `/mcpb/upload` endpoint invokes `uploadMiddleware` and `uploadMcpbFile`, so user-supplied uploads are the source of the manifest content. Next, I confirmed the behavior of the upload middleware.\n\n### Step 4: Confirm the upload middleware (`src/controllers/mcpbController.ts:8-38`)\nI examined how uploaded files are received and stored.\n\n```ts\n// src/controllers/mcpbController.ts:8-38\nconst storage = multer.diskStorage({\n destination: (_req, _file, cb) => {\n const uploadDir = path.join(process.cwd(), 'data/uploads/mcpb');\n if (!fs.existsSync(uploadDir)) {\n fs.mkdirSync(uploadDir, { recursive: true });\n }\n cb(null, uploadDir);\n },\n filename: (_req, file, cb) => {\n const timestamp = Date.now();\n const originalName = path.parse(file.originalname).name;\n cb(null, `${originalName}-${timestamp}.mcpb`);\n },\n});\n\nconst upload = multer({\n storage,\n fileFilter: (_req, file, cb) => {\n if (file.originalname.endsWith('.mcpb')) {\n cb(null, true);\n } else {\n cb(new Error('Only .mcpb files are allowed'));\n }\n },\n limits: {\n fileSize: 500 * 1024 * 1024, // 500MB limit\n },\n});\n\nexport const uploadMiddleware = upload.single('mcpbFile');\n```\n\nAnalysis: The upload middleware only checks the file extension and size. It does not restrict or validate the contents of the archive or `manifest.name`. Therefore, `manifest.name` is user-controllable input. Next, I checked whether any sanitization or normalization is applied before reaching the sink.\n\n### Step 5: Verify the lack of path validation for `manifest.name` at `src/controllers/mcpbController.ts:92-110`\nI verified that no path sanitization is performed between parsing and use.\n\n```ts\n// src/controllers/mcpbController.ts:92-110\nif (!manifest.name) {\n throw new Error('Invalid manifest: missing name');\n}\n// ...\nconst finalExtractDir = path.join(path.dirname(mcpbFilePath), `server-${manifest.name}`);\ncleanupOldMcpbServer(manifest.name);\n```\n\nAnalysis: There is no `path.resolve`/`realpath` check, no use of `basename()`, and no whitelist validation before `manifest.name` is used to construct the file system path. This confirms the path is built from untrusted input with no defenses.\n\n### Step 6: Inspect cleanup behavior using the unsanitized name (`src/controllers/mcpbController.ts:41-52`)\nI verified how `cleanupOldMcpbServer` uses the same input.\n\n```ts\n// src/controllers/mcpbController.ts:41-52\nconst uploadDir = path.join(process.cwd(), 'data/uploads/mcpb');\nconst serverPattern = `server-${serverName}`;\n\nif (fs.existsSync(uploadDir)) {\n const files = fs.readdirSync(uploadDir);\n files.forEach((file) => {\n if (file.startsWith(serverPattern)) {\n const filePath = path.join(uploadDir, file);\n if (fs.statSync(filePath).isDirectory()) {\n fs.rmSync(filePath, { recursive: true, force: true });\n }\n }\n });\n}\n```\n\nAnalysis: `serverName` is used without validation, but the deletion operation is constrained to directories that already exist within `uploadDir` as returned by `readdirSync`. The primary traversal risk remains in the path construction for `finalExtractDir` and the subsequent file system operations.\n\n### Analysis Process\n- Q1: Does user-controllable input influence the file path? → **Yes**. `manifest.name` is read from the uploaded archive's `manifest.json` and used in `path.join(...)` to construct `finalExtractDir` (`src/controllers/mcpbController.ts:89-110`).\n- Q2: Is the path normalized and validated against a base directory? → **No**. There is no `resolve`/`realpath` + `startsWith` check before `fs.mkdirSync`/`fs.renameSync` (`src/controllers/mcpbController.ts:106-116`).\n- Q3: Is `basename()`/`getName()` used to strip directory components? → **No**. `manifest.name` is used directly in a template string (`src/controllers/mcpbController.ts:106-107`).\n- Q4: Is there an effective allow-list of valid file names? → **No**. Only an existence check is performed on `manifest.name` (`src/controllers/mcpbController.ts:92-97`).\n- Q5: Is the code in a test/demo/deprecated/generated context? → **No**. This is a production controller and route (`src/controllers/mcpbController.ts:64-130`, `src/routes/index.ts:297-299`).\n- → Reached leaf node: **Real Vulnerability** (TP)\n\n## 3. Conclusion\n**Real Vulnerability**\n\n**Key Evidence:**\n- `manifest.name` flows directly into `finalExtractDir` and is used by `fs.mkdirSync` and `fs.renameSync` without sanitization (`src/controllers/mcpbController.ts:106-116`).\n- `manifest.name` is parsed from the `manifest.json` inside the uploaded archive with only a non-empty check (`src/controllers/mcpbController.ts:89-97`).\n- The `/mcpb/upload` endpoint exposes the upload handler that processes user-supplied archives (`src/routes/index.ts:297-299`).\n\n## 4. Remediation Recommendations\n- Before using `manifest.name` to construct `finalExtractDir`, add normalization and base-directory validation (e.g., `` const resolved = path.resolve(baseDir, `server-${safeName}`); if (!resolved.startsWith(baseDir)) reject; ``).\n- Use `path.basename()` to strip directory components from `manifest.name`, and enforce a strict character whitelist (letters, digits, `_`, `-`, `.`) before use.\n- Consider rejecting any `manifest.name` containing path separators or traversal sequences, and add unit tests for traversal inputs.\n\n---\n\n*Translated from: Chinese (Simplified) to English using GitHub Copilot*",
![advisory-database[bot]](https://avatars.githubusercontent.com/in/21409?v=4&size=40){kind=avatar}
0 commit comments