fix for multi vuln ids when reimporting scans

[hblankenship]

Description
[sc-5982]
Resolves #10198

To re-iterate, when reimporting scans into DefectDojo, vulnerability_ids were getting duplicated with each reimport. This fix implements the same method used by the apiv2 finding helper in the baseimporter.

Test results

Tested mutiple scan types to ensure vulnerability ids were still handled correctly and that duplicates were removed on reimport. This will not clear duplicate vulnerability ids until a reimport is done after this fix.

[dryrunsecurity]

Hi there 👋, @DryRunSecurity here, below is a summary of our analysis and findings.

DryRun Security	Status	Findings
Configured Codepaths Analyzer	✅	0 findings
Sensitive Files Analyzer	✅	0 findings
AppSec Analyzer	✅	0 findings
Authn/Authz Analyzer	✅	0 findings
Secrets Analyzer	✅	0 findings

Note

🟢 Risk threshold not exceeded.

Change Summary (click to expand)

The following is a summary of changes in this pull request made by me, your security buddy 🤖. Note that this summary is auto-generated and not meant to be a definitive list of security issues but rather a helpful summary from a security perspective.

Summary:

The code changes in this pull request cover various aspects of the DefectDojo application, focusing on the handling of vulnerability IDs, the import process, and the testing of the REST API endpoints. From an application security perspective, these changes appear to be positive improvements that enhance the security and reliability of the application.

The key changes include:

1. Vulnerability ID Handling: The changes in the dojo/finding/helper.py and unittests/test_finding_helper.py files optimize the process of saving vulnerability IDs associated with findings, improving the efficiency and performance of this critical functionality.

2. Import Process Improvements: The changes in the dojo/importers/base_importer.py file simplify the process_vulnerability_ids function, removing unnecessary logic and focusing on the core responsibility of processing and saving vulnerability IDs.

3. Comprehensive API Testing: The changes in the unittests/test_rest_framework.py file demonstrate a thorough approach to testing the security and functionality of the DefectDojo REST API, covering aspects such as authorization, permissions, input validation, and sensitive data handling.

Overall, these code changes are focused on improving the security, reliability, and maintainability of the DefectDojo application. The optimization of vulnerability ID handling, the streamlining of the import process, and the extensive API testing all contribute to a more secure and robust application.

Files Changed:

1. dojo/finding/helper.py: The changes optimize the save_vulnerability_ids function by using the bulk_create method to create multiple Vulnerability_Id objects at once, improving the efficiency of the data persistence operation.

2. unittests/test_finding_helper.py: The changes update the unit tests for the save_vulnerability_ids and save_vulnerability_id_templates functions, removing unnecessary assertions and focusing on the core functionality of managing vulnerability IDs.

3. dojo/importers/base_importer.py: The changes simplify the process_vulnerability_ids function by removing the previous logic for synchronizing the cve field and focusing on the core responsibility of processing and saving vulnerability IDs.

4. unittests/test_importers_importer.py: The changes add a new TestImporterUtils class that tests the utility functions used in the importer module, including the handling of vulnerability IDs, references, and CVEs.

5. unittests/test_rest_framework.py: The changes demonstrate a comprehensive approach to testing the security and functionality of the DefectDojo REST API, covering aspects such as authorization, permissions, input validation, and sensitive data handling.

Powered by DryRun Security

[kiblik]

I'm not sure that this is the best idea. If I may suggest, I would:

• Add db_mig which would drop (finding,vuln_id) duplicities (for inspiration: https://gist.github.com/victorono/cd9d14b013487b8b22975512225c5b4c)
• Add db constraint to avoid this situation in the future: class Meta: constraints = [models.UniqueConstraint(fields=['finding', 'vulnerability_id'])]
• Do not remove all existing entries related to finding but
  • Remove only unnecessary Vulnerability_Id.objects.filter(finding=finding).exclude(vulnerability_id__in=finding.unsaved_vulnerability_ids).delete()
  • Call DB only for the creation of new entries, outside of the for loop (to save db calls) Vulnerability_Id.objects. bulk_create([Vulnerability_Id(finding=finding, vulnerability_id=vulnerability_id) for vulnerability_id in finding.unsaved_vulnerability_ids])

[hblankenship]

I'm not sure that this is the best idea. If I may suggest, I would:

• Add db_mig which would drop (finding,vuln_id) duplicities (for inspiration: https://gist.github.com/victorono/cd9d14b013487b8b22975512225c5b4c)

• Add db constraint to avoid this situation in the future: class Meta: constraints = [models.UniqueConstraint(fields=['finding', 'vulnerability_id'])]

• Do not remove all existing entries related to finding but

  • Remove only unnecessary Vulnerability_Id.objects.filter(finding=finding).exclude(vulnerability_id__in=finding.unsaved_vulnerability_ids).delete()
  • Call DB only for the creation of new entries, outside of the for loop (to save db calls) Vulnerability_Id.objects. bulk_create([Vulnerability_Id(finding=finding, vulnerability_id=vulnerability_id) for vulnerability_id in finding.unsaved_vulnerability_ids])

Regarding this suggestion, this does not fix the issue with multiple vulnerability ids with each import (possibly due to the constraints having already been violated for existing imports). I am incorporating the bulk create but maintaining the full delete for now.

[kiblik]

If there is an issue with duplicates during processing, I suppose set() might help here.
The best would be to define unsaved_vulnerability_ids as the set. But for a quick fix, at least using bulk_create(set(...)) would remove duplicates.

[Maffooch]

This is not feedback on the approach taken here, but this function is used in other places to set the vulnerability IDs correctly. It should be used directly instead of copy/pasting code from it
https://github.com/DefectDojo/django-DefectDojo/blob/2234848f02f243b5250894fccb6fc05ccaa1bc0d/dojo/finding/helper.py#L645C5-L645C27

[hblankenship]

dropping this PR for bugfix PR