search

OWASP-Benchmark / BenchmarkUtils

OWASP Benchmark Project Utilities - Provides scorecard generation and crawling tools for Benchmark style test suites.
https://owasp.org/www-project-benchmark/
GNU General Public License v3.0
16 stars 50 forks source link

Check for matching Reader within reader and not in BenchmarkScore (+ lots of tests) #11

Closed darkspirit510 closed 2 years ago

darkspirit510 commented 3 years ago

This is just an idea about a refactoring, it does not work (for now). Just moved some lines around and created new classes.

Reason: It feels strange to have the selection of tool within BenchmarkScore.java, but other stuff seperated in several classes. Moreover, BenchmarkScore.java is almost 2k lines long. I had the idea to create a wrapper class to access a result file (including JSON/XML/String/Whatever access) and use some kind of marker (in this case abstract superclass, could be annotation/interface, too) and just ask each instance if it is the correct reader for a given file.

darkspirit510 commented 3 years ago

Especially see https://github.com/OWASP-Benchmark/BenchmarkUtils/pull/11/files#diff-99faf5b0b3ac06fff3d59a39a8b52afbd155b4a2a601423ebde59ed575283b5cR928

darkspirit510 commented 3 years ago

PS: I'd prefer to just create classes without having to touch BenchmarkScore and do crazy things like:

        readers.add(new HorusecReader());
        readers.add(new InsiderReader());
        readers.add(new ShiftLeftScanReader());
        readers.add(new WapitiJsonReader());
        readers.add(new ZapJsonReader());

But I didn't find a solution I like...

davewichers commented 3 years ago

Some of the readers provide methods BenchmarkScore can call that determine if a specific file matches for that tool. I can't recall the method name but I'm sure you can find example. The thing I like about that pattern is the logic for results file matching is in the parser for the tool, rather than in BenchmarkScore itself. Maybe that pattern can help some?

darkspirit510 commented 3 years ago

Yes, all the readers I contributed (Horusec, Insider, ShiftLeftScan, Wapiti, ZaqJson) come with such method, this is just a step further and getting rid of the if-chain in BenchmarkScore. Just iterate over a collection of Readers and find the one (if any) that is capable of reading a given file.

darkspirit510 commented 3 years ago

Migrated all Readers (except FortifyReader, will follow soon). Reduced/randomized some SAST testdata to be used in JUnit tests. Not sure about how to get result files of all the commercial tools...