forcedotcom / sfdx-scanner

MIT License
212 stars 49 forks source link

[BUG] Wrong Apex SOQL Injection Error & #1200

Closed HeartlyHarsh closed 3 months ago

HeartlyHarsh commented 11 months ago

In my Apex Class, I am receiving a recordId as String in method. Using that recordId, I am doing dynamic SOQL query using Database.query.(query) method.

For prevent ApexSOQLInjection I have used Strip.escapeSingleQuotes(recordId) method while creating string for the Database query.

While scanning the code using code analyzer for first run, I am still getting related to ApexSOQLInjection. Which shows Rule -> "ApexSOQLInjectio"n & Description -> "Avoid untrusted/unescaped variables in DML query"

I have below code.

databaseQuery = 'SELECT ' + currentRecordFields + ' FROM ' + objectApiName + ' WHERE Id = \'' + String.escapeSingleQuotes(recordId) + '\''; currentSObjectRecordList = Database.query(databaseQuery);

While scanning my Apex Code for the Second run, It shows error as below, While my method start scanning. Rule - > InternalExecutionError Category -> InternalExecutionError Description -> Graph Engine identified your source and sink, but you must manually verify that you have a sanitizer in this path. Then, add an engine directive to skip the path. Next, create a Github issue for the Code Analyzer team that includes the error and stack trace. After we fix this issue, check the Code Analyzer release notes for more info. Error and stacktrace: ClassCastException: class com.salesforce.graph.vertex.VariableExpressionVertex$Single cannot be cast to class com.salesforce.graph.vertex.SoqlExpressionVertex (com.salesforce.graph.vertex.VariableExpressionVertex$Single and com.salesforce.graph.vertex.SoqlExpressionVertex are in unnamed module of loader 'app'): com.salesforce.graph.symbols.apex.system.SObjectAccessDecision.buildSanitizedValue(SObjectAccessDecision.java:171);com.salesforce.graph.symbols.apex.system.SObjectAccessDecision.executeMethod(SObjectAccessDecision.java:112);com.salesforce.graph.symbols.PathScopeVisitor.afterMethodCall(PathScopeVisitor.java:659);com.salesforce.graph.symbols.DefaultSymbolProviderVertexVisitor.afterMethodCall(DefaultSymbolProviderVertexVisitor.java:318);com.salesforce.graph.ops.expander.ApexPathExpander.handleMethodCall(ApexPathExpander.java:681);com.salesforce.graph.ops.expander.ApexPathExpander.visit(ApexPathExpander.java:532)

Is there any issue with my code or with Code Analyzer. Do let me know if there is any other way to scan my code?

jfeingold35 commented 11 months ago

@HeartlyHarsh , can you please rewrite this to comply with our issue template? And can you please provide more information about your code? I'm unable to reproduce, but I can't be sure I'm properly replicating your setup without more information (e.g., what are the types of currentRecordFields, objectApiName and currentSObjectRecordList? Where are they declared? How are they set?).

HeartlyHarsh commented 11 months ago

currentSObjectRecordList is a List of Sobject, I inialized it as below. List currentSObjectRecordList = new List();

currentRecordFields is a String, I have defined my code, where I have some logic and I am creating comma separated String of field's api name for soql query.

objectApiName is String, I am receiving from LWC component. Its an object's api name.

recordId, I am getting from LWC from lighting record page where I have placed my component.

jfeingold35 commented 11 months ago

@HeartlyHarsh , I'm still unable to reproduce this on my end. Is it possible for you to provide a complete code block that reproduces the error?

HeartlyHarsh commented 11 months ago

Hi, Regret for delay response.

I have attached here code & the csv file which were generated post Code Scan using code ananlyzer.

CodeAnalyzerGeneral.csv DynamicController.txt

jfeingold35 commented 11 months ago

@HeartlyHarsh , I'm unable to reproduce with this file alone. Have you verified that scanning this file is enough to cause the error on your end?

HeartlyHarsh commented 11 months ago

@jfeingold35, I have created a separate Project in VS-Code & retrieved just on class from the Org & I have scan that class using Code Analyzer using code.

I have referred below link document. https://developer.salesforce.com/docs/atlas.en-us.packagingGuide.meta/packagingGuide/security_review_code_analyzer_scan.htm

For scanning, I had used the example available in above link page example. During that scanning, whatever the log is generated I have shared with you.

In log it shows that on line 60, there is an ApexSOQLInjection rule violated. While I have already used "String.escapeSingleQuotes()" for prevent those attacks.

But code analyzer still showing the error.

Please let me know, if you did not understand anything? Or you need something else related to code.

git2gus[bot] commented 7 months ago

This issue has been linked to a new work item: W-15080478

rmohan20 commented 6 months ago

Duplicate of #1384

stephen-carter-at-sf commented 4 months ago

@HeartlyHarsh What command are you running?

I'm a little confused with the above comments. It seems you the first post refers to the run dfa command (with the Salesforce Graph Engine) but the subsequent comments seem to be with the run command (with the PMD engin).

Also can you check to see if you can reproduce this issue with our latest salesforce code analyzer v4 "latest-beta"? Do an sf plugins install @salesforce/sfdx-scanner@latest-beta to get the latest.

stephen-carter-at-sf commented 4 months ago

@HeartlyHarsh

If this is referring to PMD, then I am able to get

  {
    "line": 45,
    "column": 67,
    "endLine": 45,
    "endColumn": 80,
    "severity": 3,
    "ruleName": "ApexSOQLInjection",
    "category": "Security",
    "url": "https://docs.pmd-code.org/pmd-doc-7.0.0/pmd_rules_apex_security.html#apexsoqlinjection",
    "message": "\nAvoid untrusted/unescaped variables in DML query\n"
  }

If this is the issue you are referring to, then this is coming directly from PMD and not Salesforce Code Analyzer. In that case, can you please report this issue over in PMD's repo: https://github.com/pmd/pmd/issues and then close this issue?

stephen-carter-at-sf commented 3 months ago

Closing this issue due to lack of response. Feel free to reopen this issue if needed.