Skip to content

Commit a3bd971

Browse files
bryan-anthropicbryan-anthropic
authored
Merge pull request #209 from modelcontextprotocol/feat/enhanced-validate
feat: enhanced structural validation for validate command
2 parents 7f4596d + 122ef0e commit a3bd971

6 files changed

Lines changed: 717 additions & 12 deletions

File tree

‎src/cli/pack.ts‎

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,11 @@ export async function packExtension({
8383

8484
// Validate manifest first
8585
logger.log("Validating manifest...");
86-
if (!validateManifest(manifestPath)) {
86+
if (
87+
!validateManifest(manifestPath, {
88+
projectDir: resolvedPath,
89+
})
90+
) {
8791
logger.error("ERROR: Cannot pack extension with invalid manifest");
8892
return false;
8993
}
@@ -202,15 +206,22 @@ export async function packExtension({
202206
const zipFiles: Zippable = {};
203207

204208
const isUnix = process.platform !== "win32";
209+
const isBinaryType = manifest.server?.type === "binary";
210+
const binaryEntryPoint = isBinaryType ? manifest.server.entry_point : null;
205211

206212
for (const [filePath, fileData] of Object.entries(files)) {
207213
if (isUnix) {
214+
let mode = fileData.mode & 0o777;
215+
// Ensure binary entry points are always executable in the ZIP,
216+
// regardless of filesystem permissions. Windows-built ZIPs store
217+
// 644 because Windows has no Unix execute bit — CD spawns binary
218+
// entry points directly, so they must have +x to avoid EACCES.
219+
if (binaryEntryPoint && filePath === binaryEntryPoint) {
220+
mode = mode | 0o111;
221+
}
208222
// Set external file attributes to preserve Unix permissions
209223
// The mode needs to be shifted to the upper 16 bits for ZIP format
210-
zipFiles[filePath] = [
211-
fileData.data,
212-
{ os: 3, attrs: (fileData.mode & 0o777) << 16 },
213-
];
224+
zipFiles[filePath] = [fileData.data, { os: 3, attrs: mode << 16 }];
214225
} else {
215226
// On Windows, use default ZIP attributes (no Unix permissions)
216227
zipFiles[filePath] = fileData.data;

‎src/node/validate.ts‎

Lines changed: 239 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ import { existsSync, readFileSync, statSync } from "fs";
22
import * as fs from "fs/promises";
33
import { DestroyerOfModules } from "galactus";
44
import * as os from "os";
5-
import { dirname, isAbsolute, join, resolve } from "path";
5+
import { dirname, extname, isAbsolute, join, resolve } from "path";
66
import prettyBytes from "pretty-bytes";
77

88
import { unpackExtension } from "../cli/unpack.js";
@@ -11,6 +11,7 @@ import {
1111
MANIFEST_SCHEMAS_LOOSE,
1212
} from "../shared/constants.js";
1313
import { getManifestVersionFromRawData } from "../shared/manifestVersionResolve.js";
14+
import { getAllFilesWithCount, readMcpbIgnorePatterns } from "./files.js";
1415

1516
/**
1617
* Check if a buffer contains a valid PNG file signature
@@ -108,7 +109,191 @@ function validateIcon(
108109
};
109110
}
110111

111-
export function validateManifest(inputPath: string): boolean {
112+
interface ValidationResult {
113+
valid: boolean;
114+
errors: string[];
115+
warnings: string[];
116+
}
117+
118+
// Expected file extensions by server type
119+
const NODE_EXTENSIONS = new Set([".js", ".mjs", ".cjs"]);
120+
const PYTHON_EXTENSIONS = new Set([".py"]);
121+
const SCRIPT_EXTENSIONS = new Set([".js", ".mjs", ".cjs", ".py"]);
122+
123+
/**
124+
* Validate that the server entry_point file exists and matches the server type
125+
*/
126+
function validateEntryPoint(
127+
manifest: { server: { type: string; entry_point: string } },
128+
baseDir: string,
129+
): ValidationResult {
130+
const errors: string[] = [];
131+
const warnings: string[] = [];
132+
const { type, entry_point } = manifest.server;
133+
const entryPath = join(baseDir, entry_point);
134+
135+
if (!existsSync(entryPath)) {
136+
errors.push(`Entry point file not found: ${entry_point}`);
137+
return { valid: false, errors, warnings };
138+
}
139+
140+
const ext = extname(entry_point).toLowerCase();
141+
142+
if (type === "node" && !NODE_EXTENSIONS.has(ext)) {
143+
warnings.push(
144+
`Unusual entry point extension "${ext}" for server type "node". Expected: .js, .mjs, or .cjs`,
145+
);
146+
} else if (
147+
(type === "python" || type === "uv") &&
148+
!PYTHON_EXTENSIONS.has(ext)
149+
) {
150+
warnings.push(
151+
`Unusual entry point extension "${ext}" for server type "${type}". Expected: .py`,
152+
);
153+
} else if (type === "binary" && SCRIPT_EXTENSIONS.has(ext)) {
154+
warnings.push(
155+
`Entry point has script extension "${ext}" but server type is "binary". Did you mean type "node" or "python"?`,
156+
);
157+
}
158+
159+
// For binary type on Unix, check executable bit
160+
if (type === "binary" && process.platform !== "win32") {
161+
const stat = statSync(entryPath);
162+
if (!(stat.mode & 0o111)) {
163+
errors.push(
164+
`Binary entry point is not executable: ${entry_point}. Run: chmod +x ${entry_point}`,
165+
);
166+
}
167+
}
168+
169+
return { valid: errors.length === 0, errors, warnings };
170+
}
171+
172+
// Valid variable patterns from src/shared/config.ts replaceVariables()
173+
const VALID_VARIABLE_PATTERN =
174+
/^\$\{(__dirname|pathSeparator|\/|user_config\..+)\}$/;
175+
176+
/**
177+
* Validate that ${...} variables in mcp_config are recognized
178+
*/
179+
function validateCommandVariables(manifest: {
180+
server: {
181+
mcp_config: {
182+
command?: string;
183+
args?: string[];
184+
env?: Record<string, string>;
185+
platform_overrides?: Record<
186+
string,
187+
{
188+
command?: string;
189+
args?: string[];
190+
env?: Record<string, string>;
191+
}
192+
>;
193+
};
194+
};
195+
}): ValidationResult {
196+
const errors: string[] = [];
197+
const warnings: string[] = [];
198+
199+
function checkString(value: string, context: string): void {
200+
const variablePattern = /\$\{([^}]+)\}/g;
201+
let match;
202+
while ((match = variablePattern.exec(value)) !== null) {
203+
const fullVar = match[0];
204+
if (!VALID_VARIABLE_PATTERN.test(fullVar)) {
205+
errors.push(
206+
`Invalid variable "${fullVar}" in ${context}. Valid variables: \${__dirname}, \${pathSeparator}, \${/}, \${user_config.<key>}`,
207+
);
208+
}
209+
}
210+
}
211+
212+
function checkConfig(
213+
config: { command?: string; args?: string[]; env?: Record<string, string> },
214+
prefix: string,
215+
): void {
216+
if (config.command) checkString(config.command, `${prefix}command`);
217+
if (config.args) {
218+
config.args.forEach((arg, i) => checkString(arg, `${prefix}args[${i}]`));
219+
}
220+
if (config.env) {
221+
for (const [key, val] of Object.entries(config.env)) {
222+
checkString(val, `${prefix}env.${key}`);
223+
}
224+
}
225+
}
226+
227+
const { mcp_config } = manifest.server;
228+
checkConfig(mcp_config, "mcp_config.");
229+
230+
if (mcp_config.platform_overrides) {
231+
for (const [platform, override] of Object.entries(
232+
mcp_config.platform_overrides,
233+
)) {
234+
checkConfig(override, `mcp_config.platform_overrides.${platform}.`);
235+
}
236+
}
237+
238+
return { valid: errors.length === 0, errors, warnings };
239+
}
240+
241+
// Sensitive file patterns not already covered by EXCLUDE_PATTERNS in files.ts
242+
const SENSITIVE_PATTERNS = [
243+
/(^|\/)credentials\.json$/i,
244+
/(^|\/)secrets\./i,
245+
/\.pem$/i,
246+
/\.key$/i,
247+
/\.p12$/i,
248+
/\.pfx$/i,
249+
/\.jks$/i,
250+
/(^|\/)\.aws\//,
251+
/(^|\/)\.ssh\//,
252+
/(^|\/)id_rsa/,
253+
/(^|\/)id_ed25519/,
254+
/(^|\/)id_ecdsa/,
255+
/\.keystore$/i,
256+
/(^|\/)token\.json$/i,
257+
];
258+
259+
/**
260+
* Check if the file list that would be bundled contains sensitive files
261+
*/
262+
function validateSensitiveFiles(baseDir: string): ValidationResult {
263+
const warnings: string[] = [];
264+
265+
try {
266+
const mcpbIgnorePatterns = readMcpbIgnorePatterns(baseDir);
267+
const { files } = getAllFilesWithCount(
268+
baseDir,
269+
baseDir,
270+
{},
271+
mcpbIgnorePatterns,
272+
);
273+
274+
for (const filePath of Object.keys(files)) {
275+
for (const pattern of SENSITIVE_PATTERNS) {
276+
if (pattern.test(filePath)) {
277+
warnings.push(
278+
`Potentially sensitive file will be included in bundle: ${filePath}`,
279+
);
280+
break;
281+
}
282+
}
283+
}
284+
} catch {
285+
// If we can't read the directory, skip this check silently —
286+
// pack will fail with a clearer error later
287+
}
288+
289+
// Sensitive files are always warnings, never errors — a .pem might be a legitimate TLS cert
290+
return { valid: true, errors: [], warnings };
291+
}
292+
293+
export function validateManifest(
294+
inputPath: string,
295+
options?: { projectDir?: string },
296+
): boolean {
112297
try {
113298
const resolvedPath = resolve(inputPath);
114299
let manifestPath = resolvedPath;
@@ -131,17 +316,23 @@ export function validateManifest(inputPath: string): boolean {
131316
if (result.success) {
132317
console.log("Manifest schema validation passes!");
133318

134-
// Validate icon if present
319+
const manifestDir = dirname(manifestPath);
320+
// projectDir is where source files live — defaults to the manifest's directory
321+
const projectDir = options?.projectDir
322+
? resolve(options.projectDir)
323+
: manifestDir;
324+
let hasErrors = false;
325+
326+
// Validate icon if present (always relative to manifest directory)
135327
if (manifestData.icon) {
136-
const baseDir = dirname(manifestPath);
137-
const iconValidation = validateIcon(manifestData.icon, baseDir);
328+
const iconValidation = validateIcon(manifestData.icon, manifestDir);
138329

139330
if (iconValidation.errors.length > 0) {
140331
console.log("\nERROR: Icon validation failed:\n");
141332
iconValidation.errors.forEach((error) => {
142333
console.log(` - ${error}`);
143334
});
144-
return false;
335+
hasErrors = true;
145336
}
146337

147338
if (iconValidation.warnings.length > 0) {
@@ -152,7 +343,48 @@ export function validateManifest(inputPath: string): boolean {
152343
}
153344
}
154345

155-
return true;
346+
// Validate entry point (relative to project directory)
347+
const entryPointValidation = validateEntryPoint(manifestData, projectDir);
348+
if (entryPointValidation.errors.length > 0) {
349+
console.log("\nERROR: Entry point validation failed:\n");
350+
entryPointValidation.errors.forEach((error) => {
351+
console.log(` - ${error}`);
352+
});
353+
hasErrors = true;
354+
}
355+
if (entryPointValidation.warnings.length > 0) {
356+
console.log("\nEntry point warnings:\n");
357+
entryPointValidation.warnings.forEach((warning) => {
358+
console.log(` - ${warning}`);
359+
});
360+
}
361+
362+
// Validate command variables
363+
const variableValidation = validateCommandVariables(manifestData);
364+
if (variableValidation.errors.length > 0) {
365+
console.log("\nERROR: Command variable validation failed:\n");
366+
variableValidation.errors.forEach((error) => {
367+
console.log(` - ${error}`);
368+
});
369+
hasErrors = true;
370+
}
371+
if (variableValidation.warnings.length > 0) {
372+
console.log("\nCommand variable warnings:\n");
373+
variableValidation.warnings.forEach((warning) => {
374+
console.log(` - ${warning}`);
375+
});
376+
}
377+
378+
// Check for sensitive files (relative to project directory)
379+
const sensitiveValidation = validateSensitiveFiles(projectDir);
380+
if (sensitiveValidation.warnings.length > 0) {
381+
console.log("\nSensitive file warnings:\n");
382+
sensitiveValidation.warnings.forEach((warning) => {
383+
console.log(` - ${warning}`);
384+
});
385+
}
386+
387+
return !hasErrors;
156388
} else {
157389
console.log("ERROR: Manifest validation failed:\n");
158390
result.error.issues.forEach((issue) => {

0 commit comments

Comments
 (0)