diff --git a/cli-messaging/build.gradle.kts b/cli-messaging/build.gradle.kts new file mode 100644 index 000000000..54d198e8a --- /dev/null +++ b/cli-messaging/build.gradle.kts @@ -0,0 +1,26 @@ +plugins { + java +} + +version = "1.0" +java.sourceCompatibility = JavaVersion.VERSION_1_8 +group = "com.salesforce.messaging" + +repositories { + mavenCentral() +} + +dependencies { + implementation ("com.googlecode.json-simple:json-simple:1.1.1") { + exclude("junit") + } + implementation("com.google.code.gson:gson:2.3") + testImplementation("junit", "junit", "4.12") + implementation("com.google.guava:guava:28.0-jre") + testImplementation("org.junit.jupiter:junit-jupiter-api:5.6.0") + testRuntimeOnly("org.junit.jupiter:junit-jupiter-engine") +} + +tasks.getByName("test") { + useJUnitPlatform() +} diff --git a/cli-messaging/gradle/wrapper/gradle-wrapper.jar b/cli-messaging/gradle/wrapper/gradle-wrapper.jar new file mode 100644 index 000000000..87b738cbd Binary files /dev/null and b/cli-messaging/gradle/wrapper/gradle-wrapper.jar differ diff --git a/cli-messaging/gradle/wrapper/gradle-wrapper.properties b/cli-messaging/gradle/wrapper/gradle-wrapper.properties new file mode 100644 index 000000000..4a6ebceac --- /dev/null +++ b/cli-messaging/gradle/wrapper/gradle-wrapper.properties @@ -0,0 +1,5 @@ +distributionBase=GRADLE_USER_HOME +distributionPath=wrapper/dists +distributionUrl=https\://services.gradle.org/distributions/gradle-6.2.1-bin.zip +zipStoreBase=GRADLE_USER_HOME +zipStorePath=wrapper/dists diff --git a/cli-messaging/settings.gradle.kts b/cli-messaging/settings.gradle.kts new file mode 100644 index 000000000..8b0aa7943 --- /dev/null +++ b/cli-messaging/settings.gradle.kts @@ -0,0 +1 @@ +rootProject.name = "cli-messaging" diff --git a/pmd-cataloger/src/main/java/sfdc/sfdx/scanner/messaging/SfdxMessager.java b/cli-messaging/src/main/java/com/salesforce/messaging/CliMessager.java similarity index 78% rename from pmd-cataloger/src/main/java/sfdc/sfdx/scanner/messaging/SfdxMessager.java rename to cli-messaging/src/main/java/com/salesforce/messaging/CliMessager.java index a080ff73d..bf0a0a34f 100644 --- a/pmd-cataloger/src/main/java/sfdc/sfdx/scanner/messaging/SfdxMessager.java +++ b/cli-messaging/src/main/java/com/salesforce/messaging/CliMessager.java @@ -1,12 +1,12 @@ -package sfdc.sfdx.scanner.messaging; - -import com.google.gson.Gson; +package com.salesforce.messaging; import java.util.ArrayList; import java.util.Arrays; import java.util.List; -public class SfdxMessager { +import com.google.gson.Gson; + +public class CliMessager { // The START string gives us something to scan for when we're processing output. private static final String START = "SFDX-START"; // The END string lets us know when a message stops, which should prevent bugs involving multi-line output. @@ -14,13 +14,8 @@ public class SfdxMessager { private static final List MESSAGES = new ArrayList<>(); - private static SfdxMessager INSTANCE = null; - - public static SfdxMessager getInstance() { - if (INSTANCE == null) { - INSTANCE = new SfdxMessager(); - } - return INSTANCE; + public static CliMessager getInstance() { + return LazyHolder.INSTANCE; } /** @@ -31,7 +26,7 @@ public static SfdxMessager getInstance() { * * @param exception to send to Typescript layer */ - public void addMessage(SfdxScannerException exception) { + public void addMessage(MessagePassableException exception) { final EventKey eventKey = exception.getEventKey(); addMessage( exception.getFullStacktrace(), @@ -65,7 +60,12 @@ public void addMessage(String internalLog, EventKey eventKey, String... args) { MESSAGES.add(message); } - + /** + * Convert all messages stored by the instance into a JSON-formatted string, enclosed in the start and end strings. + * Java code can use this method to log the messages to console, and TypeScript code can seek the start and stop + * strings to get an array of messages that can be deserialized. + * @return + */ public String getAllMessagesWithFormatting() { final String messagesAsJson = getMessagesAsJson(); return START + messagesAsJson + END; @@ -75,7 +75,6 @@ private String getMessagesAsJson() { return new Gson().toJson(MESSAGES); } - /** * TO BE USED ONLY BY TESTS! * @@ -93,15 +92,8 @@ public void resetMessages() { MESSAGES.clear(); } - enum MessageHandler { - UX, - INTERNAL - } - - enum MessageType { - INFO, - WARNING, - ERROR + private static final class LazyHolder { + // Postpone initialization until first use + private static final CliMessager INSTANCE = new CliMessager(); } } - diff --git a/pmd-cataloger/src/main/java/sfdc/sfdx/scanner/messaging/EventKey.java b/cli-messaging/src/main/java/com/salesforce/messaging/EventKey.java similarity index 78% rename from pmd-cataloger/src/main/java/sfdc/sfdx/scanner/messaging/EventKey.java rename to cli-messaging/src/main/java/com/salesforce/messaging/EventKey.java index c29da9408..a09f07269 100644 --- a/pmd-cataloger/src/main/java/sfdc/sfdx/scanner/messaging/EventKey.java +++ b/cli-messaging/src/main/java/com/salesforce/messaging/EventKey.java @@ -1,8 +1,8 @@ -package sfdc.sfdx.scanner.messaging; -import static sfdc.sfdx.scanner.messaging.SfdxMessager.*; +package com.salesforce.messaging; +import static com.salesforce.messaging.Message.*; public enum EventKey { - // MAKE SURE messageKey OF EVERY VALUE ADDED HERE HAS AN ENTRY IN 'messages/EventKeyTemplates.js'! + // MAKE SURE `messageKey` OF EVERY VALUE ADDED HERE HAS AN ENTRY IN 'messages/EventKeyTemplates.js'! INFO_GENERAL_INTERNAL_LOG("info.generalInternalLog", 1, MessageType.INFO, MessageHandler.INTERNAL, true), WARNING_INVALID_CAT_SKIPPED("warning.invalidCategorySkipped", 1, MessageType.WARNING, MessageHandler.UX, true), WARNING_INVALID_RULESET_SKIPPED("warning.invalidRulesetSkipped", 1, MessageType.WARNING, MessageHandler.UX, true), @@ -19,13 +19,15 @@ public enum EventKey { ERROR_EXTERNAL_MULTIPLE_RULE_DESC("error.external.multipleRuleDesc", 2, MessageType.ERROR, MessageHandler.UX, false), ERROR_EXTERNAL_RECURSION_LIMIT("error.external.recursionLimitReached", 2, MessageType.ERROR, MessageHandler.UX, false), ERROR_EXTERNAL_XML_NOT_READABLE("error.external.xmlNotReadable", 2, MessageType.ERROR, MessageHandler.UX, false), - ERROR_EXTERNAL_XML_NOT_PARSABLE("error.external.xmlNotParsable", 2, MessageType.ERROR, MessageHandler.UX, false); - - String messageKey; - int argCount; - MessageType messageType; - MessageHandler messageHandler; - boolean verbose;//true: only when verbose is true, false: ignores verbose flag and always prints + ERROR_EXTERNAL_XML_NOT_PARSABLE("error.external.xmlNotParsable", 2, MessageType.ERROR, MessageHandler.UX, false), + WARNING_MULTIPLE_METHOD_TARGET_MATCHES("warning.multipleMethodTargetMatches", 3, MessageType.WARNING, MessageHandler.UX, false), + WARNING_NO_METHOD_TARGET_MATCHES("warning.noMethodTargetMatches", 2, MessageType.WARNING, MessageHandler.UX, false); + + final String messageKey; + final int argCount; + final MessageType messageType; + final MessageHandler messageHandler; + final boolean verbose;//true: only when verbose is true, false: ignores verbose flag and always prints EventKey(String messageKey, int argCount, MessageType messageType, MessageHandler messageHandler, boolean verbose) { this.messageKey = messageKey; diff --git a/pmd-cataloger/src/main/java/sfdc/sfdx/scanner/messaging/Message.java b/cli-messaging/src/main/java/com/salesforce/messaging/Message.java similarity index 61% rename from pmd-cataloger/src/main/java/sfdc/sfdx/scanner/messaging/Message.java rename to cli-messaging/src/main/java/com/salesforce/messaging/Message.java index 3694429e9..20ba93ae0 100644 --- a/pmd-cataloger/src/main/java/sfdc/sfdx/scanner/messaging/Message.java +++ b/cli-messaging/src/main/java/com/salesforce/messaging/Message.java @@ -1,18 +1,16 @@ -package sfdc.sfdx.scanner.messaging; +package com.salesforce.messaging; -import com.google.gson.Gson; -import static sfdc.sfdx.scanner.messaging.SfdxMessager.*; import java.time.Instant; import java.util.List; public class Message { - private String messageKey; - private List args; - private String internalLog; - private MessageType type; - private MessageHandler handler; - private boolean verbose; - private long time; + final private String messageKey; + final private List args; + final private String internalLog; + final private MessageType type; + final private MessageHandler handler; + final private boolean verbose; + final private long time; Message(String messageKey, List args, String internalLog, MessageType type, MessageHandler handler, boolean verbose) { this.messageKey = messageKey; @@ -20,12 +18,8 @@ public class Message { this.internalLog = internalLog; this.type = type; this.handler = handler; - this.time = Instant.now().toEpochMilli(); this.verbose = verbose; - } - - String toJson() { - return new Gson().toJson(this); + this.time = Instant.now().toEpochMilli(); } public String getMessageKey() { @@ -40,4 +34,14 @@ public String getInternalLog() { return internalLog; } + enum MessageHandler { + UX, + INTERNAL + } + + enum MessageType { + INFO, + WARNING, + ERROR + } } diff --git a/pmd-cataloger/src/main/java/sfdc/sfdx/scanner/messaging/SfdxScannerException.java b/cli-messaging/src/main/java/com/salesforce/messaging/MessagePassableException.java similarity index 64% rename from pmd-cataloger/src/main/java/sfdc/sfdx/scanner/messaging/SfdxScannerException.java rename to cli-messaging/src/main/java/com/salesforce/messaging/MessagePassableException.java index 688561a0b..bba99811e 100644 --- a/pmd-cataloger/src/main/java/sfdc/sfdx/scanner/messaging/SfdxScannerException.java +++ b/cli-messaging/src/main/java/com/salesforce/messaging/MessagePassableException.java @@ -1,25 +1,24 @@ -package sfdc.sfdx.scanner.messaging; +package com.salesforce.messaging; import com.google.common.base.Throwables; -import sfdc.sfdx.scanner.messaging.EventKey; import java.util.Arrays; /** * Internal exception representation. * Extends RuntimeException to avoid declaring everywhere - * Handles capability to plug into SfdxMessager + * Handles capability to plug into CliMessager */ -public class SfdxScannerException extends RuntimeException { +public class MessagePassableException extends RuntimeException { private final EventKey eventKey; private final String[] args; - public SfdxScannerException(EventKey eventKey, String... args) { + public MessagePassableException(EventKey eventKey, String... args) { this(eventKey, null, args); } - public SfdxScannerException(EventKey eventKey, Throwable throwable, String... args) { + public MessagePassableException(EventKey eventKey, Throwable throwable, String... args) { super(throwable); this.eventKey = eventKey; @@ -40,7 +39,7 @@ public String getFullStacktrace() { @Override public String toString() { - return "SfdxScannerException{" + + return "MessagePassableException{" + "eventKey=" + eventKey + ", args=" + Arrays.toString(args) + '}'; diff --git a/pmd-cataloger/src/test/java/sfdc/sfdx/scanner/messaging/EventKeyTest.java b/cli-messaging/src/test/java/com/salesforce/messaging/EventKeyTest.java similarity index 98% rename from pmd-cataloger/src/test/java/sfdc/sfdx/scanner/messaging/EventKeyTest.java rename to cli-messaging/src/test/java/com/salesforce/messaging/EventKeyTest.java index b9222fe65..85acf00fa 100644 --- a/pmd-cataloger/src/test/java/sfdc/sfdx/scanner/messaging/EventKeyTest.java +++ b/cli-messaging/src/test/java/com/salesforce/messaging/EventKeyTest.java @@ -1,4 +1,4 @@ -package sfdc.sfdx.scanner.messaging; +package com.salesforce.messaging; import org.json.simple.JSONObject; import org.json.simple.parser.JSONParser; @@ -12,7 +12,7 @@ import org.junit.runners.Parameterized; import org.junit.runners.Parameterized.Parameters; -import static sfdc.sfdx.scanner.messaging.SfdxMessager.*; +import static com.salesforce.messaging.Message.*; import java.io.IOException; import java.nio.file.Files; diff --git a/messages/EventKeyTemplates.js b/messages/EventKeyTemplates.js index 8448344f8..a5d0f3a48 100644 --- a/messages/EventKeyTemplates.js +++ b/messages/EventKeyTemplates.js @@ -18,6 +18,8 @@ module.exports = { "pmdSkippedFile": "PMD failed to evaluate against file '%s'. Message: %s", "pmdSuppressedViolation": "PMD suppressed violation against file '%s'. Message: %s. Suppression Type: %s. User Message: %s", "unexpectedPmdNodeType": "Encountered unexpected PMD node of type '%s'", + "multipleMethodTargetMatches": "Total of %s methods in file %s matched name #%s", + "noMethodTargetMatches": "No methods in file %s matched name #%s()", "pmdConfigError": "PMD failed to evaluate rule '%s'. Message: %s" }, "error": { diff --git a/messages/run-dfa.js b/messages/run-dfa.js index ef4482433..fc6b70c64 100644 --- a/messages/run-dfa.js +++ b/messages/run-dfa.js @@ -21,9 +21,11 @@ module.exports = { "sevthresholdDescription": "throws an error when violations of specific severity (or more severe) are detected, invokes --normalize-severity", "sevthresholdDescriptionLong": "Throws an error if violations are found with equal or greater severity than provided value. Values are 1 (high), 2 (moderate), and 3 (low). Exit code is the most severe violation. Using this flag also invokes the --normalize-severity flag", "targetDescription": "location of source code", - "targetDescriptionLong": "Source code location. May use glob patterns. Multiple values can be specified as a comma-separated list" + "targetDescriptionLong": "Source code location. May use glob patterns, or specify individual methods with #-syntax. Multiple values can be specified as a comma-separated list" }, "validations": { + "methodLevelTargetCannotBeGlob": "Method-level targets supplied to --target cannot be globs", + "methodLevelTargetMustBeRealFile": "Method-level target %s must be a real file", "projectdirCannotBeGlob": "--projectdir cannot specify globs", "projectdirMustBeDir": "--projectdir must specify directories", "projectdirMustExist": "--projectdir must specify existing paths" @@ -37,6 +39,9 @@ module.exports = { Unix example: $ sfdx scanner:run:dfa --target './**/*.cls,!./**/IgnoreMe.cls' ... Windows example: > sfdx scanner:run:dfa --target ".\\**\\*.cls,!.\\**\\IgnoreMe.cls" ... Evaluate rules against all .cls files below the current directory, except for IgnoreMe.cls. + Individual methods within a file may be targeted by suffixing the file's path with a hash (#), and a semi-colon-delimited list of method names. This syntax is incompatible with globs and directories. + E.g., $ sfdx scanner:run:dfa --target "./File1.cls#Method1;Method2,./File2.cls#Method3" ... + Evaluates rules against ALL methods named Method1 or Method2 in File1.cls, and ALL methods named Method3 in File2.cls. Use --normalize-severity to output a normalized (across all engines) severity (1 [high], 2 [moderate], and 3 [low]) in addition to the engine specific severity (when shown). E.g., $ sfdx scanner:run:dfa --target "/some-project/" --projectdir "/some-project/" --format csv --normalize-severity Use --severity-threshold to throw a non-zero exit code when rule violations of a specific normalized severity (or greater) are found. For this example, if there are any rule violations with a severity of 2 or more (which includes 1-high and 2-moderate), the exit code will be equal to the severity of the most severe violation. diff --git a/messages/run.js b/messages/run.js index 9ad1aa43a..5e24efce5 100644 --- a/messages/run.js +++ b/messages/run.js @@ -29,9 +29,13 @@ module.exports = { 'eslintConfigDescription': 'location of eslintrc config to customize eslint engine', 'eslintConfigDescriptionLong': 'Location of eslintrc to customize eslint engine', 'pmdConfigDescription': 'location of PMD rule reference XML file to customize rule selection', - 'pmdConfigDescriptionLong': 'Location of PMD rule reference XML file to customize rule selection' + 'pmdConfigDescriptionLong': 'Location of PMD rule reference XML file to customize rule selection', + "verboseViolationsDescription": "retire-js violation messages include more details", + "verboseViolationsDescriptionLong": "retire-js violation messages contain details about each vulnerability (e.g. summary, CVE, urls, etc.)" + }, "validations": { + "methodLevelTargetingDisallowed": "Target '%s' is invalid, as this command does not support method-level targeting", "outfileFormatMismatch": "Your chosen format %s does not appear to match your output file type of %s.", "outfileMustBeValid": "--outfile must be a well-formed filepath.", "outfileMustBeSupportedType": "--outfile must be of a supported type. Current options are: .csv; .xml; .json; .html; .sarif.", diff --git a/package.json b/package.json index 42eeb200e..756d0a15f 100644 --- a/package.json +++ b/package.json @@ -1,7 +1,7 @@ { "name": "@salesforce/sfdx-scanner", "description": "Static code scanner that applies quality and security rules to Apex code, and provides feedback.", - "version": "3.1.2", + "version": "3.2.0", "author": "ISV SWAT", "bugs": "https://github.com/forcedotcom/sfdx-scanner/issues", "dependencies": { diff --git a/pmd-cataloger/build.gradle.kts b/pmd-cataloger/build.gradle.kts index 7027ace1a..1c6a061cc 100644 --- a/pmd-cataloger/build.gradle.kts +++ b/pmd-cataloger/build.gradle.kts @@ -54,6 +54,7 @@ tasks.register("installPmd") { } dependencies { + implementation(project(":cli-messaging")) implementation ("com.googlecode.json-simple:json-simple:1.1.1") { exclude("junit") } diff --git a/pmd-cataloger/settings.gradle.kts b/pmd-cataloger/settings.gradle.kts deleted file mode 100644 index 99ffef2c9..000000000 --- a/pmd-cataloger/settings.gradle.kts +++ /dev/null @@ -1,2 +0,0 @@ -rootProject.name = "pmd-cataloger" - diff --git a/pmd-cataloger/src/main/java/sfdc/sfdx/scanner/pmd/LanguageXmlFileMapping.java b/pmd-cataloger/src/main/java/sfdc/sfdx/scanner/pmd/LanguageXmlFileMapping.java index 7fd975984..a7fa620af 100644 --- a/pmd-cataloger/src/main/java/sfdc/sfdx/scanner/pmd/LanguageXmlFileMapping.java +++ b/pmd-cataloger/src/main/java/sfdc/sfdx/scanner/pmd/LanguageXmlFileMapping.java @@ -1,8 +1,8 @@ package sfdc.sfdx.scanner.pmd; -import sfdc.sfdx.scanner.messaging.EventKey; -import sfdc.sfdx.scanner.messaging.SfdxMessager; -import sfdc.sfdx.scanner.messaging.SfdxScannerException; +import com.salesforce.messaging.EventKey; +import com.salesforce.messaging.CliMessager; +import com.salesforce.messaging.MessagePassableException; import java.util.*; @@ -57,7 +57,7 @@ private void addPathForLanguage(String path, String language, String sourceJar) addCategoryPathForLanguage(path, language, sourceJar); categoryToSourceJar.put(path, sourceJar); } else { - SfdxMessager.getInstance().addMessage("Adding path " + path + " for language " + language, EventKey.WARNING_XML_DROPPED, path); + CliMessager.getInstance().addMessage("Adding path " + path + " for language " + language, EventKey.WARNING_XML_DROPPED, path); } } } @@ -81,7 +81,7 @@ private void addPath(String path, String language, String sourceJar, Map> rulePathEntries = parseArguments(args); catalogRules(rulePathEntries); - } catch (SfdxScannerException se) { + } catch (MessagePassableException se) { // Add all SfdxScannerExceptions as messages - SfdxMessager.getInstance().addMessage(se); + CliMessager.getInstance().addMessage(se); exitGracefully = false; } catch (Throwable throwable) { // Catch and handle any exceptions that may have slipped through - final SfdxScannerException exception = new SfdxScannerException(EventKey.ERROR_INTERNAL_UNEXPECTED, throwable, throwable.getMessage()); - SfdxMessager.getInstance().addMessage(exception); + final MessagePassableException exception = new MessagePassableException(EventKey.ERROR_INTERNAL_UNEXPECTED, throwable, throwable.getMessage()); + CliMessager.getInstance().addMessage(exception); exitGracefully = false; } finally { // Print all the messages we have collected in a parsable format - System.out.println(SfdxMessager.getInstance().getAllMessagesWithFormatting()); + System.out.println(CliMessager.getInstance().getAllMessagesWithFormatting()); } return exitGracefully; @@ -71,7 +71,7 @@ private void catalogRules(Map> rulePathEntries) { Map> parseArguments(String[] args) { if (args == null || args.length < 1) { - throw new SfdxScannerException(EventKey.ERROR_INTERNAL_MAIN_INVALID_ARGUMENT, NO_ARGUMENTS_FOUND); + throw new MessagePassableException(EventKey.ERROR_INTERNAL_MAIN_INVALID_ARGUMENT, NO_ARGUMENTS_FOUND); } final Map> rulePathEntries = new HashMap<>(); @@ -87,19 +87,19 @@ private void parseArg(Map> languagePathEntries, String arg) // DIVIDER should split arg in language and path list. No less, no more if (splitArg.length != 2) { - throw new SfdxScannerException(EventKey.ERROR_INTERNAL_MAIN_INVALID_ARGUMENT, String.format(EXPECTED_DIVIDER, arg)); + throw new MessagePassableException(EventKey.ERROR_INTERNAL_MAIN_INVALID_ARGUMENT, String.format(EXPECTED_DIVIDER, arg)); } final String language = splitArg[0].trim(); final String paths = splitArg[1]; if ("".equals(language.trim()) || "".equals((paths.trim()))) { - throw new SfdxScannerException(EventKey.ERROR_INTERNAL_MAIN_INVALID_ARGUMENT, String.format(MISSING_PARTS, arg)); + throw new MessagePassableException(EventKey.ERROR_INTERNAL_MAIN_INVALID_ARGUMENT, String.format(MISSING_PARTS, arg)); } final String[] pathArray = paths.split(COMMA); if (pathArray.length < 1) { - throw new SfdxScannerException(EventKey.ERROR_INTERNAL_MAIN_INVALID_ARGUMENT, String.format(NO_PATH_PROVIDED, language, arg)); + throw new MessagePassableException(EventKey.ERROR_INTERNAL_MAIN_INVALID_ARGUMENT, String.format(NO_PATH_PROVIDED, language, arg)); } // Stream path array to filter out empty path diff --git a/pmd-cataloger/src/main/java/sfdc/sfdx/scanner/pmd/PmdRuleCataloger.java b/pmd-cataloger/src/main/java/sfdc/sfdx/scanner/pmd/PmdRuleCataloger.java index 40bcb6c8f..2b01456b1 100644 --- a/pmd-cataloger/src/main/java/sfdc/sfdx/scanner/pmd/PmdRuleCataloger.java +++ b/pmd-cataloger/src/main/java/sfdc/sfdx/scanner/pmd/PmdRuleCataloger.java @@ -8,16 +8,15 @@ import com.google.gson.Gson; import com.google.gson.GsonBuilder; import org.w3c.dom.*; -import sfdc.sfdx.scanner.messaging.Message; -import sfdc.sfdx.scanner.messaging.SfdxScannerException; -import sfdc.sfdx.scanner.messaging.SfdxMessager; +import com.salesforce.messaging.EventKey; +import com.salesforce.messaging.MessagePassableException; +import com.salesforce.messaging.CliMessager; import sfdc.sfdx.scanner.pmd.catalog.PmdCatalogCategory; import sfdc.sfdx.scanner.pmd.catalog.PmdCatalogJson; import sfdc.sfdx.scanner.pmd.catalog.PmdCatalogRule; import sfdc.sfdx.scanner.pmd.catalog.PmdCatalogRuleset; import sfdc.sfdx.scanner.xml.XmlReader; import sfdc.sfdx.scanner.paths.PathManipulator; -import sfdc.sfdx.scanner.messaging.EventKey; class PmdRuleCataloger { private Map> rulePathEntries; @@ -110,7 +109,7 @@ private void processCategoryFile(String language, String path) { // If the root node isn't of type 'ruleset', this isn't a valid category file, so we should just log something and skip it. if (!root.getTagName().equalsIgnoreCase("ruleset") || !root.getAttribute("xmlns").startsWith("http://pmd.sourceforge.net")) { String fullPath = PathManipulator.getInstance().convertResourcePathToAbsolutePath(path); - SfdxMessager.getInstance().addMessage("Processing category file for language " + language + " at path " + path, EventKey.WARNING_INVALID_CAT_SKIPPED, fullPath); + CliMessager.getInstance().addMessage("Processing category file for language " + language + " at path " + path, EventKey.WARNING_INVALID_CAT_SKIPPED, fullPath); return; } @@ -143,7 +142,7 @@ private void generateRulesetRepresentation(String language, String path) { // If the root node isn't of type 'ruleset', this isn't a valid ruleset file, so we should just log something and skip it. if (!root.getTagName().equalsIgnoreCase("ruleset") || !root.getAttribute("xmlns").startsWith("http://pmd.sourceforge.net")) { String fullPath = PathManipulator.getInstance().convertResourcePathToAbsolutePath(path); - SfdxMessager.getInstance().addMessage("Generating Ruleset representation for language " + language + " at path " + path, EventKey.WARNING_INVALID_RULESET_SKIPPED, fullPath); + CliMessager.getInstance().addMessage("Generating Ruleset representation for language " + language + " at path " + path, EventKey.WARNING_INVALID_RULESET_SKIPPED, fullPath); return; } @@ -181,17 +180,22 @@ private void linkRulesToRulesets(List rules, List findXmlFilesInPath(String pathString) { final Path path = Paths.get(pathString); - List xmlContainers = new ArrayList<>(); + List xmlContainers = new ArrayList<>(); // Make sure that the path exists to begin with if (!Files.exists(path)) { - throw new SfdxScannerException(EventKey.ERROR_INTERNAL_CLASSPATH_DOES_NOT_EXIST, path.getFileName().toString()); + throw new MessagePassableException(EventKey.ERROR_INTERNAL_CLASSPATH_DOES_NOT_EXIST, path.getFileName().toString()); } if (Files.isDirectory(path)) { @@ -124,10 +124,10 @@ public List findXmlFilesInJar(String jarPath) { } } - SfdxMessager.getInstance().addMessage("", EventKey.INFO_JAR_AND_XML_PROCESSED, jarPath, xmlFiles.toString()); + CliMessager.getInstance().addMessage("", EventKey.INFO_JAR_AND_XML_PROCESSED, jarPath, xmlFiles.toString()); } catch (Exception e) { //TODO: add logging and print stacktrace for debugging - throw new SfdxScannerException(EventKey.ERROR_EXTERNAL_JAR_NOT_READABLE, e, jarPath); + throw new MessagePassableException(EventKey.ERROR_EXTERNAL_JAR_NOT_READABLE, e, jarPath); } return xmlFiles; @@ -146,7 +146,7 @@ private List scoutForFiles(FileType fileType, Path path) { filesFound.addAll(walk.map(x -> x.toString()) .filter(f -> f.endsWith(fileType.suffix)).collect(Collectors.toList())); } catch (IOException e) { - throw new SfdxScannerException(EventKey.ERROR_EXTERNAL_DIR_NOT_READABLE, e, path.toString()); + throw new MessagePassableException(EventKey.ERROR_EXTERNAL_DIR_NOT_READABLE, e, path.toString()); } return filesFound; diff --git a/pmd-cataloger/src/main/java/sfdc/sfdx/scanner/pmd/catalog/PmdCatalogRule.java b/pmd-cataloger/src/main/java/sfdc/sfdx/scanner/pmd/catalog/PmdCatalogRule.java index a24ffc4d3..08f83f5f4 100644 --- a/pmd-cataloger/src/main/java/sfdc/sfdx/scanner/pmd/catalog/PmdCatalogRule.java +++ b/pmd-cataloger/src/main/java/sfdc/sfdx/scanner/pmd/catalog/PmdCatalogRule.java @@ -4,8 +4,8 @@ import org.w3c.dom.*; import org.json.simple.*; -import sfdc.sfdx.scanner.messaging.SfdxScannerException; -import sfdc.sfdx.scanner.messaging.EventKey; +import com.salesforce.messaging.MessagePassableException; +import com.salesforce.messaging.EventKey; import static sfdc.sfdx.scanner.pmd.catalog.PmdCatalogJson.*; @@ -80,7 +80,7 @@ private String getDescription(Element element) { default: // If there was more than one description node, then something's gone crazy wrong and we should exit as gracefully // as possible. - throw new SfdxScannerException(EventKey.ERROR_EXTERNAL_MULTIPLE_RULE_DESC, getFullName(), String.valueOf(nl.getLength())); + throw new MessagePassableException(EventKey.ERROR_EXTERNAL_MULTIPLE_RULE_DESC, getFullName(), String.valueOf(nl.getLength())); } return res; } diff --git a/pmd-cataloger/src/main/java/sfdc/sfdx/scanner/pmd/catalog/PmdCatalogRuleset.java b/pmd-cataloger/src/main/java/sfdc/sfdx/scanner/pmd/catalog/PmdCatalogRuleset.java index 34c7015d9..fb35ffbfc 100644 --- a/pmd-cataloger/src/main/java/sfdc/sfdx/scanner/pmd/catalog/PmdCatalogRuleset.java +++ b/pmd-cataloger/src/main/java/sfdc/sfdx/scanner/pmd/catalog/PmdCatalogRuleset.java @@ -2,8 +2,8 @@ import org.w3c.dom.Element; import org.w3c.dom.NodeList; -import sfdc.sfdx.scanner.messaging.SfdxScannerException; -import sfdc.sfdx.scanner.messaging.EventKey; +import com.salesforce.messaging.MessagePassableException; +import com.salesforce.messaging.EventKey; import java.util.*; @@ -98,7 +98,7 @@ private void recursivelyProcessRule(PmdCatalogRule rule, PmdCatalogRuleset calle // for circular references, we're just going to forcibly exit if we go deeper than 10 layers of recursion, which is // way more than anyone could possibly want or need. if (recursionDepth > 10) { - throw new SfdxScannerException(EventKey.ERROR_EXTERNAL_RECURSION_LIMIT, caller.getPath(), rule.getFullName()); + throw new MessagePassableException(EventKey.ERROR_EXTERNAL_RECURSION_LIMIT, caller.getPath(), rule.getFullName()); } // Depending on whether this method was invoked by another ruleset, we'll either look for references to the rule's // category or references to the ruleset that invoked this method. diff --git a/pmd-cataloger/src/main/java/sfdc/sfdx/scanner/xml/XmlReader.java b/pmd-cataloger/src/main/java/sfdc/sfdx/scanner/xml/XmlReader.java index 72594ec55..f9df581ce 100644 --- a/pmd-cataloger/src/main/java/sfdc/sfdx/scanner/xml/XmlReader.java +++ b/pmd-cataloger/src/main/java/sfdc/sfdx/scanner/xml/XmlReader.java @@ -2,8 +2,8 @@ import org.w3c.dom.Document; import org.xml.sax.SAXException; -import sfdc.sfdx.scanner.messaging.SfdxScannerException; -import sfdc.sfdx.scanner.messaging.EventKey; +import com.salesforce.messaging.MessagePassableException; +import com.salesforce.messaging.EventKey; import javax.xml.parsers.DocumentBuilder; import javax.xml.parsers.DocumentBuilderFactory; @@ -68,13 +68,13 @@ public Document getDocumentFromPath(String path) { DocumentBuilder db = dbf.newDocumentBuilder(); doc = db.parse(in); } catch (FileNotFoundException fnf) { - throw new SfdxScannerException(EventKey.ERROR_INTERNAL_CLASSPATH_DOES_NOT_EXIST, path); + throw new MessagePassableException(EventKey.ERROR_INTERNAL_CLASSPATH_DOES_NOT_EXIST, path); } catch (IOException ioe) { - throw new SfdxScannerException(EventKey.ERROR_EXTERNAL_XML_NOT_READABLE, ioe, path, ioe.getMessage()); + throw new MessagePassableException(EventKey.ERROR_EXTERNAL_XML_NOT_READABLE, ioe, path, ioe.getMessage()); } catch (ParserConfigurationException | SAXException e) { - throw new SfdxScannerException(EventKey.ERROR_EXTERNAL_XML_NOT_PARSABLE, e, path, e.getMessage()); + throw new MessagePassableException(EventKey.ERROR_EXTERNAL_XML_NOT_PARSABLE, e, path, e.getMessage()); } catch (IllegalArgumentException iae) { - throw new SfdxScannerException(EventKey.ERROR_INTERNAL_XML_MISSING_IN_CLASSPATH, iae, path); + throw new MessagePassableException(EventKey.ERROR_INTERNAL_XML_MISSING_IN_CLASSPATH, iae, path); } return doc; } diff --git a/pmd-cataloger/src/test/java/sfdc/sfdx/scanner/pmd/LanguageXmlFileMappingTest.java b/pmd-cataloger/src/test/java/sfdc/sfdx/scanner/pmd/LanguageXmlFileMappingTest.java index 889fabcc7..b7874f389 100644 --- a/pmd-cataloger/src/test/java/sfdc/sfdx/scanner/pmd/LanguageXmlFileMappingTest.java +++ b/pmd-cataloger/src/test/java/sfdc/sfdx/scanner/pmd/LanguageXmlFileMappingTest.java @@ -15,7 +15,7 @@ import org.junit.Test; import org.junit.rules.ExpectedException; -import sfdc.sfdx.scanner.messaging.EventKey; +import com.salesforce.messaging.EventKey; /** * Unit test for {@link LanguageXmlFileMapping} @@ -122,7 +122,7 @@ private void testCollision(String collidingPath) { languageXmlFileMapping.addPathsForLanguage(Arrays.asList(xmlContainer1), APEX); - thrown.expect(new SfdxScannerExceptionMatcher(EventKey.ERROR_EXTERNAL_DUPLICATE_XML_PATH, + thrown.expect(new MessagePassableExceptionMatcher(EventKey.ERROR_EXTERNAL_DUPLICATE_XML_PATH, new String[] { collidingPath, jar2, jar1 })); languageXmlFileMapping.addPathsForLanguage(Arrays.asList(xmlContainer2), APEX); diff --git a/pmd-cataloger/src/test/java/sfdc/sfdx/scanner/pmd/MainArgsHandlingTest.java b/pmd-cataloger/src/test/java/sfdc/sfdx/scanner/pmd/MainArgsHandlingTest.java index c1ea39af7..5f9dc54b8 100644 --- a/pmd-cataloger/src/test/java/sfdc/sfdx/scanner/pmd/MainArgsHandlingTest.java +++ b/pmd-cataloger/src/test/java/sfdc/sfdx/scanner/pmd/MainArgsHandlingTest.java @@ -3,8 +3,8 @@ import static org.junit.Assert.*; import org.junit.Test; -import sfdc.sfdx.scanner.messaging.SfdxScannerException; -import sfdc.sfdx.scanner.messaging.EventKey; +import com.salesforce.messaging.MessagePassableException; +import com.salesforce.messaging.EventKey; import java.util.List; import java.util.Map; @@ -72,7 +72,7 @@ private void testParseArgForErrorHandling(String[] args, String expectedArgForMe try { main.parseArguments(args); fail(failureMessage); - } catch (SfdxScannerException e) { + } catch (MessagePassableException e) { assertEquals("Unexpected eventKey on exception", EventKey.ERROR_INTERNAL_MAIN_INVALID_ARGUMENT, e.getEventKey()); assertEquals("Unexpected arg list on exception", expectedArgForMessage, e.getArgs()[0]); } diff --git a/pmd-cataloger/src/test/java/sfdc/sfdx/scanner/pmd/MainMessagesTest.java b/pmd-cataloger/src/test/java/sfdc/sfdx/scanner/pmd/MainMessagesTest.java index 2aa4c3196..f6f67fa6f 100644 --- a/pmd-cataloger/src/test/java/sfdc/sfdx/scanner/pmd/MainMessagesTest.java +++ b/pmd-cataloger/src/test/java/sfdc/sfdx/scanner/pmd/MainMessagesTest.java @@ -7,10 +7,10 @@ import org.junit.After; import org.junit.Before; import org.junit.Test; -import sfdc.sfdx.scanner.messaging.EventKey; -import sfdc.sfdx.scanner.messaging.SfdxScannerException; -import sfdc.sfdx.scanner.messaging.Message; -import sfdc.sfdx.scanner.messaging.SfdxMessager; +import com.salesforce.messaging.EventKey; +import com.salesforce.messaging.MessagePassableException; +import com.salesforce.messaging.Message; +import com.salesforce.messaging.CliMessager; import java.util.List; @@ -21,14 +21,14 @@ public class MainMessagesTest { @Before @After public void clearMessages() { - SfdxMessager.getInstance().resetMessages(); + CliMessager.getInstance().resetMessages(); } @Test public void verifySfdxScannerExceptionsToMessages() { final EventKey expectedEventKey = EventKey.ERROR_INTERNAL_UNEXPECTED; final String[] expectedArgs = {"dummy arg"}; - final SfdxScannerException exception = new SfdxScannerException(expectedEventKey, expectedArgs); + final MessagePassableException exception = new MessagePassableException(expectedEventKey, expectedArgs); // Setup mock final Main.Dependencies dependencies = setupMockToThrowException(exception); @@ -74,7 +74,7 @@ private Main.Dependencies setupMockToThrowException(Exception exception) { } private List getMessages() { - final String messagesInJson = SfdxMessager.getInstance().getAllMessages(); + final String messagesInJson = CliMessager.getInstance().getAllMessages(); assertNotNull(messagesInJson); // Deserialize JSON to verify further diff --git a/pmd-cataloger/src/test/java/sfdc/sfdx/scanner/pmd/SfdxScannerExceptionMatcher.java b/pmd-cataloger/src/test/java/sfdc/sfdx/scanner/pmd/MessagePassableExceptionMatcher.java similarity index 66% rename from pmd-cataloger/src/test/java/sfdc/sfdx/scanner/pmd/SfdxScannerExceptionMatcher.java rename to pmd-cataloger/src/test/java/sfdc/sfdx/scanner/pmd/MessagePassableExceptionMatcher.java index e0d9e9686..d5d606484 100644 --- a/pmd-cataloger/src/test/java/sfdc/sfdx/scanner/pmd/SfdxScannerExceptionMatcher.java +++ b/pmd-cataloger/src/test/java/sfdc/sfdx/scanner/pmd/MessagePassableExceptionMatcher.java @@ -6,41 +6,41 @@ import org.hamcrest.Description; import org.hamcrest.TypeSafeMatcher; -import sfdc.sfdx.scanner.messaging.EventKey; -import sfdc.sfdx.scanner.messaging.SfdxScannerException; +import com.salesforce.messaging.EventKey; +import com.salesforce.messaging.MessagePassableException; /** * Custom matcher that can be used with * {@link org.junit.rules.ExpectedException#expect(org.hamcrest.Matcher)} - * + * * 
  * // Example Usage
