fix(importers): skip child rows buffered for a finding deleted mid-batch - #15428
Conversation
Vulnerability ids and Burp request/response pairs are not written as each
finding is processed. They are buffered and bulk-inserted at the batch
boundary, so a finding stays referenced by the buffer for as long as the rest
of its batch (up to a thousand findings) takes to process. A finding row can
go away inside that window -- the excess-duplicate cleanup task deletes
findings, and so does a user deleting findings or a delete cascading from a
test or engagement -- leaving a buffered row pointing at a primary key that is
no longer in dojo_finding.
Because Django declares its foreign keys DEFERRABLE INITIALLY DEFERRED, that
insert does not fail where it is issued. PostgreSQL raises it at COMMIT, past
every handler that knew what the import was doing, so the reimport dies with
an IntegrityError naming a database constraint:
insert or update on table "dojo_vulnerability_id" violates foreign key
constraint "dojo_vulnerability_r_finding_id_c9d59919_fk_dojo_find"
DETAIL: Key (finding_id)=(...) is not present in table "dojo_finding".
and takes the rest of the batch -- findings that were perfectly fine -- with
it.
A finding that is gone has nothing to own these rows, so both flushes now drop
just the rows whose finding no longer exists and log which ones, instead of
failing the whole import. The check is one indexed primary-key lookup per
flush that has something to insert, so a no-change reimport (nothing buffered)
is unaffected; close_old_findings already guards its candidates the same way.
Perf baselines move +1 on the paths that buffer child rows and are unchanged
on the no-change reimport.
|
This pull request contains a critical finding where the file 'dojo/importers/base_importer.py' was modified by an unauthorized author, 'claude'.
🔴 Configured Sensitive Codepath Modified by Non-Allowed Author in
|
| Vulnerability | Configured Sensitive Codepath Modified by Non-Allowed Author |
|---|---|
| Description | File 'dojo/importers/base_importer.py' matches configured sensitive codepath pattern 'dojo/importers/*.py' and was modified by 'claude' (commit 1ae58d7) 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.
Conflicts were the pinned query-count baselines only, both sides raising the same assertions: #15427's write-back guard (+3 on every path) against this branch's buffered-child-row guard (+1 on the paths that buffer, unchanged on the no-change reimport). The two guards are independent constant-cost lookups, so each conflicting baseline takes bugfix's value plus this branch's delta, and both explanatory comments are kept in test_tag_inheritance_perf.py. CI is the authority on the resulting numbers.
CI failure on
|
4 participants
Description
An async reimport fails with a
500/ failed task and thisIntegrityErrorwhenever a finding it has already processed is deleted before the batch it belongs to is flushed:Reported twice within four seconds from a production instance on
reimport-scan(Generic Findings Import), against two different tests of the same product — the shape of a delete racing a batch of reimports rather than a one-off.The sequence:
store_vulnerability_ids()(import) andreconcile_vulnerability_ids()(reimport) appendVulnerability_Idobjects toself.pending_vulnerability_ids, andflush_vulnerability_ids()bulk-inserts them at the batch boundary — up to a thousand findings later.async_dupe_deletedeletes excess duplicates whendelete_duplicatesis enabled, and a user deleting findings or a delete cascading from a test or engagement does the same.finding_idthat is no longer indojo_finding.Because Django declares its foreign keys
DEFERRABLE INITIALLY DEFERRED, that insert does not fail where it is issued — PostgreSQL raises it atCOMMIT. That is why the reported traceback contains no application frames at all, only the commit path: the failure surfaces past every handler that still knew what the import was doing. The whole import dies, taking the rest of the batch — findings that were perfectly fine — with it.BurpRawRequestResponseis buffered and flushed through exactly the same pattern (process_request_response_pairs→flush_burp_request_response) and has the same exposure, so it is fixed alongside rather than left as the next incident.The change
Both flushes now filter the buffer through a new
BaseImporter.drop_rows_for_deleted_findings(), which keeps only the rows whose finding still exists and logs at warning level which finding ids were dropped and how many rows went with them.Dropping is the correct outcome rather than a suppression: a finding that has been deleted has nothing to own these child rows. This is the same posture
close_old_findings()already takes —_sync_close_old_finding_status_fields()returns only the candidates that still exist, precisely because a candidate can be deleted mid-reimport (#15408). This PR extends that guard to the other place in the same run that holds finding references across a batch.Deliberately out of scope: why a finding may be deleted mid-run (that is the caller's workflow), and the
pending_vuln_id_deleteshalf of the flush, which is aDELETE ... WHERE finding_id IN (...)and is unaffected by a missing row.Performance impact
One indexed primary-key
SELECTper flush that has something to insert, independent of finding count. Measured across the pinned baselines, every delta is exactly +1, and the no-change reimport — where nothing is buffered, so no lookup happens — is unchanged. That split is itself the evidence the cost is O(1) per flush and not O(findings):EXPECTED_ZAP_IMPORT_V2/_V3EXPECTED_ZAP_REIMPORT_WITH_NEW_V2/_V3EXPECTED_ZAP_REIMPORT_NO_CHANGE_V2/_V3In
test_importers_performance.py,expected_num_queries1andexpected_num_queries2move +1 in all 11 affected cases;expected_num_queries3(no-change reimport) andexpected_num_queries4are untouched. Async task counts are unchanged.Impact on the Pro plugin
Checked as part of the acceptance criteria.
ProImporter/ProReImporterderive fromDefaultImporter/DefaultReImporterand override neitherflush_vulnerability_ids,flush_burp_request_response,store_vulnerability_idsnorreconcile_vulnerability_ids, so they inherit the guard — which is what matters here, because the reported traceback runs through the Pro async importer into this file. No Pro module bulk-createsVulnerability_Idon the import path, so there is no second copy of this defect to fix there.test_importers_performance.pywith its ownexpected_num_queries*values, which will shift by the same +1 on the buffering paths (and stay unchanged on the no-change reimport) once this merges. Those numbers have to be measured in the Pro stack, not carried over from the table above, and this sandbox has no Docker so they could not be produced here. Flagging rather than guessing.pro/finding/helpers.pycreates a singleVulnerability_Idduring EPSS enrichment for a finding held in memory, which is the same class of exposure in a different task. Different code path, different repo, not touched here.Test results
New
unittests/test_importers_deleted_finding_child_rows.py, 6 tests. The 4 guard tests were confirmed failing onbugfixbefore the change, each with the dangling-reference violation this PR is about:Each test calls
connection.check_constraints()after the flush — the check PostgreSQL runs atCOMMITin production. The test transaction never commits, so without asking for it explicitly the production failure would not appear in a test at all.Coverage: the import-path flush with a finding deleted after buffering; the reported reimport path through
reconcile_vulnerability_ids; that a dropped row is not retried by the next batch's flush; the request/response buffer; and two control cases asserting a run where nothing was deleted still writes every row.Run against PostgreSQL on Python 3.13 (the deferred-constraint behaviour is backend-specific):
unittests.test_importers_deleted_finding_child_rows— 6 passedunittests.test_importers_performance,unittests.test_tag_inheritance_perf— 35 passed (26 count assertions failing on the un-updated baselines first, then green on the updated ones)unittests.test_import_reimport,test_importers_importer,test_importers_closeold,test_importers_deduplication,test_reimport_prefetch,test_import_execution_mode,test_duplication_loops— 283 passed, 1 skipped, 0 failuresruff check --config ruff.tomlwith the pinned 0.15.20 — clean on all four touched filesDocumentation
No documentation change: this restores the documented behaviour of import/reimport (the failure was never an expected outcome) and adds no setting, input, output, or API surface.
Checklist
bugfixbranch — based on and targetingbugfix.Reported internally:
🤖 Generated with Claude Code
https://claude.ai/code/session_01WrYDZdT84SC27Mz53w1Yuj
Generated by Claude Code