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(report): add location.message to SARIF output (#3002) #3003

Merged
merged 2 commits into from
Oct 12, 2022

Conversation

candrews
Copy link
Contributor

@candrews candrews commented Oct 7, 2022

Description

SARIF allows a "message" to be provided for a location and that can have more useful information. See:
https://docs.oasis-open.org/sarif/sarif/v2.1.0/os/sarif-v2.1.0-os.html#_Toc34317675

It would be helpful to set this property so SARIF consumers can display better information. It would be particularly nice if Trivy includes the file and the package name and version (ex, pom.xml:org.yaml:snakeyaml@1.30).

Related issues

Checklist

  • I've read the guidelines for contributing to this repository.
  • I've followed the conventions in the PR title.
  • I've added tests that prove my fix is effective or that my feature works.
  • I've updated the documentation with the relevant information (if needed).
  • I've added usage information (if the PR introduces new options)
  • I've included a "before" and "after" example to the description (if the PR is a user interface change).

@CLAassistant
Copy link

CLAassistant commented Oct 7, 2022

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@candrews candrews force-pushed the sarif-location-message branch 2 times, most recently from ae018b7 to f87f9a1 Compare October 8, 2022 04:25
@knqyf263 knqyf263 requested a review from afdesk October 11, 2022 08:42
@afdesk
Copy link
Contributor

afdesk commented Oct 11, 2022

@candrews thanks for your PR!
but I'd like to update the title.
and i can see that you're updating it now. could you write me when you finish? thanks again

@candrews
Copy link
Contributor Author

I think I'm finished.

I'm having trouble running the tests myself (tinygo is broken in Fedora 37 so I haven't been able to use it), sorry for the noise :(

@candrews candrews force-pushed the sarif-location-message branch 2 times, most recently from 722a4cf to 68128f1 Compare October 11, 2022 18:42
Signed-off-by: Craig Andrews <candrews@integralblue.com>
@candrews
Copy link
Contributor Author

@afdesk can this be merged now?

If not, please let me know what I can do and I'll get right on it - thank you!

@afdesk afdesk changed the title feat(report): set location.message (#3002) feat(report): set location.message to SARIF output (#3002) Oct 12, 2022
@afdesk afdesk changed the title feat(report): set location.message to SARIF output (#3002) feat(report): add location.message to SARIF output (#3002) Oct 12, 2022
@afdesk
Copy link
Contributor

afdesk commented Oct 12, 2022

@candrews thanks!
I've updated a sarif golden file for the integration test.

@candrews
Copy link
Contributor Author

@candrews thanks! I've updated a sarif golden file for the integration test.

Thank you!

@afdesk
Copy link
Contributor

afdesk commented Oct 12, 2022

@candrews could you give me an advice?
it seems this change doesn't affect on the SARIF for github security, right?

@candrews
Copy link
Contributor Author

candrews commented Oct 12, 2022

it seems this change doesn't affect on the SARIF for github security, right?

According to https://docs.github.com/en/code-security/code-scanning/integrating-with-code-scanning/sarif-support-for-code-scanning#location-object

location.message.text | Optional. A message relevant to the location.

So GitHub at least knows about this field. I don't know exactly how it will present it in the UI.

FWIW, I'm working on improving SARIF support in general. I'm not testing / developing against GitHub. My use case involves a different tool.

@afdesk
Copy link
Contributor

afdesk commented Oct 12, 2022

@candrews thanks! it's ok, I understand your point.
it was just interesting.

@knqyf263 knqyf263 merged commit d35c668 into aquasecurity:main Oct 12, 2022
@knqyf263
Copy link
Collaborator

Thanks

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

Successfully merging this pull request may close these issues.

Specify location.message in SARIF reports
4 participants