Merge "Swift logs: don't allow links outside of the supplied path"

roles/wip-upload-logs-swift/library/test-fixtures/links/job-output.json

@@ -0,0 +1 @@
+{"test": "foo"}

roles/wip-upload-logs-swift/library/test_zuul_swift_upload.py

@@ -14,17 +14,45 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-import unittest
-from .zuul_swift_upload import FileList, Indexer
 import os
 
+import testtools
+import fixtures
 from bs4 import BeautifulSoup
 
+from .zuul_swift_upload import FileList, Indexer
+
+
 FIXTURE_DIR = os.path.join(os.path.dirname(__file__),
                            'test-fixtures')
 
 
-class TestFileList(unittest.TestCase):
+class SymlinkFixture(fixtures.Fixture):
+    links = [
+        ('bad_symlink', '/etc'),
+        ('bad_symlink_file', '/etc/issue'),
+        ('good_symlink', 'controller'),
+        ('recursive_symlink', '.'),
+        ('symlink_file', 'job-output.json'),
+        ('symlink_loop_a', 'symlink_loop'),
+        ('symlink_loop/symlink_loop_b', '..'),
+    ]
+
+    def _setUp(self):
+        self._cleanup()
+        for (src, target) in self.links:
+            path = os.path.join(FIXTURE_DIR, 'links', src)
+            os.symlink(target, path)
+        self.addCleanup(self._cleanup)
+
+    def _cleanup(self):
+        for (src, target) in self.links:
+            path = os.path.join(FIXTURE_DIR, 'links', src)
+            if os.path.exists(path):
+                os.unlink(path)
+
+
+class TestFileList(testtools.TestCase):
 
     def assert_files(self, result, files):
         self.assertEqual(len(result), len(files))
@@ -100,6 +128,25 @@ class TestFileList(unittest.TestCase):
             ('inventory.yaml', 'text/plain', None),
         ])
 
+    def test_symlinks(self):
+        '''Test symlinks'''
+        fl = FileList()
+        self.useFixture(SymlinkFixture())
+        fl.add(os.path.join(FIXTURE_DIR, 'links/'))
+        self.assert_files(fl, [
+            ('', 'application/directory', None),
+            ('controller', 'application/directory', None),
+            ('good_symlink', 'application/directory', None),
+            ('recursive_symlink', 'application/directory', None),
+            ('symlink_loop', 'application/directory', None),
+            ('symlink_loop_a', 'application/directory', None),
+            ('job-output.json', 'application/json', None),
+            ('symlink_file', 'text/plain', None),
+            ('controller/service_log.txt', 'text/plain', None),
+            ('symlink_loop/symlink_loop_b', 'application/directory', None),
+            ('symlink_loop/placeholder', 'text/plain', None),
+        ])
+
     def test_index_files(self):
         '''Test index generation'''
         fl = FileList()

roles/wip-upload-logs-swift/library/zuul_swift_upload.py

@@ -130,6 +130,15 @@ class FileList(object):
     def __len__(self):
         return len(self.file_list)
 
+    @staticmethod
+    def _path_in_tree(root, path):
+        full_path = os.path.realpath(os.path.abspath(
+            os.path.expanduser(path)))
+        if not full_path.startswith(root):
+            logging.debug("Skipping path outside root: %s" % (path,))
+            return False
+        return True
+
     def add(self, file_path):
         """
         Generate a list of files to upload to swift. Recurses through
@@ -143,6 +152,9 @@ class FileList(object):
             relative_path = os.path.basename(file_path)
             file_list.append(FileDetail(file_path, relative_path))
         elif os.path.isdir(file_path):
+            original_root = os.path.realpath(os.path.abspath(
+                os.path.expanduser(file_path)))
+
             parent_dir = os.path.dirname(file_path)
             if not file_path.endswith('/'):
                 filename = os.path.basename(file_path)
@@ -150,6 +162,9 @@ class FileList(object):
                 relative_name = os.path.relpath(full_path, parent_dir)
                 file_list.append(FileDetail(full_path, relative_name,
                                             filename))
+            # TODO: this will copy the result of symlinked files, but
+            # it won't follow directory symlinks.  If we add that, we
+            # should ensure that we don't loop.
             for path, folders, files in os.walk(file_path):
                 # Sort folder in-place so that we recurse in order.
                 files.sort(key=lambda x: x.lower())
@@ -158,16 +173,18 @@ class FileList(object):
                 # and the one being currently walked.
                 relative_path = os.path.relpath(path, parent_dir)
 
-                for f in folders:
+                for filename in folders:
-                    filename = os.path.basename(f)
                     full_path = os.path.join(path, filename)
+                    if not self._path_in_tree(original_root, full_path):
+                        continue
                     relative_name = os.path.relpath(full_path, parent_dir)
                     file_list.append(FileDetail(full_path, relative_name,
                                                 filename))
 
-                for f in files:
+                for filename in files:
-                    filename = os.path.basename(f)
                     full_path = os.path.join(path, filename)
+                    if not self._path_in_tree(original_root, full_path):
+                        continue
                     relative_name = os.path.relpath(full_path, parent_dir)
                     file_detail = FileDetail(full_path, relative_name)
                     file_list.append(file_detail)