Merge ~pfsmorigo/review-tools:check_link_improvements into review-tools:master

[Paulo Flabiano Smorigo]

I used the ValidateLinks from snapd but had to make some changes. In snapd, the validation relies on an url parser that can check the syntax. For the review tools I'm using a regex for that.

[Alex Murray (alexmurray)]

Thanks for this - I don't think we need LINK_TYPES anymore and I think you should just use the python standard library to parse / validate the URLs rather than a regex as that will be too fragile.

review: Needs Fixing

[Paulo Flabiano Smorigo (pfsmorigo)]

I just removed LINK_TYPES. For the regex, I'm willing to remove it and use a python library to validade but, since the system runs on xenial, it's hard to find a good library for it. I can't use python3-validators for example. There is urllib that can parse the url but don't validate it unless you use it for a query and check for the returning error. There is also the requests library but you also needs to do a request in order to check the result. About the regex, why too fragile? Maybe there would be cases that it will fails but than we can manually approve, right?

[Alex Murray (alexmurray)]

The snap store now uses focal not xenial and the review-tools snap is base: core18 so you should be able to use anything in bionic.

I personally don't want to have to manually approve every snap upload for a snap which uses a non-ascii URL or similar - I also don't think this is a good situation for such publishers either - so if we can just use an existing trusted library to parse + validate the URL then that seems like a better solution to me.

[Alex Murray (alexmurray)]

Let's merge this as is since it is very good - we can revisit it later if it turns out there are URLs which get caught by the regex.

review: Approve

diff --git a/reviewtools/common.py b/reviewtools/common.py
index ce47886..548b8d7 100644
--- a/reviewtools/common.py
+++ b/reviewtools/common.py
@@ -140,9 +140,6 @@ OS_RELEASE_MAP = {
     }
 }
 
-# Link categories used in snap.yaml
-LINK_TYPES = ["website", "source-code", "contact", "donation", "issues"]
-
 
 def cleanup_unpack():
     global UNPACK_DIR
