APPEND_SLASH doesn't work with DEBUG=False when template/404.html is erroneous

[dong-won kang]

I first uploaded it to google groups but I don't find where's my article is (I'm not get used to it) so I decided to make a ticket here.

as I mentioned in title, django 1.9's url slash redirection doesn't working well in debug=False. APPEND_SLASH was no effect. but in debug=True, it does well, and only these 'cached(loaded in debug mode)' url redirects well even in debug=False. (I don't know it's browser redirection or django's work ...)

anyway, I fixed django/middleware/common.py like this and works well. original code was rather a little strange, as I thought.

    def process_request(self, request):
        """
        Check for denied User-Agents and rewrite the URL based on
        settings.APPEND_SLASH and settings.PREPEND_WWW
        """

        # Check for denied User-Agents
        if 'HTTP_USER_AGENT' in request.META:
            for user_agent_regex in settings.DISALLOWED_USER_AGENTS:
                if user_agent_regex.search(request.META['HTTP_USER_AGENT']):
                    raise PermissionDenied('Forbidden user agent')

        # Check for a redirect based on settings.PREPEND_WWW
        redirect = False
        host = request.get_host()

        if settings.PREPEND_WWW and host and not host.startswith('www.'):
            redirect = True
            host = 'www.' + host

        # Check if we also need to append a slash so we can do it all
        # with a single redirect.
        if self.should_redirect_with_slash(request):
            redirect = True
            path = self.get_full_path_with_slash(request)
        else:
            path = request.get_full_path()

        if (redirect):
            return self.response_redirect_class('%s://%s%s' % (request.scheme, host, path))

Hope this issue can be meaningful.

thanks for reading

PS changed a little - previous code won't work with prepending WWW

[Tim Graham]

Could you be a little more specific with the steps to reproduce the error, perhaps providing a sample project or a regression test for Django? Your suggested changes don't pass the current tests (failures in middleware).

[dong-won kang]

Replying to timgraham:

Could you be a little more specific with the steps to reproduce the error, perhaps providing a sample project or a regression test for Django? Your suggested changes don't pass the current tests (failures in middleware).

My project is ​here and ​currently running. Changed code is applied to my project currently and running well. I don't know well about django's module test or deep hierarchy of django module, so I need to find more about them if I have to fix this problem fully correctly. I'll comment more when I find something new about it.

[dong-won kang]

I did test and look at the code, and I have question to the behavior.

I found failed test test_non_ascii_query_string_does_not_crash , and It requires http://testserver/slash/?drink=caf%C3%A9 to be redirected(301). I think It doesn't need to be, as there's no need to prepend WWW(False default) and redirect with slash.

I think the test should changed to self.assertEqual(r.status_code, None) , not 301.

and one more thing - this bug has occurred since I upgraded django from 1.8.7 to 1.9. In that case, URL without slash returns 500, not 404, and that makes process_response not working I think. I'm inspecting more about this.

thanks for your sincere.

PS. when debug=False, after Not Found URL occured, this exception occurs: TemplateSyntaxError: Could not parse the remainder: '('static', filename='img/404.jpg')' from 'url_for('static', filename='img/404.jpg')' , and that makes response code to 500, not 404.

[dong-won kang]

I found why this occurs: there was erroneous 404.html file in my ./(appname)/templates directory, and that template file was loaded when unslashed URL called which called page_not_found in views/defaults.py . But I think that shouldn't happened as it's unexpected behavior. Also, rendering template even in case of redirection is definitely a resource waste, I think.

That wasn't happened in django 1.8.7.

[Tim Graham]

Can you ​bisect to find the commit where the behavior changed? Could you put together a minimal project that reproduces the problem? I am not sure whether this is a bug in Django or a problem with your application.

[dong-won kang]

It was caused by my projects buggy template, but behavior changed since django version was updated. So It's no use to do git bisect. I'm also confused it's bug or just a simple behavior change...

​Download and unzip this project and check not working APPEND_SLASH when Debug=False with this project under django v1.9.

[Tim Graham]

The change is bisected to 434d309ef6dbecbfd2b322d3a1da78aa5cb05fa8 (#24720). As you said, I think something doesn't seem quite right if we have to render the 404 page before redirecting. Jay, as the author of the original patch could you let us know your thoughts?

[Jay Cox]

Rendering the 404 template before redirection is an unfortunate side effect of the above mentioned patch. Fortunately the overhead of this rendering is very minor compared to the overhead of a client side redirect.

I think the developer overhead of understanding what is happening when things go wrong in the 404 rendering is not nearly so minor.

Ideally the 404 would be caught before the template is rendered, but my recollection is that 404 exceptions are not sent through the process_exception middleware

In my opinion the cleanest solution would be to feed all exceptions through the process_exception middleware, and then modify the common middleware to take advantage of this. There would however be some risk of this interacting badly with existing process_exception middleware.

[Tim Graham]

kuna, your patch might be okay. There are two other test failure in admin_views and i18n, but they might just be tests that need to be adapted. Are you able to convert your patch into a pull request?

[Sanket Sudake (संकेत )]

I was facing same issue after upgrading to Django 1.9. When DEBUG=False redirection was not happening. Applied above patch manually on production system. Helped for me.

Case 1(Issue):
When DEBUG=False,
example.com/sample
Returns 404.

Case 2:
Whereas with DEBUG=True
example.com/sample/
Return 301

Thanks,
Sanket

[Tim Graham]

Got an initial ​PR up, but need to add another test.

[Michael Squashic]

I just upgraded to 1.9 today and am having the same problem. Tested your PR and it works for me fyi

[Tim Graham]

@tripples, @pulsedemon, do you have incorrect 404 templates? I'm not sure it's the same issue because in the original report, with DEBUG=False an incorrect URL will return a 500, not a 404, since the 404 template can't be rendered. If you could provide a sample project to reproduce, that would be helpful.

After understanding the issue a bit more, I think the proposed patch basically reverts #24720 and the benefit it provides. Obviously correct behavior trumps performance, but if an invalid 404 template is the only way to trigger it, then maybe we can find a way to fix this without reverting it.

[Michael Squashic]

@timgraham I did have a problem with my 404 template, which I didn't realize until I saw this ticket. I had a custom 404 handler and it was not setting a 404 status_code. So because it was returning 200, APPEND_SLASH was never activated.
The template itself worked fine, it was just the status_code causing problems.

I think that when I first created the custom handler by pointing handler404 to my custom view, I assumed that the status code would be set automatically and never thought about checking it. But it makes sense that handling the 404 like this would require implementing the status_code as well.

I don't know if this is the best idea, but Django could set the status_code automatically for cases like this.

[Tim Graham]

Thanks for that info. At this point then, I'm inclined to discard my initial patch and instead add a mention in the release notes about this as well as to try to improve the error reporting situation. To address the situation in comment 18, an idea is to log a warning in ​BaseHander.get_exception_response() if the error handler returns a response with the incorrect status code. I haven't thought through if there could be a valid reason for returning a different code, but automatically setting the code could be a bit risky for a patch that's backported to a stable branch. I'll have to look to see if we can do any better about the situation where the 404 template doesn't render correctly as reported in the ticket description.

[Sanket Sudake (संकेत )]

@timgraham I have exact case like @pulsedemon. My 404 handler was returning 200 status code instead of 404. However, I have tried APPEND_SLASH=True and even with APPEND_SLASH=True it was not hitting correct required response. Only difference I was noticing was, with DEBUG=True I was hitting correct response.

1. DEBUG=True

Always gives correct response.

2. DEUBG=FalseAPPEND_SLASH not set. OR DEBUG = False and APPEND_SLASH set.

Returning 404 handler with status code 200.

I will check if I can get sample project for reproducing issue.
Also minor query, does Site models or related settings can have any impact in this issue?

[Tim Graham]

Right, so with 434d309ef6dbecbfd2b322d3a1da78aa5cb05fa8 (#24720) your handler404 (which is only used when DEBUG=False, must return a 404 status code, otherwise APPEND_SLASH won't work correctly. I don't think this is an unreasonable requirement, but it needs to be mentioned in the 1.9 release notes as a backwards-incompatible change (unless someone thinks this needs to go through a deprecation path and thus we should revert the original patch on 1.9). We could add logging as I mentioned in comment 19 to help developers find this mistake in their handler.

[Sanket Sudake (संकेत )]

Thanks, tim. Adding some django checks for these handlers or logging would really help.

[Michael Squashic]

Replying to timgraham:

Right, so with 434d309ef6dbecbfd2b322d3a1da78aa5cb05fa8 (#24720) your handler404 (which is only used when DEBUG=False, must return a 404 status code, otherwise APPEND_SLASH won't work correctly. I don't think this is an unreasonable requirement, but it needs to be mentioned in the 1.9 release notes as a backwards-incompatible change (unless someone thinks this needs to go through a deprecation path and thus we should revert the original patch on 1.9). We could add logging as I mentioned in comment 19 to help developers find this mistake in their handler.

I think a warning and comment about this in the release notes/documentation is good enough personally. It's not unreasonable to expect the status_code be set manually when implementing a custom handler.

[Tim Graham]

​PR to add the requirement to the 1.9 release notes and docs.

[Tim Graham <timograham@…>]

In 49eeb0f5:

Fixed #25878 -- Documented requirement that handler404 return a 404 response.

[Tim Graham <timograham@…>]

In 35fd2a6f:

[1.9.x] Fixed #25878 -- Documented requirement that handler404 return a 404 response.

Backport of 49eeb0f570c91af5064d9e4ac8649e9afa0236ec from master

[James Beith]

Does this affect the django.views.csrf.csrf_failure view as well? As in, if the user sets their own view using the CSRF_FAILURE_VIEW setting then that view must return a HttpResponseForbidden? If so then it might be worth adding that to the ​csrf-failure-view settings docs similar to what was done in this ​PR.

[Tim Graham <timograham@…>]

In 62e83c7:

Refs #25878 -- Added the expected return type of CSRF_FAILURE_VIEW.

[Tim Graham <timograham@…>]

In c74b1b4:

[1.9.x] Refs #25878 -- Added the expected return type of CSRF_FAILURE_VIEW.

Backport of 62e83c71d2086b91d58c313e46933ef7aa8b6db1 from master