FIX(pmd): @W-18808343@: Prevent AppExchange rules from running in an infinite loop (#312)

Author: stephen-carter-at-sf

packages/code-analyzer-pmd-engine/package.json

@@ -1,7 +1,7 @@
 {
   "name": "@salesforce/code-analyzer-pmd-engine",
   "description": "Plugin package that adds 'pmd' and 'cpd' as engines into Salesforce Code Analyzer",
-  "version": "0.26.0",
+  "version": "0.26.1-SNAPSHOT",
   "author": "The Salesforce Code Analyzer Team",
   "license": "BSD-3-Clause",
   "homepage": "https://developer.salesforce.com/docs/platform/salesforce-code-analyzer/overview",
@@ -71,4 +71,4 @@
       "!src/index.ts"
     ]
   }
-}
+}

packages/code-analyzer-pmd-engine/pmd-rules/src/main/java/com/salesforce/security/pmd/apex/DangerousChangeProtectionMethods.java

@@ -25,17 +25,11 @@ public class DangerousChangeProtectionMethods extends AbstractApexRule {
 
     @Override
     public Object visit(ASTMethodCallExpression node, Object data) {
-        if (Helper.isTestMethodOrClass(node)) {
-            super.visit(node, data);
-        }
-
-        if (node.getFullMethodName().compareToIgnoreCase(
-            CHANGE_PROTECTION) == 0
-            || node.getFullMethodName().compareToIgnoreCase(SYSTEM_CHANGE_PROTECTION) == 0) {
+        if (!Helper.isTestMethodOrClass(node) && (node.getFullMethodName().compareToIgnoreCase(CHANGE_PROTECTION) == 0
+            || node.getFullMethodName().compareToIgnoreCase(SYSTEM_CHANGE_PROTECTION) == 0)) {
             this.handleChangeProtection(node, data);
         }
-        super.visit(node, data);
-        return data;
+        return super.visit(node, data);
     }
 
     private void handleChangeProtection(ASTMethodCallExpression node, Object data) {

packages/code-analyzer-pmd-engine/pmd-rules/src/main/java/com/salesforce/security/pmd/apex/DangerousPasswordMethods.java

@@ -14,7 +14,7 @@
 import net.sourceforge.pmd.lang.rule.RuleTargetSelector;
 
 public class DangerousPasswordMethods extends AbstractApexRule {
-    
+
     private static final String MOVE_PASSWORD = "movePassword";
     private static final String SYSTEM_MOVE_PASSWORD = "System.movePassword";
 
@@ -74,8 +74,7 @@ public Object visit(ASTMethodCallExpression node, Object data) {
         {
             this.handleSetPassword(node, data);
         }
-        super.visit(node, data);
-        return data;
+        return super.visit(node, data);
     }
 
     private void handleMovePassword(ASTMethodCallExpression node, Object data) {
@@ -113,7 +112,7 @@ private void handleSetPassword(ASTMethodCallExpression node, Object data) {
             } else {
                 asCtx(data).addViolationWithMessage(node, String.format(SET_PASSWORD_VIOLATION_LOW_CONFIDENCE, userIdVarName));
             }
-            
+
         }
     }
 

packages/code-analyzer-pmd-engine/pmd-rules/src/main/java/com/salesforce/security/pmd/apex/DetectHardcodedCredentialsInHttpRequests.java

@@ -16,7 +16,7 @@
 public class DetectHardcodedCredentialsInHttpRequests extends DetectHardcodedCredentialsBase {
     private static final List<String> HTTP_AUTH_HEADERS = SecretsInPackageUtils.AUTH_FIELD_MAPPINGS_LIST;
     private static final List<String> HTTP_AUTH_HEADER_VALUES_TO_IGNORE = Arrays.asList(SecretsInPackageUtils.STRINGS_TO_IGNORE);
-    
+
     private static final String HARD_CODED_SECRET_IN_HTTP_REQUEST_HEADER_VIOLATION = "Potentially hardcoded secret found in HTTP request header";
     private static final String MERGE_FIELD_LITERAL="{!$Credential.";
 
@@ -49,7 +49,7 @@ public Object visit(ASTMethodCallExpression node, Object data) {
 
         Node firstChild = childRefExpr.getNextSibling();
         Node secondChild = firstChild.getNextSibling();
-        
+
         List<ASTLiteralExpression> firstArgLiterals = firstChild.descendantsOrSelf()
                 .filterIs(ASTLiteralExpression.class).toList();
         List<ASTVariableExpression> firstArgVars =
@@ -99,8 +99,7 @@ public Object visit(ASTMethodCallExpression node, Object data) {
                 break;
             }
         }
-        super.visit(node, data);
-        return data;
+        return super.visit(node, data);
     }
 
     private boolean isAMergeField(String value) {

packages/code-analyzer-pmd-engine/pmd-rules/src/main/java/com/salesforce/security/pmd/apex/DetectHardcodedCredentialsInPasswordMethods.java

@@ -23,8 +23,7 @@ public Object visit(ASTMethodCallExpression node, Object data) {
                     || node.getFullMethodName().compareToIgnoreCase(SYSTEM_SET_PASSWORD) == 0) {
                 this.handleSetPassword(node, data);
             }
-            super.visit(node, data);
-            return data;
+            return super.visit(node, data);
         }
 
         private void handleSetPassword(ASTMethodCallExpression node, Object data) {

packages/code-analyzer-pmd-engine/pmd-rules/src/main/java/com/salesforce/security/pmd/visualforce/DetectCreateElementScriptTag.java

@@ -10,7 +10,7 @@
 import net.sourceforge.pmd.reporting.RuleContext;
 
 public class DetectCreateElementScriptTag extends AbstractVfRule {
-    
+
     private static ArrayList<Pattern> CREATE_ELEMENT_PATTERNS = new ArrayList<Pattern>();
     private static final String[] CREATE_ELEMENT_REGEXES = {
             "createElement.*['\"]script",
@@ -35,16 +35,15 @@ public void start(RuleContext ctx) {
     public Object visit(ASTHtmlScript node, Object data) {
         Chars c = node.getText();
         searchForPatterns(node,data,c);
-        super.visit(node, data);
-        return data;
+        return super.visit(node, data);
     }
 
     private void searchForPatterns(ASTHtmlScript node, Object data, Chars c) {
         String staticJS = c.toString();
 
         for (Pattern nextPattern: CREATE_ELEMENT_PATTERNS) {
             Matcher match = nextPattern.matcher(staticJS);
-            if (match.find()) {                
+            if (match.find()) {
                 this.asCtx(data)
                 .addViolationWithMessage(node,
                         CREATE_ELEMENT_SCRIPT_CSS_VIOLATION);

packages/code-analyzer-pmd-engine/pmd-rules/src/main/java/com/salesforce/security/pmd/visualforce/DetectHardcodedSecretsInAttributes.java

@@ -21,13 +21,12 @@ public Object visit(ASTElement node, Object data) {
         } else if (APEX_ATTRIBUTE.equalsIgnoreCase(node.getName())) {
             processASTAttribute(node,data);
         }
-        super.visit(node, data);
-        return data;
+        return super.visit(node, data);
     }
 
     private void processComponentInclusion(ASTElement node,Object data) {
         List<ASTAttribute> allAttributes = node.children(ASTAttribute.class).toList();
-        
+
         for (ASTAttribute nextAttr: allAttributes) {
             String attrName = nextAttr.getName();
             if (SecretsInPackageUtils.isAPotentialSecret(attrName)) {

packages/code-analyzer-pmd-engine/test/pmd-engine.test.ts

@@ -623,6 +623,22 @@ describe('Tests for the runRules method of PmdEngine', () => {
         expect(errorLogEvents).toHaveLength(1);
         expect(errorLogEvents[0].message).toContain('Message was null');
     });
+
+    it('When running a file that has a method that references a class name that is the same as itself, then the AppExchange rules should not run in infinite loop', async () => {
+        // This test is to ensure that we have resolved the issue associated with: W-18808343
+        const engine: PmdEngine = new PmdEngine(DEFAULT_PMD_ENGINE_CONFIG);
+
+        // Get AppExchange rules
+        const ruleDescriptions: RuleDescription[] = await engine.describeRules(createDescribeOptions());
+        const appExchangeRuleNames: string[] = ruleDescriptions.filter(r => r.tags.includes('AppExchange'))
+            .map(r => r.name);
+        expect(appExchangeRuleNames.length).toBeGreaterThan(0); // sanity check
+
+        const workspace: Workspace = new Workspace('id', [path.join(TEST_DATA_FOLDER, 'samplePmdWorkspaceFor_W-18808343')]);
+
+        const results: EngineRunResults = await engine.runRules(appExchangeRuleNames, createRunOptions(workspace)); // This should not run forever and ever.
+        expect(results.violations.length).toEqual(0);
+    });
 });
 
 describe('Tests for the getEngineVersion method of PmdEngine', () => {