Code

Opened 5 years ago

Closed 5 years ago

Last modified 3 years ago

#9923 closed (invalid)

Exceptions in middleware loading fail silently

Reported by: jaylett Owned by: nobody
Component: Core (Other) Version: 1.0
Severity: Keywords:
Cc: james@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: UI/UX:

Description

If the loading of a piece of middleware fails (either due to failure of the import of the middleware module, or an ImproperlyConfigured exception raised by Django itself), the exception will be pushed up to the handler (wsgi, modpython or whatever), which will generally speaking interpret it as an exception from within *request processing*, rather than framework initialisation. However the handlers simply check that self._response_middleware is not None to decide whether to load middleware, which it will no longer be. So on the *next* request, middleware loading will not be performed, and the request will continue as normal -- but without the failing middleware or any middleware further down the list having been loaded or registered. It's not difficult to imagine security issues coming out of this, although they should only bite in the absence of rigorous testing and review; however it can also cause subtle bugs when dealing with complex middleware which cause considerable pain in development.

One solution would be to ensure that on an exception being raised from within core.handlers.base.BaseHandler.load_middleware() self._response_middleware is set back to None, and the exception logged usefully. This would allow reloading to work.

I've attached a patch which is more aggressive than this: it bombs out (using os._exit) having printed the stack trace. In a development situation this probably doesn't make a huge amount of difference, but it may trigger more obvious warnings in a deployed system. Changing this patch to set self._response_middleware to None would be trivial in any case.

Attachments (1)

django.patch (3.8 KB) - added by jaylett 5 years ago.
Patch to bomb out on exception during middleware loading

Download all attachments as: .zip

Change History (6)

Changed 5 years ago by jaylett

Patch to bomb out on exception during middleware loading

comment:1 Changed 5 years ago by jaylett

  • Cc james@… added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 5 years ago by jacob

  • milestone set to 1.1
  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Accepted

The attached patch isn't the correct approach at all -- there's almost never a good reason to use os.exit, and never a good reason to use os._exist -- but this is indeed a bug.

comment:3 Changed 5 years ago by jacob

  • Resolution set to invalid
  • Status changed from new to closed

Actually, upon further investigation, this bug report is incorrect. The bug report states that "if the loading of a piece of middleware fails ... the exception will be pushed up to the handler [which will] simply check that self._response_middleware is not None .. which it will no longer be.

However, looking at the code, that's not true; load_middleware() essentially does:

        self._request_middleware = None
        request_middleware = []

        for middleware_path in settings.MIDDLEWARE_CLASSES:
            # ... load the middleware in middleware_path

            if hasattr(mw_instance, 'process_request'):
                request_middleware.append(mw_instance.process_request)

        self._request_middleware = request_middleware

So if the loading raises an exception the loop will never complete and self._request_middleware will remain None.

comment:4 Changed 5 years ago by jaylett

The original report wasn't invalid, because that code only went into place in [10036]. However on visual inspection it looks like this bug is no longer be present in trunk, and hence will not be present in the 1.1 release.

comment:5 Changed 3 years ago by jacob

  • milestone 1.1 deleted

Milestone 1.1 deleted

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.