Take the 2-minute tour ×
Salesforce Stack Exchange is a question and answer site for Salesforce administrators, implementation experts, developers and anybody in-between. It's 100% free, no registration required.

I'm trying to use this method in a dynamic SOQL query. Follow the sample code:

public static String getRowById(String sobjName, id id) {
    String query = 'SELECT ';            
    Map<String, Schema.SObjectField> objectFields =
        Schema.getGlobalDescribe().get(sobjName).getDescribe().fields.getMap();
    for (String f : objectFields.keySet()) {
        query += f;
        query += ',';
    }

    query = query.substring(0, query.length() - 1);
    query += ' FROM '+ sobjName;
    query += ' WHERE id = \'' + String.escapeSingleQuotes(id) + '\'';
    query += ' LIMIT 1';

    return query;
}

The problem is that this is still considered as a SOQL Injection vulnerability.

Am I missing something here? I also tried to use it as a parameter like this:

getRowById('SFT_Brand__c',
    String.escapeSingleQuotes(ApexPages.currentPage().getParameters().get('id')));
share|improve this question
3  
I'd consider the sobjName argument to be the bigger injection risk if it is coming from the query string parameters. Also, I'd be surprised in the Id type could contain single quote characters. Look at validating if sobjName is a valid sObject? –  Daniel Ballinger Feb 14 '13 at 19:19
2  
@DanielBallinger: but doesn't the use of sObjName in the Schema.getGlobalDescribe() call verify it's valid? It would throw an exception there if it wasn't. If OP is always hard-coding the object name in his calls, I can't see an injection vulnerability. –  tomlogic Feb 14 '13 at 20:43
    
@tomlogic: Yes, I think you are correct. Schema.getGlobalDescribe().get(sobjName) returns null for an unknown type. –  Daniel Ballinger Feb 14 '13 at 21:28
    
When SQL injection succeeds, can you give us the value of query variable using System.debug? –  Suman Krishna Saha Feb 15 '13 at 5:10
    
Value of query: select id,sft_brand_component_type__c,name,isdeleted,sft_brand_tire_type__c,createddate,s‌​ft_brand_vehicle_type__c,systemmodstamp,ownerid,lastmodifiedbyid,createdbyid,lastm‌​odifieddate from SFT_Brand__c where id = 'a04U0000006id9NIAQ' limit 1 –  vanderlindo Feb 15 '13 at 11:11

1 Answer 1

up vote 3 down vote accepted

You are using it correctly. Here's a simplified version of your script, using the new String.join() method and all of the string concatenations in one statement instead of spread out over multiple statements.

public static String getRowById(String sobjName, Id id) {
    Map<String, Schema.SObjectField> objectFields =
        Schema.getGlobalDescribe().get(sObjName).getDescribe().fields.getMap();

    return 'SELECT ' + String.join( objectFields.keySet(), ', ')
           + ' FROM ' + sObjName
           + ' WHERE Id = \'' + String.escapeSingleQuotes(Id) + '\''
           + ' LIMIT 1';
}

You are correctly escaping the id parameter, but that might not even be necessary, since it's an Id and not a String.

This looks to me like a false positive by the security scan. If someone disagrees, they should provide a sample exploit that:

  • Doesn't throw an exception when sObjName is used to get the field list.
  • Accepts an invalid Id (15 or 18-character alphanumeric) as the second parameter.
share|improve this answer
1  
I'd also suggest adding with sharing to the class definition so that the user doesn't get sObject access beyond their permissions. Maybe check if Schema.getGlobalDescribe().get(sObjName) returns null, indicating an unknown sObjName? –  Daniel Ballinger Feb 14 '13 at 21:31

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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