Skip to content

Support multiple sites in DAST reports

Paul Gascou-Vaillancourt requested to merge 11930-dast-multi-sites-fe into master

What does this MR do?

Up to this point, DAST reports contained a single site that was represented in the form of an Object. We now want to support cases where DAST reports can contain multiples sites, where the site property will be an Array instead of an Object. In fact, zaproxy was updated to normalize the site property into an Array, even when there's a single site.

In this MR, support for multiple sites has been implemented, while preserving support for the legacy format with site as an Object.

Here's a DAST report with 2 sites that can be used for testing this MR: gl-dast-report.json

Jul-23-2019_13-29-21

Some notes about the UX

  • The same vulnerability appears 2 times, without any clear distinction between the two occurrences
    • This could occur in production and is expected because it's possible for one vulnerability to be reported in two different sites.
    • It would be relatively easy to show the site's @name in the vulnerabilities list and/or the modal to clear out some of the confusion that comes with having duplicated entries. This would most likely require someone from UX to come up with the best way to make this distinction clear to the users.
    • When dismissing one of the occurrence then refreshing the page, both occurrences are marked as dismissed because they both have the same identifier (see https://gitlab.com/gitlab-org/gitlab-ee/issues/5678)
      • One way of quick fixing this would be to include the @name property when generating the project_fingerprint in the frontend but this would cause some backward compatibility issues since previously dismissed vulnerabilities would have their hash updated, thus being marked as non-dismissed. So I would say this is out of the question.
        • I've asked about this issue in the Vulnerability fingerprints overview and Q&A and it looks like we will still have colliding fingerprints once the hashing has been updated and moved to the backend and there doesn't seem to be a consensus on how we should address this going forward. In light of this information, I'll assume that this issue is acceptable for now and I won't do anything to mitigate it unless someone else raises some concerns.

Does this MR meet the acceptance criteria?

Conformity

Performance and testing

Closes frontend part for #11930 (closed)

Edited by Paul Gascou-Vaillancourt

Merge request reports