GHSA-p3h2-2j4p-p83g: @samanhappy/mcphub Path Traversal Vulnerability

{
  "schema_version" : "1.4.0",
  "id" : "GHSA-p3h2-2j4p-p83g",
  "modified" : "2026-05-05T21:43:32Z",
  "published" : "2026-04-22T20:50:19Z",
  "aliases" : [ ],
  "summary" : "MCPHub has Path Traversal via Malicious MCPB Manifest Name",
  "details" : "**MCPB File Upload Handler** extracts a ZIP file and reads `manifest.json` from it. The `name` field in the manifest is directly concatenated into a file path (line 107) without any sanitization or path traversal character validation. An attacker can craft a malicious MCPB file where `manifest.name` is set to something like `../../../etc/malicious`, causing the file to be extracted to an arbitrary location on the file system. The `cleanupOldMcpbServer` function (line 110) also uses the unsanitized name, potentially allowing deletion of arbitrary directories.\n\n## 1. Summary\n- **Vulnerability Type**: Path Traversal (CWE-22)\n- **Sink Location**: src/controllers/mcpbController.ts:107\n- **Vulnerability Description**: The `name` field from an uploaded MCPB manifest is used directly, without sanitization or normalization, to construct a file system path for directory creation and move operations, which may lead to path traversal attacks.\n\n## 2. Analysis Logic\n\n### Step 1: Inspect the identified sink (src/controllers/mcpbController.ts:106-116)\nI examined the upload handler and located the file system sink where `manifest.name` is used to build the final extraction path and write files 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-controlled and unsanitized, this is a path traversal sink. Next, I traced the origin 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 back the data flow 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 `manifest.json` inside the uploaded archive. The only check on `manifest.name` is that it is non‑empty; there is no sanitization, normalization, or allow‑list 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 verified the upload middleware behavior.\n\n### Step 4: Confirm the upload middleware (src/controllers/mcpbController.ts:8-38)\nI inspected how the uploaded file is 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 file extension and size. It does not restrict or validate the contents of the archive or `manifest.name`. Therefore, `manifest.name` is user‑controlled input. Next, I checked whether any sanitization or normalization is applied before reaching the sink.\n\n### Step 5: Verify lack of path validation on `manifest.name` in src/controllers/mcpbController.ts:92-110\nI verified that no path sanitization occurs between parsing and usage.\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: Before using `manifest.name` to construct a file system path, there is no `path.resolve`/`realpath` check, no use of `basename()`, and no allow‑list validation. This confirms that the path is built from untrusted input without defenses.\n\n### Step 6: Examine 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 is limited to directories already present in `uploadDir` as returned by `readdirSync`. The main traversal risk remains in constructing the path for `finalExtractDir` and the subsequent file system operations.\n\n### Analysis Walkthrough\n- Q1: Does user‑controllable input affect the file path? → **Yes**. `manifest.name` is read from the uploaded archive’s `manifest.json` and used in `path.join(...)` to build `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 a valid allow‑list for allowed 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: **True Positive**\n\n## 3. Conclusion\n**True Positive**\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 `manifest.json` inside an 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- Add normalization and base directory validation before using `manifest.name` to construct `finalExtractDir` (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 allow‑list (alphanumeric, `_`, `-`, `.`) before use.\n- Consider rejecting any `manifest.name` that contains path separators or traversal sequences, and add unit tests for malicious traversal inputs.",
  "severity" : [ {
    "type" : "CVSS_V4",
    "score" : "CVSS:4.0/AV:N/AC:L/AT:N/PR:L/UI:N/VC:N/VI:H/VA:H/SC:N/SI:N/SA:N"
  } ],
  "affected" : [ {
    "package" : {
      "ecosystem" : "npm",
      "name" : "@samanhappy/mcphub"
    },
    "ranges" : [ {
      "type" : "ECOSYSTEM",
      "events" : [ {
        "introduced" : "0"
      }, {
        "fixed" : "0.12.13"
      } ]
    } ]
  } ],
  "references" : [ {
    "type" : "WEB",
    "url" : "https://github.com/samanhappy/mcphub/security/advisories/GHSA-p3h2-2j4p-p83g"
  }, {
    "type" : "WEB",
    "url" : "https://github.com/samanhappy/mcphub/commit/af5b013c09bb0add6b7ad9aaa5b875cf150d2a7c"
  }, {
    "type" : "PACKAGE",
    "url" : "https://github.com/samanhappy/mcphub"
  } ],
  "database_specific" : {
    "cwe_ids" : [ "CWE-22" ],
    "severity" : "HIGH",
    "github_reviewed" : true,
    "github_reviewed_at" : "2026-04-22T20:50:19Z",
    "nvd_published_at" : null
  }
}