Stronger security around test data path resolution, now with tests.

lib/galaxy/tools/__init__.py

@@ -67,6 +67,7 @@ from galaxy.tools.test import parse_tests
 from galaxy.tools.toolbox import BaseGalaxyToolBox
 from galaxy.util import (
     ExecutionTimer,
+    in_directory,
     listify,
     Params,
     rst_to_html,
@@ -856,7 +857,15 @@ class Tool(Dictifiable):
                 if '.' in dirs:
                     dirs.remove('.hg')
                 if 'test-data' in dirs:
-                    return os.path.abspath(os.path.join(root, 'test-data', filename))
+                    test_data_dir = os.path.join(root, 'test-data')
+                    result = os.path.abspath(os.path.join(test_data_dir, filename))
+                    if not in_directory(result, test_data_dir):
+                        # Don't raise an explicit exception and reveal details about what
+                        # files are or are not on the path, simply return None and let the
+                        # API raise a 404.
+                        return None
+                    else:
+                        return result
         else:
             return self.app.test_data_resolver.get_filename(filename)
 

lib/galaxy/tools/verify/test_data.py

@@ -8,6 +8,7 @@ from string import Template
 
 from galaxy.util import (
     asbool,
+    in_directory,
     smart_str
 )
 
@@ -38,24 +39,14 @@ class TestDataResolver(object):
             self.resolvers = []
 
     def get_filename(self, name):
-        if not self.resolvers:
-            filename = None
-        else:
-            resolver = self.resolvers[0]
+        filename = None
+        for resolver in self.resolvers or []:
+            if not resolver.exists(name):
+                continue
             filename = resolver.path(name)
-            if not resolver.exists(filename):
-                for resolver in self.resolvers[1:]:
-                    if resolver.exists(name):
-                        filename = resolver.path(name)
-            else:
-                # For backward compat. returning first path if none
-                # exist - though I don't know if this function is ever
-                # actually used in a context where one should return
-                # a file even if it doesn't exist (e.g. a prefix or
-                # or something) - I am pretty sure it is not used in
-                # such a fashion in the context of tool tests.
-                filename = resolver.path(name)
-        return os.path.abspath(filename)
+
+        if filename:
+            return os.path.abspath(filename)
 
 
 def build_resolver(uri, environ):
@@ -71,7 +62,8 @@ class FileDataResolver(object):
         self.file_dir = file_dir
 
     def exists(self, filename):
-        return os.path.exists(self.path(filename))
+        path = os.path.abspath(self.path(filename))
+        return os.path.exists(path) and in_directory(path, self.file_dir)
 
     def path(self, filename):
         return os.path.join(self.file_dir, filename)

lib/galaxy/webapps/galaxy/api/tools.py

@@ -112,7 +112,11 @@ class ToolsController(BaseAPIController, UsesVisualizationMixin):
             kwd = kwd.get('payload')
         tool_version = kwd.get('tool_version', None)
         tool = self._get_tool(id, tool_version=tool_version, user=trans.user)
-        return tool.test_data_path(kwd.get("filename"))
+        path = tool.test_data_path(kwd.get("filename"))
+        if path:
+            return path
+        else:
+            raise exceptions.ObjectNotFound("Specified test data path not found.")
 
     @expose_api_anonymous_and_sessionless
     def tests_summary(self, trans, **kwd):

test/api/test_tools.py

@@ -135,6 +135,16 @@ class ToolsTestCase(api.ApiTestCase):
         self._assert_has_keys(tool_info, "inputs", "outputs", "panel_section_id")
         return tool_info
 
+    @skip_without_tool("composite_output")
+    def test_test_data_filepath_security(self):
+        test_data_response = self._get("tools/%s/test_data_path?filename=../CONTRIBUTORS.md" % "composite_output", admin=True)
+        assert test_data_response.status_code == 404, test_data_response.json()
+
+    @skip_without_tool("composite_output")
+    def test_test_data_admin_security(self):
+        test_data_response = self._get("tools/%s/test_data_path?filename=../CONTRIBUTORS.md" % "composite_output")
+        assert test_data_response.status_code == 403, test_data_response.json()
+
     @skip_without_tool("composite_output")
     def test_test_data_composite_output(self):
         test_data_response = self._get("tools/%s/test_data" % "composite_output")