- 
                Notifications
    You must be signed in to change notification settings 
- Fork 1.8k
feat: Add dry_run support for reimport-scan operations #13563
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Conversation
Implements optional dry_run parameter for reimport-scan API endpoints to preview changes without modifying the database. Useful for CI/CD pipelines to validate scan results before merging to production. - Add dry_run parameter to reimport-scan API endpoints - Implement dry_run_reimport() method respecting all reimport options - Return detailed preview statistics and findings details - Add comprehensive test suite with 10 test cases - Fix missing SLA_Configuration in test fixtures
| 🔴 Risk threshold exceeded.This pull request modifies several sensitive codepaths (serializers, engagement views, importers, test views) and contains security issues: missing authorization on reopen_eng (IDOR), unsafe removal of findings in view_edit_risk_acceptance (IDOR), and possible information disclosure by prefetching finding/group details into templates. These changes should be reviewed to add proper access checks and restrict edits to listed sensitive files/authors. 
🔴 Configured Codepaths Edit in  | 
| 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/engagement/views.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_importer.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/options.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/test/views.py
| Vulnerability | Configured Codepaths Edit | 
|---|---|
| Description | Sensitive edits detected for this file. Sensitive file paths and allowed authors can be configured in .dryrunsecurity.yaml. | 
🟡 Potential IDOR Vulnerability in dojo/engagement/views.py
| Vulnerability | Potential IDOR Vulnerability | 
|---|---|
| Description | The reopen_engfunction retrieves anEngagementobject using a user-supplied ID (eid) without any explicit authorization checks. While theedit_engagementanddelete_engagementfunctions in the same file are protected by@user_is_authorized(Engagement, Permissions.Engagement_Edit, "eid"), thereopen_engfunction lacks this or any similar decorator. This allows any authenticated user to potentially reopen any engagement by manipulating theeidparameter in the URL, leading to an Insecure Direct Object Reference (IDOR) vulnerability. Thereopen_engagementfunction itself does not perform authorization checks, relying on the caller to enforce permissions. | 
django-DefectDojo/dojo/engagement/views.py
Lines 1189 to 1192 in 44c8065
| eng = Engagement.objects.get(id=eid) | |
| reopen_engagement(eng) | |
| messages.add_message(request, messages.SUCCESS, "Engagement reopened successfully.", extra_tags="alert-success") | |
| return HttpResponseRedirect(reverse("view_engagements", args=(eng.product.id,))) | 
Information Disclosure via Finding Group Details Exposure in dojo/test/views.py
| Vulnerability | Information Disclosure via Finding Group Details Exposure | 
|---|---|
| Description | The get_initial_contextmethod addsfinding_groupsto the template context, prefetching relatedfindings,jira_issue,creator, andfindings__vulnerability_id_set. This loads potentially sensitive data into memory and passes it to the template. Without explicit, fine-grained authorization checks applied to each prefetched related object before rendering, a user with access to the test view could potentially view details of findings, Jira issues, or vulnerability IDs that they are not otherwise authorized to see. | 
django-DefectDojo/dojo/test/views.py
Lines 179 to 194 in 44c8065
| "person": request.user.username, | |
| "request": request, | |
| "show_re_upload": any(test.test_type.name in code for code in get_choices_sorted()), | |
| "creds": Cred_Mapping.objects.filter(engagement=test.engagement) | |
| .select_related("cred_id") | |
| .order_by("cred_id"), | |
| "cred_test": Cred_Mapping.objects.filter(test=test).select_related("cred_id").order_by("cred_id"), | |
| "jira_project": jira_helper.get_jira_project(test), | |
| "bulk_edit_form": FindingBulkUpdateForm(request.GET), | |
| "enable_table_filtering": get_system_setting("enable_ui_table_based_searching"), | |
| "finding_groups": test.finding_group_set.all().prefetch_related( | |
| "findings", "jira_issue", "creator", "findings__vulnerability_id_set" | |
| ), | |
| "finding_group_by_options": Finding_Group.GROUP_BY_OPTIONS, | |
| } | |
| # Set the form using the context, and then update the context | 
Insecure Direct Object Reference (IDOR) in dojo/engagement/views.py
| Vulnerability | Insecure Direct Object Reference (IDOR) | 
|---|---|
| Description | The view_edit_risk_acceptancefunction indojo/engagement/views.pyallows a user to remove a finding from a risk acceptance. When processing the 'remove_finding' action, the function retrieves theFindingobject usingget_object_or_404(Finding, pk=request.POST["remove_finding_id"]). Thepk(primary key) is taken directly from user input without any validation to ensure that the requestedfindingbelongs to theengagement(eid) that the user is authorized to access. This means an attacker could potentially manipulate theremove_finding_idparameter to remove a finding from a risk acceptance in an engagement they are not authorized for, leading to an Insecure Direct Object Reference (IDOR). | 
django-DefectDojo/dojo/engagement/views.py
Lines 124 to 129 in 44c8065
| @cache_page(60 * 5) # cache for 5 minutes | |
| @vary_on_cookie | |
| def engagement_calendar(request): | |
| if not get_system_setting("enable_calendar"): | |
| raise Resolver404 | |
We've notified @mtesauro.
All finding details can be found in the DryRun Security Dashboard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the PR. We'll take a look and see if the feature is something that we can add without adding too much maintenance burden. Although we have lifted the feature freeze, it's still advised to double check before starting a PR.
Some initial thoughts:
- More than half of the changes in the PR seem to be cosmetic. Would it be possible to revert these cosmetic changes so it becomes easier to digest and review?
- The dry_run now seems to be a code path seperate from the "real" reimport. Have you considered using the rel reimport code path and add some if-statements to make a dry_run mode? Two code paths increase the risk of differences/mismatches.
- The reimport matching logic can be influenced by finding that are created during reimport. For example if finding 100 and 101 in the report are new and both have the same hash_code, the finding 101 will not be imported as it is matched against finding 100. In dry_run mode this match won't happen, so the statistics might be different. I haven't verified this, but this is what I am thinking.
- The reimport process might change in the near future where it will perform batch based matching and/or batch processing of the report. I am not sure if it will be possible to maintain a 100% complete dry_run feature.
| 
 Yeah my bad, that's my IDE 😅 I'll revert that. 
 I first did that but it was lot of if conditions, didn't like it and decided to separate it, if you prefer this i can switch to that. 
 I'll verify its properly implemented and fix it if it's not the case. 
 I guess that's a good reason to switch to if statements 😅 | 