- * thrown.expect(new SfdxScannerExceptionMatcher(EventKey.WARNING_INVALID_CAT_SKIPPED,
+ * thrown.expect(new MessagePassableExceptionMatcher(EventKey.WARNING_INVALID_CAT_SKIPPED,
  * 		new String[] { "InventoryName" }));
  *
*/ -public class SfdxScannerExceptionMatcher extends TypeSafeMatcher { +public class MessagePassableExceptionMatcher extends TypeSafeMatcher { private final EventKey expectedEventKey; private final String[] expectedArgs; - public SfdxScannerExceptionMatcher(EventKey expectedEventKey, String[] expectedArgs) { + public MessagePassableExceptionMatcher(EventKey expectedEventKey, String[] expectedArgs) { this.expectedEventKey = expectedEventKey; this.expectedArgs = nullToEmpty(expectedArgs); } - + @Override - protected boolean matchesSafely(SfdxScannerException item) { + protected boolean matchesSafely(MessagePassableException item) { String[] actualArgs = nullToEmpty(item.getArgs()); return expectedEventKey.equals(item.getEventKey()) && - Arrays.equals(expectedArgs, actualArgs); + Arrays.equals(expectedArgs, actualArgs); } @Override public void describeTo(Description description) { description.appendText("EventKey=").appendValue(expectedEventKey.name()).appendText(", Args=") - .appendValue(expectedArgs); + .appendValue(expectedArgs); } - + /** * Convert a null array to empty array. The are equivalent for our purposes. */ diff --git a/pmd-cataloger/src/test/java/sfdc/sfdx/scanner/pmd/PmdRuleCatalogerTest.java b/pmd-cataloger/src/test/java/sfdc/sfdx/scanner/pmd/PmdRuleCatalogerTest.java index 77bfacefb..69ee077f3 100644 --- a/pmd-cataloger/src/test/java/sfdc/sfdx/scanner/pmd/PmdRuleCatalogerTest.java +++ b/pmd-cataloger/src/test/java/sfdc/sfdx/scanner/pmd/PmdRuleCatalogerTest.java @@ -5,50 +5,94 @@ import static org.hamcrest.Matchers.hasSize; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertThat; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.fail; import static sfdc.sfdx.scanner.TestConstants.*; +import java.nio.file.Path; +import java.nio.file.Paths; import java.util.Arrays; import java.util.Collections; import java.util.Hashtable; import java.util.List; import java.util.Map; +import com.salesforce.messaging.CliMessager; import org.json.simple.JSONObject; +import org.json.simple.parser.JSONParser; +import org.json.simple.parser.ParseException; +import org.junit.After; +import org.junit.Before; import org.junit.Rule; import org.junit.Test; import org.junit.rules.ExpectedException; import org.mockito.ArgumentCaptor; import org.mockito.Mockito; -import sfdc.sfdx.scanner.messaging.EventKey; +import com.salesforce.messaging.EventKey; import sfdc.sfdx.scanner.pmd.catalog.PmdCatalogJson; /** * Unit test for {@link PmdRuleCataloger} */ public class PmdRuleCatalogerTest { + private static final String TEST_CATALOG_DIR = "./test/path/to/a/directory"; + private static final String TEST_CATALOG_FILE = "PmdCatalog.json"; @Rule - public ExpectedException thrown = ExpectedException.none();; + public ExpectedException thrown = ExpectedException.none(); + + public ArgumentCaptor jsonContentsCaptor; + public ArgumentCaptor directoryPathCaptor; + public ArgumentCaptor fileNameCaptor; + + @Before + public void setup() { + System.setProperty("catalogHome", TEST_CATALOG_DIR); + System.setProperty("catalogName", TEST_CATALOG_FILE); + CliMessager.getInstance().resetMessages(); + } + + @After + public void teardown() { + System.clearProperty("catalogHome"); + System.clearProperty("catalogName"); + } + + public PmdRuleCataloger createPmdRuleCatalogerSpy(Map> rulePathEntries) { + PmdRuleCataloger pmdRuleCataloger = new PmdRuleCataloger(rulePathEntries); + PmdRuleCataloger pmdRuleCatalogerSpy = Mockito.spy(pmdRuleCataloger); + + jsonContentsCaptor = ArgumentCaptor.forClass(String.class); + directoryPathCaptor = ArgumentCaptor.forClass(Path.class); + fileNameCaptor = ArgumentCaptor.forClass(String.class); + + Mockito.doNothing().when(pmdRuleCatalogerSpy).persistJsonToFile(jsonContentsCaptor.capture(), directoryPathCaptor.capture(), fileNameCaptor.capture()); + + return pmdRuleCatalogerSpy; + } + @SuppressWarnings("unchecked") @Test public void testAddJar() { Map> rulePathEntries = new Hashtable<>(); - + rulePathEntries.put(APEX, Collections.singletonList(JAR_FILE_CATEGORIES_AND_RULESETS.toAbsolutePath().toString())); - PmdRuleCataloger pmdRuleCataloger = new PmdRuleCataloger(rulePathEntries); - PmdRuleCataloger pmdRuleCatalogerSpy = Mockito.spy(pmdRuleCataloger); - - ArgumentCaptor pmdCatalogJsonCaptor = ArgumentCaptor.forClass(PmdCatalogJson.class); - Mockito.doNothing().when(pmdRuleCatalogerSpy).writeJsonToFile(pmdCatalogJsonCaptor.capture()); + + PmdRuleCataloger pmdRuleCatalogerSpy = createPmdRuleCatalogerSpy(rulePathEntries); pmdRuleCatalogerSpy.catalogRules(); - - PmdCatalogJson pmdCatalogJson = pmdCatalogJsonCaptor.getValue(); - assertNotNull(pmdCatalogJson); - - JSONObject json = pmdCatalogJson.constructJson(); - assertNotNull(json); - + + String catalogJson = jsonContentsCaptor.getValue(); + Path directoryPath = directoryPathCaptor.getValue(); + String fileName = fileNameCaptor.getValue(); + + JSONObject json = null; + try { + json = (JSONObject) (new JSONParser().parse(catalogJson)); + } catch (ParseException pe) { + fail("Parse failure " + pe.getMessage()); + } + List rulesets = (List)json.get(PmdCatalogJson.JSON_RULESETS); assertNotNull(rulesets); assertThat(rulesets, hasSize(equalTo(1))); @@ -62,27 +106,32 @@ public void testAddJar() { Map category = (Map)categories.get(0); assertNotNull(category); assertThat((List)category.get(PmdCatalogJson.JSON_PATHS), contains("category/apex/cat1.xml")); + + assertEquals(directoryPath, Paths.get(TEST_CATALOG_DIR)); + assertEquals(fileName, TEST_CATALOG_FILE); } - + @SuppressWarnings("unchecked") @Test public void testAddXml() { String path = XML_FILE.toAbsolutePath().toString(); Map> rulePathEntries = new Hashtable<>(); - + rulePathEntries.put(APEX, Collections.singletonList(path)); - PmdRuleCataloger pmdRuleCataloger = new PmdRuleCataloger(rulePathEntries); - PmdRuleCataloger pmdRuleCatalogerSpy = Mockito.spy(pmdRuleCataloger); - - ArgumentCaptor pmdCatalogJsonCaptor = ArgumentCaptor.forClass(PmdCatalogJson.class); - Mockito.doNothing().when(pmdRuleCatalogerSpy).writeJsonToFile(pmdCatalogJsonCaptor.capture()); + + PmdRuleCataloger pmdRuleCatalogerSpy = createPmdRuleCatalogerSpy(rulePathEntries); pmdRuleCatalogerSpy.catalogRules(); - - PmdCatalogJson pmdCatalogJson = pmdCatalogJsonCaptor.getValue(); - assertNotNull(pmdCatalogJson); - JSONObject json = pmdCatalogJson.constructJson(); - assertNotNull(json); + String catalogJson = jsonContentsCaptor.getValue(); + Path directoryPath = directoryPathCaptor.getValue(); + String fileName = fileNameCaptor.getValue(); + + JSONObject json = null; + try { + json = (JSONObject) (new JSONParser().parse(catalogJson)); + } catch (ParseException pe) { + fail("Parse failure " + pe.getMessage()); + } List categories = (List)json.get(PmdCatalogJson.JSON_CATEGORIES); assertNotNull(categories); @@ -90,17 +139,20 @@ public void testAddXml() { Map category = (Map)categories.get(0); assertNotNull(category); assertThat((List)category.get(PmdCatalogJson.JSON_PATHS), contains(path)); + + assertEquals(directoryPath, Paths.get(TEST_CATALOG_DIR)); + assertEquals(fileName, TEST_CATALOG_FILE); } - + @Test public void testExceptionIsThrownWhenCollisionOccurs() { Map> rulePathEntries = new Hashtable<>(); - + rulePathEntries.put(APEX, Arrays.asList(COLLISION_JAR_1.toAbsolutePath().toString(), COLLISION_JAR_2.toAbsolutePath().toString())); PmdRuleCataloger pmdRuleCataloger = new PmdRuleCataloger(rulePathEntries); - - thrown.expect(new SfdxScannerExceptionMatcher(EventKey.ERROR_EXTERNAL_DUPLICATE_XML_PATH, + + thrown.expect(new MessagePassableExceptionMatcher(EventKey.ERROR_EXTERNAL_DUPLICATE_XML_PATH, new String[] { "category/joshapex/somecat.xml", COLLISION_JAR_2.toAbsolutePath().toString(), COLLISION_JAR_1.toAbsolutePath().toString() })); diff --git a/pmd-cataloger/src/test/java/sfdc/sfdx/scanner/pmd/XmlFileFinderTest.java b/pmd-cataloger/src/test/java/sfdc/sfdx/scanner/pmd/XmlFileFinderTest.java index 8a10a68e2..e8e3dd8cf 100644 --- a/pmd-cataloger/src/test/java/sfdc/sfdx/scanner/pmd/XmlFileFinderTest.java +++ b/pmd-cataloger/src/test/java/sfdc/sfdx/scanner/pmd/XmlFileFinderTest.java @@ -16,7 +16,7 @@ import org.junit.Rule; import org.junit.Test; import org.junit.rules.ExpectedException; -import sfdc.sfdx.scanner.messaging.EventKey; +import com.salesforce.messaging.EventKey; /** * Unit tests for {@link XmlFileFinder} @@ -106,7 +106,7 @@ private String getSingleFile(List xmlContainers, Str @Test public void testFindingNonExistentFile_ExpectError() { XmlFileFinder xmlFileFinder = new XmlFileFinder(); - thrown.expect(new SfdxScannerExceptionMatcher(EventKey.ERROR_INTERNAL_CLASSPATH_DOES_NOT_EXIST, new String[]{"nonexistentfile.xml"})); + thrown.expect(new MessagePassableExceptionMatcher(EventKey.ERROR_INTERNAL_CLASSPATH_DOES_NOT_EXIST, new String[]{"nonexistentfile.xml"})); List xmlContainers = xmlFileFinder.findXmlFilesInPath("nonexistentfile.xml"); } diff --git a/pmd-cataloger/src/test/java/sfdc/sfdx/scanner/pmd/catalog/PmdCatalogRuleTest.java b/pmd-cataloger/src/test/java/sfdc/sfdx/scanner/pmd/catalog/PmdCatalogRuleTest.java index 034ff90fa..45abd4071 100644 --- a/pmd-cataloger/src/test/java/sfdc/sfdx/scanner/pmd/catalog/PmdCatalogRuleTest.java +++ b/pmd-cataloger/src/test/java/sfdc/sfdx/scanner/pmd/catalog/PmdCatalogRuleTest.java @@ -1,22 +1,31 @@ package sfdc.sfdx.scanner.pmd.catalog; +import com.salesforce.messaging.EventKey; import org.json.simple.JSONObject; import static org.junit.Assert.*; +import org.junit.Rule; import org.junit.Test; import static org.mockito.Mockito.*; +import org.junit.rules.ExpectedException; import org.w3c.dom.Element; import org.w3c.dom.NodeList; +import sfdc.sfdx.scanner.pmd.MessagePassableExceptionMatcher; import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; import java.util.List; public class PmdCatalogRuleTest { + @Rule + public ExpectedException thrown = ExpectedException.none(); + private static final String NAME = "Name"; private static final String MESSAGE = "Some message"; private static final String LANGUAGE = "apex"; @@ -29,7 +38,7 @@ public class PmdCatalogRuleTest { @Test public void testCatalogRuleJsonConversion() { // Setup mock - final Element elementMock = getElementMock(1, "description"); + final Element elementMock = getElementMock(Collections.singletonList("description")); final PmdCatalogRule catalogRule = new PmdCatalogRule(elementMock, CATEGORY, LANGUAGE); @@ -55,11 +64,10 @@ public void testCatalogRuleJsonConversion() { @Test public void testCatalogRuleNoDescription() { - final int descriptionNlCount = 0; final String emptyDescription = ""; // Setup mock - final Element elementMock = getElementMock(descriptionNlCount, emptyDescription); + final Element elementMock = getElementMock(Collections.singletonList("")); final PmdCatalogRule catalogRule = new PmdCatalogRule(elementMock, CATEGORY, LANGUAGE); // Execute @@ -71,11 +79,10 @@ public void testCatalogRuleNoDescription() { @Test public void testCatalogRuleJsonWithDescription() { - final int descriptionNlCount = 1; final String description = "Some description"; // Setup mock - final Element elementMock = getElementMock(descriptionNlCount, description); + final Element elementMock = getElementMock(Collections.singletonList(description)); final PmdCatalogRule catalogRule = new PmdCatalogRule(elementMock, CATEGORY, LANGUAGE); // Execute @@ -85,18 +92,33 @@ public void testCatalogRuleJsonWithDescription() { assertEquals("Unexpected description", description, jsonObject.get(PmdCatalogJson.JSON_DESCRIPTION)); } - private Element getElementMock(int descriptionNlCount, String emptyDescription) { + @Test + public void testCatalogRuleJsonWithMultipleDescriptions_expectException() { + final String description1 = "Some Description"; + final String description2 = "Some Other Description"; + + // Setup mock + final Element elementMock = getElementMock(Arrays.asList(description1, description2)); + thrown.expect(new MessagePassableExceptionMatcher(EventKey.ERROR_EXTERNAL_MULTIPLE_RULE_DESC, + new String[]{CATEGORY.getPath() + "/" + NAME, "2"} + )); + // Even initializing the object should be enough to trigger the expected exception. + final PmdCatalogRule catalogRule = new PmdCatalogRule(elementMock, CATEGORY, LANGUAGE); + } + + private Element getElementMock(List descriptions) { final Element elementMock = mock(Element.class); doReturn(NAME).when(elementMock).getAttribute(PmdCatalogRule.ATTR_NAME); doReturn(MESSAGE).when(elementMock).getAttribute(PmdCatalogRule.ATTR_MESSAGE); - final Element descElementMock = mock(Element.class); - doReturn(emptyDescription).when(descElementMock).getTextContent(); - - final NodeList nodeList = mock(NodeList.class); - doReturn(descriptionNlCount).when(nodeList).getLength(); - doReturn(descElementMock).when(nodeList).item(0); - doReturn(nodeList).when(elementMock).getElementsByTagName(PmdCatalogRule.ATTR_DESCRIPTION); + final NodeList nodeListMock = mock(NodeList.class); + doReturn(descriptions.size()).when(nodeListMock).getLength(); + for (int i = 0; i < descriptions.size(); i++) { + final Element descElementMock = mock(Element.class); + doReturn(descriptions.get(i)).when(descElementMock).getTextContent(); + doReturn(descElementMock).when(nodeListMock).item(i); + } + doReturn(nodeListMock).when(elementMock).getElementsByTagName(PmdCatalogRule.ATTR_DESCRIPTION); return elementMock; } diff --git a/retire-js/RetireJsVulns.json b/retire-js/RetireJsVulns.json index 390af2407..1c8d04ba9 100644 --- a/retire-js/RetireJsVulns.json +++ b/retire-js/RetireJsVulns.json @@ -253,17 +253,41 @@ "hashes": {} } }, - "jquery.validator": { + "jquery-validation": { "bowername": [ - "jquery-validator" + "jquery-validation" + ], + "vulnerabilities": [ + { + "below": "1.19.4", + "severity": "medium", + "identifiers": { + "summary": "ReDoS vulnerability in URL2 validation" + }, + "info": [ + "https://github.com/jquery-validation/jquery-validation/blob/master/changelog.md#1194--2022-05-19" + ] + }, + { + "below": "1.19.3", + "severity": "medium", + "identifiers": { + "CVE": [ + "CVE-2021-21252" + ], + "summary": "Regular Expression Denial of Service vulnerability" + }, + "info": [ + "https://github.com/jquery-validation/jquery-validation/blob/master/changelog.md#1193--2021-01-09" + ] + } ], - "vulnerabilities": [], "extractors": { "filename": [ - "jquery.validation-(§§version§§)(.min)?\\.js" + "jquery.validat(?:ion|e)-(§§version§§)(.min)?\\.js" ], "uri": [ - "/(§§version§§)/jquery.validation(\\.min)?\\.js" + "/(§§version§§)/jquery.validat(ion|e)(\\.min)?\\.js" ], "filecontent": [ "/\\*!?(?:\n \\*)? jQuery Validation Plugin v(§§version§§)" @@ -783,6 +807,46 @@ "info": [ "https://www.tiny.cloud/docs/release-notes/release-notes54/" ] + }, + { + "below": "5.6.0", + "severity": "medium", + "identifiers": { + "summary": "security issue where URLs in attributes weren’t correctly sanitized. security issue in the codesample plugin" + }, + "info": [ + "https://www.tiny.cloud/docs/release-notes/release-notes56/#securityfixes" + ] + }, + { + "below": "5.7.1", + "severity": "medium", + "identifiers": { + "summary": "URLs are not correctly filtered in some cases." + }, + "info": [ + "https://www.tiny.cloud/docs/release-notes/release-notes571/#securityfixes" + ] + }, + { + "below": "5.9.0", + "severity": "medium", + "identifiers": { + "summary": "Inserting certain HTML content into the editor could result in invalid HTML once parsed. This caused a medium severity Cross Site Scripting (XSS) vulnerability" + }, + "info": [ + "https://www.tiny.cloud/docs/release-notes/release-notes59/#securityfixes" + ] + }, + { + "below": "5.10.0", + "severity": "medium", + "identifiers": { + "summary": "URLs not cleaned correctly in some cases in the link and image plugins" + }, + "info": [ + "https://www.tiny.cloud/docs/release-notes/release-notes510/#securityfixes" + ] } ], "extractors": { @@ -1898,6 +1962,26 @@ "info": [ "https://github.com/wycats/handlebars.js/blob/master/release-notes.md#v453---november-18th-2019" ] + }, + { + "below": "4.6.0", + "severity": "medium", + "identifiers": { + "summary": "Denial of service" + }, + "info": [ + "https://github.com/handlebars-lang/handlebars.js/pull/1633" + ] + }, + { + "below": "4.7.7", + "severity": "medium", + "identifiers": { + "summary": "Prototype pollution" + }, + "info": [ + "https://github.com/handlebars-lang/handlebars.js/commit/f0589701698268578199be25285b2ebea1c1e427" + ] } ], "extractors": { @@ -2985,11 +3069,38 @@ } }, "svelte": { - "vulnerabilities": [], + "vulnerabilities": [ + { + "below": "3.46.5", + "severity": "medium", + "identifiers": { + "summary": "XSS" + }, + "info": [ + "https://github.com/sveltejs/svelte/pull/7333" + ] + }, + { + "below": "2.9.8", + "severity": "medium", + "identifiers": { + "summary": "XSS" + }, + "info": [ + "https://github.com/sveltejs/svelte/pull/1623" + ] + } + ], "extractors": { - "uri": [], - "filename": [], - "filecontent": [] + "uri": [ + "/svelte@(§§version§§)/index.js" + ], + "filename": [ + "svelte[@\\-](§§version§§)(.min)?\\.js" + ], + "filecontent": [ + "generated by Svelte v\\$\\{'(§§version§§)'\\}" + ] } }, "axios": { @@ -3146,6 +3257,35 @@ ] } }, + "AlaSQL": { + "vulnerabilities": [ + { + "below": "0.7.0", + "severity": "high", + "identifiers": { + "CVE": [ + "CVE-XXXX-XXXX" + ], + "bug": "SNYK-JS-ALASQL-1082932", + "summary": "An arbitrary code execution exists as AlaSQL doesn't sanitize input when characters are placed between square brackets [] or preceded with a backtik (accent grave) ` character. Versions older that 0.7.0 were deprecated in March of 2021 and should no longer be used." + }, + "info": [ + "https://security.snyk.io/vuln/SNYK-JS-ALASQL-1082932" + ] + } + ], + "extractors": { + "uri": [ + "/alasql[/@](§§version§§)/.*\\.js" + ], + "filename": [ + "alasql-(§§version§§)(\\.min)?\\.js" + ], + "filecontent": [ + "/\\*!?[ \n]*AlaSQL v(§§version§§)" + ] + } + }, "dont check": { "extractors": { "uri": [ diff --git a/settings.gradle.kts b/settings.gradle.kts index e4b0f53a7..89468d0d5 100644 --- a/settings.gradle.kts +++ b/settings.gradle.kts @@ -7,5 +7,4 @@ rootProject.name = "sfdx-scanner" -include("pmd-cataloger") -include("sfge") +include(":cli-messaging", ":pmd-cataloger", ":sfge") diff --git a/sfge/build.gradle.kts b/sfge/build.gradle.kts index 9954e0421..fadc52fbc 100644 --- a/sfge/build.gradle.kts +++ b/sfge/build.gradle.kts @@ -10,6 +10,7 @@ repositories { } dependencies { + implementation(project(":cli-messaging")) implementation("commons-cli:commons-cli:1.4") implementation("org.apache.commons:commons-collections4:4.4") implementation("org.apache.tinkerpop:tinkergraph-gremlin:3.5.1") diff --git a/sfge/settings.gradle.kts b/sfge/settings.gradle.kts deleted file mode 100644 index 2a5845a37..000000000 --- a/sfge/settings.gradle.kts +++ /dev/null @@ -1 +0,0 @@ -rootProject.name = "sfge" diff --git a/sfge/src/main/java/com/salesforce/Main.java b/sfge/src/main/java/com/salesforce/Main.java index 28064664e..d7f4e0c31 100644 --- a/sfge/src/main/java/com/salesforce/Main.java +++ b/sfge/src/main/java/com/salesforce/Main.java @@ -5,6 +5,7 @@ import com.salesforce.exception.SfgeException; import com.salesforce.exception.SfgeRuntimeException; import com.salesforce.graph.ops.GraphUtil; +import com.salesforce.messaging.CliMessager; import com.salesforce.metainfo.MetaInfoCollector; import com.salesforce.metainfo.MetaInfoCollectorProvider; import com.salesforce.rules.AbstractRule; @@ -22,12 +23,14 @@ * The main class, invoked by sfdx-scanner. `catalog` flow lists all of the available rules in a * standardized format. * - *

The `execute` flow accepts three parameters. + *

The `execute` flow accepts as a parameter the name of a file whose contents are a JSON with + * the following structure: * *

    - *
  1. The name of a file whose * contents are directories from which the graph should be built. - *
  2. The name of a file whose contents are individual files against which rules should be run. - *
  3. A comma-separated list of rule names. + *
  4. rulesToRun: An array of rule names. + *
  5. projectDirs: An array of directories from which the graph should be built. + *
  6. targets: An array of objects with a `targetFile` property indicating the file to be + * analyzed and a `targetMethods` property indicating individual methods. *
* *

Exit codes: @@ -147,6 +150,7 @@ private int execute(String... args) { allViolations.size())); } OutputFormatter formatter = new OutputFormatter(); + System.out.println(CliMessager.getInstance().getAllMessagesWithFormatting()); System.out.println(formatter.formatViolationJsons(allViolations)); return allViolations.isEmpty() ? EXIT_NO_VIOLATIONS : EXIT_WITH_VIOLATIONS; } diff --git a/sfge/src/main/java/com/salesforce/cli/CliArgParser.java b/sfge/src/main/java/com/salesforce/cli/CliArgParser.java index 74ab4cc85..e8e7eabd3 100644 --- a/sfge/src/main/java/com/salesforce/cli/CliArgParser.java +++ b/sfge/src/main/java/com/salesforce/cli/CliArgParser.java @@ -40,7 +40,7 @@ public CLI_ACTION getCliAction(String... args) { } public static class ExecuteArgParser { - private static int ARG_COUNT = 4; + private static int ARG_COUNT = 2; private final List projectDirs; private final List targets; @@ -63,9 +63,10 @@ public void parseArgs(String... args) { "Wrong number of arguments. Expected %d; received %d", ARG_COUNT, args.length)); } - identifyTargetFiles(args[1]); - identifyProjectDirs(args[2]); - identifyRules(args[3]); + ExecuteInput input = readInputFile(args[1]); + targets.addAll(input.targets); + projectDirs.addAll(input.projectDirs); + identifyRules(input.rulesToRun); } public List getProjectDirs() { @@ -102,6 +103,17 @@ private void identifyTargetFiles(String inputFile) { } } + private ExecuteInput readInputFile(String fileName) { + try { + String inputJson = String.join("\n", readFile(fileName)); + Gson gson = new Gson(); + return gson.fromJson(inputJson, ExecuteInput.class); + } catch (IOException ex) { + throw new InvocationException( + "Could not read input file " + fileName + ": " + ex.getMessage(), ex); + } + } + private List readFile(String fileName) throws IOException { final List allLines = Files.readAllLines(Paths.get(fileName)); final List lines = @@ -112,10 +124,9 @@ private List readFile(String fileName) throws IOException { return lines; } - private void identifyRules(String ruleString) { - String[] ruleNames = ruleString.split(","); + private void identifyRules(List rulesToRun) { try { - for (String ruleName : ruleNames) { + for (String ruleName : rulesToRun) { AbstractRule rule = RuleUtil.getRule(ruleName); selectedRules.add(rule); } @@ -134,4 +145,10 @@ public static class InvocationException extends SfgeRuntimeException { super(msg, cause); } } + + public static class ExecuteInput { + private List rulesToRun; + private List projectDirs; + private List targets; + } } diff --git a/sfge/src/main/java/com/salesforce/graph/ops/CloneUtil.java b/sfge/src/main/java/com/salesforce/graph/ops/CloneUtil.java index 4329acfbc..5877a9807 100644 --- a/sfge/src/main/java/com/salesforce/graph/ops/CloneUtil.java +++ b/sfge/src/main/java/com/salesforce/graph/ops/CloneUtil.java @@ -9,6 +9,7 @@ import com.salesforce.exception.UnexpectedException; import com.salesforce.graph.DeepCloneable; import com.salesforce.graph.Immutable; +import com.salesforce.graph.ops.directive.EngineDirective; import com.salesforce.graph.symbols.AbstractClassInstanceScope; import com.salesforce.graph.symbols.ClassInstanceScope; import com.salesforce.graph.symbols.DeserializedClassInstanceScope; @@ -230,6 +231,8 @@ private static T cloneIfPossible(@Nullable T item) { return (T) cloneTreeMap((TreeMap) item); } else if (item instanceof BaseSFVertex) { return item; + } else if (item instanceof EngineDirective) { + return item; } else if (item instanceof LinkedList) { return (T) cloneLinkedList((LinkedList) item); } else if (item instanceof Enum) { diff --git a/sfge/src/main/java/com/salesforce/graph/ops/MethodUtil.java b/sfge/src/main/java/com/salesforce/graph/ops/MethodUtil.java index e1101f7b6..5e6720e96 100644 --- a/sfge/src/main/java/com/salesforce/graph/ops/MethodUtil.java +++ b/sfge/src/main/java/com/salesforce/graph/ops/MethodUtil.java @@ -53,6 +53,8 @@ import com.salesforce.graph.vertex.VariableExpressionVertex; import com.salesforce.graph.visitor.ApexPathWalker; import com.salesforce.graph.visitor.DefaultNoOpPathVertexVisitor; +import com.salesforce.messaging.CliMessager; +import com.salesforce.messaging.EventKey; import com.salesforce.metainfo.MetaInfoCollectorProvider; import com.salesforce.rules.AbstractRuleRunner.RuleRunnerTarget; import java.util.ArrayList; @@ -99,7 +101,7 @@ public static List getTargetedMethods( // things, the performance // hit is negligible. for (RuleRunnerTarget target : targets) { - methodVertices.addAll( + List targetMethodVertices = SFVertexFactory.loadVertices( g, g.V().where( @@ -112,11 +114,51 @@ public static List getTargetedMethods( .until(__.has(Schema.FILE_NAME)) .has(Schema.FILE_NAME, target.getTargetFile()) .count() - .is(P.eq(1))))); + .is(P.eq(1)))); + addMessagesForTarget(target, targetMethodVertices); + methodVertices.addAll(targetMethodVertices); } return methodVertices; } + /** + * If any of the method names specified by the provided target returned multiple results or zero + * results, adds a message to a {@link CliMessager} instance indicating such. + * + * @param target - A target that specifies a file and methods within that file + * @param vertices - The method vertices returned by the query created using the target + */ + private static void addMessagesForTarget(RuleRunnerTarget target, List vertices) { + TreeMap methodCountByName = CollectionUtil.newTreeMap(); + // Map each vertex's method name to the number of vertices sharing that name. + for (MethodVertex methodVertex : vertices) { + String methodName = methodVertex.getName(); + int priorCount = methodCountByName.getOrDefault(methodName, 0); + methodCountByName.put(methodName, priorCount + 1); + } + // For each of the methods we were instructed to target, see how many methods with that name + // were found. + for (String targetMethod : target.getTargetMethods()) { + Integer methodCount = methodCountByName.getOrDefault(targetMethod, 0); + if (methodCount == 0) { + CliMessager.getInstance() + .addMessage( + "Loading " + targetMethod + " vertices", + EventKey.WARNING_NO_METHOD_TARGET_MATCHES, + target.getTargetFile(), + targetMethod); + } else if (methodCount > 1) { + CliMessager.getInstance() + .addMessage( + "Loading " + targetMethod + " vertices", + EventKey.WARNING_MULTIPLE_METHOD_TARGET_MATCHES, + methodCount.toString(), + target.getTargetFile(), + targetMethod); + } + } + } + /** * Returns non-test methods in the target files with an @AuraEnabled annotation. An empty list * implicitly includes all files. diff --git a/sfge/src/main/java/com/salesforce/rules/AbstractRuleRunner.java b/sfge/src/main/java/com/salesforce/rules/AbstractRuleRunner.java index 9eaa3abbb..b97595a82 100644 --- a/sfge/src/main/java/com/salesforce/rules/AbstractRuleRunner.java +++ b/sfge/src/main/java/com/salesforce/rules/AbstractRuleRunner.java @@ -160,8 +160,13 @@ public String toString() { } public static final class RuleRunnerTarget { - private String targetFile; - private List targetMethods; + private final String targetFile; + private final List targetMethods; + + public RuleRunnerTarget(String targetFile, List targetMethods) { + this.targetFile = targetFile; + this.targetMethods = targetMethods; + } /** Get the name of the file that this target represents. */ public String getTargetFile() { diff --git a/sfge/src/test/java/com/salesforce/graph/ops/MethodUtilTest.java b/sfge/src/test/java/com/salesforce/graph/ops/MethodUtilTest.java new file mode 100644 index 000000000..a139421a8 --- /dev/null +++ b/sfge/src/test/java/com/salesforce/graph/ops/MethodUtilTest.java @@ -0,0 +1,315 @@ +package com.salesforce.graph.ops; + +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.hasSize; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.junit.jupiter.api.Assertions.fail; + +import com.salesforce.TestUtil; +import com.salesforce.graph.vertex.MethodVertex; +import com.salesforce.messaging.CliMessager; +import com.salesforce.messaging.EventKey; +import com.salesforce.rules.AbstractRuleRunner.RuleRunnerTarget; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; +import java.util.List; +import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversalSource; +import org.hamcrest.MatcherAssert; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +public class MethodUtilTest { + private GraphTraversalSource g; + + private static final String METHOD_THAT_DOES_NOT_EXIST = "methodThatDoesNotExist"; + private static final String METHOD_WITHOUT_OVERLOADS_1 = "methodWithoutOverloads1"; + private static final String METHOD_WITHOUT_OVERLOADS_2 = "methodWithoutOverloads2"; + private static final String METHOD_WITHOUT_OVERLOADS_3 = "methodWithoutOverloads3"; + private static final String METHOD_WITH_INTERNAL_OVERLOADS = "methodWithInternalOverloads"; + private static final String METHOD_WITH_EXTERNAL_NAME_DUPLICATION = + "methodWithExternalNameDuplication"; + private static final String METHOD_WITH_INNER_CLASS_DUPLICATION = + "methodWithInnerClassDuplication"; + + private static final String SOURCE_FILE_1 = + "public class Foo1 {\n" + + " public boolean " + + METHOD_WITHOUT_OVERLOADS_1 + + "() {\n" + + " return true;\n" + + " }\n" + + "\n" + + " public boolean " + + METHOD_WITHOUT_OVERLOADS_2 + + "() {\n" + + " return true;\n" + + " }\n" + + "\n" + + " public boolean " + + METHOD_WITH_INTERNAL_OVERLOADS + + "() {\n" + + " return true;\n" + + " }\n" + + "\n" + + " public boolean " + + METHOD_WITH_INTERNAL_OVERLOADS + + "(boolean b) {\n" + + " return b;\n" + + " }\n" + + "\n" + + " public boolean " + + METHOD_WITH_EXTERNAL_NAME_DUPLICATION + + "() {\n" + + " return true;\n" + + " }\n" + + "\n" + + " public boolean " + + METHOD_WITH_EXTERNAL_NAME_DUPLICATION + + "(boolean b) {\n" + + " return b;\n" + + " }\n" + + "}\n"; + + private static final String SOURCE_FILE_2 = + "public class Foo2 {\n" + + " public boolean " + + METHOD_WITH_EXTERNAL_NAME_DUPLICATION + + "() {\n" + + " return true;\n" + + " }\n" + + "\n" + + " public boolean " + + METHOD_WITH_EXTERNAL_NAME_DUPLICATION + + "(boolean b) {\n" + + " return b;\n" + + " }\n" + + "}\n"; + + private static final String SOURCE_FILE_3 = + "public class Foo3 {\n" + + " public boolean " + + METHOD_WITH_INNER_CLASS_DUPLICATION + + "() {\n" + + " return true;\n" + + " }\n" + + " \n" + + " public class InnerFoo {\n" + + " public boolean " + + METHOD_WITHOUT_OVERLOADS_3 + + "() {\n" + + " return true;\n" + + " }\n" + + " \n" + + " public boolean " + + METHOD_WITH_INNER_CLASS_DUPLICATION + + "() {\n" + + " return true;\n" + + " }\n" + + " }\n" + + "}\n"; + + @BeforeEach + public void setup() { + this.g = TestUtil.getGraph(); + CliMessager.getInstance().resetMessages(); + } + + @Test + public void getTargetMethods_targetSingleMethod() { + TestUtil.Config config = + TestUtil.Config.Builder.get(g, new String[] {SOURCE_FILE_1, SOURCE_FILE_2}).build(); + TestUtil.buildGraph(config); + // Create a rule target encompassing only the first non-overloaded method in the first file. + List targets = new ArrayList<>(); + targets.add( + new RuleRunnerTarget( + "TestCode0", Collections.singletonList(METHOD_WITHOUT_OVERLOADS_1))); + + List methodVertices = MethodUtil.getTargetedMethods(g, targets); + + MatcherAssert.assertThat(methodVertices, hasSize(equalTo(1))); + MethodVertex firstVertex = methodVertices.get(0); + assertEquals(METHOD_WITHOUT_OVERLOADS_1, firstVertex.getName()); + + String messages = CliMessager.getInstance().getAllMessages(); + assertEquals("[]", messages); + } + + @Test + public void getTargetMethods_targetSingleMethodCaseInsensitive() { + TestUtil.Config config = + TestUtil.Config.Builder.get(g, new String[] {SOURCE_FILE_1, SOURCE_FILE_2}).build(); + TestUtil.buildGraph(config); + // Create a rule target encompassing only the first non-overloaded method in the first file. + List targets = new ArrayList<>(); + targets.add( + new RuleRunnerTarget( + "TestCode0", + Collections.singletonList(METHOD_WITHOUT_OVERLOADS_1.toLowerCase()))); + + List methodVertices = MethodUtil.getTargetedMethods(g, targets); + + MatcherAssert.assertThat(methodVertices, hasSize(equalTo(1))); + MethodVertex firstVertex = methodVertices.get(0); + assertEquals(METHOD_WITHOUT_OVERLOADS_1, firstVertex.getName()); + + String messages = CliMessager.getInstance().getAllMessages(); + assertEquals("[]", messages); + } + + @Test + public void getTargetMethods_targetMultipleMethods() { + TestUtil.Config config = + TestUtil.Config.Builder.get(g, new String[] {SOURCE_FILE_1, SOURCE_FILE_2}).build(); + TestUtil.buildGraph(config); + // Create a rule target encompassing both non-overloaded methods in the first file. + List targets = new ArrayList<>(); + targets.add( + new RuleRunnerTarget( + "TestCode0", + Arrays.asList(METHOD_WITHOUT_OVERLOADS_1, METHOD_WITHOUT_OVERLOADS_2))); + + List methodVertices = MethodUtil.getTargetedMethods(g, targets); + + MatcherAssert.assertThat(methodVertices, hasSize(equalTo(2))); + MethodVertex firstVertex = methodVertices.get(0); + assertEquals(METHOD_WITHOUT_OVERLOADS_1, firstVertex.getName()); + + MethodVertex secondVertex = methodVertices.get(1); + assertEquals(METHOD_WITHOUT_OVERLOADS_2, secondVertex.getName()); + + String messages = CliMessager.getInstance().getAllMessages(); + assertEquals("[]", messages); + } + + @Test + public void getTargetMethods_targetOverloadedMethods() { + TestUtil.Config config = + TestUtil.Config.Builder.get(g, new String[] {SOURCE_FILE_1, SOURCE_FILE_2}).build(); + TestUtil.buildGraph(config); + // Create a rule target encompassing only the overloaded method in the first file. + List targets = new ArrayList<>(); + targets.add( + new RuleRunnerTarget( + "TestCode0", Collections.singletonList(METHOD_WITH_INTERNAL_OVERLOADS))); + + List methodVertices = MethodUtil.getTargetedMethods(g, targets); + + MatcherAssert.assertThat(methodVertices, hasSize(equalTo(2))); + MethodVertex firstVertex = methodVertices.get(0); + assertEquals(METHOD_WITH_INTERNAL_OVERLOADS, firstVertex.getName()); + + MethodVertex secondVertex = methodVertices.get(1); + assertEquals(METHOD_WITH_INTERNAL_OVERLOADS, secondVertex.getName()); + + String messages = CliMessager.getInstance().getAllMessages(); + MatcherAssert.assertThat( + messages, + containsString(EventKey.WARNING_MULTIPLE_METHOD_TARGET_MATCHES.getMessageKey())); + } + + @Test + public void getTargetMethods_targetNameDupedMethods() { + TestUtil.Config config = + TestUtil.Config.Builder.get(g, new String[] {SOURCE_FILE_1, SOURCE_FILE_2}).build(); + TestUtil.buildGraph(config); + // Create a rule target encompassing only the method in the first file whose name is used + // elsewhere. + List targets = new ArrayList<>(); + targets.add( + new RuleRunnerTarget( + "TestCode0", + Collections.singletonList(METHOD_WITH_EXTERNAL_NAME_DUPLICATION))); + + List methodVertices = MethodUtil.getTargetedMethods(g, targets); + + MatcherAssert.assertThat(methodVertices, hasSize(equalTo(2))); + MethodVertex firstVertex = methodVertices.get(0); + assertEquals(METHOD_WITH_EXTERNAL_NAME_DUPLICATION, firstVertex.getName()); + assertEquals(18, firstVertex.getBeginLine()); + + MethodVertex secondVertex = methodVertices.get(1); + assertEquals(METHOD_WITH_EXTERNAL_NAME_DUPLICATION, secondVertex.getName()); + assertEquals(22, secondVertex.getBeginLine()); + + String messages = CliMessager.getInstance().getAllMessages(); + MatcherAssert.assertThat( + messages, + containsString(EventKey.WARNING_MULTIPLE_METHOD_TARGET_MATCHES.getMessageKey())); + } + + @Test + public void getTargetMethods_targetMethodDoesNotExist() { + TestUtil.Config config = + TestUtil.Config.Builder.get(g, new String[] {SOURCE_FILE_1, SOURCE_FILE_2}).build(); + TestUtil.buildGraph(config); + // Create a rule target encompassing a method that doesn't actually exist with that name. + List targets = new ArrayList<>(); + targets.add( + new RuleRunnerTarget( + "TestCode0", Collections.singletonList(METHOD_THAT_DOES_NOT_EXIST))); + + List methodVertices = MethodUtil.getTargetedMethods(g, targets); + + MatcherAssert.assertThat(methodVertices, hasSize(equalTo(0))); + + String messages = CliMessager.getInstance().getAllMessages(); + MatcherAssert.assertThat( + messages, + containsString(EventKey.WARNING_NO_METHOD_TARGET_MATCHES.getMessageKey())); + } + + @Test + public void getTargetMethods_targetMethodInInnerClass() { + TestUtil.Config config = + TestUtil.Config.Builder.get(g, new String[] {SOURCE_FILE_3}).build(); + TestUtil.buildGraph(config); + // Create a rule target encompassing the method that exists only in the inner class. + List targets = new ArrayList<>(); + targets.add( + new RuleRunnerTarget( + "TestCode0", Collections.singletonList(METHOD_WITHOUT_OVERLOADS_3))); + + List methodVertices = MethodUtil.getTargetedMethods(g, targets); + + MatcherAssert.assertThat(methodVertices, hasSize(equalTo(1))); + } + + @Test + public void getTargetMethods_targetMethodInInnerAndOuterClass() { + TestUtil.Config config = + TestUtil.Config.Builder.get(g, new String[] {SOURCE_FILE_3}).build(); + TestUtil.buildGraph(config); + // Create a rule target encompassing the method that exists only in the inner class. + List targets = new ArrayList<>(); + targets.add( + new RuleRunnerTarget( + "TestCode0", + Collections.singletonList(METHOD_WITH_INNER_CLASS_DUPLICATION))); + + List methodVertices = MethodUtil.getTargetedMethods(g, targets); + + MatcherAssert.assertThat(methodVertices, hasSize(equalTo(2))); + boolean line2Found = false; + boolean line11Found = false; + for (MethodVertex methodVertex : methodVertices) { + assertEquals(METHOD_WITH_INNER_CLASS_DUPLICATION, methodVertex.getName()); + if (methodVertex.getBeginLine() == 2) { + line2Found = true; + } else if (methodVertex.getBeginLine() == 11) { + line11Found = true; + } else { + fail("Unexpected line number " + methodVertex.getBeginLine()); + } + } + assertTrue(line2Found); + assertTrue(line11Found); + String messages = CliMessager.getInstance().getAllMessages(); + MatcherAssert.assertThat( + messages, + containsString(EventKey.WARNING_MULTIPLE_METHOD_TARGET_MATCHES.getMessageKey())); + } +} diff --git a/sfge/src/test/java/com/salesforce/graph/ops/directive/EngineDirectiveTest.java b/sfge/src/test/java/com/salesforce/graph/ops/directive/EngineDirectiveTest.java index ffb6953ee..7b88c8a56 100644 --- a/sfge/src/test/java/com/salesforce/graph/ops/directive/EngineDirectiveTest.java +++ b/sfge/src/test/java/com/salesforce/graph/ops/directive/EngineDirectiveTest.java @@ -3,6 +3,7 @@ import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.has; import static org.hamcrest.Matchers.contains; import static org.hamcrest.Matchers.empty; +import static org.hamcrest.Matchers.hasItem; import com.salesforce.ArgumentsUtil; import com.salesforce.TestUtil; @@ -512,6 +513,66 @@ public boolean test(BaseSFVertex vertex) { MatcherAssert.assertThat(engineDirectives, contains(DISABLE_STACK_1)); } + /** + * This is a test for W-11120894, wherein the sfge-disable-stack engine directive wasn't working + * for methods with exceptions inside nested forked method calls. The attempt to clone the + * annotation during path generation was causing an exception, which was overriding the + * functionality of the annotation itself. This test recreates such a scenario, then makes sure + * the associated with it were correctly generated and contain the expected annotation. + */ + @MethodSource(value = "testDisableStackDirective") + @ParameterizedTest(name = "{displayName}: directive=({0})") + public void testDisableStackAnnotationWithForkedExceptionPaths(String directive) { + String[] sourceCode = { + "public class MyClass {\n" + + directive + + "\n" + + " public static void doSomething(boolean enterBranch, boolean throwException) {\n" + + " ExceptionThrower helper = new ExceptionThrower();\n" + + " if (enterBranch) {\n" + + " ExceptionThrower.throwExceptionIfAsked(throwException);\n" + + " }\n" + + " System.debug('hello');\n" + + " }\n" + + "}", + "public class ExceptionThrower {\n" + + " public void throwExceptionIfAsked(boolean isAsked) {\n" + + " if (isAsked) {\n" + + " throw new MyException();\n" + + " }\n" + + " }\n" + + "}" + }; + List engineDirectives = new ArrayList<>(); + VertexPredicate predicate = + new AbstractVisitingVertexPredicate() { + @Override + public boolean test(BaseSFVertex vertex) { + if (vertex instanceof MethodCallExpressionVertex) { + MethodCallExpressionVertex methodCallExpression = + (MethodCallExpressionVertex) vertex; + if (methodCallExpression.getFullMethodName().equals("System.debug")) { + engineDirectives.addAll( + ContextProviders.ENGINE_DIRECTIVE_CONTEXT + .get() + .getEngineDirectiveContext() + .getEngineDirectives()); + } + } + return true; + } + }; + + TestUtil.Config config = TestUtil.Config.Builder.get(g, sourceCode).build(); + ApexPathExpanderConfig expanderConfig = + ApexPathExpanderConfig.Builder.get() + .expandMethodCalls(true) + .withVertexPredicate(predicate) + .build(); + TestUtil.getApexPaths(config, expanderConfig, "doSomething"); + MatcherAssert.assertThat(engineDirectives, hasItem(DISABLE_STACK_1)); + } + @MethodSource(value = "testDisableStackDirective") @ParameterizedTest(name = "{displayName}: directive=({0})") public void testDisableStackAnnotationIntermediateMethod(String directive) { diff --git a/src/Constants.ts b/src/Constants.ts index e874040d8..e9f153217 100644 --- a/src/Constants.ts +++ b/src/Constants.ts @@ -94,7 +94,8 @@ export const Services = { export enum CUSTOM_CONFIG { EslintConfig = "EslintConfig", PmdConfig = "PmdConfig", - SfgeConfig = "SfgeConfig" + SfgeConfig = "SfgeConfig", + VerboseViolations = "VerboseViolations" } export const HARDCODED_RULES = { diff --git a/src/commands/scanner/run.ts b/src/commands/scanner/run.ts index 0ebe3e51f..153684981 100644 --- a/src/commands/scanner/run.ts +++ b/src/commands/scanner/run.ts @@ -104,6 +104,10 @@ export default class Run extends ScannerRunCommand { description: messages.getMessage('flags.nsDescription'), longDescription: messages.getMessage('flags.nsDescriptionLong') }), + "verbose-violations": flags.boolean({ + description: messages.getMessage('flags.verboseViolationsDescription'), + longDescription: messages.getMessage('flags.verboseViolationsDescriptionLong') + }), }; protected validateCommandFlags(): Promise { @@ -114,6 +118,12 @@ export default class Run extends ScannerRunCommand { if ((this.flags.pmdconfig || this.flags.eslintconfig) && (this.flags.category || this.flags.ruleset)) { this.ux.log(messages.getMessage('output.filtersIgnoredCustom', [])); } + // None of the pathless engines support method-level targeting, so attempting to use it should result in an error. + for (const target of (this.flags.target as string[])) { + if (target.indexOf('#') > -1) { + throw SfdxError.create('@salesforce/sfdx-scanner', 'run', 'validations.methodLevelTargetingDisallowed', [target]); + } + } return Promise.resolve(); } @@ -149,6 +159,13 @@ export default class Run extends ScannerRunCommand { const pmdConfig = normalize(untildify(this.flags.pmdconfig as string)); options.set(CUSTOM_CONFIG.PmdConfig, pmdConfig); } + + // Capturing verbose-violations flag value (used for RetireJS output) + if (this.flags["verbose-violations"]) { + options.set(CUSTOM_CONFIG.VerboseViolations, "true"); + } + + return options; } diff --git a/src/commands/scanner/run/dfa.ts b/src/commands/scanner/run/dfa.ts index 7497140ed..efb931d64 100644 --- a/src/commands/scanner/run/dfa.ts +++ b/src/commands/scanner/run/dfa.ts @@ -99,8 +99,8 @@ export default class Dfa extends ScannerRunCommand { }; protected async validateCommandFlags(): Promise { - // Entries in the projectdir array must be non-glob paths to existing directories. const fh = new FileHandler(); + // Entries in the projectdir array must be non-glob paths to existing directories. for (const dir of (this.flags.projectdir as string[])) { if (globby.hasMagic(dir)) { throw SfdxError.create('@salesforce/sfdx-scanner', 'run-dfa', 'validations.projectdirCannotBeGlob', []); @@ -110,6 +110,19 @@ export default class Dfa extends ScannerRunCommand { throw SfdxError.create('@salesforce/sfdx-scanner', 'run-dfa', 'validations.projectdirMustBeDir', []); } } + // Entries in the target array may specify methods, but only if the entry is neither a directory nor a glob. + for (const target of (this.flags.target as string[])) { + // The target specifies a method if it includes the `#` syntax. + if (target.indexOf('#') > -1) { + if( globby.hasMagic(target)) { + throw SfdxError.create('@salesforce/sfdx-scanner', 'run-dfa', 'validations.methodLevelTargetCannotBeGlob', []); + } + const potentialFilePath = target.split('#')[0]; + if (!(await fh.isFile(potentialFilePath))) { + throw SfdxError.create('@salesforce/sfdx-scanner', 'run-dfa', 'validations.methodLevelTargetMustBeRealFile', [potentialFilePath]); + } + } + } } /** diff --git a/src/lib/DefaultRuleManager.ts b/src/lib/DefaultRuleManager.ts index 726776f6a..b9ee1c31e 100644 --- a/src/lib/DefaultRuleManager.ts +++ b/src/lib/DefaultRuleManager.ts @@ -1,10 +1,10 @@ -import {Logger, Messages, SfdxError} from '@salesforce/core'; +import {Logger, Messages, SfdxError, Lifecycle} from '@salesforce/core'; import * as assert from 'assert'; import {Stats} from 'fs'; import {inject, injectable} from 'tsyringe'; -import {EngineExecutionDescriptor, RecombinedRuleResults, Rule, RuleGroup, RuleResult, RuleTarget} from '../types'; +import {EngineExecutionDescriptor, RecombinedRuleResults, Rule, RuleGroup, RuleResult, RuleTarget, TelemetryData} from '../types'; import {isEngineFilter, RuleFilter} from './RuleFilter'; -import {OutputOptions, RuleManager} from './RuleManager'; +import {RunOptions, RuleManager} from './RuleManager'; import {RuleResultRecombinator} from './formatter/RuleResultRecombinator'; import {RuleCatalog} from './services/RuleCatalog'; import {RuleEngine} from './services/RuleEngine'; @@ -14,6 +14,7 @@ import {Controller} from '../Controller'; import globby = require('globby'); import path = require('path'); import {uxEvents, EVENTS} from './ScannerEvents'; +import {CUSTOM_CONFIG} from '../Constants'; Messages.importMessagesDirectory(__dirname); const messages = Messages.loadMessages('@salesforce/sfdx-scanner', 'DefaultRuleManager'); @@ -67,7 +68,7 @@ export class DefaultRuleManager implements RuleManager { return this.catalog.getRulesMatchingFilters(filters); } - async runRulesMatchingCriteria(filters: RuleFilter[], targets: string[], outputOptions: OutputOptions, engineOptions: Map, runDfa = false): Promise { + async runRulesMatchingCriteria(filters: RuleFilter[], targets: string[], runOptions: RunOptions, engineOptions: Map): Promise { // Declare a variable that we can later use to store the engine results, as well as something to help us track // which engines actually ran. let results: RuleResult[] = []; @@ -87,7 +88,7 @@ export class DefaultRuleManager implements RuleManager { const engineTargets = await this.unpackTargets(e, targets, matchedTargets); this.logger.trace(`For ${e.getName()}, found ${engineGroups.length} groups, ${engineRules.length} rules, ${engineTargets.length} targets`); - if ((e.isDfaEngine() === runDfa) && e.shouldEngineRun(engineGroups, engineRules, engineTargets, engineOptions)) { + if ((e.isDfaEngine() === runOptions.runDfa) && e.shouldEngineRun(engineGroups, engineRules, engineTargets, engineOptions)) { this.logger.trace(`${e.getName()} is eligible to execute.`); executedEngines.add(e.getName()); // Create a descriptor for this engine run, but do not actually run it just yet. This is because the run @@ -99,7 +100,7 @@ export class DefaultRuleManager implements RuleManager { rules: engineRules, target: engineTargets, engineOptions: engineOptions, - normalizeSeverity: outputOptions.normalizeSeverity + normalizeSeverity: runOptions.normalizeSeverity } }); } else { @@ -109,7 +110,7 @@ export class DefaultRuleManager implements RuleManager { } this.validateRunDescriptors(runDescriptorList); - + await this.emitRunTelemetry(runDescriptorList, runOptions.sfdxVersion); // Warn the user if any positive targets were skipped const unmatchedTargets = targets.filter(t => !t.startsWith('!') && !matchedTargets.has(t)); @@ -126,8 +127,8 @@ export class DefaultRuleManager implements RuleManager { const psResults: RuleResult[][] = await Promise.all(ps); psResults.forEach(r => results = results.concat(r)); this.logger.trace(`Received rule violations: ${JSON.stringify(results)}`); - this.logger.trace(`Recombining results into requested format ${outputOptions.format}`); - return await RuleResultRecombinator.recombineAndReformatResults(results, outputOptions.format, executedEngines); + this.logger.trace(`Recombining results into requested format ${runOptions.format}`); + return await RuleResultRecombinator.recombineAndReformatResults(results, runOptions.format, executedEngines, engineOptions.has(CUSTOM_CONFIG.VerboseViolations)); } catch (e) { const message: string = e instanceof Error ? e.message : e as string; throw new SfdxError(message); @@ -143,6 +144,36 @@ export class DefaultRuleManager implements RuleManager { } } + protected async emitRunTelemetry(runDescriptorList: RunDescriptor[], sfdxVersion: string): Promise { + // Get the name of every engine being executed. + const executedEngineNames: Set = new Set(runDescriptorList.map(d => d.engine.getName().toLowerCase())); + // Build the base telemetry data. + const runTelemetryObject: TelemetryData = { + // This property is a requirement for the object. + eventName: 'ENGINE_EXECUTION', + // Knowing how many engines are run with each execution is valuable data. + executedEnginesCount: executedEngineNames.size, + // Creating a string of all the executed engines would yield data useful for metrics. + // Note: Calling `.sort()` without an argument causes a simple less-than to be used. + executedEnginesString: JSON.stringify([...executedEngineNames.values()].sort()), + sfdxVersion + }; + + const allEngines: RuleEngine[] = await Controller.getAllEngines(); + for (const engine of allEngines) { + const engineName = engine.getName().toLowerCase(); + // In addition to the string, assign each engine a boolean indicating whether it was executed. This will allow + // us to perform other kinds of analytics than the string. + runTelemetryObject[engineName] = executedEngineNames.has(engineName); + } + // NOTE: In addition to the information that we added here, the following useful information is always captured + // by default: + // - node version + // - plugin version + // - executed command (e.g., `scanner:run`) + await Lifecycle.getInstance().emitTelemetry(runTelemetryObject); + } + protected async resolveEngineFilters(filters: RuleFilter[], engineOptions: Map = new Map()): Promise { let filteredEngineNames: readonly string[] = null; for (const filter of filters) { @@ -206,46 +237,58 @@ export class DefaultRuleManager implements RuleManager { const pm = new PathMatcher([...engineTargets, ...negativePatterns]); for (const target of positivePatterns) { // Used to detect if the target resulted in a match - const startLength: number = ruleTargets.length; + const ruleTargetsInitialLength: number = ruleTargets.length; + // Positive patterns might use method-level targeting. We only want to do path evaluation against the part + // that's actually a path. + const targetPortions = target.split('#'); + // The array will always have at least one entry, since if there's no '#' then it will return a singleton array. + const targetPath = targetPortions[0]; if (globby.hasMagic(target)) { // The target is a magic glob. Retrieve paths in the working directory that match it, and then filter against // our pattern matcher. - const matchingTargets = await globby(target); + const matchingTargets = await globby(targetPath); // Map relative files to absolute paths. This solves ambiguity of current working directory const absoluteMatchingTargets = matchingTargets.map(t => path.resolve(t)); // Filter the targets based on our target patterns. const filteredTargets = await pm.filterPathsByPatterns(absoluteMatchingTargets); const ruleTarget = { - target, - paths: filteredTargets + target: targetPath, + paths: filteredTargets, + methods: [] }; if (ruleTarget.paths.length > 0) { ruleTargets.push(ruleTarget); } - } else if (await this.fileHandler.exists(target)) { - const stats: Stats = await this.fileHandler.stats(target); + } else if (await this.fileHandler.exists(targetPath)) { + const stats: Stats = await this.fileHandler.stats(targetPath); if (stats.isDirectory()) { // If the target is a directory, we should get everything in it, convert relative paths to absolute // paths, and then filter based our matcher. - const relativePaths = await globby(target); + const relativePaths = await globby(targetPath); const ruleTarget = { - target, + target: targetPath, isDirectory: true, - paths: await pm.filterPathsByPatterns(relativePaths.map(t => path.resolve(t))) + paths: await pm.filterPathsByPatterns(relativePaths.map(t => path.resolve(t))), + methods: [] }; if (ruleTarget.paths.length > 0) { ruleTargets.push(ruleTarget); } } else { // The target is just a file. Validate it against our matcher, and add it if eligible. - const absolutePath = path.resolve(target); + const absolutePath = path.resolve(targetPath); if (await pm.pathMatchesPatterns(absolutePath)) { - ruleTargets.push({target, paths: [absolutePath]}); + ruleTargets.push({ + target: targetPath, + paths: [absolutePath], + // If the pattern has method-level targets, then they're delimited with a semi-colon. + methods: targetPortions.length === 1 ? [] : targetPortions[1].split(';') + }); } } } - if (startLength !== ruleTargets.length) { + if (ruleTargetsInitialLength !== ruleTargets.length) { matchedTargets.add(target); } } diff --git a/src/lib/RuleManager.ts b/src/lib/RuleManager.ts index 2bc904ac0..efc370a58 100644 --- a/src/lib/RuleManager.ts +++ b/src/lib/RuleManager.ts @@ -11,9 +11,11 @@ export enum OUTPUT_FORMAT { XML = 'xml' } -export type OutputOptions = { +export type RunOptions = { format: OUTPUT_FORMAT; normalizeSeverity: boolean; + runDfa: boolean; + sfdxVersion: string; } export interface RuleManager { @@ -34,5 +36,5 @@ export interface RuleManager { /** * @param engineOptions - see RuleEngine#run */ - runRulesMatchingCriteria(filters: RuleFilter[], target: string[], outputOptions: OutputOptions, engineOptions: Map, runDfa?: boolean): Promise; + runRulesMatchingCriteria(filters: RuleFilter[], target: string[], runOptions: RunOptions, engineOptions: Map): Promise; } diff --git a/src/lib/ScannerRunCommand.ts b/src/lib/ScannerRunCommand.ts index f6f8f8d4c..4e4fb69e3 100644 --- a/src/lib/ScannerRunCommand.ts +++ b/src/lib/ScannerRunCommand.ts @@ -4,7 +4,7 @@ import {ScannerCommand} from './ScannerCommand'; import {RecombinedRuleResults} from '../types'; import {RunOutputProcessor} from './util/RunOutputProcessor'; import {Controller} from '../Controller'; -import {OUTPUT_FORMAT, OutputOptions} from './RuleManager'; +import {OUTPUT_FORMAT, RunOptions} from './RuleManager'; import untildify = require('untildify'); import normalize = require('normalize-path'); @@ -32,9 +32,11 @@ export abstract class ScannerRunCommand extends ScannerCommand { // We need to derive the output format, either from information that was explicitly provided or from default values. // We can't use the defaultValue property for the flag, because there needs to be a behavioral differenec between // defaulting to a value and having the user explicitly select it. - const outputOptions: OutputOptions = { + const runOptions: RunOptions = { format: this.determineOutputFormat(), - normalizeSeverity: normalizeSeverity + normalizeSeverity: normalizeSeverity, + runDfa: this.pathBasedEngines(), + sfdxVersion: this.config.version }; const ruleManager = await Controller.createRuleManager(); @@ -48,7 +50,7 @@ export abstract class ScannerRunCommand extends ScannerCommand { let output: RecombinedRuleResults = null; try { - output = await ruleManager.runRulesMatchingCriteria(filters, targetPaths, outputOptions, engineOptions, this.pathBasedEngines()); + output = await ruleManager.runRulesMatchingCriteria(filters, targetPaths, runOptions, engineOptions); } catch (e) { // Rethrow any errors as SFDX errors. const message: string = e instanceof Error ? e.message : e as string; @@ -56,7 +58,7 @@ export abstract class ScannerRunCommand extends ScannerCommand { } return new RunOutputProcessor({ - format: outputOptions.format, + format: runOptions.format, severityForError: this.flags['severity-threshold'] as number, outfile: this.flags.outfile as string }, this.ux) diff --git a/src/lib/cpd/CpdWrapper.ts b/src/lib/cpd/CpdWrapper.ts index b42c18231..bb1199604 100644 --- a/src/lib/cpd/CpdWrapper.ts +++ b/src/lib/cpd/CpdWrapper.ts @@ -3,7 +3,7 @@ import {FileHandler} from '../util/FileHandler'; import * as JreSetupManager from './../JreSetupManager'; import path = require('path'); import { PMD_LIB } from '../../Constants'; -import { CommandLineSupport } from '../services/CommandLineSupport'; +import { CommandLineSupport, CommandLineResultHandler, ResultHandlerArgs } from '../services/CommandLineSupport'; const MAIN_CLASS = 'net.sourceforge.pmd.cpd.CPD'; const HEAP_SIZE = '-Xmx1024m'; @@ -72,6 +72,10 @@ export default class CpdWrapper extends CommandLineSupport { return super.runCommand(); } + protected handleResults(args: ResultHandlerArgs): void { + new CommandLineResultHandler().handleResults(args); + } + constructor(options: CpdWrapperOptions) { super(options); this.path = options.path; diff --git a/src/lib/eslint/BaseEslintEngine.ts b/src/lib/eslint/BaseEslintEngine.ts index f20b7ce06..a10e2702e 100644 --- a/src/lib/eslint/BaseEslintEngine.ts +++ b/src/lib/eslint/BaseEslintEngine.ts @@ -1,7 +1,7 @@ import {Logger, SfdxError} from '@salesforce/core'; import { Catalog, ESRuleConfig, ESRuleConfigValue, LooseObject, Rule, RuleGroup, RuleResult, RuleTarget, ESRule, TargetPattern, ESRuleMetadata } from '../../types'; import {ENGINE, Severity} from '../../Constants'; -import {OutputProcessor} from '../pmd/OutputProcessor'; +import {OutputProcessor} from '../services/OutputProcessor'; import {AbstractRuleEngine} from '../services/RuleEngine'; import {Config} from '../util/Config'; import {Controller} from '../../Controller'; diff --git a/src/lib/eslint/TypescriptEslintStrategy.ts b/src/lib/eslint/TypescriptEslintStrategy.ts index 4436382de..64c40ea0b 100644 --- a/src/lib/eslint/TypescriptEslintStrategy.ts +++ b/src/lib/eslint/TypescriptEslintStrategy.ts @@ -4,7 +4,7 @@ import {FileHandler} from '../util/FileHandler'; import {ENGINE, LANGUAGE, HARDCODED_RULES} from '../../Constants'; import {ESRule, ESRuleConfigValue, ESRuleConfig, RuleViolation} from '../../types'; import { Logger, Messages, SfdxError } from '@salesforce/core'; -import { OutputProcessor } from '../pmd/OutputProcessor'; +import { OutputProcessor } from '../services/OutputProcessor'; import {deepCopy} from '../util/Utils'; import { rules } from '@typescript-eslint/eslint-plugin'; import {EslintStrategyHelper, ProcessRuleViolationType, RuleDefaultStatus} from './EslintCommons'; diff --git a/src/lib/formatter/RuleResultRecombinator.ts b/src/lib/formatter/RuleResultRecombinator.ts index 1c87daa8c..1abf62073 100644 --- a/src/lib/formatter/RuleResultRecombinator.ts +++ b/src/lib/formatter/RuleResultRecombinator.ts @@ -1,7 +1,7 @@ import {SfdxError} from '@salesforce/core'; import * as path from 'path'; import {EngineExecutionSummary, RecombinedData, RecombinedRuleResults, RuleResult, RuleViolation} from '../../types'; -import {DfaEngineFilters} from '../../Constants'; +import {DfaEngineFilters, ENGINE} from '../../Constants'; import {OUTPUT_FORMAT} from '../RuleManager'; import * as wrap from 'word-wrap'; import {FileHandler} from '../util/FileHandler'; @@ -37,7 +37,7 @@ type DfaTableRow = BaseTableRow & { export class RuleResultRecombinator { - public static async recombineAndReformatResults(results: RuleResult[], format: OUTPUT_FORMAT, executedEngines: Set): Promise { + public static async recombineAndReformatResults(results: RuleResult[], format: OUTPUT_FORMAT, executedEngines: Set, verboseViolations = false): Promise { // We need to change the results we were given into the desired final format. let formattedResults: string | {columns; rows} = null; switch (format) { @@ -45,10 +45,10 @@ export class RuleResultRecombinator { formattedResults = await this.constructCsv(results, executedEngines); break; case OUTPUT_FORMAT.HTML: - formattedResults = await this.constructHtml(results, executedEngines); + formattedResults = await this.constructHtml(results, executedEngines, verboseViolations); break; case OUTPUT_FORMAT.JSON: - formattedResults = this.constructJson(results); + formattedResults = this.constructJson(results, verboseViolations); break; case OUTPUT_FORMAT.JUNIT: formattedResults = this.constructJunit(results); @@ -344,14 +344,30 @@ URL: ${url}`; return {columns, rows}; } - private static constructJson(results: RuleResult[]): string { + private static constructJson(results: RuleResult[], verboseViolations = false): string { if (results.length === 0) { return ''; } + + if (verboseViolations) { + const resultsVerbose = JSON.parse(JSON.stringify(results)) as RuleResult[]; + for (const result of resultsVerbose) { + if (result.engine === ENGINE.RETIRE_JS) { + for (const violation of result.violations) { + // in the json format we need to replace new lines in the message + // for the first line (ending with a colon) we will replace it with a space + // for following lines, we will replace it with a semicolon and a space + violation.message = violation.message.replace(/:\n/g, ': ').replace(/\n/g, '; '); + } + } + } + return JSON.stringify(resultsVerbose.filter(r => r.violations.length > 0)); + } + return JSON.stringify(results.filter(r => r.violations.length > 0)); } - private static async constructHtml(results: RuleResult[], executedEngines: Set): Promise { + private static async constructHtml(results: RuleResult[], executedEngines: Set, verboseViolations = false): Promise { // If the results were just an empty string, we can return it. if (results.length === 0) { return ''; @@ -373,7 +389,7 @@ URL: ${url}`; ruleName: v.ruleName, category: v.category, url: v.url, - message: v.message, + message: verboseViolations && result.engine === ENGINE.RETIRE_JS ? v.message.replace(/\n/g, '
') : v.message, //
used for line breaks in html line: v.line, column: v.column, endLine: v.endLine || null, diff --git a/src/lib/pmd/PmdCatalogWrapper.ts b/src/lib/pmd/PmdCatalogWrapper.ts index 37031f5a9..6d77306eb 100644 --- a/src/lib/pmd/PmdCatalogWrapper.ts +++ b/src/lib/pmd/PmdCatalogWrapper.ts @@ -1,10 +1,9 @@ import {Logger, Messages} from '@salesforce/core'; -import {ChildProcessWithoutNullStreams} from "child_process"; import {Catalog} from '../../types'; import {FileHandler} from '../util/FileHandler'; import * as PrettyPrinter from '../util/PrettyPrinter'; import * as JreSetupManager from './../JreSetupManager'; -import {OutputProcessor} from './OutputProcessor'; +import {ResultHandlerArgs} from '../services/CommandLineSupport'; import * as PmdLanguageManager from './PmdLanguageManager'; import {PmdSupport} from './PmdSupport'; import { PMD_LIB } from '../../Constants'; @@ -21,7 +20,6 @@ const PMD_CATALOGER_LIB = path.join(__dirname, '..', '..', '..', 'dist', 'pmd-ca const MAIN_CLASS = 'sfdc.sfdx.scanner.pmd.Main'; export class PmdCatalogWrapper extends PmdSupport { - private outputProcessor: OutputProcessor; private logger: Logger; // TODO: add relevant trace logs private initialized: boolean; private sfdxScannerPath: string; @@ -30,8 +28,8 @@ export class PmdCatalogWrapper extends PmdSupport { if (this.initialized) { return; } + await super.init(); this.logger = await Logger.child('PmdCatalogWrapper'); - this.outputProcessor = await OutputProcessor.create({}); this.sfdxScannerPath = Controller.getSfdxScannerPath(); this.initialized = true; @@ -68,7 +66,7 @@ export class PmdCatalogWrapper extends PmdSupport { protected async buildClasspath(): Promise { const catalogerLibs = `${PMD_CATALOGER_LIB}/*`; - const classpathEntries = await super.buildClasspath(); + const classpathEntries = await super.buildSharedClasspath(); classpathEntries.push(catalogerLibs); return classpathEntries; } @@ -151,32 +149,20 @@ export class PmdCatalogWrapper extends PmdSupport { return path.join(PMD_LIB, "pmd-" + language + "-" + PMD_VERSION + ".jar"); } - /** - * Accepts a child process created by child_process.spawn(), and a Promise's resolve and reject function. - * Resolves/rejects the Promise once the child process finishes. - * @param cp - * @param res - * @param rej - */ - protected monitorChildProcess(cp: ChildProcessWithoutNullStreams, res: (string) => void, rej: (string) => void): void { - let stdout = ''; - - // When data is passed back up to us, pop it onto the appropriate string. - cp.stdout.on('data', data => { - stdout += data; - }); - - // When the child process exits, if it exited with a zero code we can resolve, otherwise we'll reject. - cp.on('exit', code => { - this.outputProcessor.processOutput(stdout); - this.logger.trace(`monitorChildProcess has received exit code ${code}`); - if (code === 0) { - res(stdout); - } else { - rej(messages.getMessage('error.external.errorMessageAbove')); - } - }); + protected isSuccessfulExitCode(code: number): boolean { + // By internal convention, 0 indicates success and any non-0 code indicates failure. + return code === 0; } + protected handleResults(args: ResultHandlerArgs): void { + if (this.isSuccessfulExitCode(args.code)) { + args.res(args.stdout); + } else { + // If the process errored out, then one of the Messages logged by the parent class already indicates that. + // So rather than returning stderr (which will be confusing and likely unhelpful, just return a hardcoded + // string indicating that the cause was logged elsewhere. + args.rej(messages.getMessage('error.external.errorMessageAbove')); + } + } } diff --git a/src/lib/pmd/PmdSupport.ts b/src/lib/pmd/PmdSupport.ts index ce663b8a1..93b8f4a20 100644 --- a/src/lib/pmd/PmdSupport.ts +++ b/src/lib/pmd/PmdSupport.ts @@ -14,7 +14,7 @@ export enum Format { export abstract class PmdSupport extends CommandLineSupport { - protected async buildClasspath(): Promise { + protected async buildSharedClasspath(): Promise { // Include PMD libs into classpath const pmdLibs = `${PMD_LIB}/*`; const classpathEntries = [pmdLibs]; @@ -28,15 +28,6 @@ export abstract class PmdSupport extends CommandLineSupport { return classpathEntries; } - protected isSuccessfulExitCode(code: number): boolean { - // PMD's convention is that an exit code of 0 indicates a successful run with no violations, and an exit code of - // 4 indicates a successful run with at least one violation. - return code === 0 || code === 4; - } - - - protected abstract buildCommandArray(): Promise<[string, string[]]>; - protected async getCustomRulePathEntries(): Promise>> { const customRulePathManager = await Controller.createRulePathManager(); return customRulePathManager.getRulePathEntries(PmdEngine.ENGINE_NAME); diff --git a/src/lib/pmd/PmdWrapper.ts b/src/lib/pmd/PmdWrapper.ts index 56098c9c4..0878302fa 100644 --- a/src/lib/pmd/PmdWrapper.ts +++ b/src/lib/pmd/PmdWrapper.ts @@ -2,6 +2,7 @@ import {Logger} from '@salesforce/core'; import {Format, PmdSupport} from './PmdSupport'; import * as JreSetupManager from './../JreSetupManager'; import path = require('path'); +import { CommandLineResultHandler, ResultHandlerArgs } from '../services/CommandLineSupport'; import {FileHandler} from '../util/FileHandler'; const MAIN_CLASS = 'net.sourceforge.pmd.PMD'; @@ -33,6 +34,10 @@ export default class PmdWrapper extends PmdSupport { this.initialized = true; } + protected async buildClasspath(): Promise { + return super.buildSharedClasspath(); + } + public static async execute(path: string, rules: string, reportFormat?: Format, reportFile?: string): Promise { const myPmd = await PmdWrapper.create({ path: path, @@ -62,7 +67,7 @@ export default class PmdWrapper extends PmdSupport { // Start with the arguments we know we'll always need. // NOTE: If we were going to run this command from the CLI directly, then we'd wrap the classpath in quotes, but this // is intended for child_process.spawn(), which freaks out if you do that. - const classpath = await super.buildClasspath(); + const classpath = await this.buildClasspath(); // Operating systems impose limits on the maximum length of a command line invocation. This can be problematic // when scannning a large number of files. Store the list of files to scan in a temp file. Pass the location // of the temp file to PMD. The temp file is cleaned up when the process exits. @@ -84,4 +89,13 @@ export default class PmdWrapper extends PmdSupport { return [command, args]; } + protected isSuccessfulExitCode(code: number): boolean { + // PMD's convention is that an exit code of 0 indicates a successful run with no violations, and an exit code of + // 4 indicates a successful run with at least one violation. + return code === 0 || code === 4; + } + + protected handleResults(args: ResultHandlerArgs): void { + new CommandLineResultHandler().handleResults(args); + } } diff --git a/src/lib/retire-js/RetireJsEngine.ts b/src/lib/retire-js/RetireJsEngine.ts index 88b9204de..87d0875eb 100644 --- a/src/lib/retire-js/RetireJsEngine.ts +++ b/src/lib/retire-js/RetireJsEngine.ts @@ -10,6 +10,8 @@ import * as engineUtils from '../util/CommonEngineUtils'; import cspawn = require('cross-spawn'); import path = require('path'); import StreamZip = require('node-stream-zip'); +import {CUSTOM_CONFIG} from '../../Constants'; + // Unlike the other engines we use, RetireJS doesn't really have "rules" per se. So we sorta have to synthesize a @@ -49,6 +51,8 @@ export type RetireJsInvocation = { */ type RetireJsVulnerability = { severity: string; + identifiers?: { [name: string]: string }; + info?: string[]; }; type RetireJsResult = { @@ -172,7 +176,7 @@ export class RetireJsEngine extends AbstractRuleEngine { const retireJsPromises: Promise[] = []; for (const invocation of invocationArray) { - retireJsPromises.push(this.executeRetireJs(invocation)); + retireJsPromises.push(this.executeRetireJs(invocation, engineOptions.has(CUSTOM_CONFIG.VerboseViolations))); } // We can combine the results into a single array using .reduce() instead of the more verbose for-loop. @@ -199,7 +203,7 @@ export class RetireJsEngine extends AbstractRuleEngine { return invocationArray; } - private async executeRetireJs(invocation: RetireJsInvocation): Promise { + private async executeRetireJs(invocation: RetireJsInvocation, verboseViolations: boolean): Promise { return new Promise((res, rej) => { const cp = cspawn(RetireJsEngine.NODE_EXEC_PATH, invocation.args); @@ -225,7 +229,8 @@ export class RetireJsEngine extends AbstractRuleEngine { } else if (code === 13) { // If RetireJS exits with code 13, then it ran successfully, but found at least one vulnerability. // Convert the output into RuleResult objects and resolve to that. - res(this.processOutput(stdout, invocation.rule)); + res(this.processOutput(stdout, invocation.rule, verboseViolations)); + } else { // If RetireJS exits with any other code, then it means something went wrong. The error could be // contained in either stdout or stderr, so we'll send them both to a method for processing, and @@ -261,7 +266,7 @@ export class RetireJsEngine extends AbstractRuleEngine { return stderr; } - private processOutput(cmdOutput: string, ruleName: string): RuleResult[] { + private processOutput(cmdOutput: string, ruleName: string, verboseViolations:boolean): RuleResult[] { // The output should be a valid result JSON. try { const outputJson: RetireJsOutput = RetireJsEngine.convertStringToResultObj(cmdOutput); @@ -282,6 +287,9 @@ export class RetireJsEngine extends AbstractRuleEngine { // Each `result` entry generates one RuleViolation. for (const result of data.results) { + + const message: string = verboseViolations ? this.generateVerboseMessage(result) : `${result.component} v${result.version} is insecure. Please upgrade to latest version.`; + ruleResult.violations.push({ line: 1, column: 1, @@ -290,7 +298,7 @@ export class RetireJsEngine extends AbstractRuleEngine { severity: result.vulnerabilities .map(vuln => this.retireSevToScannerSev(vuln.severity)) .reduce((min, sev) => min > sev ? sev: min, 9000), - message: `${result.component} v${result.version} is insecure. Please upgrade to latest version.`, + message: message, category: 'Insecure Dependencies' }); } @@ -307,6 +315,29 @@ export class RetireJsEngine extends AbstractRuleEngine { } } + private generateVerboseMessage(result: RetireJsResult): string { + const messageLines: string[] = [`${result.component} ${result.version} has known vulnerabilities:`]; + + // Each `vulnerability` generates a new line in the RuleViolation's message + for (const vuln of result.vulnerabilities) { + + // Array of all identifiers with starting with severity and summary (if applicable) + const vulnMessageItems: string[] = []; + + for (const identifier in vuln.identifiers) { + const text = `${identifier}: ${vuln.identifiers[identifier]}`; + identifier === "summary" ? vulnMessageItems.unshift(text) : vulnMessageItems.push(text); + } + + vulnMessageItems.unshift(`severity: ${vuln.severity}`); // unshift after other identifiers so severity is first + vulnMessageItems.push(vuln.info.join(" ")); // list info elements separated by space + messageLines.push(`${vulnMessageItems.join("; ")}`) + + } + + return messageLines.join("\n"); + } + private retireSevToScannerSev(sev: string): number { switch (sev.toLowerCase()) { case 'low': diff --git a/src/lib/services/CommandLineSupport.ts b/src/lib/services/CommandLineSupport.ts index 62aa9de37..4ad203958 100644 --- a/src/lib/services/CommandLineSupport.ts +++ b/src/lib/services/CommandLineSupport.ts @@ -1,13 +1,34 @@ import { Logger } from '@salesforce/core'; import {AsyncCreatable} from '@salesforce/kit'; -import {ChildProcessWithoutNullStreams} from 'child_process'; import cspawn = require('cross-spawn'); +import {OutputProcessor} from './OutputProcessor'; import {SpinnerManager, NoOpSpinnerManager} from './SpinnerManager'; + +export type ResultHandlerArgs = { + code: number; + isSuccess: boolean; + stdout: string; + stderr: string; + res: (string) => void; + rej: (string) => void; +}; + +export class CommandLineResultHandler { + public handleResults(args: ResultHandlerArgs): void { + if (args.isSuccess) { + args.res(args.stdout); + } else { + args.rej(args.stderr); + } + } +} + export abstract class CommandLineSupport extends AsyncCreatable { private parentLogger: Logger; private parentInitialized: boolean; + private outputProcessor: OutputProcessor; protected async init(): Promise { @@ -16,6 +37,7 @@ export abstract class CommandLineSupport extends AsyncCreatable { } this.parentLogger = await Logger.child('CommandLineSupport'); + this.outputProcessor = await OutputProcessor.create({}); this.parentInitialized = true; } @@ -32,38 +54,12 @@ export abstract class CommandLineSupport extends AsyncCreatable { } /** - * Accepts a child process created by child_process.spawn(), and a Promise's resolve and reject functions. - * Resolves/rejects the Promise once the child process finishes. - * @param cp - * @param res - * @param rej + * Perform any job-specific processing on the results of a child process execution, and either resolve or reject as + * needed. + * @param args + * @protected */ - protected monitorChildProcess(cp: ChildProcessWithoutNullStreams, res: (string) => void, rej: (string) => void): void { - let stdout = ''; - let stderr = ''; - this.getSpinnerManager().startSpinner(); - - // When data is passed back up to us, pop it onto the appropriate string. - cp.stdout.on('data', data => { - stdout += data; - }); - cp.stderr.on('data', data => { - stderr += data; - }); - - cp.on('exit', code => { - this.parentLogger.trace(`monitorChildProcess has received exit code ${code}`); - if (this.isSuccessfulExitCode(code)) { - this.getSpinnerManager().stopSpinner(true); - res(stdout); - } else { - // If we got any other error, it means something actually went wrong. We'll just reject with stderr for - // the ease of upstream error handling. - this.getSpinnerManager().stopSpinner(false); - rej(stderr); - } - }); - } + protected abstract handleResults(args: ResultHandlerArgs): void; protected abstract isSuccessfulExitCode(code: number): boolean; @@ -74,7 +70,34 @@ export abstract class CommandLineSupport extends AsyncCreatable { return new Promise((res, rej) => { const cp = cspawn.spawn(command, args); - this.monitorChildProcess(cp, res, rej); + + let stdout = ''; + let stderr = ''; + this.getSpinnerManager().startSpinner(); + + // When data is passed back up to us, pop it onto the appropriate string. + cp.stdout.on('data', data => { + stdout += data; + }); + cp.stderr.on('data', data => { + stderr += data; + }); + + cp.on('exit', code => { + this.parentLogger.trace(`runCommand has received exit code ${code}`); + const isSuccess = this.isSuccessfulExitCode(code); + this.getSpinnerManager().stopSpinner(isSuccess); + // The output processor's input is always stdout. + this.outputProcessor.processOutput(stdout); + this.handleResults({ + code, + isSuccess, + stdout, + stderr, + res, + rej + }); + }); }); } } diff --git a/src/lib/services/LocalCatalog.ts b/src/lib/services/LocalCatalog.ts index d8e9fcbef..7193bffe6 100644 --- a/src/lib/services/LocalCatalog.ts +++ b/src/lib/services/LocalCatalog.ts @@ -3,7 +3,7 @@ import * as path from 'path'; import {injectable} from 'tsyringe'; import {CATALOG_FILE} from '../../Constants'; import {Catalog, Rule, RuleEvent, RuleGroup} from '../../types'; -import {OutputProcessor} from '../pmd/OutputProcessor'; +import {OutputProcessor} from './OutputProcessor'; import {isRuleGroupFilter, RuleFilter} from '../RuleFilter'; import {FileHandler} from '../util/FileHandler'; import * as PrettyPrinter from '../util/PrettyPrinter'; diff --git a/src/lib/pmd/OutputProcessor.ts b/src/lib/services/OutputProcessor.ts similarity index 100% rename from src/lib/pmd/OutputProcessor.ts rename to src/lib/services/OutputProcessor.ts diff --git a/src/lib/sfge/SfgeEngine.ts b/src/lib/sfge/SfgeEngine.ts index a18c30fcc..09de49885 100644 --- a/src/lib/sfge/SfgeEngine.ts +++ b/src/lib/sfge/SfgeEngine.ts @@ -135,18 +135,26 @@ export class SfgeEngine extends AbstractRuleEngine { * @param engineOptions - A mapping of keys to values for engineOptions. Not all key/value pairs will apply to all engines. */ public async run(ruleGroups: RuleGroup[], rules: Rule[], targets: RuleTarget[], engineOptions: Map): Promise { - // Pull all targeted paths out of our target descriptors. - const targetPaths: string[] = targets.reduce((accumulator: string[], target: RuleTarget) => {return [...accumulator, ...target.paths]}, []); + // Make sure we have actual targets to run against. + let targetCount = 0; + targets.forEach((t) => { + if (t.methods.length > 0) { + // If we're targeting individual methods, then each method is counted as a separate target for this purpose. + targetCount += t.methods.length; + } else { + targetCount += t.paths.length; + } + }); - if (targetPaths.length === 0) { - this.logger.trace(`No targets for ${SfgeEngine.ENGINE_NAME} found. Nothing to execute. Returning early.`); + if (targetCount === 0) { + this.logger.trace(`No targets from ${SfgeEngine.ENGINE_NAME} found. Nothing to execute. Returning early.`); return []; } - this.logger.trace(`About to run ${SfgeEngine.ENGINE_NAME} rules. Targets: ${targetPaths.length}, Selected rules: ${JSON.stringify(rules)}`); + this.logger.trace(`About to run ${SfgeEngine.ENGINE_NAME} rules. Targets: ${targetCount} files and/or methods, Selected rules: ${JSON.stringify(rules)}`); try { - const output = await SfgeWrapper.runSfge(targetPaths, rules, JSON.parse(engineOptions.get(CUSTOM_CONFIG.SfgeConfig)) as SfgeConfig); + const output = await SfgeWrapper.runSfge(targets, rules, JSON.parse(engineOptions.get(CUSTOM_CONFIG.SfgeConfig)) as SfgeConfig); // TODO: There should be some kind of method-call here to pull logs and warnings from the output. const results = this.processStdout(output); diff --git a/src/lib/sfge/SfgeWrapper.ts b/src/lib/sfge/SfgeWrapper.ts index 076a4289b..723516852 100644 --- a/src/lib/sfge/SfgeWrapper.ts +++ b/src/lib/sfge/SfgeWrapper.ts @@ -4,8 +4,8 @@ import {AsyncCreatable} from '@salesforce/kit'; import {Controller} from '../../Controller'; import * as JreSetupManager from '../JreSetupManager'; import {uxEvents, EVENTS} from '../ScannerEvents'; -import {Rule, SfgeConfig} from '../../types'; -import {CommandLineSupport} from '../services/CommandLineSupport'; +import {Rule, SfgeConfig, RuleTarget} from '../../types'; +import {CommandLineSupport, CommandLineResultHandler, ResultHandlerArgs} from '../services/CommandLineSupport'; import {SpinnerManager, NoOpSpinnerManager} from '../services/SpinnerManager'; import {FileHandler} from '../util/FileHandler'; @@ -27,7 +27,7 @@ const EXIT_NO_VIOLATIONS = 0; const EXIT_WITH_VIOLATIONS = 4; interface SfgeWrapperOptions { - targetFiles: string[]; + targets: RuleTarget[]; projectDirs: string[]; command: string; rules: Rule[]; @@ -42,6 +42,12 @@ type SfgeTarget = { targetMethods: string[]; }; +type SfgeInput = { + targets: SfgeTarget[]; + projectDirs: string[]; + rulesToRun: string[]; +}; + class SfgeSpinnerManager extends AsyncCreatable implements SpinnerManager { private initialized: boolean; private intervalId: NodeJS.Timeout; @@ -76,7 +82,7 @@ export class SfgeWrapper extends CommandLineSupport { private logger: Logger; private initialized: boolean; private fh: FileHandler; - private targetFiles: string[]; + private targets: RuleTarget[]; private projectDirs: string[]; private command: string; private rules: Rule[]; @@ -88,7 +94,7 @@ export class SfgeWrapper extends CommandLineSupport { constructor(options: SfgeWrapperOptions) { super(options); - this.targetFiles = options.targetFiles; + this.targets = options.targets; this.projectDirs = options.projectDirs; this.command = options.command; this.rules = options.rules; @@ -113,9 +119,9 @@ export class SfgeWrapper extends CommandLineSupport { return Promise.resolve([`${SFGE_LIB}/*`]); } - private async createInputFile(targetFiles: string[]): Promise { + private async createInputFile(input: SfgeInput): Promise { const inputFile = await this.fh.tmpFileWithCleanup(); - await this.fh.writeFile(inputFile, targetFiles.join('\n')); + await this.fh.writeFile(inputFile, JSON.stringify(input)); return inputFile; } @@ -123,6 +129,10 @@ export class SfgeWrapper extends CommandLineSupport { return code === EXIT_NO_VIOLATIONS || code === EXIT_WITH_VIOLATIONS; } + protected handleResults(args: ResultHandlerArgs) { + new CommandLineResultHandler().handleResults(args); + } + /** * Returns a spinner that will be used while waiting for the child process to complete. For the CATALOG flow, this will * be a {@link NoOpSpinnerManager}, and for the EXECUTE flow, this will be a {@link SfgeSpinnerManager}. @@ -138,13 +148,11 @@ export class SfgeWrapper extends CommandLineSupport { const command = path.join(javaHome, 'bin', 'java'); const classpath = await this.buildClasspath(); - const targetListFile = await this.createInputFile([this.createTargetJsons()]); - const sourceListFile = await this.createInputFile(this.projectDirs); - const rulesToRun = this.rules.map(rule => rule.name).join(','); + const inputObject: SfgeInput = this.createInputJson(); + const inputFile = await this.createInputFile(inputObject); - this.logger.trace(`Stored the names of ${this.targetFiles.length} targeted files in ${targetListFile}`); - this.logger.trace(`Stored the names of ${this.projectDirs.length} source directories in ${sourceListFile}`); - this.logger.trace(`Rules to be executed: ${rulesToRun}`); + this.logger.trace(`Stored the names of ${this.targets.length} targeted files and ${this.projectDirs.length} source directories in ${inputFile}`); + this.logger.trace(`Rules to be executed: ${JSON.stringify(inputObject.rulesToRun)}`); const args = [`-Dsfge_log_name=${this.logFilePath}`, '-cp', classpath.join(path.delimiter)]; if (this.ruleThreadCount != null) { @@ -156,7 +164,7 @@ export class SfgeWrapper extends CommandLineSupport { if (this.ignoreParseErrors != null) { args.push(`-DSFGE_IGNORE_PARSE_ERRORS=${this.ignoreParseErrors.toString()}`); } - args.push(MAIN_CLASS, this.command, targetListFile, sourceListFile, rulesToRun); + args.push(MAIN_CLASS, this.command, inputFile); this.logger.trace(`Preparing to execute sfge with command: "${command}", args: "${JSON.stringify(args)}"`); return [command, args]; @@ -168,7 +176,7 @@ export class SfgeWrapper extends CommandLineSupport { public static async getCatalog() { const wrapper = await SfgeWrapper.create({ - targetFiles: [], + targets: [], projectDirs: [], command: CATALOG_COMMAND, rules: [], @@ -178,21 +186,39 @@ export class SfgeWrapper extends CommandLineSupport { return wrapper.execute(); } - private createTargetJsons(): string { - // TODO: For now, the target files can only be file-level instead of method-level. When that changes, this code - // will change too. - const targetJsons: SfgeTarget[] = this.targetFiles.map(t => { - return { - targetFile: t, - targetMethods: [] - }; + private createInputJson(): SfgeInput { + const inputJson: SfgeInput = { + targets: [], + projectDirs: this.projectDirs, + rulesToRun: this.rules.map(rule => rule.name) + }; + this.targets.forEach(t => { + // If the target specifies individual methods in a file, then create one object encompassing the file and + // those methods. + // NOTE: This code assumes that method-level targets cannot have multiple paths in the `paths` property. If + // this assumption is ever invalidated, then this code must change. + if (t.methods.length > 0) { + inputJson.targets.push({ + targetFile: t.paths[0], + targetMethods: t.methods + }); + } else { + // Otherwise, the target is a collection of paths encompassing whole files, and each path should be its + // own subject. + t.paths.forEach(p => { + inputJson.targets.push({ + targetFile: p, + targetMethods: [] + }); + }); + } }); - return JSON.stringify(targetJsons); + return inputJson; } - public static async runSfge(targetPaths: string[], rules: Rule[], sfgeConfig: SfgeConfig): Promise { + public static async runSfge(targets: RuleTarget[], rules: Rule[], sfgeConfig: SfgeConfig): Promise { const wrapper = await SfgeWrapper.create({ - targetFiles: targetPaths, + targets, projectDirs: sfgeConfig.projectDirs, command: EXEC_COMMAND, rules: rules, diff --git a/src/lib/util/EventCreator.ts b/src/lib/util/EventCreator.ts index 214179edd..8eee00211 100644 --- a/src/lib/util/EventCreator.ts +++ b/src/lib/util/EventCreator.ts @@ -1,4 +1,4 @@ -import {OutputProcessor} from '../pmd/OutputProcessor'; +import {OutputProcessor} from '../services/OutputProcessor'; import {AsyncCreatable} from '@salesforce/kit'; export class EventCreator extends AsyncCreatable { @@ -17,13 +17,13 @@ export class EventCreator extends AsyncCreatable { public createUxInfoAlwaysMessage(eventTemplateKey: string, args: string[]): void { const event = { - messageKey: eventTemplateKey, - args: args, - type: 'INFO', - handler: 'UX', - verbose: false, + messageKey: eventTemplateKey, + args: args, + type: 'INFO', + handler: 'UX', + verbose: false, time: Date.now() }; this.outputProcessor.emitEvents([event]); } -} \ No newline at end of file +} diff --git a/src/lib/util/FileHandler.ts b/src/lib/util/FileHandler.ts index d5cbc0c5a..e0adaa8b7 100644 --- a/src/lib/util/FileHandler.ts +++ b/src/lib/util/FileHandler.ts @@ -25,6 +25,10 @@ export class FileHandler { return await this.exists(filename) && (await this.stats(filename)).isDirectory(); } + async isFile(filename: string): Promise { + return await this.exists(filename) && (await this.stats(filename)).isFile(); + } + readDir(filename: string): Promise { return fs.readdir(filename); } diff --git a/src/lib/util/SfdxUtils.ts b/src/lib/util/SfdxUtils.ts new file mode 100644 index 000000000..3dcf9ed15 --- /dev/null +++ b/src/lib/util/SfdxUtils.ts @@ -0,0 +1,48 @@ +import childProcess = require('child_process'); +/** + * A variable to store SFDX's version number, so we don't have to keep re-running `sfdx-v` + */ +let SFDX_VERSION: string; + + +/** + * Returns the current version of SFDX installed on the machine, or "unknown" if the version cannot be determined. + */ +export async function getSfdxVersion(): Promise { + // If we already have a cached value, we can just return that instead of doing anything else. + if (SFDX_VERSION !== undefined) { + return SFDX_VERSION; + } + + // Get the output of `sfdx -v`. + let rawVersionString: string; + try { + rawVersionString = await new Promise((res, rej) => { + childProcess.exec('sfdx -v', (err, stdout, stderr) => { + if (err) { + rej(stderr); + } else { + res(stdout); + } + }); + }); + } catch (e) { + // If the command fails, then we have no way of determining what the version is. So just set it to 'unknown' and + // be done with it. + SFDX_VERSION = 'unknown'; + return SFDX_VERSION; + } + + // The actual output for `sfdx -v` is a long-ish string that has stuff we don't want. So use this regex to just get + // the SFDX version part. + const regex = /(sfdx-cli\/\d+\.\d+\.\d+)/g; + const match = regex.exec(rawVersionString); + if (match.length > 0) { + SFDX_VERSION = match[0]; + } else { + // Even if our regex didn't pull out the value that we expected it to, we can still use the results of the command. + // It'll just be more verbose than is strictly necessary, which isn't the worst thing in the world. + SFDX_VERSION = rawVersionString; + } + return SFDX_VERSION; +} diff --git a/src/types.d.ts b/src/types.d.ts index d782ab528..e7f733fcf 100644 --- a/src/types.d.ts +++ b/src/types.d.ts @@ -16,6 +16,15 @@ export type Rule = { url?: string; } +export type TelemetryData = { + eventName: string; + executedEnginesCount: number; + executedEnginesString: string; + sfdxVersion: string; + // eslint-disable-next-line @typescript-eslint/no-explicit-any + [key: string]: any; +} + export type LooseObject = { /* eslint-disable-next-line @typescript-eslint/no-explicit-any */ [key: string]: any; @@ -44,6 +53,7 @@ export type RuleTarget = { target: string; isDirectory?: boolean; paths: string[]; + methods?: string[]; } export type RuleResult = { engine: string; diff --git a/test/lib/RuleManager.test.ts b/test/lib/RuleManager.test.ts index 2695dc659..c805cd6a9 100644 --- a/test/lib/RuleManager.test.ts +++ b/test/lib/RuleManager.test.ts @@ -1,22 +1,26 @@ import {expect} from 'chai'; -import path = require('path'); -import Sinon = require('sinon'); + +import {Lifecycle} from '@salesforce/core'; import {Controller} from '../../src/Controller'; -import {Rule, RuleGroup, RuleTarget} from '../../src/types'; +import {Rule, RuleGroup, RuleTarget, TelemetryData} from '../../src/types'; +import {ENGINE} from '../../src/Constants'; import {CategoryFilter, EngineFilter, LanguageFilter, RuleFilter, RulesetFilter} from '../../src/lib/RuleFilter'; import {DefaultRuleManager} from '../../src/lib/DefaultRuleManager'; -import {OUTPUT_FORMAT, RuleManager} from '../../src/lib/RuleManager'; -import {uxEvents, EVENTS} from '../../src/lib/ScannerEvents'; +import {OUTPUT_FORMAT, RuleManager, RunOptions} from '../../src/lib/RuleManager'; +import {EVENTS, uxEvents} from '../../src/lib/ScannerEvents'; import {RuleCatalog} from '../../src/lib/services/RuleCatalog'; import {RuleEngine} from '../../src/lib/services/RuleEngine'; import {RetireJsEngine} from '../../src/lib/retire-js/RetireJsEngine'; +import {SfgeEngine} from '../../src/lib/sfge/SfgeEngine'; import * as TestOverrides from '../test-related-lib/TestOverrides'; import * as TestUtils from '../TestUtils'; +import path = require('path'); +import Sinon = require('sinon'); TestOverrides.initializeTestSetup(); @@ -25,10 +29,12 @@ const EMPTY_ENGINE_OPTIONS = new Map(); describe('RuleManager', () => { let uxSpy; + let telemetrySpy; beforeEach(() => { Sinon.createSandbox(); uxSpy = Sinon.spy(uxEvents, 'emit'); + telemetrySpy = Sinon.spy(Lifecycle.getInstance(), 'emitTelemetry'); }); afterEach(() => { @@ -179,10 +185,16 @@ describe('RuleManager', () => { after(() => { process.chdir("../../.."); }); + const runOptions: RunOptions = { + format: OUTPUT_FORMAT.JSON, + normalizeSeverity: false, + runDfa: false, + sfdxVersion: 'test' + }; describe('Test Case: Run without filters', () => { it('JS project files', async () => { // If we pass an empty list into the method, that's treated as the absence of filter criteria. - const {results} = await ruleManager.runRulesMatchingCriteria([], ['js'], {format: OUTPUT_FORMAT.JSON, normalizeSeverity: false}, EMPTY_ENGINE_OPTIONS); + const {results} = await ruleManager.runRulesMatchingCriteria([], ['js'], runOptions, EMPTY_ENGINE_OPTIONS); let parsedRes = null; if (typeof results !== "string") { expect(false, `Invalid output: ${results}`); @@ -194,11 +206,12 @@ describe('RuleManager', () => { for (const res of parsedRes) { expect(res.violations[0], `Message is ${res.violations[0].message}`).to.have.property("ruleName").that.is.not.null; } + Sinon.assert.callCount(telemetrySpy, 1); }); it('TS project files', async () => { // If we pass an empty list into the method, that's treated as the absence of filter criteria. - const {results} = await ruleManager.runRulesMatchingCriteria([], ['ts'], {format: OUTPUT_FORMAT.JSON, normalizeSeverity: false}, EMPTY_ENGINE_OPTIONS); + const {results} = await ruleManager.runRulesMatchingCriteria([], ['ts'], runOptions, EMPTY_ENGINE_OPTIONS); let parsedRes = null; if (typeof results !== "string") { expect(false, `Invalid output: ${results}`); @@ -210,12 +223,13 @@ describe('RuleManager', () => { for (const res of parsedRes) { expect(res.violations[0], `Message is ${res.violations[0].message}`).to.have.property("ruleName").that.is.not.null; } + Sinon.assert.callCount(telemetrySpy, 1); }); it('App project files', async () => { // If we pass an empty list into the method, that's treated as the absence of filter criteria. - const {results} = await ruleManager.runRulesMatchingCriteria([], ['app'], {format: OUTPUT_FORMAT.JSON, normalizeSeverity: false}, EMPTY_ENGINE_OPTIONS); + const {results} = await ruleManager.runRulesMatchingCriteria([], ['app'], runOptions, EMPTY_ENGINE_OPTIONS); let parsedRes = null; if (typeof results !== "string") { expect(false, `Invalid output: ${results}`); @@ -226,6 +240,7 @@ describe('RuleManager', () => { for (const res of parsedRes) { expect(res.violations[0], `Message is ${res.violations[0]['message']}`).to.have.property("ruleName").that.is.not.null; } + Sinon.assert.callCount(telemetrySpy, 1); }); it('All targets match', async () => { @@ -234,7 +249,7 @@ describe('RuleManager', () => { const categories = ['Possible Errors']; const filters = [new CategoryFilter(categories)]; - const {results} = await ruleManager.runRulesMatchingCriteria(filters, validTargets, {format: OUTPUT_FORMAT.JSON, normalizeSeverity: false}, EMPTY_ENGINE_OPTIONS); + const {results} = await ruleManager.runRulesMatchingCriteria(filters, validTargets, runOptions, EMPTY_ENGINE_OPTIONS); let parsedRes = null; if (typeof results !== "string") { expect(false, `Invalid output: ${results}`); @@ -243,6 +258,7 @@ describe('RuleManager', () => { } expect(parsedRes).to.be.an("array").that.has.length(1); Sinon.assert.callCount(uxSpy, 0); + Sinon.assert.callCount(telemetrySpy, 1); }); it('Single target file does not match', async () => { @@ -250,10 +266,11 @@ describe('RuleManager', () => { // No filters const filters = []; - const {results} = await ruleManager.runRulesMatchingCriteria(filters, invalidTarget, {format: OUTPUT_FORMAT.JSON, normalizeSeverity: false}, EMPTY_ENGINE_OPTIONS); + const {results} = await ruleManager.runRulesMatchingCriteria(filters, invalidTarget, runOptions, EMPTY_ENGINE_OPTIONS); expect(results).to.equal(''); Sinon.assert.calledWith(uxSpy, EVENTS.WARNING_ALWAYS, `Target: '${invalidTarget.join(', ')}' was not processed by any engines.`); + Sinon.assert.callCount(telemetrySpy, 1); }); }); @@ -264,7 +281,7 @@ describe('RuleManager', () => { const filters = [ new CategoryFilter([category])]; - const {results} = await ruleManager.runRulesMatchingCriteria(filters, ['app'], {format: OUTPUT_FORMAT.JSON, normalizeSeverity: false}, EMPTY_ENGINE_OPTIONS); + const {results} = await ruleManager.runRulesMatchingCriteria(filters, ['app'], runOptions, EMPTY_ENGINE_OPTIONS); let parsedRes = null; if (typeof results !== "string") { expect(false, `Invalid output: ${results}`); @@ -279,6 +296,7 @@ describe('RuleManager', () => { expect(violation.category).to.equal(category); } } + Sinon.assert.callCount(telemetrySpy, 1); }); it('Filtering by multiple categories runs any rule in either category', async () => { @@ -286,7 +304,7 @@ describe('RuleManager', () => { const categories = ['Best Practices', 'Error Prone']; const filters = [new CategoryFilter(categories)]; - const {results} = await ruleManager.runRulesMatchingCriteria(filters, ['app'], {format: OUTPUT_FORMAT.JSON, normalizeSeverity: false}, EMPTY_ENGINE_OPTIONS); + const {results} = await ruleManager.runRulesMatchingCriteria(filters, ['app'], runOptions, EMPTY_ENGINE_OPTIONS); let parsedRes = null; if (typeof results !== "string") { expect(false, `Invalid output: ${results}`); @@ -299,17 +317,52 @@ describe('RuleManager', () => { expect(res.violations[0], `Message is ${res.violations[0]['message']}`).to.have.property("ruleName").that.is.not.null; expect(res.violations[0].category).to.be.oneOf(categories); } + Sinon.assert.callCount(telemetrySpy, 1); }); }); + describe('Test Case: Run by engine', () => { + it('Filtering by engine works as expected', async () => { + const engines = [ENGINE.RETIRE_JS, ENGINE.ESLINT]; + const filters = [new EngineFilter(engines)]; + + const {results} = await ruleManager.runRulesMatchingCriteria(filters, ['app'], runOptions, EMPTY_ENGINE_OPTIONS); + let parsedRes = null; + if (typeof results !== 'string') { + expect(false, `Invalid output: ${results}`); + } else { + parsedRes = JSON.parse(results); + } + // This result indicates that not all executed engines found violations, which is what we expected. That's fine. + expect(parsedRes).to.be.an('array').that.has.length(1, 'Wrong number of engines returned violations'); + expect(parsedRes[0].engine).to.equal('eslint', 'Wrong engine returned results'); + expect(parsedRes[0].violations.length).to.equal(1, 'Wrong number of violations found'); + Sinon.assert.callCount(telemetrySpy, 1); + const telemetryArg: TelemetryData = telemetrySpy.args[0][0]; + expect(telemetryArg.eventName).to.equal('ENGINE_EXECUTION'); + expect(telemetryArg.executedEnginesCount).to.equal(2); + expect(telemetryArg.executedEnginesString).to.equal(JSON.stringify(['eslint', 'retire-js'])); + expect(telemetryArg['pmd']).to.equal(false); + expect(telemetryArg['pmd-custom']).to.equal(false); + expect(telemetryArg['eslint']).to.equal(true); + expect(telemetryArg['eslint-lwc']).to.equal(false); + expect(telemetryArg['eslint-typescript']).to.equal(false); + expect(telemetryArg['eslint-custom']).to.equal(false); + expect(telemetryArg['retire-js']).to.equal(true); + expect(telemetryArg['cpd']).to.equal(false); + expect(telemetryArg['sfge']).to.equal(false); + }); + }) + describe('Edge Cases', () => { it('When no rules match the given criteria, an empty string is returned', async () => { // Define our preposterous filter array. const filters = [new CategoryFilter(['beebleborp'])]; - const {results} = await ruleManager.runRulesMatchingCriteria(filters, ['app'], {format: OUTPUT_FORMAT.JSON, normalizeSeverity: false}, EMPTY_ENGINE_OPTIONS); + const {results} = await ruleManager.runRulesMatchingCriteria(filters, ['app'], runOptions, EMPTY_ENGINE_OPTIONS); expect(typeof results).to.equal('string', `Output ${results} should have been a string`); expect(results).to.equal('', `Output ${results} should have been an empty string`); + Sinon.assert.callCount(telemetrySpy, 1); }); it('Single target file does not match', async () => { @@ -318,11 +371,12 @@ describe('RuleManager', () => { const categories = ['Best Practices', 'Error Prone']; const filters = [new CategoryFilter(categories)]; - const {results} = await ruleManager.runRulesMatchingCriteria(filters, invalidTarget, {format: OUTPUT_FORMAT.JSON, normalizeSeverity: false}, EMPTY_ENGINE_OPTIONS); + const {results} = await ruleManager.runRulesMatchingCriteria(filters, invalidTarget, runOptions, EMPTY_ENGINE_OPTIONS); expect(results).to.equal(''); Sinon.assert.callCount(uxSpy, 1); Sinon.assert.calledWith(uxSpy, EVENTS.WARNING_ALWAYS, `Target: '${invalidTarget.join(', ')}' was not processed by any engines.`); + Sinon.assert.callCount(telemetrySpy, 1); }); @@ -332,11 +386,12 @@ describe('RuleManager', () => { const categories = ['Best Practices', 'Error Prone']; const filters = [new CategoryFilter(categories)]; - const {results} = await ruleManager.runRulesMatchingCriteria(filters, invalidTarget, {format: OUTPUT_FORMAT.JSON, normalizeSeverity: false}, EMPTY_ENGINE_OPTIONS); + const {results} = await ruleManager.runRulesMatchingCriteria(filters, invalidTarget, runOptions, EMPTY_ENGINE_OPTIONS); expect(results).to.equal(''); Sinon.assert.callCount(uxSpy, 1); Sinon.assert.calledWith(uxSpy, EVENTS.WARNING_ALWAYS, `Target: '${invalidTarget.join(', ')}' was not processed by any engines.`); + Sinon.assert.callCount(telemetrySpy, 1); }); it('Multiple targets do not match', async () => { @@ -345,11 +400,12 @@ describe('RuleManager', () => { const categories = ['Best Practices', 'Error Prone']; const filters = [new CategoryFilter(categories)]; - const {results} = await ruleManager.runRulesMatchingCriteria(filters, invalidTargets, {format: OUTPUT_FORMAT.JSON, normalizeSeverity: false}, EMPTY_ENGINE_OPTIONS); + const {results} = await ruleManager.runRulesMatchingCriteria(filters, invalidTargets, runOptions, EMPTY_ENGINE_OPTIONS); expect(results).to.equal(''); Sinon.assert.callCount(uxSpy, 1); Sinon.assert.calledWith(uxSpy, EVENTS.WARNING_ALWAYS, `Targets: '${invalidTargets.join(', ')}' were not processed by any engines.`); + Sinon.assert.callCount(telemetrySpy, 1); }); it('Some targets do not match', async () => { @@ -359,7 +415,7 @@ describe('RuleManager', () => { const categories = ['Possible Errors']; const filters = [new CategoryFilter(categories)]; - const {results} = await ruleManager.runRulesMatchingCriteria(filters, [...invalidTargets, ...validTargets], {format: OUTPUT_FORMAT.JSON, normalizeSeverity: false}, EMPTY_ENGINE_OPTIONS); + const {results} = await ruleManager.runRulesMatchingCriteria(filters, [...invalidTargets, ...validTargets], runOptions, EMPTY_ENGINE_OPTIONS); let parsedRes = null; if (typeof results !== "string") { expect(false, `Invalid output: ${results}`); @@ -369,6 +425,7 @@ describe('RuleManager', () => { expect(parsedRes).to.be.an("array").that.has.length(1); Sinon.assert.callCount(uxSpy, 1); Sinon.assert.calledWith(uxSpy, EVENTS.WARNING_ALWAYS, `Targets: '${invalidTargets.join(', ')}' were not processed by any engines.`); + Sinon.assert.callCount(telemetrySpy, 1); }); }); }); @@ -460,6 +517,35 @@ describe('RuleManager', () => { expect(results[0].paths.length).to.equal(2, 'Wrong number of paths matched'); }); + it('Positive method-level targets are properly matched', async () => { + // All tests will use the SFGE engine, since method-level targeting is intended for that engine anyway. + const engine = new SfgeEngine(); + await engine.init(); + + // Targets are all going to be normalized Unix paths, some of which also specify individual methods. + const targetFile1 = 'test/code-fixtures/projects/sfge-working-app/force-app/main/default/classes/AuraEnabledFls.cls'; + const targetMethods1 = ['flsNoEnforcementAttempted', 'flsDoneCorrectly']; + const targetFile2 = 'test/code-fixtures/projects/sfge-working-app/force-app/main/default/classes/VfControllerFls.cls'; + const targetMethods2 = ['flsWrongPermissionChecked']; + const targets = [ + `${targetFile1}#${targetMethods1.join(';')}`, + `${targetFile2}#${targetMethods2.join(';')}` + ]; + + const testRuleManager: UnpackTargetsDRM = new UnpackTargetsDRM(); + await testRuleManager.init(); + + // THIS IS THE INVOCATION OF THE TARGET METHOD! + const results: RuleTarget[] = await testRuleManager.unpackTargets(engine, targets, new Set()); + + // Validate the results. + expect(results.length).to.equal(2, 'Wrong number of targets matched'); + expect(results[0].target).to.equal(targetFile1, 'Expected different first file'); + expect(results[0].methods).to.deep.equal(targetMethods1, 'Expected different first methods'); + expect(results[1].target).to.equal(targetFile2, 'Expected different second file'); + expect(results[1].methods).to.deep.equal(targetMethods2, 'Expected different second methods'); + }); + it('Positive glob-type targets are properly matched', async () => { // All of the tests will use the RetireJS engine, since it's got the most straightforward inclusion/exclusion rules. const engine = new RetireJsEngine(); diff --git a/test/lib/eslint/TypescriptEslintStrategy.test.ts b/test/lib/eslint/TypescriptEslintStrategy.test.ts index 82f3fe92a..f161cf148 100644 --- a/test/lib/eslint/TypescriptEslintStrategy.test.ts +++ b/test/lib/eslint/TypescriptEslintStrategy.test.ts @@ -273,7 +273,7 @@ describe('TypescriptEslint Strategy', () => { }); it('The typescript engine should convert the eslint error to something more user friendly', async () => { - const {results} = await ruleManager.runRulesMatchingCriteria([], ['invalid-ts'], {format: OUTPUT_FORMAT.JSON, normalizeSeverity: false}, EMPTY_ENGINE_OPTIONS); + const {results} = await ruleManager.runRulesMatchingCriteria([], ['invalid-ts'], {format: OUTPUT_FORMAT.JSON, normalizeSeverity: false, runDfa: false, sfdxVersion: 'test'}, EMPTY_ENGINE_OPTIONS); // Parse the json in order to make the string match easier. // There should be a single violation with a single message const ruleResults: RuleResult[] = JSON.parse(results.toString()); diff --git a/test/lib/formatter/RuleResultRecombinator.test.ts b/test/lib/formatter/RuleResultRecombinator.test.ts index 414808b5c..66f118484 100644 --- a/test/lib/formatter/RuleResultRecombinator.test.ts +++ b/test/lib/formatter/RuleResultRecombinator.test.ts @@ -288,6 +288,21 @@ const allFakeDfaRuleResultsNormalized: RuleResult[] = [ } ]; +const retireJsVerboseViolations: RuleResult[] = [ + { + engine: 'retire-js', + fileName: sampleFile1, + violations: [{ + "line": 1, + "column": 1, + "severity": 2, + "message": "jquery 3.1.0 has known vulnerabilities:\nseverity: medium; summary: jQuery before 3.4.0, as used in Drupal, Backdrop CMS, and other products, mishandles jQuery.extend(true, {}, ...) because of Object.prototype pollution; CVE: CVE-2019-11358; https://blog.jquery.com/2019/04/10/jquery-3-4-0-released/ https://nvd.nist.gov/vuln/detail/CVE-2019-11358 https://github.com/jquery/jquery/commit/753d591aea698e57d6db58c9f722cd0808619b1b\nseverity: medium; summary: Regex in its jQuery.htmlPrefilter sometimes may introduce XSS; CVE: CVE-2020-11022; https://blog.jquery.com/2020/04/10/jquery-3-5-0-released/\nseverity: medium; summary: Regex in its jQuery.htmlPrefilter sometimes may introduce XSS; CVE: CVE-2020-11023; https://blog.jquery.com/2020/04/10/jquery-3-5-0-released/", + "ruleName": "insecure-bundled-dependencies", + "category": "Insecure Dependencies", + }] + } +]; + function isString(x: string | {columns; rows}): x is string { return typeof x === 'string'; } @@ -841,6 +856,12 @@ describe('RuleResultRecombinator', () => { expect(summaryMap.get('sfge')).to.deep.equal({fileCount: 1, violationCount: 1}, 'SFGE summary should be correct'); }); + it ('Using --verbose-violations', async () => { + const results = (await RuleResultRecombinator.recombineAndReformatResults(retireJsVerboseViolations, OUTPUT_FORMAT.JSON, new Set(['retire-js']), true)).results; + const ruleResults: RuleResult[] = JSON.parse(results as string); + expect(ruleResults[0].violations[0].message).to.equal("jquery 3.1.0 has known vulnerabilities: severity: medium; summary: jQuery before 3.4.0, as used in Drupal, Backdrop CMS, and other products, mishandles jQuery.extend(true, {}, ...) because of Object.prototype pollution; CVE: CVE-2019-11358; https://blog.jquery.com/2019/04/10/jquery-3-4-0-released/ https://nvd.nist.gov/vuln/detail/CVE-2019-11358 https://github.com/jquery/jquery/commit/753d591aea698e57d6db58c9f722cd0808619b1b; severity: medium; summary: Regex in its jQuery.htmlPrefilter sometimes may introduce XSS; CVE: CVE-2020-11022; https://blog.jquery.com/2020/04/10/jquery-3-5-0-released/; severity: medium; summary: Regex in its jQuery.htmlPrefilter sometimes may introduce XSS; CVE: CVE-2020-11023; https://blog.jquery.com/2020/04/10/jquery-3-5-0-released/"); + }); + it ('Edge Cases', async () => { const results = await (await RuleResultRecombinator.recombineAndReformatResults(edgeCaseResults, OUTPUT_FORMAT.JSON, new Set(['eslint']))).results; const ruleResults: RuleResult[] = JSON.parse(results as string); @@ -1268,5 +1289,14 @@ describe('RuleResultRecombinator', () => { expect(problemNumber).to.equal(6, 'Problem Number Index'); }); }); + + describe('Output Format: HTML', () => { + it ('Using --verbose-violations', async () => { + const results = (await RuleResultRecombinator.recombineAndReformatResults(retireJsVerboseViolations, OUTPUT_FORMAT.HTML, new Set(['retire-js']), true)).results as string; + const violationString = results.split("const violations = [")[1].split("];\n")[0]; + const violation: RuleViolation = JSON.parse(violationString as string); + expect(violation.message).to.equal("jquery 3.1.0 has known vulnerabilities:
severity: medium; summary: jQuery before 3.4.0, as used in Drupal, Backdrop CMS, and other products, mishandles jQuery.extend(true, {}, ...) because of Object.prototype pollution; CVE: CVE-2019-11358; https://blog.jquery.com/2019/04/10/jquery-3-4-0-released/ https://nvd.nist.gov/vuln/detail/CVE-2019-11358 https://github.com/jquery/jquery/commit/753d591aea698e57d6db58c9f722cd0808619b1b
severity: medium; summary: Regex in its jQuery.htmlPrefilter sometimes may introduce XSS; CVE: CVE-2020-11022; https://blog.jquery.com/2020/04/10/jquery-3-5-0-released/
severity: medium; summary: Regex in its jQuery.htmlPrefilter sometimes may introduce XSS; CVE: CVE-2020-11023; https://blog.jquery.com/2020/04/10/jquery-3-5-0-released/"); + }); + }); }); }); diff --git a/test/lib/retire-js/RetireJsEngine.test.ts b/test/lib/retire-js/RetireJsEngine.test.ts index b61a222b3..5067a0bba 100644 --- a/test/lib/retire-js/RetireJsEngine.test.ts +++ b/test/lib/retire-js/RetireJsEngine.test.ts @@ -301,7 +301,7 @@ describe('RetireJsEngine', () => { }; // THIS IS THE ACTUAL METHOD BEING TESTED: Now we feed that fake result into the engine and see what we get back. - const results: RuleResult[] = (testEngine as any).processOutput(JSON.stringify(fakeRetireOutput), 'insecure-bundled-dependencies'); + const results: RuleResult[] = (testEngine as any).processOutput(JSON.stringify(fakeRetireOutput), 'insecure-bundled-dependencies', false); // Now we run our assertions. expect(results.length).to.equal(2, 'Should be two result objects because of the two spoofed files.'); @@ -314,6 +314,61 @@ describe('RetireJsEngine', () => { expect(results[1].violations[1].severity).to.equal(3, 'Sev should be translated to 3'); }); + it('Properly generates message for --verbose-violations', async () => { + // First, we need to seed the test engine with some fake aliases. + const firstOriginal = path.join('first', 'unimportant', 'path', 'jquery-3.1.0.js'); + const firstAlias = path.join('first', 'unimportant', 'alias', 'jquery-3.1.0.js'); + const secondOriginal = path.join('first', 'unimportant', 'path', 'angular-scenario.js'); + const secondAlias = path.join('first', 'unimportant', 'alias', 'angular-scenario.js'); + + (testEngine as any).originalFilesByAlias.set(firstAlias, firstOriginal); + (testEngine as any).originalFilesByAlias.set(secondAlias, secondOriginal); + + // Next, we want to spoof some output that looks like it came from RetireJS. + const fakeRetireResult = { + "version": "3.1.0", + "component": "jquery", + "vulnerabilities": [{ + "below": "3.4.0", + "identifiers": { + "CVE": ["CVE-2019-11358"], + "summary": "summary one", + "random": "this could be anything" + }, + "info": [ + 'https://blog.jquery.com/2019/04/10/jquery-3-4-0-released/', + 'https://nvd.nist.gov/vuln/detail/CVE-2019-11358' + ], + "severity": "medium" + }, { + "below": "3.5.0", + "identifiers": { + "summary": "summary two" + }, + "info": [ + 'https://blog.jquery.com/2020/04/10/jquery-3-5-0-released/' + ], + "severity": "medium" + }, { + "below": "3.6.0", + "identifiers": { + "CVE": ["CVE-2020-11111"], + }, + "info": [ + 'https://blog.jquery.com/2020/04/10/jquery-3-5-0-released/' + ], + "severity": "low" + }] + } + + // THIS IS THE ACTUAL METHOD BEING TESTED: Now we feed that fake result into the engine and see what we get back. + const message: string = (testEngine as any).generateVerboseMessage(fakeRetireResult, 'insecure-bundled-dependencies', true); + + // Now we run our assertions. + expect(message).to.equal("jquery 3.1.0 has known vulnerabilities:\nseverity: medium; summary: summary one; CVE: CVE-2019-11358; random: this could be anything; https://blog.jquery.com/2019/04/10/jquery-3-4-0-released/ https://nvd.nist.gov/vuln/detail/CVE-2019-11358\nseverity: medium; summary: summary two; https://blog.jquery.com/2020/04/10/jquery-3-5-0-released/\nseverity: low; CVE: CVE-2020-11111; https://blog.jquery.com/2020/04/10/jquery-3-5-0-released/", 'Verbose message should contain correct information and format'); + + }); + // Changes to the codebase make it unclear how this corner case would occur, but it's worth having the automation // so we avoid introducing any weird bugs in the future. it('Corner Case: When file has multiple aliases, results are consolidated', async () => { @@ -377,7 +432,7 @@ describe('RetireJsEngine', () => { }; // THIS IS THE ACTUAL METHOD BEING TESTED: Now we feed that fake result into the engine and see what we get back. - const results: RuleResult[] = (testEngine as any).processOutput(JSON.stringify(fakeRetireOutput), 'insecure-bundled-dependencies'); + const results: RuleResult[] = (testEngine as any).processOutput(JSON.stringify(fakeRetireOutput), 'insecure-bundled-dependencies', false); // Now we run our assertions. expect(results.length).to.equal(1, 'Should be one result object, since both aliases correspond to the same original file'); @@ -393,7 +448,7 @@ describe('RetireJsEngine', () => { const invalidJson = '{"beep": ['; try { - const results: RuleResult[] = (testEngine as any).processOutput(invalidJson, 'insecure-bundled-dependencies'); + const results: RuleResult[] = (testEngine as any).processOutput(invalidJson, 'insecure-bundled-dependencies', false); expect(true).to.equal(false, 'Exception should be thrown'); expect(results).to.equal(null, 'This assertion should never fire. It is needed to make the TS compiler stop complaining'); } catch (e) { @@ -408,7 +463,7 @@ describe('RetireJsEngine', () => { }; try { - const results: RuleResult[] = (testEngine as any).processOutput(JSON.stringify(malformedJson), 'insecure-bundled-dependencies'); + const results: RuleResult[] = (testEngine as any).processOutput(JSON.stringify(malformedJson), 'insecure-bundled-dependencies', false); expect(true).to.equal(false, 'Exception should be thrown'); expect(results).to.equal(null, 'This assertion should never fire. It is needed to make the TS compiler stop complaining'); } catch (e) {