diff --git a/.github/workflows/publish-package-to-npm.yml b/.github/workflows/publish-package-to-npm.yml index 40698451..fdf5cd16 100644 --- a/.github/workflows/publish-package-to-npm.yml +++ b/.github/workflows/publish-package-to-npm.yml @@ -172,6 +172,10 @@ jobs: build-and-test-and-publish: runs-on: ubuntu-latest needs: [validate-packages-as-releasable, prepare-release-branch] + permissions: + # Required for npm Trusted Publishing (OIDC). + id-token: write + contents: read steps: - name: Support long paths run: git config --global core.longpaths true @@ -180,7 +184,21 @@ jobs: ref: ${{ needs.prepare-release-branch.outputs.branch-name }} - uses: actions/setup-node@v4 with: - node-version: 20 + # Trusted Publishing requires Node >= 22.14.0. + node-version: 22 + # Trusted Publishing requires npm >= 11.5.1, and managing dist-tags via OIDC requires npm >= 11.21.0. + # Pinned to the npm 11 line to stay consistent with the code-analyzer repo (npm 12 removed shrinkwrap support). + - name: Upgrade npm for Trusted Publishing + run: | + npm install -g npm@11 + # Print the resolved version, and fail fast if the moving 'npm@11' tag ever resolves below what + # dist-tag management via OIDC needs, rather than failing later in the middle of a release. + NPM_VERSION="$(npm --version)" + echo "Using npm ${NPM_VERSION}" + if [ "$(printf '%s\n' "11.21.0" "${NPM_VERSION}" | sort -V | head -n1)" != "11.21.0" ]; then + echo "::error::npm ${NPM_VERSION} is older than the required 11.21.0" + exit 1 + fi - name: Build and test and publish packages run: | PACKAGES_TO_PUBLISH=(${{ needs.validate-packages-as-releasable.outputs.packages-to-release }}) @@ -191,8 +209,8 @@ jobs: npm install npm run build npm run test - # We need the NPM token to publish. - echo "//registry.npmjs.org/:_authToken=${{ secrets.NPM_TOKEN }}" > ~/.npmrc + # No NPM token is needed. npm authenticates via Trusted Publishing (OIDC). Each package must have this + # workflow configured as a trusted publisher on npmjs.com, with "Allow npm dist-tag" enabled. PUBLISHED_VERSION=$(jq -r ".version" package.json) if [[ "${{ inputs.dryrun }}" == "true" ]]; then npm publish --tag latest-beta --access public --verbose --dry-run diff --git a/packages/code-analyzer-flow-engine/FlowScanner/flow_parser/parse.py b/packages/code-analyzer-flow-engine/FlowScanner/flow_parser/parse.py index b14dd479..2eb617b5 100644 --- a/packages/code-analyzer-flow-engine/FlowScanner/flow_parser/parse.py +++ b/packages/code-analyzer-flow-engine/FlowScanner/flow_parser/parse.py @@ -763,7 +763,14 @@ def get_run_mode(self) -> RunMode: if len(elems) == 0: return RunMode.DefaultMode else: - return RunMode[elems[0].text] + declared = (elems[0].text or '').strip() + try: + return RunMode[declared] + except KeyError: + # A runInMode value we don't know about (for example, one added in a newer API version) must not crash + # the whole scan. Fall back to the default (user context) behavior and say so in the log. + logger.warning(f"Unrecognized runInMode '{declared}' in {self.flow_path}, treating it as DefaultMode") + return RunMode.DefaultMode def get_api_version(self) -> str: """Get the API version of the flow. @@ -1085,6 +1092,10 @@ def build_vartype_from_elem(elem: El) -> VariableType | None: elif tag == 'decisions': return VariableType(tag=tag, reference=ReferenceType.NodeReference) + elif tag == 'ends': + # API v68+ top-level End element. Terminal node, holds no value. + return VariableType(tag=tag, reference=ReferenceType.NodeReference) + elif tag == 'dynamicChoiceSets': # These are effectively record lookups # TODO: handle this better, right now we just have a stub diff --git a/packages/code-analyzer-flow-engine/FlowScanner/public/enums.py b/packages/code-analyzer-flow-engine/FlowScanner/public/enums.py index 1e0505f9..3e3812f2 100644 --- a/packages/code-analyzer-flow-engine/FlowScanner/public/enums.py +++ b/packages/code-analyzer-flow-engine/FlowScanner/public/enums.py @@ -89,6 +89,7 @@ class RunMode(Enum): SystemModeWithoutSharing = 0 SystemModeWithSharing = 1 DefaultMode = 2 + UserMode = 3 # API v68+: runInMode=UserMode. Runs in user context, so it behaves like DefaultMode. class DataType(Enum): diff --git a/packages/code-analyzer-flow-engine/FlowScanner/public/parse_utils.py b/packages/code-analyzer-flow-engine/FlowScanner/public/parse_utils.py index 7c06d28c..ea9b8cab 100644 --- a/packages/code-analyzer-flow-engine/FlowScanner/public/parse_utils.py +++ b/packages/code-analyzer-flow-engine/FlowScanner/public/parse_utils.py @@ -80,6 +80,7 @@ "collectionProcessors", "customErrors", "decisions", + "ends", "loops", "orchestratedStages", "recordLookups", diff --git a/packages/code-analyzer-flow-engine/FlowScanner/queries/optional_query.py b/packages/code-analyzer-flow-engine/FlowScanner/queries/optional_query.py index 61b49cc1..abb91562 100644 --- a/packages/code-analyzer-flow-engine/FlowScanner/queries/optional_query.py +++ b/packages/code-analyzer-flow-engine/FlowScanner/queries/optional_query.py @@ -24,7 +24,7 @@ DEFAULT_HELP_URL = ("https://developer.salesforce.com/docs/atlas.en-us.secure_coding_guide.meta" "/secure_coding_guide/secure_coding_considerations_flow_design.htm") -id_pattern = re.compile(r"^(?=.*[0-9][0-9])[a-zA-Z0-9]+[a-zA-Z0-9]{15}(?:[0-5][A-Z0-5]{3})?$") +id_pattern = re.compile(r"^[a-zA-Z0-9]{5}0[a-zA-Z0-9]{9}(?:[A-Z0-5]{3})?$") copy_name_pattern = re.compile(r"^Copy_\d+_of_[a-zA-Z]+") copy_label_pattern = re.compile(r"^Copy \d+ of [a-zA-Z]+") diff --git a/packages/code-analyzer-flow-engine/FlowScanner/tests/test_parse_utils.py b/packages/code-analyzer-flow-engine/FlowScanner/tests/test_parse_utils.py index eb7fc58e..3fea3341 100644 --- a/packages/code-analyzer-flow-engine/FlowScanner/tests/test_parse_utils.py +++ b/packages/code-analyzer-flow-engine/FlowScanner/tests/test_parse_utils.py @@ -71,5 +71,18 @@ def test_missing_file_returns_none(self): self.assertIsNone(pu.quick_validate(self.flow_path + '.does-not-exist')) +class EndsElementTest(unittest.TestCase): + """Regression tests for issue #2098: API v68 top-level elements.""" + + def test_ends_is_control_flow_element(self): + self.assertIn('ends', pu.CTRL_FLOW_ELEM) + + def test_ends_has_variable_type(self): + import xml.etree.ElementTree as ET + from flow_parser import parse + elem = ET.fromstring('End1') + self.assertIsNotNone(parse.build_vartype_from_elem(elem)) + + if __name__ == '__main__': unittest.main() diff --git a/packages/code-analyzer-flow-engine/FlowScanner/tests/test_run_mode.py b/packages/code-analyzer-flow-engine/FlowScanner/tests/test_run_mode.py new file mode 100644 index 00000000..af3109ec --- /dev/null +++ b/packages/code-analyzer-flow-engine/FlowScanner/tests/test_run_mode.py @@ -0,0 +1,64 @@ +"""Tests for how the parser handles the flow's runInMode element. + +Run from the FlowScanner directory with:: + + python3 -m unittest tests.test_run_mode +""" +import os +import tempfile +import unittest + +from flow_parser.parse import Parser +from public.enums import RunMode +from flow_scanner import util + +FLOW_TEMPLATE = ( + '\n' + '' + '68.0' + '' + 'AutoLaunchedFlow' + '{run_in_mode}' + 'Active' + '00' + '' +) + + +def parse_flow_with_run_in_mode(run_in_mode_value): + """Writes a minimal flow to disk and returns its parser's declared run mode.""" + element = '' if run_in_mode_value is None else f'{run_in_mode_value}' + with tempfile.TemporaryDirectory() as tmp: + path = os.path.join(tmp, 'RunModeTest.flow-meta.xml') + with open(path, 'w', encoding='utf-8') as f: + f.write(FLOW_TEMPLATE.format(run_in_mode=element)) + return Parser.from_file(path).get_declared_run_mode() + + +class RunInModeTest(unittest.TestCase): + """Regression tests for API v68's runInMode=UserMode, which used to raise KeyError and crash the scan.""" + + def test_user_mode_is_recognized(self): + self.assertIs(parse_flow_with_run_in_mode('UserMode'), RunMode.UserMode) + + def test_existing_modes_are_unchanged(self): + self.assertIs(parse_flow_with_run_in_mode('SystemModeWithoutSharing'), RunMode.SystemModeWithoutSharing) + self.assertIs(parse_flow_with_run_in_mode('SystemModeWithSharing'), RunMode.SystemModeWithSharing) + self.assertIs(parse_flow_with_run_in_mode('DefaultMode'), RunMode.DefaultMode) + + def test_missing_run_in_mode_is_default_mode(self): + self.assertIs(parse_flow_with_run_in_mode(None), RunMode.DefaultMode) + + def test_unknown_run_in_mode_falls_back_to_default_mode_instead_of_crashing(self): + with self.assertLogs('flow_parser.parse', level='WARNING'): + self.assertIs(parse_flow_with_run_in_mode('SomeFutureMode'), RunMode.DefaultMode) + + def test_user_mode_subflow_inherits_the_parent_mode(self): + # Like DefaultMode, UserMode must not override a system mode that a parent flow is already running in + self.assertIs(util.get_effective_run_mode(RunMode.SystemModeWithoutSharing, RunMode.UserMode), + RunMode.SystemModeWithoutSharing) + self.assertIs(util.get_effective_run_mode(None, RunMode.UserMode), RunMode.UserMode) + + +if __name__ == '__main__': + unittest.main() diff --git a/packages/code-analyzer-flow-engine/FlowScanner/tests/test_salesforce_id.py b/packages/code-analyzer-flow-engine/FlowScanner/tests/test_salesforce_id.py new file mode 100644 index 00000000..97923681 --- /dev/null +++ b/packages/code-analyzer-flow-engine/FlowScanner/tests/test_salesforce_id.py @@ -0,0 +1,32 @@ +"""Tests for queries.optional_query.is_valid_salesforce_id. + +Run from the FlowScanner directory with:: + + python3 -m unittest tests.test_salesforce_id +""" +import unittest + +from queries.optional_query import is_valid_salesforce_id, to18 + + +class IsValidSalesforceIdTest(unittest.TestCase): + def test_valid_18_char_id(self): + self.assertTrue(is_valid_salesforce_id("001cU000009Tt2MQAS")) + + def test_valid_15_char_id(self): + self.assertTrue(is_valid_salesforce_id("001cU000009Tt2M")) + + def test_valid_18_char_id_without_adjacent_digits(self): + base = "aBcDe0FgHiJkLmN" + self.assertTrue(is_valid_salesforce_id(to18(base))) + + def test_invalid_18_char_checksum(self): + self.assertFalse(is_valid_salesforce_id("001cU000009Tt2MAAA")) + + def test_rejects_other_lengths(self): + for word in ["abcdefghij11klmn", "abcdefghij11klmno", "abcdefghijk11lmnopq", "short11"]: + self.assertFalse(is_valid_salesforce_id(word), word) + + +if __name__ == "__main__": + unittest.main() diff --git a/packages/code-analyzer-flow-engine/src/messages.ts b/packages/code-analyzer-flow-engine/src/messages.ts index 0dba4231..549c5b8f 100644 --- a/packages/code-analyzer-flow-engine/src/messages.ts +++ b/packages/code-analyzer-flow-engine/src/messages.ts @@ -20,6 +20,12 @@ const MESSAGE_CATALOG: {[key: string]: string} = { ResultsFileNotValidJson: `Unexpected error: Results file contents are not a valid JSON: %s`, + ResultsFileNotProduced: + `Unexpected error: The Flow Scanner did not produce a results file, which usually means it failed to parse one or more flows. See the log file for details: %s`, + + ResultsFileNotReadable: + `Unexpected error: The Flow Scanner results file '%s' could not be read: %s`, + CouldNotParseExecutionResults: `Unexpected error: Could not parse results from %s`, diff --git a/packages/code-analyzer-flow-engine/src/python/FlowScannerCommandWrapper.ts b/packages/code-analyzer-flow-engine/src/python/FlowScannerCommandWrapper.ts index 257de506..548501a4 100644 --- a/packages/code-analyzer-flow-engine/src/python/FlowScannerCommandWrapper.ts +++ b/packages/code-analyzer-flow-engine/src/python/FlowScannerCommandWrapper.ts @@ -101,7 +101,18 @@ export class RunTimeFlowScannerCommandWrapper implements FlowScannerCommandWrapp await this.pythonCommandExecutor.exec(pythonArgs, processStdout); - const outputFileContents: string = await fs.promises.readFile(flowScannerResultsFile, 'utf-8'); + let outputFileContents: string; + try { + outputFileContents = await fs.promises.readFile(flowScannerResultsFile, 'utf-8'); + } catch (err) { + // Only a missing file means the scanner failed to produce results. Any other failure (permissions, the + // path being a directory, etc.) is a different problem, so we keep the original error as the cause. + if ((err as NodeJS.ErrnoException)?.code === 'ENOENT') { + throw new Error(getMessage('ResultsFileNotProduced', absLogFilePath)); + } + throw new Error(getMessage('ResultsFileNotReadable', flowScannerResultsFile, (err as Error)?.message ?? String(err)), + {cause: err}); + } let parsedResults: object; try { diff --git a/packages/code-analyzer-flow-engine/test/python/FlowScannerCommandWrapper.test.ts b/packages/code-analyzer-flow-engine/test/python/FlowScannerCommandWrapper.test.ts index 48bda38f..e8c24712 100644 --- a/packages/code-analyzer-flow-engine/test/python/FlowScannerCommandWrapper.test.ts +++ b/packages/code-analyzer-flow-engine/test/python/FlowScannerCommandWrapper.test.ts @@ -184,6 +184,25 @@ describe('FlowScannerCommandWrapper implementations', () => { .rejects .toThrow(expectedMessage); }); + + it.each([ + {code: 'ENOENT', expectedMessage: 'did not produce a results file'}, + {code: 'EACCES', expectedMessage: 'could not be read'}, + {code: 'EISDIR', expectedMessage: 'could not be read'} + ])('When reading the results file fails with $code, the error message reflects the real problem', async ({code, expectedMessage}) => { + jest.spyOn(PythonCommandExecutor.prototype, 'exec').mockImplementation(async (_args, _processStdout) => { + return Promise.resolve(); + }); + const fsError: NodeJS.ErrnoException = Object.assign(new Error(`${code}: simulated failure`), {code}); + jest.spyOn(fs.promises, 'readFile').mockRejectedValue(fsError); + + const wrapper: RunTimeFlowScannerCommandWrapper = new RunTimeFlowScannerCommandWrapper(PYTHON_COMMAND); + const promise = wrapper.runFlowScannerRules(workingFolder, [PATH_TO_EXAMPLE1, PATH_TO_EXAMPLE2], [PATH_TO_EXAMPLE1, PATH_TO_EXAMPLE2], tempLogFile, [], (_num: number) => {}); + await expect(promise).rejects.toThrow(expectedMessage); + if (code !== 'ENOENT') { + await expect(promise).rejects.toMatchObject({cause: fsError}); + } + }); }); }); }); diff --git a/packages/code-analyzer-pmd-engine/src/config.ts b/packages/code-analyzer-pmd-engine/src/config.ts index d7a79210..49955a4e 100644 --- a/packages/code-analyzer-pmd-engine/src/config.ts +++ b/packages/code-analyzer-pmd-engine/src/config.ts @@ -42,7 +42,7 @@ export type PmdEngineConfig = { // List of xml ruleset files containing custom PMD rules to be made available for rule selection. // Each ruleset must be an xml file that is either:: // - on disk (provided as an absolute path or a relative path to 'config_root') - // - or a relative resource found on the Java classpath. + // - or a relative resource found on the Java classpath (or, for a ruleset that references another ruleset file by a path relative to the folder you run Code Analyzer from, that referenced file). // Not all custom rules can be fully defined within an xml ruleset file. For example, Java based rules may be defined in jar files. // In these cases, you will need to also add your additional files to the Java classpath using the 'java_classpath_entries' field. // See https://pmd.github.io/pmd/pmd_userdocs_making_rulesets.html diff --git a/packages/code-analyzer-pmd-engine/src/messages.ts b/packages/code-analyzer-pmd-engine/src/messages.ts index 6884c9dd..9339e3bb 100644 --- a/packages/code-analyzer-pmd-engine/src/messages.ts +++ b/packages/code-analyzer-pmd-engine/src/messages.ts @@ -27,7 +27,7 @@ const MESSAGE_CATALOG : { [key: string]: string } = { `List of xml ruleset files containing custom PMD rules to be made available for rule selection.\n` + `Each ruleset must be an xml file that is either:\n` + ` - on disk (provided as an absolute path or a relative path to 'config_root')\n` + - ` - or a relative resource found on the Java classpath.\n` + + ` - or a relative resource found on the Java classpath (or, for a ruleset that references another ruleset file by a path relative to the folder you run Code Analyzer from, that referenced file).\n` + `Not all custom rules can be fully defined within an xml ruleset file. For example, Java based rules may be defined in jar files.\n` + `In these cases, you will need to also add your additional files to the Java classpath using the 'java_classpath_entries' field.\n` + `See https://pmd.github.io/pmd/pmd_userdocs_making_rulesets.html to learn more about PMD rulesets.`, diff --git a/packages/code-analyzer-pmd-engine/src/pmd-wrapper.ts b/packages/code-analyzer-pmd-engine/src/pmd-wrapper.ts index ed26a98f..8c4efae3 100644 --- a/packages/code-analyzer-pmd-engine/src/pmd-wrapper.ts +++ b/packages/code-analyzer-pmd-engine/src/pmd-wrapper.ts @@ -1,6 +1,7 @@ import {JavaCommandExecutor} from "@salesforce/code-analyzer-engine-api/utils"; import path from "node:path"; import fs from "node:fs"; +import {stageReferencedRulesets} from "./ruleset-staging"; import {getMessageFromCatalog, LogLevel, SHARED_MESSAGE_CATALOG} from "@salesforce/code-analyzer-engine-api"; const PMD_WRAPPER_JAVA_CLASS: string = "com.salesforce.sfca.pmdwrapper.PmdWrapper"; @@ -78,6 +79,26 @@ export class PmdWrapperInvoker { this.emitLogEvent = emitLogEvent; } + /** + * Creates the Java classpath. Note that the Java process is pinned to the engine's install folder (instead of the + * inherited, possibly untrusted, folder being scanned) to avoid CWE-427. But PMD resolves refs inside of custom + * rulesets (like ) relative to the cwd or the classpath. To keep those refs + * resolving without exposing the entire repository to the JVM as classes/resources, we stage copies of only the + * referenced xml ruleset files into a dedicated folder that we place last on the classpath. + */ + private async createJavaClassPaths(rulesets: string[], workingFolder: string): Promise { + const classPaths: string[] = [ + path.join(PMD_WRAPPER_LIB_FOLDER, '*'), + ...this.userProvidedJavaClasspathEntries.map(toJavaClasspathEntry) + ]; + const stagingFolder: string | undefined = await stageReferencedRulesets( + rulesets, process.cwd(), path.join(workingFolder, 'stagedRulesets')); + if (stagingFolder) { + classPaths.push(stagingFolder + path.sep); // Last so that it has the lowest precedence + } + return classPaths; + } + async invokeDescribeCommand(customRulesets: string[], pmdRuleLanguageIds: string[], workingFolder: string, emitProgress: (percComplete: number) => void): Promise { @@ -87,10 +108,7 @@ export class PmdWrapperInvoker { emitProgress(10); const javaCmdArgs: string[] = [PMD_WRAPPER_JAVA_CLASS, 'describe', pmdRulesOutputFile, customRulesetsListFile, pmdRuleLanguageIds.join(',')]; - const javaClassPaths: string[] = [ - path.join(PMD_WRAPPER_LIB_FOLDER, '*'), - ... this.userProvidedJavaClasspathEntries.map(toJavaClasspathEntry) - ]; + const javaClassPaths: string[] = await this.createJavaClassPaths(customRulesets, workingFolder); await this.javaCommandExecutor.exec(javaCmdArgs, javaClassPaths, (stdOutMsg) => { if (stdOutMsg.startsWith(STDOUT_ERROR_MARKER)) { const errorMessage: string = stdOutMsg.slice(STDOUT_ERROR_MARKER.length).replaceAll('{NEWLINE}','\n'); @@ -138,10 +156,8 @@ export class PmdWrapperInvoker { const resultsOutputFile: string = path.join(workingFolder, 'resultsFile.json'); const javaCmdArgs: string[] = [PMD_WRAPPER_JAVA_CLASS, 'run', inputFile, resultsOutputFile]; - const javaClassPaths: string[] = [ - path.join(PMD_WRAPPER_LIB_FOLDER, '*'), - ... this.userProvidedJavaClasspathEntries.map(toJavaClasspathEntry) - ]; + const javaClassPaths: string[] = await this.createJavaClassPaths( + [...new Set(pmdRuleInfoList.map(ruleInfo => ruleInfo.ruleSetFile))], workingFolder); this.emitLogEvent(LogLevel.Fine, `Calling JAVA command with class path containing ${JSON.stringify(javaClassPaths)} and arguments: ${JSON.stringify(javaCmdArgs)}`); await this.javaCommandExecutor.exec(javaCmdArgs, javaClassPaths, (stdOutMsg: string) => { if (stdOutMsg.startsWith(STDOUT_PROGRESS_MARKER)) { @@ -189,10 +205,7 @@ export class PmdWrapperInvoker { const resultsOutputFile: string = path.join(workingFolder, 'astDumpResults.json'); const javaCmdArgs: string[] = [PMD_WRAPPER_JAVA_CLASS, 'ast-dump', inputFile, resultsOutputFile]; - const javaClassPaths: string[] = [ - path.join(PMD_WRAPPER_LIB_FOLDER, '*'), - ...this.userProvidedJavaClasspathEntries.map(toJavaClasspathEntry) - ]; + const javaClassPaths: string[] = await this.createJavaClassPaths([], workingFolder); this.emitLogEvent(LogLevel.Fine, `Calling AST dump for file: ${fileToDump}`); diff --git a/packages/code-analyzer-pmd-engine/src/ruleset-staging.ts b/packages/code-analyzer-pmd-engine/src/ruleset-staging.ts new file mode 100644 index 00000000..14572b00 --- /dev/null +++ b/packages/code-analyzer-pmd-engine/src/ruleset-staging.ts @@ -0,0 +1,112 @@ +import fs from "node:fs"; +import path from "node:path"; + +// Matches the "ref" attribute of and style elements. +const RULE_REF_REGEX: RegExp = /]*?\bref\s*=\s*["']([^"']+)["']/gi; + +/** + * PMD resolves refs inside of a ruleset (like ) relative to the folder it was run + * from or from the Java classpath. Since we pin the Java process away from the (untrusted) folder being scanned, those + * refs would no longer resolve. Instead of putting the whole folder on the Java classpath (which would let a scanned + * repository supply arbitrary classes and resources to the JVM), we copy only the xml ruleset files that the custom + * rulesets actually reference (transitively) into a dedicated staging folder, and put just that folder on the classpath. + * + * Only regular ".xml" files that live underneath the base folder are ever staged. + * + * @param rulesetFiles the rulesets to inspect (absolute paths on disk, or paths relative to the base folder). Entries that are not xml files on disk are ignored. + * @param baseFolder the folder that relative refs are resolved against + * @param stagingFolder the folder to copy the referenced rulesets into + * @returns the staging folder if at least one file was staged, otherwise undefined + */ +export async function stageReferencedRulesets(rulesetFiles: string[], baseFolder: string, stagingFolder: string): Promise { + const realBase: string = await realpathOrUndefined(baseFolder) ?? baseFolder; + const visited: Set = new Set(); + const queue: string[] = []; + let stagedSomething: boolean = false; + + // Stages the file given by a (relative) ref and queues it up so that its own refs get processed too + const stageRef = async (ref: string): Promise => { + const relativeRulesetPath: string | undefined = extractRelativeXmlPath(ref); + if (relativeRulesetPath === undefined) { + return; + } + const source: string | undefined = await resolveStageableFile(relativeRulesetPath, realBase); + if (source === undefined) { + return; + } + const destination: string = path.join(stagingFolder, relativeRulesetPath); + await fs.promises.mkdir(path.dirname(destination), {recursive: true}); + await fs.promises.copyFile(source, destination); + stagedSomething = true; + queue.push(source); + }; + + for (const rulesetFile of rulesetFiles) { + if (path.isAbsolute(rulesetFile)) { + queue.push(rulesetFile); + } else { + await stageRef(rulesetFile); + } + } + + while (queue.length > 0) { + const currentFile: string = queue.shift()!; + if (visited.has(currentFile)) { + continue; + } + visited.add(currentFile); + + let contents: string; + try { + contents = await fs.promises.readFile(currentFile, 'utf-8'); + } catch (_err) { + continue; // Not a readable file on disk (for example, a ruleset inside of a jar file) + } + + for (const match of contents.matchAll(RULE_REF_REGEX)) { + await stageRef(match[1]); + } + } + return stagedSomething ? stagingFolder : undefined; +} + +/** + * Returns the relative path to the xml file portion of a rule ref (stripping off any trailing "/RuleName"), or + * undefined if the ref is not a relative path to an xml file (for example, "category/apex/design.xml" is fine to stage + * if it exists in the base folder, but absolute paths and non-xml refs are ignored). + */ +function extractRelativeXmlPath(ref: string): string | undefined { + const xmlMatch: RegExpMatchArray | null = ref.match(/^(.*?\.xml)(\/.*)?$/i); + if (!xmlMatch) { + return undefined; + } + const candidate: string = xmlMatch[1]; + if (path.isAbsolute(candidate) || path.win32.isAbsolute(candidate)) { + return undefined; + } + return path.normalize(candidate); +} + +async function resolveStageableFile(relativePath: string, realBase: string): Promise { + const real: string | undefined = await realpathOrUndefined(path.join(realBase, relativePath)); + if (real === undefined) { + return undefined; + } + const relativeToBase: string = path.relative(realBase, real); + if (relativeToBase === '' || relativeToBase.startsWith('..') || path.isAbsolute(relativeToBase)) { + return undefined; // Never stage anything that resolves (even through symlinks) outside of the base folder + } + try { + return (await fs.promises.stat(real)).isFile() ? real : undefined; + } catch (_err) /* istanbul ignore next */ { + return undefined; + } +} + +async function realpathOrUndefined(p: string): Promise { + try { + return await fs.promises.realpath(p); + } catch (_err) { + return undefined; + } +} diff --git a/packages/code-analyzer-pmd-engine/test/pmd-engine.test.ts b/packages/code-analyzer-pmd-engine/test/pmd-engine.test.ts index 198de645..bdcf9ed6 100644 --- a/packages/code-analyzer-pmd-engine/test/pmd-engine.test.ts +++ b/packages/code-analyzer-pmd-engine/test/pmd-engine.test.ts @@ -16,6 +16,7 @@ import { import {PmdEngine} from "../src/pmd-engine"; import fs from "node:fs"; import * as fsPromises from "node:fs/promises"; +import os from "node:os"; import path from "node:path"; import {Language, PMD_VERSION} from "../src/constants"; import {DEFAULT_PMD_ENGINE_CONFIG, PMD_AVAILABLE_LANGUAGES, PmdEngineConfig} from "../src/config"; @@ -321,6 +322,31 @@ describe('Tests for the describeRules method of PmdEngine', () => { } }); + it('When a custom ruleset references a rule using a path relative to the folder Code Analyzer is run from, then the rule is resolved', async () => { + const repoFolder: string = fs.mkdtempSync(path.join(os.tmpdir(), 'pmd-cwd-ref-test-')); + const originalCwd: string = process.cwd(); + try { + await fsPromises.mkdir(path.join(repoFolder, 'scanner')); + await fsPromises.copyFile(path.join(TEST_DATA_FOLDER, 'custom rules', 'somecat3.xml'), path.join(repoFolder, 'scanner', 'full.xml')); + const wrapperRuleset: string = path.join(repoFolder, 'wrapper.xml'); + await fsPromises.writeFile(wrapperRuleset, `\n` + + `\n` + + ` Wrapper\n` + + ` \n` + + `\n`, 'utf-8'); + process.chdir(repoFolder); + const engine: PmdEngine = new PmdEngine({ + ...DEFAULT_PMD_ENGINE_CONFIG, + custom_rulesets: [wrapperRuleset] + }); + const ruleDescriptions: RuleDescription[] = await engine.describeRules(createDescribeOptions()); + expectContainsRuleWithName(ruleDescriptions, 'fakerule7'); + } finally { + process.chdir(originalCwd); + fs.rmSync(repoFolder, {recursive: true, force: true}); + } + }); + it('When specifying a custom_ruleset that cannot be found, then error with a good error message', async () => { const engine: PmdEngine = new PmdEngine({ ...DEFAULT_PMD_ENGINE_CONFIG, diff --git a/packages/code-analyzer-pmd-engine/test/ruleset-staging.test.ts b/packages/code-analyzer-pmd-engine/test/ruleset-staging.test.ts new file mode 100644 index 00000000..21a3c54b --- /dev/null +++ b/packages/code-analyzer-pmd-engine/test/ruleset-staging.test.ts @@ -0,0 +1,92 @@ +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import {stageReferencedRulesets} from "../src/ruleset-staging"; + +describe('Tests for stageReferencedRulesets', () => { + let baseFolder: string; + let stagingFolder: string; + + beforeEach(() => { + const tempRoot: string = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), 'staging-test-'))); + baseFolder = path.join(tempRoot, 'repo'); + stagingFolder = path.join(tempRoot, 'staged'); + fs.mkdirSync(path.join(baseFolder, 'scanner'), {recursive: true}); + }); + + afterEach(() => { + fs.rmSync(path.dirname(baseFolder), {recursive: true, force: true}); + }); + + const rulesetWithRefs = (...refs: string[]): string => + `${refs.map(r => ``).join('')}`; + + it('When a ruleset references a sibling xml file, then only that xml file is staged', async () => { + fs.writeFileSync(path.join(baseFolder, 'scanner', 'full.xml'), rulesetWithRefs()); + fs.writeFileSync(path.join(baseFolder, 'scanner', 'unrelated.xml'), rulesetWithRefs()); + fs.writeFileSync(path.join(baseFolder, 'Evil.class'), 'not really a class'); + const wrapper: string = path.join(baseFolder, 'wrapper.xml'); + fs.writeFileSync(wrapper, rulesetWithRefs('scanner/full.xml/MyRule')); + + const result: string | undefined = await stageReferencedRulesets([wrapper], baseFolder, stagingFolder); + + expect(result).toEqual(stagingFolder); + expect(fs.readdirSync(stagingFolder, {recursive: true}).sort()).toEqual(['scanner', path.join('scanner', 'full.xml')]); + }); + + it('When referenced rulesets reference other rulesets, then they are staged transitively (and cycles are tolerated)', async () => { + fs.writeFileSync(path.join(baseFolder, 'a.xml'), rulesetWithRefs('b.xml/R')); + fs.writeFileSync(path.join(baseFolder, 'b.xml'), rulesetWithRefs('a.xml/R', 'scanner/c.xml')); + fs.writeFileSync(path.join(baseFolder, 'scanner', 'c.xml'), rulesetWithRefs()); + const wrapper: string = path.join(baseFolder, 'wrapper.xml'); + fs.writeFileSync(wrapper, rulesetWithRefs('a.xml/R')); + + await stageReferencedRulesets([wrapper], baseFolder, stagingFolder); + + expect(fs.existsSync(path.join(stagingFolder, 'a.xml'))).toBe(true); + expect(fs.existsSync(path.join(stagingFolder, 'b.xml'))).toBe(true); + expect(fs.existsSync(path.join(stagingFolder, 'scanner', 'c.xml'))).toBe(true); + }); + + it('When a relative ruleset is given directly, then it is resolved against the base folder and staged', async () => { + fs.writeFileSync(path.join(baseFolder, 'scanner', 'full.xml'), rulesetWithRefs()); + + const result: string | undefined = await stageReferencedRulesets(['scanner/full.xml'], baseFolder, stagingFolder); + + expect(result).toEqual(stagingFolder); + expect(fs.existsSync(path.join(stagingFolder, 'scanner', 'full.xml'))).toBe(true); + }); + + it('When refs try to escape the base folder, are absolute, or are not xml, then nothing is staged', async () => { + const outsideFile: string = path.join(path.dirname(baseFolder), 'outside.xml'); + fs.writeFileSync(outsideFile, rulesetWithRefs()); + fs.writeFileSync(path.join(baseFolder, 'Evil.class'), 'x'); + const wrapper: string = path.join(baseFolder, 'wrapper.xml'); + fs.writeFileSync(wrapper, rulesetWithRefs('../outside.xml/R', outsideFile + '/R', 'Evil.class', 'does/not/exist.xml/R')); + + const result: string | undefined = await stageReferencedRulesets([wrapper], baseFolder, stagingFolder); + + expect(result).toBeUndefined(); + expect(fs.existsSync(stagingFolder)).toBe(false); + }); + + it('When a ref is a symlink pointing outside of the base folder, then it is not staged', async () => { + const outsideFile: string = path.join(path.dirname(baseFolder), 'secret.xml'); + fs.writeFileSync(outsideFile, rulesetWithRefs()); + try { + fs.symlinkSync(outsideFile, path.join(baseFolder, 'link.xml')); + } catch (_err) { + return; // Symlink creation isn't permitted in this environment (e.g. Windows without privileges) + } + const wrapper: string = path.join(baseFolder, 'wrapper.xml'); + fs.writeFileSync(wrapper, rulesetWithRefs('link.xml/R')); + + expect(await stageReferencedRulesets([wrapper], baseFolder, stagingFolder)).toBeUndefined(); + }); + + it('When rulesets are not readable files on disk (like jar based rulesets), then they are ignored', async () => { + const result: string | undefined = await stageReferencedRulesets( + [path.join(baseFolder, 'missing.xml'), 'category/apex/design.xml'], baseFolder, stagingFolder); + expect(result).toBeUndefined(); + }); +}); diff --git a/packages/code-analyzer-sfge-engine/gradle/libs.versions.toml b/packages/code-analyzer-sfge-engine/gradle/libs.versions.toml index ab26b93f..423a8c57 100644 --- a/packages/code-analyzer-sfge-engine/gradle/libs.versions.toml +++ b/packages/code-analyzer-sfge-engine/gradle/libs.versions.toml @@ -10,11 +10,10 @@ hamcrest = "3.0" junit-jupiter = "5.13.4" commons-cli = "1.10.0" commons-collections4 = "4.5.0" -tinkergraph-gremlin = "4.0.0-beta.1" -gremlin-driver = "4.0.0-beta.1" +tinkergraph-gremlin = "3.7.7" antlr-runtime = "3.5.3" -log4j-api = "2.25.1" -log4j-core = "2.25.1" +log4j-api = "2.25.5" +log4j-core = "2.25.5" gson = "2.13.2" guava = "33.4.8-jre" jsr305 = "3.0.2" @@ -23,7 +22,12 @@ reflections = "0.10.2" asm = "9.8" mockito-core = "5.19.0" mockito-junit-jupiter = "5.19.0" -netty-all = "4.1.127.Final" +# Versions below force patched releases of dependencies that arrive transitively (via gremlin-core). See sfge/build.gradle.kts. +commons-configuration2 = "2.15.1" +commons-beanutils = "1.11.0" +commons-lang3 = "3.21.0" +commons-text = "1.15.0" +snakeyaml = "2.7" [libraries] hamcrest = { module = "org.hamcrest:hamcrest", version.ref = "hamcrest" } @@ -31,7 +35,6 @@ junit-jupiter = { module = "org.junit.jupiter:junit-jupiter", version.ref = "jun commons-cli = { module = "commons-cli:commons-cli", version.ref = "commons-cli" } commons-collections4 = { module = "org.apache.commons:commons-collections4", version.ref = "commons-collections4" } tinkergraph-gremlin = { module = "org.apache.tinkerpop:tinkergraph-gremlin", version.ref = "tinkergraph-gremlin" } -gremlin-driver = { module = "org.apache.tinkerpop:gremlin-driver", version.ref = "gremlin-driver" } antlr-runtime = { module = "org.antlr:antlr-runtime", version.ref = "antlr-runtime" } log4j-api = { module = "org.apache.logging.log4j:log4j-api", version.ref = "log4j-api" } log4j-core = { module = "org.apache.logging.log4j:log4j-core", version.ref = "log4j-core" } @@ -44,4 +47,8 @@ asm = { module = "org.ow2.asm:asm", version.ref = "asm" } junit-platform-launcher = { module = "org.junit.platform:junit-platform-launcher" } mockito-core = { module = "org.mockito:mockito-core", version.ref = "mockito-core" } mockito-junit-jupiter = { module = "org.mockito:mockito-junit-jupiter", version.ref = "mockito-junit-jupiter" } -netty-all = { module = "io.netty:netty-all", version.ref = "netty-all" } +commons-configuration2 = { module = "org.apache.commons:commons-configuration2", version.ref = "commons-configuration2" } +commons-beanutils = { module = "commons-beanutils:commons-beanutils", version.ref = "commons-beanutils" } +commons-lang3 = { module = "org.apache.commons:commons-lang3", version.ref = "commons-lang3" } +commons-text = { module = "org.apache.commons:commons-text", version.ref = "commons-text" } +snakeyaml = { module = "org.yaml:snakeyaml", version.ref = "snakeyaml" } diff --git a/packages/code-analyzer-sfge-engine/package.json b/packages/code-analyzer-sfge-engine/package.json index b9583ecc..2826a8d3 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.25.0", + "version": "0.26.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-sfge-engine/sfge/build.gradle.kts b/packages/code-analyzer-sfge-engine/sfge/build.gradle.kts index f5d29853..d4076831 100644 --- a/packages/code-analyzer-sfge-engine/sfge/build.gradle.kts +++ b/packages/code-analyzer-sfge-engine/sfge/build.gradle.kts @@ -22,8 +22,18 @@ repositories { dependencies { implementation(libs.commons.cli) implementation(libs.commons.collections4) - implementation(libs.tinkergraph.gremlin) - implementation(libs.gremlin.driver) + implementation(libs.tinkergraph.gremlin) { + // commons-collections 3.x (pulled in by commons-beanutils) has known vulnerabilities and no patched release. + // SFGE and the Gremlin graph layer do not use it, so it is removed from the classpath entirely. + exclude(group = "commons-collections", module = "commons-collections") + // gremlin-shaded bundles (relocated) copies of Jackson and Kryo that are only used for graph import/export + // (GraphSON/Gryo/GraphML). SFGE only builds and traverses an in-memory graph, and the bundled Jackson version + // contains known vulnerabilities that cannot be patched via dependency constraints, so we do not ship it. + exclude(group = "org.apache.tinkerpop", module = "gremlin-shaded") + } + // These are used directly by SFGE source (they previously arrived only transitively), so declare them explicitly. + implementation(libs.commons.lang3) + implementation(libs.commons.text) implementation(libs.antlr.runtime) implementation(libs.log4j.api) implementation(libs.log4j.core) @@ -35,9 +45,16 @@ dependencies { } implementation(libs.reflections) implementation(libs.asm) + // Force patched versions of transitive dependencies of gremlin-core (embedded in-memory graph). constraints { - implementation(libs.netty.all) { - because("Force specific version of transitive dependency") + implementation(libs.commons.configuration2) { + because("Patched version of a transitive dependency of gremlin-core") + } + implementation(libs.commons.beanutils) { + because("Patched version of a transitive dependency of gremlin-core") + } + implementation(libs.snakeyaml) { + because("Patched version of a transitive dependency of gremlin-core") } } implementation(files("lib/apex-jorje-lsp-sfge.jar")) @@ -71,11 +88,53 @@ tasks.jar { tasks.register("copyDependencies") { from(configurations.runtimeClasspath) // This includes all runtime dependencies into(javaLibDir) // Target directory for dependencies + // The directory is also the output of the jar task (sfge-*.jar), so we can't use Sync. Instead, remove any stale + // dependency jars left over from earlier builds (e.g. ones since removed from the runtime classpath) so that an + // incremental build packs exactly the same dependency set as a clean build. + doFirst { + delete(fileTree(javaLibDir) { exclude("sfge-*.jar") }) + } } tasks.build { dependsOn("copyDependencies") // Ensure dependencies are copied during the build process } +// Fail the build if a library that is known to carry unpatchable vulnerabilities (or that SFGE has no need for) sneaks +// back onto the runtime classpath, e.g., through a Gremlin upgrade. Everything on the runtime classpath gets shipped. +val bannedRuntimeDependencies = mapOf( + "io.netty" to "Only needed by gremlin-driver (remote Gremlin client), which SFGE does not use", + "software.amazon.awssdk" to "Only needed by gremlin-driver (remote Gremlin client), which SFGE does not use", + "org.apache.tinkerpop:gremlin-driver" to "SFGE only uses the embedded in-memory graph", + "org.apache.tinkerpop:gremlin-util" to "SFGE only uses the embedded in-memory graph", + "org.apache.tinkerpop:gremlin-shaded" to "Bundles Jackson/Kryo with known vulnerabilities that cannot be patched via constraints", + "commons-collections:commons-collections" to "3.x has known vulnerabilities and no patched release (SFGE uses commons-collections4)" +) +tasks.register("verifyNoBannedDependencies") { + group = "verification" + description = "Fails if a banned dependency is on the runtime classpath (and therefore would be shipped)." + val resolvedModules = configurations.runtimeClasspath.map { config -> + config.incoming.resolutionResult.allComponents + .mapNotNull { it.moduleVersion } + .map { "${it.group}:${it.name}:${it.version}" } + } + doLast { + val violations = resolvedModules.get().flatMap { coordinates -> + val (group, name) = coordinates.split(":") + bannedRuntimeDependencies.filter { (banned, _) -> banned == group || banned == "$group:$name" } + .map { (_, reason) -> "$coordinates -> $reason" } + } + if (violations.isNotEmpty()) { + throw GradleException("Banned dependencies found on the runtime classpath:\n " + violations.joinToString("\n ")) + } + } +} +tasks.check { + dependsOn("verifyNoBannedDependencies") +} +tasks.build { + dependsOn("verifyNoBannedDependencies") +} + // ======== TEST RELATED TASKS ========================================================================================= jacoco { diff --git a/packages/code-analyzer-sfge-engine/sfge/src/main/java/org/apache/tinkerpop/shaded/jackson/core/JsonProcessingException.java b/packages/code-analyzer-sfge-engine/sfge/src/main/java/org/apache/tinkerpop/shaded/jackson/core/JsonProcessingException.java new file mode 100644 index 00000000..300ba775 --- /dev/null +++ b/packages/code-analyzer-sfge-engine/sfge/src/main/java/org/apache/tinkerpop/shaded/jackson/core/JsonProcessingException.java @@ -0,0 +1,23 @@ +package org.apache.tinkerpop.shaded.jackson.core; + +import java.io.IOException; + +/** + * Minimal stand-in for the relocated Jackson exception type that {@code TinkerGraph} references in a {@code catch} + * clause. + * + *

