Opened 7 years ago

Closed 7 years ago

Last modified 4 years ago

#8381 closed (fixed)

Common middleware should use path_info

Reported by: jcassee Owned by: nobody
Component: Uncategorized Version: master
Severity: Keywords: middleware
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:


The CommonMiddleware works its magic on request.path. Since changeset [8015] this should be request.path_info. Trivial patch attached.

Attachments (2)

8381-r8423.diff (964 bytes) - added by jcassee 7 years ago.
8381-r8423-2.diff (959 bytes) - added by jcassee 7 years ago.

Download all attachments as: .zip

Change History (8)

Changed 7 years ago by jcassee

comment:1 Changed 7 years ago by mtredinnick

  • milestone set to 1.0
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

Have you actually tested this on a system with a non-empty SCRIPT_NAME or are you generating these patches just from code auditing? They need to be tested on an actual install before committing. I realise you can't write anything for Django's test suite for this -- such tests would be too fake to actually convey any confidence in affirming real-world behviour -- but can you at least confirm you've tested it manually.

comment:2 Changed 7 years ago by jcassee

Yes, I am using these changes on my own Django installation. The website uses my localeurl application that modifies request.path_info to strip a language prefix, so I run into these things. :-) This patch (and the one from #8376) work for me.

comment:3 Changed 7 years ago by mtredinnick

  • Patch needs improvement set

This patch doesn't seem to be completely correct yet. The problem is when the new URL is constructed, it doesn't include the script name portion any longer. So the redirect (and any other uses of it) will be to the wrong location. Somewhere in the construction of newurl I would expect to see request.META['SCRIPT_NAME'], or, even better, a reuse of request.path once you've determined it does need a trailing slash added.

I suspect the solution might actually be as simple as not making the change on line 46 and only needing to make the one on line 57. But could you check that out, please? If you get to the redirect line, it looks like your current patch will lose the script name in the redirect. That shouldn't happen.

comment:4 Changed 7 years ago by jcassee

  • Patch needs improvement unset

You are correct. I was testing the patch on my non-SCRIPT_NAME testing system. Thanks for your sharp eye!

Your solution is almost correct, I just had to fix the resolve check for the new url. I have updated the patch and tested it on my SCRIPT_NAME-enabled testing system.

Changed 7 years ago by jcassee

comment:5 Changed 7 years ago by mtredinnick

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

(In [8456]) Fixed #8381 -- Fixed a problem with appending slashes in the common middleware
when SCRIPT_NAME contains something other than '/'. Patch from jcassee.

Also fixed the middleware tests to work with this patch.

comment:6 Changed 4 years ago by jacob

  • milestone 1.0 deleted

Milestone 1.0 deleted

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