Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fillup missing vulnerabilities summary #1767

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

TG1999
Copy link
Contributor

@TG1999 TG1999 commented Jan 29, 2025

Reference: #859

Signed-off-by: Tushar Goel <tushar.goel.dav@gmail.com>
@TG1999 TG1999 force-pushed the missing_summaries_pipeline branch from 9557b88 to f4140cd Compare February 11, 2025 14:09
Copy link
Member

@pombredanne pombredanne left a comment

Choose a reason for hiding this comment

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

Here are some feedback.

created_by="nvd_importer", summary__isnull=False
).exclude(summary="")
self.log(
f"Found {nvd_importer_advisories.count()} advisories from NVD importer",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
f"Found {nvd_importer_advisories.count()} advisories from NVD importer",
f"Found {nvd_importer_advisories.count()} advisories with summaries from NVD importer",

matching_advisories = nvd_importer_advisories.filter(Q(aliases__contains=alias))

if matching_advisories.exists():
# Take the first matching advisory with a summary
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# Take the first matching advisory with a summary
# Take the first matching advisory with a summary.
# NVD advisories only have one alias, always a CVE

if matching_advisories.exists():
# Take the first matching advisory with a summary
best_advisory = matching_advisories.first()
vulnerability.summary = best_advisory.summary
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
vulnerability.summary = best_advisory.summary
# Note: we filtered above to only get non-empty summaries
vulnerability.summary = best_advisory.summary


if matching_advisories.exists():
# Take the first matching advisory with a summary
best_advisory = matching_advisories.first()
Copy link
Member

Choose a reason for hiding this comment

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

We should take the latest NVD advisory with the most up-to-date summary, and not the oldest

Signed-off-by: Tushar Goel <tushar.goel.dav@gmail.com>
Copy link
Member

@keshav-space keshav-space left a comment

Choose a reason for hiding this comment

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

Thanks @TG1999, few nits for your consideration.

level=logging.INFO,
)

for vulnerability in vulnerabilities_qs.paginated():
Copy link
Member

Choose a reason for hiding this comment

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

We should use LoopProgress here

Suggested change
for vulnerability in vulnerabilities_qs.paginated():
progress = LoopProgress(total_iterations=vulnerabilities_qs.count(), logger=self.log)
for vulnerability in progress.iter(vulnerabilities_qs.paginated()):

for vulnerability in vulnerabilities_qs.paginated():
aliases = vulnerability.aliases.values_list("alias", flat=True)
# get alias that start with CVE- with filter
alias = aliases.filter(alias__startswith="CVE-").first()
Copy link
Member

Choose a reason for hiding this comment

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

We are prefetching aliases with vulnerabilities_qs = Vulnerability.objects.filter(summary="").prefetch_related("aliases"), but that will be rendered useless since this will trigger fresh database query.

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.

3 participants