Tag: Update allowed characters for a unified format (#12194)

* Implement migration

* Remove lower case requirement

* Create common validator

* UI: Apply validator

* API: Apply validator

* Add release notes

* Add unit tests

* Fixing ruff

* Fix some migration updates

* Applying feedback

* Silly copy/paste

Author: Maffooch

docs/content/en/open_source/upgrading/2.46.md

@@ -2,7 +2,42 @@
 title: 'Upgrading to DefectDojo Version 2.46.x'
 toc_hide: true
 weight: -20250407
-description: Import Payload Changes
+description: Tag Formatting Changes + Import Payload Changes
+---
+
+### Tag Formatting Update
+
+Tags can no longer contain the following characters:
+
+- **Commas (,)**
+- **Quotations** (both single `'` and double `"`)
+- **Spaces**
+
+#### Automatic Migration
+
+To ensure a smooth transition, an automatic migration will be applied to existing tags as follows:
+
+- **Commas** → Replaced with **hyphens (`-`)**
+- **Quotations** (single and double) → **Removed**
+- **Spaces** → Replaced with **underscores (`_`)**
+
+#### Examples
+
+- `example,tag` → `example-tag`
+- `'SingleQuoted'` → `SingleQuoted`
+- `"DoubleQuoted"` → `DoubleQuoted`
+- `space separated tag` → `space_separated_tag`
+
+#### Why This Change?
+
+This update improves consistency, enhances search capabilities, and aligns with best practices for tag formatting.
+
+#### Recommended Actions
+
+We recommend reviewing your current tags, specifically CI/CD processes before upgrading to ensure they align with the new format. Performing a database backup is also suggested as this migration cannot be reverted.
+
+Following the deployment of these new behaviors, requests sent to the API or through the UI with any of the violations listed above will result in an error, with the details of the error raised in the response.
+
 ---
 
 ### Asynchronous Import deprecation in 2.47.0

dojo/api_v2/serializers.py

@@ -118,7 +118,7 @@
     requires_tool_type,
 )
 from dojo.user.utils import get_configuration_permissions_codenames
-from dojo.utils import is_scan_file_too_large
+from dojo.utils import is_scan_file_too_large, tag_validator
 
 logger = logging.getLogger(__name__)
 deduplicationLogger = logging.getLogger("dojo.specific-loggers.deduplication")
@@ -225,6 +225,8 @@ def to_internal_value(self, data):
                 self.fail("not_a_str")
             # Run the children validation
             self.child.run_validation(s)
+            # Validate the tag to ensure it doesn't contain invalid characters
+            tag_validator(s, exception_class=RestFrameworkValidationError)
             substrings = re.findall(r'(?:"[^"]*"|[^",]+)', s)
             data_safe.extend(substrings)
 

dojo/db_migrations/0227_migrate_tags.py

