launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/snapcraft-yaml-remote-symlink into lp:launchpad.

Commit message:
Try to manually resolve symlinks in remote Git repositories when fetching snapcraft.yaml.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1797366 in Launchpad itself: "Fetching snapcraft.yaml from GitHub doesn't honour symlinks"
  https://bugs.launchpad.net/launchpad/+bug/1797366

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/snapcraft-yaml-remote-symlink/+merge/368899
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/snapcraft-yaml-remote-symlink into lp:launchpad.

=== modified file 'lib/lp/snappy/model/snap.py'
--- lib/lp/snappy/model/snap.py	2019-05-16 10:21:14 +0000
+++ lib/lp/snappy/model/snap.py	2019-06-17 08:07:25 +0000
@@ -14,9 +14,12 @@
 from operator import attrgetter
 from urlparse import urlsplit
 
+from bzrlib import urlutils
+from bzrlib.errors import InvalidURLJoin
 from lazr.lifecycle.event import ObjectCreatedEvent
 from pymacaroons import Macaroon
 import pytz
+import six
 from storm.expr import (
     And,
     Desc,
@@ -1298,6 +1301,21 @@
             for path in paths:
                 try:
                     blob = context.getBlob(path)
+                    if (IGitRef.providedBy(context) and
+                            context.repository_url is not None and
+                            isinstance(blob, bytes) and
+                            b":" not in blob and b"\n" not in blob):
+                        # Heuristic.  It seems somewhat common for Git
+                        # hosting sites to return the symlink target path
+                        # when fetching a blob corresponding to a symlink
+                        # committed to a repository: GitHub and GitLab both
+                        # have this property.  If it looks like this is what
+                        # has happened here, try resolving the symlink (only
+                        # to one level).
+                        resolved_path = urlutils.join(
+                            urlutils.dirname(path),
+                            six.ensure_str(blob)).lstrip("/")
+                        blob = context.getBlob(resolved_path)
                     break
                 except (BranchFileNotFound, GitRepositoryBlobNotFound):
                     pass
@@ -1309,6 +1327,8 @@
                 raise MissingSnapcraftYaml(context.unique_name)
         except GitRepositoryBlobUnsupportedRemote as e:
             raise CannotFetchSnapcraftYaml(str(e), unsupported_remote=True)
+        except InvalidURLJoin as e:
+            raise CannotFetchSnapcraftYaml(str(e))
         except (BranchHostingFault, GitRepositoryScanFault) as e:
             msg = "Failed to get snap manifest from %s"
             if logger is not None:

=== modified file 'lib/lp/snappy/tests/test_snap.py'
--- lib/lp/snappy/tests/test_snap.py	2019-05-16 10:21:14 +0000
+++ lib/lp/snappy/tests/test_snap.py	2019-06-17 08:07:25 +0000
@@ -1764,6 +1764,64 @@
         self.assertEqual(0, unsafe_load.mock.call_count)
         self.assertEqual(1, safe_load.mock.call_count)
 
+    @responses.activate
+    def test_getSnapcraftYaml_symlink(self):
+        for path in ("snap/snapcraft.yaml", "build-aux/snap/snapcraft.yaml"):
+            responses.add(
+                "GET",
+                "https://raw.githubusercontent.com/owner/name/HEAD/%s"; % path,
+                status=404)
+        responses.add(
+            "GET",
+            "https://raw.githubusercontent.com/owner/name/HEAD/snapcraft.yaml";,
+            body=b"pkg/snap/snapcraft.yaml")
+        responses.add(
+            "GET",
+            "https://raw.githubusercontent.com/owner/name/HEAD/";
+            "pkg/snap/snapcraft.yaml",
+            body=b"name: test-snap")
+        git_ref = self.factory.makeGitRefRemote(
+            repository_url="https://github.com/owner/name";, path="HEAD")
+        snap = self.factory.makeSnap(git_ref=git_ref)
+        with default_timeout(1.0):
+            self.assertEqual(
+                {"name": "test-snap"},
+                getUtility(ISnapSet).getSnapcraftYaml(snap))
+
+    @responses.activate
+    def test_getSnapcraftYaml_symlink_via_parent(self):
+        responses.add(
+            "GET",
+            "https://raw.githubusercontent.com/owner/name/HEAD/";
+            "snap/snapcraft.yaml",
+            body=b"../pkg/snap/snapcraft.yaml")
+        responses.add(
+            "GET",
+            "https://raw.githubusercontent.com/owner/name/HEAD/";
+            "pkg/snap/snapcraft.yaml",
+            body=b"name: test-snap")
+        git_ref = self.factory.makeGitRefRemote(
+            repository_url="https://github.com/owner/name";, path="HEAD")
+        snap = self.factory.makeSnap(git_ref=git_ref)
+        with default_timeout(1.0):
+            self.assertEqual(
+                {"name": "test-snap"},
+                getUtility(ISnapSet).getSnapcraftYaml(snap))
+
+    @responses.activate
+    def test_getSnapcraftYaml_symlink_above_root(self):
+        responses.add(
+            "GET",
+            "https://raw.githubusercontent.com/owner/name/HEAD/snapcraft.yaml";,
+            body=b"../pkg/snap/snapcraft.yaml")
+        git_ref = self.factory.makeGitRefRemote(
+            repository_url="https://github.com/owner/name";, path="HEAD")
+        snap = self.factory.makeSnap(git_ref=git_ref)
+        with default_timeout(1.0):
+            self.assertRaises(
+                CannotFetchSnapcraftYaml,
+                getUtility(ISnapSet).getSnapcraftYaml, snap)
+
     def test__findStaleSnaps(self):
         # Stale; not built automatically.
         self.factory.makeSnap(is_stale=True)