5

I am getting soql injection vulnerability issue in below. How to resolve it.

class:

public class Clsname {
public String varString { get; set; }

public void updateAcc() {
    if (varString!=null) {    
        varString = String.escapesingleQuotes(varString); 
        qString += ' and ( Contact.Name like \'%' + varString + '%\') ';
    }
}
5
  • What have you tried so far? – ytiq Sep 25 '19 at 10:30
  • I have tried this "varString = String.escapesingleQuotes(varString); " as mentioned. but still getting soql injection vulnerability. – SFDC Student Sep 25 '19 at 10:31
  • 2
    Hi, when you say you get the SOQL injection vulnerability, you mean Checkmarx scan is listing this as having the vulnerability? Have you tried making it: qString += ' AND (Contact.Name LIKE \'%' + String.escapeSingleQuotes(varString) + '%\') '); Checkmarx is fragile where it comes to parsing code and factoring checks out into separate statements can easily be missed by it. – Phil W Sep 25 '19 at 10:34
  • Yes. this is listed by Checkmarx. I haven't tried - qString += ' AND (Contact.Name LIKE \'%' + String.escapeSingleQuotes(varString) + '%\') '); Can I try this ? Will it resolve the issue. I got the message like "varString element’s value then flows through the code without being properly sanitized or validated, and is eventually used in a database query in method". – SFDC Student Sep 25 '19 at 10:37
  • @Phil W: Please help – SFDC Student Sep 25 '19 at 10:52
13

I really recommend you to use Bind Expressions when you are using either static or dynamic queries (when it is possible): So in your case, your method would look like:

public void updateAcc() {
    if (varString != null) {    
        varString = '%' + varString + '%'; 
        qString += ' AND (Contact.Name LIKE :varString) ';
    }
}
4
  • 1
    You don't want to include the escapeSingleQuotes in setting varString if you are using a binding. This will prevent correct matching as the escape will be looked for. Please update your answer. – Phil W Sep 25 '19 at 15:17
  • 2
    You should also add to your answer where varString exists. This code fragment clearly expects varString to exist outside the scope of the updateAcc method (which is fine). The important point would be, however, that it must exist in a manner that it is accessible within the scope of the code that actually performs the Database.query. – Phil W Sep 25 '19 at 15:21
  • Absolutely agree – Oleh Berehovskyi Sep 25 '19 at 15:22
  • Just noting that this only works with local variables. If you're used to using this.varString, that'll not work in the dynamic query. Binding to static variables also does not work. The actual binding of the variables takes place when you execute the query, so you have to be careful when you construct the query in a separate function. Basically; always make sure you have the variables locally in the method you're executing the query in. – rael_kid Sep 26 '19 at 7:19

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.