@@ -0,0 +1,122 @@
+# Generated by Django 5.0.8 on 2024-09-12 18:22
+
+import logging
+from django.db import migrations
+from django.db.models import Q
+
+logger = logging.getLogger(__name__)
+
+# Only apply the process to models that _could_ have tags
+model_names = [
+    "Product",
+    "Endpoint",
+    "Engagement",
+    "Test",
+    "Finding",
+    "Finding_Template",
+    "App_Analysis",
+    "Objects_Product",
+]
+
+
+def clean_tag_value(tag: str) -> str:
+    """
+    Clean each tag value by:
+    - Converting all commas to hyphens
+    - Converting all spaces to underscores
+    - Removing all single/double quotes
+    """
+    return tag.replace(",", "-").replace(" ", "_").replace('"', "").replace("'", "")
+
+
+def clean_all_tag_fields(apps, schema_editor):
+    """
+    Cleans tag values for all models in the `model_names` list, removing unwanted characters.
+    Updates both 'tags' and 'inherited_tags' fields where applicable.
+    """
+    updated_count = {}
+    for model_name in model_names:
+        TaggedModel = apps.get_model("dojo", model_name)
+        unique_tags_per_model = {}
+        count_per_model = 0
+        # Only fetch the objects with tags that contain a character in violation
+        queryset = (
+            TaggedModel.objects.filter(
+                Q(**{"tags__name__icontains": ","})
+                | Q(**{"tags__name__icontains": " "})
+                | Q(**{"tags__name__icontains": '"'})
+                | Q(**{"tags__name__icontains": "'"})
+            )
+            .distinct()
+            .prefetch_related("tags")
+        )
+        # Iterate over each instance to clean the tags. The iterator is used here
+        # to prevent loading the entire queryset into memory at once. Instead, we
+        # will only process 500 objects at a time
+        for instance in queryset.iterator(chunk_size=500):
+            # Get the current list of tags to work with
+            raw_tags = instance.tags.all()
+            # Clean each tag here while preserving the original value
+            cleaned_tags = {tag.name: clean_tag_value(tag.name) for tag in raw_tags}
+            # Quick check to avoid writing things without impact
+            if cleaned_tags:
+                instance.tags.set(list(cleaned_tags.values()), clear=True)
+                count_per_model += 1
+                # Update the running list of cleaned tags with the changes on this model
+                unique_tags_per_model.update(cleaned_tags)
+            # Add a quick logging statement every 100 objects cleaned
+            if count_per_model > 0 and count_per_model % 100 == 0:
+                logger.info(
+                    f"{TaggedModel.__name__}.tags: cleaned {count_per_model} tags..."
+                )
+        # Update the final count of the tags cleaned for the given model
+        if count_per_model:
+            updated_count[f"{TaggedModel.__name__}"] = (
+                count_per_model,
+                unique_tags_per_model,
+            )
+    """
+    Write a helpful statement about what tags were changed for each model in the list.
+    It looks something like this:
+
+    Product: 1 instances cleaned
+      "quoted string with spaces" -> quoted_string_with_spaces
+      "quoted with spaces, and also commas!" -> quoted_with_spaces-_and_also_commas!
+      "quoted,comma,tag" -> quoted-comma-tag
+    Engagement: 1 instances cleaned
+      "quoted string with spaces" -> quoted_string_with_spaces
+      "quoted with spaces, and also commas!" -> quoted_with_spaces-_and_also_commas!
+      "quoted,comma,tag" -> quoted-comma-tag
+    Test: 1 instances cleaned
+      "quoted string with spaces" -> quoted_string_with_spaces
+      "quoted with spaces, and also commas!" -> quoted_with_spaces-_and_also_commas!
+      "quoted,comma,tag" -> quoted-comma-tag
+    Finding: 1 instances cleaned
+      "quoted string with spaces" -> quoted_string_with_spaces
+      "quoted with spaces, and also commas!" -> quoted_with_spaces-_and_also_commas!
+      "quoted,comma,tag" -> quoted-comma-tag
+    """
+    for key, (count, tags) in updated_count.items():
+        logger.info(f"{key}: {count} instances cleaned")
+        for old, new in tags.items():
+            if old != new:
+                logger.info(f"  {old} -> {new}")
+
+
+def cannot_turn_back_time(apps, schema_editor):
+    """
+    We cannot possibly return to the original state without knowing
+    the original value at the time the migration is revoked. Instead
+    we will do nothing.
+    """
+    pass
+
+
+class Migration(migrations.Migration):
+    dependencies = [
+        ("dojo", "0226_import_history_left_untouched_rename"),
+    ]
+
+    operations = [
+        migrations.RunPython(clean_all_tag_fields, cannot_turn_back_time),
+    ]

dojo/forms.py

@@ -110,6 +110,7 @@
     get_system_setting,
     is_finding_groups_enabled,
     is_scan_file_too_large,
+    tag_validator,
 )
 from dojo.widgets import TableCheckboxWidget
 
@@ -338,6 +339,9 @@ class Meta:
                 "business_criticality", "platform", "lifecycle", "origin", "user_records", "revenue", "external_audience", "enable_product_tag_inheritance",
                 "internet_accessible", "enable_simple_risk_acceptance", "enable_full_risk_acceptance", "disable_sla_breach_notifications"]
 