Description
This PR implements a dry_run feature for reimport-scan operations that allows users to preview what changes would occur during a reimport without making any database modifications.
Key Features:
dry_runboolean parameter (defaults tofalse) on reimport-scan API endpointsclose_old_findings,do_not_reactivate, severity filters, etc.)Use Case:
This feature is particularly useful for CI/CD pipelines where teams want to validate scan results and preview changes before merging to production environments.
API Response Example:
{ "test_id": 123, "dry_run": true, "changes_preview": { "would_create": 5, "would_reactivate": 2, "would_close": 3, "would_leave_untouched": 10, "total_changes": 10 }, "findings_details": { "new_findings": [...], "reactivated_findings": [...], "closed_findings": [...], "unchanged_findings": [...] } }Documentation
API documentation is included in the serializer field descriptions (
help_text).Files Changed
dojo/api_v2/serializers.py- API integration for dry_run parameterdojo/importers/default_reimporter.py- Core dry_run_reimport() logicdojo/importers/default_importer.py- Return signature updatedojo/importers/options.py- dry_run parameter validationdojo/fixtures/dojo_testdata.json- Fixed missing SLA_Configurationdojo/engagement/views.py- View layer dry_run supportdojo/test/views.py- View layer dry_run supportunittests/dojo_test_case.py- Test helper methodsunittests/test_import_reimport.py- Minor test adjustmentsunittests/test_import_reimport_dry_run.py- New comprehensive test suiteLabels to add:
enhancementfeature