pmd / pmd

An extensible multilanguage static code analyzer.
https://pmd.github.io
Other
4.85k stars 1.5k forks source link

[apex] ApexCRUDViolation on Lists of Objects #3877

Open aaronmorris opened 2 years ago

aaronmorris commented 2 years ago

Affects PMD Version: 6.42.0+ 7.0.0

Rule: ApexCRUDViolation

Description: I'm getting the "Validate CRUD permission before SOQL/DML operation" error when updating a list of a custom object, but not on the individual.

Code Sample demonstrating the issue:

public static CustomObject__c updateCustomObject(CustomObject__c customObject) {
    if (CustomObject__c.getSObjectType().getDescribe().isUpdateable() == false) {
        throw new DmlException('No permissions to update.');
    }

    // This is fine with no PMD Error
    update resultObject;
    return resultObject;
}

public static List<CustomObject__c> updateCustomObjects(List<CustomObject__c> customObjects) {
    if (CustomObject__c.getSObjectType().getDescribe().isUpdateable() == false) {
        throw new DmlException('No permissions to update.');
    }

    // Getting PMD Error: Validate CRUD permission before SOQL/DML operation
    update resultObjects;
    return resultObjects;
}

Expected outcome: PMD reports a violation at line ..., but that's wrong. That's a false positive. Since I've checked the object is Updateable, shouldn't that apply to individuals and lists

Running PMD through: [CLI | Ant | Maven | Gradle | Designer | Other] ApexPMD Extension in VSCode - chuckjonas.apex-pmd (https://marketplace.visualstudio.com/items?itemName=chuckjonas.apex-pmd)

sreenath-tm commented 2 years ago

I'm interested in contributing to this issue, so before I start working it, would you mind sparing your time explaining what the fix will be like and pointing me to some resources to get started.

adangel commented 2 years ago

Hi @sreenath-tm,

here are a few pointers to get you started:

The specific rule is implemented here: https://github.com/pmd/pmd/blob/master/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexCRUDViolationRule.java

The test cases are in an xml file: https://github.com/pmd/pmd/blob/master/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/security/xml/ApexCRUDViolation.xml

For understanding the test framework, see https://pmd.github.io/latest/pmd_userdocs_extending_testing.html

The first step would be to add a test case to reproduce the issue. And then dig into the code of the rule to figure out, what's the problem.

lfalcantar commented 2 years ago

@sreenath-tm are you still working on this, otherwise I will like to take it.

sternlexa commented 1 year ago

I have exactly the same problem with custom objects. Has anyone found a solution?

adangel commented 1 year ago

I had a look at the problem and I can't directly reproduce it... without a SOQL expression, the rule is not triggered at all.

So, my sample code looks like this:

public class Foo {
    public static List<CustomObject__c> updateCustomObjects() {
        List<CustomObject__c> customObjects = [SELECT CustomObject__c FROM CustomObject WITH SECURITY_ENFORCED];
        if (CustomObject__c.SObjectType.getDescribe().isUpdateable() == false) {
            throw new DmlException('No permissions to update.');
        }

        // Getting PMD Error: Validate CRUD permission before SOQL/DML operation
        update customObjects;
        return customObjects;
    }
}

With that, it works.

Note, the difference in accessing the SObjectType:

CustomObject__c.SObjectType.getDescribe().isUpdateable()

vs.

CustomObject__c.getSObjectType().getDescribe().isUpdateable()

Are these two equivalent? If yes, then the problem is, that the rule doesn't support the getter, it only supports the property SObjectType...

SurinderjitBajwa commented 1 year ago

CWS_AzureAccessTokenc existingToken = [Select ID, Name, CWS_Access_Tokenc, CWS_Expire_Atc From CWS_AzureAccessTokenc WITH SECURITY_ENFORCED Limit 1]; DateTime currentDateTime = DateTime.now();
if(existingToken != null){
if(Schema.SObjectType.CWS_AzureAccessToken
c.IsUpdateable() == true
&& Schema.SObjectType.CWS_AzureAccessToken
c.fields.CWS_Access_Token__c.IsUpdateable() == true && Schema.SObjectType.CWS_AzureAccessTokenc.fields.CWS_Expire_Atc.IsUpdateable() == true ){
existingToken.CWS_Access_Tokenc = newAccessToken; existingToken.CWS_Expire_Atc = currentDateTime.addSeconds(3000); Update existingToken;
// Still getting PMD error on this line. }else{ throwDMLException(); } } image