Add Support for GitLab SAST #86
Conversation
|
@darkspirit510 - Can you review this and let me know if you are OK with me to merge this? Or provide feedback on the PR? |
plugin/src/main/java/org/owasp/benchmarkutils/score/parsers/GitLabSastReader.java
Show resolved
Hide resolved
plugin/src/test/java/org/owasp/benchmarkutils/score/parsers/GitLabSastReaderTest.java
Show resolved
Hide resolved
| assertEquals(CweNumber.PATH_TRAVERSAL, result.get(5).get(0).getCWE()); | ||
| } | ||
|
|
||
| @Test |
There was a problem hiding this comment.
Do you test the test here? What does this test do?
There was a problem hiding this comment.
Actually before implementing a logic in the main java file, I was trying it out in the test file. Only when It passes, i add the same logic there. This is an unnecessary test, I'll remove it
There was a problem hiding this comment.
Please let me know, if I need to remove this test
There was a problem hiding this comment.
I'd recommend to remove it, since it's basically just testing the testfile, not the reader.
| String className = vulnerability.getJSONObject("location").getString("file"); | ||
| className = (className.substring(className.lastIndexOf('/') + 1)).split("\\.")[0]; | ||
|
|
||
| if (className.startsWith(BenchmarkScore.TESTCASENAME)) { |
There was a problem hiding this comment.
Can you use Reader#testNumber and pass BenchmarkScore.TESTCASENAME to it? So instead of
String className = vulnerability.getJSONObject("location").getString("file");
className = (className.substring(className.lastIndexOf('/') + 1)).split("\\.")[0];
if (className.startsWith(BenchmarkScore.TESTCASENAME)) {
[...]
tcr.setNumber(testNumber(className));
Directly check for the test number:
int testNumber = testNumber(vulnerability.getJSONObject("location").getString("file"));
if(testNumber > -1) {
[...]
tcr.setNumber(testNumber);
Just an idea to reduce the amount of code. Disclaimer: Untested, just an idea 😅
There was a problem hiding this comment.
Thanks, Modified it and it is working
|
Hey @darkspirit510 @davewichers, I found that the results are not properly reflected in the scoreboard. I mean, the report contains a finding, but the scoreboard says that it is not present. I found the same with snyk report also. Can you please, explain me on what basis are these being checked. |
|
Did you debug the file and see if it the reader successfully reads the line/result you mentioned? Benchmark compares results to the expected results file (true/false positive and negative). Some results can be parsed, but the CWE number of a tool might be more specific than the one Benchmark is expecting. That's why some Readers map one CWE number to another. You already do this for CWE 327 in your Reader). If there's still an issue, I can have a look on the result file, but I'm on vacation, so this has to wait until next weekend 😉 |
|
@darkspirit510 - This has been sitting out there for a month+. Can you look at it when you get a chance? |
|
@davewichers The code itself looks fine to me, but according to @0xgoto's last comment, he expected some different results. To verify, I'd have to create a result file myself (or 0xgoto would have to provide one). |
|
@0xgoto - can you provide @darkspirit510 with a results file, or better yet provide him instructions on how he can generate one himself? |
|
@0xgoto - This has been dormant for 5+ months. Are you going to continue to work on this to get it working properly? |
This pull request is to add Gitlab SAST parser to Benchmark tool