[Django] #26812: APPEND_SLASH not working correctly with URL ending with slash
Django
-------------------------------+--------------------
Reporter: melwas | Owner: nobody
Type: Uncategorized | Status: new
Component: HTTP handling | Version: 1.9
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------
Hi,
In ''middleware/common.py'' there is a code, which checks whether the
slash should be appended to URL.
{{{
def should_redirect_with_slash(self, request):
"""
Return True if settings.APPEND_SLASH is True and appending a slash
to
the request path turns an invalid path into a valid one.
"""
if settings.APPEND_SLASH and not
request.get_full_path().endswith('/'):
urlconf = getattr(request, 'urlconf', None)
return (
not urlresolvers.is_valid_path(request.path_info, urlconf)
and urlresolvers.is_valid_path('%s/' % request.path_info,
urlconf)
)
return False
}}}
In most cases it works fine, but it fails to append slash, when used with
LOGIN_URL without slash and with "next" parameter. For example:
It works fine:'' /accounts/login/?next=/polls/3/'' -> LOGIN_URL =
''/accounts/login/''
But this one returns 404: ''/accounts/login?next=/polls/3/'' -> LOGIN_URL
= ''/accounts/login (no trailing slash).''
It is because: ''request.get_full_path().endswith('/')'' checks also query
string and therefore returns true and the slash is never appended.
--
Ticket URL: <https://code.djangoproject.com/ticket/26812>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: melwas | Owner: nobody
Type: Uncategorized | Status: new
Component: HTTP handling | Version: 1.9
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
It seems odd to have `LOGIN_URL = /accounts/login` (no trailing slash) but
`APPEND_SLASH = True`. Do you have a practical use case for that
configuration?
--
Ticket URL: <https://code.djangoproject.com/ticket/26812#comment:1>
Django
-------------------------------+--------------------------------------
Reporter: melwas | Owner: nobody
Type: Uncategorized | Status: closed
Component: HTTP handling | Version: 1.9
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* status: new => closed
* resolution: => wontfix
Comment:
I'm not convinced that the complexity of making `APPEND_SLASH` work with
URLs that have query strings is worth it. Feel free to explain the use
case is more detail.
--
Ticket URL: <https://code.djangoproject.com/ticket/26812#comment:2>
Django
Reporter: Andrzej Winnicki | Owner: nobody
Type: Uncategorized | Status: new
Component: HTTP handling | Version: 1.10
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Sven Engström):
* status: closed => new
* version: 1.9 => 1.10
* resolution: wontfix =>
Comment:
The problem is the inconsistency in CommonMiddleware between
`should_redirect_with_slash` and `get_full_path_with_slash` (with
`force_append_slash=True`) where the first checks if the url including
query string ends with a slash and the later adds a slash between the path
and the query string.
In my opinion the best way to solve this is to replace
`request.get_full_path().endswith('/')` with
`request.path_info.endswith('/')` in `should_redirect_with_slash`.
--
Ticket URL: <https://code.djangoproject.com/ticket/26812#comment:3>
Django
----------------------------------+--------------------------------------
Type: Uncategorized | Status: new
Component: HTTP handling | Version: 1.10
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Sven Engström):
I wrote a patch and opened a pull request that does what I mentioned
earlier even though this ticket isn't accepted yet. Feel free to close the
request if this ticket is rejected.
--
Ticket URL: <https://code.djangoproject.com/ticket/26812#comment:4>
Django
Reporter: Andrzej Winnicki | Owner: nobody
Component: HTTP handling | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Simon Charette):
* has_patch: 0 => 1
* version: 1.10 => master
* type: Uncategorized => Bug
* stage: Unreviewed => Ready for checkin
Comment:
Patch LGTM. I think it's worth shipping as relying on `path_info` makes
more sense there.
--
Ticket URL: <https://code.djangoproject.com/ticket/26812#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: Andrzej Winnicki | Owner: nobody
Component: HTTP handling | Version: master
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"f46a838efc9482c9c7834a83a152b57a56888fff" f46a838e]:
{{{
#!CommitTicketReference repository=""
revision="f46a838efc9482c9c7834a83a152b57a56888fff"
Fixed #26812 -- Fixed APPEND_SLASH on a URL including querystring with a
trailing slash.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/26812#comment:6>