API: prevent duplicate saves of taggable entities or when pushing to JIRA

[valentijnscholten]

When creating or updating findings (or other entities that have a tags field), the Finding.save() method was called multiple times. Ultimately the finding was saved correctly, but this lead to unnecessary use of resources and could in the future lead to problems if/when the Defect Dojo could be relying more on Django signals such as post_save().

The above was caused by the way the tags fields were implemented long ago. I tried to tweak the TaggitSerializer to prevent the duplicate saves from happening, but this lead nowhere. Then I realized I could just tweak our TagListSerializer to make it work without the "extra" TaggitSerialiser.

I also looked at the from tagulous.contrib.drf import TagSerializer, but that doesn't support the single string with multiple comma separated tags that we re-added support for in #12434

Duplicate saves could also be triggered when the finding needed to be pushed to JIRA. This PR also resolves that.

Added some unit tests to make sure all looks well.

I had to rerecord one JIRA related tests around finding groups. Usually this is a red flag, but looking at the test case and asserts I think all is still good. Maybe the old codebase did some duplicagte/unnecessary/idempotent calls to JIRA?

[sc-10686]

[github-actions]

This pull request has conflicts, please resolve those before we can evaluate the pull request.

[dryrunsecurity]

🔴 Risk threshold exceeded.

This pull request contains multiple security vulnerabilities including debug information exposure, verbose error logging that could aid attackers, and a potential CVE ID manipulation risk in different parts of the application's codebase.

Debug Information Exposure in dojo/jira_link/helper.py

Vulnerability	Debug Information Exposure
Description	Debug log messages directly log object contents without sanitization, potentially exposing sensitive internal details if debug logging is enabled in production.

django-DefectDojo/dojo/jira_link/helper.py

Lines 667 to 675 in 7f3d035

	raise ValueError(msg)
	if isinstance(obj, Finding):
	if obj.has_finding_group:
	logger.debug("pushing finding group for %s to JIRA", obj)
	return push_finding_group_to_jira(obj.finding_group, *args, **kwargs)
	return push_finding_to_jira(obj, *args, **kwargs)
	if isinstance(obj, Finding_Group):

Verbose Error Logging in dojo/api_v2/views.py

Vulnerability	Verbose Error Logging
Description	Error messages include the full list of tags and specific invalid tag, which could aid an attacker in enumerating valid tags through targeted error probing.

django-DefectDojo/dojo/api_v2/views.py

Lines 989 to 1001 in 7f3d035

	all_tags = serializers.TagSerializer({"tags": all_tags}).data[
	"tags"
	]
	for tag in new_tags.validated_data["tags"]:
	for sub_tag in tagulous.utils.parse_tags(tag):
	if sub_tag not in all_tags:
	all_tags.append(sub_tag)
	new_tags = tagulous.utils.render_tags(all_tags)
	finding.tags = new_tags
	finding.save()
	else:

CVE ID Manipulation in dojo/api_v2/serializers.py

Vulnerability	CVE ID Manipulation
Description	The code sets the CVE field by taking the first vulnerability ID from a user-provided list without validation. This could allow an attacker to manipulate the CVE identifier for a finding by providing a crafted list of vulnerability IDs.

django-DefectDojo/dojo/api_v2/serializers.py

Lines 1734 to 1765 in 7f3d035

	# Overriding this to push add Push to JIRA functionality
	def update(self, instance, validated_data):
	# push_all_issues already checked in api views.py
	push_to_jira = validated_data.pop("push_to_jira")
	# Save vulnerability ids and pop them
	parsed_vulnerability_ids = []
	if (vulnerability_ids := validated_data.pop("vulnerability_id_set", None)):
	logger.debug("VULNERABILITY_ID_SET: %s", vulnerability_ids)
	parsed_vulnerability_ids.extend(vulnerability_id["vulnerability_id"] for vulnerability_id in vulnerability_ids)
	logger.debug("SETTING CVE FROM VULNERABILITY_ID_SET: %s", parsed_vulnerability_ids[0])
	validated_data["cve"] = parsed_vulnerability_ids[0]
	# Save the reporter on the finding
	if reporter_id := validated_data.get("reporter"):
	instance.reporter = reporter_id
	instance = super().update(
	instance, validated_data,
	)
	if parsed_vulnerability_ids:
	save_vulnerability_ids(instance, parsed_vulnerability_ids)
	if push_to_jira:
	jira_helper.push_to_jira(instance)
	return instance
	def validate(self, data):
	if self.context["request"].method == "PATCH":

We've notified @mtesauro.

---

All finding details can be found in the DryRun Security Dashboard.

[github-actions]

Conflicts have been resolved. A maintainer will review the pull request shortly.

[Maffooch]

This is looking really promising! Excellent work here

[Maffooch]

I have been manually adding this back in a pinch for so long, but have always forgot to commit it 🥴 thank you for doing so!

[Maffooch]

Seems like the extra save could be removed since th vulnerability IDs have. dedicated function now

[valentijnscholten]

that extra functions doesn't actually save anything on the finding itself, I think we have to add a small test to ensure a CVE is stored on the finding itself. Unless that field is slated for deprecation?

[valentijnscholten]

I added some tests to make sure vulnerability ids are stored and the cve is populated

[Maffooch]

Please clean up those extra comments when you are done with all the manual testing/validation

[Maffooch]

curious why this was needed. Does the tagulous.utils.parse_tags function not work as expected when passed a list vs individual strings?

[valentijnscholten]

IIRC it parses the string repesentation of the list ending up with something like:

• ["tag1"
• "tag2"
• "tag3"]
  But it could be that it was needed because I also changed the TagListSerializerField to not render the tags as one string. That changes allows us to just let the ModelSerializer serialize the tags without having to pop them and process them ourselves.

[Maffooch]

Excellent work here! These are my favorite kinds of changes to see here 🤗

[mtesauro]

Approved