From 2826897cd652edb628709a865f5e870b5bf59f27 Mon Sep 17 00:00:00 2001
From: Michael Hanselmann <hansmi@google.com>
Date: Mon, 5 Nov 2012 19:12:51 +0100
Subject: [PATCH] utils.io: Improve handling of double and single slashes
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Up until now β€œIsBelowDir("/", …)” would never return True. The reason
was that an additional slash was added to the root path resulting in
β€œ//", which is β€œimplementation-defined” in posix and treated specially
by β€œos.path.normpath”.

This patch fixes the behaviour for this special case and adds tests
(also for IsNormAbsPath). A typo in the docstring is fixed. Calls to
β€œassert_” and β€œassertFalse” are changed to pass a message by keyword
argument.

It is a bit of a mess, but I hope the resulting behaviour is correct.

Signed-off-by: Michael Hanselmann <hansmi@google.com>
Reviewed-by: Dato SimΓ³ <dato@google.com>
---
 lib/utils/io.py                  | 21 ++++++++++++++++----
 test/ganeti.utils.io_unittest.py | 34 ++++++++++++++++++++++++++++++--
 2 files changed, 49 insertions(+), 6 deletions(-)

diff --git a/lib/utils/io.py b/lib/utils/io.py
index bb34b11e2..bb0a1237c 100644
--- a/lib/utils/io.py
+++ b/lib/utils/io.py
@@ -647,15 +647,28 @@ def IsNormAbsPath(path):
 def IsBelowDir(root, other_path):
   """Check whether a path is below a root dir.
 
-  This works around the nasty byte-byte comparisation of commonprefix.
+  This works around the nasty byte-byte comparison of commonprefix.
 
   """
   if not (os.path.isabs(root) and os.path.isabs(other_path)):
     raise ValueError("Provided paths '%s' and '%s' are not absolute" %
                      (root, other_path))
-  prepared_root = "%s%s" % (os.path.normpath(root), os.sep)
-  return os.path.commonprefix([prepared_root,
-                               os.path.normpath(other_path)]) == prepared_root
+
+  norm_other = os.path.normpath(other_path)
+
+  if norm_other == os.sep:
+    # The root directory can never be below another path
+    return False
+
+  norm_root = os.path.normpath(root)
+
+  if norm_root == os.sep:
+    # This is the root directory, no need to add another slash
+    prepared_root = norm_root
+  else:
+    prepared_root = "%s%s" % (norm_root, os.sep)
+
+  return os.path.commonprefix([prepared_root, norm_other]) == prepared_root
 
 
 def PathJoin(*args):
diff --git a/test/ganeti.utils.io_unittest.py b/test/ganeti.utils.io_unittest.py
index 458ecabf1..cffea24de 100755
--- a/test/ganeti.utils.io_unittest.py
+++ b/test/ganeti.utils.io_unittest.py
@@ -630,10 +630,10 @@ class TestIsNormAbsPath(unittest.TestCase):
   def _pathTestHelper(self, path, result):
     if result:
       self.assert_(utils.IsNormAbsPath(path),
-          "Path %s should result absolute and normalized" % path)
+          msg="Path %s should result absolute and normalized" % path)
     else:
       self.assertFalse(utils.IsNormAbsPath(path),
-          "Path %s should not result absolute and normalized" % path)
+          msg="Path %s should not result absolute and normalized" % path)
 
   def testBase(self):
     self._pathTestHelper("/etc", True)
@@ -642,6 +642,17 @@ class TestIsNormAbsPath(unittest.TestCase):
     self._pathTestHelper("/etc/../root", False)
     self._pathTestHelper("/etc/", False)
 
+  def testSlashes(self):
+    # Root directory
+    self._pathTestHelper("/", True)
+
+    # POSIX' "implementation-defined" double slashes
+    self._pathTestHelper("//", True)
+
+    # Three and more slashes count as one, so the path is not normalized
+    for i in range(3, 10):
+      self._pathTestHelper("/" * i, False)
+
 
 class TestIsBelowDir(unittest.TestCase):
   """Testing case for IsBelowDir"""
@@ -673,6 +684,25 @@ class TestIsBelowDir(unittest.TestCase):
     self.assertRaises(ValueError, utils.IsBelowDir, "/a/b/c", "d")
     self.assertRaises(ValueError, utils.IsBelowDir, "a/b/c", "/d")
     self.assertRaises(ValueError, utils.IsBelowDir, "a/b/c", "d")
+    self.assertRaises(ValueError, utils.IsBelowDir, "", "/")
+    self.assertRaises(ValueError, utils.IsBelowDir, "/", "")
+
+  def testRoot(self):
+    self.assertFalse(utils.IsBelowDir("/", "/"))
+
+    for i in ["/a", "/tmp", "/tmp/foo/bar", "/tmp/"]:
+      self.assertTrue(utils.IsBelowDir("/", i))
+
+  def testSlashes(self):
+    # In POSIX a double slash is "implementation-defined".
+    self.assertFalse(utils.IsBelowDir("//", "//"))
+    self.assertFalse(utils.IsBelowDir("//", "/tmp"))
+    self.assertTrue(utils.IsBelowDir("//tmp", "//tmp/x"))
+
+    # Three (or more) slashes count as one
+    self.assertFalse(utils.IsBelowDir("/", "///"))
+    self.assertTrue(utils.IsBelowDir("/", "///tmp"))
+    self.assertTrue(utils.IsBelowDir("/tmp", "///tmp/a/b"))
 
 
 class TestPathJoin(unittest.TestCase):
-- 
GitLab