Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 16 additions & 2 deletions debug_toolbar/panels/profiling.py
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
import cProfile
import os
import uuid
from colorsys import hsv_to_rgb
from pstats import Stats

from django.conf import settings
from django.core import signing
from django.utils.html import format_html
from django.utils.translation import gettext_lazy as _

Expand Down Expand Up @@ -183,8 +185,15 @@ def generate_stats(self, request, response):
self.stats = Stats(self.profiler)
self.stats.calc_callees()

root_func = cProfile.label(super().process_request.__code__)
if (
root := dt_settings.get_config()["PROFILER_PROFILE_ROOT"]
) and os.path.exists(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)
Comment on lines +188 to +194
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing error handling for profiler.dump_stats(): If the dump_stats call fails (e.g., due to disk full, permissions error, or directory deleted after existence check), the prof_file_path will not be set but no error will be logged or reported. Consider wrapping the dump_stats call in a try-except block to gracefully handle failures and log errors, so the profiling panel continues to work even if file saving fails.

Copilot uses AI. Check for mistakes.

root_func = cProfile.label(super().process_request.__code__)
if root_func in self.stats.stats:
root = FunctionCall(self.stats, root_func, depth=0)
Comment on lines +188 to 198
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Variable name collision: The variable 'root' on line 189 shadows the 'root' variable used on line 198 for the FunctionCall object. This makes the code confusing to read. Consider renaming the root directory variable to something like 'profile_root' or 'root_dir' to avoid confusion.

Copilot uses AI. Check for mistakes.
func_list = []
Expand All @@ -197,4 +206,9 @@ def generate_stats(self, request, response):
dt_settings.get_config()["PROFILER_MAX_DEPTH"],
cum_time_threshold,
)
self.record_stats({"func_list": [func.serialize() for func in func_list]})
self.record_stats(
{
"func_list": [func.serialize() for func in func_list],
"prof_file_path": getattr(self, "prof_file_path", None),
}
)
15 changes: 14 additions & 1 deletion debug_toolbar/panels/sql/tracking.py
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,20 @@ def _last_executed_query(self, sql, params):
# process during the .last_executed_query() call.
self.db._djdt_logger = None
try:
return self.db.ops.last_executed_query(self.cursor, sql, params)
# Handle executemany: take the first set of parameters for formatting
if (
isinstance(params, (list, tuple))
and len(params) > 0
and isinstance(params[0], (list, tuple))
):
sample_params = params[0]
else:
sample_params = params

try:
return self.db.ops.last_executed_query(self.cursor, sql, sample_params)
except Exception:
return sql
Comment on lines +149 to +162
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes to handle executemany parameters appear unrelated to the PR's stated purpose of adding profile file downloads. This change modifies SQL query logging behavior and should be in a separate PR. If this is intentionally included, the PR description should explain why these SQL tracking changes are necessary for the profiling download feature.

Copilot uses AI. Check for mistakes.
finally:
self.db._djdt_logger = self.logger

Expand Down
1 change: 1 addition & 0 deletions debug_toolbar/settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ def _is_running_tests():
"PRETTIFY_SQL": True,
"PROFILER_CAPTURE_PROJECT_CODE": True,
"PROFILER_MAX_DEPTH": 10,
"PROFILER_PROFILE_ROOT": None,
"PROFILER_THRESHOLD_RATIO": 8,
"SHOW_TEMPLATE_CONTEXT": True,
"SKIP_TEMPLATE_PREFIXES": ("django/forms/widgets/", "admin/widgets/"),
Expand Down
4 changes: 4 additions & 0 deletions debug_toolbar/static/debug_toolbar/css/toolbar.css
Original file line number Diff line number Diff line change
Expand Up @@ -1223,3 +1223,7 @@ To regenerate:
#djDebug .djdt-community-panel a:hover {
text-decoration: underline;
}

#djDebug .djdt-profiling-control {
margin-bottom: 10px;
}
39 changes: 24 additions & 15 deletions debug_toolbar/templates/debug_toolbar/panels/profiling.html
Original file line number Diff line number Diff line change
@@ -1,4 +1,13 @@
{% load i18n %}

{% if prof_file_path %}
<div class="djdt-profiling-control">
<a href="{% url 'djdt:debug_toolbar_download_prof_file' %}?path={{ prof_file_path|urlencode }}" class="djDebugButton">
Download .prof file
</a>
</div>
{% endif %}

