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

Migrate all SARIF readers to extend from SarifReader #70

Merged
merged 10 commits into from
May 4, 2024
Merged

Migrate all SARIF readers to extend from SarifReader #70

merged 10 commits into from
May 4, 2024

Conversation

darkspirit510
Copy link
Contributor

This PR migrates all SARIF reader classes extending them from SarifReader class. I moved basically all the reader's code to the parent class and used constructor instead of abstract methods. That's why this PR deletes lots of code 🙈 (Special thanks to @ericwb who "kind of" made me think that way).

  • Now some of readers are just a constructor call.
  • If required, some methods can be overwritten to adapt special use cases (custom mapping, "fixing" cwe numbers).
  • Most tools uses the possibility to bundle rules (and CWE numbers) in the result file. Contrast does not, so it needs a custom mapping table
  • Although SARIF standard recommends to use taxonomies for CWE numbers, none of the supported tools use this 🤣

PS: I am super sorry for all the contributers of SARIF classes for deleting most of their code, but I think it's way cleaner now.
PPS: @ericwb Does Precaution support Java, yet? I was able to verify changes because I do not have a valid result file for Precaution and running it on my own has an empty result set 🤷‍♂️

@davewichers
Copy link
Contributor

davewichers commented Apr 18, 2024

@darkspirit510 - This changes look really good to me, and much cleaner. I'd like to compare the scores this change generates against the scores generated before this merge before I merge it just to make sure that no scores inadvertently change because of all this refactoring. I hope to do that tonight.

@ericwb
Copy link
Contributor

ericwb commented Apr 24, 2024

@darkspirit510
Looks great and I see the start and end time are now handled. That was an issue I recently observed.

Precaution definitely does some Java analysis. I put up a PR in BenchmarkJava on how to use. Maybe this will help.
OWASP-Benchmark/BenchmarkJava#226

@darkspirit510
Copy link
Contributor Author

@davewichers any news on this PR?

@davewichers davewichers merged commit 6808369 into OWASP-Benchmark:main May 4, 2024
1 check passed
@davewichers
Copy link
Contributor

Sorry, I was on holiday for a week and then lost track of this. I just tested and didn't see any score changes so merged. Thanks for doing this!

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.

3 participants