MCPHub has Path Traversal via Malicious MCPB Manifest Name

Analysis: 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.

Step 3: Trace the HTTP entry point in src/routes/index.ts:297-299

I located the route that exposes the upload handler.

// src/routes/index.ts:297-299
// MCPB upload routes
router.post('/mcpb/upload', uploadMiddleware, uploadMcpbFile);

Analysis: 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.

Step 4: Confirm the upload middleware (src/controllers/mcpbController.ts:8-38)

I inspected how the uploaded file is received and stored.

// src/controllers/mcpbController.ts:8-38
const storage = multer.diskStorage({
  destination: (_req, _file, cb) => {
    const uploadDir = path.join(process.cwd(), 'data/uploads/mcpb');
    if (!fs.existsSync(uploadDir)) {
      fs.mkdirSync(uploadDir, { recursive: true });
    }
    cb(null, uploadDir);
  },
  filename: (_req, file, cb) => {
    const timestamp = Date.now();
    const originalName = path.parse(file.originalname).name;
    cb(null, `${originalName}-${timestamp}.mcpb`);
  },
});

const upload = multer({
  storage,
  fileFilter: (_req, file, cb) => {
    if (file.originalname.endsWith('.mcpb')) {
      cb(null, true);
    } else {
      cb(new Error('Only .mcpb files are allowed'));
    }
  },
  limits: {
    fileSize: 500 * 1024 * 1024, // 500MB limit
  },
});

export const uploadMiddleware = upload.single('mcpbFile');

Analysis: 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.

Step 5: Verify lack of path validation on manifest.name in src/controllers/mcpbController.ts:92-110

I verified that no path sanitization occurs between parsing and usage.

// src/controllers/mcpbController.ts:92-110
if (!manifest.name) {
  throw new Error('Invalid manifest: missing name');
}
// ...
const finalExtractDir = path.join(path.dirname(mcpbFilePath), `server-${manifest.name}`);
cleanupOldMcpbServer(manifest.name);

Analysis: 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.

Step 6: Examine cleanup behavior using the unsanitized name (src/controllers/mcpbController.ts:41-52)

I verified how cleanupOldMcpbServer uses the same input.

// src/controllers/mcpbController.ts:41-52
const uploadDir = path.join(process.cwd(), 'data/uploads/mcpb');
const serverPattern = `server-${serverName}`;

if (fs.existsSync(uploadDir)) {
  const files = fs.readdirSync(uploadDir);
  files.forEach((file) => {
    if (file.startsWith(serverPattern)) {
      const filePath = path.join(uploadDir, file);
      if (fs.statSync(filePath).isDirectory()) {
        fs.rmSync(filePath, { recursive: true, force: true });
      }
    }
  });
}

Analysis: 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.

Analysis Walkthrough

• 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).
• 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).
• 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).
• 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).
• 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).
• → Reached leaf node: True Positive

3. Conclusion

True Positive

Key evidence:

• manifest.name flows directly into finalExtractDir and is used by fs.mkdirSync and fs.renameSync without sanitization (src/controllers/mcpbController.ts:106-116).
• manifest.name is parsed from manifest.json inside an uploaded archive, with only a non‑empty check (src/controllers/mcpbController.ts:89-97).
• The /mcpb/upload endpoint exposes the upload handler that processes user‑supplied archives (src/routes/index.ts:297-299).

4. Remediation Recommendations

• 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;).
• Use path.basename() to strip directory components from manifest.name and enforce a strict character allow‑list (alphanumeric, _, -, .) before use.
• Consider rejecting any manifest.name that contains path separators or traversal sequences, and add unit tests for malicious traversal inputs.