<table>
<thead>
<tr>
Expand All @@ -13,22 +22,22 @@
<tbody>
{% for call in func_list %}
<tr class="djdt-profile-row {% if call.is_project_func %}djdt-highlighted {% endif %} {% for parent_id in call.parent_ids %} djToggleDetails_{{ parent_id }}{% endfor %}" id="profilingMain_{{ call.id }}">
<td>
<div data-djdt-styles="paddingLeft:{{ call.indent }}px">
{% if call.has_subfuncs %}
<td>
<div data-djdt-styles="paddingLeft:{{ call.indent }}px">
{% if call.has_subfuncs %}
<button type="button" class="djProfileToggleDetails djToggleSwitch" data-toggle-name="profilingMain" data-toggle-id="{{ call.id }}">-</button>
{% else %}
<span class="djNoToggleSwitch"></span>
{% endif %}
<span class="djdt-stack">{{ call.func_std_string|safe }}</span>
</div>
</td>
<td>{{ call.cumtime|floatformat:3 }}</td>
<td>{{ call.cumtime_per_call|floatformat:3 }}</td>
<td>{{ call.tottime|floatformat:3 }}</td>
<td>{{ call.tottime_per_call|floatformat:3 }}</td>
<td>{{ call.count }}</td>
</tr>
{% else %}
<span class="djNoToggleSwitch"></span>
{% endif %}
<span class="djdt-stack">{{ call.func_std_string|safe }}</span>
</div>
</td>
<td>{{ call.cumtime|floatformat:3 }}</td>
<td>{{ call.cumtime_per_call|floatformat:3 }}</td>
<td>{{ call.tottime|floatformat:3 }}</td>
<td>{{ call.tottime_per_call|floatformat:3 }}</td>
<td>{{ call.count }}</td>
</tr>
{% endfor %}
</tbody>
</table>
5 changes: 5 additions & 0 deletions debug_toolbar/toolbar.py
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,11 @@ def get_urls(cls) -> list[URLPattern | URLResolver]:
# Global URLs
urlpatterns = [
path("render_panel/", views.render_panel, name="render_panel"),
path(
"download_prof_file/",
views.download_prof_file,
name="debug_toolbar_download_prof_file",
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

URL name inconsistency: The URL name "debug_toolbar_download_prof_file" is inconsistent with the naming convention used by other URLs in this codebase. Other URLs use simple names without the "debug_toolbar_" prefix (e.g., "render_panel" on line 165, "history_sidebar", "sql_select", "template_source"). Consider renaming to just "download_prof_file" for consistency.

Suggested change
name="debug_toolbar_download_prof_file",
name="download_prof_file",

Copilot uses AI. Check for mistakes.
),
]
# Per-panel URLs
for panel_class in cls.get_panel_classes():
Expand Down
1 change: 1 addition & 0 deletions debug_toolbar/urls.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,5 @@
from debug_toolbar.toolbar import DebugToolbar

app_name = APP_NAME

urlpatterns = DebugToolbar.get_urls()
31 changes: 30 additions & 1 deletion debug_toolbar/views.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,12 @@
from django.http import HttpRequest, JsonResponse
import pathlib

from django.core import signing
from django.http import FileResponse, Http404, HttpRequest, JsonResponse
from django.utils.html import escape
from django.utils.translation import gettext as _
from django.views.decorators.http import require_GET

from debug_toolbar import settings as dt_settings
from debug_toolbar._compat import login_not_required
from debug_toolbar.decorators import render_with_toolbar_language, require_show_toolbar
from debug_toolbar.panels import Panel
Expand All @@ -28,3 +33,27 @@ def render_panel(request: HttpRequest) -> JsonResponse:
content = panel.content
scripts = panel.scripts
return JsonResponse({"content": content, "scripts": scripts})


Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The download_prof_file view is missing authentication and authorization decorators. The render_panel view on line 16-18 uses @login_not_required, @require_show_toolbar, and @render_with_toolbar_language decorators. At minimum, this view should have @login_not_required and @require_show_toolbar decorators to ensure only authorized users can download profile files.

Suggested change
@login_not_required
@require_show_toolbar

Copilot uses AI. Check for mistakes.
@require_GET
def download_prof_file(request):
if not (root := dt_settings.get_config()["PROFILER_PROFILE_ROOT"]):
raise Http404()

if not (file_path := request.GET.get("path")):
raise Http404()

try:
filename = signing.loads(file_path)
except signing.BadSignature:
raise Http404() from None

resolved_path = pathlib.Path(root) / filename
if not resolved_path.exists():
Comment on lines +51 to +52
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Path traversal vulnerability: The code does not validate that the resolved path stays within the configured root directory. An attacker could sign a filename like "../../../etc/passwd" to access files outside the intended directory. Add validation using resolved_path.resolve().is_relative_to(pathlib.Path(root).resolve()) to ensure the file path is within the allowed directory.

Suggested change
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():

Copilot uses AI. Check for mistakes.
raise Http404()

response = FileResponse(
open(resolved_path, "rb"), content_type="application/octet-stream"
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resource leak: The file opened with open(resolved_path, "rb") is never explicitly closed. While FileResponse will eventually close it, it's better to use FileResponse with a path string or use a context manager to ensure proper resource cleanup. Consider using FileResponse(resolved_path, ...) which accepts a path-like object and handles file closing automatically.

Suggested change
open(resolved_path, "rb"), content_type="application/octet-stream"
resolved_path, content_type="application/octet-stream"

Copilot uses AI. Check for mistakes.
)
response["Content-Disposition"] = f'attachment; filename="{resolved_path.name}"'
Comment on lines +56 to +58
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential header injection vulnerability: The filename in the Content-Disposition header is not properly escaped. If a malicious filename contains quotes or newlines, it could lead to header injection attacks. Consider using Django's http.urlquote or properly escaping the filename value, or use Django's FileResponse which can handle this automatically when you pass the filename parameter instead of setting the header manually.

