From a82c6d8b1b8d4909d575ae3482b1b858f5874ebc Mon Sep 17 00:00:00 2001 From: rmohan20 <58573547+rmohan20@users.noreply.github.com> Date: Thu, 30 May 2024 13:12:10 -0700 Subject: [PATCH 1/7] NEW (GraphEngine): @W-15639963@: Caches tainted file to entry methods that have paths that taint the file. (#1479) --- .gitignore | 4 + sfge/.gitignore | 1 + sfge/build.gradle.kts | 3 + sfge/src/main/java/com/salesforce/Main.java | 7 +- .../java/com/salesforce/cli/CacheCreator.java | 116 ++++++++++++++ .../com/salesforce/cli/FilesToEntriesMap.java | 120 +++++++++++++++ .../main/java/com/salesforce/cli/Result.java | 28 +++- .../java/com/salesforce/config/EnvUtil.java | 24 ++- .../com/salesforce/config/SfgeConfig.java | 6 + .../com/salesforce/config/SfgeConfigImpl.java | 10 ++ .../java/com/salesforce/graph/ApexPath.java | 1 + .../ops/expander/ops/TaintedFileTracker.java | 34 +++++ .../salesforce/rules/PathBasedRuleRunner.java | 35 ++++- .../rules/ThreadableRuleExecutor.java | 53 ++++--- .../java/com/salesforce/rules/Violation.java | 28 +++- .../test/java/com/salesforce/TestUtil.java | 4 +- .../com/salesforce/cli/CacheCreatorTest.java | 93 ++++++++++++ .../salesforce/cli/FilesToEntriesMapTest.java | 105 +++++++++++++ .../salesforce/cli/PathEntryCaptureTest.java | 141 ++++++++++++++++++ .../config/SfgeConfigProviderTest.java | 16 ++ .../com/salesforce/config/TestSfgeConfig.java | 10 ++ .../rules/PathBasedRuleRunnerTest.java | 7 +- .../com/salesforce/rules/RuleRunnerTest.java | 12 +- .../unusedmethod/BaseUnusedMethodTest.java | 4 +- .../testEntryNotOnKeyFile/EntryClass.cls | 8 + .../testEntryNotOnKeyFile/KeyClass.cls | 5 + .../testEntryOnKeyFile/EntryAndKeyClass.cls | 10 ++ .../EntryClass.cls | 14 ++ .../KeyClass1.cls | 5 + .../KeyClass2.cls | 5 + .../testNoKeyForUnusedFile/EntryClass.cls | 8 + .../testNoKeyForUnusedFile/KeyClass.cls | 5 + .../testNoKeyForUnusedFile/UnusedClass.cls | 5 + 33 files changed, 868 insertions(+), 59 deletions(-) create mode 100644 sfge/src/main/java/com/salesforce/cli/CacheCreator.java create mode 100644 sfge/src/main/java/com/salesforce/cli/FilesToEntriesMap.java create mode 100644 sfge/src/main/java/com/salesforce/graph/ops/expander/ops/TaintedFileTracker.java create mode 100644 sfge/src/test/java/com/salesforce/cli/CacheCreatorTest.java create mode 100644 sfge/src/test/java/com/salesforce/cli/FilesToEntriesMapTest.java create mode 100644 sfge/src/test/java/com/salesforce/cli/PathEntryCaptureTest.java create mode 100644 sfge/src/test/resources/test-files/com/salesforce/cli/PathEntryCaptureTest/testEntryNotOnKeyFile/EntryClass.cls create mode 100644 sfge/src/test/resources/test-files/com/salesforce/cli/PathEntryCaptureTest/testEntryNotOnKeyFile/KeyClass.cls create mode 100644 sfge/src/test/resources/test-files/com/salesforce/cli/PathEntryCaptureTest/testEntryOnKeyFile/EntryAndKeyClass.cls create mode 100644 sfge/src/test/resources/test-files/com/salesforce/cli/PathEntryCaptureTest/testMultiplePathsToDifferentClasses/EntryClass.cls create mode 100644 sfge/src/test/resources/test-files/com/salesforce/cli/PathEntryCaptureTest/testMultiplePathsToDifferentClasses/KeyClass1.cls create mode 100644 sfge/src/test/resources/test-files/com/salesforce/cli/PathEntryCaptureTest/testMultiplePathsToDifferentClasses/KeyClass2.cls create mode 100644 sfge/src/test/resources/test-files/com/salesforce/cli/PathEntryCaptureTest/testNoKeyForUnusedFile/EntryClass.cls create mode 100644 sfge/src/test/resources/test-files/com/salesforce/cli/PathEntryCaptureTest/testNoKeyForUnusedFile/KeyClass.cls create mode 100644 sfge/src/test/resources/test-files/com/salesforce/cli/PathEntryCaptureTest/testNoKeyForUnusedFile/UnusedClass.cls diff --git a/.gitignore b/.gitignore index ef7fe04da..2b561613e 100644 --- a/.gitignore +++ b/.gitignore @@ -159,3 +159,7 @@ pmd-cataloger/bin sfge*.log.gz npm-shrinkwrap.json +/cli-messaging/out/ + +# Cache file(s) +/.sfge-cache/fileToEntryMapData.json diff --git a/sfge/.gitignore b/sfge/.gitignore index f505b0748..1d8277c0e 100644 --- a/sfge/.gitignore +++ b/sfge/.gitignore @@ -44,3 +44,4 @@ sfge*.log.* # Informational file when generating sObject lists /src/main/resources/sObjects/sObjectNotFoundList.txt +/out/ diff --git a/sfge/build.gradle.kts b/sfge/build.gradle.kts index 4ba04bc00..8801a503e 100644 --- a/sfge/build.gradle.kts +++ b/sfge/build.gradle.kts @@ -23,6 +23,9 @@ dependencies { implementation("com.google.code.gson:gson:2.10.1") implementation("com.google.guava:guava:26.0-jre") implementation("com.google.code.findbugs:jsr305:3.0.2") + implementation ("com.googlecode.json-simple:json-simple:1.1.1") { + exclude("junit") + } implementation("org.reflections:reflections:0.9.12") implementation("org.ow2.asm:asm:9.2") implementation(files("lib/apex-jorje-lsp-sfge.jar")) diff --git a/sfge/src/main/java/com/salesforce/Main.java b/sfge/src/main/java/com/salesforce/Main.java index 39c7bf1c5..05a9e2312 100644 --- a/sfge/src/main/java/com/salesforce/Main.java +++ b/sfge/src/main/java/com/salesforce/Main.java @@ -1,6 +1,7 @@ package com.salesforce; import com.google.common.annotations.VisibleForTesting; +import com.salesforce.cli.CacheCreator; import com.salesforce.cli.CliArgParser; import com.salesforce.cli.OutputFormatter; import com.salesforce.cli.Result; @@ -184,7 +185,7 @@ private int execute(String... args) { // No matter the outcome, share the results found so far dependencies.printOutput(CliMessager.getInstance().getAllMessagesWithFormatting()); - final List violations = result.getViolations(); + final List violations = result.getOrderedViolations(); OutputFormatter formatter = new OutputFormatter(); dependencies.printOutput(formatter.formatViolationJsons(violations)); @@ -194,6 +195,10 @@ private int execute(String... args) { dependencies.printError(formatError(throwable)); } + // Create cache data, if any + final CacheCreator cacheCreator = new CacheCreator(); + cacheCreator.create(result); + if (!errorsThrown.isEmpty()) { return violations.isEmpty() ? EXIT_WITH_INTERNAL_ERROR_NO_VIOLATIONS diff --git a/sfge/src/main/java/com/salesforce/cli/CacheCreator.java b/sfge/src/main/java/com/salesforce/cli/CacheCreator.java new file mode 100644 index 000000000..bb66a6d09 --- /dev/null +++ b/sfge/src/main/java/com/salesforce/cli/CacheCreator.java @@ -0,0 +1,116 @@ +package com.salesforce.cli; + +import java.io.File; +import java.io.FileWriter; +import java.io.IOException; +import java.nio.file.Path; + +import com.salesforce.config.SfgeConfig; +import com.salesforce.config.SfgeConfigProvider; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; + +/** Creates cache data that can be used to execute Delta runs. */ +public final class CacheCreator { + private static final Logger LOGGER = LogManager.getLogger(CacheCreator.class); + + private Dependencies dependencies; + + public CacheCreator() { + this.dependencies = new Dependencies(); + } + + CacheCreator(Dependencies dependencies) { + this.dependencies = dependencies; + } + + /** + * Creates cache data that can be used for future delta runs + * + * @param result from the current execution + */ + public void create(Result result) { + if (SfgeConfigProvider.get().isCachingDisabled()) { + if (LOGGER.isInfoEnabled()) { + LOGGER.info( + "Skipping to cache information since it has been disabled."); + } + return; + } + createFilesToEntriesCache(result.getFilesToEntriesMap()); + } + + private void createFilesToEntriesCache(FilesToEntriesMap filesToEntriesMap) { + final String jsonData = filesToEntriesMap.toJsonString(); + final String filesToEntriesDataFilename = getFilesToEntriesDataFilename(); + + if (LOGGER.isInfoEnabled()) { + LOGGER.info( + "About to create cache for filesToEntriesMap at " + filesToEntriesDataFilename); + } + if (LOGGER.isDebugEnabled()) { + LOGGER.debug( + "Content to be written in " + filesToEntriesDataFilename + ": " + jsonData); + } + + try { + dependencies.writeFile(filesToEntriesDataFilename, jsonData); + } catch (IOException e) { + // Putting a warning instead of stopping the process since this is not in the critical + // path + if (LOGGER.isWarnEnabled()) { + LOGGER.warn("Exception occurred while writing filesToEntriesMap cache data.", e); + } + } + } + + private String getFilesToEntriesDataFilename() { + return SfgeConfigProvider.get().getFilesToEntriesCacheLocation(); + } + + static class Dependencies { + + /** + * Write to file. The current assumption is to overwrite the file. TODO: This could be + * problematic when the delta run wants to preserve the original content. The ideal approach + * would be to overwrite entries of only the files that were changed. + * + * @param filename of the file to create/overwrite. + * @param data that should be written to the file. + * @throws IOException when an error is encountered. + */ + void writeFile(String filename, String data) throws IOException { + mkdirsIfNeeded(filename); + + final boolean append = false; + FileWriter fileWriter = new FileWriter(filename, append); + try { + fileWriter.write(data); + } finally { + if (fileWriter != null) { + fileWriter.flush(); + fileWriter.close(); + } + } + } + + /** + * Create new directories in the path, if necessary + * + * @param filename + */ + private void mkdirsIfNeeded(String filename) { + final Path filepath = Path.of(filename); + final Path directory = filepath.getParent(); + + final boolean isNewDirectory = directory.toFile().mkdirs(); + if (LOGGER.isDebugEnabled()) { + if (isNewDirectory) { + LOGGER.debug("Created new cache directory: " + directory.getFileName()); + } else { + LOGGER.debug("Cache directory already exists: " + directory.getFileName()); + } + } + } + } +} diff --git a/sfge/src/main/java/com/salesforce/cli/FilesToEntriesMap.java b/sfge/src/main/java/com/salesforce/cli/FilesToEntriesMap.java new file mode 100644 index 000000000..a23611166 --- /dev/null +++ b/sfge/src/main/java/com/salesforce/cli/FilesToEntriesMap.java @@ -0,0 +1,120 @@ +package com.salesforce.cli; + +import java.util.HashMap; +import java.util.HashSet; +import java.util.Map; +import java.util.Objects; + +import com.google.common.annotations.VisibleForTesting; +import org.json.simple.JSONArray; +import org.json.simple.JSONObject; + +/** + * Mapping of files that occur in an {@link com.salesforce.graph.ApexPath} and the entry method from + * which the path originates. + */ +public class FilesToEntriesMap { + @VisibleForTesting static final String FIELD_DATA = "data"; + @VisibleForTesting static final String FIELD_FILENAME = "filename"; + @VisibleForTesting static final String FIELD_ENTRIES = "entries"; + private final HashMap> map; + + public FilesToEntriesMap() { + this.map = new HashMap<>(); + } + + /** + * Creates a new mapping of the file name and the corresponding entry method it impacts + * + * @param filename that a path touches + * @param entryFile of the entry method + * @param entryMethod of the entry method + */ + public void put(String filename, String entryFile, String entryMethod) { + final Entry entry = new Entry(entryFile, entryMethod); + final HashSet value = map.computeIfAbsent(filename, key -> new HashSet<>()); + value.add(entry); + } + + /** + * Merges entries of another {@link FilesToEntriesMap} instance into the current instance. + * + * @param other {@link FilesToEntriesMap} instance to merge + */ + public void merge(FilesToEntriesMap other) { + for (Map.Entry> otherMapEntry : other.map.entrySet()) { + map.merge( + otherMapEntry.getKey(), + otherMapEntry.getValue(), + (oldValue, newValue) -> { + oldValue.addAll(newValue); + return oldValue; + }); + } + } + + /** + * @return JSON data representing the state of this instance + */ + public String toJsonString() { + final JSONArray data = new JSONArray(); + for (String filename : map.keySet()) { + data.add(getFileToEntriesJson(filename)); + } + final JSONObject mapping = new JSONObject(); + mapping.put(FIELD_DATA, data); + + return mapping.toJSONString(); + } + + private JSONObject getFileToEntriesJson(String filename) { + final HashSet entries = map.get(filename); + final JSONArray entriesJson = new JSONArray(); + for (Entry entry : entries) { + entriesJson.add(entry.toString()); + } + final JSONObject fileToEntriesJson = new JSONObject(); + fileToEntriesJson.put(FIELD_FILENAME, filename); + fileToEntriesJson.put(FIELD_ENTRIES, entriesJson); + + return fileToEntriesJson; + } + + @VisibleForTesting + HashMap> getMap() { + return map; + } + + @VisibleForTesting + static class Entry { + private final String filename; + private final String methodName; + + // Graph Engine CLI understands this format (filename#method) + // and handles the target method as an entry point + private static final String FORMAT = "%s#%s"; + + Entry(String filename, String methodName) { + this.filename = filename; + this.methodName = methodName; + } + + @Override + public String toString() { + return String.format(FORMAT, this.filename, this.methodName); + } + + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (o == null || getClass() != o.getClass()) return false; + Entry entry = (Entry) o; + return Objects.equals(filename, entry.filename) && Objects.equals(methodName, entry.methodName); + } + + @Override + public int hashCode() { + return Objects.hash(filename, methodName); + } + } +} diff --git a/sfge/src/main/java/com/salesforce/cli/Result.java b/sfge/src/main/java/com/salesforce/cli/Result.java index 9b115f535..b88c5b53c 100644 --- a/sfge/src/main/java/com/salesforce/cli/Result.java +++ b/sfge/src/main/java/com/salesforce/cli/Result.java @@ -1,9 +1,8 @@ package com.salesforce.cli; import com.salesforce.rules.Violation; -import java.util.ArrayList; -import java.util.Collection; -import java.util.List; +import java.util.*; +import java.util.stream.Collectors; /** Represents the result of an SFGE execution. */ public class Result { @@ -17,15 +16,20 @@ public class Result { /** Indicates if the execution completed successfully * */ private boolean completedSuccessfully; + /** Mapping of file names to entry points that lead to paths that traverse these paths * */ + private final FilesToEntriesMap filesToEntriesMap; + public Result() { this.violations = new ArrayList<>(); this.errorsThrown = new ArrayList<>(); this.completedSuccessfully = false; + this.filesToEntriesMap = new FilesToEntriesMap(); } public void merge(Result result) { - this.addViolations(result.getViolations()); - this.addThrowable(result.getErrorsThrown()); + this.addViolations(result.violations); + this.addThrowable(result.errorsThrown); + this.filesToEntriesMap.merge(result.filesToEntriesMap); } public void addViolations(Collection violations) { @@ -48,8 +52,14 @@ public void addThrowable(Throwable ex) { this.errorsThrown.add(ex); } - public List getViolations() { - return violations; + public void addFileToEntryPoint(String filename, String entryFile, String entryMethod) { + this.filesToEntriesMap.put(filename, entryFile, entryMethod); + } + + public List getOrderedViolations() { + final TreeSet orderedViolations = new TreeSet<>(); + orderedViolations.addAll(violations); + return orderedViolations.stream().collect(Collectors.toList()); } public boolean isCompletedSuccessfully() { @@ -59,4 +69,8 @@ public boolean isCompletedSuccessfully() { public List getErrorsThrown() { return errorsThrown; } + + public FilesToEntriesMap getFilesToEntriesMap() { + return this.filesToEntriesMap; + } } diff --git a/sfge/src/main/java/com/salesforce/config/EnvUtil.java b/sfge/src/main/java/com/salesforce/config/EnvUtil.java index 27e36f8fc..26539b48e 100644 --- a/sfge/src/main/java/com/salesforce/config/EnvUtil.java +++ b/sfge/src/main/java/com/salesforce/config/EnvUtil.java @@ -2,6 +2,9 @@ import com.google.common.annotations.VisibleForTesting; import com.salesforce.graph.ops.registry.RegistryDataLimitCalculator; + +import java.io.File; +import java.nio.file.Files; import java.util.concurrent.TimeUnit; public final class EnvUtil { @@ -9,11 +12,12 @@ public final class EnvUtil { private static final String ENV_RULE_THREAD_TIMEOUT = "SFGE_RULE_THREAD_TIMEOUT"; private static final String ENV_RULE_DISABLE_WARNING_VIOLATION = "SFGE_RULE_DISABLE_WARNING_VIOLATION"; - private static final String ENV_IGNORE_PARSE_ERRORS = "SFGE_IGNORE_PARSE_ERRORS"; private static final String ENV_LOG_WARNINGS_ON_VERBOSE = "SFGE_LOG_WARNINGS_ON_VERBOSE"; private static final String ENV_PROGRESS_INCREMENTS = "SFGE_PROGRESS_INCREMENTS"; private static final String ENV_STACK_DEPTH_LIMIT = "SFGE_STACK_DEPTH_LIMIT"; private static final String ENV_PATH_EXPANSION_LIMIT = "SFGE_PATH_EXPANSION_LIMIT"; + private static final String ENV_FILES_TO_ENTRIES_CACHE_LOCATION = "SFGE_FILES_TO_ENTRIES_CACHE_LOCATION"; + private static final String ENV_DISABLE_CACHING = "SFGE_DISABLE_CACHING"; // TODO: These should move to SfgeConfigImpl and this class should return Optionals @VisibleForTesting @@ -35,6 +39,11 @@ public final class EnvUtil { static final int DEFAULT_PATH_EXPANSION_LIMIT = RegistryDataLimitCalculator.getApexPathExpanderRegistryLimit(); + @VisibleForTesting static final String DEFAULT_FILES_TO_ENTRIES_CACHE_LOCATION = ".sfge-cache" + File.separator + "fileToEntryMapData.json"; + + @VisibleForTesting static final boolean DEFAULT_DISABLE_CACHING = false; + + /** * Returns the value of the {@link #ENV_RULE_THREAD_COUNT} environment variable if set, else * {@link #DEFAULT_RULE_THREAD_COUNT}. Should be used to set the number of threads that can be @@ -101,6 +110,14 @@ static int getPathExpansionLimit() { return getIntOrDefault(ENV_PATH_EXPANSION_LIMIT, DEFAULT_PATH_EXPANSION_LIMIT); } + static String getFilesToEntriesCacheLocation() { + return getStringOrDefault(ENV_FILES_TO_ENTRIES_CACHE_LOCATION, DEFAULT_FILES_TO_ENTRIES_CACHE_LOCATION); + } + + static boolean isCachingDisabled() { + return getBoolOrDefault(ENV_DISABLE_CACHING, DEFAULT_DISABLE_CACHING); + } + private static int getIntOrDefault(String name, int defaultValue) { String strVal = System.getProperty(name); return strVal == null ? defaultValue : Integer.parseInt(strVal); @@ -116,5 +133,10 @@ private static boolean getBoolOrDefault(String name, boolean defaultValue) { return strVal == null ? defaultValue : Boolean.parseBoolean(strVal); } + private static String getStringOrDefault(String name, String defaultValue) { + String strVal = System.getProperty(name); + return strVal == null ? defaultValue : strVal; + } + private EnvUtil() {} } diff --git a/sfge/src/main/java/com/salesforce/config/SfgeConfig.java b/sfge/src/main/java/com/salesforce/config/SfgeConfig.java index b9bdf6ca5..37b3b7831 100644 --- a/sfge/src/main/java/com/salesforce/config/SfgeConfig.java +++ b/sfge/src/main/java/com/salesforce/config/SfgeConfig.java @@ -31,4 +31,10 @@ public interface SfgeConfig { /** Limit to control the growth of path expansion to help alleviate OutOfMemoryError. */ int getPathExpansionLimit(); + + /** Filename of data that stores Files to Entries **/ + String getFilesToEntriesCacheLocation(); + + /** Indicates if caching should be disabled in the current run **/ + boolean isCachingDisabled(); } diff --git a/sfge/src/main/java/com/salesforce/config/SfgeConfigImpl.java b/sfge/src/main/java/com/salesforce/config/SfgeConfigImpl.java index 8411f002f..3fbbb287d 100644 --- a/sfge/src/main/java/com/salesforce/config/SfgeConfigImpl.java +++ b/sfge/src/main/java/com/salesforce/config/SfgeConfigImpl.java @@ -40,6 +40,16 @@ public int getPathExpansionLimit() { return EnvUtil.getPathExpansionLimit(); } + @Override + public String getFilesToEntriesCacheLocation() { + return EnvUtil.getFilesToEntriesCacheLocation(); + } + + @Override + public boolean isCachingDisabled() { + return EnvUtil.isCachingDisabled(); + } + static SfgeConfigImpl getInstance() { return SfgeConfigImpl.LazyHolder.INSTANCE; } diff --git a/sfge/src/main/java/com/salesforce/graph/ApexPath.java b/sfge/src/main/java/com/salesforce/graph/ApexPath.java index aeea3af8b..6d132cc65 100644 --- a/sfge/src/main/java/com/salesforce/graph/ApexPath.java +++ b/sfge/src/main/java/com/salesforce/graph/ApexPath.java @@ -1,5 +1,6 @@ package com.salesforce.graph; +import com.google.common.collect.ImmutableList; import com.salesforce.Collectible; import com.salesforce.NullCollectible; import com.salesforce.collections.CollectionUtil; diff --git a/sfge/src/main/java/com/salesforce/graph/ops/expander/ops/TaintedFileTracker.java b/sfge/src/main/java/com/salesforce/graph/ops/expander/ops/TaintedFileTracker.java new file mode 100644 index 000000000..424638201 --- /dev/null +++ b/sfge/src/main/java/com/salesforce/graph/ops/expander/ops/TaintedFileTracker.java @@ -0,0 +1,34 @@ +package com.salesforce.graph.ops.expander.ops; + +import com.salesforce.graph.ApexPath; +import com.salesforce.graph.ops.expander.PathExpansionObserver; +import com.salesforce.graph.vertex.MethodVertex; + +import java.util.Collections; +import java.util.HashSet; +import java.util.Optional; +import java.util.Set; + +/** + * Tracks apex class files that were encountered while expanding paths. + */ +public class TaintedFileTracker implements PathExpansionObserver { + final HashSet taintedFiles; + + public TaintedFileTracker() { + taintedFiles = new HashSet<>(); + } + + @Override + public void onPathVisit(ApexPath path) { + Optional methodOpt = path.getMethodVertex(); + if (methodOpt.isPresent()) { + taintedFiles.add(methodOpt.get().getFileName()); + } + } + + public Set getTaintedFiles() { + return Collections.unmodifiableSet(taintedFiles); + } + +} diff --git a/sfge/src/main/java/com/salesforce/rules/PathBasedRuleRunner.java b/sfge/src/main/java/com/salesforce/rules/PathBasedRuleRunner.java index 397bf6b6f..781899959 100644 --- a/sfge/src/main/java/com/salesforce/rules/PathBasedRuleRunner.java +++ b/sfge/src/main/java/com/salesforce/rules/PathBasedRuleRunner.java @@ -1,5 +1,6 @@ package com.salesforce.rules; +import com.salesforce.cli.Result; import com.salesforce.exception.UnexpectedException; import com.salesforce.graph.ApexPath; import com.salesforce.graph.ApexPathVertexMetaInfo; @@ -8,6 +9,7 @@ import com.salesforce.graph.ops.expander.ApexPathExpanderConfig; import com.salesforce.graph.ops.expander.PathExpansionException; import com.salesforce.graph.ops.expander.PathExpansionObserver; +import com.salesforce.graph.ops.expander.ops.TaintedFileTracker; import com.salesforce.graph.vertex.BaseSFVertex; import com.salesforce.graph.vertex.MethodVertex; import com.salesforce.graph.vertex.SFVertex; @@ -58,22 +60,27 @@ public PathBasedRuleRunner( * * @return a set of violations that were detected */ - public Set runRules() { + public Result runRules() { + final Result result = new Result(); if (allRules.isEmpty()) { if (LOGGER.isInfoEnabled()) { LOGGER.info( "EntryPoint=" + methodVertex.toSimpleString() + "; No interested rules."); } // TODO: SURFACE A WARNING TO THE USER. - return new HashSet<>(); + return result; } + final TaintedFileTracker taintedFileTracker = new TaintedFileTracker(); + // Build configuration to define how apex paths will be expanded - final ApexPathExpanderConfig expanderConfig = getApexPathExpanderConfig(); + final ApexPathExpanderConfig expanderConfig = getApexPathExpanderConfig(taintedFileTracker); // Get all the paths that originate in the entry point final ApexPathRetrievalSummary pathSummary = getPathSummary(expanderConfig); + extractFileEntryMapping(taintedFileTracker, methodVertex, result); + // Execute rules on the paths rejected executeRulesOnAnomalies(pathSummary.getRejectionReasons()); @@ -95,8 +102,18 @@ public Set runRules() { } progressListener.finishedAnalyzingEntryPoint(pathSummary.getAcceptedPaths(), violations); + result.addViolations(violations); - return violations; + return result; + } + + private void extractFileEntryMapping(TaintedFileTracker taintedFileTracker, MethodVertex entryMethod, Result result) { + final String entryFile = entryMethod.getFileName(); + final String entryMethodName = entryMethod.getName(); + + for (String taintedFile : taintedFileTracker.getTaintedFiles()) { + result.addFileToEntryPoint(taintedFile, entryFile, entryMethodName); + } } private void executeRulesOnAnomalies(List anomalies) { @@ -222,9 +239,15 @@ private ApexPathRetrievalSummary getPathSummary(ApexPathExpanderConfig expanderC return ApexPathUtil.summarizeForwardPaths(g, methodVertex, expanderConfig); } - private ApexPathExpanderConfig getApexPathExpanderConfig() { + private ApexPathExpanderConfig getApexPathExpanderConfig(TaintedFileTracker taintedFileTracker) { ApexPathExpanderConfig.Builder expanderConfigBuilder = ApexPathUtil.getFullConfiguredPathExpanderConfigBuilder(); + // Add default PathExpansionObservers + + // We need a new instance of TaintedFileTracker since the tainted files are unique to each path expansion. + expanderConfigBuilder = expanderConfigBuilder.withPathExpansionObserver(taintedFileTracker); + + // Add observers that the rules request for (AbstractPathBasedRule rule : allRules) { Optional observerOptional = rule.getPathExpansionObserver(); if (observerOptional.isPresent()) { @@ -232,6 +255,8 @@ private ApexPathExpanderConfig getApexPathExpanderConfig() { expanderConfigBuilder.withPathExpansionObserver(observerOptional.get()); } } + + // Add vertices that rules express interest in for (AbstractPathTraversalRule rule : traversalRules) { expanderConfigBuilder = expanderConfigBuilder.withVertexPredicate(rule); } diff --git a/sfge/src/main/java/com/salesforce/rules/ThreadableRuleExecutor.java b/sfge/src/main/java/com/salesforce/rules/ThreadableRuleExecutor.java index 90f7c34a7..55f8dfa30 100644 --- a/sfge/src/main/java/com/salesforce/rules/ThreadableRuleExecutor.java +++ b/sfge/src/main/java/com/salesforce/rules/ThreadableRuleExecutor.java @@ -39,8 +39,7 @@ public static Result run(List submissions) { try { // Create a threadpool and a completion service to monitor it. ExecutorService pool = Executors.newWorkStealingPool(THREAD_COUNT); - CompletionService> completionService = - new ExecutorCompletionService(pool); + CompletionService completionService = new ExecutorCompletionService(pool); // Submit an appropriate amount of callables into the completion service. int submissionCount = submitRunners(completionService, submissions); @@ -51,15 +50,15 @@ public static Result run(List submissions) { LOGGER.info("Beginning wait #" + (completedCount + 1)); } // Get the next set of results. - Set violations = waitForCallable(completionService); - int priorSize = result.getViolations().size(); - result.addViolations(violations); + result.merge(waitForCallable(completionService)); + int priorSize = result.getOrderedViolations().size(); + if (LOGGER.isInfoEnabled()) { LOGGER.info( "Wait #" + (completedCount + 1) + " finished, adding " - + (result.getViolations().size() - priorSize) + + (result.getOrderedViolations().size() - priorSize) + " new entries"); } completedCount += 1; @@ -88,7 +87,7 @@ public static Result run(List submissions) { * points and rules. Returns the number of new runners submitted to the pool. */ private static int submitRunners( - CompletionService> completionService, + CompletionService completionService, List submissions) { int submissionCount = 0; final ProgressListener progressListener = ProgressListenerProvider.get(); @@ -109,11 +108,10 @@ private static int submitRunners( * * @throws ThreadableRuleExecutorException - Thrown when a runner is unable to complete. */ - private static Set waitForCallable( - CompletionService> completionService) { + private static Result waitForCallable(CompletionService completionService) { try { long startTime = System.currentTimeMillis(); - Future> future = completionService.take(); + Future future = completionService.take(); long endTime = System.currentTimeMillis(); if (LOGGER.isInfoEnabled()) { LOGGER.info("Future returned after " + (endTime - startTime) + " ms"); @@ -124,7 +122,7 @@ private static Set waitForCallable( } } - private static class CallableExecutor implements Callable> { + private static class CallableExecutor implements Callable { private final ThreadableRuleSubmission submission; private boolean timedOut; @@ -133,9 +131,9 @@ public CallableExecutor( this.submission = submission; } - public Set call() { + public Result call() { Timer timer = new Timer(this.getClass().getSimpleName() + " Timer"); - TreeSet violations = new TreeSet<>(); + Result result = new Result(); try (CloseableThreadContext.Instance closeable = LogUtil.startRuleRun()) { submission.initializeThreadLocals(); submission.beforeRun(); @@ -163,7 +161,7 @@ public void run() { } }; timer.schedule(task, TIMEOUT); - violations.addAll( + result.merge( runRules( submission.getGraph(), submission.getRules(), @@ -177,7 +175,7 @@ public void run() { if (timedOut) { // If the thread timed out, we should create a violation indicating that this // occurred. - violations.add( + result.addViolation( new Violation.TimeoutViolation( "Path evaluation timed out after " + TIMEOUT + " ms", submission.getPathEntry())); @@ -201,7 +199,7 @@ public void run() { + ex.getMessage() + ": " + frameString; - violations.add( + result.addViolation( new Violation.InternalErrorViolation( details, submission.getPathEntry())); if (LOGGER.isErrorEnabled()) { @@ -215,25 +213,26 @@ public void run() { if (LOGGER.isInfoEnabled()) { LOGGER.info("Finished. method=" + submission.getPathEntry().toSimpleString()); } - submission.afterRun(violations); - return violations; + submission.afterRun(result); + return result; } - private Set runRules( + private Result runRules( GraphTraversalSource graph, List rules, MethodVertex pathEntry) { - Set violations = new HashSet<>(); + final Result result = new Result(); final PathBasedRuleRunner pathBasedRuleRunner = new PathBasedRuleRunner(graph, rules, pathEntry); try { - violations.addAll(pathBasedRuleRunner.runRules()); + result.merge(pathBasedRuleRunner.runRules()); } catch (PathExpansionLimitReachedException ex) { - violations.add(new Violation.LimitReachedViolation(ex.getMessage(), pathEntry)); + result.addViolation( + new Violation.LimitReachedViolation(ex.getMessage(), pathEntry)); } catch (UserActionException ex) { - violations.add(new Violation.UserActionViolation(ex.getMessage(), pathEntry)); + result.addViolation(new Violation.UserActionViolation(ex.getMessage(), pathEntry)); } - return violations; + return result; } } @@ -271,10 +270,10 @@ default void beforeRun() { /** * Invoked in a Callable after the rules are evaluated * - * @param violations - All violations that were created by this execution. TreeSet - * guarantees proper ordering. + * @param result - All violations that were created by this execution. TreeSet guarantees + * proper ordering. */ - default void afterRun(TreeSet violations) { + default void afterRun(Result result) { // NO OP } } diff --git a/sfge/src/main/java/com/salesforce/rules/Violation.java b/sfge/src/main/java/com/salesforce/rules/Violation.java index 6a436e152..d5c318c83 100644 --- a/sfge/src/main/java/com/salesforce/rules/Violation.java +++ b/sfge/src/main/java/com/salesforce/rules/Violation.java @@ -143,13 +143,27 @@ public int hashCode() { public int compareTo(Violation o) { // First, compare the attributes that are common to all violation types. final int baseComparison = - Comparator.comparing(Violation::getSourceFileName) - .thenComparing(Violation::getSourceDefiningType) - .thenComparing(Violation::getSourceLineNumber) - .thenComparing(Violation::getSourceColumnNumber) - .thenComparing(Violation::getSourceVertexName) - .thenComparing(Violation::getRuleName) - .thenComparing(Violation::getMessage) + Comparator.comparing( + Violation::getSourceFileName, + Comparator.nullsLast(Comparator.naturalOrder())) + .thenComparing( + Violation::getSourceDefiningType, + Comparator.nullsLast(Comparator.naturalOrder())) + .thenComparing( + Violation::getSourceLineNumber, + Comparator.nullsLast(Comparator.naturalOrder())) + .thenComparing( + Violation::getSourceColumnNumber, + Comparator.nullsLast(Comparator.naturalOrder())) + .thenComparing( + Violation::getSourceVertexName, + Comparator.nullsLast(Comparator.naturalOrder())) + .thenComparing( + Violation::getRuleName, + Comparator.nullsLast(Comparator.naturalOrder())) + .thenComparing( + Violation::getMessage, + Comparator.nullsLast(Comparator.naturalOrder())) .compare(this, o); // If comparison of the base attributes revealed a definitive ordering, then we should use diff --git a/sfge/src/test/java/com/salesforce/TestUtil.java b/sfge/src/test/java/com/salesforce/TestUtil.java index 7118e9525..fab8421a0 100644 --- a/sfge/src/test/java/com/salesforce/TestUtil.java +++ b/sfge/src/test/java/com/salesforce/TestUtil.java @@ -9,6 +9,7 @@ import com.salesforce.apex.jorje.ASTConstants; import com.salesforce.apex.jorje.AstNodeWrapper; import com.salesforce.apex.jorje.JorjeUtil; +import com.salesforce.cli.Result; import com.salesforce.exception.UnexpectedException; import com.salesforce.graph.ApexPath; import com.salesforce.graph.Schema; @@ -320,7 +321,8 @@ public static List getViolations( final PathBasedRuleRunner ruleRunner = new PathBasedRuleRunner(g, Arrays.asList(rules), methodVertex); - final List violations = new ArrayList<>(ruleRunner.runRules()); + Result result = ruleRunner.runRules(); + final List violations = new ArrayList<>(result.getOrderedViolations()); return violations; } diff --git a/sfge/src/test/java/com/salesforce/cli/CacheCreatorTest.java b/sfge/src/test/java/com/salesforce/cli/CacheCreatorTest.java new file mode 100644 index 000000000..7dfb48e2e --- /dev/null +++ b/sfge/src/test/java/com/salesforce/cli/CacheCreatorTest.java @@ -0,0 +1,93 @@ +package com.salesforce.cli; + +import com.salesforce.config.SfgeConfigTestProvider; +import com.salesforce.config.TestSfgeConfig; +import org.hamcrest.MatcherAssert; +import org.hamcrest.Matchers; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.mockito.ArgumentCaptor; +import org.mockito.Mock; +import org.mockito.Mockito; +import org.mockito.MockitoAnnotations; + +import java.io.File; +import java.io.IOException; + +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.*; + +public class CacheCreatorTest { + private static final String DUMMY_FILE_NAME = ".test-sfge-cache" + File.separator + "myFileToEntriesDummy.json"; + @Mock CacheCreator.Dependencies dependencies; + + AutoCloseable openMocks; + + @BeforeEach + void setUp() { + openMocks = MockitoAnnotations.openMocks(this); + SfgeConfigTestProvider.set(new TestSfgeConfig() { + @Override + public String getFilesToEntriesCacheLocation() { + return DUMMY_FILE_NAME; + } + }); + } + + @AfterEach + void tearDown() throws Exception { + openMocks.close(); + SfgeConfigTestProvider.remove(); + } + + @Test + public void testFilesToEntriesDataCreationForFileCreation() throws IOException { + Result result = new Result(); + CacheCreator cacheCreator = new CacheCreator(dependencies); + cacheCreator.create(result); + + final ArgumentCaptor fileNameCaptor = ArgumentCaptor.forClass(String.class); + final ArgumentCaptor dataCaptor = ArgumentCaptor.forClass(String.class); + verify(dependencies, times(1)).writeFile(fileNameCaptor.capture(), dataCaptor.capture()); + + String actualFilename = fileNameCaptor.getValue(); + MatcherAssert.assertThat(actualFilename, Matchers.containsString(DUMMY_FILE_NAME)); + + } + + @Test + public void testIOExceptionDoesNotDisrupt() throws IOException { + doThrow(new IOException("dummy exception")).when(dependencies).writeFile(anyString(), anyString()); + + Result result = new Result(); + CacheCreator cacheCreator = new CacheCreator(dependencies); + + assertDoesNotThrow(() -> cacheCreator.create(result)); + } + + @Test + public void testNoFileWrittenWhenCachingIsDisabled() throws IOException { + + // Disable caching + SfgeConfigTestProvider.set(new TestSfgeConfig() { + @Override + public boolean isCachingDisabled() { + return true; + } + }); + + try { + Result result = new Result(); + CacheCreator cacheCreator = new CacheCreator(dependencies); + cacheCreator.create(result); + + verify(dependencies, Mockito.never()).writeFile(anyString(), anyString()); + + } finally { + // Cleanup test config override + SfgeConfigTestProvider.remove(); + } + } +} diff --git a/sfge/src/test/java/com/salesforce/cli/FilesToEntriesMapTest.java b/sfge/src/test/java/com/salesforce/cli/FilesToEntriesMapTest.java new file mode 100644 index 000000000..fc5bfb3d1 --- /dev/null +++ b/sfge/src/test/java/com/salesforce/cli/FilesToEntriesMapTest.java @@ -0,0 +1,105 @@ +package com.salesforce.cli; + +import org.hamcrest.Matchers; +import org.json.simple.JSONArray; +import org.json.simple.JSONObject; +import org.json.simple.parser.JSONParser; +import org.json.simple.parser.ParseException; +import org.junit.jupiter.api.Test; + +import java.util.HashMap; +import java.util.HashSet; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.*; + +public class FilesToEntriesMapTest { + + private static final String FILE_1 = "file1"; + private static final String FILE_2 = "file2"; + private static final String FILE_3 = "file3"; + private static final String ENTRY_FILE_1 = "entryFile1"; + private static final String ENTRY_FILE_2 = "entryFile2"; + private static final String ENTRY_METHOD_1 = "entryMethod1"; + private static final String ENTRY_METHOD_2 = "entryMethod2"; + private static final String ENTRY_METHOD_3 = "entryMethod3"; + + + @Test + public void testEntryCreation() { + FilesToEntriesMap filesToEntriesMap = new FilesToEntriesMap(); + filesToEntriesMap.put(FILE_1, ENTRY_FILE_1, ENTRY_METHOD_1); + filesToEntriesMap.put(FILE_2, ENTRY_FILE_1, ENTRY_METHOD_2); + filesToEntriesMap.put(FILE_1, ENTRY_FILE_2, ENTRY_METHOD_3); + + HashMap> actualMap = filesToEntriesMap.getMap(); + assertThat(actualMap.keySet(), containsInAnyOrder(FILE_1, FILE_2)); + + HashSet entries1 = actualMap.get(FILE_1); + assertThat(entries1, containsInAnyOrder(new FilesToEntriesMap.Entry(ENTRY_FILE_1, ENTRY_METHOD_1), new FilesToEntriesMap.Entry(ENTRY_FILE_2, ENTRY_METHOD_3))); + + HashSet entries2 = actualMap.get(FILE_2); + assertThat(entries2, contains(new FilesToEntriesMap.Entry(ENTRY_FILE_1, ENTRY_METHOD_2))); + } + + @Test + public void testMerge_appendingToExistingFilename() { + FilesToEntriesMap filesToEntriesMap1 = new FilesToEntriesMap(); + filesToEntriesMap1.put(FILE_1, ENTRY_FILE_1, ENTRY_METHOD_1); + filesToEntriesMap1.put(FILE_2, ENTRY_FILE_1, ENTRY_METHOD_2); + filesToEntriesMap1.put(FILE_1, ENTRY_FILE_2, ENTRY_METHOD_3); + + FilesToEntriesMap filesToEntriesMap2 = new FilesToEntriesMap(); + filesToEntriesMap2.put(FILE_2, ENTRY_FILE_2, ENTRY_METHOD_3); + + filesToEntriesMap1.merge(filesToEntriesMap2); + + HashMap> actualMap = filesToEntriesMap1.getMap(); + + HashSet entries2 = actualMap.get(FILE_2); + assertThat(entries2, contains(new FilesToEntriesMap.Entry(ENTRY_FILE_1, ENTRY_METHOD_2), new FilesToEntriesMap.Entry(ENTRY_FILE_2, ENTRY_METHOD_3))); + + } + + @Test + public void testMerge_addingNewFilename() { + FilesToEntriesMap filesToEntriesMap1 = new FilesToEntriesMap(); + filesToEntriesMap1.put(FILE_1, ENTRY_FILE_1, ENTRY_METHOD_1); + filesToEntriesMap1.put(FILE_2, ENTRY_FILE_1, ENTRY_METHOD_2); + filesToEntriesMap1.put(FILE_1, ENTRY_FILE_2, ENTRY_METHOD_3); + + FilesToEntriesMap filesToEntriesMap2 = new FilesToEntriesMap(); + filesToEntriesMap2.put(FILE_3, ENTRY_FILE_2, ENTRY_METHOD_3); + + filesToEntriesMap1.merge(filesToEntriesMap2); + + HashMap> actualMap = filesToEntriesMap1.getMap(); + + HashSet entries3 = actualMap.get(FILE_3); + assertThat(entries3, contains(new FilesToEntriesMap.Entry(ENTRY_FILE_2, ENTRY_METHOD_3))); + } + + @Test + public void testJsonString() throws ParseException { + FilesToEntriesMap filesToEntriesMap = new FilesToEntriesMap(); + filesToEntriesMap.put(FILE_1, ENTRY_FILE_1, ENTRY_METHOD_1); + filesToEntriesMap.put(FILE_2, ENTRY_FILE_1, ENTRY_METHOD_2); + filesToEntriesMap.put(FILE_1, ENTRY_FILE_2, ENTRY_METHOD_3); + + String jsonString = filesToEntriesMap.toJsonString(); + JSONParser parser = new JSONParser(); + JSONObject data = (JSONObject) parser.parse(jsonString); + + assertThat(data.containsKey("data"), Matchers.equalTo(true)); + + JSONArray filesToEntriesMapData = (JSONArray) data.get(FilesToEntriesMap.FIELD_DATA); + assertThat(filesToEntriesMapData.size(), equalTo(2)); + + JSONObject file2Data = (JSONObject) filesToEntriesMapData.get(0); + assertThat(file2Data.get(FilesToEntriesMap.FIELD_FILENAME), equalTo(FILE_2)); + JSONArray entries2 = (JSONArray) file2Data.get(FilesToEntriesMap.FIELD_ENTRIES); + assertThat(entries2.size(), equalTo(1)); + + + } +} diff --git a/sfge/src/test/java/com/salesforce/cli/PathEntryCaptureTest.java b/sfge/src/test/java/com/salesforce/cli/PathEntryCaptureTest.java new file mode 100644 index 000000000..4516fccd2 --- /dev/null +++ b/sfge/src/test/java/com/salesforce/cli/PathEntryCaptureTest.java @@ -0,0 +1,141 @@ +package com.salesforce.cli; + +import com.salesforce.TestUtil; +import com.salesforce.graph.ops.GraphUtil; +import com.salesforce.graph.vertex.MethodVertex; +import com.salesforce.rules.AbstractPathBasedRule; +import com.salesforce.rules.ApexFlsViolationRule; +import com.salesforce.rules.PathBasedRuleRunner; +import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversalSource; +import org.hamcrest.Matchers; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.TestInfo; + +import java.nio.file.Path; +import java.util.*; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.*; + +/** + * Executes path expansion on physical files to verify accuracy of {@link FilesToEntriesMap} created. + */ +public class PathEntryCaptureTest { + private GraphTraversalSource g; + + @BeforeEach + public void setup() throws Exception { + g = GraphUtil.getGraph(); + } + + @Test + public void testEntryOnKeyFile(TestInfo testInfo) throws GraphUtil.GraphLoadException { + String entryClass = "EntryAndKeyClass"; + String entryMethod = "doSomething"; + String keyClass = "EntryAndKeyClass"; + + TestUtil.compileTestFiles(g, testInfo); + FilesToEntriesMap filesToEntriesMap = executePathBasedRuleRunner(entryClass, entryMethod); + + Verifier verifier = new Verifier(filesToEntriesMap, testInfo); + verifier.assertKeyCount(1); + verifier.assertFileToEntryExists(keyClass, entryClass, entryMethod); + } + + @Test + public void testEntryNotOnKeyFile(TestInfo testInfo) throws GraphUtil.GraphLoadException { + String entryClass = "EntryClass"; + String entryMethod = "doSomething"; + String keyClass = "KeyClass"; + + TestUtil.compileTestFiles(g, testInfo); + FilesToEntriesMap filesToEntriesMap = executePathBasedRuleRunner(entryClass, entryMethod); + + Verifier verifier = new Verifier(filesToEntriesMap, testInfo); + verifier.assertKeyCount(2); + verifier.assertFileToEntryExists(keyClass, entryClass, entryMethod); + verifier.assertFileToEntryExists(entryClass, entryClass, entryMethod); + } + + @Test + public void testNoKeyForUnusedFile(TestInfo testInfo) throws GraphUtil.GraphLoadException { + String entryClass = "EntryClass"; + String entryMethod = "doSomething"; + String unusedClass = "UnusedClass"; + + TestUtil.compileTestFiles(g, testInfo); + FilesToEntriesMap filesToEntriesMap = executePathBasedRuleRunner(entryClass, entryMethod); + + Verifier verifier = new Verifier(filesToEntriesMap, testInfo); + verifier.assertFileToEntryDoesNotExist(unusedClass, entryClass, entryMethod); + } + + @Test + public void testMultiplePathsToDifferentClasses(TestInfo testInfo) throws GraphUtil.GraphLoadException { + String entryClass = "EntryClass"; + String entryMethod = "doSomething"; + + TestUtil.compileTestFiles(g, testInfo); + FilesToEntriesMap filesToEntriesMap = executePathBasedRuleRunner(entryClass, entryMethod); + + Verifier verifier = new Verifier(filesToEntriesMap, testInfo); + verifier.assertKeyCount(3); + verifier.assertFileToEntryExists("KeyClass1", entryClass, entryMethod); + verifier.assertFileToEntryExists("KeyClass2", entryClass, entryMethod); + verifier.assertFileToEntryExists(entryClass, entryClass, entryMethod); + } + + + + private FilesToEntriesMap executePathBasedRuleRunner(String entryClass, String entryMethod) { + MethodVertex methodVertex = TestUtil.getMethodVertex(g, entryClass, entryMethod); + List rules = + Collections.singletonList(ApexFlsViolationRule.getInstance()); + + // Define a PathBasedRuleRunner to apply the rule against the method vertex. + PathBasedRuleRunner runner = new PathBasedRuleRunner(g, rules, methodVertex); + + Result result = runner.runRules(); + return result.getFilesToEntriesMap(); + } + + /** + * Helper class to offload the brunt of assertions and make the tests more readable. + */ + class Verifier { + private final HashMap> map; + private final Path testFileDirectory; + + Verifier(FilesToEntriesMap filesToEntriesMap, TestInfo testInfo) { + this.map = filesToEntriesMap.getMap(); + this.testFileDirectory = TestUtil.getTestFileDirectory(testInfo); + } + + void assertKeyCount(int expectedCount) { + assertThat("Incorrect key count.", map, Matchers.aMapWithSize(expectedCount)); + } + + void assertFileToEntryExists(String keyClass, String entryClass, String entryMethod) { + assertThat("Expected key file not found: " + keyClass, map.keySet(), hasItem(getAbsolutePath(testFileDirectory, keyClass))); + + HashSet entries = map.get(getAbsolutePath(testFileDirectory, keyClass)); + assertThat("Expected entry not found.", entries, hasItem(getExpectedEntry(testFileDirectory, entryClass, entryMethod))); + } + + void assertFileToEntryDoesNotExist(String keyClass, String entryClass, String entryMethod) { + assertThat("Unxpected key file found: " + keyClass, map.keySet(), not(hasItem(getAbsolutePath(testFileDirectory, keyClass)))); + + HashSet entries = map.get(getAbsolutePath(testFileDirectory, keyClass)); + assertThat("Unexpected entry found.", entries, not(hasItem(getExpectedEntry(testFileDirectory, entryClass, entryMethod)))); + } + + private FilesToEntriesMap.Entry getExpectedEntry(Path testFileDirectory, String entryClass, String entryMethod) { + return new FilesToEntriesMap.Entry(getAbsolutePath(testFileDirectory, entryClass), entryMethod); + } + + private String getAbsolutePath(Path testFileDirectory, String entryClass) { + return testFileDirectory.resolve(entryClass + ".cls").toAbsolutePath().toString(); + } + } +} diff --git a/sfge/src/test/java/com/salesforce/config/SfgeConfigProviderTest.java b/sfge/src/test/java/com/salesforce/config/SfgeConfigProviderTest.java index bfc1a0779..10c171b08 100644 --- a/sfge/src/test/java/com/salesforce/config/SfgeConfigProviderTest.java +++ b/sfge/src/test/java/com/salesforce/config/SfgeConfigProviderTest.java @@ -9,6 +9,10 @@ import org.junit.jupiter.api.Test; public class SfgeConfigProviderTest { + + private static final String DUMMY_CACHE_DIR = "dummyCacheDir"; + private static final String DUMMY_FILES_TO_ENTRIES_LOCATION = "dummyFilesToEntriesData"; + @Test public void testDefaultImplementation() { final SfgeConfig sfgeConfig = SfgeConfigProvider.get(); @@ -56,6 +60,16 @@ public int getStackDepthLimit() { public int getPathExpansionLimit() { return -1 * EnvUtil.DEFAULT_PATH_EXPANSION_LIMIT; } + + @Override + public String getFilesToEntriesCacheLocation() { + return DUMMY_FILES_TO_ENTRIES_LOCATION; + } + + @Override + public boolean isCachingDisabled() { + return true; + } }); final SfgeConfig sfgeConfig = SfgeConfigProvider.get(); @@ -81,6 +95,8 @@ public int getPathExpansionLimit() { MatcherAssert.assertThat( sfgeConfig.getPathExpansionLimit(), equalTo(-1 * EnvUtil.DEFAULT_PATH_EXPANSION_LIMIT)); + MatcherAssert.assertThat(sfgeConfig.getFilesToEntriesCacheLocation(), equalTo(DUMMY_FILES_TO_ENTRIES_LOCATION)); + MatcherAssert.assertThat(sfgeConfig.isCachingDisabled(), equalTo(true)); } finally { SfgeConfigTestProvider.remove(); } diff --git a/sfge/src/test/java/com/salesforce/config/TestSfgeConfig.java b/sfge/src/test/java/com/salesforce/config/TestSfgeConfig.java index c6776a995..702014bd5 100644 --- a/sfge/src/test/java/com/salesforce/config/TestSfgeConfig.java +++ b/sfge/src/test/java/com/salesforce/config/TestSfgeConfig.java @@ -39,4 +39,14 @@ public int getStackDepthLimit() { public int getPathExpansionLimit() { return SfgeConfigImpl.getInstance().getPathExpansionLimit(); } + + @Override + public String getFilesToEntriesCacheLocation() { + return SfgeConfigImpl.getInstance().getFilesToEntriesCacheLocation(); + } + + @Override + public boolean isCachingDisabled() { + return SfgeConfigImpl.getInstance().isCachingDisabled(); + } } diff --git a/sfge/src/test/java/com/salesforce/rules/PathBasedRuleRunnerTest.java b/sfge/src/test/java/com/salesforce/rules/PathBasedRuleRunnerTest.java index c05c3cf17..08cc60e9f 100644 --- a/sfge/src/test/java/com/salesforce/rules/PathBasedRuleRunnerTest.java +++ b/sfge/src/test/java/com/salesforce/rules/PathBasedRuleRunnerTest.java @@ -3,10 +3,10 @@ import static org.hamcrest.Matchers.hasSize; import com.salesforce.TestUtil; +import com.salesforce.cli.Result; import com.salesforce.graph.vertex.MethodVertex; import java.util.Collections; import java.util.List; -import java.util.Set; import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversalSource; import org.hamcrest.MatcherAssert; import org.junit.jupiter.api.BeforeEach; @@ -33,7 +33,7 @@ public void pathsWithoutInterestingVerticesAreIgnored() { + " }\n" + " }\n" + "}"; - TestUtil.buildGraph(g, sourceCode, true); + TestUtil.buildGraph(g, sourceCode, false); // Get the vertex corresponding to the method, and an instance of the FLS rule in a // singleton list. @@ -44,7 +44,8 @@ public void pathsWithoutInterestingVerticesAreIgnored() { // Define a PathBasedRuleRunner to apply the rule against the method vertex. PathBasedRuleRunner runner = new PathBasedRuleRunner(g, rules, methodVertex); - Set violations = runner.runRules(); + Result result = runner.runRules(); + List violations = result.getOrderedViolations(); MatcherAssert.assertThat(violations, hasSize(0)); } } diff --git a/sfge/src/test/java/com/salesforce/rules/RuleRunnerTest.java b/sfge/src/test/java/com/salesforce/rules/RuleRunnerTest.java index d2d1fa9f9..4c692c234 100644 --- a/sfge/src/test/java/com/salesforce/rules/RuleRunnerTest.java +++ b/sfge/src/test/java/com/salesforce/rules/RuleRunnerTest.java @@ -55,7 +55,7 @@ public void properlyRunsStaticRule() { AbstractRuleRunner rr = new TestRuleRunner(g, VertexCacheProvider.get()); final Result result = rr.runRules(rules); - List vs = result.getViolations(); + List vs = result.getOrderedViolations(); MatcherAssert.assertThat(vs, hasSize(equalTo(1))); assertEquals("Hard-coded static violation", vs.get(0).getMessage()); @@ -82,7 +82,7 @@ public void runsPathBasedRulesOnAuraMethods() { AbstractRuleRunner rr = new TestRuleRunner(g, VertexCacheProvider.get()); final Result result = rr.runRules(rules); - List vs = result.getViolations(); + List vs = result.getOrderedViolations(); MatcherAssert.assertThat(vs, hasSize(equalTo(1))); assertEquals("Hard-coded path violation", vs.get(0).getMessage()); @@ -108,7 +108,7 @@ public void runsPathBasedRulesOnPageReferenceMethods() { AbstractRuleRunner rr = new TestRuleRunner(g, VertexCacheProvider.get()); final Result result = rr.runRules(rules); - List vs = result.getViolations(); + List vs = result.getOrderedViolations(); MatcherAssert.assertThat(vs, hasSize(equalTo(1))); assertEquals("Hard-coded path violation", vs.get(0).getMessage()); @@ -162,7 +162,7 @@ public void filtersViolationsFromNontargetedFiles() { // Since we're providing a list of targeted files, only those files should have violations // associated with them. final Result result = rr.runRules(rules, targets); - List vs = result.getViolations(); + List vs = result.getOrderedViolations(); MatcherAssert.assertThat(vs, hasSize(4)); Set filesWithStaticViolation = new HashSet<>(); @@ -303,8 +303,8 @@ public void initializeThreadLocals() { } @Override - public void afterRun(TreeSet violations) { - super.afterRun(violations); + public void afterRun(Result result) { + super.afterRun(result); VertexCacheTestProvider.remove(); } }; diff --git a/sfge/src/test/java/com/salesforce/rules/unusedmethod/BaseUnusedMethodTest.java b/sfge/src/test/java/com/salesforce/rules/unusedmethod/BaseUnusedMethodTest.java index 54fca2afa..f2b613c58 100644 --- a/sfge/src/test/java/com/salesforce/rules/unusedmethod/BaseUnusedMethodTest.java +++ b/sfge/src/test/java/com/salesforce/rules/unusedmethod/BaseUnusedMethodTest.java @@ -7,6 +7,7 @@ import com.google.common.collect.Lists; import com.salesforce.TestUtil; import com.salesforce.apex.jorje.ASTConstants.NodeType; +import com.salesforce.cli.Result; import com.salesforce.graph.Schema; import com.salesforce.graph.vertex.MethodVertex; import com.salesforce.graph.vertex.SFVertexFactory; @@ -274,7 +275,8 @@ protected void assertExpectations( TestUtil.getMethodVertex(g, entryDefiningType, entryMethod); PathBasedRuleRunner runner = new PathBasedRuleRunner(g, Lists.newArrayList(rule), entryMethodVertex); - List violations = new ArrayList<>(runner.runRules()); + Result result = runner.runRules(); + List violations = new ArrayList<>(result.getOrderedViolations()); violations.addAll( rule.postProcess( g)); // Important! This is where the violations are created for this rule diff --git a/sfge/src/test/resources/test-files/com/salesforce/cli/PathEntryCaptureTest/testEntryNotOnKeyFile/EntryClass.cls b/sfge/src/test/resources/test-files/com/salesforce/cli/PathEntryCaptureTest/testEntryNotOnKeyFile/EntryClass.cls new file mode 100644 index 000000000..99a52da77 --- /dev/null +++ b/sfge/src/test/resources/test-files/com/salesforce/cli/PathEntryCaptureTest/testEntryNotOnKeyFile/EntryClass.cls @@ -0,0 +1,8 @@ +public class EntryClass { + @AuraEnabled + public static void doSomething() { + KeyClass key = new KeyClass(); + Integer int = key.getMyInt(); + System.debug(int); + } +} diff --git a/sfge/src/test/resources/test-files/com/salesforce/cli/PathEntryCaptureTest/testEntryNotOnKeyFile/KeyClass.cls b/sfge/src/test/resources/test-files/com/salesforce/cli/PathEntryCaptureTest/testEntryNotOnKeyFile/KeyClass.cls new file mode 100644 index 000000000..28b66fe06 --- /dev/null +++ b/sfge/src/test/resources/test-files/com/salesforce/cli/PathEntryCaptureTest/testEntryNotOnKeyFile/KeyClass.cls @@ -0,0 +1,5 @@ +public class KeyClass { + public Integer getMyInt() { + return 5; + } +} diff --git a/sfge/src/test/resources/test-files/com/salesforce/cli/PathEntryCaptureTest/testEntryOnKeyFile/EntryAndKeyClass.cls b/sfge/src/test/resources/test-files/com/salesforce/cli/PathEntryCaptureTest/testEntryOnKeyFile/EntryAndKeyClass.cls new file mode 100644 index 000000000..67174e302 --- /dev/null +++ b/sfge/src/test/resources/test-files/com/salesforce/cli/PathEntryCaptureTest/testEntryOnKeyFile/EntryAndKeyClass.cls @@ -0,0 +1,10 @@ +public class EntryAndKeyClass { + @AuraEnabled + public static void doSomething() { + Integer int = getMyInt(); + } + + static Integer getMyInt() { + return 5; + } +} diff --git a/sfge/src/test/resources/test-files/com/salesforce/cli/PathEntryCaptureTest/testMultiplePathsToDifferentClasses/EntryClass.cls b/sfge/src/test/resources/test-files/com/salesforce/cli/PathEntryCaptureTest/testMultiplePathsToDifferentClasses/EntryClass.cls new file mode 100644 index 000000000..9ea84a4ba --- /dev/null +++ b/sfge/src/test/resources/test-files/com/salesforce/cli/PathEntryCaptureTest/testMultiplePathsToDifferentClasses/EntryClass.cls @@ -0,0 +1,14 @@ +public class EntryClass { + @AuraEnabled + public static void doSomething(boolean decider) { + if (decider) { + KeyClass1 key1 = new KeyClass1(); + Integer int = key1.getMyInt(); + System.debug(int); + } else { + KeyClass2 key2 = new KeyClass2(); + String str = key2.getMyString(); + System.debug(str); + } + } +} diff --git a/sfge/src/test/resources/test-files/com/salesforce/cli/PathEntryCaptureTest/testMultiplePathsToDifferentClasses/KeyClass1.cls b/sfge/src/test/resources/test-files/com/salesforce/cli/PathEntryCaptureTest/testMultiplePathsToDifferentClasses/KeyClass1.cls new file mode 100644 index 000000000..8bf7d58a0 --- /dev/null +++ b/sfge/src/test/resources/test-files/com/salesforce/cli/PathEntryCaptureTest/testMultiplePathsToDifferentClasses/KeyClass1.cls @@ -0,0 +1,5 @@ +public class KeyClass1 { + public Integer getMyInt() { + return 5; + } +} diff --git a/sfge/src/test/resources/test-files/com/salesforce/cli/PathEntryCaptureTest/testMultiplePathsToDifferentClasses/KeyClass2.cls b/sfge/src/test/resources/test-files/com/salesforce/cli/PathEntryCaptureTest/testMultiplePathsToDifferentClasses/KeyClass2.cls new file mode 100644 index 000000000..565ab1edd --- /dev/null +++ b/sfge/src/test/resources/test-files/com/salesforce/cli/PathEntryCaptureTest/testMultiplePathsToDifferentClasses/KeyClass2.cls @@ -0,0 +1,5 @@ +public class KeyClass2 { + public String getMyString() { + return 'hi'; + } +} diff --git a/sfge/src/test/resources/test-files/com/salesforce/cli/PathEntryCaptureTest/testNoKeyForUnusedFile/EntryClass.cls b/sfge/src/test/resources/test-files/com/salesforce/cli/PathEntryCaptureTest/testNoKeyForUnusedFile/EntryClass.cls new file mode 100644 index 000000000..99a52da77 --- /dev/null +++ b/sfge/src/test/resources/test-files/com/salesforce/cli/PathEntryCaptureTest/testNoKeyForUnusedFile/EntryClass.cls @@ -0,0 +1,8 @@ +public class EntryClass { + @AuraEnabled + public static void doSomething() { + KeyClass key = new KeyClass(); + Integer int = key.getMyInt(); + System.debug(int); + } +} diff --git a/sfge/src/test/resources/test-files/com/salesforce/cli/PathEntryCaptureTest/testNoKeyForUnusedFile/KeyClass.cls b/sfge/src/test/resources/test-files/com/salesforce/cli/PathEntryCaptureTest/testNoKeyForUnusedFile/KeyClass.cls new file mode 100644 index 000000000..28b66fe06 --- /dev/null +++ b/sfge/src/test/resources/test-files/com/salesforce/cli/PathEntryCaptureTest/testNoKeyForUnusedFile/KeyClass.cls @@ -0,0 +1,5 @@ +public class KeyClass { + public Integer getMyInt() { + return 5; + } +} diff --git a/sfge/src/test/resources/test-files/com/salesforce/cli/PathEntryCaptureTest/testNoKeyForUnusedFile/UnusedClass.cls b/sfge/src/test/resources/test-files/com/salesforce/cli/PathEntryCaptureTest/testNoKeyForUnusedFile/UnusedClass.cls new file mode 100644 index 000000000..d98eed799 --- /dev/null +++ b/sfge/src/test/resources/test-files/com/salesforce/cli/PathEntryCaptureTest/testNoKeyForUnusedFile/UnusedClass.cls @@ -0,0 +1,5 @@ +public class UnusedClass { + static void notCalled() { + System.debug('no one called me :('); + } +} From ac36203314ce7036433d7d385f11177e0894e102 Mon Sep 17 00:00:00 2001 From: Stephen Carter <123964848+stephen-carter-at-sf@users.noreply.github.com> Date: Thu, 30 May 2024 16:36:21 -0400 Subject: [PATCH 2/7] CHANGE (DevOps): @W-15896661@: Update publish script to make v4 use latest tag for June release (#1492) --- .github/ISSUE_TEMPLATE/0-scanner_run_bug.yml | 2 +- .github/ISSUE_TEMPLATE/1-scanner_run_dfa_bug.yml | 2 +- .github/ISSUE_TEMPLATE/2-scanner_run_false_result.yml | 2 +- .github/ISSUE_TEMPLATE/3-scanner_run_dfa_false_result.yml | 2 +- .github/workflows/publish-to-npm.yml | 6 +++--- 5 files changed, 7 insertions(+), 7 deletions(-) diff --git a/.github/ISSUE_TEMPLATE/0-scanner_run_bug.yml b/.github/ISSUE_TEMPLATE/0-scanner_run_bug.yml index 1af86319a..cbacd1a18 100644 --- a/.github/ISSUE_TEMPLATE/0-scanner_run_bug.yml +++ b/.github/ISSUE_TEMPLATE/0-scanner_run_bug.yml @@ -77,7 +77,7 @@ body: description: | What do you get from the command "sf plugins"? placeholder: | - Example: @salesforce/sfdx-scanner 4.1.0 (latest-beta) + Example: @salesforce/sfdx-scanner 4.3.0 (latest) validations: required: true - type: textarea diff --git a/.github/ISSUE_TEMPLATE/1-scanner_run_dfa_bug.yml b/.github/ISSUE_TEMPLATE/1-scanner_run_dfa_bug.yml index 928089ae9..cee2cb8cf 100644 --- a/.github/ISSUE_TEMPLATE/1-scanner_run_dfa_bug.yml +++ b/.github/ISSUE_TEMPLATE/1-scanner_run_dfa_bug.yml @@ -81,7 +81,7 @@ body: description: | What do you get from the command "sf plugins"? placeholder: | - Example: @salesforce/sfdx-scanner 4.1.0 (latest-beta) + Example: @salesforce/sfdx-scanner 4.3.0 (latest) validations: required: true - type: input diff --git a/.github/ISSUE_TEMPLATE/2-scanner_run_false_result.yml b/.github/ISSUE_TEMPLATE/2-scanner_run_false_result.yml index 729fb7684..a27802a2a 100644 --- a/.github/ISSUE_TEMPLATE/2-scanner_run_false_result.yml +++ b/.github/ISSUE_TEMPLATE/2-scanner_run_false_result.yml @@ -71,7 +71,7 @@ body: description: | What do you get from the command "sf plugins"? placeholder: | - Example: @salesforce/sfdx-scanner 4.1.0 (latest-beta) + Example: @salesforce/sfdx-scanner 4.3.0 (latest) validations: required: true - type: textarea diff --git a/.github/ISSUE_TEMPLATE/3-scanner_run_dfa_false_result.yml b/.github/ISSUE_TEMPLATE/3-scanner_run_dfa_false_result.yml index fee67e2ab..f2017f7db 100644 --- a/.github/ISSUE_TEMPLATE/3-scanner_run_dfa_false_result.yml +++ b/.github/ISSUE_TEMPLATE/3-scanner_run_dfa_false_result.yml @@ -58,7 +58,7 @@ body: description: | What do you get from the command "sf plugins"? placeholder: | - Example: @salesforce/sfdx-scanner 4.1.0 (latest-beta) + Example: @salesforce/sfdx-scanner 4.3.0 (latest) validations: required: true - type: input diff --git a/.github/workflows/publish-to-npm.yml b/.github/workflows/publish-to-npm.yml index 5f89a4133..0689b8070 100644 --- a/.github/workflows/publish-to-npm.yml +++ b/.github/workflows/publish-to-npm.yml @@ -53,7 +53,7 @@ jobs: with: ctc: false # We've been told we don't have to care about this until someone makes us care. sign: true - tag: latest-beta-rc # Publish as a release candidate, so we can do our validations against it. + tag: latest-rc # Publish as a release candidate, so we can do our validations against it. githubTag: ${{ github.event.release.tag_name || inputs.tag }} secrets: inherit # Step 3: Run smoke tests against the release candidate. @@ -81,7 +81,7 @@ jobs: java-version: '11' # For now, Java version is hardcoded. # Install SF, and the release candidate version. - run: npm install -g @salesforce/cli - - run: sf plugins install @salesforce/sfdx-scanner@latest-beta-rc + - run: sf plugins install @salesforce/sfdx-scanner@latest-rc # Log the installed plugins for easier debugging. - run: sf plugins # Attempt to run the smoke tests. @@ -102,7 +102,7 @@ jobs: node-version: 'lts/*' - run: | echo "//registry.npmjs.org/:_authToken=${{ secrets.NPM_TOKEN }}" > ~/.npmrc - npm dist-tag add @salesforce/sfdx-scanner@${{ github.event.release.tag_name || inputs.tag }} latest-beta + npm dist-tag add @salesforce/sfdx-scanner@${{ github.event.release.tag_name || inputs.tag }} latest # Step 5: Create a Pull Request for merging `main` into `dev` create-main2dev-pull-request: needs: promote-to-latest From a90943099c29a7c37dfe6e67c9d08547f2f604fe Mon Sep 17 00:00:00 2001 From: Stephen Carter <123964848+stephen-carter-at-sf@users.noreply.github.com> Date: Thu, 13 Jun 2024 13:19:50 -0400 Subject: [PATCH 3/7] CHANGE (PMD): @W-15992482@: Upgrade to PMD 7.2.0 from 7.1.0 (#1505) --- pmd-cataloger/build.gradle.kts | 22 ++++++++++++++++++---- src/Constants.ts | 2 +- src/lib/pmd/PmdCommandInfo.ts | 5 ++--- 3 files changed, 21 insertions(+), 8 deletions(-) diff --git a/pmd-cataloger/build.gradle.kts b/pmd-cataloger/build.gradle.kts index 12bcfa0db..e22895ee0 100644 --- a/pmd-cataloger/build.gradle.kts +++ b/pmd-cataloger/build.gradle.kts @@ -63,7 +63,7 @@ tasks.register("deletePmdCatalogerDist") { // ======== DEFINE/UPDATE PMD7 DIST RELATED TASKS ===================================================================== val pmd7DistDir = "$distDir/pmd7" -val pmd7Version = "7.1.0" +val pmd7Version = "7.2.0" val pmd7File = "pmd-dist-$pmd7Version-bin.zip" tasks.register("downloadPmd7") { @@ -76,12 +76,26 @@ tasks.register("installPmd7") { dependsOn("downloadPmd7") from(zipTree("$buildDir/$pmd7File")) - // I went to https://github.com/pmd/pmd/tree/pmd_releases/7.1.0 and for each of the languages that we support + // TO KEEP THE BELOW MODULE DEPENDENCIES LIST CORRECT AND UP TO DATE: + // I went to https://github.com/pmd/pmd/tree/pmd_releases/7.2.0 and for each of the languages that we support // (apex, java, visualforce, xml), I took a look at its direct and indirect dependencies at // https://central.sonatype.com/artifact/net.sourceforge.pmd/pmd-apex/dependencies - // by selecting the 7.1.0 dropdown and clicking on "Dependencies" and selecting "All Dependencies". + // by selecting the 7.2.0 dropdown and clicking on "Dependencies" and selecting "All Dependencies". // For completeness, I listed the modules and all their compile time dependencies (direct and indirect). // Duplicates don't matter since we use setOf. + // + // A tip when we upgrade to see if there are any differences between minor releases of a given module: + // 1) Open the following in two different tabs (using pmd-core as an example): + // * Go to https://mvnrepository.com/artifact/net.sourceforge.pmd/pmd-core/7.1.0 + // * Go to https://mvnrepository.com/artifact/net.sourceforge.pmd/pmd-core/7.2.0 + // Then compare the compile dependencies and their versions to see if there are any changes. + // Do this will all modules we care about. Obviously the pmd-core dependency version will change but if nothing else + // changes then this means no updates are needed for that module. If there are changes to pmd-core's dependencies... + // then all modules that depend on pmd-core should be updated below with their new indirect dependencies. + // 2) As a sanity check it is also worth comparing the versions of the jars that are bundled between the two release + // zip files to spot any version differences. + // * https://github.com/pmd/pmd/archive/refs/tags/pmd_releases/7.2.0.zip + // * https://github.com/pmd/pmd/archive/refs/tags/pmd_releases/7.1.0.zip val pmd7ModulesToInclude = setOf( // LANGUAGE MODULE DEPENDENCIES (direct and indirect) "pmd-apex", "Saxon-HE", "annotations", "antlr4-runtime", "apex-parser", "apexlink", "asm", "checker-compat-qual", "checker-qual", "commons-lang3", "error_prone_annotations", "failureaccess", "flogger", "flogger-system-backend", "geny_2.13", "gson", "gson-extras", "guava", "j2objc-annotations", "jsr250-api", "jsr305", "jul-to-slf4j", "kotlin-stdlib", "kotlin-stdlib-common", "kotlin-stdlib-jdk7", "kotlin-stdlib-jdk8", "listenablefuture", "nice-xml-messages", "pcollections", "pkgforce_2.13", "pmd-core", "runforce", "scala-collection-compat_2.13", "scala-json-rpc-upickle-json-serializer_2.13", "scala-json-rpc_2.13", "scala-library", "scala-parallel-collections_2.13", "scala-reflect", "scala-xml_2.13", "slf4j-api", "summit-ast", "ujson_2.13", "upack_2.13", "upickle-core_2.13", "upickle-implicits_2.13", "upickle_2.13", "xmlresolver", @@ -91,7 +105,7 @@ tasks.register("installPmd7") { "pmd-visualforce", "Saxon-HE", "antlr4-runtime", "apex-parser", "apexlink", "asm", "checker-compat-qual", "checker-qual", "commons-lang3", "error_prone_annotations", "failureaccess", "flogger", "flogger-system-backend", "geny_2.13", "gson", "gson-extras", "guava", "j2objc-annotations", "jsr250-api", "jsr305", "jul-to-slf4j", "kotlin-stdlib", "kotlin-stdlib-common", "kotlin-stdlib-jdk7", "kotlin-stdlib-jdk8", "listenablefuture", "nice-xml-messages", "pcollections", "pkgforce_2.13", "pmd-apex", "pmd-core", "runforce", "scala-collection-compat_2.13", "scala-json-rpc-upickle-json-serializer_2.13", "scala-json-rpc_2.13", "scala-library", "scala-parallel-collections_2.13", "scala-reflect", "scala-xml_2.13", "slf4j-api", "summit-ast", "ujson_2.13", "upack_2.13", "upickle-core_2.13", "upickle-implicits_2.13", "upickle_2.13", "xmlresolver", "pmd-xml", "Saxon-HE", "antlr4-runtime", "asm", "checker-qual", "commons-lang3", "gson", "jul-to-slf4j", "nice-xml-messages", "pcollections", "pmd-core", "slf4j-api", "xmlresolver", // MAIN CLI MODULE DEPENDENCIES (direct and indirect) - "pmd-cli", "Saxon-HE", "antlr4-runtime", "asm", "checker-qual", "commons-lang3", "gson", "jline", "jul-to-slf4j", "nice-xml-messages", "pcollections", "picocli", "pmd-core", "progressbar", "slf4j-api", "slf4j-simple", "xmlresolver", + "pmd-cli", "Saxon-HE", "antlr4-runtime", "asm", "checker-qual", "commons-lang3", "gson", "jline", "jul-to-slf4j", "nice-xml-messages", "pcollections", "picocli", "pmd-core", "progressbar", "slf4j-api", "slf4j-simple", "xmlresolver", // Do not include pmd-designer since we don't use it ) val pmd7JarsToIncludeRegexes = mutableSetOf("""^LICENSE""".toRegex()) pmd7ModulesToInclude.forEach { diff --git a/src/Constants.ts b/src/Constants.ts index 1901ce6cc..fc2c7ca93 100644 --- a/src/Constants.ts +++ b/src/Constants.ts @@ -1,7 +1,7 @@ import os = require('os'); import path = require('path'); -export const PMD7_VERSION = '7.1.0'; +export const PMD7_VERSION = '7.2.0'; export const PMD_APPEXCHANGE_RULES_VERSION = '0.12'; export const SFGE_VERSION = '1.0.1-pilot'; export const DEFAULT_SCANNER_PATH = path.join(os.homedir(), '.sfdx-scanner'); diff --git a/src/lib/pmd/PmdCommandInfo.ts b/src/lib/pmd/PmdCommandInfo.ts index af73b8104..7133877b4 100644 --- a/src/lib/pmd/PmdCommandInfo.ts +++ b/src/lib/pmd/PmdCommandInfo.ts @@ -2,7 +2,6 @@ import {PMD7_LIB, PMD7_VERSION} from "../../Constants"; import * as path from 'path'; const PMD7_CLI_CLASS = 'net.sourceforge.pmd.cli.PmdCli'; -const HEAP_SIZE = '-Xmx1024m'; export interface PmdCommandInfo { getVersion(): string @@ -25,7 +24,7 @@ export class Pmd7CommandInfo implements PmdCommandInfo { constructJavaCommandArgsForPmd(fileList: string, classPathsForExternalRules: string[], rulesets: string): string[] { const classpath = classPathsForExternalRules.concat([`${PMD7_LIB}/*`]).join(path.delimiter); - const args = ['-cp', classpath, HEAP_SIZE, PMD7_CLI_CLASS, 'check', '--file-list', fileList, + const args = ['-cp', classpath, PMD7_CLI_CLASS, 'check', '--file-list', fileList, '--format', 'xml']; if (rulesets.length > 0) { args.push('--rulesets', rulesets); @@ -35,7 +34,7 @@ export class Pmd7CommandInfo implements PmdCommandInfo { constructJavaCommandArgsForCpd(fileList: string, minimumTokens: number, language: string): string[] { const classpath = `${PMD7_LIB}/*`; - return ['-cp', classpath, HEAP_SIZE, PMD7_CLI_CLASS, 'cpd', '--file-list', fileList, '--format', 'xml', + return ['-cp', classpath, PMD7_CLI_CLASS, 'cpd', '--file-list', fileList, '--format', 'xml', '--minimum-tokens', minimumTokens.toString(), '--language', language, '--skip-lexical-errors']; } } From a129b7f99d65bc0ef9672773ba31ed2888d17eee Mon Sep 17 00:00:00 2001 From: Stephen Carter <123964848+stephen-carter-at-sf@users.noreply.github.com> Date: Fri, 14 Jun 2024 12:45:01 -0400 Subject: [PATCH 4/7] CHANGE (pmd-appexchange): @W-15995965@: Update pmd-appexchange rules to v0.13 (#1506) --- pmd-appexchange/docs/AvoidApiSessionId.md | 4 +- .../docs/AvoidApiSessionIdInXML.md | 2 +- .../docs/AvoidAuraWithLockerDisabled.md | 18 ------- ...ingSystemResetPasswordWithEmailTemplate.md | 18 ------- pmd-appexchange/docs/AvoidChangeProtection.md | 18 ------- .../docs/AvoidChangeProtectionUnprotected.md | 2 +- .../docs/AvoidCreateElementScriptLinkTag.md | 26 ++++++++++ .../docs/AvoidGetInstanceWithTaint.md | 20 ++++++++ .../AvoidGlobalInstallUninstallHandlers.md | 7 ++- .../docs/AvoidHardcodedCredentials.md | 47 ------------------ .../AvoidHardcodedCredentialsInFieldDecls.md | 19 +++++++ .../AvoidHardcodedCredentialsInHttpHeader.md | 17 +++++++ .../AvoidHardcodedCredentialsInVarAssign.md | 23 +++++++++ .../AvoidHardcodedCredentialsInVarDecls.md | 19 +++++++ .../AvoidInvalidCrudContentDistribution.md | 33 ++++++++++++ .../AvoidJavaScriptCustomObjectPackage.md | 18 ------- .../docs/AvoidJavaScriptCustomRule.md | 18 ------- .../docs/AvoidJavaScriptHomePageComponent.md | 3 +- .../docs/AvoidJsLinksInCustomObject.md | 2 +- .../AvoidJsLinksInCustomObjectInternalOnly.md | 20 -------- .../docs/AvoidJsLinksInWebLinks.md | 4 +- pmd-appexchange/docs/AvoidLmcIsExposedTrue.md | 5 +- .../AvoidSecurityEnforcedOldApiVersion.md | 18 +++++++ .../docs/AvoidSystemModeInFlows.md | 3 +- .../AvoidUnauthorizedApiSessionIdInApex.md | 6 +-- .../AvoidUnauthorizedApiSessionIdInFlows.md | 2 +- ...voidUnauthorizedApiSessionIdVisualforce.md | 2 +- .../AvoidUnauthorizedGetSessionIdInApex.md | 4 +- ...idUnauthorizedGetSessionIdInVisualforce.md | 2 +- ...md => AvoidUnsafePasswordManagementUse.md} | 4 +- .../docs/AvoidUnsafeSystemMovePassword.md | 18 ------- .../docs/AvoidUnsafeSystemResetPassword.md | 18 ------- .../AvoidWithoutSharingInRestApiController.md | 18 ------- ...thoutSharingInRestApiControllerInternal.md | 18 ------- .../docs/InstallUninstallHandlerInternal.md | 18 ------- .../docs/LimitConnectedAppScope.md | 4 +- .../docs/LimitPermissionSetAssignment.md | 18 ------- .../LimitPermissionSetAssignmentInternal.md | 18 ------- pmd-appexchange/docs/LoadCSSLinkHref.md | 2 +- pmd-appexchange/docs/ProtectSensitiveData.md | 37 +------------- .../ProtectSensitiveDataPackageInternal.md | 45 ----------------- ...ProtectSensitiveDataPackageInternalOnly.md | 46 ----------------- pmd-appexchange/docs/UseHttpsCallbackUrl.md | 3 +- pmd-appexchange/docs/UseLwcDomManual.md | 6 +-- .../ValidateCrudFlsEmailMessageWhoIdWhatId.md | 18 ------- ...eCrudFlsEmailMessageWhoIdWhatIdInternal.md | 18 ------- .../lib/pmd-customrules-utils-0.0.1.jar | Bin 0 -> 4112 bytes pmd-appexchange/lib/pmd-xml-sf-0.0.1.jar | Bin 3468 -> 3468 bytes pmd-appexchange/lib/sfca-pmd-apex-0.12.jar | Bin 8789 -> 0 bytes pmd-appexchange/lib/sfca-pmd-apex-0.13.jar | Bin 0 -> 35725 bytes ...d-html-0.12.jar => sfca-pmd-html-0.13.jar} | Bin 1910 -> 1910 bytes ...-0.12.jar => sfca-pmd-javascript-0.13.jar} | Bin 1941 -> 1941 bytes .../lib/sfca-pmd-sfmetadata-0.12.jar | Bin 6129 -> 0 bytes .../lib/sfca-pmd-sfmetadata-0.13.jar | Bin 0 -> 3539 bytes .../lib/sfca-pmd-visualforce-0.12.jar | Bin 13350 -> 0 bytes .../lib/sfca-pmd-visualforce-0.13.jar | Bin 0 -> 15327 bytes pmd-appexchange/lib/sfca-pmd-xml-0.12.jar | Bin 8268 -> 0 bytes pmd-appexchange/lib/sfca-pmd-xml-0.13.jar | Bin 0 -> 6772 bytes src/Constants.ts | 2 +- test/commands/scanner/run.filters.test.ts | 2 +- 60 files changed, 209 insertions(+), 484 deletions(-) delete mode 100644 pmd-appexchange/docs/AvoidAuraWithLockerDisabled.md delete mode 100644 pmd-appexchange/docs/AvoidCallingSystemResetPasswordWithEmailTemplate.md delete mode 100644 pmd-appexchange/docs/AvoidChangeProtection.md create mode 100644 pmd-appexchange/docs/AvoidCreateElementScriptLinkTag.md create mode 100644 pmd-appexchange/docs/AvoidGetInstanceWithTaint.md delete mode 100644 pmd-appexchange/docs/AvoidHardcodedCredentials.md create mode 100644 pmd-appexchange/docs/AvoidHardcodedCredentialsInFieldDecls.md create mode 100644 pmd-appexchange/docs/AvoidHardcodedCredentialsInHttpHeader.md create mode 100644 pmd-appexchange/docs/AvoidHardcodedCredentialsInVarAssign.md create mode 100644 pmd-appexchange/docs/AvoidHardcodedCredentialsInVarDecls.md create mode 100644 pmd-appexchange/docs/AvoidInvalidCrudContentDistribution.md delete mode 100644 pmd-appexchange/docs/AvoidJavaScriptCustomObjectPackage.md delete mode 100644 pmd-appexchange/docs/AvoidJavaScriptCustomRule.md delete mode 100644 pmd-appexchange/docs/AvoidJsLinksInCustomObjectInternalOnly.md create mode 100644 pmd-appexchange/docs/AvoidSecurityEnforcedOldApiVersion.md rename pmd-appexchange/docs/{AvoidUnsafeSystemSetPassword.md => AvoidUnsafePasswordManagementUse.md} (66%) delete mode 100644 pmd-appexchange/docs/AvoidUnsafeSystemMovePassword.md delete mode 100644 pmd-appexchange/docs/AvoidUnsafeSystemResetPassword.md delete mode 100644 pmd-appexchange/docs/AvoidWithoutSharingInRestApiController.md delete mode 100644 pmd-appexchange/docs/AvoidWithoutSharingInRestApiControllerInternal.md delete mode 100644 pmd-appexchange/docs/InstallUninstallHandlerInternal.md delete mode 100644 pmd-appexchange/docs/LimitPermissionSetAssignment.md delete mode 100644 pmd-appexchange/docs/LimitPermissionSetAssignmentInternal.md delete mode 100644 pmd-appexchange/docs/ProtectSensitiveDataPackageInternal.md delete mode 100644 pmd-appexchange/docs/ProtectSensitiveDataPackageInternalOnly.md delete mode 100644 pmd-appexchange/docs/ValidateCrudFlsEmailMessageWhoIdWhatId.md delete mode 100644 pmd-appexchange/docs/ValidateCrudFlsEmailMessageWhoIdWhatIdInternal.md create mode 100644 pmd-appexchange/lib/pmd-customrules-utils-0.0.1.jar delete mode 100644 pmd-appexchange/lib/sfca-pmd-apex-0.12.jar create mode 100644 pmd-appexchange/lib/sfca-pmd-apex-0.13.jar rename pmd-appexchange/lib/{sfca-pmd-html-0.12.jar => sfca-pmd-html-0.13.jar} (53%) rename pmd-appexchange/lib/{sfca-pmd-javascript-0.12.jar => sfca-pmd-javascript-0.13.jar} (53%) delete mode 100644 pmd-appexchange/lib/sfca-pmd-sfmetadata-0.12.jar create mode 100644 pmd-appexchange/lib/sfca-pmd-sfmetadata-0.13.jar delete mode 100644 pmd-appexchange/lib/sfca-pmd-visualforce-0.12.jar create mode 100644 pmd-appexchange/lib/sfca-pmd-visualforce-0.13.jar delete mode 100644 pmd-appexchange/lib/sfca-pmd-xml-0.12.jar create mode 100644 pmd-appexchange/lib/sfca-pmd-xml-0.13.jar diff --git a/pmd-appexchange/docs/AvoidApiSessionId.md b/pmd-appexchange/docs/AvoidApiSessionId.md index 0f4a66bd4..97a2490a5 100644 --- a/pmd-appexchange/docs/AvoidApiSessionId.md +++ b/pmd-appexchange/docs/AvoidApiSessionId.md @@ -3,14 +3,14 @@ AvoidApiSessionId[](#avoidapisessionid) **Violation:** - Session ID use is not approved. + Session ID use may not be approved. **Priority:** High (2) **Description:** - Detects use of Api.Session_ID to retrieve a session ID. + Detects use of Api.Session_ID to retrieve a session ID. For more guidance on approved use cases, read the [Session Id Guidance][https://partners.salesforce.com/sfc/servlet.shepherd/version/download/0684V00000O83jT?asPdf=false&operationContext=CHATTER] document. **Example(s):** diff --git a/pmd-appexchange/docs/AvoidApiSessionIdInXML.md b/pmd-appexchange/docs/AvoidApiSessionIdInXML.md index dec547b97..a8b810832 100644 --- a/pmd-appexchange/docs/AvoidApiSessionIdInXML.md +++ b/pmd-appexchange/docs/AvoidApiSessionIdInXML.md @@ -10,7 +10,7 @@ AvoidApiSessionIdInXML[](#avoidapisessionidinxml) **Description:** - Detects use of Api.Session_ID to retrieve a session ID. + Detects use of Api.Session_ID to retrieve a session ID. For more guidance on approved use cases, read the [Session Id Guidance][https://partners.salesforce.com/sfc/servlet.shepherd/version/download/0684V00000O83jT?asPdf=false&operationContext=CHATTER] document. **Example(s):** diff --git a/pmd-appexchange/docs/AvoidAuraWithLockerDisabled.md b/pmd-appexchange/docs/AvoidAuraWithLockerDisabled.md deleted file mode 100644 index 487d9fd31..000000000 --- a/pmd-appexchange/docs/AvoidAuraWithLockerDisabled.md +++ /dev/null @@ -1,18 +0,0 @@ -AvoidAuraWithLockerDisabled[](#avoidaurawithlockerdisabled) ------------------------------------------------------------------------------------------------------------------------------------------------------- - -**Violation:** - - To enable Lightning Locker, update the apiVersion to version 40 or greater. - - -**Priority:** Critical (1) - -**Description:** - - Detects use of API versions with Lightning Locker disabled in Aura components. Use API version 40 or greater. - -**Example(s):** - - - diff --git a/pmd-appexchange/docs/AvoidCallingSystemResetPasswordWithEmailTemplate.md b/pmd-appexchange/docs/AvoidCallingSystemResetPasswordWithEmailTemplate.md deleted file mode 100644 index d522f0b12..000000000 --- a/pmd-appexchange/docs/AvoidCallingSystemResetPasswordWithEmailTemplate.md +++ /dev/null @@ -1,18 +0,0 @@ -AvoidCallingSystemResetPasswordWithEmailTemplate[](#avoidcallingsystemresetpasswordwithemailtemplate) ------------------------------------------------------------------------------------------------------------------------------------------------------- - -**Violation:** - - Before calling System.resetPasswordWithEmailTemplate(), perform the necessary authorization checks. - - -**Priority:** Critical (1) - -**Description:** - - Detects where System.resetPasswordWithEmailTemplate() exists in Apex code. Use this method with caution. - -**Example(s):** - - - diff --git a/pmd-appexchange/docs/AvoidChangeProtection.md b/pmd-appexchange/docs/AvoidChangeProtection.md deleted file mode 100644 index ea3644f55..000000000 --- a/pmd-appexchange/docs/AvoidChangeProtection.md +++ /dev/null @@ -1,18 +0,0 @@ -AvoidChangeProtection[](#avoidchangeprotection) ------------------------------------------------------------------------------------------------------------------------------------------------------- - -**Violation:** - - Update your code to avoid using FeatureManagement.changeProtection. - - -**Priority:** High (2) - -**Description:** - - Detects potential misuse of FeatureManagement.changeProtection. - -**Example(s):** - - - diff --git a/pmd-appexchange/docs/AvoidChangeProtectionUnprotected.md b/pmd-appexchange/docs/AvoidChangeProtectionUnprotected.md index 3abed876f..c66a30f3d 100644 --- a/pmd-appexchange/docs/AvoidChangeProtectionUnprotected.md +++ b/pmd-appexchange/docs/AvoidChangeProtectionUnprotected.md @@ -3,7 +3,7 @@ AvoidChangeProtectionUnprotected[](#avoidchangeprotectionunprotected) **Violation:** - Update your code to avoid using FeatureManagement.changeProtection called by an UnProtected argument. + Ensure appropriate authorization checks are in-place before invoking FeatureManagement.changeProtection called with 'UnProtected' argument. **Priority:** Critical (1) diff --git a/pmd-appexchange/docs/AvoidCreateElementScriptLinkTag.md b/pmd-appexchange/docs/AvoidCreateElementScriptLinkTag.md new file mode 100644 index 000000000..38dd0388f --- /dev/null +++ b/pmd-appexchange/docs/AvoidCreateElementScriptLinkTag.md @@ -0,0 +1,26 @@ +AvoidCreateElementScriptLinkTag[](#avoidcreateelementscriptlinktag) +------------------------------------------------------------------------------------------------------------------------------------------------------ + +**Violation:** + + Load JavaScript/CSS only from static resources. + + +**Priority:** High (2) + +**Description:** + + Detects dynamic creation of script or link tags + +**Example(s):** + + + +``` +