forcedotcom / sfdx-scanner

MIT License
213 stars 49 forks source link

[BUG] "ApexFlsViolationRule" wrongly detected in Dynamic Query with USER MODE applied #1180

Open vc4u opened 1 year ago

vc4u commented 1 year ago

Describe the bug Code Scanner is unable to recognize a Dynamic Apex query with FLS implemented correctly and reporting a Security Violation of ApexFlsViolationRule in output report.

To Reproduce Scan APEX code like this: Database.query('SELECT Id, RecordTypeId, RecordType.DeveloperName FROM '+string.escapeSingleQuotes(sObjName)+' WHERE Id =: recordId', AccessLevel.USER_MODE);

Expected behavior The ApexFlsViolationRule should not be applied here, as AccessLevel.USER_MODE is already applied to Database.query method.

Desktop (please complete the following information):

"Workaround": None

"Urgency": "Business stopping" - stopping us for submitting for security review.

jfeingold35 commented 1 year ago

@vc4u , thanks for bringing this one to our attention. We'll add it to our backlog. Unfortunately, the only workaround we can recommend at this time is to switch from performing the query via Database.query to using the bracket-syntax ([SELECT whatever FROM whatever]).

git2gus[bot] commented 1 year ago

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

vc4u commented 1 year ago

Object Name being dynamic, we can't use [SELECT ...] query here. As, without Database.query we can't make it a dynamic SOQL query with support to query any object..

jfeingold35 commented 1 year ago

@vc4u , yeah, I get it. Unfortunately, we just don't have a workaround right now aside from not using a dynamic query. And our team's bandwidth is really limited right now, so I'm unable to make any guarantees about when a fix can be ready. So while I get that you'd like this false positive resolved before you submit for security review, my understanding is that the security review process allows you to document false positives for exactly this reason. So you shouldn't feel like this is blocking you from submitting for security review.

vc4u commented 1 year ago

If security review team agrees with this false positive finding, I would be the happiest man, I'll proceed with your suggestion and hope for the best! Thnx for your help so far. :-)

vc4u commented 1 year ago

@jfeingold35 We found following case failing as well, could you confirm if that would be a new bug and not the same as this?

We've a query like this:

Database.query('SELECT '+String.escapeSingleQuotes(fields)+' FROM MyObj__c WHERE ID =: recordId', AccessLevel.USER_MODE)

This would result into an error like this:

ApexFlsViolationRule Salesforce Graph Engine couldn't resolve the parameter passed to [READ] operation with field(s) [Unknown]. Confirm that this operation has the necessary FLS checks.

The above error would go away, if a query is without any concatenation string. It seems any concatenation in a query would add an incorrect ApexFlsViolationRule violation.

jfeingold35 commented 1 year ago

@vc4u , I personally would consider these to be the same issue: Database.query(X, AccessLevel.USER_MODE) should be secure, no matter what X is.

vc4u commented 4 months ago

@jfeingold35 Do we know when is the support for dynamic SOQL with user mode support coming to scanner in dfa scan? I know we can use --preview-pmd7 in run scan, but it doesn't work for dfa scanning.