address problems

Author: JohananOppongAmoateng

debug_toolbar/panels/profiling.py

@@ -1,4 +1,5 @@
 import cProfile
+import logging
 import os
 import uuid
 from colorsys import hsv_to_rgb
@@ -12,6 +13,8 @@
 from debug_toolbar import settings as dt_settings
 from debug_toolbar.panels import Panel
 
+logger = logging.getLogger(__name__)
+
 
 class FunctionCall:
     def __init__(
@@ -186,14 +189,24 @@ def generate_stats(self, request, response):
         self.stats.calc_callees()
 
         if (
-            root := dt_settings.get_config()["PROFILER_PROFILE_ROOT"]
-        ) and os.path.exists(root):
+            profile_root := dt_settings.get_config()["PROFILER_PROFILE_ROOT"]
+        ) and os.path.exists(profile_root):
             filename = f"{uuid.uuid4().hex}.prof"
-            prof_file_path = os.path.join(root, filename)
-            self.profiler.dump_stats(prof_file_path)
-            self.prof_file_path = signing.dumps(filename)
-
-        root_func = cProfile.label(super().process_request.__code__)
+            prof_file_path = os.path.join(profile_root, filename)
+            try:
+                self.profiler.dump_stats(prof_file_path)
+                self.prof_file_path = signing.dumps(filename)
+            except OSError:
+                logger.error(
+                    "Failed to dump profiling stats to %s",
+                    prof_file_path,
+                    exc_info=True,
+                )
+                # If writing to the file fails, we don't want to break the
+                # whole page.
+
+        code = super().process_request.__code__
+        root_func = (code.co_filename, code.co_firstlineno, code.co_name)
         if root_func in self.stats.stats:
             root = FunctionCall(self.stats, root_func, depth=0)
             func_list = []

debug_toolbar/views.py

@@ -36,6 +36,7 @@ def render_panel(request: HttpRequest) -> JsonResponse:
 
 
 @require_GET
+@login_not_required
 def download_prof_file(request):
     if not (root := dt_settings.get_config()["PROFILER_PROFILE_ROOT"]):
         raise Http404()
@@ -48,12 +49,14 @@ def download_prof_file(request):
     except signing.BadSignature:
         raise Http404() from None
 
-    resolved_path = pathlib.Path(root) / filename
-    if not resolved_path.exists():
+    root_path = pathlib.Path(root).resolve()
+    resolved_path = (root_path / filename).resolve()
+    if not resolved_path.is_relative_to(root_path) or not resolved_path.exists():
         raise Http404()
 
-    response = FileResponse(
-        open(resolved_path, "rb"), content_type="application/octet-stream"
+    return FileResponse(
+        open(resolved_path, "rb"),
+        as_attachment=True,
+        filename=resolved_path.name,
+        content_type="application/octet-stream",
     )
-    response["Content-Disposition"] = f'attachment; filename="{resolved_path.name}"'
-    return response

tests/panels/test_profiling.py

@@ -3,6 +3,7 @@
 import sys
 import tempfile
 import unittest
+from unittest import mock
 
 from django.contrib.auth.models import User
 from django.core import signing
@@ -83,17 +84,16 @@ def test_generate_stats_no_profiler(self):
         response = HttpResponse()
         self.assertIsNone(self.panel.generate_stats(self.request, response))
 
-    @override_settings(
-        DEBUG_TOOLBAR_CONFIG={"PROFILER_PROFILE_ROOT": tempfile.gettempdir()}
-    )
     def test_generate_stats_signed_path(self):
-        response = self.panel.process_request(self.request)
-        self.panel.generate_stats(self.request, response)
-        path = self.panel.prof_file_path
-        self.assertTrue(path)
-        # Check that it's a valid signature
-        filename = signing.loads(path)
-        self.assertTrue(filename.endswith(".prof"))
+        with tempfile.TemporaryDirectory() as tmpdir:
+            with self.settings(DEBUG_TOOLBAR_CONFIG={"PROFILER_PROFILE_ROOT": tmpdir}):
+                response = self.panel.process_request(self.request)
+                self.panel.generate_stats(self.request, response)
+                path = self.panel.prof_file_path
+                self.assertTrue(path)
+                # Check that it's a valid signature
+                filename = signing.loads(path)
+                self.assertTrue(filename.endswith(".prof"))
 
     def test_generate_stats_no_root(self):
         response = self.panel.process_request(self.request)
@@ -112,6 +112,20 @@ def test_generate_stats_no_root_func(self):
         self.panel.generate_stats(self.request, response)
         self.assertNotIn("func_list", self.panel.get_stats())
 
+    @mock.patch("cProfile.Profile.dump_stats")
+    def test_generate_stats_oserror(self, mock_dump_stats):
+        mock_dump_stats.side_effect = OSError
+        with tempfile.TemporaryDirectory() as tmpdir:
+            with self.settings(DEBUG_TOOLBAR_CONFIG={"PROFILER_PROFILE_ROOT": tmpdir}):
+                response = self.panel.process_request(self.request)
+                with self.assertLogs(
+                    "debug_toolbar.panels.profiling", level="ERROR"
+                ) as cm:
+                    self.panel.generate_stats(self.request, response)
+                self.assertIn("Failed to dump profiling stats", cm.output[0])
+                # Ensure prof_file_path is not set/updated if dump fails
+                self.assertFalse(hasattr(self.panel, "prof_file_path"))
+
 
 @override_settings(
     DEBUG=True, DEBUG_TOOLBAR_PANELS=["debug_toolbar.panels.profiling.ProfilingPanel"]
@@ -172,3 +186,35 @@ def test_download_missing_file(self):
             path = signing.dumps("missing.prof")
             response = self.client.get(url, {"path": path})
             self.assertEqual(response.status_code, 404)
+
+    def test_download_path_traversal(self):
+        with override_settings(
+            DEBUG_TOOLBAR_CONFIG={"PROFILER_PROFILE_ROOT": self.root}
+        ):
+            url = reverse("djdt:debug_toolbar_download_prof_file")
+            # Sign a filename that traverses safely out of the root
+            path = signing.dumps("../passwd")
+            response = self.client.get(url, {"path": path})
+            self.assertEqual(response.status_code, 404)
+
+    def test_download_absolute_path(self):
+        with override_settings(
+            DEBUG_TOOLBAR_CONFIG={"PROFILER_PROFILE_ROOT": self.root}
+        ):
+            url = reverse("djdt:debug_toolbar_download_prof_file")
+            # Create a file outside the root and try to access it via absolute path
+            with tempfile.NamedTemporaryFile() as tmp:
+                path = signing.dumps(tmp.name)
+                response = self.client.get(url, {"path": path})
+                self.assertEqual(response.status_code, 404)
+
+    def test_download_recursive_traversal(self):
+        with override_settings(
+            DEBUG_TOOLBAR_CONFIG={"PROFILER_PROFILE_ROOT": self.root}
+        ):
+            url = reverse("djdt:debug_toolbar_download_prof_file")
+            # Try a convoluted path that resolves outside
+            # e.g. root/subdir/../../outside_root
+            path = signing.dumps(os.path.join("subdir", "..", "..", "passwd"))
+            response = self.client.get(url, {"path": path})
+            self.assertEqual(response.status_code, 404)