Skip to content
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

New rule: Validate presence and consistency of result.ruleId/result.rule.id #1880

Closed
ghost opened this issue May 21, 2020 · 2 comments
Closed

Comments

@ghost
Copy link

ghost commented May 21, 2020

At least one of result.ruleId or result.rule.id must be present, and if both are present, they must be equal.

@eddynaka
Copy link
Collaborator

Hi @lgolding ,

I have some questions about this rule:

  1. Naming: for the other rules that we have in Sarif.Multitool.Rules, every rule starts with a code (for example: SARIF1001.DoNotUseFriendlyNameAsRuleId). Are we going to maintain that pattern? If yes, what code should I use?
  2. Resources: looking at the file RuleResources.resx, we have at least two rows for each rule: Default and Problem. Based on the other rules, the _default is used as full description and, when we find an issue, we assign a new value, right? For this case, I was thinking in creating 3 rows in the resources file: _default, _ruleIdMustbepresent, _ruleIdMustBeEqual.

Thank you for the help

@ghost
Copy link
Author

ghost commented May 30, 2020

  1. Yes, we will maintain that pattern. Choose the next available number in src\Sarif.Multitool\Rules\RuleId.cs, namely SARIF1019.

  2. To understand the resources names, you need to know that the definition of a SARIF rule includes two kinds of user-facing strings: its description and its result messages. A SARIF rule can actually have two descriptions, a "short description" and a "full description", but the validation rules don't bother with the short description. You can recognize the "full description" resource string because it has the same name as the class. For example, in SARIF1014.UriBaseIdRequiresRelativeUri, you see that the rule's full description comes from the resource string SARIF1014_UriBaseIdRequiresRelativeUri.

    Now for the result messages. Most rules produce the same result message for all results, and for those, the resource name is (for example) SARIF1014_Default. But occasionally it's helpful to produce different messages in different circumstances. According to the resources file, we currently do this for SARIF1009 and SARIF1018. You can recognize these variant messages in two ways:

  • They aren't named "Default", and they don't have the same name as the class.
  • They include formatting replacement sequences like {0}.

In my opinion our new rule SARIF1019 should have two messages:

  • When neither result.ruleId nor result.rule.id is present, the message should be SARIF1019_MissingResultRuleId: "{0}: The result contains neither result.ruleId nor result.rule.id."
  • When both result.ruleId and result.rule.id are present, but they are not equal, the message should be SARIF1019_InconsistentResultRuleId: "{0}: The result contains both the ruleId property '{1}' and the rule.id property '{2}', and they are not equal."

NOTE: The {0} in all the messages is a placeholder for the JSON path to the property containing the error.

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant