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

SARIF : format's specification conformity #603

Open
2 of 6 tasks
mmorel-35 opened this issue May 3, 2021 · 6 comments
Open
2 of 6 tasks

SARIF : format's specification conformity #603

mmorel-35 opened this issue May 3, 2021 · 6 comments

Comments

@mmorel-35
Copy link
Contributor

mmorel-35 commented May 3, 2021

Github is using fingerprints in SARIF format. It helps avoiding duplicates.

Cf. https://docs.github.com/en/code-security/secure-coding/sarif-support-for-code-scanning#preventing-duplicate-alerts-using-fingerprints

The fingerprint shall be translated from TypeScript to Golang to be used to fulfill the partialFingerprints field in the SARIF format.

With the use of https://sarifweb.azurewebsites.net/Validation we can see the following remarks :

  • SARIF2003: Provide 'versionControlProvenance' to record which version of the code was analyzed, and to enable paths to be expressed relative to the root of the repository.

  • SARIF2011: Provide context regions to enable users to see a portion of the code that surrounds each result, even if they are not enlisted in the code.

  • GH1001: Each result location must provide the property 'physicalLocation.artifactLocation.uri'. GitHub Advanced Security code scanning will not display a result whose location does not provide the URI of the artifact that contains the result.

  • SARIF2010 : Provide code snippets to enable users to see the code that triggered each result, even if they are not enlisted in the code.

  • SARIF2002: In result messages, use the 'message.id' and 'message.arguments' properties rather than 'message.text'. This has several advantages. If 'text' is lengthy, using 'id' and 'arguments' makes the SARIF file smaller. If the rule metadata is stored externally to the SARIF log file, the message text can be improved (for example, by adding more text, clarifying the phrasing, or fixing typos), and the result messages will pick up the improvements the next time it is displayed. Finally, SARIF supports localizing messages into different languages, which is possible if the SARIF file contains 'message.id' and 'message.arguments', but not if it contains 'message.text' directly.

  • SARIF2012: Rule metadata should provide information that makes it easy to understand and fix the problem. Provide the 'name' property, which contains a "friendly name" that helps users see at a glance the purpose of the rule. For uniformity of experience across all tools that produce SARIF, the friendly name should be a single Pascal-case identifier, for example, 'ProvideRuleFriendlyName'. Provide the 'helpUri' property, which contains a URI where users can find detailed information about the rule. This information should include a detailed description of the invalid pattern, an explanation of why the pattern is poor practice (particularly in contexts such as security or accessibility where driving considerations might not be readily apparent), guidance for resolving the problem (including describing circumstances in which ignoring the problem altogether might be appropriate), examples of invalid and valid patterns, and special considerations (such as noting when a violation should never be ignored or suppressed, noting when a violation could cause downstream tool noise, and noting when a rule can be configured in some way to refine or alter the analysis).

@mmorel-35 mmorel-35 changed the title SARIF : partial Fingerprints is missing SARIF : format's specification conformity May 5, 2021
@ccojocar
Copy link
Member

@mmorel-35 Can this issue be closed? Thanks

@mmorel-35
Copy link
Contributor Author

GH1001 can be removed but the others are still active, if you try the tool with one of your resultset you'll get the the same recommendation.
Now, you can always say, I don't want to implement this rule or the other, it's up to you.

@Jeeppler
Copy link

@ccojocar This is more like an epic (scrum), rather than an issue. It might make more sense to have individual issues referencing this issue.

@Jeeppler
Copy link

SARIF2011 refers to the context region. This would be nice to have.

@ccojocar
Copy link
Member

This is more like an epic (scrum), rather than an issue. It might make more sense to have individual issues referencing this issue.

Agree. Please just create issues which can be referenced in this issue just to keep track.

@Jeeppler
Copy link

It might help to add the label "epic" to this issue. At least, this is what SecHub does.

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

3 participants