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 2.1.0 discard suppressed results #156

Closed
KalleOlaviNiemitalo opened this issue Jul 24, 2022 · 7 comments
Closed

SARIF 2.1.0 discard suppressed results #156

KalleOlaviNiemitalo opened this issue Jul 24, 2022 · 7 comments

Comments

@KalleOlaviNiemitalo
Copy link

KalleOlaviNiemitalo commented Jul 24, 2022

A SARIF 2.1.0 log can include some results that are suppressed. For example, the Roslyn C# compiler can write such a log if the source code includes a #pragma directive that disables a warning that it would otherwise deserve. Because Violation does not support the suppression concept, I think SarifParser should discard the suppressed results rather than translate them to Violation instances. Please see [SARIF-v2.1.0] §3.27.23 for information about the result.suppressions property, and #155 (comment) for a sample SARIF log with a suppressed warning.

@ejohn20
Copy link

ejohn20 commented Aug 23, 2022

Agreed. We are currently working on displaying a number of results processed by the sarif-sdk, which supports suppressing results. Jenkins is displaying all of the results, rather than only the not suppressed results.

@ejohn20
Copy link

ejohn20 commented Aug 26, 2022

I spent a bit of time looking at this, and it appears that the result.getSuppressions() data is not being deserialized. This would also require the data in the suppression.getProperties() bag to be deserialized as well, so we can check suppression expiration.

The rules should be as follows (following the details in the SARIF SDK: https://github.com/microsoft/sarif-sdk/blob/025f64caf6603d5cc800ce1b4139641cd286ebee/src/Sarif/Core/Result.cs#L102

  • Filter results with a baselineState == Absent
  • If the status of any of the suppressions is "underReview" or "rejected", then the result should not be considered suppressed. Otherwise, the result should be considered suppressed.
  • If the status of a suppression is Accepted and the suppression.getProperties() bag contains an element called expiryUtc, and the expiryUtc value > Date.UtcNow, the result should be considered suppressed.

@KalleOlaviNiemitalo
Copy link
Author

expiryUtc is not part of the SARIF-v2.1.0 standard though. Is it used in some SARIF logs that you want to parse with this library, or are you just trying to support all known features?

@ejohn20
Copy link

ejohn20 commented Aug 26, 2022

Agreed. While not part of the standard, I'm just trying to set parity between the sarif-sdk processing SARIF results and the violations parser used to display those results in Jenkins.

In this case, the sarif-sdk supports setting the --expiryInDays argument on the suppress command, which puts a timestamp in the expiryUtc property. So, anyone using that option is likely going to expect the viewer to hide those results. https://github.com/microsoft/sarif-sdk/blob/2bdb21252c42f1f5dea71d2c87cc259e18877b2e/src/Sarif.Multitool.Library/SuppressOptions.cs#L34

@tomasbjerre
Copy link
Owner

I want the library to just normalize the reports into a common format. And the tools using the Violation-model can decide on how to act on it. So I added supressed in the specifics-attribute.

expiryUtc can be added the same way, if such a report is supplied.

@ejohn20
Copy link

ejohn20 commented Aug 26, 2022

That makes sense. Sounds like we may need to open a related issue in either the analysis-model or ng-warnings repos to filter suppressed issues using the new specifics field.

@tomasbjerre
Copy link
Owner

I'm closing this issue. Open it again if any problems.

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

No branches or pull requests

3 participants