Opened 7 years ago

Closed 4 years ago

#9805 closed Bug (wontfix)

reverse() does not add SCRIPT_NAME to the returned URL if called from middleware modules

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

Description (last modified by mtredinnick)

For example, take the following middleware.py file, with one example middleware thing that redirects you to the login page if you are not logged in:

login_url=reverse('login')

class LoginMiddleware():
        def process_request(self, request):             
                if(request.user.is_anonymous() and request.path!=login_url):
                        #force login
                        return HttpResponseRedirect(login_url+'?next='+request.path)

In this case, login_url will be correct except for the fact that it will be missing the SCRIPT_NAME. Putting the reverse() call inside the process_request function solves the problem, but it makes no sense at all to call that reverse function for every request when it can be called once at server startup.

The cause is in django.core.handles.wsgi and django.core.handlers.modpython

if self._request_middleware is None:
   self.load_middleware()

set_script_prefix(req.get_options().get('django.root', ''))

As you can see, "set_script_prefix" is called AFTER middleware is loaded, so the script name is still unset when reverse() is called in the middleware above. Moving the set_script_prefix call above the load_middleware function solves the problem.

Attachments (1)

reverse.diff (1.5 KB) - added by ElliottM 7 years ago.
Patch that fixes the error

Download all attachments as: .zip

Change History (9)

Changed 7 years ago by ElliottM

Patch that fixes the error

comment:1 Changed 7 years ago by ElliottM

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Summary changed from reverse() does not add SCRIPT_NAME to the returned URL if called from modules that contain middleware to reverse() does not add SCRIPT_NAME to the returned URL if called from middleware modules

comment:2 follow-up: Changed 7 years ago by mtredinnick

  • Description modified (diff)
  • Owner changed from nobody to mtredinnick

(Fixed description formatting.)

This isn't really fixing the problem, so much as hiding it. Loading the middlewares should not require access to SCRIPT_NAME (it can change between requests and the middleware is only loaded once). So the code you give actually contains a small bug -- probably not observable for something like the "logout" view, but a bug nonetheless, as it could reverse to a different URL for a different request.

There's a metnion in another ticket that we might add a kind of "lazy" reverse() that resolves upon usage, rather than declaration. When I find that ticket, I'll drop in a comment here and wontfix this one. Leaving open for now to remind me to hunt down the other case.

comment:3 Changed 7 years ago by ElliottM

In this particular case, my only reason for moving the reverse() call out to the middleware loading area is because unlike that example, I actually have several urls that I want to avoid redirecting on, and it just feels dumb to have to reverse those several urls in every single request, and then test them one by one, when i can just put them in a set at loading time and test for membership in that set. So a lazy reverse function wouldn't exactly help me.

It didn't occur to me when I was writing the patch that the exact same site could be used with different script_names though.

comment:4 Changed 7 years ago by jacob

  • Triage Stage changed from Unreviewed to Accepted

comment:5 Changed 6 years ago by darkpixel

mtredinnick: I think you are talking about ticket #5925

comment:6 Changed 4 years ago by lukeplant

  • Severity set to Normal
  • Type set to Bug

comment:7 in reply to: ↑ 2 Changed 4 years ago by aaugustin

  • Easy pickings unset
  • UI/UX unset

Replying to mtredinnick:

There's a mention in another ticket that we might add a kind of "lazy" reverse() that resolves upon usage, rather than declaration. When I find that ticket, I'll drop in a comment here and wontfix this one.

lazy_reverse was committed at r16121.

comment:8 Changed 4 years ago by julien

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

Closing as advised in comment:2. reverse_lazy should be used in this case.

Note: See TracTickets for help on using tickets.
Back to Top