Merge pull request #1640 from pypa/bugfix/1635-disallow-parent-paths

Disallow parent path traversal in resource paths, part 1 (deprecation)

4 files changed, 69 insertions, 5 deletions

diff --git a/changelog.d/1635.change.rst b/changelog.d/1635.change.rst
new file mode 100644
index 00000000..d23f3fe3
--- /dev/null
+++ b/changelog.d/1635.change.rst
@@ -0,0 +1 @@
+Resource paths are passed to ``pkg_resources.resource_string`` and similar no longer accept paths that traverse parents, that begin with a leading ``/``. Violations of this expectation raise DeprecationWarnings and will become errors. Additionally, any paths that are absolute on Windows are strictly disallowed and will raise ValueErrors.
diff --git a/docs/pkg_resources.txt b/docs/pkg_resources.txt
index 0c9fb5f2..806f1b14 100644
--- a/docs/pkg_resources.txt
+++ b/docs/pkg_resources.txt
@@ -1132,8 +1132,8 @@ relative to the root of the identified distribution; i.e. its first path
 segment will be treated as a peer of the top-level modules or packages in the
 distribution.
 
-Note that resource names must be ``/``-separated paths and cannot be absolute
-(i.e. no leading ``/``) or contain relative names like ``".."``.  Do *not* use
+Note that resource names must be ``/``-separated paths rooted at the package,
+cannot contain relative names like ``".."``, and cannot be absolute.  Do *not* use
 ``os.path`` routines to manipulate resource paths, as they are *not* filesystem
 paths.
 
diff --git a/pkg_resources/__init__.py b/pkg_resources/__init__.py
index 6ca68daa..dcfa1d08 100644
--- a/pkg_resources/__init__.py
+++ b/pkg_resources/__init__.py
@@ -39,6 +39,8 @@ import tempfile
 import textwrap
 import itertools
 import inspect
+import ntpath
+import posixpath
 from pkgutil import get_importer
 
 try:
@@ -1466,10 +1468,73 @@ class NullProvider:
         )
 
     def _fn(self, base, resource_name):
+        self._validate_resource_path(resource_name)
         if resource_name:
             return os.path.join(base, *resource_name.split('/'))
         return base
 
+    @staticmethod
+    def _validate_resource_path(path):
+        """
+        Validate the resource paths according to the docs.
+        https://setuptools.readthedocs.io/en/latest/pkg_resources.html#basic-resource-access
+
+        >>> warned = getfixture('recwarn')
+        >>> warnings.simplefilter('always')
+        >>> vrp = NullProvider._validate_resource_path
+        >>> vrp('foo/bar.txt')
+        >>> bool(warned)
+        False
+        >>> vrp('../foo/bar.txt')
+        >>> bool(warned)
+        True
+        >>> warned.clear()
+        >>> vrp('/foo/bar.txt')
+        >>> bool(warned)
+        True
+        >>> vrp('foo/../../bar.txt')
+        >>> bool(warned)
+        True
+        >>> warned.clear()
+        >>> vrp('foo/f../bar.txt')
+        >>> bool(warned)
+        False
+
+        Windows path separators are straight-up disallowed.
+        >>> vrp(r'\\foo/bar.txt')
+        Traceback (most recent call last):
+        ...
+        ValueError: Use of .. or absolute path in a resource path \
+is not allowed.
+
+        >>> vrp(r'C:\\foo/bar.txt')
+        Traceback (most recent call last):
+        ...
+        ValueError: Use of .. or absolute path in a resource path \
+is not allowed.
+        """
+        invalid = (
+            os.path.pardir in path.split(posixpath.sep) or
+            posixpath.isabs(path) or
+            ntpath.isabs(path)
+        )
+        if not invalid:
+            return
+
+        msg = "Use of .. or absolute path in a resource path is not allowed."
+
+        # Aggressively disallow Windows absolute paths
+        if ntpath.isabs(path) and not posixpath.isabs(path):
+            raise ValueError(msg)
+
+        # for compatibility, warn; in future
+        # raise ValueError(msg)
+        warnings.warn(
+            msg[:-1] + " and will raise exceptions in a future release.",
+            DeprecationWarning,
+            stacklevel=4,
+        )
+
     def _get(self, path):
         if hasattr(self.loader, 'get_data'):
             return self.loader.get_data(path)
@@ -1888,7 +1953,7 @@ def find_eggs_in_zip(importer, path_item, only=False):
     if only:
         # don't yield nested distros
         return
-    for subitem in metadata.resource_listdir('/'):
+    for subitem in metadata.resource_listdir(''):
         if _is_egg_path(subitem):
             subpath = os.path.join(path_item, subitem)
             dists = find_eggs_in_zip(zipimport.zipimporter(subpath), subpath)
diff --git a/pkg_resources/tests/test_pkg_resources.py b/pkg_resources/tests/test_pkg_resources.py
index 416f9aff..2c2c9c7f 100644
--- a/pkg_resources/tests/test_pkg_resources.py
+++ b/pkg_resources/tests/test_pkg_resources.py
@@ -93,7 +93,6 @@ class TestZipProvider:
 
         expected_root = ['data.dat', 'mod.py', 'subdir']
         assert sorted(zp.resource_listdir('')) == expected_root
-        assert sorted(zp.resource_listdir('/')) == expected_root
 
         expected_subdir = ['data2.dat', 'mod2.py']
         assert sorted(zp.resource_listdir('subdir')) == expected_subdir
@@ -106,7 +105,6 @@ class TestZipProvider:
         zp2 = pkg_resources.ZipProvider(mod2)
 
         assert sorted(zp2.resource_listdir('')) == expected_subdir
-        assert sorted(zp2.resource_listdir('/')) == expected_subdir
 
         assert zp2.resource_listdir('subdir') == []
         assert zp2.resource_listdir('subdir/') == []