Repository navigation
fix(scan): collapse advisory records OSV marks as the same vulnerability - #1320
Open
sonukapoor wants to merge 2 commits into
Open
sonukapoor wants to merge 2 commits into
sonukapoor wants to merge 2 commits into
Conversation
Two databases can describe one flaw, and OSV records that by having each entry name the other in aliases. We read OSV faithfully, so both arrive and one vulnerability is counted twice on the same package. Real cases: two GHSAs for a lodash prototype pollution, and an EEF and a GHSA record for phoenix, which crosses two databases. The test is mutual naming and nothing weaker. OSV aliasing is not always symmetric, so a one-directional reference can mean related rather than same, and a shared CVE is weaker still since two advisories may legitimately cite one CVE. The survivor keeps the higher severity so maxSeverity cannot change, ties break on the smaller id so output is stable, and the absorbed id is kept in the survivor's aliases. That last part matters: the accuracy sweep compares our advisory ids against OSV's, and losing an id reads as a missed advisory rather than a de-duplicated one. Measured: Max's tree 52 advisory entries to 51, CopilotKit 333 to 332, findings and severity distribution identical in both.
…dead Eight cases. Mutation testing found two problems in the implementation rather than in the tests, which is what it is for. Accepting a one-directional alias, keeping the lower severity, and making the tie-break non-deterministic each fail only the case named for them. Two mutations survived the first pass. Removing `ids.add(droppedId)` changed nothing, because mutual naming is the collapse condition, so the survivor already lists every absorbed id and that line could never add anything. It is gone. Dropping the malformed-alias filter also changed nothing, because the test only asserted that it did not throw, and nothing here calls a string method on an alias. The filter does earn its place for a different reason: the survivor's aliases is typed string[] and is serialised into the JSON and SARIF payloads, so a null reaching it would ship. The test now asserts the output is clean, and both mutations fail against it.
This branch has not been deployed
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.
OSV can serve the same vulnerability as two advisory records that name each other as aliases, and we counted both, so one vulnerability appeared twice inside a finding.
Records are now collapsed only where the aliasing is mutual, so a one-way alias is left alone. The survivor keeps the higher severity, ties break on the smaller id, and the absorbed record's other aliases carry across so nothing leaves the id set.
Mutation testing on the tests found a line in the first draft that nothing could observe, and it is gone rather than left in place with a test written around it.
⚠ Worth knowing before the next accuracy sweep: the sweep must compare against the union of
idandaliasesrather than readingidalone. A collapsed pair otherwise reads as a disagreement that is not one, which is exactly what happened the first time this ran.Closes #1318