Merge lp:~lool/snapcraft/no-separate-toplevel-dir into lp:~snappy-dev/snapcraft/core

[Loïc Minier]

Rework tarball unpack to use tarfile. New implementation deals properly with tarballs which ship all files under a common directory, but without an entry for this common directory. Also handles dangerous tarballs with pathes starting with / or ../. Adjust tests accordingly.

NB: this was found while trying to use the upstream tomcat tarball; toplevel parent dir didn't get stripped on unpack.

Rework tarball unpack to use tarfile. New implementation deals properly with tarballs which ship all files under a common directory, but without an entry for this common directory. Also handles dangerous tarballs with pathes starting with / or ../. Adjust tests accordingly.

NB: this was found while trying to use the upstream tomcat tarball; toplevel parent dir didn't get stripped on unpack.

[Michael Terry (mterry)]

Thanks! I didn't realize tar didn't handle evil paths for us. :(

And I totally forgot about the tarfile module.

review: Approve

=== added file 'integration-tests/data/simple-tar/evil-parent.tar.gz'
Binary files integration-tests/data/simple-tar/evil-parent.tar.gz	1970-01-01 00:00:00 +0000 and integration-tests/data/simple-tar/evil-parent.tar.gz	2015-07-24 01:25:43 +0000 differ
=== added file 'integration-tests/data/simple-tar/evil-slash.tar.gz'
Binary files integration-tests/data/simple-tar/evil-slash.tar.gz	1970-01-01 00:00:00 +0000 and integration-tests/data/simple-tar/evil-slash.tar.gz	2015-07-24 01:25:43 +0000 differ
=== added file 'integration-tests/data/simple-tar/nodir.tar.xz'
Binary files integration-tests/data/simple-tar/nodir.tar.xz	1970-01-01 00:00:00 +0000 and integration-tests/data/simple-tar/nodir.tar.xz	2015-07-24 01:25:43 +0000 differ
=== added file 'integration-tests/data/simple-tar/simple.tar.bz2'
Binary files integration-tests/data/simple-tar/simple.tar.bz2	1970-01-01 00:00:00 +0000 and integration-tests/data/simple-tar/simple.tar.bz2	2015-07-24 01:25:43 +0000 differ
=== modified file 'integration-tests/data/simple-tar/snapcraft.yaml'
--- integration-tests/data/simple-tar/snapcraft.yaml	2015-07-17 19:39:00 +0000
+++ integration-tests/data/simple-tar/snapcraft.yaml	2015-07-24 01:25:43 +0000
@@ -5,9 +5,21 @@
   onedeep:
     plugin: tar-content
     source: onedeep.tar.gz
+  nodir:
+    plugin: tar-content
+    source: nodir.tar.xz
   flat:
     plugin: tar-content
     source: flat.tar.xz
+  simple:
+    plugin: tar-content
+    source: simple.tar.bz2
+  evil-parent:
+    plugin: tar-content
+    source: evil-parent.tar.gz
+  evil-slash:
+    plugin: tar-content
+    source: evil-slash.tar.gz
   project:
     plugin: make-project
     source: project.tar.xz

=== modified file 'integration-tests/units/jobs.pxu'
--- integration-tests/units/jobs.pxu	2015-07-22 21:05:20 +0000
+++ integration-tests/units/jobs.pxu	2015-07-24 01:25:43 +0000
@@ -82,6 +82,12 @@
     test -e stage/onedeep
     test -e stage/onedeepdir/onedeep2
     test -e stage/oneflat
+    test -e stage/dir-simple
+    test -e stage/top-simple
+    test -e stage/notop
+    test -e stage/notopdir
+    test -e stage/parent
+    test -e stage/slash
     test "$(./stage/bin/test)" = "tarproject"
 
 id: snapcraft/normal/simple-make

=== modified file 'snapcraft/__init__.py'
--- snapcraft/__init__.py	2015-07-22 18:59:28 +0000
+++ snapcraft/__init__.py	2015-07-24 01:25:43 +0000
@@ -15,9 +15,11 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
 import os
+import re
+import tarfile
+import urllib.parse
+
 import snapcraft.common
-import subprocess
-import urllib.parse
 
 
 class BasePlugin:
@@ -102,16 +104,37 @@
         else:
             tarball = os.path.abspath(source)
 
-        # If there's a single toplevel directory, ignore it
-        try:
-            topfiles = subprocess.check_output(['tar', '--list', '-f', tarball, '--exclude', '*/*']).strip()
-        except Exception:
-            return False
-        strip_cmd = []
-        if topfiles and len(topfiles.split(b'\n')) == 1 and chr(topfiles[-1]) == '/':
-            strip_cmd = ['--strip-components=1']
-
-        return self.run(['tar'] + strip_cmd + ['-xf', tarball], cwd=destdir)
+        with tarfile.open(tarball) as tar:
+            def filter_members(tar):
+                """Filters members and member names:
+                    - strips common prefix
+                    - bans dangerous names"""
+                members = tar.getmembers()
+                common = os.path.commonprefix([m.name for m in members])
+
+                # commonprefix() works a character at a time and will
+                # consider "d/ab" and "d/abc" to have common prefix "d/ab";
+                # check all members either start with common dir
+                for m in members:
+                    if not (m.name.startswith(common + "/") or
+                            m.isdir() and m.name == common):
+                        # commonprefix() didn't return a dir name; go up one
+                        # level
+                        common = os.path.dirname(common)
+                        break
+
+                for m in members:
+                    if m.name == common:
+                        continue
+                    if m.name.startswith(common + "/"):
+                        m.name = m.name[len(common + "/"):]
+                    # strip leading "/", "./" or "../" as many times as needed
+                    m.name = re.sub(r'^(\.{0,2}/)*', r'', m.name)
+                    yield m
+
+            tar.extractall(members=filter_members(tar), path=destdir)
+
+        return True
 
     def get_source(self, source, source_type=None, source_tag=None, source_branch=None):
         if source_type is None: