diff --git a/packages/ENGINE-TEMPLATE/package.json b/packages/ENGINE-TEMPLATE/package.json index 68277a4f..17922219 100644 --- a/packages/ENGINE-TEMPLATE/package.json +++ b/packages/ENGINE-TEMPLATE/package.json @@ -14,7 +14,7 @@ "types": "dist/index.d.ts", "dependencies": { "@types/node": "^20.0.0", - "@salesforce/code-analyzer-engine-api": "0.41.0" + "@salesforce/code-analyzer-engine-api": "0.42.0-SNAPSHOT" }, "devDependencies": { "@eslint/js": "^9.39.5", diff --git a/packages/code-analyzer-apexguru-engine/package.json b/packages/code-analyzer-apexguru-engine/package.json index 9dcaa50c..df1e492f 100644 --- a/packages/code-analyzer-apexguru-engine/package.json +++ b/packages/code-analyzer-apexguru-engine/package.json @@ -13,7 +13,7 @@ "main": "dist/index.js", "types": "dist/index.d.ts", "dependencies": { - "@salesforce/code-analyzer-engine-api": "0.41.0", + "@salesforce/code-analyzer-engine-api": "0.42.0-SNAPSHOT", "@salesforce/core": "^8.31.2", "archiver": "^7.0.1", "form-data": "^4.0.6" diff --git a/packages/code-analyzer-core/package.json b/packages/code-analyzer-core/package.json index 0e886b54..85add65f 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.52.0", + "version": "0.53.0-SNAPSHOT", "author": "The Salesforce Code Analyzer Team", "license": "BSD-3-Clause", "homepage": "https://developer.salesforce.com/docs/platform/salesforce-code-analyzer/overview", @@ -16,7 +16,7 @@ }, "types": "dist/index.d.ts", "dependencies": { - "@salesforce/code-analyzer-engine-api": "0.41.0", + "@salesforce/code-analyzer-engine-api": "0.42.0-SNAPSHOT", "@types/node": "^20.0.0", "csv-stringify": "^6.8.1", "isbinaryfile": "^5.0.7", diff --git a/packages/code-analyzer-core/test/java-command-security.test.ts b/packages/code-analyzer-core/test/java-command-security.test.ts new file mode 100644 index 00000000..83dedbb2 --- /dev/null +++ b/packages/code-analyzer-core/test/java-command-security.test.ts @@ -0,0 +1,209 @@ +import {CodeAnalyzer, CodeAnalyzerConfig, EventType, LogEvent, RuleSelection} from "../src"; +import {changeWorkingDirectoryToPackageRoot, FakeFileSystem, FixedUniqueIdGenerator} from "./test-helpers"; +import * as engApi from "@salesforce/code-analyzer-engine-api"; + +/** + * Regression test for the H1 bug bounty finding: + * An auto-discovered code-analyzer.yml that sets engines.pmd.java_command to a repository-relative executable path + * (e.g. './scripts/afv-pmd-java') used to be spawned during the eager per-engine config extraction that happens on + * addEnginePlugin, even when the user only requested the eslint engine via 'sf code-analyzer rules --rule-selector + * eslint'. This resulted in arbitrary repo-controlled code execution. + * + * These tests lock in the two layers of the fix: + * 1. A relative java_command must be rejected during config validation WITHOUT ever spawning the binary (asserted by + * a JavaVersionIdentifier spy that fails the test if it is ever asked to identify the java version). + * 2. Core's per-engine try/catch isolation must let an eslint-only run continue to select the eslint rules even when + * the pmd engine failed to instantiate because of the malicious config (defense-in-depth ordering). + */ +describe("Security regression: relative java_command in an auto-discovered config is never spawned", () => { + changeWorkingDirectoryToPackageRoot(); + + let codeAnalyzer: CodeAnalyzer; + let logEvents: LogEvent[]; + + // The malicious auto-discovered config, expressed as engine overrides just like an auto-discovered + // code-analyzer.yml would be parsed into. + const MALICIOUS_CONFIG: object = { + engines: { + stubPmd: { + java_command: './scripts/afv-pmd-java' + } + } + }; + + beforeEach(() => { + codeAnalyzer = new CodeAnalyzer(CodeAnalyzerConfig.fromObject(MALICIOUS_CONFIG), new FakeFileSystem()); + codeAnalyzer._setUniqueIdGenerator(new FixedUniqueIdGenerator()); + logEvents = []; + codeAnalyzer.onEvent(EventType.LogEvent, (event: LogEvent) => logEvents.push(event)); + }); + + it("An eslint-only run with a malicious relative java_command never spawns the binary and still selects eslint rules", async () => { + const javaVersionSpy: ThrowIfCalledJavaVersionIdentifier = new ThrowIfCalledJavaVersionIdentifier(); + const pmdPlugin: StubPmdEnginePlugin = new StubPmdEnginePlugin(javaVersionSpy); + const eslintPlugin: StubEslintEnginePlugin = new StubEslintEnginePlugin(); + + // Adding the malicious PMD-like plugin must not throw even though its config extraction fails. + await expect(codeAnalyzer.addEnginePlugin(pmdPlugin)).resolves.not.toThrow(); + await expect(codeAnalyzer.addEnginePlugin(eslintPlugin)).resolves.not.toThrow(); + + // Acceptance criterion #2 & repro: the relative-path binary was never spawned. + expect(javaVersionSpy.wasCalled).toEqual(false); + + // Acceptance criterion #1: the pmd engine failed to instantiate (bad config) and was NOT added. + expect(codeAnalyzer.getEngineNames()).not.toContain('stubPmd'); + expect(() => codeAnalyzer.getEngineConfig('stubPmd')).toThrow(); + + // Per-engine failure isolation: the eslint engine was still added successfully. + expect(codeAnalyzer.getEngineNames()).toContain('stubEslint'); + + // Acceptance criterion #4: an eslint-only rule selection still succeeds and returns the eslint rules. + const selection: RuleSelection = await codeAnalyzer.selectRules(['stubEslint']); + expect(selection.getEngineNames()).toEqual(['stubEslint']); + expect(selection.getRulesFor('stubEslint').map(r => r.getName())).toEqual(['eslintRule']); + + // The pmd engine failure should have been surfaced as a log event rather than silently swallowed. + const errorLogMessages: string[] = logEvents.map(e => e.message); + expect(errorLogMessages.some(m => m.includes('stubPmd'))).toEqual(true); + }); + + it("A malicious relative java_command is rejected with a clear validation error mentioning the field and reason", async () => { + const javaVersionSpy: ThrowIfCalledJavaVersionIdentifier = new ThrowIfCalledJavaVersionIdentifier(); + await codeAnalyzer.addEnginePlugin(new StubPmdEnginePlugin(javaVersionSpy)); + + expect(javaVersionSpy.wasCalled).toEqual(false); + const combinedLogText: string = logEvents.map(e => e.message).join('\n'); + expect(combinedLogText).toContain(`The 'engines.stubPmd.java_command' configuration value is invalid.`); + expect(combinedLogText).toContain('relative file paths are not allowed'); + }); +}); + +/** + * A JavaVersionIdentifier spy that stands in for the real one that spawns 'java -version'. It fails the test if it is + * ever invoked, which is exactly what would happen if a relative java_command were passed through to be spawned. + */ +class ThrowIfCalledJavaVersionIdentifier { + wasCalled: boolean = false; + + identifyJavaVersion(_javaCommand: string): Promise { + this.wasCalled = true; + return Promise.reject(new Error('spawn must not be called for a relative java_command')); + } +} + +/** + * A minimal PMD-like engine plugin that mirrors the real pmd-engine's extractJavaCommand ordering: it validates the + * java_command value using the shared ValueValidator.validateJavaCommand helper (rejecting relative paths) BEFORE it + * ever asks the JavaVersionIdentifier to spawn the command. + */ +class StubPmdEnginePlugin extends engApi.EnginePluginV1 { + private readonly javaVersionIdentifier: ThrowIfCalledJavaVersionIdentifier; + + constructor(javaVersionIdentifier: ThrowIfCalledJavaVersionIdentifier) { + super(); + this.javaVersionIdentifier = javaVersionIdentifier; + } + + getAvailableEngineNames(): string[] { + return ['stubPmd']; + } + + describeEngineConfig(_engineName: string): engApi.ConfigDescription { + return {}; + } + + async createEngineConfig(engineName: string, configValueExtractor: engApi.ConfigValueExtractor): Promise { + const javaCommand: string | undefined = configValueExtractor.extractString('java_command'); + if (javaCommand) { + try { + // Reject relative paths BEFORE spawning, exactly like the real SharedConfigValueExtractor now does. + engApi.ValueValidator.validateJavaCommand(javaCommand, configValueExtractor.getFieldPath('java_command')); + await this.javaVersionIdentifier.identifyJavaVersion(javaCommand); + } catch (err) { + throw new Error(`The '${configValueExtractor.getFieldPath('java_command')}' configuration value is invalid. ${(err as Error).message}`, {cause: err}); + } + } + return {...configValueExtractor.getObject(), engine_name: engineName}; + } + + async createEngine(_engineName: string, config: engApi.ConfigObject): Promise { + return new StubPmdEngine(config); + } +} + +class StubPmdEngine extends engApi.Engine { + constructor(_config: engApi.ConfigObject) { + super(); + } + + getName(): string { + return 'stubPmd'; + } + + getEngineVersion(): Promise { + return Promise.resolve('0.0.1'); + } + + async describeRules(_describeOptions: engApi.DescribeOptions): Promise { + return [{ + name: 'pmdRule', + severityLevel: engApi.SeverityLevel.Moderate, + tags: ['Recommended'], + description: 'Some pmd rule', + resourceUrls: [] + }]; + } + + async runRules(_ruleNames: string[], _runOptions: engApi.RunOptions): Promise { + return {violations: []}; + } +} + +/** + * A minimal ESLint-like engine plugin whose config extraction has no java_command and thus always succeeds. + */ +class StubEslintEnginePlugin extends engApi.EnginePluginV1 { + getAvailableEngineNames(): string[] { + return ['stubEslint']; + } + + describeEngineConfig(_engineName: string): engApi.ConfigDescription { + return {}; + } + + async createEngineConfig(engineName: string, configValueExtractor: engApi.ConfigValueExtractor): Promise { + return {...configValueExtractor.getObject(), engine_name: engineName}; + } + + async createEngine(_engineName: string, config: engApi.ConfigObject): Promise { + return new StubEslintEngine(config); + } +} + +class StubEslintEngine extends engApi.Engine { + constructor(_config: engApi.ConfigObject) { + super(); + } + + getName(): string { + return 'stubEslint'; + } + + getEngineVersion(): Promise { + return Promise.resolve('0.0.1'); + } + + async describeRules(_describeOptions: engApi.DescribeOptions): Promise { + return [{ + name: 'eslintRule', + severityLevel: engApi.SeverityLevel.Moderate, + tags: ['Recommended'], + description: 'Some eslint rule', + resourceUrls: [] + }]; + } + + async runRules(_ruleNames: string[], _runOptions: engApi.RunOptions): Promise { + return {violations: []}; + } +} diff --git a/packages/code-analyzer-engine-api/package.json b/packages/code-analyzer-engine-api/package.json index 4b778906..6b67bd5f 100644 --- a/packages/code-analyzer-engine-api/package.json +++ b/packages/code-analyzer-engine-api/package.json @@ -1,7 +1,7 @@ { "name": "@salesforce/code-analyzer-engine-api", "description": "Engine API Package for the Salesforce Code Analyzer", - "version": "0.41.0", + "version": "0.42.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-engine-api/src/config.ts b/packages/code-analyzer-engine-api/src/config.ts index f089069c..7662f224 100644 --- a/packages/code-analyzer-engine-api/src/config.ts +++ b/packages/code-analyzer-engine-api/src/config.ts @@ -392,6 +392,31 @@ export class ValueValidator { return strValue; } + /** + * Validates that the provided value is a valid java command specifier: either the bare name of a command that is + * resolvable via the PATH (containing no file path separators) or an absolute file path. Relative file paths are + * rejected because they would resolve against the current working directory and could point to a + * repository-controlled executable. + * + * NOTE: This check mitigates relative-path execution but does not fully eliminate the threat of repo-controlled + * absolute paths in auto-discovered configs. Consider: should auto-discovered configs be allowed to override + * java_command at all for engines the user did not explicitly select? + * @param value the value that you wish to validate + * @param fieldPath the field path of the value as you want it to appear in validation error messages + */ + static validateJavaCommand(value: unknown, fieldPath: string): string { + const strValue: string = ValueValidator.validateString(value, fieldPath); + if (path.isAbsolute(strValue)) { + return strValue; + } + // A bare command name (no path separators) is resolved via the PATH environment variable and is allowed. + // Reject drive-relative paths (Windows-specific: 'C:foo' where path.isAbsolute('C:foo') == false). + if (!strValue.includes('/') && !strValue.includes('\\') && !strValue.includes(':')) { + return strValue; + } + throw new Error(getMessage('ConfigValueMustBeCommandNameOrAbsolutePath', fieldPath)); + } + /** * Validates that the provided value is a {@link SeverityLevel}. * @param rawValue the value that you wish to validate diff --git a/packages/code-analyzer-engine-api/src/messages.ts b/packages/code-analyzer-engine-api/src/messages.ts index 8236264a..64a5b15b 100644 --- a/packages/code-analyzer-engine-api/src/messages.ts +++ b/packages/code-analyzer-engine-api/src/messages.ts @@ -43,6 +43,10 @@ export const SHARED_MESSAGE_CATALOG: MessageCatalog = { ConfigValueMustMatchRegExp: `The '%s' configuration value is invalid. The string did not match the regular expression pattern: %s`, + ConfigValueMustBeCommandNameOrAbsolutePath: + `The '%s' configuration value is invalid. The value must either be the name of a command that exists on ` + + `the path (containing no file path separators) or an absolute file path; relative file paths are not allowed.`, + ConfigValueNotAValidSeverityLevel: `The '%s' configuration value must be one of the following: %s. Instead received: %s`, diff --git a/packages/code-analyzer-engine-api/test/config.test.ts b/packages/code-analyzer-engine-api/test/config.test.ts index 13753ffe..8d9d3196 100644 --- a/packages/code-analyzer-engine-api/test/config.test.ts +++ b/packages/code-analyzer-engine-api/test/config.test.ts @@ -60,6 +60,28 @@ describe("Tests for ValueValidator", () => { getMessage('ConfigValueMustMatchRegExp', 'someFieldName', '/^he.*/i')); }); + it("When a bare command name is given to validateJavaCommand, then the value is returned", () => { + expect(ValueValidator.validateJavaCommand('java', 'engines.pmd.java_command')).toEqual('java'); + }); + + it("When an absolute path is given to validateJavaCommand, then the value is returned unchanged", () => { + const absPath: string = path.resolve('/usr/bin/java'); + expect(ValueValidator.validateJavaCommand(absPath, 'engines.pmd.java_command')).toEqual(absPath); + // A raw absolute path should also be returned unchanged regardless of platform + expect(ValueValidator.validateJavaCommand('/usr/bin/java', 'engines.pmd.java_command')).toEqual('/usr/bin/java'); + }); + + it.each(['./scripts/afv-pmd-java', 'scripts/foo', '../x/java', 'a/b'])( + "When a relative path '%s' is given to validateJavaCommand, then error", (relPath) => { + expect(() => ValueValidator.validateJavaCommand(relPath, 'engines.pmd.java_command')).toThrow( + getMessage('ConfigValueMustBeCommandNameOrAbsolutePath', 'engines.pmd.java_command')); + }); + + it("When a non-string value is given to validateJavaCommand, then the validateString type error is thrown", () => { + expect(() => ValueValidator.validateJavaCommand(3, 'engines.pmd.java_command')).toThrow( + getMessage('ConfigValueMustBeOfType', 'engines.pmd.java_command', 'string', 'number')); + }); + it("When an object value is given to validateObject, then the value is returned", () => { expect(ValueValidator.validateObject({a:1}, 'someFieldName')).toEqual({a:1}); }); diff --git a/packages/code-analyzer-eslint-engine/package.json b/packages/code-analyzer-eslint-engine/package.json index 2d8f76f4..5f999ef2 100644 --- a/packages/code-analyzer-eslint-engine/package.json +++ b/packages/code-analyzer-eslint-engine/package.json @@ -1,7 +1,7 @@ { "name": "@salesforce/code-analyzer-eslint-engine", "description": "Plugin package that adds 'eslint' as an engine into Salesforce Code Analyzer", - "version": "0.46.0", + "version": "0.47.0-SNAPSHOT", "author": "The Salesforce Code Analyzer Team", "license": "BSD-3-Clause", "homepage": "https://developer.salesforce.com/docs/platform/salesforce-code-analyzer/overview", @@ -18,8 +18,8 @@ "@lwc/eslint-plugin-lwc": "^3.5.0", "@lwc/eslint-plugin-lwc-platform": "^6.3.0", "@salesforce-ux/eslint-plugin-slds": "^1.2.1", - "@salesforce/code-analyzer-engine-api": "0.41.0", - "@salesforce/code-analyzer-eslint8-engine": "0.18.0", + "@salesforce/code-analyzer-engine-api": "0.42.0-SNAPSHOT", + "@salesforce/code-analyzer-eslint8-engine": "0.19.0-SNAPSHOT", "@salesforce/eslint-config-lwc": "^4.1.2", "@salesforce/eslint-plugin-lightning": "^2.0.0", "@types/node": "^20.0.0", diff --git a/packages/code-analyzer-eslint8-engine/package.json b/packages/code-analyzer-eslint8-engine/package.json index 473bf057..7d133b08 100644 --- a/packages/code-analyzer-eslint8-engine/package.json +++ b/packages/code-analyzer-eslint8-engine/package.json @@ -1,7 +1,7 @@ { "name": "@salesforce/code-analyzer-eslint8-engine", "description": "Plugin package that adds 'eslint' (version 8) as an engine into Salesforce Code Analyzer", - "version": "0.18.0", + "version": "0.19.0-SNAPSHOT", "author": "The Salesforce Code Analyzer Team", "license": "BSD-3-Clause", "homepage": "https://developer.salesforce.com/docs/platform/salesforce-code-analyzer/overview", @@ -18,7 +18,7 @@ "@eslint/js": "8.57.1", "@lwc/eslint-plugin-lwc": "2.2.0", "@lwc/eslint-plugin-lwc-platform": "5.2.0", - "@salesforce/code-analyzer-engine-api": "0.41.0", + "@salesforce/code-analyzer-engine-api": "0.42.0-SNAPSHOT", "@salesforce/eslint-config-lwc": "3.7.2", "@salesforce/eslint-plugin-lightning": "1.0.1", "@types/node": "^20.0.0", diff --git a/packages/code-analyzer-flow-engine/package.json b/packages/code-analyzer-flow-engine/package.json index 2a9adc39..68e968ec 100644 --- a/packages/code-analyzer-flow-engine/package.json +++ b/packages/code-analyzer-flow-engine/package.json @@ -13,7 +13,7 @@ "main": "dist/index.js", "types": "dist/index.d.ts", "dependencies": { - "@salesforce/code-analyzer-engine-api": "0.41.0", + "@salesforce/code-analyzer-engine-api": "0.42.0-SNAPSHOT", "@types/node": "^20.0.0", "@types/semver": "^7.7.1", "semver": "^7.8.5" diff --git a/packages/code-analyzer-pmd-engine/package.json b/packages/code-analyzer-pmd-engine/package.json index 647b9d6b..61d61e03 100644 --- a/packages/code-analyzer-pmd-engine/package.json +++ b/packages/code-analyzer-pmd-engine/package.json @@ -1,7 +1,7 @@ { "name": "@salesforce/code-analyzer-pmd-engine", "description": "Plugin package that adds 'pmd' and 'cpd' as engines into Salesforce Code Analyzer", - "version": "0.45.0", + "version": "0.46.0-SNAPSHOT", "author": "The Salesforce Code Analyzer Team", "license": "BSD-3-Clause", "homepage": "https://developer.salesforce.com/docs/platform/salesforce-code-analyzer/overview", @@ -13,7 +13,7 @@ "main": "dist/index.js", "types": "dist/index.d.ts", "dependencies": { - "@salesforce/code-analyzer-engine-api": "0.41.0", + "@salesforce/code-analyzer-engine-api": "0.42.0-SNAPSHOT", "@types/node": "^20.0.0", "@types/semver": "^7.7.1", "semver": "^7.8.5" diff --git a/packages/code-analyzer-pmd-engine/src/config.ts b/packages/code-analyzer-pmd-engine/src/config.ts index dd4da090..d7a79210 100644 --- a/packages/code-analyzer-pmd-engine/src/config.ts +++ b/packages/code-analyzer-pmd-engine/src/config.ts @@ -222,6 +222,10 @@ abstract class SharedConfigValueExtractor { } try { + // Reject relative file paths (only bare PATH command names or absolute paths are allowed) BEFORE we + // ever spawn the command to check its version, otherwise a repo-controlled relative executable path in + // an auto-discovered config could be executed. + ValueValidator.validateJavaCommand(javaCommand, this.configValueExtractor.getFieldPath('java_command')); await this.validateJavaCommandContainsValidVersion(javaCommand); } catch (err) { throw new Error(getMessage('InvalidUserSpecifiedJavaCommand', diff --git a/packages/code-analyzer-pmd-engine/test/plugin.test.ts b/packages/code-analyzer-pmd-engine/test/plugin.test.ts index efa5b9c2..160ca7ad 100644 --- a/packages/code-analyzer-pmd-engine/test/plugin.test.ts +++ b/packages/code-analyzer-pmd-engine/test/plugin.test.ts @@ -139,6 +139,41 @@ describe('Tests for the PmdCpdEnginesPlugin', () => { getMessage('JavaBelowMinimumVersion', '/some/java', '1.9.0', '11.0.0'))); }); + it.each([ + {engineName: 'cpd', javaCommand: './scripts/afv-pmd-java'}, + {engineName: 'pmd', javaCommand: './scripts/afv-pmd-java'}, + {engineName: 'cpd', javaCommand: 'scripts/foo'}, + {engineName: 'pmd', javaCommand: 'scripts/foo'}, + {engineName: 'cpd', javaCommand: '../foo'}, + {engineName: 'pmd', javaCommand: '../foo'} + ])(`When createEngineConfig for '$engineName' is given a relative java_command '$javaCommand', then reject it without ever spawning the command`, async ({engineName, javaCommand}) => { + const throwingStub: ThrowIfCalledJavaVersionIdentifier = new ThrowIfCalledJavaVersionIdentifier(); + const pluginWithStub: PmdCpdEnginesPlugin = new PmdCpdEnginesPlugin(throwingStub); + try { + await pluginWithStub.createEngineConfig(engineName, new ConfigValueExtractor({java_command: javaCommand}, `engines.${engineName}`)); + fail('Expected error to be thrown'); + } catch (err) { + const errMsg: string = (err as Error).message; + expect(errMsg).toContain(`The 'engines.${engineName}.java_command' configuration value is invalid.`); + expect(errMsg).toContain('relative file paths are not allowed'); + } + expect(throwingStub.wasCalled).toEqual(false); + }); + + it.each(['cpd','pmd'])(`When createEngineConfig for '%s' is given an absolute path java_command, then it still resolves (no spawn regression)`, async (engineName) => { + const pluginWithStub: PmdCpdEnginesPlugin = new PmdCpdEnginesPlugin(new StubJavaVersionIdentifier(new SemVer('21.4.0'))); + const normalizedConfig: PmdEngineConfig = await pluginWithStub.createEngineConfig(engineName, + new ConfigValueExtractor({java_command: '/some/java'}, `engines.${engineName}`)) as PmdEngineConfig; + expect(normalizedConfig.java_command).toEqual('/some/java'); + }); + + it.each(['cpd','pmd'])(`When createEngineConfig for '%s' is given a bare command name java_command, then it still resolves`, async (engineName) => { + const pluginWithStub: PmdCpdEnginesPlugin = new PmdCpdEnginesPlugin(new StubJavaVersionIdentifier(new SemVer('21.4.0'))); + const normalizedConfig: PmdEngineConfig = await pluginWithStub.createEngineConfig(engineName, + new ConfigValueExtractor({java_command: 'java'}, `engines.${engineName}`)) as PmdEngineConfig; + expect(normalizedConfig.java_command).toEqual('java'); + }); + it.each(['cpd','pmd'])(`When createEngineConfig for '%s' is given a java_command that is greater than the minimum required, then use it`, async (engineName) => { const pluginWithStub: PmdCpdEnginesPlugin = new PmdCpdEnginesPlugin(new StubJavaVersionIdentifier(new SemVer('21.4.0'))); const rawConfig: ConfigObject = {java_command: '/some/java'}; @@ -464,4 +499,14 @@ class StubJavaVersionIdentifier implements JavaVersionIdentifier { async identifyJavaVersion(_javaCommand: string): Promise { return this.version; } -} \ No newline at end of file +} + +// Fails the test if identifyJavaVersion is ever called (i.e. if the java_command binary would ever be spawned). +class ThrowIfCalledJavaVersionIdentifier implements JavaVersionIdentifier { + wasCalled: boolean = false; + + async identifyJavaVersion(_javaCommand: string): Promise { + this.wasCalled = true; + throw new Error('spawn must not be called for a relative java_command'); + } +} diff --git a/packages/code-analyzer-regex-engine/package.json b/packages/code-analyzer-regex-engine/package.json index 8415a560..45d69bf3 100644 --- a/packages/code-analyzer-regex-engine/package.json +++ b/packages/code-analyzer-regex-engine/package.json @@ -1,7 +1,7 @@ { "name": "@salesforce/code-analyzer-regex-engine", "description": "Plugin package that adds 'regex' as an engine into Salesforce Code Analyzer", - "version": "0.39.0", + "version": "0.40.0-SNAPSHOT", "author": "The Salesforce Code Analyzer Team", "license": "BSD-3-Clause", "homepage": "https://developer.salesforce.com/docs/platform/salesforce-code-analyzer/overview", @@ -13,7 +13,7 @@ "main": "dist/index.js", "types": "dist/index.d.ts", "dependencies": { - "@salesforce/code-analyzer-engine-api": "0.41.0", + "@salesforce/code-analyzer-engine-api": "0.42.0-SNAPSHOT", "@types/node": "^20.0.0", "isbinaryfile": "^5.0.7", "p-limit": "^3.1.0" diff --git a/packages/code-analyzer-retirejs-engine/package.json b/packages/code-analyzer-retirejs-engine/package.json index a578e5ea..3127e0f9 100644 --- a/packages/code-analyzer-retirejs-engine/package.json +++ b/packages/code-analyzer-retirejs-engine/package.json @@ -1,7 +1,7 @@ { "name": "@salesforce/code-analyzer-retirejs-engine", "description": "Plugin package that adds 'retire-js' as an engine into Salesforce Code Analyzer", - "version": "0.38.0", + "version": "0.39.0-SNAPSHOT", "author": "The Salesforce Code Analyzer Team", "license": "BSD-3-Clause", "homepage": "https://developer.salesforce.com/docs/platform/salesforce-code-analyzer/overview", @@ -13,7 +13,7 @@ "main": "dist/index.js", "types": "dist/index.d.ts", "dependencies": { - "@salesforce/code-analyzer-engine-api": "0.41.0", + "@salesforce/code-analyzer-engine-api": "0.42.0-SNAPSHOT", "@types/node": "^20.0.0", "isbinaryfile": "^5.0.7", "node-stream-zip": "^1.15.0", diff --git a/packages/code-analyzer-sfge-engine/package.json b/packages/code-analyzer-sfge-engine/package.json index 1f029c77..a02698fd 100644 --- a/packages/code-analyzer-sfge-engine/package.json +++ b/packages/code-analyzer-sfge-engine/package.json @@ -1,7 +1,7 @@ { "name": "@salesforce/code-analyzer-sfge-engine", "description": "Plugin package that adds 'Salesforce Graph Engine' as an engine into Salesforce Code Analyzer", - "version": "0.24.0", + "version": "0.25.0-SNAPSHOT", "author": "The Salesforce Code Analyzer Team", "license": "BSD-3-Clause", "homepage": "https://developer.salesforce.com/docs/platform/salesforce-code-analyzer/overview", @@ -13,7 +13,7 @@ "main": "dist/index.js", "types": "dist/index.d.ts", "dependencies": { - "@salesforce/code-analyzer-engine-api": "0.41.0", + "@salesforce/code-analyzer-engine-api": "0.42.0-SNAPSHOT", "@types/node": "^20.0.0", "semver": "^7.8.5" }, diff --git a/packages/code-analyzer-sfge-engine/src/config.ts b/packages/code-analyzer-sfge-engine/src/config.ts index 6eea7783..b078176d 100644 --- a/packages/code-analyzer-sfge-engine/src/config.ts +++ b/packages/code-analyzer-sfge-engine/src/config.ts @@ -1,6 +1,7 @@ import { ConfigDescription, ConfigValueExtractor, + ValueValidator, } from "@salesforce/code-analyzer-engine-api"; import {indent} from '@salesforce/code-analyzer-engine-api/utils'; import {getMessage} from "./messages"; @@ -109,6 +110,10 @@ class SfgeConfigValueExtractor { } try { + // Reject relative file paths (only bare PATH command names or absolute paths are allowed) BEFORE we + // ever spawn the command to check its version, otherwise a repo-controlled relative executable path in + // an auto-discovered config could be executed. + ValueValidator.validateJavaCommand(javaCommand, this.delegateExtractor.getFieldPath('java_command')); await this.validateJavaCommandContainsValidVersion(javaCommand); } catch (err) { throw new Error(getMessage('InvalidConfigValue', diff --git a/packages/code-analyzer-sfge-engine/test/plugin.test.ts b/packages/code-analyzer-sfge-engine/test/plugin.test.ts index 2af8174c..caf93d7a 100644 --- a/packages/code-analyzer-sfge-engine/test/plugin.test.ts +++ b/packages/code-analyzer-sfge-engine/test/plugin.test.ts @@ -101,6 +101,20 @@ describe('SfgeEnginePlugin', () => { javaVersionIdentifierBuilder: () => new StubJavaVersionIdentifier(new SemVer('1.9.0')), mainMessage: `The 'engines.sfge.java_command' configuration value is invalid.`, reasonMessage: `The command '/some/version/of/java' specifies Java v1.9.0, which is below minimum supported version v11.0.0.`, + }, + { + case: 'specified as a relative path', + configObject: {java_command: './scripts/afv-pmd-java'} as ConfigObject, + javaVersionIdentifierBuilder: () => new ThrowIfCalledJavaVersionIdentifier(), + mainMessage: `The 'engines.sfge.java_command' configuration value is invalid.`, + reasonMessage: 'relative file paths are not allowed' + }, + { + case: 'specified as a relative path without a leading dot', + configObject: {java_command: 'scripts/foo'} as ConfigObject, + javaVersionIdentifierBuilder: () => new ThrowIfCalledJavaVersionIdentifier(), + mainMessage: `The 'engines.sfge.java_command' configuration value is invalid.`, + reasonMessage: 'relative file paths are not allowed' } ])('When java_command is $case, an error is thrown', async ({configObject, javaVersionIdentifierBuilder, mainMessage, reasonMessage}) => { const pluginWithStub: SfgeEnginePlugin = new SfgeEnginePlugin(javaVersionIdentifierBuilder()); @@ -121,6 +135,16 @@ describe('SfgeEnginePlugin', () => { const normalizedConfig: ConfigObject = await pluginWithStub.createEngineConfig('sfge', configValueExtractor); expect(normalizedConfig).toHaveProperty('java_command', '/some/java'); }); + + it('When java_command is a relative path, it is rejected before the command is ever spawned', async () => { + const throwingStub: ThrowIfCalledJavaVersionIdentifier = new ThrowIfCalledJavaVersionIdentifier(); + const pluginWithStub: SfgeEnginePlugin = new SfgeEnginePlugin(throwingStub); + const configValueExtractor: ConfigValueExtractor = + new ConfigValueExtractor({java_command: './scripts/afv-pmd-java'}, 'engines.sfge'); + await expect(pluginWithStub.createEngineConfig('sfge', configValueExtractor)).rejects.toThrow( + `The 'engines.sfge.java_command' configuration value is invalid.`); + expect(throwingStub.wasCalled).toEqual(false); + }); }); describe(`Validating the boolean 'disable_limit_reached_violations' property`, () => { @@ -296,3 +320,13 @@ class StubJavaVersionIdentifier implements JavaVersionIdentifier { return Promise.resolve(this.version); } } + +// Fails the test if identifyJavaVersion is ever called (i.e. if the java_command binary would ever be spawned). +class ThrowIfCalledJavaVersionIdentifier implements JavaVersionIdentifier { + wasCalled: boolean = false; + + public identifyJavaVersion(_javaCommand: string): Promise { + this.wasCalled = true; + return Promise.reject(new Error('spawn must not be called for a relative java_command')); + } +}