-
Notifications
You must be signed in to change notification settings - Fork 29
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update impact or defaultSeverity to match each other #4444
base: master
Are you sure you want to change the base?
Conversation
Is it normal that S6967 is categorized as CODE_SMELL/Critical when it has HIGH impact on both Security and Relibability (and MEDIUM on Maintainability)? |
This is how I read the information given by @andrei-epure-sonarsource. The mapping is like so: CCT-based severities: Info (new) / Low / Medium / High / Blocker (new)
I assumed that if there are multiple impacts the most severe one should be the one chosen for severity. |
My question is more: If the most severe impact is on Security and Reliability, does it make sense to categorize it as a code smell? I could see the rule changing from Code Smell/Major to Vulnerability/Critical or Bug/Critical, or the maybe the impacts on Reliability and Security being lowered, if it is indeed a code smell. It just strikes me as odd. I guess on some level it validates the idea of CCT. |
As far as I know, we usually keep the Bug category for symbolic execution rules due to increased precision. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good so far but I cannot find the changes for S6932
rules/S6776/metadata.json
Outdated
@@ -41,7 +41,7 @@ | |||
"quickfix": "unknown", | |||
"code": { | |||
"impacts": { | |||
"SECURITY": "LOW" | |||
"SECURITY": "MEDIUM" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@daniel-teuchert-sonarsource and @hendrik-buchwald-sonarsource can you please confirm this RSpec change? The "defaultSeverity" of the rule is "Major" and "MEDIUM" corresponds with that.
See also this comment for some background info regarding the purpose of the PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would probably change the default severity to minor then and keep the impact at low.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in d5d926a
You probably meant S6776. It was pushed in f8dcac7. I pinged Daniel and Hendrik for confirmation of that change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Quality Gate passed for 'rspec-tools'Issues Measures |
Quality Gate passed for 'rspec-frontend'Issues Measures |
Review
A dedicated reviewer checked the rule description successfully for:
Rules updated
Reviewer: please mark as checked after review.
Discrepancies
Multiple CCT:
Sources
https://trello.com/c/uPdUhaFh
Discrepencies.txt
Multiple CCT