Merge pull request #7983 from minrk/contents-escape-root

contents: double check that os_path is always within root

IPython/html/services/contents/fileio.py

@@ -96,8 +96,16 @@ class FileManagerMixin(object):
         -------
         path : string
             Native, absolute OS path to for a file.
+
+        Raises
+        ------
+        404: if path is outside root
         """
-        return to_os_path(path, self.root_dir)
+        root = os.path.abspath(self.root_dir)
+        os_path = to_os_path(path, root)
+        if not (os.path.abspath(os_path) + os.path.sep).startswith(root):
+            raise HTTPError(404, "%s is outside root contents directory" % path)
+        return os_path
 
     def _read_notebook(self, os_path, as_version=4):
         """Read a notebook from an os path."""

IPython/html/services/contents/filemanager.py

@@ -373,10 +373,11 @@ class FileContentsManager(FileManagerMixin, ContentsManager):
         if 'content' not in model and model['type'] != 'directory':
             raise web.HTTPError(400, u'No file content provided')
 
-        self.run_pre_save_hook(model=model, path=path)
-
         os_path = self._get_os_path(path)
         self.log.debug("Saving %s", os_path)
+
+        self.run_pre_save_hook(model=model, path=path)
+
         try:
             if model['type'] == 'notebook':
                 nb = nbformat.from_dict(model['content'])

IPython/html/services/contents/tests/test_manager.py

@@ -5,6 +5,7 @@ from __future__ import print_function
 import os
 import sys
 import time
+from contextlib import contextmanager
 
 from nose import SkipTest
 from tornado.web import HTTPError
@@ -164,7 +165,17 @@ class TestContentsManager(TestCase):
 
     def tearDown(self):
         self._temp_dir.cleanup()
-
+    
+    @contextmanager
+    def assertRaisesHTTPError(self, status, msg=None):
+        msg = msg or "Should have raised HTTPError(%i)" % status
+        try:
+            yield
+        except HTTPError as e:
+            self.assertEqual(e.status_code, status)
+        else:
+            self.fail(msg)
+    
     def make_dir(self, api_path):
         """make a subdirectory at api_path
         
@@ -461,4 +472,29 @@ class TestContentsManager(TestCase):
         cm.mark_trusted_cells(nb, path)
         cm.check_and_sign(nb, path)
         assert cm.notary.check_signature(nb)
+    
+    def test_escape_root(self):
+        cm = self.contents_manager
+        # make foo, bar next to root
+        with open(os.path.join(cm.root_dir, '..', 'foo'), 'w') as f:
+            f.write('foo')
+        with open(os.path.join(cm.root_dir, '..', 'bar'), 'w') as f:
+            f.write('bar')
+        
+        with self.assertRaisesHTTPError(404):
+            cm.get('..')
+        with self.assertRaisesHTTPError(404):
+            cm.get('foo/../../../bar')
+        with self.assertRaisesHTTPError(404):
+            cm.delete('../foo')
+        with self.assertRaisesHTTPError(404):
+            cm.rename('../foo', '../bar')
+        with self.assertRaisesHTTPError(404):
+            cm.save(model={
+                'type': 'file',
+                'content': u'',
+                'format': 'text',
+            }, path='../foo')
+        
+