Fail lookups on invalid resource names (#69)

* Fail lookups on absolute paths Signed-off-by: Rob Clarke <robert.clarke@bristol.ac.uk> * Update for linter errors Signed-off-by: Rob Clarke <robert.clarke@bristol.ac.uk> * Hopefully final linter fix Signed-off-by: Rob Clarke <robert.clarke@bristol.ac.uk> * Add valid package name check Signed-off-by: Rob Clarke <robert.clarke@bristol.ac.uk> * Linting Signed-off-by: Rob Clarke <robert.clarke@bristol.ac.uk> * Add backslash to invalid chars for resource name Signed-off-by: Rob Clarke <robert.clarke@bristol.ac.uk> * Add validity check for type Signed-off-by: Rob Clarke <robert.clarke@bristol.ac.uk> * Fix quote style Signed-off-by: Rob Clarke <robert.clarke@bristol.ac.uk> * Fix import ordering Signed-off-by: Rob Clarke <robert.clarke@bristol.ac.uk> * Update package name regex to match REP-127 Now allows uppercase letters, dashes and leading numerals. Test modified to reflect this Signed-off-by: Rob Clarke <robert.clarke@bristol.ac.uk> * Add docstrings Signed-off-by: Rob Clarke <robert.clarke@bristol.ac.uk> * Add private API marker Signed-off-by: Rob Clarke <robert.clarke@bristol.ac.uk>
ament · Jun 4, 2021 · e04af7b · e04af7b
1 parent 028080c
commit e04af7b
Show file tree

Hide file tree

Showing 4 changed files with 124 additions and 0 deletions.
diff --git a/ament_index_python/ament_index_python/__init__.py b/ament_index_python/ament_index_python/__init__.py
@@ -22,6 +22,8 @@
 from .resources import get_resource_types
 from .resources import get_resources
 from .resources import has_resource
+from .resources import InvalidResourceNameError
+from .resources import InvalidResourceTypeNameError
 from .search_paths import get_search_paths
 
 __all__ = [
@@ -34,6 +36,8 @@
     'get_search_paths',
     'get_resource_types',
     'has_resource',
+    'InvalidResourceTypeNameError',
+    'InvalidResourceNameError',
     'PackageNotFoundError',
     'RESOURCE_INDEX_SUBFOLDER',
 ]
diff --git a/ament_index_python/ament_index_python/packages.py b/ament_index_python/ament_index_python/packages.py
@@ -14,6 +14,7 @@
 
 import os
 import pathlib
+import re
 
 from .resources import get_resource
 from .resources import get_resources
@@ -45,7 +46,13 @@ def get_package_prefix(package_name):
     :param str package_name: name of the package to locate
     :returns: installation prefix of the package
     :raises: :exc:`PackageNotFoundError` if the package is not found
+    :raises: :exc:`ValueError` if the package name is invalid
     """
+    # This regex checks for a valid package name as defined by REP-127 including the recommended
+    #  exemptions. See https://ros.org/reps/rep-0127.html#name
+    if re.fullmatch('[a-zA-Z0-9][a-zA-Z0-9_-]+', package_name, re.ASCII) is None:
+        raise ValueError(
+            "'{}' is not a valid package name".format(package_name))
     try:
         content, package_prefix = get_resource('packages', package_name)
     except LookupError:
@@ -66,6 +73,7 @@ def get_package_share_directory(package_name):
     :param str package_name: name of the package to locate
     :returns: share directory of the package
     :raises: :exc:`PackageNotFoundError` if the package is not found
+    :raises: :exc:`ValueError` if the package name is invalid
     """
     return os.path.join(get_package_prefix(package_name), 'share', package_name)
 

diff --git a/ament_index_python/ament_index_python/resources.py b/ament_index_python/ament_index_python/resources.py
@@ -19,6 +19,36 @@
 from .search_paths import get_search_paths
 
 
+class InvalidResourceTypeNameError(ValueError):
+    """Raised when a resource type name is invalid."""
+
+    pass
+
+
+class InvalidResourceNameError(ValueError):
+    """Raised when a resource name is invalid."""
+
+    pass
+
+
+def _name_is_invalid(resource_name):
+    """
+    Get the whether a resource name or a resource type name is invalid.
+
+    This is not considered public API.
+
+    A name is considered invalid if it contains any path separators. The index represents resources
+    as files and resource types as folders so allowing path separators causes issues.
+
+    For a more complete discussion, see: https://github.com/ament/ament_index/pull/69
+
+    :param resource_name: the name of the resource or resource type
+    :type resource_name: str
+    :returns: True or False
+    """
+    return ('/' in resource_name) or ('\\' in resource_name)
+
+
 def get_resource(resource_type, resource_name):
     """
     Get the content of a specific resource and its prefix path.
@@ -31,9 +61,17 @@ def get_resource(resource_type, resource_name):
     :raises: :exc:`EnvironmentError`
     :raises: :exc:`OSError`
     :raises: :exc:`LookupError`
+    :raises: :exc:`InvalidResourceTypeNameError`
+    :raises: :exc:`InvalidResourceNameError`
     """
     assert resource_type, 'The resource type must not be empty'
     assert resource_name, 'The resource name must not be empty'
+    if _name_is_invalid(resource_type):
+        raise InvalidResourceTypeNameError(
+            "Resource type '%s' is invalid" % resource_type)
+    if _name_is_invalid(resource_name):
+        raise InvalidResourceNameError(
+            "Resource name '%s' is invalid" % resource_name)
     for path in get_search_paths():
         resource_path = os.path.join(path, RESOURCE_INDEX_SUBFOLDER, resource_type, resource_name)
         if os.path.isfile(resource_path):
@@ -57,8 +95,12 @@ def get_resources(resource_type):
     :type resource_type: str
     :returns: dict of resource names to the prefix path they are in
     :raises: :exc:`EnvironmentError`
+    :raises: :exc:`InvalidResourceTypeNameError`
     """
     assert resource_type, 'The resource type must not be empty'
+    if _name_is_invalid(resource_type):
+        raise InvalidResourceTypeNameError(
+            "Resource type '%s' is invalid" % resource_type)
     resources = {}
     for path in get_search_paths():
         resource_path = os.path.join(path, RESOURCE_INDEX_SUBFOLDER, resource_type)
@@ -103,9 +145,17 @@ def has_resource(resource_type, resource_name):
     :type resource_name: str
     :returns: The prefix path if the resource exists, False otherwise
     :raises: :exc:`EnvironmentError`
+    :raises: :exc:`InvalidResourceTypeNameError`
+    :raises: :exc:`InvalidResourceNameError`
     """
     assert resource_type, 'The resource type must not be empty'
     assert resource_name, 'The resource name must not be empty'
+    if _name_is_invalid(resource_type):
+        raise InvalidResourceTypeNameError(
+            "Resource type '%s' is invalid" % resource_type)
+    if _name_is_invalid(resource_name):
+        raise InvalidResourceNameError(
+            "Resource name '%s' is invalid" % resource_name)
     for path in get_search_paths():
         resource_path = os.path.join(path, RESOURCE_INDEX_SUBFOLDER, resource_type, resource_name)
         if os.path.isfile(resource_path):

diff --git a/ament_index_python/test/test_ament_index_python.py b/ament_index_python/test/test_ament_index_python.py
@@ -24,6 +24,8 @@
 from ament_index_python import get_resources
 from ament_index_python import get_search_paths
 from ament_index_python import has_resource
+from ament_index_python import InvalidResourceNameError
+from ament_index_python import InvalidResourceTypeNameError
 from ament_index_python import PackageNotFoundError
 from ament_index_python.cli import main
 from ament_index_python.cli import resource_name_completer
@@ -67,6 +69,25 @@ def test_unknown_resources():
     assert len(resources) == 0, 'Expected no resources'
 
 
+def test_invalid_resources():
+    set_ament_prefix_path(['prefix1'])
+
+    invalid_resource_type_names = [
+        '/invalid/name', 'invalid/name', '\\invalid\\name', 'invalid\\name']
+
+    for name in invalid_resource_type_names:
+        with pytest.raises(InvalidResourceTypeNameError):
+            resources = get_resources(name)
+            assert len(resources) == 0, 'Expected no resources'
+
+        with pytest.raises(InvalidResourceTypeNameError):
+            exists = has_resource(name, 'example_resource')
+            assert not exists, 'Resource should not exist'
+
+        with pytest.raises(InvalidResourceTypeNameError):
+            get_resource(name, 'example_resource')
+
+
 def test_resources():
     set_ament_prefix_path(['prefix1'])
     resources = get_resources('resource_type1')
@@ -97,6 +118,33 @@ def test_unknown_resource():
         get_resource('resource_type4', 'bar')
 
 
+def test_invalid_resource_names():
+    set_ament_prefix_path(['prefix1'])
+
+    invalid_resource_names = [
+        '/invalid/name', 'invalid/name', '\\invalid\\name', 'invalid\\name']
+
+    for name in invalid_resource_names:
+        with pytest.raises(InvalidResourceNameError):
+            exists = has_resource('resource_type4', name)
+            assert not exists, 'Resource should not exist'
+
+        with pytest.raises(InvalidResourceNameError):
+            get_resource('resource_type4', name)
+
+
+def test_absolute_path_resource():
+    extant_absolute_path = os.path.abspath(__file__)
+
+    set_ament_prefix_path(['prefix1'])
+    with pytest.raises(InvalidResourceNameError):
+        exists = has_resource('resource_type4', str(extant_absolute_path))
+        assert not exists, 'Resource should not exist'
+
+    with pytest.raises(InvalidResourceNameError):
+        get_resource('resource_type4', str(extant_absolute_path))
+
+
 def test_resource():
     set_ament_prefix_path(['prefix1'])
     exists = has_resource('resource_type4', 'foo')
@@ -145,6 +193,17 @@ def get_package_prefix_basename(package_name):
         get_package_prefix('does_not_exist')
     assert issubclass(PackageNotFoundError, KeyError)
 
+    invalid_package_names = [
+        '_package', 'package a', 'package/a', 'package.a']
+    for name in invalid_package_names:
+        with pytest.raises(ValueError):
+            get_package_prefix(name)
+
+    with pytest.raises(ValueError):
+        # An absolue path is not a valid package name
+        extant_absolute_path = os.path.abspath(__file__)
+        get_package_prefix(extant_absolute_path)
+
 
 def test_get_package_share_directory():
     set_ament_prefix_path(['prefix1', 'prefix2'])
@@ -166,6 +225,9 @@ def get_package_share_directory_test(package_name, expect_prefix):
     with pytest.raises(PackageNotFoundError):
         get_package_share_directory('does_not_exist')
 
+    with pytest.raises(ValueError):
+        get_package_share_directory('/invalid/package/name')
+
 
 def test_get_package_share_path():
     set_ament_prefix_path(['prefix1', 'prefix2'])