3

I went through the trailhead module on SOQL injection, however I am not clear on below items:

Scenario 1) Is escapeSingleQuotes here needed ?
// Consider that userinput is coming from user 
userinput = String.escapeSingleQuotes(userinput); // Is this needed ? 
String soql = 'SELECT Id FROM Account WHERE Name =:userinput';
System.debug(Database.query(soql));

In above case, if there was concatenation, then I can see the risk of SOQL injection but when variable binding happens, then I am not very sure if escaping is needed. If escaping is needed, then I would love to see example where if escaping is not done, how soql injection can happen with binding of variables.

Scenario 2) Is escapeSingleQuotes here needed ?
// Consider names is holding list of names and its coming from user
List<String> escapedNames = new List<String>();
for(String name : names){
  escapedNames.add(String.escapeSingleQuotes(name)); // Is this needed ?
}
String soql = 'SELECT Id FROM Account WHERE Name IN:escapedNames';
System.debug(Database.query(soql));

Note: I have went through many stack exchange questions and I see the suggestion of preferring static query, which I agree. However, I would like to understand what should be done in the cases of dynamic queries in which some form of concatenation is used along with binding of variables. To keep example simple, I did not include example of concatenation + binding.

2 Answers 2

5

Both scenarios are false positives if flagged by PMD, Code Analyser or Checkmarx.

An attempt to escape values used in bindings will actually prevent correct matching. Imagine a Contact LastName like "O'Connor". If you escape the value used in a binding, this Contact will not be found since it would be searching for "O\'Connor" instead.

The best solution is to flag as a false positive in the code with a trivial explanation like:

SOQL Injection False Positive: the values are in a binding

Making this a comment on the end of the first line of the statement is ideal since the Checkmarx report will then include this text, so the security review team see the explanation immediately.

Our false positives documentation only covers a small number of specific cases and otherwise states that the code comments in the Checkmarx report explain the false positives.

3

We faced this conflict in one of our security meetings. This query is not subjected to ApexSOQLInjection because a binding variable ensures that the value is safely inserted into the query.

But this gets flagged by CheckMarx or PMD. So it all comes down to how can you convince security partner that this is not ApexSOQLInjection and how much time do you have to convince them.

Or is it simple to just add a escapeSingleQuotes check and dont let the security system flag this.

We choose the second one and added escapeSingleQuotes whereever this was flagged.

4
  • You should flag it as a false positive. Escaping the values could easily prevent values being matched unless you do exactly the same when also performing DML to store the data (and that includes throughthe Salesforce UI, where you do not have direct control). Imagine a Contact LastName like "O'Connor". If you escape the value used in a binding, this Contact will not be found.
    – Phil W
    Oct 5, 2023 at 6:00
  • Agreed, but for us even making the security team understand this is false positive was taking huge time, even with examples. If it would have been updated in documentation that binding variables when used in static or dynamic query, wont cause SOQL injection, it would be awesome Oct 5, 2023 at 6:07
  • Does not match my experience which was easily addressed as outlined in my answer.
    – Phil W
    Oct 5, 2023 at 6:42
  • Ease of getting through a security review rather than edification to the security team actively harms other ISVs by making them go through the same process. The only way to fix the problem "for real" is to call out false positives so the security team can learn what is a real vulnerability and what is not. It's a shame that the security scanning tools we have today can't make this distinction.
    – sfdcfox
    Oct 5, 2023 at 11:04

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .