fix: validate appstream metadata links and prefer links from project …

…metadata (#4888)

- Validate that update_contact, donation, vcs-browser, bugtracker, and homepage
  fields adopted from an appstream metadata file are valid URLs or email addresses.
- Contact, donation, source-code, issues, and website fields in a snapcraft.yaml take
  priority over appstream metadata

docs/requirements.txt

@@ -121,6 +121,7 @@ typing_extensions==4.11.0
 uc-micro-py==1.0.3
 urllib3==1.26.19
 uvicorn==0.29.0
+validators==0.28.3
 wadllib==1.3.6
 watchfiles==0.21.0
 wcmatch==8.5.1

requirements-devel.txt

@@ -114,6 +114,7 @@ types-toml==0.10.8.20240310
 types-urllib3==1.26.25.14
 typing_extensions==4.9.0
 urllib3==1.26.19
+validators==0.28.3
 venusian==3.1.0
 virtualenv==20.25.1
 wadllib==1.3.6

requirements.txt

@@ -68,6 +68,7 @@ types-Deprecated==1.2.9.20240311
 types-PyYAML==6.0.12.20240311
 typing_extensions==4.9.0
 urllib3==1.26.19
+validators==0.28.3
 wadllib==1.3.6
 wrapt==1.16.0
 ws4py==0.5.1

snapcraft/meta/appstream.py

@@ -23,6 +23,8 @@
 from typing import List, Optional, cast
 
 import lxml.etree
+import validators
+from craft_cli import emit
 from xdg.DesktopEntry import DesktopEntry
 
 from snapcraft import errors
@@ -102,7 +104,15 @@ def extract(relpath: str, *, workdir: str) -> Optional[ExtractedMetadata]:
     version = _get_latest_release_from_nodes(dom.findall("releases/release"))
     project_license = _get_value_from_xml_element(dom, "project_license")
     update_contact = _get_value_from_xml_element(dom, "update_contact")
-    contact = [update_contact] if update_contact else None
+    contact = None
+    if update_contact:
+        if validators.url(update_contact) or validators.email(update_contact):
+            contact = [update_contact]
+        else:
+            emit.progress(
+                f"Ignoring invalid url {update_contact!r} in update_contact from appstream metadata.",
+                permanent=True,
+            )
 
     issues = _get_urls_from_xml_element(dom.findall("url"), "bugtracker")
     donation = _get_urls_from_xml_element(dom.findall("url"), "donation")
@@ -184,7 +194,14 @@ def _get_urls_from_xml_element(nodes, url_type) -> Optional[List[str]]:
             and node.attrib["type"] == url_type
             and node.text.strip() not in urls
         ):
-            urls.append(node.text.strip())
+            link = node.text.strip()
+            if validators.url(link) or validators.email(link):
+                urls.append(link)
+            else:
+                emit.progress(
+                    f"Ignoring invalid url or email {link!r} in {url_type!r} from appstream metadata.",
+                    permanent=True,
+                )
     if urls:
         return urls
     return None

snapcraft/parts/update_metadata.py

@@ -17,7 +17,7 @@
 """External metadata helpers."""
 
 from pathlib import Path
-from typing import Dict, Final, List, cast
+from typing import Dict, Final, List, OrderedDict, cast
 
 import pydantic
 from craft_application.models import ProjectTitle, SummaryStr, UniqueStrList, VersionStr
