diff --git a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexSharingViolationsRule.java b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexSharingViolationsRule.java index c6ebd5c600..c960c56916 100644 --- a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexSharingViolationsRule.java +++ b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexSharingViolationsRule.java @@ -4,6 +4,9 @@ package net.sourceforge.pmd.lang.apex.rule.security; +import java.util.List; + +import net.sourceforge.pmd.lang.apex.ast.ASTMethodCallExpression; import net.sourceforge.pmd.lang.apex.ast.ASTUserClass; import net.sourceforge.pmd.lang.apex.ast.ApexNode; import net.sourceforge.pmd.lang.apex.rule.AbstractApexRule; @@ -27,19 +30,51 @@ public class ApexSharingViolationsRule extends AbstractApexRule { @Override public Object visit(ASTUserClass node, Object data) { if (!Helper.isTestMethodOrClass(node)) { - checkForSharingDeclaration(node, data); + boolean sharingFound = isSharingPresent(node); + checkForSharingDeclaration(node, data, sharingFound); + checkForDatabaseMethods(node, data, sharingFound); } return data; } + /** + * Check if class contains any Database.query / Database.insert [ Database.* + * ] methods + * + * @param node + * @param data + */ + private void checkForDatabaseMethods(ASTUserClass node, Object data, boolean sharingFound) { + List calls = node.findDescendantsOfType(ASTMethodCallExpression.class); + for (ASTMethodCallExpression call : calls) { + if (Helper.isMethodName(call, "Database", Helper.ANY_METHOD)) { + if (!sharingFound) { + addViolation(data, node); + } + } + } + } + /** * Check if class has no sharing declared * * @param node * @param data */ - private void checkForSharingDeclaration(ApexNode node, Object data) { + private void checkForSharingDeclaration(ApexNode node, Object data, boolean sharingFound) { final boolean foundAnyDMLorSOQL = Helper.foundAnyDML(node) && Helper.foundAnySOQLorSOSL(node); + if (!sharingFound && !Helper.isTestMethodOrClass(node) && foundAnyDMLorSOQL) { + addViolation(data, node); + } + } + + /** + * Does class have sharing keyword declared? + * + * @param node + * @return + */ + private boolean isSharingPresent(ApexNode node) { boolean sharingFound = false; for (ModifierOrAnnotationTypeInfo type : node.getNode().getDefiningType().getModifiers().all()) { @@ -51,10 +86,7 @@ public class ApexSharingViolationsRule extends AbstractApexRule { } } - - if (!sharingFound && !Helper.isTestMethodOrClass(node) && foundAnyDMLorSOQL) { - addViolation(data, node); - } + return sharingFound; } } diff --git a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/Helper.java b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/Helper.java index 3ed3118f5b..27c16b7e9f 100644 --- a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/Helper.java +++ b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/Helper.java @@ -39,6 +39,8 @@ import apex.jorje.semantic.ast.statement.VariableDeclaration; * */ public final class Helper { + protected static final String ANY_METHOD = "*"; + private Helper() { throw new AssertionError("Can't instantiate helper classes"); } @@ -99,13 +101,15 @@ public final class Helper { final String methodName) { final ASTReferenceExpression reference = methodNode.getFirstChildOfType(ASTReferenceExpression.class); if (reference.getNode().getJadtIdentifiers().size() == 1) { - if (reference.getNode().getJadtIdentifiers().get(0).value.equalsIgnoreCase(className) - && isMethodName(methodNode, methodName)) { - return true; + if (reference.getNode().getJadtIdentifiers().get(0).value.equalsIgnoreCase(className)) { + if (methodName.equals(ANY_METHOD) || isMethodName(methodNode, methodName)) { + return true; + } } } return false; + } static boolean isMethodName(final ASTMethodCallExpression m, final String methodName) { diff --git a/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/security/xml/ApexSharingViolations.xml b/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/security/xml/ApexSharingViolations.xml index 5022dc74bc..4045a2661e 100644 --- a/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/security/xml/ApexSharingViolations.xml +++ b/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/security/xml/ApexSharingViolations.xml @@ -27,6 +27,32 @@ public class Foo { } ]]> + + + Apex class without any sharing declared with Database method + + 1 + test1() { + return Database.query("Select Id from Account LIMIT 100"); + } +} + ]]> + + + + Apex class with sharing and Database method + + 0 + test1() { + return Database.query("Select Id from Account LIMIT 100"); + } +} + ]]> + Apex class with explicit sharing with DML diff --git a/src/site/markdown/overview/changelog.md b/src/site/markdown/overview/changelog.md index 9f0ff75135..b3e2dc0cf0 100644 --- a/src/site/markdown/overview/changelog.md +++ b/src/site/markdown/overview/changelog.md @@ -274,4 +274,5 @@ to avoid XSS attacks. * [#181](https://github.com/pmd/pmd/pull/181): \[apex] Control flow based CRUD rule checking * [#184](https://github.com/pmd/pmd/pull/184): \[apex] Improving open redirect detection for static fields & assignment operations * [#189](https://github.com/pmd/pmd/pull/189): \[apex] Bug fix of SOQL concatenated vars detection +* [#191](https://github.com/pmd/pmd/pull/191): \[apex] Detection of sharing violation when Database. methods are used