Skip to content

fix(nvd): repo from commit in refs different from CPE cache resulting in failed conversion#4770

Merged
jess-lowe merged 3 commits intogoogle:masterfrom
jess-lowe:fix/repos-different
Feb 9, 2026
Merged

fix(nvd): repo from commit in refs different from CPE cache resulting in failed conversion#4770
jess-lowe merged 3 commits intogoogle:masterfrom
jess-lowe:fix/repos-different

Conversation

@jess-lowe
Copy link
Contributor

CVE-2026-23522 has a different repository in its CPE cache than in the commit extracted from references, causing a weird bug where it considers it a failed conversion.

@jess-lowe jess-lowe requested a review from another-rex February 8, 2026 22:30
@jess-lowe jess-lowe changed the title fix(nvd: repo from commit in refs different from CPE cache resulting in failed conversion fix(nvd): repo from commit in refs different from CPE cache resulting in failed conversion Feb 8, 2026
Copy link
Contributor

@another-rex another-rex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a comment around

Comment on lines 54 to 73
break
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this is the best fix. What is the point of the first section where we check fixed commits against repos if we are going to do another loop without caring about the repo?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Can we fix the root cause of the repos being mismatched
  2. If 1 is inevitable, do we need to change the logic everywhere HasFixedCommits(repo) is used? I see it called in a few different places
  3. Either way I don't think we need both loops? We can just remove the first loop right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TBH I want to rewrite this whole thing to rely natively on the OSV schema fields instead and make the logic a lot clearer, but it's not a high priority rn.

Removed the first block.

}

v := vulns.FromNVDCVE(cve.ID, cve)
versions := cves.ExtractVersionInfo(cve, nil, http.DefaultClient, metrics)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: can we rename this to affectedVersionsAndCommits. I was quite confused for a bit, had to look at the definitions to understand what this actually contained.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it's confusing cause it's referencing the VersionInfo struct (which consists of AffectedCommits and AffectedVersions). I can refactor to rename this but eventually I do want to move completely off the VersionInfo struct

@jess-lowe jess-lowe merged commit 5790b6f into google:master Feb 9, 2026
19 checks passed
@jess-lowe jess-lowe deleted the fix/repos-different branch February 9, 2026 00:03
jess-lowe added a commit that referenced this pull request Feb 9, 2026
Actual root cause of what #4770 was trying to solve is actually linkrot.
This may involve more requests but also less failures due to linkrot.

~Also refactored FindRepos to do less repetitive work.~ if I make it
less repetitive, vp cache doesn't work ):
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants