Skip to content

feat: add Remove from Finding bulk action on View Finding page#14461

Open
valentijnscholten wants to merge 6 commits intoDefectDojo:bugfixfrom
valentijnscholten:feat/remove-endpoints-from-finding
Open

feat: add Remove from Finding bulk action on View Finding page#14461
valentijnscholten wants to merge 6 commits intoDefectDojo:bugfixfrom
valentijnscholten:feat/remove-endpoints-from-finding

Conversation

@valentijnscholten
Copy link
Member

@valentijnscholten valentijnscholten commented Mar 7, 2026

Summary

While working on #14660 I noticed the "delete" icon was missing from the endpoint bulk edit menu on the View Finding page.

  • Add a Remove from Finding trash button next to the Bulk Edit dropdown on the View Finding page, allowing users to disassociate selected endpoints from a finding without deleting the endpoint objects themselves
  • Non-V3 path (endpoint_status_bulk_update): when remove_from_finding is posted, delete the matching Endpoint_Status records for the selected endpoints
  • V3/Locations path (finding_location_bulk_update): when remove_from_finding is posted, delete the matching LocationFindingReference records
  • Both views already require Finding_Edit permission via @user_is_authorized; no additional permission check is needed
  • The JS appends selected endpoint IDs and the remove_from_finding flag to the existing bulk_change_form before submitting, reusing the same form and URL as the bulk status update
  • Add unit tests covering single removal, partial removal, multi-endpoint removal, Endpoint_Status cleanup, no-op without the flag, and redirect behaviour
image

Adds a trash button next to the Bulk Edit dropdown on the View Finding
page that removes the selected endpoints from the finding without
deleting the endpoint objects themselves.

- endpoint_status_bulk_update (non-V3): if remove_from_finding is posted,
  delete the matching Endpoint_Status records for the selected endpoints
- finding_location_bulk_update (V3): if remove_from_finding is posted,
  delete the matching LocationFindingReference records
- view_finding.html: add "Remove from Finding" button that appends the
  selected endpoint IDs and a remove_from_finding flag to the existing
  bulk_change_form before submitting
- Add unit tests covering single, partial, and full removal, Endpoint_Status
  cleanup, no-op without the flag, and redirect behaviour
@override_settings does not affect URL routing (which is set up at
startup). In V3=true CI, endpoints_status_bulk resolves to the V3
handler (finding_location_bulk_update) rather than the non-V3 handler
(endpoint_status_bulk_update), causing the non-V3 tests to fail.

Fix by:
- Wrapping each test class with @skipUnless so it only runs when the
  matching URL handler is active
- Adding TestRemoveLocationsFromFindingView to exercise the V3 path
  (finding_location_bulk_update + LocationFindingReference) when
  V3_FEATURE_LOCATIONS=True
@valentijnscholten valentijnscholten marked this pull request as ready for review March 8, 2026 10:30
@dryrunsecurity
Copy link

dryrunsecurity bot commented Mar 8, 2026

DryRun Security

🟡 Please give this pull request extra attention during review.

This pull request introduces a DOM-based XSS risk: JavaScript builds HTML strings by concatenating checkbox element IDs into jQuery-created inputs (e.g., $('')) without escaping or validation, allowing a maliciously controlled id to break out of the attribute and inject arbitrary HTML/JS. The vulnerable code is in dojo/templates/dojo/view_finding.html (lines ~1480–1483) and should use safe DOM APIs or properly escape/validate IDs before insertion.

🟡 Potential Cross-Site Scripting in dojo/templates/dojo/view_finding.html (drs_d4ab1456)
Vulnerability Potential Cross-Site Scripting
Description The JavaScript concatenates checkbox element IDs directly into HTML strings used to create hidden input elements: $(''). If an attacker can control an element's id, they can inject characters that break out of the attribute context and add arbitrary HTML/JS, leading to DOM-based XSS. The code does not escape or otherwise validate the id before concatenation and uses innerHTML-like construction via jQuery HTML parser instead of safe DOM methods.

var hidden_input = $('<input type="hidden" value="' + this.id + '" name="endpoints_to_update">');
$('form#bulk_change_form').append(hidden_input);
});
$('form#bulk_change_form').append($('<input type="hidden" name="remove_from_finding" value="1">'));


Comment to provide feedback on these findings.

Report false positive: @dryrunsecurity fp [FINDING ID] [FEEDBACK]
Report low-impact: @dryrunsecurity nit [FINDING ID] [FEEDBACK]

Example: @dryrunsecurity fp drs_90eda195 This code is not user-facing

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

@Maffooch Maffooch modified the milestones: 2.56.1, 2.56.2 Mar 9, 2026
Copy link
Contributor

@dogboat dogboat left a comment

Choose a reason for hiding this comment

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

Handy! This works, I just had some questions about the way the UI renders. Could be just my system tho.

</ul>
</div>
<div class="btn-group mr-2" role="group">
<button type="button" class="btn btn-sm btn-danger" data-toggle="tooltip" data-placement="bottom" title="Remove selected endpoints from this finding" aria-label="Remove selected endpoints from this finding">
Copy link
Contributor

Choose a reason for hiding this comment

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

This is what it looks like to me, on both Chrome and Firefox (MacOS):

Image

...which is weird, because you posted a screenshot that looks fantastic. Is it just me?

});
});

$('a.remove-from-finding').on('click', function(e) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Any chance we could have the click handler be on the entire button and not just the anchor? As it is now, I have to click the anchor content, which is a bit smaller than the button itself. When I was testing I hit the button but not the icon, which didn't trigger the action.

@valentijnscholten
Copy link
Member Author

The screenshot is old and now I have an even newer one where the trash button looks the same as in other places, red on a grey background. Click handler adjusted, can't believe I didn't do that myself as I hate buttons not being clickable.

image

From another place in dojo the trash icon:

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants