phoedos/pmd
1
1
Fork 1
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Adding support for accessibility detection, fixing bugs with detection of fall-through checks

Browse Source
This commit is contained in:
Sergey
2016-11-29 11:14:25 -08:00
committed by Juan Martín Sotuyo Dodero
parent 074bab99b8
commit 1e5cbb8d25
2 changed files with 302 additions and 66 deletions
Download Patch File Download Diff File Expand all files Collapse all files

174
pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexCRUDViolationRule.java
View File

@@ -2,12 +2,16 @@ package net.sourceforge.pmd.lang.apex.rule.security;
import java.util.List;
import java.util.stream.Collectors;
import com.google.common.collect.ArrayListMultimap;
import com.google.common.collect.ListMultimap;
import java.util.Arrays;
import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
import apex.jorje.data.ast.Identifier;
import net.sourceforge.pmd.lang.apex.ast.ASTAssignmentExpression;
import net.sourceforge.pmd.lang.apex.ast.ASTDmlDeleteStatement;
import net.sourceforge.pmd.lang.apex.ast.ASTDmlInsertStatement;
import net.sourceforge.pmd.lang.apex.ast.ASTDmlMergeStatement;
@@ -20,6 +24,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.ASTSoqlExpression;
import net.sourceforge.pmd.lang.apex.ast.ASTSoslExpression;
import net.sourceforge.pmd.lang.apex.ast.ASTUserClass;
import net.sourceforge.pmd.lang.apex.ast.ASTVariableDeclaration;
import net.sourceforge.pmd.lang.apex.ast.ASTVariableExpression;
@@ -34,24 +39,30 @@ import net.sourceforge.pmd.lang.apex.rule.AbstractApexRule;
*/
public class ApexCRUDViolationRule extends AbstractApexRule {
private final HashMap<String, String> varToTypeMapping = new HashMap<>();
private final HashMap<String, String> typeToDMLOperationMapping = new HashMap<>();
private final HashSet<String> esapiCheckedTypes = new HashSet<>();
private final ListMultimap<String, String> typeToDMLOperationMapping = ArrayListMultimap.create();
private final HashMap<String, String> checkedTypeToDMLOperationViaESAPI = new HashMap<>();
private static final String IS_CREATEABLE = "isCreateable";
private static final String IS_DELETABLE = "isDeletable";
private static final String IS_UPDATEABLE = "isUpdateable";
private static final String IS_MERGEABLE = "isMergeable";
private static final String IS_ACCESSIBLE = "isAccessible";
private static final String ANY = "ANY";
private static final String S_OBJECT_TYPE = "sObjectType";
private static final String GET_DESCRIBE = "getDescribe";
// ESAPI.accessController().isAuthorizedToView(Lead.sObject, fields)
private static final String[] ESAPI_ISAUTHORIZED = new String[] { "ESAPI", "accessController",
private static final String[] ESAPI_ISAUTHORIZED_TO_VIEW = new String[] { "ESAPI", "accessController",
"isAuthorizedToView" };
private static final String[] RESERVED_KEYS_FLS = new String[] { "Schema", S_OBJECT_TYPE };
private static final String[] ESAPI_ISAUTHORIZED_TO_CREATE = new String[] { "ESAPI", "accessController",
"isAuthorizedToCreate" };
private static final String[] ESAPI_ISAUTHORIZED_TO_UPDATE = new String[] { "ESAPI", "accessController",
"isAuthorizedToUpdate" };
private static final String[] ESAPI_ISAUTHORIZED_TO_DELETE = new String[] { "ESAPI", "accessController",
"isAuthorizedToDelete" };
// private static final String FIELDS = "fields";
// private static final String GET_MAP = "getMap";
private static final String[] RESERVED_KEYS_FLS = new String[] { "Schema", S_OBJECT_TYPE };
public ApexCRUDViolationRule() {
setProperty(CODECLIMATE_CATEGORIES, new String[] { "Security" });
@@ -72,8 +83,20 @@ public class ApexCRUDViolationRule extends AbstractApexRule {
extractObjectAndFields(a, method, node.getNode().getDefiningType().getApexName());
} else {
// see if ESAPI
if (Helper.isMethodCallChain(node, ESAPI_ISAUTHORIZED)) {
extractObjectTypeFromESAPI(node);
if (Helper.isMethodCallChain(node, ESAPI_ISAUTHORIZED_TO_VIEW)) {
extractObjectTypeFromESAPI(node, IS_ACCESSIBLE);
}
if (Helper.isMethodCallChain(node, ESAPI_ISAUTHORIZED_TO_CREATE)) {
extractObjectTypeFromESAPI(node, IS_CREATEABLE);
}
if (Helper.isMethodCallChain(node, ESAPI_ISAUTHORIZED_TO_UPDATE)) {
extractObjectTypeFromESAPI(node, IS_UPDATEABLE);
}
if (Helper.isMethodCallChain(node, ESAPI_ISAUTHORIZED_TO_DELETE)) {
extractObjectTypeFromESAPI(node, IS_DELETABLE);
}
// see if getDescribe()
@@ -115,6 +138,7 @@ public class ApexCRUDViolationRule extends AbstractApexRule {
@Override
public Object visit(ASTDmlUpsertStatement node, Object data) {
checkForCRUD(node, data, IS_CREATEABLE);
checkForCRUD(node, data, IS_UPDATEABLE);
return data;
}
@@ -125,19 +149,34 @@ public class ApexCRUDViolationRule extends AbstractApexRule {
}
@Override
public Object visit(ASTSoqlExpression node, Object data) {
final ASTVariableDeclaration variable = node.getFirstParentOfType(ASTVariableDeclaration.class);
if (variable != null) {
String type = variable.getNode().getLocalInfo().getType().getApexName();
StringBuilder sb = new StringBuilder().append(variable.getNode().getDefiningType().getApexName())
.append(":").append(variable.getNode().getLocalInfo().getName());
varToTypeMapping.put(sb.toString(), type);
public Object visit(final ASTAssignmentExpression node, Object data) {
ASTSoslExpression sosl = node.getFirstChildOfType(ASTSoslExpression.class);
if (sosl != null) {
checkForAccessibility(sosl, data);
}
ASTSoqlExpression soql = node.getFirstChildOfType(ASTSoqlExpression.class);
if (soql != null) {
checkForAccessibility(soql, data);
}
return data;
}
@Override
public Object visit(final ASTVariableDeclaration node, Object data) {
ASTSoslExpression sosl = node.getFirstChildOfType(ASTSoslExpression.class);
if (sosl != null) {
checkForAccessibility(sosl, data);
}
ASTSoqlExpression soql = node.getFirstChildOfType(ASTSoqlExpression.class);
if (soql != null) {
checkForAccessibility(soql, data);
}
String type = node.getNode().getLocalInfo().getType().getApexName();
StringBuilder sb = new StringBuilder().append(node.getNode().getDefiningType().getApexName()).append(":")
.append(node.getNode().getLocalInfo().getName());
@@ -195,13 +234,6 @@ public class ApexCRUDViolationRule extends AbstractApexRule {
if (flsIndex != -1) {
String objectTypeName = strings.get(flsIndex + RESERVED_KEYS_FLS.length);
typeToDMLOperationMapping.put(definingType + ":" + objectTypeName, method);
// TODO: add real FLS check
// if (FIELDS.equalsIgnoreCase(strings.get(++flsIndex +
// RESERVED_KEYS_FLS.length))) {
// String fieldName = strings.get(++flsIndex +
// RESERVED_KEYS_FLS.length);
// }
}
}
@@ -218,27 +250,31 @@ public class ApexCRUDViolationRule extends AbstractApexRule {
StringBuilder sb = new StringBuilder().append(node.getNode().getDefiningType().getApexName()).append(":")
.append(variable.getNode().getIdentifier().value);
String type = varToTypeMapping.get(sb.toString());
final String type = varToTypeMapping.get(sb.toString());
if (type != null) {
StringBuilder typeCheck = new StringBuilder().append(node.getNode().getDefiningType()).append(":")
.append(type);
if (!typeToDMLOperationMapping.containsKey(typeCheck.toString())) {
if (!esapiCheckedTypes.contains(typeCheck.toString())) {
addViolation(data, node);
}
} else {
boolean properChecksHappened = typeToDMLOperationMapping.get(typeCheck.toString())
.equalsIgnoreCase(CRUDMethod);
if (!properChecksHappened) {
addViolation(data, node);
}
}
validateCRUDCheckPresent(node, data, CRUDMethod, typeCheck.toString());
}
}
}
private void extractObjectTypeFromESAPI(final ASTMethodCallExpression node) {
private boolean isProperESAPICheckForDML(final String typeToCheck, final String dmlOperation) {
final boolean hasMapping = checkedTypeToDMLOperationViaESAPI.containsKey(typeToCheck.toString());
if (hasMapping) {
if (dmlOperation.equals(ANY)) {
return true;
}
String dmlChecked = checkedTypeToDMLOperationViaESAPI.get(typeToCheck);
return dmlChecked.equals(dmlOperation);
}
return false;
}
private void extractObjectTypeFromESAPI(final ASTMethodCallExpression node, final String dmlOperation) {
final ASTVariableExpression var = node.getFirstChildOfType(ASTVariableExpression.class);
if (var != null) {
final ASTReferenceExpression reference = var.getFirstChildOfType(ASTReferenceExpression.class);
@@ -247,11 +283,75 @@ public class ApexCRUDViolationRule extends AbstractApexRule {
if (identifiers.size() == 1) {
StringBuilder sb = new StringBuilder().append(node.getNode().getDefiningType().getApexName())
.append(":").append(identifiers.get(0).value);
esapiCheckedTypes.add(sb.toString());
checkedTypeToDMLOperationViaESAPI.put(sb.toString(), dmlOperation);
}
}
}
}
private void validateCRUDCheckPresent(final AbstractApexNode<?> node, final Object data, final String CRUDMethod,
final String typeCheck) {
if (!typeToDMLOperationMapping.containsKey(typeCheck)) {
if (!isProperESAPICheckForDML(typeCheck, CRUDMethod)) {
addViolation(data, node);
}
} else {
boolean properChecksHappened = false;
List<String> dmlOperationsChecked = typeToDMLOperationMapping.get(typeCheck);
for (String dmlOp : dmlOperationsChecked) {
if (dmlOp.equalsIgnoreCase(CRUDMethod)) {
properChecksHappened = true;
break;
}
if (CRUDMethod.equals(ANY)) {
properChecksHappened = true;
break;
}
}
if (!properChecksHappened) {
addViolation(data, node);
}
}
}
private void checkForAccessibility(final AbstractApexNode<?> node, Object data) {
final ASTMethod wrappingMethod = node.getFirstParentOfType(ASTMethod.class);
final ASTUserClass wrappingClass = node.getFirstParentOfType(ASTUserClass.class);
if (Helper.isTestMethodOrClass(wrappingClass) || Helper.isTestMethodOrClass(wrappingMethod)) {
return;
}
final ASTVariableDeclaration variableDecl = node.getFirstParentOfType(ASTVariableDeclaration.class);
if (variableDecl != null) {
String type = variableDecl.getNode().getLocalInfo().getType().getApexName();
StringBuilder typeCheck = new StringBuilder().append(variableDecl.getNode().getDefiningType().getApexName())
.append(":").append(type);
validateCRUDCheckPresent(node, data, ANY, typeCheck.toString());
}
final ASTAssignmentExpression assignment = node.getFirstParentOfType(ASTAssignmentExpression.class);
if (assignment != null) {
final ASTVariableExpression variable = assignment.getFirstChildOfType(ASTVariableExpression.class);
StringBuilder variableWithClass = new StringBuilder()
.append(variable.getNode().getDefiningType().getApexName()).append(":")
.append(variable.getNode().getIdentifier().value);
if (varToTypeMapping.containsKey(variableWithClass.toString())) {
String type = varToTypeMapping.get(variableWithClass.toString());
validateCRUDCheckPresent(node, data, ANY, type);
}
}
}
}

194
pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/security/xml/ApexCRUDViolation.xml
View File

@@ -2,8 +2,82 @@
<test-data>
<test-code>
<description>Proper CRUD,FLS via getters</description>
<test-code>
<description>No CRUD,FLS needed via sObject property</description>
<expected-problems>1</expected-problems>
<code><![CDATA[
public class Foo {
public Contact contactProperty { get; set; }
public void foo(String tempID) {
contactProperty = [SELECT Name FROM Contact WHERE Id=:tempID];
}
}
]]></code>
</test-code>
<test-code>
<description>Proper FLS check </description>
<expected-problems>0</expected-problems>
<code><![CDATA[
public class Foo {
public void foo(String newName, String tempID) {
List<String> fieldsToView = new List<String> { 'name' };
if (ESAPI.accessController().isAuthorizedToUpdate(Contact.sObject, fieldsToView)) {
Contact c = [SELECT Name FROM Contact WHERE Id=:tempID];
c.Name = newName;
update c;
}
}
} ]]></code>
</test-code>
<test-code>
<description>Proper FLS fall-through check </description>
<expected-problems>0</expected-problems>
<code><![CDATA[
public class Foo {
public void foo(String newName, String tempID) {
if (Schema.sObjectType.Contact.fields.Name.isUpdateable()) {
Contact c = [SELECT Name FROM Contact WHERE Id=:tempID];
c.Name = newName;
update c;
}
}
} ]]></code>
</test-code>
<test-code>
<description>Improper accessibility CRUD </description>
<expected-problems>1</expected-problems>
<code><![CDATA[
public class Foo {
public Contact foo(String tempID) {
Contact c = [SELECT Name FROM Contact WHERE Id=:tempID];
return c;
}
} ]]></code>
</test-code>
<test-code>
<description>Proper accessibility CRUD,FLS </description>
<expected-problems>0</expected-problems>
<code><![CDATA[
public class Foo {
public Contact foo(String tempID) {
if (Contact.sObjectType.getDescribe().isAccessible()) {
Contact c = [SELECT Name FROM Contact WHERE Id=:tempID];
return c;
}
return null;
}
} ]]></code>
</test-code>
<test-code>
<description>Proper CRUD,FLS via property</description>
<expected-problems>0</expected-problems>
<code><![CDATA[
public class Foo {
@@ -12,7 +86,7 @@ public class Foo {
public void foo(String newName, String tempID) {
if (Contact.sObjectType.getDescribe().isCreateable()) {
MyWriteOnlyProp = new Contact(FirstName = 'First', LastName = 'Last', Phone = '414-414-4414');
upsert MyWriteOnlyProp;
insert MyWriteOnlyProp;
}
}
@@ -21,8 +95,9 @@ public class Foo {
</test-code>
<test-code>
<description>No CRUD,FLS via getters</description>
<test-code>
<description>No CRUD,FLS via sObject property, write is not protected
</description>
<expected-problems>1</expected-problems>
<code><![CDATA[
public class Foo {
@@ -30,19 +105,49 @@ public class Foo {
public void foo(String newName, String tempID) {
MyWriteOnlyProp = new Contact(FirstName = 'First', LastName = 'Last', Phone = '414-414-4414');
upsert MyWriteOnlyProp;
insert MyWriteOnlyProp;
}
}
]]></code>
</test-code>
<test-code>
<description>CRUD,FLS via upsert</description>
<description>No CRUD,FLS via String property</description>
<expected-problems>1</expected-problems>
<code><![CDATA[
public class Foo {
public String nameProperty { get; set; }
public void foo(String tempID) {
Contact c = [SELECT Name FROM Contact WHERE Id=:tempID];
nameProperty = c.Name;
}
}
]]></code>
</test-code>
<test-code>
<description>Proper CRUD,FLS via upsert</description>
<expected-problems>0</expected-problems>
<code><![CDATA[
public class Foo {
public void foo(String newName, String tempID) {
if (Contact.sObjectType.getDescribe().isCreateable()) {
if (Contact.sObjectType.getDescribe().isCreateable() && Contact.sObjectType.getDescribe().isUpdateable()) {
Contact myCon = new Contact(FirstName = 'First', LastName = 'Last', Phone = '414-414-4414');
upsert myCon;
}
}
} ]]></code>
</test-code>
<test-code>
<description>Partial CRUD,FLS via upsert</description>
<expected-problems>1</expected-problems>
<code><![CDATA[
public class Foo {
public void foo(String newName, String tempID) {
if (Contact.sObjectType.getDescribe().isUpdateable()) {
Contact myCon = new Contact(FirstName = 'First', LastName = 'Last', Phone = '414-414-4414');
upsert myCon;
}
@@ -52,7 +157,7 @@ public class Foo {
<test-code>
<description> No CRUD,FLS via upsert</description>
<expected-problems>1</expected-problems>
<expected-problems>2</expected-problems>
<code><![CDATA[
public class Foo {
public void foo(String newName, String tempID) {
@@ -63,13 +168,15 @@ public class Foo {
</test-code>
<test-code>
<description>Proper CRUD,FLS via ESAPI</description>
<expected-problems>0</expected-problems>
<description>Improper CRUD,FLS via ESAPI 1</description>
<expected-problems>2</expected-problems>
<code><![CDATA[
public class Foo {
public void foo(String newName, String tempID) {
// missing accessibility check
Contact c = [SELECT Name FROM Contact WHERE Id=:tempID];
List<String> fieldsToView = new List<String> { 'name' };
// wrong ESAPI check
if (ESAPI.accessController().isAuthorizedToView(Contact.sObject, fieldsToView)) {
c.Name = newName;
update c;
@@ -78,14 +185,37 @@ public class Foo {
} ]]></code>
</test-code>
<test-code>
<description>Improper CRUD,FLS via ESAPI</description>
<expected-problems>1</expected-problems>
<description>Proper CRUD,FLS via ESAPI</description>
<expected-problems>0</expected-problems>
<code><![CDATA[
public class Foo {
public void foo(String newName, String tempID) {
List<String> fieldsToView = new List<String> { 'name' };
if (ESAPI.accessController().isAuthorizedToView(Contact.sObject, fieldsToView)) {
Contact c = [SELECT Name FROM Contact WHERE Id=:tempID];
if (ESAPI.accessController().isAuthorizedToUpdate(Contact.sObject, fieldsToView)) {
c.Name = newName;
update c;
}
}
}
} ]]></code>
</test-code>
<test-code>
<description>Improper CRUD,FLS via ESAPI 2</description>
<expected-problems>2</expected-problems>
<code><![CDATA[
public class Foo {
public void foo(String newName, String tempID) {
// missing accessibility check
Contact c = [SELECT Name FROM Contact WHERE Id=:tempID];
List<String> fieldsToView = new List<String> { 'name' };
// wrong object check
if (ESAPI.accessController().isAuthorizedToView(Lead.sObject, fieldsToView)) {
c.Name = newName;
update c;
@@ -100,10 +230,13 @@ public class Foo {
<code><![CDATA[
public class Foo {
public void foo(String newName, String tempID) {
Contact c = [SELECT Name FROM Contact WHERE Id=:tempID];
if(Schema.sObjectType.Contact.fields.Name.isUpdateable()) {
c.Name = newName;
update c;
List<String> fieldsToView = new List<String> { 'name' };
if (ESAPI.accessController().isAuthorizedToView(Contact.sObject, fieldsToView)) {
Contact c = [SELECT Name FROM Contact WHERE Id=:tempID];
if(Schema.sObjectType.Contact.fields.Name.isUpdateable()) {
c.Name = newName;
update c;
}
}
}
} ]]></code>
@@ -112,7 +245,7 @@ public class Foo {
<test-code>
<description>No CRUD,FLS check for update</description>
<expected-problems>1</expected-problems>
<expected-problems>2</expected-problems>
<code><![CDATA[
public class Foo {
public void foo(String newName, String tempID) {
@@ -130,11 +263,13 @@ public class Foo {
<code><![CDATA[
public class Foo {
public Contact foo(String newName, String tempID) {
Contact c = [SELECT Name FROM Contact WHERE Id=:tempID];
if (!Schema.sObjectType.Contact.fields.Name.isUpdateable()){
return null;
}
c.Name = newName;
if (Contact.sObjectType.getDescribe().isAccessible()) {
Contact c = [SELECT Name FROM Contact WHERE Id=:tempID];
if (!Schema.sObjectType.Contact.fields.Name.isUpdateable()){
return null;
}
c.Name = newName;
}
update c;
return c;
}
@@ -171,7 +306,7 @@ public class Foo {
<test-code>
<description>No CRUD check for delete</description>
<expected-problems>1</expected-problems>
<expected-problems>2</expected-problems>
<code><![CDATA[
public class Foo {
public void bar(String contactId) {
@@ -188,13 +323,14 @@ public class Foo {
<code><![CDATA[
public class Foo {
public void bar(String contactId) {
if (Contact.sObjectType.getDescribe().isDeletable()) {
Contact toDelete = [SELECT Id FROM Contact WHERE Id=: contactId];
delete toDelete;
}
if (Contact.sObjectType.getDescribe().isAccessible()) {
Contact toDelete = [SELECT Id FROM Contact WHERE Id=: contactId];
if (Contact.sObjectType.getDescribe().isDeletable()) {
delete toDelete;
}
}
}
}
]]></code>
</test-code>
</test-data>