Suggested change
open(resolved_path, "rb"), content_type="application/octet-stream"
)
response["Content-Disposition"] = f'attachment; filename="{resolved_path.name}"'
open(resolved_path, "rb"),
as_attachment=True,
filename=resolved_path.name,
content_type="application/octet-stream",
)

Copilot uses AI. Check for mistakes.
return response
3 changes: 3 additions & 0 deletions docs/changes.rst
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@ Pending
* Added test to confirm Django's ``TestCase.assertNumQueries`` works.
* Fixed string representation of values in settings panel.
* Declared support for Django 6.0.
* Added the ability to download the profiling data as a file. This feature is
disabled by default and requires the ``PROFILER_PROFILE_ROOT`` setting to be
configured.

6.1.0 (2025-10-30)
------------------
Expand Down
11 changes: 11 additions & 0 deletions docs/configuration.rst
Original file line number Diff line number Diff line change
Expand Up @@ -351,6 +351,17 @@ Panel options
This setting affects the depth of function calls in the profiler's
analysis.

* ``PROFILER_PROFILE_ROOT``

Default: ``None``

Panel: profiling

This setting controls the directory where profile files are saved. If set
to ``None`` (the default), the profile file is not saved and the download
link is not shown. This directory must exist and be writable by the
web server process.

* ``PROFILER_THRESHOLD_RATIO``

Default: ``8``
Expand Down
69 changes: 69 additions & 0 deletions tests/panels/test_profiling.py
Original file line number Diff line number Diff line change
@@ -1,10 +1,16 @@
import os
import shutil
import sys
import tempfile
import unittest

from django.contrib.auth.models import User
from django.core import signing
from django.db import IntegrityError, transaction
from django.http import HttpResponse
from django.test import TestCase
from django.test.utils import override_settings
from django.urls import reverse

from debug_toolbar.panels.profiling import ProfilingPanel

Expand Down Expand Up @@ -77,6 +83,24 @@ 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"))
Comment on lines +86 to +96
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test cleanup issue: The test_generate_stats_signed_path creates a .prof file in tempfile.gettempdir() but doesn't clean it up after the test. Consider adding cleanup in a tearDown method or using a temporary directory that gets cleaned up automatically (similar to ProfilingDownloadViewTestCase which uses tempfile.mkdtemp and shutil.rmtree).

Copilot uses AI. Check for mistakes.

def test_generate_stats_no_root(self):
response = self.panel.process_request(self.request)
self.panel.generate_stats(self.request, response)
# Should not have a path if root is not set
self.assertFalse(hasattr(self.panel, "prof_file_path"))

def test_generate_stats_no_root_func(self):
"""
Test generating stats using profiler without root function.
Expand All @@ -103,3 +127,48 @@ def test_view_executed_once(self):
with self.assertRaises(IntegrityError), transaction.atomic():
response = self.client.get("/new_user/")
self.assertEqual(User.objects.count(), 1)


class ProfilingDownloadViewTestCase(TestCase):
def setUp(self):
self.root = tempfile.mkdtemp()
self.filename = "test.prof"
self.filepath = os.path.join(self.root, self.filename)
with open(self.filepath, "wb") as f:
f.write(b"data")
self.signed_path = signing.dumps(self.filename)

def tearDown(self):
shutil.rmtree(self.root)

def test_download_no_root_configured(self):
response = self.client.get(reverse("djdt:debug_toolbar_download_prof_file"))
self.assertEqual(response.status_code, 404)

def test_download_valid(self):
with override_settings(
DEBUG_TOOLBAR_CONFIG={"PROFILER_PROFILE_ROOT": self.root}
):
url = reverse("djdt:debug_toolbar_download_prof_file")
response = self.client.get(url, {"path": self.signed_path})
self.assertEqual(response.status_code, 200)
self.assertEqual(list(response.streaming_content), [b"data"])

def test_download_invalid_signature(self):
with override_settings(
DEBUG_TOOLBAR_CONFIG={"PROFILER_PROFILE_ROOT": self.root}
):
url = reverse("djdt:debug_toolbar_download_prof_file")
# Tamper with the signature
response = self.client.get(url, {"path": self.signed_path + "bad"})
self.assertEqual(response.status_code, 404)

def test_download_missing_file(self):
with override_settings(
DEBUG_TOOLBAR_CONFIG={"PROFILER_PROFILE_ROOT": self.root}
):
url = reverse("djdt:debug_toolbar_download_prof_file")
# Sign a filename that doesn't exist
path = signing.dumps("missing.prof")
response = self.client.get(url, {"path": path})
self.assertEqual(response.status_code, 404)
Comment on lines +132 to +174
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing test for path traversal attack: The test suite should include a test that verifies path traversal attempts (e.g., signing "../../../etc/passwd" or "subdir/../../../etc/passwd") are properly rejected and return 404. This is important for security validation.

Copilot uses AI. Check for mistakes.
Loading