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

feat: Add impact and resolve fields in sarif output. #1558

Merged

Conversation

ipapast
Copy link
Contributor

@ipapast ipapast commented Dec 7, 2020

CC-517

  • Ready for review
  • Follows CONTRIBUTING rules
  • Reviewed by Snyk internal team

What does this PR do?

This PR adds the 'resolve' and 'impact' fields in the results of the sarif output in order to give more useful information to the users and show that to the Github Security page (coming from the registry's AnnotatedIssues object).

Where should the reviewer start?

Check the two new fields in `iac-output.ts. The resolution is added to the 'help' field and the impact is included in the full description.

How should this be manually tested?

  • Run iac test --sarif and see the results

  • Check Github Security

Any background context you want to provide?

We are using the reportingDescriptor object, more info on the docs here Github Security SARIF Docs

What are the relevant tickets?

Jira CC-517

Screenshots

CLI Output:
image

Github Security:
image

@ipapast ipapast force-pushed the feat/add-impact-and-resolution-to-github-security-cc-517 branch from 7905a79 to 0baca2f Compare December 7, 2020 15:53
@ipapast ipapast self-assigned this Dec 7, 2020
@ipapast ipapast force-pushed the feat/add-impact-and-resolution-to-github-security-cc-517 branch from bb1e682 to 030fe25 Compare December 7, 2020 16:10
@ipapast ipapast requested review from aron and almog27 and removed request for almog27 December 7, 2020 16:50
@ipapast ipapast marked this pull request as ready for review December 7, 2020 16:51
@ipapast ipapast requested a review from a team December 7, 2020 16:51
Copy link
Contributor

@aron aron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fantastic, I think now seeing the output in the GitHub UI, the description isn't the best place for the impact.

The help field where we show the resolution looks like a better spot to put all of this information and describe to the user the issue, impact and resolution as we do in the Snyk UI.

What do you think?

src/lib/snyk-test/iac-test-result.ts Show resolved Hide resolved
src/cli/commands/test/iac-output.ts Outdated Show resolved Hide resolved
@ipapast ipapast force-pushed the feat/add-impact-and-resolution-to-github-security-cc-517 branch from f98e042 to 15a2cd5 Compare December 11, 2020 16:49
@ipapast ipapast requested review from almog27 and aron December 11, 2020 16:49
Copy link
Contributor

@aron aron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, looks fantastic. I think it might be easier to keep the nice template literals but to trim the leading whitespace from each line with a replace() function.

Comment on lines 226 to 232
${issue.iacDescription.issue}

The impact of this is...
${issue.iacDescription.impact}

You can resolve this by...
${issue.iacDescription.resolve}`,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're going to have the same issue here as we had with the markdown. The string is going to look like:

The issue is...
        ${issue.iacDescription.issue}

        The impact of this is...
        ${issue.iacDescription.impact}
        
        You can resolve this by...
        ${issue.iacDescription.resolve}

Copy link
Contributor Author

@ipapast ipapast Dec 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct me if I'm wrong, but my understanding is, that if the markdown field exists, then the sarif output will display that. The text field is only mandatory if the markdown field does not exist.
image

In this case, I think we can even have an empty string as a text field and the output would still be the same.
In summary, I think the best thing is to do is to display text: ' ' and then markdown as we have already formmated it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you have it is absolutely fine, we want both as it means that the content can be output anywhere as plaintext but also enhanced as markdown if the client understands it. All I meant by my comment was that the plain text version is going to be indented after the first line. Imagine if this Sarif file was printed by a command line tool as plain text to the terminal, it would look odd if the first line of subsequent sections were indented.

Lets chat if this still doesn't make sense :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the back and forth - I was talking for the specific purposes of Github Security display and you were talking about the case of the plain output. I get you now :) I'll do that change now

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the original PR comment with the most updated screenshots for any future reference.
Added a replace function to the help fields.
Squashed the commits and will merge as soon as the checks passed.

Adds fields in sarif output but also markdown text for the Github Security tab.
CC-517

refactor: Format helpText with a replace function
@ipapast ipapast force-pushed the feat/add-impact-and-resolution-to-github-security-cc-517 branch from 7b21492 to bb2a470 Compare December 14, 2020 13:39
@github-actions
Copy link
Contributor

github-actions bot commented Dec 14, 2020

Expected release notes (by @ipapast)

features:
Add impact and resolve fields in sarif output. (bb2a470)

  • I hereby acknowledge these release notes are 🥙 AWESOME 🥙

@ipapast ipapast merged commit 898ae29 into master Dec 14, 2020
@ipapast ipapast deleted the feat/add-impact-and-resolution-to-github-security-cc-517 branch December 14, 2020 14:50
@snyksec
Copy link

snyksec commented Dec 14, 2020

🎉 This PR is included in version 1.437.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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