Skip to content

Conversation

@manuel-sommer
Copy link
Contributor

@manuel-sommer manuel-sommer commented Aug 4, 2025

  • I advanced fix_available to be updated within each reimport: Add "has fix" filter for CVE finding #12633 (comment)
  • I added field fix_version to populate a dedicated fixed version in the header of a finding. This field can also be used to be updated equal to fix_available within the reimport. An example screenshot can be found here:
grafik - I added anchore grype to populate these fields

#12633 (comment)

@manuel-sommer
Copy link
Contributor Author

@valentijnscholten is this the right direction for updating fix_available in reimport?

@valentijnscholten
Copy link
Member

Yes

@manuel-sommer
Copy link
Contributor Author

Shall I target this against bugfix, or do we want a longer testphase and keep it against dev?

@github-actions github-actions bot added the docs label Aug 4, 2025
@manuel-sommer manuel-sommer marked this pull request as ready for review August 4, 2025 21:21
@dryrunsecurity
Copy link

dryrunsecurity bot commented Aug 4, 2025

DryRun Security

🔴 Risk threshold exceeded.

This pull request modifies several sensitive files (dojo/models.py, dojo/importers/default_reimporter.py, dojo/templates/dojo/view_finding.html, dojo/db_migrations/0247_remove_finding_insert_insert_and_more.py) flagged by the configured codepaths scanner and also includes a change in default_reimporter.py where the re-import process updates finding metadata (fix_available and fix_version) without explicit authorization checks, potentially allowing users with only scan import permissions to alter finding metadata. Please verify that these edits are intentional and either restrict the affected codepaths/authors in .dryrunsecurity.yaml or add proper authorization checks to prevent unauthorized metadata modification.

🔴 Configured Codepaths Edit in dojo/models.py
Vulnerability Configured Codepaths Edit
Description Sensitive edits detected for this file. Sensitive file paths and allowed authors can be configured in .dryrunsecurity.yaml.
🔴 Configured Codepaths Edit in dojo/templates/dojo/view_finding.html
Vulnerability Configured Codepaths Edit
Description Sensitive edits detected for this file. Sensitive file paths and allowed authors can be configured in .dryrunsecurity.yaml.
🔴 Configured Codepaths Edit in dojo/db_migrations/0247_remove_finding_insert_insert_and_more.py
Vulnerability Configured Codepaths Edit
Description Sensitive edits detected for this file. Sensitive file paths and allowed authors can be configured in .dryrunsecurity.yaml.
🔴 Configured Codepaths Edit in dojo/templates/dojo/view_finding.html
Vulnerability Configured Codepaths Edit
Description Sensitive edits detected for this file. Sensitive file paths and allowed authors can be configured in .dryrunsecurity.yaml.
🔴 Configured Codepaths Edit in dojo/models.py
Vulnerability Configured Codepaths Edit
Description Sensitive edits detected for this file. Sensitive file paths and allowed authors can be configured in .dryrunsecurity.yaml.
🔴 Configured Codepaths Edit in dojo/importers/default_reimporter.py
Vulnerability Configured Codepaths Edit
Description Sensitive edits detected for this file. Sensitive file paths and allowed authors can be configured in .dryrunsecurity.yaml.
🔴 Configured Codepaths Edit in dojo/importers/default_reimporter.py
Vulnerability Configured Codepaths Edit
Description Sensitive edits detected for this file. Sensitive file paths and allowed authors can be configured in .dryrunsecurity.yaml.
🔴 Configured Codepaths Edit in dojo/importers/default_reimporter.py
Vulnerability Configured Codepaths Edit
Description Sensitive edits detected for this file. Sensitive file paths and allowed authors can be configured in .dryrunsecurity.yaml.
🔴 Configured Codepaths Edit in dojo/importers/default_reimporter.py
Vulnerability Configured Codepaths Edit
Description Sensitive edits detected for this file. Sensitive file paths and allowed authors can be configured in .dryrunsecurity.yaml.
🔴 Configured Codepaths Edit in dojo/models.py
Vulnerability Configured Codepaths Edit
Description Sensitive edits detected for this file. Sensitive file paths and allowed authors can be configured in .dryrunsecurity.yaml.
🔴 Configured Codepaths Edit in dojo/templates/dojo/view_finding.html
Vulnerability Configured Codepaths Edit
Description Sensitive edits detected for this file. Sensitive file paths and allowed authors can be configured in .dryrunsecurity.yaml.
🔴 Configured Codepaths Edit in dojo/models.py
Vulnerability Configured Codepaths Edit
Description Sensitive edits detected for this file. Sensitive file paths and allowed authors can be configured in .dryrunsecurity.yaml.
🔴 Configured Codepaths Edit in dojo/importers/default_reimporter.py
Vulnerability Configured Codepaths Edit
Description Sensitive edits detected for this file. Sensitive file paths and allowed authors can be configured in .dryrunsecurity.yaml.
🔴 Configured Codepaths Edit in dojo/templates/dojo/view_finding.html
Vulnerability Configured Codepaths Edit
Description Sensitive edits detected for this file. Sensitive file paths and allowed authors can be configured in .dryrunsecurity.yaml.
🔴 Configured Codepaths Edit in dojo/importers/default_reimporter.py
Vulnerability Configured Codepaths Edit
Description Sensitive edits detected for this file. Sensitive file paths and allowed authors can be configured in .dryrunsecurity.yaml.
🔴 Configured Codepaths Edit in dojo/models.py
Vulnerability Configured Codepaths Edit
Description Sensitive edits detected for this file. Sensitive file paths and allowed authors can be configured in .dryrunsecurity.yaml.
🔴 Configured Codepaths Edit in dojo/models.py
Vulnerability Configured Codepaths Edit
Description Sensitive edits detected for this file. Sensitive file paths and allowed authors can be configured in .dryrunsecurity.yaml.
🔴 Configured Codepaths Edit in dojo/models.py
Vulnerability Configured Codepaths Edit
Description Sensitive edits detected for this file. Sensitive file paths and allowed authors can be configured in .dryrunsecurity.yaml.
Unauthorized Modification of Finding Metadata in dojo/importers/default_reimporter.py
Vulnerability Unauthorized Modification of Finding Metadata
Description The re-import process, triggered by a user with scan import permissions, updates the fix_available and fix_version fields of an existing finding. This update occurs within the process_matched_active_finding function without performing explicit authorization checks to ensure the user has the necessary permissions to directly edit the finding's metadata. This allows a user with potentially lower privileges (scan import) to modify critical information on a finding that they might not otherwise be authorized to change.

