Tag: Update allowed characters for a unified format

[Maffooch]

Tags do not have consistent behavior between the UI and the API. This is caused by a handful of characters:

• Spaces
• Commas
• Quotes single/double

To create cohesion, we should no longer allow these characters as they tend to make global searches very tricky/nonfunctional anyways. The PR accomplishes the following

• Migrations to convert spaces to underscores, commas to hyphens, and removes singe/double quotes.
• Release notes explaining the previous point in further detail
• Common validation for the UI and API
• Unit tests to reflect the change in behavior

Screenshots:

[sc-10605]

[dryrunsecurity]

DryRun Security Summary

DefectDojo version 2.46.x implements comprehensive tag validation and sanitization improvements to enhance security by preventing potential injection vulnerabilities and standardizing tag formatting across the application.

Expand for full summary

Summary: DefectDojo version 2.46.x introduces comprehensive tag formatting changes across multiple components, implementing stricter tag validation and cleaning mechanisms to improve input sanitization and prevent potential security risks.

Security Findings:

1. Tag Validation Improvements

   • Systematic input validation for tags across multiple forms and API endpoints
   • Prevents tags containing:
     • Spaces
     • Commas
     • Single quotes
     • Double quotes
   • Helps mitigate potential injection and parsing vulnerabilities

2. Logging Considerations

   • Potential information disclosure risk in migration logs
   • Recommendation to ensure proper log configuration and rotation

3. Input Sanitization

   • Automatic tag cleaning during database migration
   • Replaces problematic characters with safe alternatives
   • Removes quotes and standardizes tag formatting

4. Error Handling

   • Consistent use of validation errors for non-compliant tags
   • Rejects invalid tag requests early in processing pipeline
   • Prevents downstream processing of malformed inputs

5. Operational Security

   • Changes will require users to modify existing tags and CI/CD processes
   • Enforces stricter tag formatting rules across the application

View PR in the DryRun Dashboard.

[mtesauro]

Approved

[cneill]

A couple comments/questions here

[valentijnscholten]

Have you considered using bulk_update to make it faster. Not sure of this is possible with the way tags are implemented.
Or using raw sql?
Otherwise it might be helpful to output some kind of (info level) logging every 1000 models/tags. We see some reports on the #defectdojo slack where people think the migration is frozen or they don't realize it's not done yet or restart the initializer while it's still migrating, etc.

[Maffooch]

Have you considered using bulk_update to make it faster.

I did try to take this method, but was stuck on the m2m relationship. Will see what I can do about logging

[github-actions]

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

[github-actions]

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

[dryrunsecurity]

This pull request introduces enhanced input validation for tags, focusing on preventing injection attacks and potential security risks, while also recommending careful migration procedures and improved error handling to protect system integrity and data privacy.

💭 Unconfirmed Findings (5)

Vulnerability	Input Validation Enhancements
Description	New tag validation mechanism implemented across multiple files, preventing tags from containing commas, quotation marks (single and double), and spaces. Focuses on input sanitization and blocking potential special characters.

Vulnerability	Potential Logging Sensitivity
Description	Migration logs may expose tag transformation details, raising potential privacy and security concerns. Recommendation includes securing and rotating log files to mitigate information disclosure risks.

Vulnerability	Security Considerations in Tag Processing
Description	Proactive approach to preventing injection attacks through consistent validation across forms and API endpoints. Uses regex-based validation to block special characters in tags, enhancing overall system security.

Vulnerability	Migration Process Risks
Description	Potential impact on existing systems and integrations during version upgrade. Recommends database backup before upgrade and warns of possible data transformation risks during migration process.

Vulnerability	Error Handling Improvements
Description	Utilizes RestFrameworkValidationError to catch and reject invalid tags, implementing early validation in the processing pipeline to prevent malformed tag inputs from being processed.

---

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

[Maffooch]

Have you considered using bulk_update to make it faster. Not sure of this is possible with the way tags are implemented.
Or using raw sql?

Turns out m2m is not supported by bulk update. I did some research on this, and it turns out that using the .set method is the best way to this sort of thing.

Otherwise it might be helpful to output some kind of (info level) logging every 1000 models/tags.

I went with every 100 in case things went slow for some users. This will at least provide some good feedback if needed to manage expectations

[valentijnscholten]

Looks good now, thanks.

[valentijnscholten]

Not blocking for merge: Could there be more characters that could cause problems? I was wondering if there should be an allowlist regex instead of block the 3 charactes in this PR.
What happens if someone posts a tag with one of these bad boys in it:

[cneill]

Not blocking for merge: Could there be more characters that could cause problems? I was wondering if there should be an allowlist regex instead of block the 3 charactes in this PR.

I think this is a good idea if Tagulous doesn't already reject random non-printable characters, but I would guess that it rejects such characters already? If not, maybe worth a follow-up PR

[Maffooch]

Let's save that for another time 😅