From fad8df3c7dbe661ac00632af5e7ced1041b0fbd4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E0=A6=B8=E0=A7=8C=E0=A6=AE=E0=A7=8D=E0=A6=AF=E0=A6=A6?= =?UTF-8?q?=E0=A7=80=E0=A6=AA=20=E0=A6=98=E0=A7=8B=E0=A6=B7?= <72045785+soumyaDghosh@users.noreply.github.com> Date: Sat, 6 Jul 2024 00:46:47 +0530 Subject: [PATCH] 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 | 1 + requirements-devel.txt | 1 + requirements.txt | 1 + snapcraft/meta/appstream.py | 21 +++++++- snapcraft/parts/update_metadata.py | 34 +++++++----- tests/unit/meta/test_appstream.py | 69 +++++++++++++++++++++--- tests/unit/parts/test_update_metadata.py | 58 ++++++++++++++++---- 7 files changed, 154 insertions(+), 31 deletions(-) diff --git a/docs/requirements.txt b/docs/requirements.txt index d591c34a63..ebda86fc35 100644 --- a/docs/requirements.txt +++ b/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 diff --git a/requirements-devel.txt b/requirements-devel.txt index d42d2069c1..c20066a4bf 100644 --- a/requirements-devel.txt +++ b/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 diff --git a/requirements.txt b/requirements.txt index af830482b7..c75faaeb21 100644 --- a/requirements.txt +++ b/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 diff --git a/snapcraft/meta/appstream.py b/snapcraft/meta/appstream.py index 60d500acb6..abd91caf2e 100644 --- a/snapcraft/meta/appstream.py +++ b/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 diff --git a/snapcraft/parts/update_metadata.py b/snapcraft/parts/update_metadata.py index 0c04133e89..d2ae9d9e71 100644 --- a/snapcraft/parts/update_metadata.py +++ b/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]): diff --git a/tests/unit/meta/test_appstream.py b/tests/unit/meta/test_appstream.py index 6bc64bd42b..a7d6bcfc17 100644 --- a/tests/unit/meta/test_appstream.py +++ b/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):
  • Publica snaps en la tienda.
    • + https://hello.com https://johnfactotum.github.io/foliate/ https://github.com/johnfactotum/foliate https://github.com/johnfactotum/foliate/issues @@ -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): GPL-3.0-or-later Drawing + rrroschan@gmail.com https://johnfactotum.github.io/foliate/ https://github.com/johnfactotum/foliate @@ -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( + """\ + + + com.github.maoschanz.drawing + CC0-1.0 + GPL-3.0-or-later + + Drawing + bad_invalid_mail + https://hello.com + file://invalid.file.link + http://missing-dot-com + https://github.com/alainm23/planify/issues + https:/missing-slash.com + https://www.buymeacoffee.com/alainm23 + paypal.me/bad + rrroschan@gmail.com + malformed_invalid@mailcom + + """ + ) + 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( diff --git a/tests/unit/parts/test_update_metadata.py b/tests/unit/parts/test_update_metadata.py index 0e85ba4f3e..9d208db701 100644 --- a/tests/unit/parts/test_update_metadata.py +++ b/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,7 +295,7 @@ 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"] @@ -304,6 +303,47 @@ def test_update_project_metadata_multiple( 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", [