Why this exists: {@code gremlin-shaded} bundles (relocated) copies of Jackson and Kryo. The Jackson version it + * bundles contains known vulnerabilities that cannot be fixed with dependency constraints, and SFGE does not use it + * since SFGE only builds and traverses an in-memory graph (no GraphSON/Gryo/GraphML import or export). So we exclude + * {@code gremlin-shaded} from the runtime classpath (see {@code build.gradle.kts}). However, the JVM must still be able + * to load this one exception class when it verifies {@code TinkerGraph}, even though no Jackson code ever runs. + * + *

This class intentionally contains no Jackson code. Remove it (and re-evaluate the {@code gremlin-shaded} exclusion) + * when upgrading to a Gremlin release whose bundled Jackson is patched. {@code NoVulnerableBundledLibrariesTest} + * guards this behavior. + */ +public class JsonProcessingException extends IOException { + public JsonProcessingException(String message) { + super(message); + } +} diff --git a/packages/code-analyzer-sfge-engine/sfge/src/test/java/com/salesforce/graph/ops/NoVulnerableBundledLibrariesTest.java b/packages/code-analyzer-sfge-engine/sfge/src/test/java/com/salesforce/graph/ops/NoVulnerableBundledLibrariesTest.java new file mode 100644 index 00000000..679f3532 --- /dev/null +++ b/packages/code-analyzer-sfge-engine/sfge/src/test/java/com/salesforce/graph/ops/NoVulnerableBundledLibrariesTest.java @@ -0,0 +1,43 @@ +package com.salesforce.graph.ops; + +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertThrows; + +import org.junit.jupiter.api.Test; + +/** + * Guards the dependency-hygiene decisions documented in {@code build.gradle.kts}: SFGE only needs an in-memory + * TinkerGraph, and must not ship libraries that carry known vulnerabilities. + */ +public class NoVulnerableBundledLibrariesTest { + @Test + public void inMemoryGraphWorksWithoutGremlinShaded() { + // Fails with NoClassDefFoundError if TinkerGraph needs anything from gremlin-shaded beyond what we provide. + assertNotNull(GraphUtil.getGraph()); + } + + @Test + public void gremlinShadedJacksonImplementationIsNotOnClasspath() { + assertThrows( + ClassNotFoundException.class, + () -> Class.forName("org.apache.tinkerpop.shaded.jackson.databind.ObjectMapper")); + assertThrows( + ClassNotFoundException.class, + () -> Class.forName("org.apache.tinkerpop.shaded.jackson.core.JsonFactory")); + } + + @Test + public void commonsCollections3IsNotOnClasspath() { + assertThrows( + ClassNotFoundException.class, + () -> Class.forName("org.apache.commons.collections.FastHashMap")); + } + + @Test + public void nettyAndGremlinDriverAreNotOnClasspath() { + assertThrows(ClassNotFoundException.class, () -> Class.forName("io.netty.channel.Channel")); + assertThrows( + ClassNotFoundException.class, + () -> Class.forName("org.apache.tinkerpop.gremlin.driver.Cluster")); + } +}