django-commons / django-debug-toolbar

A configurable set of panels that display various debug information about the current request/response.
https://django-debug-toolbar.readthedocs.io
BSD 3-Clause "New" or "Revised" License
8.08k stars 1.05k forks source link

Make toolbar compatible with `FORCE_SCRIPT_NAME` #1970

Closed dmartin closed 2 months ago

dmartin commented 3 months ago

Description

Previously, if a project used the FORCE_SCRIPT_NAME setting (such as when hosting a Django application under a subdirectory path via a reverse proxy), is_toolbar_request() would always return False.

This is because request.resolver_match is not yet available in the middleware context, so django.urls.resolve() is called instead. resolve() then raised Resolver404, because it does not take FORCE_SCRIPT_NAME into account.

This caused internal toolbar URLs to be inspected, leading to a request loop when refreshing the history panel.

This PR makes calls to django.urls.resolve() take request.path_info rather than request.path. This effectively strips the request's leading SCRIPT_NAME if one is present.

Fixes #1961

Checklist:

dmartin commented 3 months ago

Could you try using request.path_info instead of request.path? The former should do the right thing if Django isn't mounted at '/' without having to use the script prefix explicitly. (I haven't tested it myself.)

Oh, that does work and is a lot cleaner! Thanks for the suggestion. I can make that change, but what would you like to do with the existing tests that just override request.path? For example:

@override_settings(ROOT_URLCONF="tests.urls_invalid")
    def test_is_toolbar_request_override_request_urlconf(self):
        """Test cases when the toolbar URL is configured on the request."""
        self.request.path = "/__debug__/render_panel/"
        self.assertFalse(self.toolbar.is_toolbar_request(self.request))

I can also add matching request.path_info overrides to each of these, or I suppose they could be outright replaced with request.path_info overrides for all tests except mine which is explicitly testing the behavior when they differ.

tim-schilling commented 3 months ago

@dmartin could we switch them with a RequestFactory().get('/__debug__/render_panel/') request?

dmartin commented 3 months ago

I made those changes, though I didn't rework every test to use a RequestFactory to keep the scope of the PR under control, only the tests that were manually setting request.path.

matthiask commented 2 months ago

Thank you!