fix(findings): route every group id through one definition - #2359
Open
Subramaniyajothi6 wants to merge 2 commits into
Open
fix(findings): route every group id through one definition#2359Subramaniyajothi6 wants to merge 2 commits into
Subramaniyajothi6 wants to merge 2 commits into
Conversation
Closes utksh1#1834 Group identity was computed three different ways: generate_finding_key() plugin, asset, signature, owner_id normalize_and_correlate_findings() plugin, asset, signature build_finding_groups() fallback title, target The third is the one the issue reports: an uncorrelated finding grouped on title+target alone, so two genuinely distinct issues sharing a title on the same target merged into one — two different open ports both reported as "Open port", for instance. That path fires only when a finding carries neither finding_group_id nor id, i.e. was never persisted or correlated. compute_finding_group_id() is now the single definition and all three callers go through it. The correlate path keeps passing its already-computed asset_id, so the digest it writes is unchanged — this matters because finding_group_id is persisted under a unique index on (owner_id, finding_group_id) from migration 008, and changing the hash would orphan every stored row. TestPersistedGroupIdIsStable pins three known digests so a future edit cannot drift them without a migration. owner_id is dropped from the key rather than added to the others. Owner scoping already lives in the storage layer — that same unique index, and the ON CONFLICT (owner_id, finding_group_id) upsert — so hashing it in was redundant and made generate_finding_key unable to reproduce any id actually stored. generate_finding_key has no callers anywhere in the repo, so nothing observes the change; its parameter is kept for compatibility and documented as unused. Verified by mutation: reverting the fallback to title+target fails four tests, and folding owner_id back into the digest fails the pinned-digest test.
utksh1
requested changes
Aug 4, 2026
utksh1
left a comment
Owner
There was a problem hiding this comment.
Request changes: removing owner_id from the finding-group digest changes every persisted group key, despite the PR describing the digests as stable. Existing rows and cross-task correlation need a migration/backfill or an explicit compatibility strategy, plus collision tests for old and new data. Do not merge a hash-material change that silently orphanes existing group identities.
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
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.
Closes #1834
Problem
Group identity was computed three different ways:
generate_finding_key()plugin_id, asset_id, signature, owner_idnormalize_and_correlate_findings()plugin_id, asset_id, signaturebuild_finding_groups()fallbacktitle, targetThe third is the one the issue reports. An uncorrelated finding grouped on
title + targetalone, so two genuinely distinct issues sharing a title on the same target merged into one — two different open ports both reported as"Open port", for instance.Worth scoping precisely: that fallback fires only when a finding carries neither
finding_group_idnorid, i.e. was never persisted or correlated.build_finding_groupsprefers both of those first.Changes
compute_finding_group_id()is now the single definition of group identity —(plugin, asset, signature)— and all three producers route through it.The persisted digest is unchanged
normalize_and_correlate_findingskeeps passing its already-computedasset_id, so the id it writes is byte-identical. This matters:finding_group_idis persisted under a unique index on(owner_id, finding_group_id)(migration008), and the upsert keys on that same pair — changing the hash would orphan every existing row.TestPersistedGroupIdIsStablepins three known digests so a future edit cannot drift them without someone noticing a migration is needed.owner_idis dropped rather than added to the othersOwner scoping already lives in the storage layer — that same unique index, plus
ON CONFLICT (owner_id, finding_group_id)in the upsert. Folding the owner into the digest was therefore redundant, and it meantgenerate_finding_keycould never reproduce an id that was actually stored.generate_finding_keyhas no callers anywhere in the repo, so nothing observes the change. Itsowner_idparameter is kept for compatibility and documented as unused. Happy to remove the function outright instead if you would prefer — it is dead code either way.Verification
test_finding_intelligence_groups.py— 26 passedtesting/backend/unit— 2333 passed, 21 skipped, 0 failedruff check backend testing/backend— cleanMutation-checked: reverting the fallback to
title + targetfails four tests; foldingowner_idback into the digest fails the pinned-digest test.Behaviour, before → after:
Note
I was not able to run the integration suite locally on this machine. The change touches
build_finding_groups, whichroutes.pycalls in three places, so that CI job is the one worth watching.