From 62e4a5d07246599975aa42dfbca453ad2e316505 Mon Sep 17 00:00:00 2001
From: Min RK <benjaminrk@gmail.com>
Date: Thu, 5 Mar 2015 16:42:40 -0800
Subject: [PATCH] contents: double check that os_path is always within root

check based on tornado `StaticFileHandler.validate_absolute_path`

I haven't been able to actually escape root, but we should have this check anyway.
---
 IPython/html/services/contents/fileio.py      | 10 ++++-
 IPython/html/services/contents/filemanager.py |  5 ++-
 .../services/contents/tests/test_manager.py   | 38 ++++++++++++++++++-
 3 files changed, 49 insertions(+), 4 deletions(-)

diff --git a/IPython/html/services/contents/fileio.py b/IPython/html/services/contents/fileio.py
index 77c39c2f5..0f7ff5c7a 100644
--- a/IPython/html/services/contents/fileio.py
+++ b/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."""
diff --git a/IPython/html/services/contents/filemanager.py b/IPython/html/services/contents/filemanager.py
index f775ba89f..01ce07b64 100644
--- a/IPython/html/services/contents/filemanager.py
+++ b/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'])
diff --git a/IPython/html/services/contents/tests/test_manager.py b/IPython/html/services/contents/tests/test_manager.py
index ad64ada0b..840d10db0 100644
--- a/IPython/html/services/contents/tests/test_manager.py
+++ b/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')
+        
+