1

Scenario:

While updating some code to satisfy SOQL injection issues brought up by security review report, I stumbled across a code that was not brought up. I thought it was similar to the code in the report, so I proceeded to apply the same change. In this case, the recordId originated from a URL parameter while the objName is not. This is the code before:

private static Boolean valId(String recordId, String objName) {
    String id = String.escapeSingleQuotes(recordId);
    if((id.length() == 15 || id.length() == 18) && Pattern.matches('^[a-zA-Z0-9]*$', id)) {
        return (database.countQuery('SELECT count() FROM ' + objName + ' WHERE Id = \'' + id + '\'') > 0) ? true : false;
    }
    return false;
}

This is the code after making the changes:

private static Boolean valId(String recordId, String objName) {
    String id = String.escapeSingleQuotes(recordId);
    if((id.length() == 15 || id.length() == 18) && Pattern.matches('^[a-zA-Z0-9]*$', id)) {
        return (database.countQuery('SELECT count() FROM :objName WHERE Id = :id ') > 0) ? true : false;
    }
    return false;
}

Problem:

I am getting the following error:

FATAL_ERROR System.QueryException: unexpected token: ':'

I get the same error when I simplified the code in an Execute Anonymous Window

String objName = 'User';
system.debug(database.countQuery('SELECT count() FROM :objName'));

Other Observations:

It seems countQuery() returns an error when a colon-prefixed variable is used in the FROM section of the query. I was able to run the query successfully by hard-coding the Object name. And after trying the alternative (see Attempted Fixes), AggregateResult also has the same restriction.

Attempted Fixes:

I've looked at the question posted Return type of count() versus count(fieldName) and the alternative seems to be using an AggregateResult list.

Question:

How do I keep my code dry for this particular method without relying on SOQL injection code practice?

Update:

So here's the workaround I used... sigh

private static Boolean valId(String recordId, String objName) {
    String id = String.escapeSingleQuotes(recordId);
    if((id.length() == 15 || id.length() == 18) && Pattern.matches('^[a-zA-Z0-9]*$', id)) {
        if (objName == 'User') {
            return (database.countQuery('SELECT count() FROM User WHERE Id = :id') > 0) ? true : false;
        } else if (objName == 'Report') {
            return (database.countQuery('SELECT count() FROM Report WHERE Id = :id') > 0) ? true : false;
        }
    }
    return false;
}
  • 2
    You can't merge in the object type. Must use literal. – Adrian Larson Jul 29 '16 at 0:00
  • Hi @Adrian. I'd like to be able to assign any object name when calling the method without resorting to concatenating the SOQL string as the I think security review considers that a weakness. I updated my question with the workaround using literals, much to my dismay. Thank you for your useful comment. – Bryan Jul 29 '16 at 0:55
  • Near duplicate: Genuine SOQL Injection issue or False Positive? – Adrian Larson Jul 29 '16 at 1:07
3

There's really no reason that you need to do all that fancy footwork. You can just trust the system to tell you what you need to know. Here's an implementation that I whipped up for you:

public class Utils {
    static Boolean validId(String recordIdString) {
        try {
            Id recordId = (Id)recordIdString;
            String sobjectName = String.valueOf(recordId.getSObjectType());
            return Database.countQuery('SELECT COUNT() FROM '+sobjectName+' WHERE Id = :recordId') > 0;
        } catch(Exception e) {
            return false;
        }
    }
}

I've also posted this as a gist.

The try-catch block will catch the TypeException if it's not a valid Id (from the cast), or a NullPointerException if it's a null Id, and possibly a QueryException if the Id points to an object that can't be queried (rare, but they are there).

I'm pretty sure that even the automated scanner won't give you a red flag for this code, but even if it did, a simple review should clearly identify that no injection is possible as long as it's used in the appropriate sharing context; user-facing code should be "with sharing" to prevent leaking Id values they should't know about.


You can actually do this without a try-catch as well:

static Boolean validId(String recordIdString) {
    return recordIdString instanceOf Id &&
         Id.valueOf(recordIdString).getSObjectType().getDescribe().isQueryable() &&
         0 < Database.countQuery(
             'SELECT COUNT() FROM '+
             String.valueOf(Id.valueOf(recordIdString).getSObjectType())+
             ' WHERE Id = :recordIdString');
    }
}

I find this to be a little less readable, but is Exception-free and probably better for performance.

Note: Id.valueOf is broken in older versions of the API, so make sure you're using a recent version of the API for your class (e.g. version 37.0).

  • Ah I didn't even notice recordId was a parameter. – Adrian Larson Jul 29 '16 at 2:39
  • I'm hesitant to up-vote a pokemon catch. Hmm. What's your opinion on catching specific exception types? I understand it adds a few extra lines in this instance, but I'm really not a fan of non-explicit catch blocks and curious to hear your thoughts. – Adrian Larson Jul 29 '16 at 4:20
  • 1
    @AdrianLarson I personally dislike catching Exception directly. It makes programmers less cognizant of the API that a language provides than they should be. I'd be more than happy to share an exception-less version of the code, but it's more verbose than the current solution. – sfdcfox Jul 29 '16 at 4:31
  • Thank you very much for the verbose answer and sharing your code. I learned one new thing from the first snippet. However, the second snippet taught me four new things, in spite being very intimidating. Regardless, I enjoyed taking the time to study it. I will definitely be keeping this one for future reference. – Bryan Jul 29 '16 at 17:10
  • 1
    @Bryan Yes, you can shorten it to just "recordIdString instanceOf Id"; it'll return false if null or not an Id, true if it is in the correct format. – sfdcfox Jul 29 '16 at 17:52

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.