diff --git a/packages/mcp-provider-dx-core/src/shared/soqlUtils.ts b/packages/mcp-provider-dx-core/src/shared/soqlUtils.ts new file mode 100644 index 00000000..32c4cfd4 --- /dev/null +++ b/packages/mcp-provider-dx-core/src/shared/soqlUtils.ts @@ -0,0 +1,88 @@ +/** + * Utility functions for safe SOQL query construction + */ + +/** + * Escapes single quotes in a string for use in SOQL queries. + */ +export function escapeSoqlString(value: string): string { + if (typeof value !== 'string') { + throw new Error('SOQL escape requires string input'); + } + // Escape single quotes by doubling them (SOQL standard) + return value.replace(/'/g, "\\'"); +} + +/** + * Checks if a string contains potential SQL injection patterns. + * Returns true if injection patterns are detected. + */ +export function containsSqlInjectionPatterns(value: string): boolean { + if (typeof value !== 'string') { + return true; + } + + const trimmed = value.trim(); + + // Check for common SQL injection patterns + const injectionPatterns = [ + /--/, // SQL comment + /;/, // Statement terminator + /\bOR\b/i, // OR keyword + /\bAND\b/i, // AND keyword + /\bUNION\b/i, // UNION keyword + /\bSELECT\b/i, // SELECT keyword + /\bDROP\b/i, // DROP keyword + /\bINSERT\b/i, // INSERT keyword + /\bUPDATE\b/i, // UPDATE keyword + /\bDELETE\b/i, // DELETE keyword + /\bEXEC\b/i, // EXEC keyword + ]; + + // Check for injection patterns (excluding the single quote check since we'll escape those) + return injectionPatterns.some(pattern => pattern.test(trimmed)); +} + +/** + * Validates a Salesforce username format. + * Must contain @ and follow email-like format without injection patterns. + */ +export function isValidUsername(username: string): boolean { + if (typeof username !== 'string') { + return false; + } + + const trimmed = username.trim(); + + // Must contain @ + if (!trimmed.includes('@')) { + return false; + } + + // Basic email format: something@something.something + const emailPattern = /^[a-zA-Z0-9._%+-]+@[a-zA-Z0-9.-]+\.[a-zA-Z]{2,}$/; + if (!emailPattern.test(trimmed)) { + return false; + } + + // Check for SQL injection patterns + if (containsSqlInjectionPatterns(trimmed)) { + return false; + } + + return true; +} + +/** + * Validates and escapes a Salesforce username for SOQL queries. + * Throws an error if the username format is invalid or contains injection attempts. + */ +export function validateAndEscapeUsername(username: string): string { + const trimmed = username.trim(); + + if (!isValidUsername(trimmed)) { + throw new Error(`Invalid username format or potential SQL injection detected: ${username}`); + } + + return escapeSoqlString(trimmed); +} diff --git a/packages/mcp-provider-dx-core/src/tools/assign_permission_set.ts b/packages/mcp-provider-dx-core/src/tools/assign_permission_set.ts index d55cba96..0e741a6e 100644 --- a/packages/mcp-provider-dx-core/src/tools/assign_permission_set.ts +++ b/packages/mcp-provider-dx-core/src/tools/assign_permission_set.ts @@ -20,6 +20,7 @@ import { McpTool, McpToolConfig, ReleaseState, Services, Toolset } from '@salesf import { CallToolResult } from '@modelcontextprotocol/sdk/types.js'; import { directoryParam, usernameOrAliasParam } from '../shared/params.js'; import { textResponse } from '../shared/utils.js'; +import { validateAndEscapeUsername } from '../shared/soqlUtils.js'; /* * Assign permission set @@ -117,10 +118,21 @@ export class AssignPermissionSetMcpTool extends McpTool( - `SELECT Id FROM User WHERE Username='${assignTo}'`, + `SELECT Id FROM User WHERE Username='${validatedUsername}'`, ); await user.assignPermissionSets(queryResult.Id, [input.permissionSetName]); diff --git a/packages/mcp-provider-dx-core/test/shared/soqlUtils.test.ts b/packages/mcp-provider-dx-core/test/shared/soqlUtils.test.ts new file mode 100644 index 00000000..8ab69003 --- /dev/null +++ b/packages/mcp-provider-dx-core/test/shared/soqlUtils.test.ts @@ -0,0 +1,155 @@ +import { expect } from 'chai'; +import { + escapeSoqlString, + containsSqlInjectionPatterns, + isValidUsername, + validateAndEscapeUsername +} from '../../src/shared/soqlUtils.js'; + +describe('soqlUtils', () => { + describe('escapeSoqlString', () => { + it('should escape single quotes', () => { + expect(escapeSoqlString("O'Brien")).to.equal("O\\'Brien"); + expect(escapeSoqlString("It's working")).to.equal("It\\'s working"); + }); + + it('should handle strings without quotes', () => { + expect(escapeSoqlString("normal text")).to.equal("normal text"); + }); + + it('should handle empty strings', () => { + expect(escapeSoqlString("")).to.equal(""); + }); + + it('should throw on non-string values', () => { + expect(() => escapeSoqlString(null as any)).to.throw(); + expect(() => escapeSoqlString(123 as any)).to.throw(); + }); + }); + + describe('containsSqlInjectionPatterns', () => { + it('should detect SQL injection patterns', () => { + expect(containsSqlInjectionPatterns("' OR '1'='1")).to.be.true; + expect(containsSqlInjectionPatterns("'; DROP TABLE--")).to.be.true; + expect(containsSqlInjectionPatterns("test UNION SELECT")).to.be.true; + expect(containsSqlInjectionPatterns("value--comment")).to.be.true; + expect(containsSqlInjectionPatterns("item; DELETE FROM")).to.be.true; + }); + + it('should allow safe strings', () => { + expect(containsSqlInjectionPatterns("user@example.com")).to.be.false; + expect(containsSqlInjectionPatterns("admin@test.org")).to.be.false; + expect(containsSqlInjectionPatterns("test.user@company.com")).to.be.false; + }); + + it('should handle non-string values', () => { + expect(containsSqlInjectionPatterns(null as any)).to.be.true; + expect(containsSqlInjectionPatterns(undefined as any)).to.be.true; + expect(containsSqlInjectionPatterns(123 as any)).to.be.true; + }); + }); + + describe('isValidUsername', () => { + it('should accept valid email-like usernames', () => { + expect(isValidUsername('user@example.com')).to.be.true; + expect(isValidUsername('test.user@company.org')).to.be.true; + expect(isValidUsername('admin+test@domain.co.uk')).to.be.true; + expect(isValidUsername('user_name@sub.domain.com')).to.be.true; + }); + + it('should reject usernames without @', () => { + expect(isValidUsername('username')).to.be.false; + expect(isValidUsername('admin')).to.be.false; + }); + + it('should reject invalid email formats', () => { + expect(isValidUsername('@example.com')).to.be.false; + expect(isValidUsername('user@')).to.be.false; + expect(isValidUsername('user@domain')).to.be.false; + expect(isValidUsername('user@@domain.com')).to.be.false; + }); + + it('should reject SQL injection attempts', () => { + expect(isValidUsername("user@example.com' OR 1=1--")).to.be.false; + expect(isValidUsername("x@example.com' OR IsActive = true OR Username='x")).to.be.false; + expect(isValidUsername("admin@test.com; DROP TABLE")).to.be.false; + expect(isValidUsername("user@domain.com' UNION SELECT")).to.be.false; + }); + + it('should reject non-string values', () => { + expect(isValidUsername(null as any)).to.be.false; + expect(isValidUsername(undefined as any)).to.be.false; + expect(isValidUsername(123 as any)).to.be.false; + }); + }); + + describe('validateAndEscapeUsername', () => { + it('should return escaped valid usernames', () => { + expect(validateAndEscapeUsername('user@example.com')).to.equal('user@example.com'); + expect(validateAndEscapeUsername(' admin@test.org ')).to.equal('admin@test.org'); + expect(validateAndEscapeUsername('test.user@company.com')).to.equal('test.user@company.com'); + }); + + it('should escape single quotes in valid usernames', () => { + expect(validateAndEscapeUsername("o'brien@example.com")).to.equal("o\\'brien@example.com"); + }); + + it('should throw on invalid usernames', () => { + expect(() => validateAndEscapeUsername('invalid')).to.throw(/Invalid username format/); + expect(() => validateAndEscapeUsername('user@')).to.throw(/Invalid username format/); + expect(() => validateAndEscapeUsername('@domain.com')).to.throw(/Invalid username format/); + }); + + it('should throw on SQL injection attempts', () => { + expect(() => validateAndEscapeUsername("user@example.com' OR 1=1--")).to.throw(/SQL injection/); + expect(() => validateAndEscapeUsername("x@example.com' OR IsActive = true OR Username='x")).to.throw(/SQL injection/); + expect(() => validateAndEscapeUsername("admin@test.com; DROP TABLE")).to.throw(/SQL injection/); + }); + + it('should reject the proof of concept from the bug report', () => { + const pocUsername = "x@example.com' OR IsActive = true OR Username='x"; + expect(() => validateAndEscapeUsername(pocUsername)).to.throw(/SQL injection/); + }); + }); + + describe('Security - Prevent injection from bug W-22550508', () => { + it('should block the exact proof of concept attack', () => { + // From the bug report: + // "onBehalfOf": "x@example.com' OR IsActive = true OR Username='x" + const attackUsername = "x@example.com' OR IsActive = true OR Username='x"; + + // Should be detected as invalid + expect(isValidUsername(attackUsername)).to.be.false; + + // Should throw when trying to validate + expect(() => validateAndEscapeUsername(attackUsername)).to.throw(); + }); + + it('should allow legitimate usernames', () => { + const legitimateUsernames = [ + 'authorized-org-admin@example.com', + 'test-user@company.org', + 'admin+alias@domain.co.uk' + ]; + + legitimateUsernames.forEach(username => { + expect(isValidUsername(username), `Should accept: ${username}`).to.be.true; + expect(() => validateAndEscapeUsername(username)).to.not.throw(); + }); + }); + + it('should prevent predicate modification via various injection techniques', () => { + const injectionAttempts = [ + "user@test.com' OR 1=1--", + "admin@test.com'; DROP TABLE Users--", + "user@test.com' UNION SELECT * FROM User--", + "test@example.com' AND '1'='1", + "user@test.com' OR IsActive=true--" + ]; + + injectionAttempts.forEach(attempt => { + expect(() => validateAndEscapeUsername(attempt), `Should reject: ${attempt}`).to.throw(); + }); + }); + }); +});