Opened 16 years ago

Closed 16 years ago

Last modified 12 years ago

#8381 closed (fixed)

Common middleware should use path_info

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

Description

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 Joost Cassee 16 years ago.
8381-r8423-2.diff (959 bytes ) - added by Joost Cassee 16 years ago.

Download all attachments as: .zip

Change History (8)

by Joost Cassee, 16 years ago

Attachment: 8381-r8423.diff added

comment:1 by Malcolm Tredinnick, 16 years ago

milestone: 1.0
Triage Stage: UnreviewedAccepted

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 by Joost Cassee, 16 years ago

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 by Malcolm Tredinnick, 16 years ago

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 by Joost Cassee, 16 years ago

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.

by Joost Cassee, 16 years ago

Attachment: 8381-r8423-2.diff added

comment:5 by Malcolm Tredinnick, 16 years ago

Resolution: fixed
Status: newclosed

(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 by Jacob, 12 years ago

milestone: 1.0

Milestone 1.0 deleted

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