-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Mayhem SARIF support (new parser) #12624
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
base: dev
Are you sure you want to change the base?
Conversation
No security concerns detected in this pull request. All finding details can be found in the DryRun Security Dashboard. |
I believe the issue raised by the security bot are by design - the SARIF included various examples of exploits, and credentials are known demo credentials for public demo endpoints. |
c629bcf
to
b72f2a9
Compare
Thank you for raising the PR. I just wanted to double check if there's really no way around copying and pasting the existing parser. It's not a lot of code, but I can imagine if SARIF gets more populat we might get more parsers that need slightly different handling of things. Have you considered instead to inherit/extend the existing parser class and overwrite some methods? When I do a "lazy" compare of the SARIF parser with the Mayhem parsers the difference seem very limited, mainly some markdown handling and title cleanup + cwe handling? |
Good catch - I went ahead and extended the SarifParser class instead of copying code. There were a couple of issues with how SarifParser uses member functions vs package functions, so I ended up having to copy code anyways (otherwise polymorphism wasn't working and reverting back to the original SarifParser functions), but I tried to keep this to a minimum. Let me know if there's anything else. |
Looks like we need to make some changes to the Sarif parser to make it more extensible. That might be too much to try and achieve in this PR. If you could ensure the Mayhem unit tests are testing all the Mayhem specific fields/data, that will help in the future when we make the Sarif parser more extensible. |
@xansec What do you think about the unit tests? I believe the cwe handling is different from the default SARIF parser, so it's good to have that present in the unit test assertions. What else is different/extended in the Mayhem parser and would be good to assert on in the unit tests? |
Description
This supports parsing Mayhem-generated SARIF reports. In general, the existing SARIF support should work, however, there are some idiosyncrasies as Mayhem is a DAST tool, where the output fields are used in a somewhat non-idiomatic way. Rather than creating a patch change for the existing SARIF report support, I propose this new Mayhem specific support, so that we can keep up with the changes to the SARIF output without burdening or imposing on the existing SARIF parser.
Test results
Tests have been included in
dojo/unittests
Documentation
Documentation has been added under
dojo/docs/content/en/connecting_your_tools/parsers/file/mayhem.md
Checklist
This checklist is for your information.
dev
.dev
.