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

HdfConverter: Set security-severity property used by GitHub #2705

Merged
merged 1 commit into from
Aug 25, 2023

Conversation

@candrews
Copy link
Collaborator Author

@michaelcfanning could you please take a look at this when you get a chance?

@michaelcfanning
Copy link
Member

We can take this change if you're confident GitHub consumes this data. I thought that security severity was only relevant for the rules metadata. Someone else, actually, had raised the issue with us (and GitHub) that security-severity can't currently be overridden at the result level. Take a look at the content below, see how it requires adding this data to the reportingDescriptorReference?

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

@michaelcfanning
Copy link
Member

          "id": "7",

This is the level where you need to add security-severity, I think. If security-severity is a dynamically computed per-result value that can change within a rule, then we need to work with GitHub to get that functionality going for you (it's already been requested by someone else as well).


Refers to: src/Test.UnitTests.Sarif.Converters/TestData/HdfConverter/ExpectedOutputs/ValidResults.sarif:4949 in e1ca167. [](commit_id = e1ca167, deletion_comment = False)

@candrews
Copy link
Collaborator Author

I thought that security severity was only relevant for the rules metadata.

You're right...

This is the level where you need to add security-severity, I think.

Agreed.

I've updated this PR accordingly.

Copy link
Member

@michaelcfanning michaelcfanning left a comment

Choose a reason for hiding this comment

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

:shipit:

@michaelcfanning michaelcfanning merged commit e7cf46f into main Aug 25, 2023
7 checks passed
@michaelcfanning michaelcfanning deleted the security-severity branch August 25, 2023 20:04
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.

2 participants