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;
}
3
  • 2
    You can't merge in the object type. Must use literal.
    – Adrian Larson
    Jul 29, 2016 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, 2016 at 0:55
  • Near duplicate: Genuine SOQL Injection issue or False Positive?
    – Adrian Larson
    Jul 29, 2016 at 1:07

1 Answer 1

4

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).

6
  • Ah I didn't even notice recordId was a parameter.
    – Adrian Larson
    Jul 29, 2016 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, 2016 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, 2016 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, 2016 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, 2016 at 17:52

You must log in to answer this question.

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