Close Old Findings doesn't work for parsers that set the service field

[valentijnscholten]

Whenever parsers set the service field on findings, this breaks the close_old_findings functionality.

For example the StackHawk parser sets the service field based on the application name from the report. All findings get a service value, for example "Application A".
But the Reimport requests did not contain a service value. The Close Old Findings feature will only close findings that have the service value specified in the request. So any findings that have a non-empty value for service will not get closed. In this case no findings get closed as all of them have Application A from the original import of the report.

Also if the application name changes the findings in the reimport report will no longer be matched against existing findings.

I think parser should never set the service fields as it is meant to be a meta field.

A quick search shows these affected parsers:

[mtesauro]

I agree with that thinking.

I see service as something a user of DefectDojo would set vs something a parser should never set since it cannot know what any specific user wants for service.

[manuel-sommer]

Interesting finding. Multiple contributions here were implemented from me. So if the right approach is to remove the service field, I can submit PRs to fix it in the parsers. Please give me feedback @valentijnscholten or @mtesauro before I submit a PR.
Also: Is it possible to cover this edge case (and maybe other fields) through a global unittest? Something like: "This field is used within a parser, but is not foreseen to be used within a parser as it breaks other functionalities"? Are there additional fields that affect close_old_findings or other functionalities?

[valentijnscholten]

The challenge is that if we adjust the parsers to no longer set the service field, this will affect the hash_code calculation. So probably we would have to have a data migration that unsets the service field for existing findings. However there is no way to know if those service values came from the parser or from the import/reimport request parameter. Do you have any idea on what a sensible approach forward might be?

[manuel-sommer]

1. Fix parsers: Update affected parsers to no longer set the service field at all.
2. To avoid breaking hash matching and reimport behavior, we can follow the same pattern used in previous migrations (for example 0208_merge_acunetix.py):

• Identify findings created by affected parsers (based on test.test_type)
• Only touch findings where the service is set and the parser is known to have populated it
• For those findings set service = None, recompute hash_code, save the finding

How does this sound like?

[manuel-sommer]

Do you have any feedback on this @valentijnscholten or @mtesauro ?

[valentijnscholten]

I think that is the best we can do, but we risk overwriting/cleaning service values that were set by the import/reimport parameter. But we can't distinguish these values. Maybe we should let this issue sit for a bit and then use the extra data from #14673 to take that parameter into account.