Improve keyword key for secret detection#17
Conversation
Up to standards ✅🟢 Issues
|
![codacy-production[bot]](https://avatars.githubusercontent.com/in/56611?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull Request Overview
The PR successfully introduces more granular secret detection by adding prefixes to the 'key' keyword. However, it introduces a significant logic regression in the SQL lookup-type-without-language-or-apps-fnd rule. The refactoring from pattern-either to a sequential list of patterns changes the detection logic from 'OR' to 'AND', which will cause the rule to miss many valid violations.
Additionally, there is an inconsistency in how the regex was updated. While the Java rule correctly deduplicated the keyword clave, the same duplication remains in the C#, JavaScript, and PL/SQL rules. Finally, the PR lacks a description and includes out-of-scope changes to the SQL rule that contradict the PR title.
About this PR
- The modifications to the 'codacy.generic.sql.lookup-type-without-language-or-apps-fnd' rule appear out of scope based on the PR title and represent a significant change in behavior.
- The PR description is missing. Please provide context regarding why these specific changes were made and why the SQL rule logic was modified.
Test suggestions
- Verify Java rule detects 'String aws_key = "..."'
- Verify C# rule detects 'var api-key = "..."'
- Verify JavaScript rule detects 'const private_key = "..."'
- Verify SQL rule flags files with 'lookup_type' AND 'apps.fnd_lookup_values' when 'language' is missing
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Verify Java rule detects 'String aws_key = "..."'
2. Verify C# rule detects 'var api-key = "..."'
3. Verify JavaScript rule detects 'const private_key = "..."'
4. Verify SQL rule flags files with 'lookup_type' AND 'apps.fnd_lookup_values' when 'language' is missing
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
| - pattern-regex: "(?im)^(?:(?!--|/\\*)[^\\n])*\\bapps\\.fnd_lookup_values\\b" | ||
| - pattern-regex: "(?im)^(?:(?!--|/\\*)[^\\n])*\\blookup_type\\b" | ||
| - pattern-not-regex: "(?im)^(?:(?!--|/\\*)[^\\n])*\\blanguage\\b" |
There was a problem hiding this comment.
🔴 HIGH RISK
The removal of pattern-either converts the logic from OR to AND. This rule will now only trigger if both apps.fnd_lookup_values and lookup_type appear on the same line, which misses cases where only one is present. Additionally, the new patterns lose the multi-line detection capability of the previous regex (which used (?:[^;\n]*\n)*). Restore the pattern-either block to ensure valid SQL violations are still detected.
| - metavariable-regex: | ||
| metavariable: "$PASSWORD" | ||
| regex: "(?i).*(password|motdepasse|heslo|adgangskode|wachtwoord|salasana|passwort|passord|senha|geslo|clave|losenord|clave|parola|secret|pwd|key).*" | ||
| regex: "(?i).*(password|motdepasse|heslo|adgangskode|wachtwoord|salasana|passwort|passord|senha|geslo|clave|losenord|clave|parola|secret|pwd|(api|secret|private|access|aws|ssh|auth|session|encryption|decryption|gcp)[_-]?key).*" |
There was a problem hiding this comment.
⚪ LOW RISK
Suggestion: Duplicate keyword clave detected in the regex. Please remove the redundant entry to match the clean version used in the Java rule.
| regex: "(?i).*(password|motdepasse|heslo|adgangskode|wachtwoord|salasana|passwort|passord|senha|geslo|clave|losenord|clave|parola|secret|pwd|(api|secret|private|access|aws|ssh|auth|session|encryption|decryption|gcp)[_-]?key).*" | |
| regex: "(?i).*(password|motdepasse|heslo|adgangskode|wachtwoord|salasana|passwort|passord|senha|geslo|clave|losenord|parola|secret|pwd|(api|secret|private|access|aws|ssh|auth|session|encryption|decryption|gcp)[_-]?key).*" |
| - metavariable-regex: | ||
| metavariable: "$PASSWORD" | ||
| regex: "(?i).*(password|motdepasse|heslo|adgangskode|wachtwoord|salasana|passwort|passord|senha|geslo|clave|losenord|clave|parola|secret|pwd|key).*" | ||
| regex: "(?i).*(password|motdepasse|heslo|adgangskode|wachtwoord|salasana|passwort|passord|senha|geslo|clave|losenord|clave|parola|secret|pwd|(api|secret|private|access|aws|ssh|auth|session|encryption|decryption|gcp)[_-]?key).*" |
There was a problem hiding this comment.
⚪ LOW RISK
Suggestion: The keyword clave is duplicated in this regex string. For consistency and efficiency, remove the redundant second occurrence.
| regex: "(?i).*(password|motdepasse|heslo|adgangskode|wachtwoord|salasana|passwort|passord|senha|geslo|clave|losenord|clave|parola|secret|pwd|(api|secret|private|access|aws|ssh|auth|session|encryption|decryption|gcp)[_-]?key).*" | |
| regex: "(?i).*(password|motdepasse|heslo|adgangskode|wachtwoord|salasana|passwort|passord|senha|geslo|clave|losenord|parola|secret|pwd|(api|secret|private|access|aws|ssh|auth|session|encryption|decryption|gcp)[_-]?key).*" |
| - metavariable-regex: | ||
| metavariable: "$PASSWORD" | ||
| regex: "(?i).*(password|motdepasse|heslo|adgangskode|wachtwoord|salasana|passwort|passord|senha|geslo|clave|losenord|clave|parola|secret|pwd|key).*" | ||
| regex: "(?i).*(password|motdepasse|heslo|adgangskode|wachtwoord|salasana|passwort|passord|senha|geslo|clave|losenord|clave|parola|secret|pwd|(api|secret|private|access|aws|ssh|auth|session|encryption|decryption|gcp)[_-]?key).*" |
There was a problem hiding this comment.
⚪ LOW RISK
Suggestion: The keyword clave is duplicated in the regex string. Removing the second instance will make the regex cleaner and maintain consistency with the Java rule update on line 28.
| regex: "(?i).*(password|motdepasse|heslo|adgangskode|wachtwoord|salasana|passwort|passord|senha|geslo|clave|losenord|clave|parola|secret|pwd|(api|secret|private|access|aws|ssh|auth|session|encryption|decryption|gcp)[_-]?key).*" | |
| regex: "(?i).*(password|motdepasse|heslo|adgangskode|wachtwoord|salasana|passwort|passord|senha|geslo|clave|losenord|parola|secret|pwd|(api|secret|private|access|aws|ssh|auth|session|encryption|decryption|gcp)[_-]?key).*" |
1 participant
No description provided.