feat:Add DAST audit command and related functionality - #1078
Conversation
- Implemented AviatorSSCDastAuditCommand for auditing DAST findings in SSC applications. - Created DastAuditRequestMapper and DastAuditResponseMapper for handling DAST audit requests and responses. - Added unit tests for DAST audit request and response mappers, stream processor, and command. - Introduced AviatorSSCFprTransferHelper for managing DAST FPR downloads and uploads. - Updated AviatorSSCAuditHelper to include DAST-specific audit statistics. - Enhanced AviatorSSCCommands to include the new DAST audit command. - Updated internationalization properties for DAST audit command messages.
| int likelyFalsePositives = 0; | ||
| int failed = 0; | ||
| int serverSkipped = 0; | ||
| Set<String> respondedIssueIds = new java.util.HashSet<>(); |
There was a problem hiding this comment.
Use proper import (here, and any other similar occurences)
| String status = succeeded == workItems.size() ? "AUDITED" | ||
| : succeeded > 0 ? "PARTIALLY_AUDITED" : "FAILED"; | ||
| String message = succeeded == 0 ? "No DAST audit responses were successfully processed" : null; | ||
| return new DastAuditFprResult( |
There was a problem hiding this comment.
Way too many constructor arguments; especially where arguments have same type, it's easy to accidentally put them in wrong order. Use Lombok @Builder pattern or similar.
| AuditResponse response = DastAuditDecisionMapper.toAuditResponse(result); | ||
| if ("SUCCESS".equalsIgnoreCase(response.getStatus()) && response.getAuditResult() != null) { | ||
| successfulResponses.put(result.issueId(), response); | ||
| var success = (com.fortify.cli.aviator.grpc.DastAuditResult.Success) result; |
| } | ||
|
|
||
| private static EligibilityResult eligibleWorkItems( | ||
| List<com.fortify.cli.aviator.dast.DastSession> sessions, |
| continue; | ||
| } | ||
| if (auditIssue != null && isProcessedByAviator(auditIssue)) { | ||
| processed++; |
There was a problem hiding this comment.
Here and in for example EligibilityResult, it's unclear whether processed refers to 'processed in current run', or 'already processed in earlier run'. Better to nae this for example alreadyProcessed.
| import picocli.CommandLine.Mixin; | ||
| import picocli.CommandLine.Option; | ||
|
|
||
| @Command(name = "audit-dast") |
There was a problem hiding this comment.
Why do we have separate audit commands for SAST and DAST? From a user perspective, wouldn't it be more logical to have a single audit command that can audit both SAST & DAST issues (either audit all issues with a single command invocation, or have a CLI option to select between SAST or DAST audit)?
Whether this is feasible largely depends on whether SAST & DAST audits share the same CLI options, or we need specific CLI options that are relevant for DAST but not SAST, or vice versa (also see other comment/question as to why audit seems to have many more options than audit-dast).
If we do keep this as separate commands, we should implement consistent command names like we've done elsewhere:
- Rename
AviatorSSCAuditCommandtoAviatorSSCSastAuditCommandwith correspondingaudit-sastcommand name - For backward compatibility, introduce a deprecated
auditcommand that just invokes theaudit-sastcommand
| private DastAuditFprResult auditFpr( | ||
| Path fprPath, | ||
| SSCAppVersionDescriptor appVersion, | ||
| com.fortify.cli.aviator._common.session.user.helper.AviatorUserSessionDescriptor session, |
| public class AviatorSSCDastAuditCommand extends AbstractSSCJsonNodeOutputCommand implements IActionCommandResultSupplier { | ||
| private static final Logger LOG = LoggerFactory.getLogger(AviatorSSCDastAuditCommand.class); | ||
|
|
||
| @Getter @Mixin private OutputHelperMixins.DetailsNoQuery outputHelper; |
There was a problem hiding this comment.
The SAST audit command seems to have a lot more options (quota management, filter set/folder options, ...); why don't we have the same for DAST audit? In SSC, DAST issues are also organized in folders based on issue templates/filter sets, so I'd expect the filter set/folder-related options to be available for both SAST & DAST audits.
| private String actionResult = "SKIPPED"; | ||
|
|
||
| @Override | ||
| public JsonNode getJsonNode(UnirestInstance unirest) { |
There was a problem hiding this comment.
I haven't checked, but I'd assume that SAST & DAST audit commands share the same overall structure; would it make sense to introduce a common abstract base class that defines the shared logic, both to ensure consistency and reduce code duplication?
| } | ||
|
|
||
| private static String getDastAuditMessage(DastAuditFprResult auditResult) { | ||
| return switch (auditResult.status()) { |
There was a problem hiding this comment.
String comparison is fragile; if anyone every changes/adds/removes any of these strings in core Aviator code, this switch statement might produce inconsistent results. Better to use enum values for example.
This PR adds DAST auditing support to FCLI, allowing teams to audit WebInspect findings with Fortify Aviator and upload the updated DAST FPR to SSC.
What changed
fcli aviator ssc audit-dastaudit.xml.Behavior
Command
A custom tag mapping can be supplied with:
Example Response