Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#26293 closed Bug (fixed)

Warnings regarding 404s logged for URLs missing trailing slashes

Reported by: JK Laiho Owned by: Sam
Component: HTTP handling Version: 1.9
Severity: Normal Keywords: CommonMiddleware 404 logging regression
Cc: Emett Speer Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description (last modified by Emett Speer)

I recently deployed a project into production using Django 1.9.2 and started getting strange logged warning messages from Sentry for 404's. Looking into it, this occurred in django.core.handers.base.BaseHandler.get_response and was related to people visiting URLs without trailing slashes.

I compared the behaviour against an earlier, similarly configured project still running Django 1.7.x and this didn't occur there.

Digging deeper, it seems that commit 434d309e to fix 24720 inside CommonMiddleware was causing this. In lines 56-66 (58-68 in 1.9.2), the path is only checked for a missing slash if the prerequisites for PREPEND_WWW processing are met, since they are indented beneath it. This doesn't really make sense, since the two settings are not interdependent.

As a result, an Http404 is raised after request middleware processing in BaseHandler.get_response, at which point a warning is logged—for every single request for a path without a trailing slash.

APPEND_SLASH still takes effect eventually, but only when CommonMiddleware is called again, this time for process_response, whereupon the normal redirect gets done. Thanks to this, everything appears to function normally for the end user, but unnecessary 404 warnings end up getting logged. (Though if APPEND_SLASH was False, you'd probably want them logged.)

It seems to me that CommonMiddleware.process_request needs a bit of reworking to run the checks for PREPEND_WWW and APPEND_SLASH independently, and to determine the need for a redirect based on whether at least one of these is necessary, still fulfilling the purpose of 24720. I've provisionally marked the ticket as "easy pickings", as it seems to be that way from my admittedly limited research into this.

Change History (13)

comment:1 Changed 5 years ago by JK Laiho

A further note: I'm naturally using URL reversing judiciously in all of my templates so that the URLs that people actually click on have trailing slashes already in place, but for some reason some people still type some URLs by hand, leaving out the trailing slashes.

comment:2 Changed 5 years ago by Emett Speer

Description: modified (diff)
Owner: changed from nobody to Emett Speer
Status: newassigned

comment:3 Changed 5 years ago by Emett Speer

Cc: Emett Speer added
Description: modified (diff)

comment:4 Changed 5 years ago by Emett Speer

Has patch: set
Needs tests: set

comment:6 Changed 5 years ago by JK Laiho

I haven't tested your changes, but by my reading of the code, it looks incorrect. After your unindentation, process_request now always returns a HttpResponsePermanentRedirect, as long as the forbidden UA check at the top passes. It should only do that when either the slash has been appended or "www" has been prepended.

comment:7 Changed 5 years ago by Tim Graham

Triage Stage: UnreviewedAccepted

comment:8 in reply to:  6 Changed 5 years ago by Emett Speer

You are correct. My patch just breaks more things. My guess for the logging is because it first has to 404 before it tries APPEND_SLASH. I will have to look into this much further then I thought I would.

Replying to jklaiho:

I haven't tested your changes, but by my reading of the code, it looks incorrect. After your unindentation, process_request now always returns a HttpResponsePermanentRedirect, as long as the forbidden UA check at the top passes. It should only do that when either the slash has been appended or "www" has been prepended.

comment:9 Changed 5 years ago by Emett Speer

Owner: Emett Speer deleted
Status: assignednew

comment:10 Changed 5 years ago by Sam

Owner: set to Sam
Status: newassigned

comment:11 Changed 5 years ago by Sam

Needs tests: unset

PR Submitted including regression test. Tests pass and docs build without issue.

patch: https://github.com/ieatkittens/django/tree/ticket_26293

comment:12 Changed 5 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: assignedclosed

In 9390da7:

Fixed #26293 -- Fixed CommonMiddleware to process PREPEND_WWW and APPEND_SLASH independently.

comment:13 Changed 5 years ago by Tim Graham <timograham@…>

In ccc367fd:

[1.9.x] Fixed #26293 -- Fixed CommonMiddleware to process PREPEND_WWW and APPEND_SLASH independently.

Backport of 9390da7fb6e251eaa9a785692f987296cb14523f from master

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