Backport latest codacy-semgrep changes and bump opengrep to 1.15.1 #1
Conversation
Codacy's Analysis Summary12 new issues (≤ 0 minor issue) Review Pull Request in Codacy →
|
![codacy-production[bot]](https://avatars.githubusercontent.com/in/56611?s=60&v=4){kind=small}
|
|
||
| public void processOrder(String customer, int count) { | ||
| // BAD: Concatenated string | ||
| System.out.println("Processing order for " + customer + " with " + count + " items."); // ❌ |
There was a problem hiding this comment.
Codacy found an issue: Line is longer than 80 characters (found 99).
| import java.util.ResourceBundle; | ||
|
|
||
| public class OrderApp { | ||
| private static final ResourceBundle messages = ResourceBundle.getBundle("Messages", Locale.ENGLISH); |
There was a problem hiding this comment.
Codacy found an issue: Line is longer than 80 characters (found 104).
| System.out.println(MessageFormat.format(msg, customer, amount)); | ||
| } else { | ||
| // BAD: Inline error | ||
| System.out.println("Payment failed for " + customer + "! Amount: " + amount); // ❌ |
There was a problem hiding this comment.
Codacy found an issue: Line is longer than 80 characters (found 94).
| this.messages = messages; | ||
| } | ||
|
|
||
| public void processPayment(String customer, double amount, boolean success) { |
There was a problem hiding this comment.
Codacy found an issue: Line is longer than 80 characters (found 81).
| @@ -0,0 +1,25 @@ | |||
| import java.util.ResourceBundle; | |||
|
|
|||
| public class UILayer { | |||
There was a problem hiding this comment.
Codacy found an issue: All classes, interfaces, enums and annotations must belong to a named package
| import java.text.MessageFormat; | ||
| import java.util.ResourceBundle; | ||
|
|
||
| public class PaymentService { |
There was a problem hiding this comment.
Codacy found an issue: All classes, interfaces, enums and annotations must belong to a named package
| import java.text.MessageFormat; | ||
| import java.util.ResourceBundle; | ||
|
|
||
| public class OrderService { |
There was a problem hiding this comment.
Codacy found an issue: All classes, interfaces, enums and annotations must belong to a named package
| import java.util.Locale; | ||
| import java.util.ResourceBundle; | ||
|
|
||
| public class OrderApp { |
There was a problem hiding this comment.
Codacy found an issue: All classes, interfaces, enums and annotations must belong to a named package
| var parameters []codacy.PatternParameter | ||
| seenParams := make(map[string]bool) | ||
|
|
||
| var searchForRegex func(obj interface{}) |
There was a problem hiding this comment.
Codacy found an issue: Method has a cyclomatic complexity of 9 (limit is 7)
| const [orders, setOrders] = useState([]); | ||
|
|
||
| useEffect(() => { | ||
| fetch("http://localhost:8080/api/orders") |
There was a problem hiding this comment.
Codacy found an issue: Unencrypted request over HTTP detected.
| using Google.GenAI; | ||
| using Google.GenAI.Types; | ||
|
|
||
| public class GenerateContentSimpleText { |
There was a problem hiding this comment.
Codacy found an issue: Add a 'protected' constructor or the 'static' keyword to the class declaration.
| var response = await client.Models.GenerateContentAsync( | ||
| model: "deepseek-v3.2", contents: "Explain how AI works in a few words" | ||
| ); | ||
| var response2 = await client.Models.GenerateContentAsync( |
There was a problem hiding this comment.
Codacy found an issue: Remove the unused local variable 'response2'.
There was a problem hiding this comment.
Pull request overview
This pull request backports the latest changes from codacy-semgrep and bumps the opengrep version from 1.11.5 to 1.15.1. The main enhancement introduces support for parameterized rules using HTML comment placeholders (e.g., <!-- MODEL_ALLOW_LIST -->), enabling dynamic configuration of Semgrep rules. Additionally, it adds new i18n and AI security rules with comprehensive test cases.
Changes:
- Upgraded opengrep from v1.11.5 to v1.15.1 in Dockerfile and .tool_version
- Implemented parameter placeholder replacement mechanism for dynamic rule configuration
- Added i18n rules to detect hardcoded strings in Java, JavaScript, and TypeScript
- Added AI security rules to detect usage of unauthorized LLM models in C#
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| Dockerfile | Updated opengrep version from v1.11.5 to v1.15.1 |
| .tool_version | Updated version string to match Dockerfile |
| internal/tool/configuration.go | Added parameter placeholder replacement logic with HTML comment parsing, regex conversion for allow lists, and camelCase name formatting |
| internal/docgen/parsing.go | Added parameter extraction from YAML rules, support for multiple custom rule files, and parameter propagation to pattern definitions |
| internal/docgen/pattern-with-explanation.go | Added Parameters field to pattern structures |
| docs/codacy-rules-i18n.yaml | New file defining i18n linting rules for Java and JavaScript/TypeScript |
| docs/codacy-rules-ai.yaml | New file defining AI security rules with parameterized model allow lists |
| docs/multiple-tests/i18n/* | Test files demonstrating i18n rule detection across multiple languages |
| docs/multiple-tests/ai/* | Test files demonstrating AI security rule with allow list parameters |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // formatParameterName converts UPPER_CASE to camelCase | ||
| func formatParameterName(name string) string { | ||
| parts := strings.Split(strings.ToLower(name), "_") | ||
| if len(parts) == 0 { | ||
| return name | ||
| } | ||
|
|
||
| result := parts[0] | ||
| for i := 1; i < len(parts); i++ { | ||
| if len(parts[i]) > 0 { | ||
| result += strings.ToUpper(string(parts[i][0])) + parts[i][1:] | ||
| } | ||
| } | ||
| return result | ||
| } |
There was a problem hiding this comment.
The function formatParameterName is duplicated in both internal/tool/configuration.go and internal/docgen/parsing.go with identical implementations. This code duplication violates the DRY (Don't Repeat Yourself) principle and can lead to maintenance issues. Consider extracting this function to a shared utility package that can be imported by both modules.
| var htmlCommentRegex = regexp.MustCompile(`<!--\s*([A-Z_]+)\s*-->`) | ||
|
|
There was a problem hiding this comment.
The htmlCommentRegex variable is duplicated in both internal/tool/configuration.go and internal/docgen/parsing.go with identical regex patterns. This duplication can lead to maintenance issues if the pattern needs to be updated. Consider extracting this regex to a shared utility package that can be imported by both modules.
| var htmlCommentRegex = regexp.MustCompile(`<!--\s*([A-Z_]+)\s*-->`) | |
| const htmlCommentPattern = `<!--\s*([A-Z_]+)\s*-->` | |
| var htmlCommentRegex = regexp.MustCompile(htmlCommentPattern) |
| func defaultRuleIsConfiguredWithPattern(line string, patterns *[]codacy.Pattern, idIsPresent bool, currentPattern *codacy.Pattern) (bool, *codacy.Pattern) { | ||
| if strings.Contains(line, "- id:") { | ||
| id := strings.TrimSpace(strings.Split(line, ":")[1]) | ||
| pattern, found := lo.Find(*patterns, func(item codacy.Pattern) bool { | ||
| return item.ID == id | ||
| }) | ||
| if found { | ||
| return true, &pattern | ||
| } | ||
| return false, nil | ||
| } | ||
| return idIsPresent, currentPattern | ||
| } | ||
|
|
||
| func isIDPresent(id string, patterns *[]codacy.Pattern) bool { | ||
| _, res := lo.Find(*patterns, func(item codacy.Pattern) bool { | ||
| return item.ID == id | ||
| }) | ||
| return res | ||
| } | ||
|
|
||
| // replaceParameterPlaceholders replaces HTML comment placeholders (e.g., <!-- MODEL_REGEX -->) | ||
| // with the corresponding parameter values from the pattern | ||
| func replaceParameterPlaceholders(line string, pattern *codacy.Pattern) string { | ||
| if pattern == nil || len(pattern.Parameters) == 0 { | ||
| return line | ||
| } | ||
|
|
||
| // Check if line contains an HTML comment placeholder | ||
| if !htmlCommentRegex.MatchString(line) { | ||
| return line | ||
| } | ||
|
|
||
| // Replace each HTML comment with the corresponding parameter value | ||
| result := htmlCommentRegex.ReplaceAllStringFunc(line, func(match string) string { | ||
| matches := htmlCommentRegex.FindStringSubmatch(match) | ||
| if len(matches) > 1 { | ||
| paramName := matches[1] | ||
| // Convert UPPER_CASE to camelCase to match parameter name format | ||
| formattedParamName := formatParameterName(paramName) | ||
| // Find the parameter in the pattern | ||
| for _, param := range pattern.Parameters { | ||
|
|
||
| if param.Name == formattedParamName { | ||
| // Use Value if set, otherwise use Default | ||
| value := param.Value | ||
| if value == nil { | ||
| value = param.Default | ||
| } | ||
| if value != nil { | ||
| valueStr := fmt.Sprintf("%v", value) | ||
|
|
||
| // If parameter name ends with _ALLOW_LIST, convert comma-separated list to regex pattern | ||
| if strings.HasSuffix(paramName, "_ALLOW_LIST") { | ||
| return convertListToRegex(valueStr, false) | ||
| } | ||
| return valueStr | ||
| } | ||
| } | ||
| } | ||
| } | ||
| // If no parameter found, keep the original placeholder | ||
| return match | ||
| }) | ||
|
|
||
| return result | ||
| } | ||
|
|
||
| // convertListToRegex converts a comma-separated list into a regex alternation pattern | ||
| // Example: "gemini-2.5-flash,gpt-3.5-turbo,old-llama-model" -> "^(gemini-2\\.5-flash|gpt-3\\.5-turbo|old-llama-model)$" | ||
| func convertListToRegex(list string, include bool) string { | ||
| // Split by comma and trim spaces | ||
| items := strings.Split(list, ",") | ||
| for i, item := range items { | ||
| // Trim whitespace | ||
| item = strings.TrimSpace(item) | ||
| // Escape dots for regex | ||
| item = strings.ReplaceAll(item, ".", "\\.") | ||
| items[i] = item | ||
| } | ||
|
|
||
| // Join with pipe separator and wrap in regex anchors | ||
| if include { | ||
| return fmt.Sprintf("^(%s)$", strings.Join(items, "|")) | ||
| } | ||
|
|
||
| return fmt.Sprintf("^(?!(%s)$).*", strings.Join(items, "|")) | ||
| } | ||
|
|
||
| // formatParameterName converts UPPER_CASE to camelCase | ||
| func formatParameterName(name string) string { | ||
| parts := strings.Split(strings.ToLower(name), "_") | ||
| if len(parts) == 0 { | ||
| return name | ||
| } | ||
|
|
||
| result := parts[0] | ||
| for i := 1; i < len(parts); i++ { | ||
| if len(parts[i]) > 0 { | ||
| result += strings.ToUpper(string(parts[i][0])) + parts[i][1:] | ||
| } | ||
| } | ||
| return result | ||
| } |
There was a problem hiding this comment.
The new functions replaceParameterPlaceholders, convertListToRegex, formatParameterName, and defaultRuleIsConfiguredWithPattern lack test coverage. The existing test file configuration_test.go contains tests for other functions in this file, but these new functions are not tested. Consider adding unit tests to ensure the parameter replacement logic, regex conversion, and name formatting work correctly, especially given the complexity of the regex escaping and negation logic.
| // extractParametersFromRule recursively searches for regex fields with HTML comment placeholders | ||
| // and creates PatternParameters for each one found | ||
| func extractParametersFromRule(ruleMap map[string]interface{}) []codacy.PatternParameter { | ||
| var parameters []codacy.PatternParameter | ||
| seenParams := make(map[string]bool) | ||
|
|
||
| var searchForRegex func(obj interface{}) | ||
| searchForRegex = func(obj interface{}) { | ||
| switch v := obj.(type) { | ||
| case map[string]interface{}: | ||
| for key, value := range v { | ||
| if key == "regex" { | ||
| if regexStr, ok := value.(string); ok { | ||
| if matches := htmlCommentRegex.FindStringSubmatch(regexStr); len(matches) > 1 { | ||
| paramName := matches[1] | ||
| if !seenParams[paramName] { | ||
| seenParams[paramName] = true | ||
| // Convert to proper case (e.g., MODEL_REGEX -> modelRegex) | ||
| formattedName := formatParameterName(paramName) | ||
| parameters = append(parameters, codacy.PatternParameter{ | ||
| Name: formattedName, | ||
| Description: fmt.Sprintf("Regular expression pattern for %s", strings.ToLower(strings.ReplaceAll(paramName, "_", " "))), | ||
| Default: ".*", | ||
| }) | ||
| } | ||
| } | ||
| } | ||
| } else { | ||
| searchForRegex(value) | ||
| } | ||
| } | ||
| case []interface{}: | ||
| for _, item := range v { | ||
| searchForRegex(item) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| searchForRegex(ruleMap) | ||
| return parameters | ||
| } | ||
|
|
||
| // formatParameterName converts UPPER_CASE to camelCase | ||
| func formatParameterName(name string) string { | ||
| parts := strings.Split(strings.ToLower(name), "_") | ||
| if len(parts) == 0 { | ||
| return name | ||
| } | ||
|
|
||
| result := parts[0] | ||
| for i := 1; i < len(parts); i++ { | ||
| if len(parts[i]) > 0 { | ||
| result += strings.ToUpper(string(parts[i][0])) + parts[i][1:] | ||
| } | ||
| } | ||
| return result | ||
| } |
There was a problem hiding this comment.
The function extractParametersFromRule and the updated formatParameterName function lack test coverage. The existing test file parsing_test.go contains tests for other functions in this file, but these new functions are not tested. Consider adding unit tests to ensure the parameter extraction logic correctly identifies HTML comment placeholders in various nested structures and that the name formatting works as expected.
| "codacy-rules-ai.yaml", | ||
| } | ||
| for _, file := range customRulesFiles { | ||
| filePath, _ := filepath.Abs(path.Join(docsDir, file)) |
There was a problem hiding this comment.
The error returned from filepath.Abs() is silently ignored using the blank identifier. If filepath.Abs() fails, it returns an empty string and an error, which means filePath would be empty and the subsequent call to getRules() would likely fail with a less clear error message. Consider handling this error explicitly to provide better error diagnostics.
| filePath, _ := filepath.Abs(path.Join(docsDir, file)) | |
| filePath, err := filepath.Abs(path.Join(docsDir, file)) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to get absolute path for %q: %w", path.Join(docsDir, file), err) | |
| } |
| func convertListToRegex(list string, include bool) string { | ||
| // Split by comma and trim spaces | ||
| items := strings.Split(list, ",") | ||
| for i, item := range items { | ||
| // Trim whitespace | ||
| item = strings.TrimSpace(item) | ||
| // Escape dots for regex | ||
| item = strings.ReplaceAll(item, ".", "\\.") | ||
| items[i] = item | ||
| } | ||
|
|
||
| // Join with pipe separator and wrap in regex anchors | ||
| if include { | ||
| return fmt.Sprintf("^(%s)$", strings.Join(items, "|")) | ||
| } | ||
|
|
||
| return fmt.Sprintf("^(?!(%s)$).*", strings.Join(items, "|")) | ||
| } |
There was a problem hiding this comment.
The convertListToRegex function has a parameter named include but its usage is counterintuitive. When include=false, it generates a negative lookahead pattern (exclusion/block list), and when include=true, it generates a positive match pattern (inclusion/allow list). However, the caller uses convertListToRegex(valueStr, false) for parameters ending with _ALLOW_LIST, which semantically suggests an allow list but produces exclusion logic. While this may be correct for the use case (flagging items NOT in the allow list), the parameter name include doesn't clearly convey this intent. Consider renaming to matchItems or positiveMatch to better reflect that true means "match these items" and false means "match everything except these items".
| func convertListToRegex(list string, include bool) string { | ||
| // Split by comma and trim spaces | ||
| items := strings.Split(list, ",") | ||
| for i, item := range items { | ||
| // Trim whitespace | ||
| item = strings.TrimSpace(item) | ||
| // Escape dots for regex | ||
| item = strings.ReplaceAll(item, ".", "\\.") | ||
| items[i] = item | ||
| } | ||
|
|
||
| // Join with pipe separator and wrap in regex anchors | ||
| if include { | ||
| return fmt.Sprintf("^(%s)$", strings.Join(items, "|")) | ||
| } | ||
|
|
||
| return fmt.Sprintf("^(?!(%s)$).*", strings.Join(items, "|")) | ||
| } |
There was a problem hiding this comment.
The convertListToRegex function does not escape other regex metacharacters (such as *, +, ?, [, ], {, }, (, ), |, ^, $, \) that may appear in the list items. Currently, only dots are escaped. If model names or other values contain regex metacharacters, they could be interpreted as regex patterns rather than literal strings, leading to incorrect pattern matching. Consider using a proper regex escaping function to escape all special characters.
| // convertListToRegex converts a comma-separated list into a regex alternation pattern | ||
| // Example: "gemini-2.5-flash,gpt-3.5-turbo,old-llama-model" -> "^(gemini-2\\.5-flash|gpt-3\\.5-turbo|old-llama-model)$" |
There was a problem hiding this comment.
The documentation comment for convertListToRegex is misleading. The example shows the output as ^(gemini-2\\.5-flash|gpt-3\\.5-turbo|old-llama-model)$, but this is only the output when include=true. When include=false (which is how it's actually used for _ALLOW_LIST), the output would be ^(?!(gemini-2\\.5-flash|gpt-3\\.5-turbo|old-llama-model)$).*. The documentation should clarify both cases or explain what the include parameter controls.
| // convertListToRegex converts a comma-separated list into a regex alternation pattern | |
| // Example: "gemini-2.5-flash,gpt-3.5-turbo,old-llama-model" -> "^(gemini-2\\.5-flash|gpt-3\\.5-turbo|old-llama-model)$" | |
| // convertListToRegex converts a comma-separated list into a regex alternation pattern. | |
| // If include is true, it returns a pattern that matches only the listed items. | |
| // If include is false, it returns a pattern that matches anything except the listed items. | |
| // Example input: "gemini-2.5-flash,gpt-3.5-turbo,old-llama-model" | |
| // include == true -> "^(gemini-2\\.5-flash|gpt-3\\.5-turbo|old-llama-model)$" | |
| // include == false -> "^(?!(gemini-2\\.5-flash|gpt-3\\.5-turbo|old-llama-model)$).*" |
| var response2 = await client.Models.GenerateContentAsync( | ||
| model: "gemini-2.5-flash", contents: "Explain how AI works in a few words" | ||
| ); | ||
| Console.WriteLine(response.Candidates[0].Content.Parts[0].Text); |
There was a problem hiding this comment.
This assignment to response2 is useless, since its value is never read.
| Console.WriteLine(response.Candidates[0].Content.Parts[0].Text); | |
| Console.WriteLine(response.Candidates[0].Content.Parts[0].Text); | |
| Console.WriteLine(response2.Candidates[0].Content.Parts[0].Text); |
|
|
||
| export default function OrderList() { | ||
| const { t, i18n } = useTranslation(); | ||
| const [orders, setOrders] = useState([ |
There was a problem hiding this comment.
Unused variable setOrders.
| const [orders, setOrders] = useState([ | |
| const [orders] = useState([ |
No description provided.