@@ -115,18 +115,26 @@ def _update_project_links(
     :param project: The Project model to update.
     :param metadata_list: A list of parsed information from metadata files.
     """
-    for metadata in metadata_list:
-        fields = ["contact", "donation", "source_code", "issues", "website"]
-        for field in fields:
-            metadata_field = getattr(metadata, field)
-            project_field = getattr(project, field)
-            if metadata_field and project_field and project_field != metadata_field:
-                project_list = list(project_field)
-                project_list.extend(set(metadata_field) - set(project_list))
-                setattr(project, field, cast(UniqueStrList, project_list))
-
-            if not getattr(project, field):
-                setattr(project, field, cast(UniqueStrList, metadata_field))
+    fields = ["contact", "donation", "source_code", "issues", "website"]
+    for field in fields:
+        project_field = getattr(project, field)
+
+        # only update the project if the project has not defined the field
+        if not project_field:
+
+            # values for a field from all metadata files
+            metadata_values: list[str] = list()
+
+            # iterate through all metadata and create a set of values for the field
+            for metadata in metadata_list:
+                if metadata_field := getattr(metadata, field):
+                    metadata_values = list(
+                        OrderedDict.fromkeys(metadata_values + metadata_field)
+                    )
+
+            # update project with all new values from the metadata
+            if metadata_values:
+                setattr(project, field, cast(UniqueStrList, metadata_values))
 
 
 def _update_project_variables(project: Project, project_vars: Dict[str, str]):

tests/unit/meta/test_appstream.py

@@ -50,22 +50,40 @@ class TestAppstreamData:
             ("id", {}, "common_id", "test-id", "test-id"),
             ("name", {}, "title", "test-title", "test-title"),
             ("project_license", {}, "license", "test-license", "test-license"),
-            ("update_contact", {}, "contact", "test-contact", ["test-contact"]),
-            ("url", {"type": "homepage"}, "website", "test-website", ["test-website"]),
-            ("url", {"type": "bugtracker"}, "issues", "test-issues", ["test-issues"]),
+            (
+                "update_contact",
+                {},
+                "contact",
+                "test-contact@test.com",
+                ["test-contact@test.com"],
+            ),
+            (
+                "url",
+                {"type": "homepage"},
+                "website",
+                "http://test-website.test",
+                ["http://test-website.test"],
+            ),
+            (
+                "url",
+                {"type": "bugtracker"},
+                "issues",
+                "http://test-issues.test",
+                ["http://test-issues.test"],
+            ),
             (
                 "url",
                 {"type": "donation"},
                 "donation",
-                "test-donation",
-                ["test-donation"],
+                "http://test-donation.test",
+                ["http://test-donation.test"],
             ),
             (
                 "url",
                 {"type": "vcs-browser"},
                 "source_code",
-                "test-source",
-                ["test-source"],
+                "test@test.com",
+                ["test@test.com"],
             ),
         ],
     )
@@ -678,6 +696,7 @@ def test_appstream_links(self):
                   <li xml:lang="es">Publica snaps en la tienda.</li>
                 </ul>
               </description>
+              <update_contact>https://hello.com</update_contact>
               <url type="homepage">https://johnfactotum.github.io/foliate/</url>
               <url type="vcs-browser">https://github.com/johnfactotum/foliate</url>
               <url type="bugtracker">https://github.com/johnfactotum/foliate/issues</url>
@@ -692,6 +711,7 @@ def test_appstream_links(self):
         metadata = appstream.extract(file_name, workdir=".")
 
         assert metadata is not None
+        assert metadata.contact == ["https://hello.com"]
         assert metadata.website == ["https://johnfactotum.github.io/foliate/"]
         assert metadata.issues == ["https://github.com/johnfactotum/foliate/issues"]
         assert metadata.donation == ["https://www.buymeacoffee.com/johnfactotum"]
@@ -708,6 +728,7 @@ def test_appstream_url(self):
               <project_license>GPL-3.0-or-later</project_license>
               <content_rating type="oars-1.1"/>
               <name>Drawing</name>
+              <update_contact>rrroschan@gmail.com</update_contact>
               <url type="homepage">https://johnfactotum.github.io/foliate/</url>
               <url type="vcs-browser">https://github.com/johnfactotum/foliate</url>
               </component>
@@ -717,6 +738,7 @@ def test_appstream_url(self):
 
         metadata = appstream.extract(file_name, workdir=".")
         assert metadata is not None
+        assert metadata.contact == ["rrroschan@gmail.com"]
         assert metadata.source_code == ["https://github.com/johnfactotum/foliate"]
 
     def test_appstream_with_multiple_lists(self):
@@ -749,6 +771,39 @@ def test_appstream_with_multiple_lists(self):
         assert metadata.website == ["https://johnfactotum.github.io/foliate/"]
         assert metadata.donation is None
 
+    def test_appstream_with_malformed_links(self):
+        file_name = "test.appdata.xml"
+        content = textwrap.dedent(
+            """\
+            <?xml version="1.0" encoding="UTF-8"?>
+              <component type="desktop">
+              <id>com.github.maoschanz.drawing</id>
+              <metadata_license>CC0-1.0</metadata_license>
+              <project_license>GPL-3.0-or-later</project_license>
+              <content_rating type="oars-1.1"/>
+              <name>Drawing</name>
+              <url type="homepage">bad_invalid_mail</url>
+              <url type="homepage">https://hello.com</url>
+              <url type="vcs-browser">file://invalid.file.link</url>
+              <url type="vcs-browser">http://missing-dot-com</url>
+              <url type="bugtracker">https://github.com/alainm23/planify/issues</url>
+              <url type="bugtracker">https:/missing-slash.com</url>
+              <url type="donation">https://www.buymeacoffee.com/alainm23</url>
+              <url type="donation">paypal.me/bad</url>
+              <update_contact>rrroschan@gmail.com</update_contact>
+              <update_contact>malformed_invalid@mailcom</update_contact>
+              </component>
+        """
+        )
+        Path(file_name).write_text(content)
+        metadata = appstream.extract(file_name, workdir=".")
+        assert metadata is not None
+        assert metadata.contact == ["rrroschan@gmail.com"]
+        assert metadata.donation == ["https://www.buymeacoffee.com/alainm23"]
+        assert metadata.website == ["https://hello.com"]
+        assert metadata.source_code == None
+        assert metadata.issues == ["https://github.com/alainm23/planify/issues"]
+
     def test_appstream_parse_error(self):
         file_name = "snapcraft_legacy.appdata.xml"
         content = textwrap.dedent(

tests/unit/parts/test_update_metadata.py

@@ -73,10 +73,8 @@ def yaml_data(extra_args: Dict[str, Any]):
             "confinement": "strict",
             "license": "license",
             "contact": "contact1",
-            "donation": "donation1",
             "issues": "issues1",
             "website": "website1",
-            "source-code": "source-code",
             "parts": {},
             **extra_args,
         }
@@ -133,11 +131,11 @@ def test_update_project_metadata(project_yaml_data, appstream_file, new_dir):
     assert project.summary == "summary"  # already set in project
     assert project.description == "description"  # already set in project
     assert project.version == "0.1"  # already set in project
-    assert project.contact == ["contact1", "contact2"]
-    assert project.issues == ["issues1", "issues2"]
-    assert project.donation == ["donation1", "donation2"]
-    assert project.website == ["website1", "website2"]
-    assert project.source_code == ["source-code", "vcs-browser"]  # adopts from project
+    assert project.contact == ["contact1"]  # already set in project
+    assert project.issues == ["issues1"]  # already set in project
+    assert project.donation == ["donation2"]
+    assert project.website == ["website1"]  # already set in project
+    assert project.source_code == ["vcs-browser"]
     assert project.icon == "assets/icon.png"
     assert project.apps["app3"].desktop == "assets/file.desktop"
 
@@ -256,6 +254,7 @@ def test_update_project_metadata_multiple(
     metadata2 = ExtractedMetadata(
         summary="metadata summary",
         description="metadata description",
+        contact=["contact1"],
         website=["website1"],
         source_code=["source-code"],
         issues=["issues1", "issues3"],
@@ -267,7 +266,7 @@ def test_update_project_metadata_multiple(
     metadata4 = ExtractedMetadata(
         summary="extra summary", description="extra description"
     )
-    metadata5 = ExtractedMetadata(license="GPL-3.0", contact=["test@test.com"])
+    metadata5 = ExtractedMetadata(license="GPL-3.0", contact=["contact2", "contact1"])
     metadata6 = ExtractedMetadata(
         source_code=["source-code", "vcs-browser"],
         website=["website2"],
@@ -296,14 +295,55 @@ def test_update_project_metadata_multiple(
     assert project.description == expected["description"]
     assert project.title == expected["title"]
     assert project.grade == expected["grade"]
-    assert project.contact == ["test@test.com"]
+    assert project.contact == ["contact1", "contact2"]
     assert project.license == "GPL-3.0"
     assert project.donation == ["donation1", "donation2"]
     assert project.source_code == ["source-code", "vcs-browser"]
     assert project.issues == ["issues1", "issues3", "issues2"]
     assert project.website == ["website1", "website2"]
 
 
+def test_update_project_metadata_overriding_appstream(new_dir):
+    yaml_data = {
+        "name": "my-project",
+        "base": "core22",
+        "confinement": "strict",
+        "adopt-info": "part",
+        "parts": {},
+        "contact": "test@test.com",
+        "donation": "https://paypal.me/",
+        "issues": "https://test.com/issues",
+        "source-code": "https://test.com/source-code",
+        "website": "https://test.com/website",
+    }
+    project = Project(**yaml_data)
+    metadata = ExtractedMetadata(
+        version="4.5.6",
+        summary="metadata summary",
+        description="metadata description",
+        contact=["help@help.me"],
+        website=["https://example.com/website"],
+        source_code=["https://example.com/source-code"],
+        issues=["https://example.com/issues", "https://example.com/issues2"],
+        donation=["https://buyme.coffee", "https://github.com/sponsors"],
+    )
+    prj_vars = {"version": "", "grade": ""}
+    update_project_metadata(
+        project,
+        project_vars=prj_vars,
+        metadata_list=[metadata],
+        assets_dir=new_dir,
+        prime_dir=new_dir,
+    )
+
+    assert project is not None
+    assert project.contact == ["test@test.com"]
+    assert project.donation == ["https://paypal.me/"]
+    assert project.issues == ["https://test.com/issues"]
+    assert project.source_code == ["https://test.com/source-code"]
+    assert project.website == ["https://test.com/website"]
+
+
 @pytest.mark.parametrize(
     "project_entries,icon_exists,asset_exists,expected_icon",
     [