Repository navigation
Create new rule detect system() c - #21
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates the CircleCI plugins-test orb version, lowers the confidence level of the standard system call detection rule in docs/codacy-rules.yaml, and shortens the expected error messages in results.xml. However, a critical issue was identified where the shortened error messages in results.xml no longer match the actual rule message defined in docs/codacy-rules.yaml. This mismatch will cause integration tests to fail unless the rule definition is also updated or the changes in results.xml are reverted.
Up to standards ✅🟢 Issues
|
There was a problem hiding this comment.
Pull Request Overview
While the PR is technically up to Codacy standards, there are significant process and implementation concerns. The PR title claims to create a new rule, but the code actually modifies an existing security rule for system() calls. There is also a lack of documentation in the PR description explaining the rationale for these changes.
Crucially, although new detection patterns and exclusions (e.g., osutility::system()) were introduced to the configuration, no test files (C/C++ samples) were included to verify these patterns. This makes it impossible to confirm if the exclusion logic works as intended or if it creates false negatives. Furthermore, there is a technical disagreement between agents on whether lowering the confidence of a deterministic security risk like system() is appropriate; the consensus suggests that refining patterns is preferable to a global confidence downgrade.
About this PR
- No test files (C/C++ samples) are included in the PR to verify that the patterns correctly identify target calls (global, std::, ::) and correctly respect the exclusion for
osutility::system(). - The PR description is empty, providing no context for the confidence downgrade or the orb upgrade. Additionally, the PR title 'Create new rule' contradicts the actual implementation, which modifies an existing rule.
Test suggestions
- Detection of global system() calls
- Detection of std::system() calls in C++
- Detection of ::system() calls in C++
- Exclusion of osutility::system() calls
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Detection of global system() calls
2. Detection of std::system() calls in C++
3. Detection of ::system() calls in C++
4. Exclusion of osutility::system() calls
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
| Ensure safe alternatives or proper input validation/sanitization is used. | ||
| impact: MEDIUM | ||
| confidence: HIGH No newline at end of file | ||
| confidence: LOW No newline at end of file |
There was a problem hiding this comment.
🟡 MEDIUM RISK
There are multiple concerns with this rule update:
- Identity Mismatch: The PR title suggests a new rule, but this modifies
codacy.c.security.avoid-std-system. If a new rule was intended, use a unique ID; otherwise, update the title. - Confidence Level: Lowering confidence to LOW for a deterministic security risk like
system()is counter-intuitive. If the goal is to reduce noise from C++ member functions, it is better to refine the patterns while keeping the confidence HIGH. - Documentation: If the change proceeds, consider updating the rule message to recommend safer alternatives like
execveorposix_spawnto improve utility.
No description provided.