From ae5dd6a0f49f6c4c9ec9d6f0ea88ac1389981439 Mon Sep 17 00:00:00 2001 From: Nikhil Mittal Date: Tue, 14 Jul 2026 22:09:59 +0530 Subject: [PATCH] security: remove custom_engine_plugin_modules and dynamicallyAddEnginePlugin (RCE fix) Remove the undocumented custom_engine_plugin_modules config field and the dynamicallyAddEnginePlugin method that together enabled arbitrary code execution via dynamic import of user-controlled paths. - Remove CUSTOM_ENGINE_PLUGIN_MODULES from FIELDS, TopLevelConfig type, DEFAULT_CONFIG, and bypass-validation list so the field is now actively rejected by schema validation as an invalid key - Remove getCustomEnginePluginModules() accessor from CodeAnalyzerConfig - Remove dynamicallyAddEnginePlugin() method and dynamicallyImport() helper from CodeAnalyzer class - Remove pathToFileURL import (no longer needed) - Remove FailedToDynamicallyLoadModule and FailedToDynamicallyAddEnginePlugin error messages - Add security tests asserting config rejection and method absence - Update test fixtures and existing tests for removed functionality --- packages/code-analyzer-core/package.json | 2 +- .../code-analyzer-core/src/code-analyzer.ts | 43 ------------------- packages/code-analyzer-core/src/config.ts | 15 +------ packages/code-analyzer-core/src/messages.ts | 6 --- .../test/add-engines.test.ts | 25 ++--------- .../code-analyzer-core/test/config.test.ts | 22 +++++----- .../test/test-data/sample-config-02b.Yml | 12 ++++++ 7 files changed, 29 insertions(+), 96 deletions(-) create mode 100644 packages/code-analyzer-core/test/test-data/sample-config-02b.Yml diff --git a/packages/code-analyzer-core/package.json b/packages/code-analyzer-core/package.json index 4d4fe3da..5fdbc898 100644 --- a/packages/code-analyzer-core/package.json +++ b/packages/code-analyzer-core/package.json @@ -1,7 +1,7 @@ { "name": "@salesforce/code-analyzer-core", "description": "Core Package for the Salesforce Code Analyzer", - "version": "0.49.0", + "version": "0.50.0-SNAPSHOT", "author": "The Salesforce Code Analyzer Team", "license": "BSD-3-Clause", "homepage": "https://developer.salesforce.com/docs/platform/salesforce-code-analyzer/overview", diff --git a/packages/code-analyzer-core/src/code-analyzer.ts b/packages/code-analyzer-core/src/code-analyzer.ts index de91ee60..c1583b95 100644 --- a/packages/code-analyzer-core/src/code-analyzer.ts +++ b/packages/code-analyzer-core/src/code-analyzer.ts @@ -1,4 +1,3 @@ -import {pathToFileURL} from "node:url"; import {RuleImpl, RuleSelection, RuleSelectionImpl} from "./rules" import { EngineRunResults, @@ -235,40 +234,6 @@ export class CodeAnalyzer { await Promise.all(promises); } - /** - * Dynamically loads a module containing an {@link EnginePlugin} and adds its engines to Code Analyzer - * Note that the module must export a createEnginePlugin() function that returns an EnginePlugin. - * @param enginePluginModulePath string containing a discoverable name or location of an engine plugin module - */ - public async dynamicallyAddEnginePlugin(enginePluginModulePath: string): Promise { - let pluginModule; - let resolvedModulePath: string; - try { - try { - resolvedModulePath = require.resolve(enginePluginModulePath, {paths: [this.config.getConfigRoot()]}); - } catch (err) /* istanbul ignore next */ { - // On windows, there is an edge case where a standalone file in the same directory as the user's config - // file may not be resolved by require.resolve if given as just the file name. So we attempt to resolve - // this using path.resolve for this edge case. - this.emitLogEvent(LogLevel.Fine, `While dynamically importing '${enginePluginModulePath}', ` + - `require.resolve failed with the following exception, so we will attempt to resolve with ` + - `path.resolve instead.\nError:\n` + - (err instanceof Error) ? (err as Error).stack || (err as Error).message : (err as string)); - resolvedModulePath = path.resolve(this.config.getConfigRoot(), enginePluginModulePath); - } - - pluginModule = await dynamicallyImport(resolvedModulePath); - } catch (err) { - throw new Error(getMessage('FailedToDynamicallyLoadModule', enginePluginModulePath, (err as Error).message), {cause: err}); - } - - if (typeof pluginModule.createEnginePlugin !== 'function') { - throw new Error(getMessage('FailedToDynamicallyAddEnginePlugin', enginePluginModulePath)); - } - const enginePlugin: engApi.EnginePlugin = pluginModule.createEnginePlugin(); - return this.addEnginePlugin(enginePlugin); - } - /** * Returns the names of the engines that have been added to Code Analyzer */ @@ -1141,11 +1106,3 @@ function findCaseInsensitiveKey(obj: object, key: string): string | undefined { return Object.keys(obj).find(k => k.toLowerCase() === key.toLowerCase()); } -// eslint-disable-next-line @typescript-eslint/no-explicit-any -async function dynamicallyImport(absJavaScriptFilePath: string): Promise { - // To avoid issues with dynamically importing absolute paths on Windows, we need to convert to url with pathToFileURL. - const moduleUrl: string = pathToFileURL(absJavaScriptFilePath).href; - const pluginModule = await import(moduleUrl); - /* istanbul ignore next */ - return pluginModule.default ?? pluginModule; // Return the default export if it exists, otherwise the module itself -} diff --git a/packages/code-analyzer-core/src/config.ts b/packages/code-analyzer-core/src/config.ts index fc6eb5e3..b53f73f2 100644 --- a/packages/code-analyzer-core/src/config.ts +++ b/packages/code-analyzer-core/src/config.ts @@ -14,7 +14,6 @@ export const FIELDS = { LOG_FOLDER: 'log_folder', LOG_LEVEL: 'log_level', ROOT_WORKING_FOLDER: 'root_working_folder', // Hidden - CUSTOM_ENGINE_PLUGIN_MODULES: 'custom_engine_plugin_modules', // Hidden PRESERVE_ALL_WORKING_FOLDERS: 'preserve_all_working_folders', // Hidden SUPPRESSIONS: 'suppressions', DISABLE_SUPPRESSIONS: 'disable_suppressions', @@ -77,7 +76,6 @@ type TopLevelConfig = { suppressions: Suppressions root_working_folder: string, // INTERNAL USE ONLY preserve_all_working_folders: boolean // INTERNAL USE ONLY - custom_engine_plugin_modules: string[] // INTERNAL USE ONLY } // Only exported internally to help with testing @@ -91,7 +89,6 @@ export const DEFAULT_CONFIG: TopLevelConfig = { ignores: { files: [] }, root_working_folder: os.tmpdir(), // INTERNAL USE ONLY preserve_all_working_folders: false, // INTERNAL USE ONLY - custom_engine_plugin_modules: [], // INTERNAL USE ONLY }; /** @@ -176,16 +173,13 @@ export class CodeAnalyzerConfig { configRoot = !rawConfig.config_root ? (configRoot ?? process.cwd()) : validateAbsoluteFolder(rawConfig.config_root, FIELDS.CONFIG_ROOT); const configExtractor: engApi.ConfigValueExtractor = new engApi.ConfigValueExtractor(rawConfig, '', configRoot); - configExtractor.addKeysThatBypassValidation([FIELDS.CUSTOM_ENGINE_PLUGIN_MODULES, FIELDS.PRESERVE_ALL_WORKING_FOLDERS, FIELDS.ROOT_WORKING_FOLDER]); // Hidden fields bypass validation + configExtractor.addKeysThatBypassValidation([FIELDS.PRESERVE_ALL_WORKING_FOLDERS, FIELDS.ROOT_WORKING_FOLDER]); // Hidden fields bypass validation configExtractor.validateContainsOnlySpecifiedKeys([FIELDS.CONFIG_ROOT, FIELDS.LOG_FOLDER, FIELDS.LOG_LEVEL, FIELDS.RULES, FIELDS.ENGINES, FIELDS.IGNORES, FIELDS.SUPPRESSIONS]); const config: TopLevelConfig = { config_root: configRoot, log_folder: configExtractor.extractFolder(FIELDS.LOG_FOLDER, DEFAULT_CONFIG.log_folder)!, log_level: extractLogLevel(configExtractor), suppressions: extractSuppressionsValue(configExtractor), - custom_engine_plugin_modules: configExtractor.extractArray(FIELDS.CUSTOM_ENGINE_PLUGIN_MODULES, - engApi.ValueValidator.validateString, - DEFAULT_CONFIG.custom_engine_plugin_modules)!, root_working_folder: configExtractor.extractFolder(FIELDS.ROOT_WORKING_FOLDER, DEFAULT_CONFIG.root_working_folder)!, preserve_all_working_folders: configExtractor.extractBoolean(FIELDS.PRESERVE_ALL_WORKING_FOLDERS, DEFAULT_CONFIG.preserve_all_working_folders)!, rules: extractRulesValue(configExtractor), @@ -298,13 +292,6 @@ export class CodeAnalyzerConfig { return this.config.config_root; } - /** - * Returns any user-specified custom engine plugin modules that have been specified in the configuration. - */ - public getCustomEnginePluginModules(): string[] { - return this.config.custom_engine_plugin_modules; - } - /** * Returns a boolean indicating whether working folders are always preserved. * If false, then the working folders are deleted unless the engine issues an error. diff --git a/packages/code-analyzer-core/src/messages.ts b/packages/code-analyzer-core/src/messages.ts index c54807a5..6bcabc57 100644 --- a/packages/code-analyzer-core/src/messages.ts +++ b/packages/code-analyzer-core/src/messages.ts @@ -120,12 +120,6 @@ const MESSAGE_CATALOG : MessageCatalog = { ` %s:\n` + ` disable_engine: true`, - FailedToDynamicallyLoadModule: - `Failed to dynamically load module '%s'. Error: %s`, - - FailedToDynamicallyAddEnginePlugin: - `Failed to dynamically add engine plugin from module '%s' because the module does not seem to export a 'createEnginePlugin' function.`, - FailedToGetEngineConfig: `Failed to get configuration for engine with name '%s' since this engine has not been added to Code Analyzer.`, diff --git a/packages/code-analyzer-core/test/add-engines.test.ts b/packages/code-analyzer-core/test/add-engines.test.ts index 793f7a95..1306d98d 100644 --- a/packages/code-analyzer-core/test/add-engines.test.ts +++ b/packages/code-analyzer-core/test/add-engines.test.ts @@ -60,7 +60,7 @@ describe("Tests for adding engines to Code Analyzer", () => { }); it('When adding engine plugin using non-default config then engines are correctly added with engine specific configurations', async () => { - codeAnalyzer = createCodeAnalyzer(CodeAnalyzerConfig.fromFile(path.join(TEST_DATA_DIR, 'sample-config-02.Yml'))); + codeAnalyzer = createCodeAnalyzer(CodeAnalyzerConfig.fromFile(path.join(TEST_DATA_DIR, 'sample-config-02b.Yml'))); const stubEnginePlugin: stubs.StubEnginePlugin = new stubs.StubEnginePlugin(); await codeAnalyzer.addEnginePlugin(stubEnginePlugin); @@ -145,27 +145,8 @@ describe("Tests for adding engines to Code Analyzer", () => { expect(codeAnalyzer.getEngineNames().sort()).toEqual([]); }) - it('When calling dynamicallyAddEnginePlugin on a module that has a createEnginePlugin function, then it is used to create the plugin and then add it', async () => { - const pluginModulePath: string = require.resolve('./stubs'); - await codeAnalyzer.dynamicallyAddEnginePlugin(pluginModulePath); - expect(codeAnalyzer.getEngineNames().sort()).toEqual(["stubEngine1", "stubEngine2", "stubEngine3"]); - }); - - it('When calling dynamicallyAddEnginePlugin on a module that is missing a createEnginePlugin function, then an error is thrown', async () => { - const badPluginModulePath: string = require.resolve('./test-helpers'); - await expect(codeAnalyzer.dynamicallyAddEnginePlugin(badPluginModulePath)).rejects.toThrow( - getMessage('FailedToDynamicallyAddEnginePlugin', badPluginModulePath)); - }); - - it('When calling dynamicallyAddEnginePlugin on a module that does not exist, then an error is thrown', async () => { - const expectedErrorMessageSubstring: string = getMessage('FailedToDynamicallyLoadModule', 'doesNotExist', ''); - await expect(codeAnalyzer.dynamicallyAddEnginePlugin('doesNotExist')).rejects.toThrow(expectedErrorMessageSubstring); - }); - - it('When calling dynamicallyAddEnginePlugin on a file that is not a module, then an error is thrown', async () => { - const nonModuleFile: string = path.resolve('package.json'); - const expectedErrorMessageSubstring: string = getMessage('FailedToDynamicallyAddEnginePlugin', nonModuleFile); - await expect(codeAnalyzer.dynamicallyAddEnginePlugin(nonModuleFile)).rejects.toThrow(expectedErrorMessageSubstring); + it('[Security] dynamicallyAddEnginePlugin method does not exist on CodeAnalyzer', () => { + expect((codeAnalyzer as any).dynamicallyAddEnginePlugin).toBeUndefined(); }); it('When an engine is disabled, then addEnginePlugin does not add that particular engine and gives debug log', async () => { diff --git a/packages/code-analyzer-core/test/config.test.ts b/packages/code-analyzer-core/test/config.test.ts index 3dfa8044..459c3a2e 100644 --- a/packages/code-analyzer-core/test/config.test.ts +++ b/packages/code-analyzer-core/test/config.test.ts @@ -18,7 +18,6 @@ describe("Tests for creating and accessing configuration values", () => { expect(conf.getConfigRoot()).toEqual(DEFAULT_CONFIG_ROOT); expect(conf.getLogFolder()).toEqual(os.tmpdir()); expect(conf.getLogLevel()).toEqual(LogLevel.Debug); - expect(conf.getCustomEnginePluginModules()).toEqual([]); expect(conf.getPreserveAllWorkingFolders()).toEqual(false); expect(conf.getRootWorkingFolder()).toEqual(os.tmpdir()); expect(conf.getRuleOverridesFor("stubEngine1")).toEqual({}); @@ -79,11 +78,15 @@ describe("Tests for creating and accessing configuration values", () => { }); }); - it("When constructing config from file with yml extension then it is parsed as a yaml file", () => { + it("When constructing config from file with yml extension containing custom_engine_plugin_modules then it is rejected as invalid key", () => { + expect(() => CodeAnalyzerConfig.fromFile(path.join(TEST_DATA_DIR, 'sample-config-02.Yml'))).toThrow( + "invalid key 'custom_engine_plugin_modules'"); + }); + + it("When constructing config from file with yml extension (without custom_engine_plugin_modules) then it is parsed as a yaml file", () => { // Also note that Yml should work just like yml. Case doesn't matter. - const conf: CodeAnalyzerConfig = CodeAnalyzerConfig.fromFile(path.join(TEST_DATA_DIR, 'sample-config-02.Yml')); + const conf: CodeAnalyzerConfig = CodeAnalyzerConfig.fromFile(path.join(TEST_DATA_DIR, 'sample-config-02b.Yml')); expect(conf.getLogFolder()).toEqual(os.tmpdir()); - expect(conf.getCustomEnginePluginModules()).toEqual(['dummy_plugin_module_path']); expect(conf.getPreserveAllWorkingFolders()).toEqual(true); expect(conf.getRootWorkingFolder()).toEqual(os.tmpdir()); expect(conf.getRuleOverridesFor('stubEngine1')).toEqual({}); @@ -107,7 +110,6 @@ describe("Tests for creating and accessing configuration values", () => { it("When constructing config from json file then values from file are parsed correctly", () => { const conf: CodeAnalyzerConfig = CodeAnalyzerConfig.fromFile(path.join(TEST_DATA_DIR, 'sample-config-03.json')); expect(conf.getLogFolder()).toEqual(path.join(TEST_DATA_DIR, 'sampleLogFolder')); - expect(conf.getCustomEnginePluginModules()).toEqual([]); expect(conf.getPreserveAllWorkingFolders()).toEqual(false); expect(conf.getRuleOverridesFor('stubEngine1')).toEqual({}); expect(conf.getRuleOverridesFor('stubEngine2')).toEqual({}); @@ -347,12 +349,12 @@ describe("Tests for creating and accessing configuration values", () => { getMessage('ConfigValueNotAValidEnumValue', 'log_level', '["Error","Warn","Info","Debug","Fine",1,2,3,4,5]', '0')); }); - it("When custom_engine_plugin_modules is not a string array, then throw an error", () => { - expect(() => CodeAnalyzerConfig.fromObject({custom_engine_plugin_modules: 3})).toThrow( - getMessageFromCatalog(SHARED_MESSAGE_CATALOG, 'ConfigValueMustBeOfType','custom_engine_plugin_modules', 'array', 'number')); + it("[Security] When custom_engine_plugin_modules is provided, it is rejected as an invalid config key", () => { + expect(() => CodeAnalyzerConfig.fromObject({custom_engine_plugin_modules: ['./evil.js']})).toThrow( + "invalid key 'custom_engine_plugin_modules'"); - expect(() => CodeAnalyzerConfig.fromObject({custom_engine_plugin_modules: 'oops'})).toThrow( - getMessageFromCatalog(SHARED_MESSAGE_CATALOG, 'ConfigValueMustBeOfType','custom_engine_plugin_modules', 'array', 'string')); + expect(() => CodeAnalyzerConfig.fromObject({custom_engine_plugin_modules: []})).toThrow( + "invalid key 'custom_engine_plugin_modules'"); }); it("When preserve_all_working_folders is not a boolean, then throw an error", () => { diff --git a/packages/code-analyzer-core/test/test-data/sample-config-02b.Yml b/packages/code-analyzer-core/test/test-data/sample-config-02b.Yml new file mode 100644 index 00000000..aa8ddec2 --- /dev/null +++ b/packages/code-analyzer-core/test/test-data/sample-config-02b.Yml @@ -0,0 +1,12 @@ +preserve_all_working_folders: true +rules: + stubEngine2: + stub2RuleC: + severity: 3 + +engines: + stubEngine1: + miscSetting1: true + miscSetting2: + miscSetting2A: 3 + miscSetting2B: ["hello", "world"]