+    def clean_tags(self):
+        tag_validator(self.cleaned_data.get("tags"))
+
 
 class DeleteProductForm(forms.ModelForm):
     id = forms.IntegerField(required=True,
@@ -602,6 +606,9 @@ def clean(self):
 
         return cleaned_data
 
+    def clean_tags(self):
+        tag_validator(self.cleaned_data.get("tags"))
+
     # date can only be today or in the past, not the future
     def clean_scan_date(self):
         date = self.cleaned_data.get("scan_date", None)
@@ -708,6 +715,9 @@ def clean(self):
 
         return cleaned_data
 
+    def clean_tags(self):
+        tag_validator(self.cleaned_data.get("tags"))
+
     # date can only be today or in the past, not the future
     def clean_scan_date(self):
         date = self.cleaned_data.get("scan_date", None)
@@ -1021,6 +1031,9 @@ def is_valid(self):
             return False
         return True
 
+    def clean_tags(self):
+        tag_validator(self.cleaned_data.get("tags"))
+
     class Meta:
         model = Engagement
         exclude = ("first_contacted", "real_start", "engagement_type", "inherited_tags",
@@ -1076,6 +1089,9 @@ class Meta:
                   "environment", "percent_complete", "tags", "lead", "version", "branch_tag", "build_id", "commit_hash",
                   "api_scan_configuration"]
 
+    def clean_tags(self):
+        tag_validator(self.cleaned_data.get("tags"))
+
 
 class DeleteTestForm(forms.ModelForm):
     id = forms.IntegerField(required=True,
@@ -1172,6 +1188,9 @@ def clean(self):
 
         return cleaned_data
 
+    def clean_tags(self):
+        tag_validator(self.cleaned_data.get("tags"))
+
     class Meta:
         model = Finding
         exclude = ("reporter", "url", "numerical_severity", "under_review", "reviewers", "cve", "inherited_tags",
@@ -1249,6 +1268,9 @@ def clean(self):
 
         return cleaned_data
 
+    def clean_tags(self):
+        tag_validator(self.cleaned_data.get("tags"))
+
     class Meta:
         model = Finding
         exclude = ("reporter", "url", "numerical_severity", "under_review", "reviewers", "cve", "inherited_tags",
@@ -1306,6 +1328,9 @@ def clean(self):
 
         return cleaned_data
 
+    def clean_tags(self):
+        tag_validator(self.cleaned_data.get("tags"))
+
     class Meta:
         model = Finding
         exclude = ("reporter", "url", "numerical_severity", "active", "false_p", "verified", "endpoint_status", "cve", "inherited_tags",
@@ -1428,6 +1453,9 @@ def clean(self):
 
         return cleaned_data
 
+    def clean_tags(self):
+        tag_validator(self.cleaned_data.get("tags"))
+
     def _post_clean(self):
         super()._post_clean()
 
@@ -1504,6 +1532,9 @@ def clean(self):
 
         return cleaned_data
 
+    def clean_tags(self):
+        tag_validator(self.cleaned_data.get("tags"))
+
     class Meta:
         fields = ["title", "cwe", "vulnerability_ids", "cvssv3", "severity", "description", "mitigation", "impact", "references", "tags"]
         order = ("title", "cwe", "vulnerability_ids", "cvssv3", "severity", "description", "impact", "is_mitigated")
@@ -1534,6 +1565,9 @@ class Meta:
         order = ("title", "cwe", "vulnerability_ids", "cvssv3", "severity", "description", "impact")
         exclude = ("numerical_severity", "is_mitigated", "last_used", "endpoint_status", "cve")
 
+    def clean_tags(self):
+        tag_validator(self.cleaned_data.get("tags"))
+
 
 class DeleteFindingTemplateForm(forms.ModelForm):
     id = forms.IntegerField(required=True,
@@ -1587,6 +1621,9 @@ def clean(self):
             raise forms.ValidationError(msg)
         return cleaned_data
 
+    def clean_tags(self):
+        tag_validator(self.cleaned_data.get("tags"))
+
     class Meta:
         model = Finding
         fields = ("severity", "date", "planned_remediation_date", "active", "verified", "false_p", "duplicate", "out_of_scope",
@@ -1637,6 +1674,9 @@ def clean(self):
 
         return cleaned_data
 
+    def clean_tags(self):
+        tag_validator(self.cleaned_data.get("tags"))
+
 
 class AddEndpointForm(forms.Form):
     endpoint = forms.CharField(max_length=5000, required=True, label="Endpoint(s)",
@@ -1700,6 +1740,9 @@ def clean(self):
 
         return cleaned_data
 
+    def clean_tags(self):
+        tag_validator(self.cleaned_data.get("tags"))
+
 
 class DeleteEndpointForm(forms.ModelForm):
     id = forms.IntegerField(required=True,

dojo/utils.py

@@ -25,6 +25,7 @@
 from django.conf import settings
 from django.contrib import messages
 from django.contrib.auth.signals import user_logged_in, user_logged_out, user_login_failed
+from django.core.exceptions import ValidationError
 from django.core.paginator import Paginator
 from django.db.models import Case, Count, IntegerField, Q, Sum, Value, When
 from django.db.models.query import QuerySet
@@ -2697,3 +2698,21 @@ def generate_file_response_from_file_path(
     response["Content-Disposition"] = f'attachment; filename="{full_file_name}"'
     response["Content-Length"] = file_size
     return response
+
+
+def tag_validator(value: str | list[str], exception_class: Callable = ValidationError) -> None:
+    TAG_PATTERN = re.compile(r'[ ,\'"]')
+    error_messages = []
+
+    if isinstance(value, list):
+        for tag in value:
+            if TAG_PATTERN.search(tag):
+                error_messages.append(f"Invalid tag: '{tag}'. Tags should not contain spaces, commas, or quotes.")
+    elif isinstance(value, str):
+        if TAG_PATTERN.search(value):
+            error_messages.append(f"Invalid tag: '{value}'. Tags should not contain spaces, commas, or quotes.")
+    else:
+        error_messages.append(f"Value must be a string or list of strings: {value} - {type(value)}.")
+
+    if error_messages:
+        raise exception_class(error_messages)

unittests/dojo_test_case.py

@@ -632,15 +632,15 @@ def get_finding_api(self, finding_id):
         self.assertEqual(200, response.status_code, response.content[:1000])
         return response.data
 
-    def post_new_finding_api(self, finding_details, push_to_jira=None):
+    def post_new_finding_api(self, finding_details: dict, push_to_jira=None, expected_status_code: int = 201):
         payload = copy.deepcopy(finding_details)
         if push_to_jira is not None:
             payload["push_to_jira"] = push_to_jira
 
         # logger.debug('posting new finding push_to_jira: %s', payload.get('push_to_jira', None))
 
         response = self.client.post(reverse("finding-list"), payload, format="json")
-        self.assertEqual(201, response.status_code, response.content[:1000])
+        self.assertEqual(expected_status_code, response.status_code, response.content[:1000])
         return response.data
 
     def put_finding_api(self, finding_id, finding_details, push_to_jira=None):