Merge lp:~adeuring/charmworld/check-symlinks into lp:~juju-jitsu/charmworld/trunk

[Abel Deuring]

CharmFileSet.save_file(): Check if the target of a symlink exists and if the target is within the charm.

This branch fixes bug 1172747: File ingestion assumes symlinks are not broken.

I added two checks to CharmFileSet.save_file(): That the target of a symlink exists and that it is not outside the charm's root directory.

If such a bad symlink is found, a warning is logged, and no data is stored for this file.

[Curtis Hovey (sinzui)]

Thank you.

review: Approve (code)

=== modified file 'charmworld/jobs/ingest.py'
--- charmworld/jobs/ingest.py	2013-04-30 20:11:59 +0000
+++ charmworld/jobs/ingest.py	2013-05-03 15:54:12 +0000
@@ -112,7 +112,7 @@
         """
         self.log.info('Storing files of branch into gridfs')
         filestore = CharmFileSet.save_files(
-            self.fs, charm_data, charm_data['branch_dir'])
+            self.fs, charm_data, charm_data['branch_dir'], self.log)
         self.log.info('Completed gridfs storage.')
         return filestore
 

=== modified file 'charmworld/models.py'
--- charmworld/models.py	2013-04-26 20:33:30 +0000
+++ charmworld/models.py	2013-05-03 15:54:12 +0000
@@ -6,8 +6,10 @@
 import logging
 from os import listdir
 from os.path import dirname
+from os.path import exists
 from os.path import isdir
 from os.path import join
+from os.path import realpath
 import random
 import re
 
@@ -276,7 +278,8 @@
         )
 
     @classmethod
-    def save_file(cls, fs, charm_data, filename, local_directory, subdir=None):
+    def save_file(cls, fs, logger, charm_data, filename, local_directory,
+                  subdir=None):
         """Do the actual storage of the file contents.
 
         :param fs: The mongo fs connection to use.
@@ -285,7 +288,23 @@
         :param directory: The directory is this file located in on disk.
 
         """
-
+        charm_root = realpath(local_directory)
+        if subdir is not None:
+            file_path = join(local_directory, subdir, filename)
+        else:
+            file_path = join(local_directory, filename)
+        real_file_path = realpath(file_path)
+        if not real_file_path.startswith(charm_root):
+            logger.warn(
+                "Invalid file path: Path %s resolves to %s, which is not "
+                "inside the charm's root directory %s" % (
+                    file_path, real_file_path, charm_root))
+            return
+        if not exists(real_file_path):
+            logger.warn(
+                "File does not exist: %s (resolving to %s)." % (
+                    file_path, real_file_path))
+            return
         if subdir:
             complete_filename = join(subdir, filename)
         else:
@@ -334,7 +353,7 @@
         return cf
 
     @classmethod
-    def save_files(cls, fs, charm_data, charm_bzr_path):
+    def save_files(cls, fs, charm_data, charm_bzr_path, logger):
         """Given a bzr branch, load the files we care about."""
         root_files = listdir(charm_bzr_path)
         stored = []
@@ -348,12 +367,14 @@
                     found_filename = filename
 
             if found_filename:
-                stored.append(
-                    cls.save_file(
-                        fs,
-                        charm_data,
-                        found_filename,
-                        charm_bzr_path))
+                charm_file = cls.save_file(
+                    fs,
+                    logger,
+                    charm_data,
+                    found_filename,
+                    charm_bzr_path)
+                if charm_file is not None:
+                    stored.append(charm_file)
 
         for directory in cls.store_directories:
             dir_path = join(charm_bzr_path, directory)
@@ -366,10 +387,11 @@
 
             for filename in dir_files:
                 if not isdir(join(dir_path, filename)):
-                    stored.append(
-                        cls.save_file(
-                            fs, charm_data, filename, charm_bzr_path,
-                            subdir=directory))
+                    charm_file = cls.save_file(
+                        fs, logger, charm_data, filename, charm_bzr_path,
+                        subdir=directory)
+                    if charm_file is not None:
+                        stored.append(charm_file)
         return stored
 
 

=== modified file 'charmworld/test_models.py'
--- charmworld/test_models.py	2013-04-29 20:13:31 +0000
+++ charmworld/test_models.py	2013-05-03 15:54:12 +0000
@@ -3,6 +3,8 @@
 # the file LICENSE).
 
 """Unit tests for our application models."""
+import logging
+import os
 from os.path import dirname
 from os.path import join
 
@@ -195,6 +197,11 @@
         'testing/data/sample_charm'
     )
 
+    def setUp(self):
+        super(TestCharmFileSet, self).setUp()
+        self.logger = logging.getLogger(self.__class__.__name__)
+        self.log_handler = self.get_handler(self.logger.name)
+
     def test_fileid_generation(self):
         """A charm must generate an unique fileid."""
         self.assertEqual(
@@ -207,7 +214,8 @@
         CharmFileSet.save_files(
             self.fs,
             self.charm_data,
-            self.charm_path)
+            self.charm_path,
+            self.logger)
 
         # There should be the following files stored into the gridfs
         # filestore.
@@ -238,7 +246,8 @@
         CharmFileSet.save_files(
             self.fs,
             self.charm_data,
-            self.charm_path)
+            self.charm_path,
+            self.logger)
 
         # Verify that the filename of the README is kept.
         found = self.fs.get_last_version('README.md')
@@ -251,7 +260,8 @@
         CharmFileSet.save_files(
             self.fs,
             self.charm_data,
-            self.charm_path)
+            self.charm_path,
+            self.logger)
 
         readme_id = self.base_fileid + quote_key('README.md')
 
@@ -276,7 +286,8 @@
         CharmFileSet.save_files(
             self.fs,
             self.charm_data,
-            self.charm_path)
+            self.charm_path,
+            self.logger)
 
         install_id = self.base_fileid + 'hooks/install'
 
@@ -295,7 +306,50 @@
         files = CharmFileSet.save_files(
             self.fs,
             self.charm_data,
-            path)
+            path,
+            self.logger)
+        self.assertEqual([], files)
+
+    def test_save_file_ignores_symlink_without_target(self):
+        # Symlinks that point to nowhere are not saved.
+        path = self.use_context(temp_dir())
+        link_path = join(path, 'readme')
+        target_name = 'does-not-exist'
+        target_path = join(path, target_name)
+        os.symlink(target_name, link_path)
+        files = CharmFileSet.save_files(
+            self.fs,
+            self.charm_data,
+            path,
+            self.logger)
+        expected = [
+            'File does not exist: %s (resolving to %s).' % (
+                link_path, target_path)
+        ]
+        messages = [rec.msg for rec in self.log_handler.buffer]
+        self.assertEqual(expected, messages)
+        self.assertEqual([], files)
+
+    def test_save_file_ignores_symlink_with_outside_target(self):
+        # Symlinks that point to a location outside the charm's root
+        # directory are also ignored.
+        path = self.use_context(temp_dir())
+        link_path = join(path, 'readme')
+        target_name = '../no-trespassing'
+        target_path = join(dirname(path), 'no-trespassing')
+        os.symlink(target_name, link_path)
+        files = CharmFileSet.save_files(
+            self.fs,
+            self.charm_data,
+            path,
+            self.logger)
+        expected = [
+            "Invalid file path: Path %s resolves to %s, which is not inside "
+            "the charm's root directory %s" % (
+                link_path, target_path, path)
+        ]
+        messages = [rec.msg for rec in self.log_handler.buffer]
+        self.assertEqual(expected, messages)
         self.assertEqual([], files)