[BUG] False Positive for PerformNullCheckOnSoqlVariables with global methods

[mattlacey]

Have you tried to resolve this issue yourself first?

Yes

Bug Description

PerformNullCheckOnSoqlVariables returns a false positive in global methods.

Output / Logs

~/work/Security/ScannerTesting took 5s
󱞩  sf scanner run dfa --target ./force-app/main/default/classes/NullCheck.cls --projectdir ./force-app
Analyzing with Salesforce Graph Engine. See /Users/matt/.sfdx-scanner/sfge.log for details.... Done
 Source Location                                Sink Location                                  Description                                     Category    URL                                                                                        
 ────────────────────────────────────────────── ────────────────────────────────────────────── ─────────────────────────────────────────────── ─────────── ───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
 force-app/main/default/classes/NullCheck.cls:3 force-app/main/default/classes/NullCheck.cls:5 Null check is missing for variable name used in Performance https://developer.salesforce.com/docs/platform/salesforce-code-analyzer/guide/performnullcheckonsoqlvariables-rule.html

Steps To Reproduce

Run the scan dfa command on this class:

global with sharing class NullCheck {

    global static void NullCheckIssue(String name) {
         if (String.isNotBlank(name)) {
            List<Attachment> a = [SELECT Body, BodyLength, ContentType FROM Attachment WHERE Name = :name LIMIT 1];
         }

    }

    private static void NullCheckIssue2(String name) {
         if (String.isNotBlank(name)) {
            List<Attachment> a = [SELECT Body, BodyLength, ContentType FROM Attachment WHERE Name = :name LIMIT 1];
         }

    }

    public static void NullCheckIssue3(String name) {
         if (String.isNotBlank(name)) {
            List<Attachment> a = [SELECT Body, BodyLength, ContentType FROM Attachment WHERE Name = :name LIMIT 1];
         }

    }
}

Expected Behavior

Scanner should report no errors - instead it reports a violation for line 5, but not for 12 or 19.

Operating System

macOS Sequoia 15.0.1

Salesforce CLI Version

@salesforce/cli/2.60.13 darwin-arm64 node-v20.17.0

Code Analyzer Plugin (@salesforce/sfdx-scanner) Version

@salesforce/sfdx-scanner 4.6.0

Java Version

java 17.0.4 2022-07-19 LTS

Additional Context (Screenshots, Files, etc)

NullCheck.txt

Workaround

No response

Urgency

Low

[mattlacey]

Seems telling that the code formatter for GitHub also treats the global differently - same code parsing going on?

[jfeingold35]

@mattlacey , the method being global is irrelevant. It only matters in the code you posted because the global method is automatically a path entry point and neither of the other two methods are, so they're not being scanned. The actual problem is that the rule doesn't recognize that String.isNotBlank counts as a null check. Thanks for bringing this to our attention; we'll add it to our backlog.

[git2gus[bot]]

This issue has been linked to a new work item: W-16982105

[mattlacey]

@jfeingold35 Of course, that makes sense! This code was me trying to reproduce the issue I was seeing in our code base, it seems as though even adding the null check with the call to isNotBlank() fails:

~/work/Security/ScannerTesting
󱞩  sf scanner run dfa --target ./force-app/main/default/classes/NullCheck.cls --projectdir ./force-app
Warning: We're continually improving Salesforce Code Analyzer. Tell us what you think! Give feedback at https://research.net/r/SalesforceCA
Analyzing with Salesforce Graph Engine. See /Users/matt/.sfdx-scanner/sfge.log for details.... Done
 Source Location                                Sink Location                                  Description                                     Category    URL                                                                                        
 ────────────────────────────────────────────── ────────────────────────────────────────────── ─────────────────────────────────────────────── ─────────── ───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
 force-app/main/default/classes/NullCheck.cls:3 force-app/main/default/classes/NullCheck.cls:5 Null check is missing for variable name used in Performance https://developer.salesforce.com/docs/platform/salesforce-code-analyzer/guide/performnullcheckonsoqlvariables-rule.html
                                                                                               SOQL query.                                                                                                                                            
Executed sfge, found 1 violation(s) across 1 file(s).
Rule violations were logged to the console.

~/work/Security/ScannerTesting took 7s
󱞩  head -5 ./force-app/main/default/classes/NullCheck.cls
global with sharing class NullCheck {

    global static void NullCheckIssue(String name) {
         if (name!= null && String.isNotBlank(name)) {
            List<Attachment> a = [SELECT Body, BodyLength, ContentType FROM Attachment WHERE Name = :name LIMIT 1];