-
Notifications
You must be signed in to change notification settings - Fork 147
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
Add support for SARIF output for secret scanning #920
Conversation
7541f1d
to
f6dffa5
Compare
This prepare the room to add more formats, like SARIF.
f6dffa5
to
5f7fb41
Compare
This is important to let GitHub import SARIF output files
5f7fb41
to
c9b06df
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great addition :D , I tested and I have a few questions on options flags.
For the multi match secret it does not seem ideal but it is probably the best we can do. Do we know how Github Advance security & others displays those with SARIF format ?
I tested with the validator and pycharm extension it looks good to me !
I have no idea :/
Oh, I could not find a Pycharm extension! Need to look again. |
This way the help output for `--json` can say it's a shorthand for `--format json`, making the behavior between both less ambiguous.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good ! I tested it and it was great 😄
I agree with Garance, output might be a bit weird for multimatch secrets but it's a detail
Context
This PR adds support for generating reports in SARIF format when scanning for secrets.
See #869.
What has been done
The PR first introduces a
--format (text|json)
option, and make existing commands use it.--format json
is a more verbose version of--json
, but it makes it possible to add new formats without adding a new flag for each format.Then it introduces a
sarif
value for--format
. This value is only used bysecret scan
commands for now. Adding it to other verticals will be done separately.Representing multi-match secrets in SARIF output
There is a bit of an impedance mismatch between ggshield secret output and SARIF output. A SARIF result is supposed to have one location, but a secret often contains multiple matches, so each match needs to be a separate location. To reconcile this, the result location starts at the start of the first match and ends at the end of the last match. Then each match is added as a "related" location.
This can be seen in this screenshot of a GitHub import of a ggshield SARIF document:
I am open to suggestions regarding better ways to represent multi-match secrets.
Validation
The code can be tested in different ways.
The first thing is to generate some SARIF report. This can be done with a command like this:
Then you can try to use the generated report in different places:
PR check list
skip-changelog
label has been added to the PR.