[apex] False positives with ApexCRUDViolation

[trentchilders]

Affects PMD Version:
All
Rule:
ApexCrudViolation
Description:
Hello, I have encountered what I believe to be a false positive in the ApexCRUDViolation rule. Specifically the READ portion of that rule (or an isAccessible() check in Apex). I created a helper method so that I could dynamically do this check rather than hardcode every single field that I'm querying for. However, this still causes ApexCRUDViolation to fail its read check.
Code Sample demonstrating the issue:
Helper method:

public with sharing class Utilities {
  public static void checkReadAccess(Map<String,Set<String>> fieldMap){
        Set<String> keySet = fieldMap.keySet();
        
        for(String key : keySet){
            
            Schema.SObjectType objectType = Schema.getGlobalDescribe().get(key);
            
            Schema.DescribeSObjectResult describeSObject = objectType.getDescribe(); 
            if(!describeSObject.isAccessible()){
                throw new System.NoAccessException();
            }
            Map<String,Schema.SObjectField> schemaFieldMap = describeSObject.fields.getMap();
            for(String fieldName : fieldMap.get(key)){
                Schema.DescribeFieldResult describeField = schemaFieldMap.get(fieldName).getDescribe();
                if(!describeField.isAccessible()){
                    throw new System.NoAccessException();
                }
            }
        }
}

And then to call it:

Set<String> fieldSet;
fieldSet = new Set<String>{'Id','Name'};
Map<String,Set<String>> fieldMap;
fieldMap = new Map<String,Set<String>>{'Account'=>fieldSet};
Utilities.checkReadAccess(fieldMap);
List<Account> templateList = [SELECT Id, Name FROM Account];

Running PMD through: Codacy

[jsotuyod]

@trentchilders thanks for the report.

This is indeed a false positive, but it's one that PMD can't currently sort out. PMD is currently limited to same-file analysis. So, when it checks the code:

Set<String> fieldSet;
fieldSet = new Set<String>{'Id','Name'};
Map<String,Set<String>> fieldMap;
fieldMap = new Map<String,Set<String>>{'Account'=>fieldSet};
Utilities.checkReadAccess(fieldMap);
List<Account> templateList = [SELECT Id, Name FROM Account];

PMD has no idea what Utilities.checkReadAccess is, or what it does, and is therefore unable to waive this usage.

[trentchilders]

Thanks for the reply!

[trentchilders]

So, even by adding the method to the file, I still get the false positive. Is this expected behavior?

public static void Foo(){
   Set<String> fieldSet;
   fieldSet = new Set<String>{'Id','Name'};
   Map<String,Set<String>> fieldMap;
   fieldMap = new Map<String,Set<String>>{'Account'=>fieldSet};
   checkReadAccess(fieldMap);

   List<Account> templateList = [SELECT Id, Name FROM Account];
}

public static void checkReadAccess(Map<String,Set<String>> fieldMap){
        Set<String> keySet = fieldMap.keySet();
        
        for(String key : keySet){
            
            Schema.SObjectType objectType = Schema.getGlobalDescribe().get(key);
            
            Schema.DescribeSObjectResult describeSObject = objectType.getDescribe(); 
            if(!describeSObject.isAccessible()){
                throw new System.NoAccessException();
            }
            Map<String,Schema.SObjectField> schemaFieldMap = describeSObject.fields.getMap();
            for(String fieldName : fieldMap.get(key)){
                Schema.DescribeFieldResult describeField = schemaFieldMap.get(fieldName).getDescribe();
                if(!describeField.isAccessible()){
                    throw new System.NoAccessException();
                }
            }
        }

[jsotuyod]

@trentchilders yes, this is expected. As you use a Map and Set, working this out would require not only having access to checkReadAccess but understanding how the map / set works (what get(key) does, etc.) and following through the code which values are in the map and set.

The one thing we support is a List of fields when using the ESAPI:

List<String> fieldsToView = new List<String> { 'Id', 'Name' };
if (ESAPI.accessController().isAuthorizedToView(Account.sObject, fieldsToView)) {
    …
}

but support for this is very tailor made.

Moreover, the check is currently very naive, as Apex has no real control flow support (unlike Java). This means, we can understand:

Account.sObjectType.getDescribe().isAccessible()

But we don't follow through a split statement

Schema.DescribeFieldResult describeField = Account.sObjectType.getDescribe()
describeField.isAccessible()

There is plenty of room for improvement, shall anyone be willing to take on this. Adding full support for control flow on Apex is a major task, but one that would greatly help many rules besides this one.

[boBNunny]

I know you are looking "in stream" for the Schema check, but maybe you could look for the same final method names like "isAccessible", "isCreatable", etc. In this way, we could create Utility methods to do the checks and make it standardized instead of repeating the multi-line IF statements for (in our case) 3000 SOQL's/DML's. We created 4 Utility methods to meet the "spirit" of the security check, but PMD still identifies them.