diff --git a/reviewtools/sr_lint.py b/reviewtools/sr_lint.py
index 6cca8d9..819ef5a 100644
--- a/reviewtools/sr_lint.py
+++ b/reviewtools/sr_lint.py
@@ -20,7 +20,6 @@ from reviewtools.common import (
     find_external_symlinks,
     STORE_PKGNAME_SNAPV2_MAXLEN,
     STORE_PKGNAME_SNAPV2_MINLEN,
-    LINK_TYPES,
 )
 from reviewtools.overrides import (
     redflagged_snap_types_overrides,
@@ -348,23 +347,70 @@ class SnapReviewLint(SnapReview):
         if "links" not in self.snap_yaml:
             return
 
-        for val in self.snap_yaml["links"]:
-            if val not in LINK_TYPES:
+        # Check links is partially based on:
+        # https://github.com/snapcore/snapd/blob/master/snap/validate.go#L1132
+
+        valid_link = re.compile(r"^[a-zA-Z](?:-?[a-zA-Z0-9])*$")
+        valid_url = re.compile(
+            r"^http[s]?://(?:[a-zA-Z]|[0-9]|[$-_@.&+]|[!*\(\),]|(?:%[0-9a-fA-F][0-9a-fA-F]))+$"
+        )
+        valid_email = re.compile(
+            r"^(mailto:|)[A-Za-z0-9._%+-]+@[A-Za-z0-9.-]+\.[A-Z|a-z]{2,}$"
+        )
+
+        for linkskey in self.snap_yaml["links"]:
+            if linkskey.strip() == "":
                 t = "error"
-                s = "'unknown field for links in snap.yaml: %s" % val
+                s = "links key cannot be empty"
                 self._add_result(t, n, s)
                 return
 
-            content = self.snap_yaml["links"][val]
-            if not isinstance(content, str) and not (
-                isinstance(content, list)
-                and all(isinstance(item, str) for item in content)
-            ):
+            if not valid_link.match(linkskey):
+                t = "error"
+                s = "links key is invalid: %s" % linkskey
+                self._add_result(t, n, s)
+                return
+
+            linksvalues = self.snap_yaml["links"][linkskey]
+
+            if not isinstance(linksvalues, list):
+                t = "error"
+                s = "%s is not a list of strings" % linkskey
+                self._add_result(t, n, s)
+                return
+
+            if len(linksvalues) == 0:
                 t = "error"
-                s = "'invalid value for %s in snap.yaml" % val
+                s = "%s links cannot be specified and empty" % linkskey
                 self._add_result(t, n, s)
                 return
 
+            for link in linksvalues:
+                if link == "":
+                    t = "error"
+                    s = "empty %s link" % linkskey
+                    self._add_result(t, n, s)
+                    return
+
+                if not isinstance(link, str):
+                    t = "error"
+                    s = "%s is not a string" % link
+                    self._add_result(t, n, s)
+                    return
+
+                if not (
+                    linkskey == "contact"
+                    and valid_email.match(link)
+                    or valid_url.match(link)
+                ):
+                    t = "error"
+                    s = (
+                        "%s link must have one of http|https schemes or it must be an email address: %s"
+                        % (linkskey, link)
+                    )
+                    self._add_result(t, n, s, manual_review=True)
+                    return
+
     def check_unknown_entries(self):
         """Check for any unknown fields"""
         t = "info"
diff --git a/reviewtools/tests/test_sr_lint.py b/reviewtools/tests/test_sr_lint.py
index 5d9aba7..27b7533 100644
--- a/reviewtools/tests/test_sr_lint.py
+++ b/reviewtools/tests/test_sr_lint.py
@@ -976,21 +976,16 @@ class TestSnapReviewLint(sr_tests.TestSnapReview):
 
     def test_check_link(self):
         """Test check_link"""
-        self.set_test_snap_yaml("links", [])
-        c = SnapReviewLint(self.test_name)
-        c.check_links()
-        r = c.review_report
-        expected_counts = {"info": None, "warn": 0, "error": 0}
-        self.check_results(r, expected_counts)
-
-    def test_check_link_valid(self):
-        """Test check_link_valid"""
         links = {
-            "website": "https://snapcraft.io",
-            "source-code": "https://github.com/snapcore/snapcraft",
-            "contact": ["https://forum.snapcraft.io", "[e-mail]"],
-            "donation": "http://donate.me",
-            "issues": "https://bugs.launchpad.net/snapcraft/+filebug",
+            "website": ["https://snapcraft.io"],
+            "source-code": ["https://github.com/snapcore/snapcraft"],
+            "contact": [
+                "https://forum.snapcraft.io",
+                "mailto:[e-mail]",
+                "[e-mail]",
+            ],
+            "donation": ["http://donate.me"],
+            "issues": ["https://bugs.launchpad.net/snapcraft/+filebug"],
         }
         self.set_test_snap_yaml("links", links)
         c = SnapReviewLint(self.test_name)
@@ -1001,15 +996,51 @@ class TestSnapReviewLint(sr_tests.TestSnapReview):
 
     def test_check_link_invalid_field(self):
         """Test check_link_invalid_field"""
-        self.set_test_snap_yaml("links", {"invalid_field": ""})
+        self.set_test_snap_yaml("links", {"invalid_field": [""]})
+        c = SnapReviewLint(self.test_name)
+        c.check_links()
+        r = c.review_report
+        expected_counts = {"info": None, "warn": 0, "error": 1}
+        self.check_results(r, expected_counts)
+
+    def test_check_link_invalid_url_no_http_prefix(self):
+        """Test check_link_invalid_url_no_http_prefix"""
+        self.set_test_snap_yaml("links", {"website": "www.snapcraft.io"})
         c = SnapReviewLint(self.test_name)
         c.check_links()
         r = c.review_report
         expected_counts = {"info": None, "warn": 0, "error": 1}
         self.check_results(r, expected_counts)
 
-    def test_check_link_invalid_content_1(self):
+    def test_check_link_invalid_email(self):
         """Test check_link_invalid_email"""
+        self.set_test_snap_yaml("links", {"contact": "invalid(at)email.com"})
+        c = SnapReviewLint(self.test_name)
+        c.check_links()
+        r = c.review_report
+        expected_counts = {"info": None, "warn": 0, "error": 1}
+        self.check_results(r, expected_counts)
+
+    def test_check_link_email_not_in_contacts(self):
+        """Test check_link_email_not_in_contacts"""
+        self.set_test_snap_yaml("links", {"issues": "[e-mail]"})
+        c = SnapReviewLint(self.test_name)
+        c.check_links()
+        r = c.review_report
+        expected_counts = {"info": None, "warn": 0, "error": 1}
+        self.check_results(r, expected_counts)
+
+    def test_check_link_invalid_content_string(self):
+        """Test check_link_invalid_content_string"""
+        self.set_test_snap_yaml("links", {"website": "http://www.snapcraft.io"})
+        c = SnapReviewLint(self.test_name)
+        c.check_links()
+        r = c.review_report
+        expected_counts = {"info": None, "warn": 0, "error": 1}
+        self.check_results(r, expected_counts)
+
+    def test_check_link_invalid_content_number(self):
+        """Test check_link_invalid_content_number"""
         self.set_test_snap_yaml("links", {"contact": 123})
         c = SnapReviewLint(self.test_name)
         c.check_links()
@@ -1017,8 +1048,8 @@ class TestSnapReviewLint(sr_tests.TestSnapReview):
         expected_counts = {"info": None, "warn": 0, "error": 1}
         self.check_results(r, expected_counts)
 
-    def test_check_link_invalid_content_2(self):
-        """Test check_link_invalid_email"""
+    def test_check_link_invalid_content_number_list(self):
+        """Test check_link_invalid_content_number_list"""
         self.set_test_snap_yaml("links", {"website": [456, 789]})
         c = SnapReviewLint(self.test_name)
         c.check_links()
diff --git a/snapcraft.yaml b/snapcraft.yaml
index b651682..1cc7be2 100644
--- a/snapcraft.yaml
+++ b/snapcraft.yaml
@@ -7,9 +7,6 @@ description: |
   and can be used to verify your snap before upload.
 confinement: strict
 base: core18
-website: https://launchpad.net/review-tools
-source-code: https://git.launchpad.net/review-tools
-issues: https://bugs.launchpad.net/review-tools
 
 environment:
   LANG: C.UTF-8