GHSA-p3h2-2j4p-p83g — @samanhappy/mcphub

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.

1. Summary

• Vulnerability Type: Path Traversal (CWE-22)
• Sink Location: src/controllers/mcpbController.ts:107
• 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.

2. Analysis Logic

Step 1: Inspect the identified sink (src/controllers/mcpbController.ts:106-116)

I 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.

// src/controllers/mcpbController.ts:106-116
// Use server name as the final extract directory for automatic version management
const finalExtractDir = path.join(path.dirname(mcpbFilePath), `server-${manifest.name}`);

// Clean up any existing version of this server
cleanupOldMcpbServer(manifest.name);
if (!fs.existsSync(finalExtractDir)) {
  fs.mkdirSync(finalExtractDir, { recursive: true });
}

// Move the temporary directory to the final location
fs.renameSync(tempExtractDir, finalExtractDir);

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

Step 2: Trace the source of manifest.name in the upload handler (src/controllers/mcpbController.ts:83-104)

I traced back the data flow to see how the manifest is read and validated.

// src/controllers/mcpbController.ts:83-104
const manifestPath = path.join(tempExtractDir, 'manifest.json');
if (!fs.existsSync(manifestPath)) {
  throw new Error('manifest.json not found in MCPB file');
}

const manifestContent = fs.readFileSync(manifestPath, 'utf-8');
const manifest = JSON.parse(manifestContent);

// Validate required fields in manifest
if (!manifest.manifest_version) {
  throw new Error('Invalid manifest: missing manifest_version');
}
if (!manifest.name) {
  throw new Error('Invalid manifest: missing 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.