Improving getter detection

zeitiger · Dec 10, 2016 · f9882be · f9882be
1 parent 5ded17c
commit f9882be
Show file tree

Hide file tree

Showing 2 changed files with 108 additions and 1 deletion.
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
@@ -25,6 +25,7 @@
 import net.sourceforge.pmd.lang.apex.ast.ASTMethodCallExpression;
 import net.sourceforge.pmd.lang.apex.ast.ASTProperty;
 import net.sourceforge.pmd.lang.apex.ast.ASTReferenceExpression;
+import net.sourceforge.pmd.lang.apex.ast.ASTReturnStatement;
 import net.sourceforge.pmd.lang.apex.ast.ASTSoqlExpression;
 import net.sourceforge.pmd.lang.apex.ast.ASTUserClass;
 import net.sourceforge.pmd.lang.apex.ast.ASTVariableDeclaration;
@@ -175,6 +176,16 @@ public Object visit(final ASTVariableDeclaration node, Object data) {
 
     }
 
+    @Override
+    public Object visit(final ASTReturnStatement node, Object data) {
+        final ASTSoqlExpression soql = node.getFirstChildOfType(ASTSoqlExpression.class);
+        if (soql != null) {
+            checkForAccessibility(soql, data);
+        }
+
+        return data;
+    }
+
     private void addVariableToMapping(final String variableName, final String type) {
         varToTypeMapping.put(variableName, getSimpleType(type));
     }
@@ -322,6 +333,9 @@ private void validateCRUDCheckPresent(final AbstractApexNode<?> node, final Obje
     }
 
     private void checkForAccessibility(final AbstractApexNode<?> node, Object data) {
+        boolean isGetter = false;
+        String returnType = null;
+
         final ASTMethod wrappingMethod = node.getFirstParentOfType(ASTMethod.class);
         final ASTUserClass wrappingClass = node.getFirstParentOfType(ASTUserClass.class);
 
@@ -330,14 +344,21 @@ private void checkForAccessibility(final AbstractApexNode<?> node, Object data)
             return;
         }
 
+        if (wrappingMethod != null) {
+            isGetter = isMethodAGetter(wrappingMethod);
+            returnType = getReturnType(wrappingMethod);
+        }
+
         final ASTVariableDeclaration variableDecl = node.getFirstParentOfType(ASTVariableDeclaration.class);
         if (variableDecl != null) {
             String type = variableDecl.getNode().getLocalInfo().getType().getApexName();
             type = getSimpleType(type);
             StringBuilder typeCheck = new StringBuilder().append(variableDecl.getNode().getDefiningType().getApexName())
                     .append(":").append(type);
 
-            validateCRUDCheckPresent(node, data, ANY, typeCheck.toString());
+            if (!isGetter) {
+                validateCRUDCheckPresent(node, data, ANY, typeCheck.toString());
+            }
 
         }
 
@@ -353,5 +374,26 @@ private void checkForAccessibility(final AbstractApexNode<?> node, Object data)
             }
 
         }
+
+        final ASTReturnStatement returnStatement = node.getFirstParentOfType(ASTReturnStatement.class);
+        if (returnStatement != null) {
+            if (!isGetter) {
+                validateCRUDCheckPresent(node, data, ANY, returnType == null ? "" : returnType);
+            }
+        }
+    }
+
+    private String getReturnType(final ASTMethod method) {
+        return new StringBuilder().append(method.getNode().getDefiningType().getApexName()).append(":")
+                .append(method.getNode().getMethodInfo().getEmitSignature().getReturnType().getApexName()).toString();
+    }
+
+    private boolean isMethodAGetter(final ASTMethod method) {
+        final Pattern p = Pattern.compile("^(string|void)$", Pattern.CASE_INSENSITIVE);
+        final boolean startsWithGet = method.getNode().getMethodInfo().getCanonicalName().startsWith("get");
+        final boolean voidOrString = p
+                .matcher(method.getNode().getMethodInfo().getEmitSignature().getReturnType().getApexName()).matches();
+
+        return (startsWithGet && !voidOrString);
     }
 }
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
@@ -1,6 +1,71 @@
 <?xml version="1.0" encoding="UTF-8"?>
 
 <test-data>
+	<test-code>
+		<description>VF built-in CRUD checks via getter</description>
+		<expected-problems>0</expected-problems>
+		<code><![CDATA[ 
+public class Foo { 	
+	public Contact getFoo() {
+		String tempID = 'someID';
+	 	Contact c = [SELECT Name FROM Contact WHERE Id=:tempID];
+	 	return c;
+	} 
+	
+	public Contact getBaz() {
+		String tempID = 'someID';
+	 	return  [SELECT Name FROM Contact WHERE Id=:tempID];
+	} 
+} 
+]]></code>
+	</test-code>
+
+	<test-code>
+		<description>Bypassing VF built-in CRUD checks via getter
+		</description>
+		<expected-problems>1</expected-problems>
+		<code><![CDATA[ 
+public class Foo { 	
+	public String getUserName() {
+		String tempID = 'someID';
+	 	Contact c = [SELECT Name FROM Contact WHERE Id=:tempID];
+	 	return c.Name;
+	} 
+} 
+]]></code>
+	</test-code>
+
+
+
+	<test-code>
+		<description>No VF provided CRUD checks via getter</description>
+		<expected-problems>1</expected-problems>
+		<code><![CDATA[ 
+public class Foo { 	
+	public Contact justGiveMeFoo() {
+		String tempID = 'someID';
+	 	return [SELECT Name FROM Contact WHERE Id=:tempID];
+	} 
+} 
+]]></code>
+	</test-code>
+
+	<test-code>
+		<description>Proper CRUD checks</description>
+		<expected-problems>0</expected-problems>
+		<code><![CDATA[ 
+public class Foo { 	
+	public Contact justGiveMeFoo() {
+		String tempID = 'someID';
+		if (Contact.sObjectType.getDescribe().isAccessible()) {
+	 		return [SELECT Name FROM Contact WHERE Id=:tempID];
+	 	}
+	 	return null;
+	} 
+} 
+]]></code>
+	</test-code>
+
 	<test-code>
 		<description>No CRUD,FLS needed via sObject property</description>
 		<expected-problems>1</expected-problems>