4

I am using a dynamic query in one of our AppExchange products.

There is an LWC component within our app, which has a property called filterLogic as below:

<targetConfig targets="lightning__RecordPage">
   <property name="filterLogic" label="Filter Logic" type="String"/>
</targetConfig>

Admin user can drag and drop the component on a Standard record page and in the properties for the component they can configure a filter logic as below:

Filter Logic: (Email <> NULL and RecordType.Name like 'Employee')

This filter logic will be used in an dyanmic query like below (sample code):

string strQuery = 'SELECT Name FROM Account WHERE ' + filterLogic;
List<sObject> lstResult = Database.query(strQuery);

Question: As the field Filter Logic will be always under control of a Salesforce System admin user and not a business or community user, is it okay to add the above code as False Positive? Because, if we escapeSingleQuote for the above query, it will not work and we need to implement a custom implementation to generate dynamic filters.

6

No this is not a false positive and your app can fail Salesforce security review process due to this!

Since this is a user input there are chances that users can put in a SOQL that can have a security flaw!

It's always recommended you put enough guardrails and security checks in your code.

I would make sure to adopt techniques such as character replace or block listing to secure the code.

Read through all the techniques explained here to make sure you have sufficient validations in your code or have a UI that's less free text and more of a picker so user does not enter something that can cause issue.

5
  • 4
    Going off of this, a Salesforce System Admin is just a user with an admin profile - there's no telling their knowledge/training or whether they should even have that profile. If you allow it, they might put in something that could do unintended harm. Dec 24 '20 at 16:40
  • 2
    Even if the component property is only ever configured by an admin, it may still be possible fo ra non admin user to use their browser debugger to intercept the property and change the filter string before it gets sent to your apex code
    – Sean
    Dec 24 '20 at 16:44
  • 1
    Ya I will be surprised if this passes sec review process! It is a security hole if they do not validate this. Dec 24 '20 at 16:48
  • 1
    Thanks a lot Mohit for the answer. Sure, we will make sure we have enough security checks and sufficient validation in the code.
    – Ashish
    Dec 24 '20 at 17:02
  • 1
    You cannot escape quotes in this context; this isn't a value that is being inserted, it is one or more WHERE clause terms. See my answer for other details.
    – Phil W
    Dec 25 '20 at 17:24
5

Mohith is correct that this isn't a simple "false positive" and you do need to engineer the use of the setting/property appropriately. Since this is actually looking to provide a filter, and thus is providing content for an SOQL (or SOSL) WHERE clause, this needs to be handled in a different way to handling security concerns for user inputs that are themselves inserted as values.

Based on our own experiences doing much the same and passing security review:

  • Always insert the filter expression into the query by wrapping it in parentheses. This has several benefits:
    • If you have other WHERE clause terms that are always applied, the inserted filter expression will be applied with appropriate precedence in comparison, regardless of how it is internally structured
    • Any attempt to use other keywords, such as GROUP BY or LIMIT in the filter expression, will result in syntax errors rather than exposing a vector for security circumvention
  • Explicitly comment the absence of use of single quote escaping as reasonable, and not a security issue, given that the filter is not a value but instead one or more WHERE clause terms.

It should be noted that, since you have used parentheses, you can be certain that only an entirely balanced WHERE clause snippet will work. Even if someone tried to inject inappropriate terms, such as ORDER BY, into the snippet this will not work; there would be an unbalanced number of parentheses in the query and the query will not be executed or these keywords would be embedded within the WHERE which is not legal SOQL/SOSL.

3
  • 1
    This is great!! I have in the past built UI to provide ability to filter the query than leaving open ended like a text field! Makes it bit more easier to secure Dec 25 '20 at 18:24
  • Thanks Phil and Mohit for the detailed explanation. It is very helpful. Mohit, we also think about custom UI for filters. We have used it in another product. But, because in this component we are using "Design Attributes", we don't have much customization options in "Design Attributes" (<targetConfig targets="lightning__RecordPage">). The filterLogic variable can be different for different lists on a single App Builder page. Can you please suggest if there is a way to customize the "Design Attributes" screen?
    – Ashish
    Dec 28 '20 at 6:38
  • 1
    AFAIK the only customization supported is for drop list selection of values, so I think you cannot take that approach here. But like I said, there's no need at least from the security review perspective.
    – Phil W
    Dec 28 '20 at 9:28

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.