[False Result] 'Key__c' field flagged by ProtectSensitiveData rule

[kryvyifedir]

Have you verified this is Salesforce Code Analyzer specific?

Yes

Description

I have a 2GP package that was released on the AppExchange in June 2024. This package includes a couple of Custom sObjects that have "Keyc" fields. Since sfdx-scanner update to 4.2.0 these fields are flagged by "ProtectSensitiveData", with the issue: "Keyc is a potential auth token in the object".

Link to a .md file with the rule: https://github.com/forcedotcom/sfdx-scanner/blob/dev/pmd-appexchange/docs/ProtectSensitiveData.md

While I understand the intent behind this rule, but it has a couple of problems behind it:

1. It runs the check against metadata that you can't change after the package was released - I can't change ApiName since package was already released, so this issue will always be flagged.
2. It doesn't rely on concrete evidence that the issue is there, instead it assumes security issue is present because of the field ApiName, while it can have zero relevance to the functionality of the field. In my case, this is a formula field that concatenates values from other fields for a search/query purposes - it can't include any security information.

I can deal with it by reporting this as false-positive to SF Security Review Team, but this requires manual work every time I do a release

The bigger issue is that now, I can't use --engine pmd-appexchange in CI/CD since it will always produce an issue

Output / Logs

No response

Steps To Reproduce

1. Add formula field called Key__c to a CustomObject
2. Run sf scanner run --engine pmd-appexchange

Expected Behavior

No violations were found

Operating System

MacOS Sequoia 15.0

Salesforce CLI Version

@salesforce/cli/2.60.13 darwin-arm64 node-v20.13.1

Code Analyzer Plugin (@salesforce/sfdx-scanner) Version

@salesforce/sfdx-scanner 4.2.0 (4.2.0)

Additional Context (Screenshots, Files, etc)

No response

Workaround

I can report this to SF Security Team as false-positive, but I would have to do that every time I am releasing an update

Urgency

High

[stephen-carter-at-sf]

"Key" is a reserved word that I believe the security team is more comfortable flagging every time to alert you to at least check even though it might report false positives. I'll ask a member of our security team to respond to this thread to give more info.

What type of file is this getting flagged on? Is it xml, apex, or something else? I ask because I'm not sure if you can just add a suppressing annotation or comment for the violation that is being flagged. See https://docs.pmd-code.org/latest/pmd_userdocs_suppressing_warnings.html

[stephen-carter-at-sf]

@rrajaram-salesforce I was able to reproduce and it looks as if the ProtectSensitiveData rule isn't even looking at the code... it's just looking at the file names. For example, if I just take any sample salesforce project and rename one of the object fields, like /main/default/objects/SomeObject__c/fields/SomeField__c.field-meta.xml to instead be /main/default/objects/SomeObject__c/fields/Key__c.field-meta.xml then I get the violation

Key__c is a potential auth token in a custom AppExchange Security Review https://github.com/forcedotcom/sfdx-scanner/blob/dev/pmd-appexchange/docs/ProtectSensitiveData.md 

This is one of the pmd appexchange rules... but since it doesn't parse code - I don't see a mechanism that users can suppress the violation since it is purely based on file name. Does this rule have another mechanism it can look for to suppress false positives?

[rrajaram-salesforce]

@kryvyifedir I understand your concern; Where the reported issue is not a security vulnerability, there is no need for the developer to do anything. There is no need to issue a false positive explanation either when submitting for security review; unlike the issues reported in Apex/VF/Aura etc,. where some business logic explanation may be warranted, for a security engineer to better understand the rational behind why a security pattern was not used. When the reviewers - that use the same rules - review the issue like this one, they will notice that the field does not carry an authentication token and will never report a vulnerability. However, unfortunately since there is no way to "track" the reported issue - in xml - as a false positive (Apex allows for annotations so that future PMD scans do not report the issue) the issue will be reported again in a future scan; but developers can ignore the reported issue. I can issue a fix that will not detect the string "key" - but I'm certain there are other strings that fall into a similar category where string matching will result in false positives. Infact, the rule is capable enough to report only those key words that map to a text data type and ignore any other data types (integer, date time etc,.) Hope this helps; I'll explore PMD further to see if there is a way to track previously reported issues as false positives in XML (similar to annotations or comments for Apex)

[kryvyifedir]

@rrajaram-salesforce Thank you for your response. Unfortunately, my concern is somewhat multi-faceted, so let me explain it a bit more.

Let's start from my specific example and my project:

1. "There is no need to issue a false positive explanation either when submitting for security review; " - well, I am not a part of SF Security Review team, so I can't know exactly what and how they check reports. But I can imagine, that the fact that my first report didn't have any issues, and now my new report has 2 new issues would raise concerns with them. Sure, I can perfectly explain why, but I would have to explain it every time I release something or I would have to trust that security team would ignore severity level 3 issues (which is possible). They are human, they also make mistakes, and I would rather my solution has zero violations of any sorts compared to 2 perfectly explainable violation
2. Since I have an open source project, I am using code scanner as a part of my CI/CD. This is important for me as an automated quality control - if CI/CD validations fail, there is no need for me to review a PR (I only review PRs after they pass all automatic validation). With these 2 issues, I would have to a. Ignore them when I review PRs OR write some script that excludes PMD automation from CI/CD (right now any issue found during automation blocks PR from merging), b. explain every contributer who runs PMD locally on their PCs that those 2 issues are expected and don't have to be fixed. So you can see how much extra work and communication it requires on my end.

And now, I want to address the issue that I have specifically with this check (without the context of my project):

1. Let's assume that I am "malicious" or just unexperienced developer, and I want to expose some secret/key/token publicly, because I don't know any other way around. Would this validation stop me? No, I will just change the name of the field to "FieldThatHasInfo__c" and validation will not prevent me from a bad design.
2. The biggest problem that I have with this validation is that is doesn't really check concrete fact or rule, but it assumes that there is a problem judging from the name of the file. For example, "Key" is fairly widely used field name for Unique Indexes in some Databases, and it has nothing to-do with Security Keys or Access Tokens etc.

Also, I appreciate that only text data type fields are being flagged but I think it would be nice also to check that the flagged field is not a formula (as you understand, users and code can't save any security info in this field, so if other fields in the table are not flagged - formula can't expose anything)

[stephen-carter-at-sf]

@kryvyifedir Just so you are aware @rrajaram-salesforce is on the SF Security Review team. He is saying, it doesn't raise concerns with him. I agree that we should work towards having this violation something you can exclude. It is simply at this point going to have to be an enhancement request for us to work on... so we will keep this issue open.

[kryvyifedir]

@stephen-carter-at-sf thank you for your support, I will be looking forward for the future updates for this. I am also planning a new release in November so I will be able to update this thread with the result of security review of my app