if existing_finding.fix_available != unsaved_finding.fix_available:
existing_finding.fix_available = unsaved_finding.fix_available
existing_finding.fix_version = unsaved_finding.fix_version

We've notified @mtesauro.


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

@valentijnscholten
Copy link
Member

The main reason fields are not updated during reimport is that there's a risk it overwrites fields that were changed by the user. At least that's what I think the reason is. And this could be an issue here are the user may have set the fix_available field to None or False explicity. On the other hand we want to make that if a fix has become available we update the finding. What are your thoughts on this @Maffooch @mtesauro

@manuel-sommer
Copy link
Contributor Author

I mean that is what is requested also in the linked issue explicitly.

@valentijnscholten
Copy link
Member

We've discussed this and since this is sort of a "status" field or "meta" field it's OK to let reimport update it. Two remarks:

  • I think we would just always need to update it, so also the case where it goes from True to False (or to None)
  • I think it would have to be updated for any existing finding, so all branches of process_matched_finding.

@Maffooch
Copy link
Contributor

Maffooch commented Aug 7, 2025

We've discussed this and since this is sort of a "status" field or "meta" field it's OK to let reimport update it. Two remarks:

  • I think we would just always need to update it, so also the case where it goes from True to False (or to None)
  • I think it would have to be updated for any existing finding, so all branches of process_matched_finding.

I agree with this approach

@manuel-sommer manuel-sommer force-pushed the reimport_fix_available branch from 7b12ecf to f32fa7a Compare August 26, 2025 04:33
@manuel-sommer
Copy link
Contributor Author

Please review @valentijnscholten

@manuel-sommer
Copy link
Contributor Author

manuel-sommer commented Aug 26, 2025

Another point @valentijnscholten :
I can also remove the mitigation field from the deduplication settings in general and update it as well. Also take a look here:
#13053
#13055
With these two PRs, all mitigation fields in use would be removed from HASHCODE_FIELDS_PER_SCANNER in settings.dist.py
This enables us to update the mitigation field also as soon as a fix_available changes. These two fields often correlate.

@valentijnscholten
Copy link
Member

Maybe start with just the fix_available field first as we are all in agreement that is good change to make.

@manuel-sommer
Copy link
Contributor Author

Could we release this for the next release on Monday @mtesauro ?

@valentijnscholten valentijnscholten added this to the 2.50.0 milestone Aug 28, 2025
@github-actions github-actions bot added New Migration Adding a new migration file. Take care when merging. ui parser and removed parser labels Aug 28, 2025
@manuel-sommer manuel-sommer force-pushed the reimport_fix_available branch from c8edad7 to cc73836 Compare August 28, 2025 10:02
@valentijnscholten
Copy link
Member

@manuel-sommer I think this PR is a nice addition. Do you need any help to get across?

@manuel-sommer
Copy link
Contributor Author

manuel-sommer commented Oct 31, 2025

@valentijnscholten , as there were multiple rebases, the db migration causes these failures. I just rebased again and adapted the db migration, everything should be fine.

Copy link
Member

@valentijnscholten valentijnscholten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@manuel-sommer Can you regenerated the migrations? This is needed due to the addition of django-pghistory.

@manuel-sommer
Copy link
Contributor Author

@manuel-sommer Can you regenerated the migrations? This is needed due to the addition of django-pghistory.

just regenerated a new migration @valentijnscholten

@manuel-sommer
Copy link
Contributor Author

I would love to see this being released as I opened the PR already in August.

Copy link
Member

@valentijnscholten valentijnscholten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved. Do you think a small note in 2.52.md should be added to make users aware? It's the first time we let reimport override any non-status fields.

@manuel-sommer
Copy link
Contributor Author

Approved. Do you think a small note in 2.52.md should be added to make users aware? It's the first time we let reimport override any non-status fields.

done @valentijnscholten

Reimport will update existing findings 'fix_available' and 'fix_version' fields based on the incoming scan report.
@valentijnscholten valentijnscholten modified the milestones: 2.52.0, 2.53.0 Nov 3, 2025
@manuel-sommer
Copy link
Contributor Author

manuel-sommer commented Nov 4, 2025

Updated the upgrading notes to 2.53

Just checking in to see if there's a timeline for the review. I'd really appreciate it if this could be merged soon – it would also save me from having to maintain the upgrade notes and db migrations every month. @mtesauro

@valentijnscholten valentijnscholten merged commit 3052ac3 into DefectDojo:dev Nov 5, 2025
151 checks passed
@manuel-sommer manuel-sommer deleted the reimport_fix_available branch November 5, 2025 20:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs New Migration Adding a new migration file. Take care when merging. parser ui unittests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants