launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #32967
[Merge] ~enriqueesanchz/launchpad:fix-soss-import-duplicated into launchpad:master
Enrique Sánchez has proposed merging ~enriqueesanchz/launchpad:fix-soss-import-duplicated into launchpad:master.
Commit message:
Fix SOSSImport ordering and exception handling
Create the vulnerability first, then the bug. This will allow us to only
create the bug if we successfully created the vulnerability.
Catch GitRepository exception to stop the job if there is a problem
related to the git repository.
Add valid_name() check to SOSSRecord as Security team wants to ignore
SOSSRecord.Packages that have invalid names.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~enriqueesanchz/launchpad/+git/launchpad/+merge/492457
This MP is a result of the svt data full import done in qastaging. We identified some problems and this will fix them.
--
Your team Launchpad code reviewers is requested to review the proposed merge of ~enriqueesanchz/launchpad:fix-soss-import-duplicated into launchpad:master.
diff --git a/lib/lp/bugs/model/importvulnerabilityjob.py b/lib/lp/bugs/model/importvulnerabilityjob.py
index 4ef6e08..5eb9dc6 100644
--- a/lib/lp/bugs/model/importvulnerabilityjob.py
+++ b/lib/lp/bugs/model/importvulnerabilityjob.py
@@ -27,7 +27,7 @@ from lp.bugs.model.vulnerabilityjob import (
)
from lp.bugs.scripts.soss.models import SOSSRecord
from lp.bugs.scripts.soss.sossimport import SOSSImporter
-from lp.code.errors import GitRepositoryScanFault
+from lp.code.errors import GitRepositoryBlobNotFound, GitRepositoryScanFault
from lp.code.interfaces.githosting import IGitHostingClient
from lp.code.interfaces.gitlookup import IGitLookup
from lp.services.config import config
@@ -245,6 +245,7 @@ class ImportVulnerabilityJob(VulnerabilityJobDerived):
)
continue
+ success = False
try:
logger.debug(f"[ImportVulnerabilityJob] Getting {file}")
blob = ref.getBlob(file)
@@ -257,11 +258,19 @@ class ImportVulnerabilityJob(VulnerabilityJobDerived):
bug, vulnerability = importer(record, cve_sequence)
if bug and vulnerability:
+ success = True
self.metadata["result"]["succeeded"].append(cve_sequence)
- else:
- self.metadata["result"]["failed"].append(cve_sequence)
+ except (GitRepositoryScanFault, GitRepositoryBlobNotFound):
+ # If there is a problem with git repository, we stop as we
+ # won't be able to fetch more files
+ raise VulnerabilityJobException(
+ f"Failed to get file from Git repository: {file}"
+ )
except Exception as e:
- self.notifyUserError(e)
+ self.notifyUserError(f"{cve_sequence}: {e}")
+ finally:
+ if not success:
+ self.metadata["result"]["failed"].append(cve_sequence)
def notifyUserError(self, error):
"""Calls up and also saves the error text in this job's metadata.
diff --git a/lib/lp/bugs/scripts/soss/models.py b/lib/lp/bugs/scripts/soss/models.py
index f8fe1a9..a87975c 100644
--- a/lib/lp/bugs/scripts/soss/models.py
+++ b/lib/lp/bugs/scripts/soss/models.py
@@ -9,6 +9,8 @@ from typing import Any, Dict, List, Optional
import yaml
from packaging.version import Version
+from lp.app.validators.name import valid_name
+
__all__ = [
"SOSSRecord",
]
@@ -149,6 +151,7 @@ class SOSSRecord:
note=package["Note"],
)
for package in pkgs
+ if valid_name(package["Name"])
]
packages[package_type] = package_list
diff --git a/lib/lp/bugs/scripts/soss/sossexport.py b/lib/lp/bugs/scripts/soss/sossexport.py
index 9a75eb6..a8659d5 100644
--- a/lib/lp/bugs/scripts/soss/sossexport.py
+++ b/lib/lp/bugs/scripts/soss/sossexport.py
@@ -5,7 +5,7 @@
import logging
from collections import defaultdict
from datetime import datetime
-from typing import Dict, List, Optional
+from typing import Dict, List, Optional, Union
from lp.app.enums import InformationType
from lp.bugs.model.bug import Bug as BugModel
@@ -116,7 +116,7 @@ class SOSSExporter:
public_date = self._normalize_date_without_timezone(
vulnerability.date_made_public
)
- notes = vulnerability.notes.split("\n") if vulnerability.notes else []
+ notes = self._format_notes(vulnerability.notes)
priority = SOSSRecord.PriorityEnum(
PRIORITY_ENUM_MAP_REVERSE[vulnerability.importance]
)
@@ -134,6 +134,24 @@ class SOSSExporter:
public_date=public_date,
)
+ def _format_notes(self, notes: str) -> List[Union[Dict, str]]:
+ """Return a list of dicts or strings using from the notes string. Each
+ dict contains the user and the note added.
+ """
+ if not notes:
+ return []
+
+ formatted_notes = []
+ for note in notes.split("\n\n"):
+ if len(note.split(":", maxsplit=1)) == 2:
+ key, value = note.split(":")
+ formatted_notes.append({key: value.strip()})
+ else:
+ # Fallback to simple string
+ formatted_notes.append(str(note))
+
+ return formatted_notes
+
def _validate_to_record_args(
self,
lp_cve: CveModel,
diff --git a/lib/lp/bugs/scripts/soss/sossimport.py b/lib/lp/bugs/scripts/soss/sossimport.py
index 9f59e92..6f85194 100644
--- a/lib/lp/bugs/scripts/soss/sossimport.py
+++ b/lib/lp/bugs/scripts/soss/sossimport.py
@@ -6,7 +6,7 @@ import logging
import os
from collections import defaultdict
from datetime import timezone
-from typing import Dict, List, Optional, Tuple
+from typing import Dict, List, Optional, Tuple, Union
import transaction
from zope.component import getUtility
@@ -122,30 +122,28 @@ class SOSSImporter:
self, soss_record: SOSSRecord, cve_sequence: str
) -> Tuple[BugModel, Vulnerability]:
"""Import CVE from SOSS record."""
- if not self._validate_soss_record(soss_record, cve_sequence):
- return None, None
-
+ self._validate_soss_record(soss_record, cve_sequence)
lp_cve = self._get_launchpad_cve(cve_sequence)
- if lp_cve is None:
- return None, None
-
- bug = self._find_existing_bug(soss_record, lp_cve, self.soss)
- if not bug:
- bug = self._create_bug(soss_record, lp_cve)
- else:
- bug = self._update_bug(bug, soss_record, lp_cve)
vulnerability = self._find_existing_vulnerability(lp_cve, self.soss)
if not vulnerability:
vulnerability = self._create_vulnerability(
- bug, soss_record, lp_cve, self.soss
+ soss_record, lp_cve, self.soss
)
else:
vulnerability = self._update_vulnerability(
vulnerability, soss_record
)
- if not self.dry_run:
+ bug = self._find_existing_bug(soss_record, lp_cve, self.soss)
+ if not bug:
+ bug = self._create_bug(soss_record, lp_cve)
+ else:
+ bug = self._update_bug(bug, soss_record, lp_cve)
+
+ vulnerability.linkBug(bug, check_permissions=False)
+
+ if not self.dry_run and vulnerability and bug:
transaction.commit()
logger.info(
"[SOSSImporter] Successfully committed changes for "
@@ -194,6 +192,9 @@ class SOSSImporter:
)
)
+ if not bug:
+ raise ValueError(f"Error creating bug for {lp_cve.sequence}")
+
# Create next bugtasks
self._create_or_update_bugtasks(bug, soss_record)
@@ -222,7 +223,6 @@ class SOSSImporter:
def _create_vulnerability(
self,
- bug: BugModel,
soss_record: SOSSRecord,
lp_cve: CveModel,
distribution: Distribution,
@@ -232,7 +232,6 @@ class SOSSImporter:
the given SOSSRecord instance and link to the specified Bug
and LP's Cve model.
- :param bug: Bug model associated with the vulnerability
:param soss_record: SOSSRecord with information from a SOSS cve
:param lp_cve: Launchpad Cve model
:param distribution: a Distribution affected by the vulnerability
@@ -243,11 +242,11 @@ class SOSSImporter:
distribution=distribution,
status=VulnerabilityStatus.NEEDS_TRIAGE,
importance=PRIORITY_ENUM_MAP[soss_record.priority],
- creator=bug.owner,
+ creator=self.bug_importer,
information_type=self.information_type,
cve=lp_cve,
description=soss_record.description,
- notes="\n".join(soss_record.notes),
+ notes=self._format_notes(soss_record.notes),
mitigation=None,
importance_explanation=soss_record.priority_reason,
date_made_public=self._normalize_date_with_timezone(
@@ -258,7 +257,12 @@ class SOSSImporter:
cvss=self._prepare_cvss_data(soss_record),
)
)
- vulnerability.linkBug(bug, bug.owner)
+
+ if not vulnerability:
+ raise ValueError(
+ f"[SOSSImporter] Error creating vulnerability for "
+ f"{lp_cve.sequence}"
+ )
logger.info(
"[SOSSImporter] Created vulnerability with ID: "
@@ -279,7 +283,7 @@ class SOSSImporter:
"""
vulnerability.status = VulnerabilityStatus.NEEDS_TRIAGE
vulnerability.description = soss_record.description
- vulnerability.notes = "\n".join(soss_record.notes)
+ vulnerability.notes = self._format_notes(soss_record.notes)
vulnerability.mitigation = None
vulnerability.importance = PRIORITY_ENUM_MAP[soss_record.priority]
vulnerability.importance_explanation = soss_record.priority_reason
@@ -322,9 +326,6 @@ class SOSSImporter:
self, lp_cve: CveModel, distribution: Distribution
) -> Optional[Vulnerability]:
"""Find existing vulnerability for the current distribution"""
- if not lp_cve:
- return None
-
vulnerability = lp_cve.getDistributionVulnerability(distribution)
return removeSecurityProxy(vulnerability)
@@ -407,6 +408,7 @@ class SOSSImporter:
cve_sequence,
cve_sequence,
)
+ raise NotFoundError(f"Could not find {cve_sequence} in LP")
return lp_cve
def _make_bug_description(self, soss_record: SOSSRecord) -> str:
@@ -473,7 +475,10 @@ class SOSSImporter:
soss_record.candidate,
cve_sequence,
)
- return False
+ raise ValueError(
+ f"CVE sequence mismatch: {soss_record.candidate} != "
+ f"{cve_sequence}"
+ )
if not soss_record.packages:
logger.warning(
@@ -482,7 +487,7 @@ class SOSSImporter:
cve_sequence,
cve_sequence,
)
- return False
+ raise ValueError(f"{cve_sequence}: has no affected packages")
return True
@@ -499,3 +504,17 @@ class SOSSImporter:
return SecurityAdminDistribution(self.soss).checkAuthenticated(
IPersonRoles(user)
)
+
+ def _format_notes(self, notes: List[Union[Dict, str]]) -> str:
+ """Return a string from a list of notes. Notes can contain dicts or
+ strings.
+ """
+ formatted_notes = []
+ for note in notes:
+ if isinstance(note, dict):
+ for key, value in note.items():
+ value = value.replace("\n", " ")
+ formatted_notes.append(f"{key}: {value}")
+ else:
+ formatted_notes.append(str(note))
+ return "\n\n".join(formatted_notes)
diff --git a/lib/lp/bugs/scripts/soss/tests/sampledata/CVE-2025-1979 b/lib/lp/bugs/scripts/soss/tests/sampledata/CVE-2025-1979
index a562efa..904ea32 100644
--- a/lib/lp/bugs/scripts/soss/tests/sampledata/CVE-2025-1979
+++ b/lib/lp/bugs/scripts/soss/tests/sampledata/CVE-2025-1979
@@ -5,7 +5,10 @@ References:
- https://security.snyk.io/vuln/SNYK-PYTHON-RAY-8745212
- https://ubuntu.com/security/notices/SSN-148-1.json?show_hidden=true
Notes:
-- This is a sample soss cve with all the fields filled for testing
+- username: since 1.0, a package issues a warning when text() is omitted this fix
+ is not important, marking priority as low
+- username: since 1.0, a package issues a warning when text() is omitted this fix
+ is not important, marking priority as low
- sample note 2
Priority: Low
Priority-Reason: 'Unrealistic exploitation scenario. Logs are stored locally and not
diff --git a/lib/lp/bugs/scripts/soss/tests/test_sossimport.py b/lib/lp/bugs/scripts/soss/tests/test_sossimport.py
index ef21825..98b31e2 100644
--- a/lib/lp/bugs/scripts/soss/tests/test_sossimport.py
+++ b/lib/lp/bugs/scripts/soss/tests/test_sossimport.py
@@ -5,6 +5,7 @@ import transaction
from zope.component import getUtility
from lp.app.enums import InformationType
+from lp.app.errors import NotFoundError
from lp.app.interfaces.launchpad import ILaunchpadCelebrities
from lp.bugs.interfaces.bugtask import BugTaskImportance, BugTaskStatus
from lp.bugs.scripts.soss import SOSSRecord
@@ -163,8 +164,11 @@ class TestSOSSImporter(TestCaseWithFactory):
)
self.notes = (
- "This is a sample soss cve with all the fields filled for "
- "testing\nsample note 2"
+ "username: since 1.0, a package issues a warning when text() is "
+ "omitted this fix is not important, marking priority as low\n\n"
+ "username: since 1.0, a package issues a warning when text() is "
+ "omitted this fix is not important, marking priority as low\n\n"
+ "sample note 2"
)
def _check_bugtasks(
@@ -219,10 +223,7 @@ class TestSOSSImporter(TestCaseWithFactory):
self.importance_explanation,
)
self.assertEqual(vulnerability.creator, self.bug_importer)
- self.assertEqual(
- vulnerability.notes,
- self.notes,
- )
+ self.assertEqual(vulnerability.notes, self.notes)
self.assertEqual(vulnerability.mitigation, None)
self.assertEqual(vulnerability.cve, self.cve)
@@ -299,8 +300,9 @@ class TestSOSSImporter(TestCaseWithFactory):
soss_importer = SOSSImporter()
bug = soss_importer._create_bug(self.soss_record, self.cve)
vulnerability = soss_importer._create_vulnerability(
- bug, self.soss_record, self.cve, self.soss
+ self.soss_record, self.cve, self.soss
)
+ vulnerability.linkBug(bug, check_permissions=False)
self.assertEqual(vulnerability.distribution, self.soss)
self.assertEqual(
@@ -390,7 +392,12 @@ class TestSOSSImporter(TestCaseWithFactory):
self.assertEqual(
soss_importer._get_launchpad_cve("2025-1979"), self.cve
)
- self.assertEqual(soss_importer._get_launchpad_cve("2000-1111"), None)
+ self.assertRaisesWithContent(
+ NotFoundError,
+ "'Could not find 2000-1111 in LP'",
+ soss_importer._get_launchpad_cve,
+ "2000-1111",
+ )
def test_make_bug_description(self):
"""Test make a bug description from a SOSSRecord"""
@@ -445,17 +452,23 @@ class TestSOSSImporter(TestCaseWithFactory):
# SOSSRecord without packages is not valid
self.soss_record.packages = {}
- valid = soss_importer._validate_soss_record(
- self.soss_record, f"CVE-{self.cve.sequence}"
+ self.assertRaisesWithContent(
+ ValueError,
+ "CVE-2025-1979: has no affected packages",
+ soss_importer._validate_soss_record,
+ self.soss_record,
+ f"CVE-{self.cve.sequence}",
)
- self.assertEqual(valid, False)
# SOSSRecord with candidate != sequence is not valid
self.soss_record.candidate = "nonvalid"
- valid = soss_importer._validate_soss_record(
- self.soss_record, f"CVE-{self.cve.sequence}"
+ self.assertRaisesWithContent(
+ ValueError,
+ "CVE sequence mismatch: nonvalid != CVE-2025-1979",
+ soss_importer._validate_soss_record,
+ self.soss_record,
+ f"CVE-{self.cve.sequence}",
)
- self.assertEqual(valid, False)
def test_checkUserPermissions(self):
soss_importer = SOSSImporter()
diff --git a/lib/lp/bugs/scripts/soss/tests/test_sossrecord.py b/lib/lp/bugs/scripts/soss/tests/test_sossrecord.py
index a1cff07..1d673f8 100644
--- a/lib/lp/bugs/scripts/soss/tests/test_sossrecord.py
+++ b/lib/lp/bugs/scripts/soss/tests/test_sossrecord.py
@@ -30,8 +30,20 @@ class TestSOSSRecord(TestCase):
"show_hidden=true",
],
notes=[
- "This is a sample soss cve with all the fields filled for "
- "testing",
+ {
+ "username": (
+ "since 1.0, a package issues a warning when text() is "
+ "omitted this fix is not important, marking priority "
+ "as low"
+ )
+ },
+ {
+ "username": (
+ "since 1.0, a package issues a warning when text() is "
+ "omitted this fix is not important, marking priority "
+ "as low"
+ )
+ },
"sample note 2",
],
priority=SOSSRecord.PriorityEnum.LOW,
@@ -142,8 +154,20 @@ class TestSOSSRecord(TestCase):
"show_hidden=true",
],
"Notes": [
- "This is a sample soss cve with all the fields filled for "
- "testing",
+ {
+ "username": (
+ "since 1.0, a package issues a warning when text() is "
+ "omitted this fix is not important, marking priority "
+ "as low"
+ )
+ },
+ {
+ "username": (
+ "since 1.0, a package issues a warning when text() is "
+ "omitted this fix is not important, marking priority "
+ "as low"
+ )
+ },
"sample note 2",
],
"Priority": "Low",
@@ -270,6 +294,25 @@ class TestSOSSRecord(TestCase):
ValueError, SOSSRecord.from_dict, self.soss_record_dict
)
+ def test_from_dict_bad_package_name(self):
+ """LP source packages can't contain some special characters. If there
+ is a package that does it, we ignore.
+ """
+ # Add a package that we will ignore
+ self.soss_record_dict["Packages"]["unpackaged"].append(
+ {
+ "Name": "wrong_name",
+ "Channel": "noble:0.7.3/stable",
+ "Repositories": ["soss-src-stable-local"],
+ "Status": "needed",
+ "Note": "",
+ }
+ )
+
+ # Output is still the same
+ soss_record = SOSSRecord.from_dict(self.soss_record_dict)
+ self.assertEqual(self.soss_record, soss_record)
+
def test_from_yaml(self):
load_from = Path(__file__).parent / "sampledata" / "CVE-2025-1979"
diff --git a/lib/lp/bugs/tests/test_importvulnerabilityjob.py b/lib/lp/bugs/tests/test_importvulnerabilityjob.py
index aa1640c..af334e6 100644
--- a/lib/lp/bugs/tests/test_importvulnerabilityjob.py
+++ b/lib/lp/bugs/tests/test_importvulnerabilityjob.py
@@ -628,8 +628,10 @@ class ImportVulnerabilityJobTests(TestCaseWithFactory):
job.metadata.get("result"),
{
"succeeded": [],
- "failed": [],
- "error_description": ["None is not a valid PriorityEnum"],
+ "failed": ["CVE-2025-1979"],
+ "error_description": [
+ "CVE-2025-1979: None is not a valid PriorityEnum"
+ ],
},
)
