Merge pull request #3470 from jfeingold35:addSuperCall

[apex] Fix ApexCRUDViolationRule - add super call #3470
pmd · Aug 22, 2021 · 2636fde · 2636fde
2 parents b2adc91 + e22a434
commit 2636fde
Show file tree

Hide file tree

Showing 3 changed files with 81 additions and 13 deletions.
diff --git a/docs/pages/release_notes.md b/docs/pages/release_notes.md
@@ -16,6 +16,9 @@ This is a {{ site.pmd.release_type }} release.
 
 ### Fixed Issues
 
+*   apex
+    *   [#3462](https://github.com/pmd/pmd/issues/3462): \[apex] SOQL performed in a for-each loop doesn't trigger ApexCRUDViolationRule
+    *   [#3484](https://github.com/pmd/pmd/issues/3484): \[apex] ApexCRUDViolationRule maintains state across files
 *   core
     *   [#3446](https://github.com/pmd/pmd/issues/3446): \[core] Allow XPath rules to access the current file name
 *   java-bestpractices
@@ -26,6 +29,7 @@ This is a {{ site.pmd.release_type }} release.
 ### External Contributions
 
 *   [#3445](https://github.com/pmd/pmd/pull/3445): \[java] Fix #3403 about MethodNamingConventions and JUnit5 parameterized tests - [Cyril Sicard](https://github.com/CyrilSicard)
+*   [#3470](https://github.com/pmd/pmd/pull/3470): \[apex] Fix ApexCRUDViolationRule - add super call - [Josh Feingold](https://github.com/jfeingold35)
 
 {% endtocmaker %}
 
diff --git a/...apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexCRUDViolationRule.java b/...apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexCRUDViolationRule.java
@@ -18,6 +18,7 @@
 import java.util.regex.Pattern;
 import java.util.stream.Collectors;
 
+import net.sourceforge.pmd.RuleContext;
 import net.sourceforge.pmd.lang.apex.ast.ASTAssignmentExpression;
 import net.sourceforge.pmd.lang.apex.ast.ASTBlockStatement;
 import net.sourceforge.pmd.lang.apex.ast.ASTDmlDeleteStatement;
@@ -85,10 +86,10 @@ public class ApexCRUDViolationRule extends AbstractApexRule {
 
     private static final Pattern WITH_SECURITY_ENFORCED = Pattern.compile("(?is).*[^']\\s*WITH\\s+SECURITY_ENFORCED\\s*[^']*");
 
-    private final Map<String, String> varToTypeMapping = new HashMap<>();
-    private final ListMultimap<String, String> typeToDMLOperationMapping = ArrayListMultimap.create();
-    private final Map<String, String> checkedTypeToDMLOperationViaESAPI = new HashMap<>();
-    private final Map<String, ASTMethod> classMethods = new WeakHashMap<>();
+    private Map<String, String> varToTypeMapping;
+    private ListMultimap<String, String> typeToDMLOperationMapping;
+    private Map<String, String> checkedTypeToDMLOperationViaESAPI;
+    private Map<String, ASTMethod> classMethods;
     private String className;
 
     public ApexCRUDViolationRule() {
@@ -97,6 +98,19 @@ public ApexCRUDViolationRule() {
         setProperty(CODECLIMATE_BLOCK_HIGHLIGHTING, false);
     }
 
+    @Override
+    public void start(RuleContext ctx) {
+        // At the start of each rule execution, these member variables need to be fresh. So they're initialized in the
+        // .start() method instead of the constructor, since .start() is called before every execution.
+        varToTypeMapping = new HashMap<>();
+        typeToDMLOperationMapping = ArrayListMultimap.create();
+        checkedTypeToDMLOperationViaESAPI = new HashMap<>();
+        classMethods = new WeakHashMap<>();
+        className = null;
+        super.start(ctx);
+    }
+
+
     @Override
     public Object visit(ASTUserClass node, Object data) {
         if (Helper.isTestMethodOrClass(node) || Helper.isSystemLevelClass(node)) {
@@ -246,7 +260,7 @@ public Object visit(final ASTForEachStatement node, Object data) {
             checkForAccessibility(soql, data);
         }
 
-        return data;
+        return super.visit(node, data);
     }
 
     private void addVariableToMapping(final String variableName, final String type) {
@@ -539,7 +553,7 @@ private void extractObjectTypeFromESAPI(final ASTMethodCallExpression node, fina
     }
 
 
-    private void validateCRUDCheckPresent(final ApexNode<?> node, final Object data, final String crudMethod,
+    private boolean validateCRUDCheckPresent(final ApexNode<?> node, final Object data, final String crudMethod,
             final String typeCheck) {
         boolean missingKey = !typeToDMLOperationMapping.containsKey(typeCheck);
         boolean isImproperDMLCheck = !isProperESAPICheckForDML(typeCheck, crudMethod);
@@ -548,6 +562,7 @@ private void validateCRUDCheckPresent(final ApexNode<?> node, final Object data,
             //if condition returns true, add violation, otherwise return.
             if (isImproperDMLCheck && noSecurityEnforced) {
                 addViolation(data, node);
+                return true;
             }
         } else {
             boolean properChecksHappened = false;
@@ -566,8 +581,10 @@ private void validateCRUDCheckPresent(final ApexNode<?> node, final Object data,
 
             if (!properChecksHappened) {
                 addViolation(data, node);
+                return true;
             }
         }
+        return false;
     }
 
     private void checkForAccessibility(final ASTSoqlExpression node, Object data) {
@@ -591,7 +608,7 @@ private void checkForAccessibility(final ASTSoqlExpression node, Object data) {
         if (wrappingMethod != null) {
             returnType = getReturnType(wrappingMethod);
         }
-
+        boolean violationAdded = false;
         final ASTVariableDeclaration variableDecl = node.getFirstParentOfType(ASTVariableDeclaration.class);
         if (variableDecl != null) {
             String type = variableDecl.getType();
@@ -600,15 +617,20 @@ private void checkForAccessibility(final ASTSoqlExpression node, Object data) {
                     .append(":").append(type);
 
             if (typesFromSOQL.isEmpty()) {
-                validateCRUDCheckPresent(node, data, ANY, typeCheck.toString());
+                violationAdded = validateCRUDCheckPresent(node, data, ANY, typeCheck.toString());
             } else {
                 for (String typeFromSOQL : typesFromSOQL) {
-                    validateCRUDCheckPresent(node, data, ANY, typeFromSOQL);
+                    violationAdded |= validateCRUDCheckPresent(node, data, ANY, typeFromSOQL);
                 }
             }
 
         }
 
+        // If the node's already in violation, we don't need to keep checking.
+        if (violationAdded) {
+            return;
+        }
+
         final ASTAssignmentExpression assignment = node.getFirstParentOfType(ASTAssignmentExpression.class);
         if (assignment != null) {
             final ASTVariableExpression variable = assignment.getFirstChildOfType(ASTVariableExpression.class);
@@ -617,28 +639,38 @@ private void checkForAccessibility(final ASTSoqlExpression node, Object data) {
                 if (varToTypeMapping.containsKey(variableWithClass)) {
                     String type = varToTypeMapping.get(variableWithClass);
                     if (typesFromSOQL.isEmpty()) {
-                        validateCRUDCheckPresent(node, data, ANY, type);
+                        violationAdded = validateCRUDCheckPresent(node, data, ANY, type);
                     } else {
                         for (String typeFromSOQL : typesFromSOQL) {
-                            validateCRUDCheckPresent(node, data, ANY, typeFromSOQL);
+                            violationAdded |= validateCRUDCheckPresent(node, data, ANY, typeFromSOQL);
                         }
                     }
                 }
             }
 
         }
 
+        // If the node's already in violation, we don't need to keep checking.
+        if (violationAdded) {
+            return;
+        }
+
         final ASTReturnStatement returnStatement = node.getFirstParentOfType(ASTReturnStatement.class);
         if (returnStatement != null) {
             if (typesFromSOQL.isEmpty()) {
-                validateCRUDCheckPresent(node, data, ANY, returnType);
+                violationAdded = validateCRUDCheckPresent(node, data, ANY, returnType);
             } else {
                 for (String typeFromSOQL : typesFromSOQL) {
-                    validateCRUDCheckPresent(node, data, ANY, typeFromSOQL);
+                    violationAdded |= validateCRUDCheckPresent(node, data, ANY, typeFromSOQL);
                 }
             }
         }
 
+        // If the node's already in violation, we don't need to keep checking.
+        if (violationAdded) {
+            return;
+        }
+
         final ASTForEachStatement forEachStatement = node.getFirstParentOfType(ASTForEachStatement.class);
         if (forEachStatement != null) {
             if (typesFromSOQL.isEmpty()) {

diff --git a/.../src/test/resources/net/sourceforge/pmd/lang/apex/rule/security/xml/ApexCRUDViolation.xml b/.../src/test/resources/net/sourceforge/pmd/lang/apex/rule/security/xml/ApexCRUDViolation.xml
@@ -1049,6 +1049,38 @@ public class Foo {
         ]]></code>
     </test-code>
 
+    <test-code>
+        <description>No CRUD check inside for-each loop</description>
+        <expected-problems>1</expected-problems>
+        <code><![CDATA[
+public class Foo {
+    void bar() {
+        Id[] accIds = new List<Id>();
+        for (Id accId : accIds) {
+            Account acc = [SELECT Id FROM Account WHERE Id = :accId];
+        }
+    }
+}
+        ]]></code>
+    </test-code>
+
+    <test-code>
+        <description>Proper CRUD check inside for-each loop</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+public class Foo {
+    void bar() {
+        Id[] accIds = new List<Id>();
+        if (Account.sObjectType.getDescribe().isAccessible()) {
+            for (Id accId : accIds) {
+                Account a = [SELECT Id FROM Account WHERE Id = :accId];
+            }
+        }
+    }
+}
+        ]]></code>
+    </test-code>
+
     <test-code>
         <description>Proper CRUD check in SOQL for-loop with security enforced</description>
         <expected-problems>0</expected-problems>