backport(importers): make bulk_new_finding_writes functional on 3.2.x - #15770
Open
devGregA wants to merge 5 commits into
Open
backport(importers): make bulk_new_finding_writes functional on 3.2.x#15770devGregA wants to merge 5 commits into
devGregA wants to merge 5 commits into
Conversation
…written (DefectDojo#15597) LocationManager accumulated finding locations in a dict keyed by the Finding itself. Django's Model.__hash__ raises on an instance with no primary key ("Model instances without primary key value are unhashable"), so recording a location for a finding that has not been saved yet was a TypeError. Nothing about persisting locations requires the finding to be hashable. Both consumers in _persist_locations() simply iterate, and they run inside persist(), by which point the findings are written. The dict was only ever there to coalesce repeated records for one finding -- record_for_finding() records twice, once for unsaved_locations and once for extras. So the accumulator becomes a list of (finding, locations) pairs plus a slot index keyed on object identity. That is the shape the tag accumulator threaded through finding_post_processing() already uses. id() is safe here specifically because the pair list holds a strong reference to every finding recorded, so no entry can be collected and have its id reused while the accumulator is live. Identity keying is also more correct than the dict was: two distinct unsaved findings compare equal under Model.__eq__ (both have pk None), so an equality-keyed accumulator would have merged their locations. This unblocks an importer that buffers inserts and writes them in bulk at a batch boundary -- it records a finding's locations while the finding is still unsaved. The same accommodation get_original_findings, get_reimport_match_candidates_for_batch and _flush_post_processing_batch already provide, for the same reason. Tests: five in unittests/test_bulk_locations.py covering recording for an unsaved finding, coalescing repeats, keeping two distinct unsaved findings apart, the record-then-write-then-persist sequence end to end, and that both accumulators clear together so a stale slot cannot outlive its list. Falsifiability checked by reverting the source with the tests in place: all five fail with the TypeError above, and pass with it. Run natively (Django runner, PostgreSQL): test_bulk_locations, test_location_models, test_merge_findings_locations, test_code_location_emission, test_report_location_finding_scoping, test_importers_closeold, test_importers_deduplication, test_reimport_batch_flush -- 131 OK. Plus test_import_reimport and test_importers_performance -- 143 OK, with the pinned query counts unchanged. ruff 0.16.1 (the requirements-lint.txt pin) clean. Co-authored-by: devGregA <greg-agent-2@defectdojo.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> (cherry picked from commit 6c5e7a3)
…ing is written (DefectDojo#15620) Vulnerability ids and CWEs are already buffered and flushed at the batch boundary rather than written per finding. Reconciling them, though, read the finding's existing rows back through a reverse relation, and Django refuses that on an instance with no primary key: ValueError: 'Finding' instance needs to have a primary key value before this relationship can be used. Both reads are unconditional -- reconcile_cwes() reads finding_cwe_set, and reconcile_vulnerability_ids() reads through finding_vulnerability_id_strings() before it compares anything -- so every finding hit them. An importer that buffers the finding inserts themselves, to write them in bulk at that same batch boundary, therefore could not reconcile at all. For a finding with no row there is nothing to read: no persisted CWEs, no persisted vulnerability ids, and nothing to delete. Treating the existing set as empty is exact rather than a workaround -- every parsed value is new, which is what those comparisons would conclude from an empty row set anyway. Three guards, all no-ops on today's paths because the finding is always saved before finding_post_processing() runs: - reconcile_cwes(): skip the finding_cwe_set read when there is no pk, and do not append None to pending_cwe_deletes -- a finding with no row has no CWE rows to delete, and the None would land in the filter flush_vulnerability_ids() builds. - reconcile_vulnerability_ids(): skip the finding_vulnerability_id_strings() read when there is no pk. - finding_post_processing(): the trailing save on a changed cve exists to push that change onto an existing row. A buffered finding has no row to update, and saving there would insert it early, splitting one batched INSERT into per-finding ones. The value rides along when the buffer flushes. This is the same accommodation get_original_findings, get_reimport_match_candidates_for_batch and _flush_post_processing_batch already provide so downstream editions can override behaviour without copying the full process_findings() implementation. Tests: four in unittests/test_importers_deleted_finding_child_rows.py, alongside the existing buffered-child-row coverage -- reconciling vulnerability ids on an unsaved finding, the record-then-write-then-flush sequence writing the reference correctly, reconciling CWEs while queueing no delete, and a changed cve not writing a buffered finding. Falsifiability checked per guard rather than in aggregate. Reverting both reconcile guards makes all four error with the ValueError above. Reverting ONLY the cve save guard, with the other two in place, makes the buffered finding get written (`103 is not None`) -- so each guard is independently necessary. Run natively (Django runner, PostgreSQL): test_importers_deleted_finding_child_rows, test_importers_importer, test_import_reimport, test_importers_closeold, test_importers_deduplication, test_finding_cwe, test_vulnerability_id, test_vulnerability_id_type, test_importers_performance, test_bulk_locations -- 343 OK, with the pinned query counts unchanged. ruff 0.16.1 clean. Co-authored-by: devGregA <greg-agent-2@defectdojo.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> (cherry picked from commit e546fc9)
…ectDojo#15621) process_finding_that_was_not_matched() writes each unmatched finding inline with save_no_options(). A downstream edition that wants to buffer those inserts and write them in bulk at the batch boundary has no way to do that today: the call is in the middle of a ~35-line method, so overriding means copying the whole body, which then drifts from upstream on every change to it. Extracts the single call into persist_new_finding(), which defaults to exactly what it replaces. Behaviour is unchanged -- one method call where there was one method call. This is the same accommodation get_original_findings and get_reimport_match_candidates_for_batch already provide, and the docstring of the former says so explicitly: This is intentionally a separate method (like get_reimport_match_candidates_for_batch) so downstream editions can override it without copying the full process_findings() implementation The seam is placed deliberately: everything the caller does after it -- finding groups, the new_items list, request/response pairs -- is already safe on a finding that has not been written, and DefectDojo#15620 made the reconcile paths safe too. So overriding this one method, plus _flush_post_processing_batch to write the buffer before super(), is sufficient to defer the write. The docstring records that contract so it is not re-derived. Tests: unittests/test_reimporter_persist_seam.py drives a real BufferingReImporter -- an actual downstream edition written out in full rather than a mock -- through a genuine Acunetix reimport. It pins that the default still writes inline, that an override can defer to the batch boundary and drains its buffer, that deferring produces identical findings to the stock path, and that child rows still land (which is what breaks if the buffer flushes after the block that needs primary keys rather than before it). That test is also what proved DefectDojo#15620 was a prerequisite rather than a nicety: against a dev without those guards it errors in reconcile_cwes on finding_cwe_set.all(); with them it passes. Run natively (Django runner, PostgreSQL): test_reimporter_persist_seam, test_importers_deleted_finding_child_rows, test_reimport_batch_flush, test_import_reimport, test_importers_closeold, test_importers_deduplication, test_importers_performance -- 216 OK, pinned query counts unchanged. ruff 0.16.1 clean. Co-authored-by: devGregA <greg-agent-2@defectdojo.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> (cherry picked from commit bc3be8d)
…sisted_fields save() normalizes and derives a finding's own columns inline: title casing and truncation, blank-component normalization, the date default, numerical severity, CVSS vector parsing, and the same-tool hash. A batched writer using bulk_create bypasses save() and its signals entirely, so it has no way to produce rows identical to the ones save() produces short of reimplementing that transform. Extracts it into derive_persisted_fields(), which save() now calls. Behaviour is unchanged: the body is moved verbatim, and the new-finding static/dynamic branch moves with it because it reads only file_path and the parser's in-memory locations/endpoints. The existing-finding branch stays in save(), since it queries self.locations/self.endpoints and needs a primary key. Also adds Finding.persisted_title() as the single source of truth for the title transform, replacing the hardcoded titlecase(title[:511]). It reads max_length off the field rather than repeating the literal, and tolerates a None title. This matters because titlecase() is not only a case change: it collapses consecutive newlines and turns tabs into spaces, so a caller that truncated without titlecasing diverged for any multi-line title. Backport of the equivalent change on dev, ported by hand rather than cherry-picked: on dev this arrived inside a large model-split commit whose diff also carries unrelated index removals (DefectDojo#15659) and hashing changes (DefectDojo#15513, DefectDojo#15588), none of which belong on a patch line. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
process_findings() accumulates per-batch work and flushes it inside the per-finding loop, gated on `len(batch_finding_ids) >= dedupe_batch_max_size or is_final`. The matched branch ends in `continue`. That skip is what makes the tail fragile: the flush sits after it in the loop body, so a report whose LAST finding took the matched branch never reached the is_final flush, and everything appended since the previous size-triggered flush was dropped. Silently, because nothing raises. Those findings got no deduplication, no rules, no issue updater and no JIRA dispatch, and none of their parser or inherited tags; pending location status updates, vulnerability ids and burp request/response pairs were discarded with them. A reimport whose final finding matches an existing one is the ordinary case, not an edge case, so this fires on unchanged re-syncs rather than on unusual reports. Extracts the flush into _flush_post_processing_batch() and calls it once after the loop instead of relying on a flag inside it. The final call runs however the last iteration ended, and is deliberately unconditional: matched findings can accumulate location status updates without appending anything to dispatch, and every step is already a no-op on empty state (close_old_findings calls persist() the same way). The in-loop call keeps the size trigger only. is_final and is_final_batch are now unused and removed, which also leaves the loop's enumerate() index unused, so the loop iterates the batch directly. Tests: unittests/test_reimport_final_drain.py reimports a report whose matches all take the force_continue path, so the final iteration is guaranteed to be the skipping one, and asserts the batch was still flushed. It fails on the code before this change with exactly the dropped-work assertion and passes after. test_reimport_prefetch and test_import_reimport were compared before and after and are unchanged (both carry pre-existing failures on this branch that this change neither causes nor fixes). ruff 0.16.0 clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
This pull request contains two critical findings where sensitive codepaths in
🔴 Configured Sensitive Codepath Modified by Non-Allowed Author in
|
| Vulnerability | Configured Sensitive Codepath Modified by Non-Allowed Author |
|---|---|
| Description | File 'dojo/finding/models.py' matches configured sensitive codepath pattern 'dojo/finding/*.py' and was modified by '' (commit c6209d9) who is not in the allowed authors list. |
🔴 Configured Sensitive Codepath Modified by Non-Allowed Author in dojo/importers/default_reimporter.py (drs_bec76e0a)
| Vulnerability | Configured Sensitive Codepath Modified by Non-Allowed Author |
|---|---|
| Description | File 'dojo/importers/default_reimporter.py' matches configured sensitive codepath pattern 'dojo/importers/*.py' and was modified by '' (commit 0097c65) who is not in the allowed authors list. |
We've notified @mtesauro.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Makes the Pro feature flag bulk_new_finding_writes actually functional on the 3.2.x line. It ships in 3.2.200 and is currently inert there: the Pro side defines persist_new_finding(), but the OSS seam that calls it only exists on dev, so nothing ever invokes the override. Enabling the flag on 3.2.x today changes nothing.
Please read the note on commit 4 before reviewing. This is not a uniformly low-risk backport.
What this brings across, all already on dev:
refactor(locations): let locations be recorded before the finding is written #15597 - let locations be recorded before the finding is written. The accumulator keyed _locations_by_finding by the Finding object, and an unsaved finding is unhashable, so it becomes a list of (finding, locations) with an id()-keyed slot index. Conflicted on this line and was resolved by hand; only the one test class that refactor(locations): let locations be recorded before the finding is written #15597 itself added was taken, since the surrounding dev tests depend on APIs (cleaned_unsaved_locations) that are not on this line.
refactor(importers): reconcile a finding's child rows before the finding is written #15620 - reconcile a finding's child rows before the finding is written. Three finding.pk guards so CWE and vulnerability-id reconciliation treat an unwritten finding as having no persisted rows rather than raising, and so a cve change does not trigger an early single-row insert.
refactor(reimporter): a seam for deferring the new-finding write #15621 - the persist_new_finding() seam itself. One method call where there was one method call.
derive_persisted_fields + persisted_title. THIS IS THE ONE TO SCRUTINISE. It extracts the column-derivation half of Finding.save() into a reusable method so a bulk writer can produce rows identical to what save() produces. The body is moved verbatim and the new-finding static/dynamic branch moves with it, because it reads only file_path and in-memory parser state; the existing-finding branch stays in save() since it queries relations and needs a primary key. Finding.save() is the hottest write path in the product and this is a patch line, so it deserves a closer read than the other three.
Ported by hand rather than cherry-picked: on dev this arrived inside a large model-split commit whose diff also carries unrelated index removals (#15659) and hashing changes (#15513, #15588), none of which belong here.
Verification
The point of the exercise, so it was checked rather than assumed. Before this PR, the Pro flag-on suite (unit_tests/connectors/test_bulk_new_finding_writer.py, 21 tests) passed while never executing the bulk path at all: a raise placed at the top of the flag resolver changed nothing, because the OSS seam that reaches it does not exist on this line. Those tests were passing vacuously.
With this PR the same 21 tests pass AND the same mutation now fails all 21 with the injected error, which is what proves the path is live and the tests are real gates.
ruff 0.16.0 (the version pinned in requirements-lint.txt) reports no findings across the branch.