refactor(dedupe): one definition of the location prefetch - #15520
refactor(dedupe): one definition of the location prefetch#15520valentijnscholten wants to merge 2 commits into
Conversation
69c0bf5 to
a3b76c1
Compare
|
This pull request contains a critical finding where the sensitive file 'dojo/finding/deduplication.py' was modified by an author not on the allowed list. Although the issue is marked as failing, it is not currently blocking the merge.
🔴 Configured Sensitive Codepath Modified by Non-Allowed Author in
|
| Vulnerability | Configured Sensitive Codepath Modified by Non-Allowed Author |
|---|---|
| Description | File 'dojo/finding/deduplication.py' matches configured sensitive codepath pattern 'dojo/finding/*.py' and was modified by '' (commit c81e08e) 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 pull request has conflicts, please resolve those before we can evaluate the pull request. |
261e420 to
d334b6e
Compare
|
Conflicts have been resolved. A maintainer will review the pull request shortly. |
Which location relation to prefetch is a property of what Finding.get_locations() reads, not of any individual caller, but it was spelled out separately at every call site. That drifts, in both directions: the hash-recompute paths disagreed about it for months, and the batch dedupe loader prefetched the deprecated endpoints relation, which raises under V3_FEATURE_LOCATIONS on an instance migrated from endpoints. Adds location_prefetch_lookups() to dojo/location/queries.py, alongside vulnerability_id_prefetch, and routes build_candidate_scope_queryset and the dedupe command through it. The command's V3 branch prefetched "locations", one hop, where get_locations() reads location.url; it now prefetches the whole path like the other call sites, and its two near-identical select_related/prefetch_related blocks collapse into one. get_finding_models_for_deduplication is left alone deliberately: DefectDojo#15508 fixes the crash there and re-baselines the perf query counts it shifts. Measured that change independently here -- +2 queries on each V3 step, and -104 on the second import -- which matches the numbers in that PR. A TODO marks the one-line swap to the helper once it lands. The prefix argument exists for callers that page a model reaching the finding through a relation rather than paging Finding itself.
… a fork Distributions that store extra hash fields or need extra scoping have maintained near-verbatim copies of this command, and a copy is what lets the two drift. The vulnerability-id prefetch was fixed in a copy first and had to be fixed again elsewhere months later; the location prefetch was fixed elsewhere and never reached a copy, leaving it prefetching the deprecated endpoint relation that raises under V3_FEATURE_LOCATIONS. Such a distribution can now subclass this command and override a hook instead. The hooks are deliberately narrow -- extra arguments, extra scope and its description, the hash generator, an extra recompute pass, the two batch-dedupe entry points, and product grading -- so what an edition adds stays visible and a stale override is a signature mismatch rather than silent drift. Behaviour is unchanged: every default does what the code did before. The scope build is also unified, replacing the parser/no-parser if/else with one queryset that filters down, which is what makes an extra scope filter composable. unittests/test_dedupe_command_hooks.py covers both halves of the contract: every hook has a working default, and a subclass's hooks are actually reached by the run -- including that --dedupe_only skips the extra hash pass and that both batch paths and grading go through their hooks. This command had no tests at all before.
d334b6e to
b4c01bc
Compare
CI note: the remaining red is a dev-level failure, not this PRAfter rebasing onto current dev, the only failing check is:
This isn't caused by this PR:
Everything else is green (44 checks). This PR should go green once the dev-level webhooks console error is resolved. Happy to chase that root cause separately if useful, but it's out of scope for this dedupe refactor. |
Summary
Finding.get_locations()reads, not of any individual caller — but it was spelled out separately at every call site, and that drifts in both directions. The hash-recompute paths disagreed about it for months, and the batch dedupe loader prefetched the deprecatedendpointsrelation, which raises underV3_FEATURE_LOCATIONSon an instance migrated from endpoints.location_prefetch_lookups(prefix="")todojo/location/queries.py, next tovulnerability_id_prefetch, and routesbuild_candidate_scope_querysetandmanage.py dedupethrough it.locations— one hop — whereget_locations()readslocation.url, leaving a query per location reference. It now prefetches the full path like the other call sites, and its two near-identicalselect_related/prefetch_relatedblocks collapse into one.get_finding_models_for_deduplicationis deliberately left alone: fix(dedupe): prefetch locations, not endpoints, in the batch dedupe loader #15508 fixes the crash there and re-baselines the perf query counts that change shifts. I measured that change independently while scoping this PR — +2 queries on each V3 step, and −104 on the second import — which matches fix(dedupe): prefetch locations, not endpoints, in the batch dedupe loader #15508's numbers, so re-deriving them here would only create a conflict. ATODOmarks the one-line swap to the helper once it lands.unittests/test_dedupe_location_prefetch.pypins the helper (both flag states, the fulllocation.urlpath, and the relation-prefix form) and asserts bothbuild_candidate_scope_querysetmodes use it. Queryset shape only — no fixtures, no timing. 6 tests pass, andtest_deduplication_logicstill passes at 85.prefixargument exists for callers that page a model reaching the finding through a relation rather than pagingFindingitself.Extension points instead of a fork (second commit)
if/else, which is what makes an extra scope filter composable.unittests/test_dedupe_command_hooks.pycovers both halves: every hook has a working default, and a subclass's hooks are actually reached — including that--dedupe_onlyskips the extra hash pass, and that both batch paths and grading go through their hooks. This command had no tests at all before. 10 tests, verified locally.