Opened 8 years ago

Closed 8 years ago

#26812 closed Bug (fixed)

APPEND_SLASH doesn't work with URLs that have query strings

Reported by: Andrzej Winnicki Owned by: nobody
Component: HTTP handling Version: dev
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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.

Change History (6)

comment:1 by Tim Graham, 8 years ago

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?

comment:2 by Tim Graham, 8 years ago

Resolution: wontfix
Status: newclosed
Summary: APPEND_SLASH not working correctly with URL ending with slashAPPEND_SLASH doesn't work with URLs that have query strings

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.

comment:3 by Sven Engström, 8 years ago

Resolution: wontfix
Status: closednew
Version: 1.91.10

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.

comment:4 by Sven Engström, 8 years ago

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.

Version 0, edited 8 years ago by Sven Engström (next)

comment:5 by Simon Charette, 8 years ago

Has patch: set
Triage Stage: UnreviewedReady for checkin
Type: UncategorizedBug
Version: 1.10master

Patch LGTM. I think it's worth shipping as relying on path_info makes more sense there.

comment:6 by Tim Graham <timograham@…>, 8 years ago

Resolution: fixed
Status: newclosed

In f46a838e:

Fixed #26812 -- Fixed APPEND_SLASH on a URL including querystring with a trailing slash.

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