[apex] ApexCRUDViolation ignores Schema.isDeletable on Delete DML

[rsoesemann]

Affects PMD Version:
7.0.1 (and all versions before)

Rule:
ApexCRUDViolation - https://pmd.github.io/pmd/pmd_rules_apex_security.html#apexcrudviolation

Description:
PMD incorrectly reports the need for validation of CRUD permissions before a DML Delete statement although the Schema.isDeletable is called.

This issues was raised in https://salesforce.stackexchange.com/questions/421548/apexcrudviolation-check-for-object-level-delete-permission-does-not-work where the user used PMD inside the Salesforce Code Analyser CLI tool (uses PMD internally)

Code Sample demonstrating the issue:

public static void exampleMethod() {
        List<Account> accounts = [SELECT Id FROM Account WITH SECURITY_ENFORCED];
        if (Schema.SObjectType.Account.isDeletable()) {
            delete accounts;
        }
    }

Expected outcome:

PMD should report no violation.

Running PMD through: Salesforce Code Analyser CLI https://developer.salesforce.com/docs/platform/salesforce-code-analyzer/overview using:

sf scanner run --format csv --outfile CodeAnalyzerGeneral.csv --target ./ --category Security

[rsoesemann]

@adangel The PMD Security rules have mostly become inconsistent with all the changes around WITH SECURITY ENFORCED, USer Mode SOQL, User Mode DML, etc.

I believe there should be ONE opinionated rule provided and maintained by Salkesforce that:

1. Enforces User Mode SOQL and DML as in update as user accounts; +or [SELECT Field FROM Account WITH USER_MODE];
2. Discourages SECURITY ENFORCED
3. Discourages Schema checks

For sure a quick fix of the existing rule to accept the delete is also possible short term.

Related issues:

• [apex] ApexCrudViolation: Recommend USER MODE, discourage WITH SECURITY ENFORCED #4368

For visibility: @jfeingold35 @johnbelosf @kfidelak94 @anand13s @FishOfPrey

[rsoesemann]

Related Ticket on the Code Analyser repo is forcedotcom/sfdx-scanner#1458

[FishOfPrey]

In this particular scenario the best practice would be be delete as user accounts;

Any WITH SECURITY_ENFORCED scenarios have been superseded by the equivalent user mode operation.

There are scenarios where Security.stripInaccessible may make more sense, such as being able to selectively handle what occurs if the user doesn't have access. Although for a delete operation that would need to be a schema describe check.

[jh480]

Schema checks should not be discouraged, as ("managed") code within app-store components (esp. triggers) should not let users with unsufficient access run into errors (worst-case scenario: SF users cannot save their own settings because of crashing User triggers).