Opened 9 years ago
Closed 9 years ago
#25971 closed Bug (fixed)
BrokenLinkEmailsMiddleware shouldn't report 404s when URL = Referer + "/"
Reported by: | Jon Ribbens | Owned by: | HARI KRISHNA KANCHI |
---|---|---|---|
Component: | Core (Other) | Version: | 1.8 |
Severity: | Normal | Keywords: | |
Cc: | Triage Stage: | Accepted | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
CommonMiddleware with APPEND_SLASH redirects "/foo" to "/foo/".
BrokenLinkEmailsMiddleware goes "omg /foo/ doesn't exist and it was referred to by /foo, that's an INTERNAL BROKEN LINK!"
This means the "don't alert about links with no referer" check basically doesn't work at all.
If APPEND_SLASH is set and the referer is the URL with a trailing '/' removed then BrokenLinkEmailsMiddleware shouldn't send an email.
Change History (23)
comment:1 by , 9 years ago
comment:2 by , 9 years ago
I believe you can still hit this case if the URL resolves, but the view raises Http404 -- for example because it uses get_object_or_404
.
comment:3 by , 9 years ago
Indeed. Consider for example if you are using django-cms, where almost any URL will end up matching the view that then goes on to see if a page has been published with that name - so urlresolvers will basically always say "yes that URL might exist".
comment:4 by , 9 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:5 by , 9 years ago
Hello and Happy New Year!
I've made an attempt at creating a failing test against 1.8.x as a first step - https://github.com/django/django/compare/stable/1.8.x...matason:ticket_25971?expand=1
Does it look like it captures the bug correctly?
comment:6 by , 9 years ago
As far as my limited understanding of the test framework goes, it looks probably correct to me ;-)
comment:7 by , 9 years ago
Thanks jribbens, I'll see if I can come up with a pull request that fixes the issue!
comment:8 by , 9 years ago
Unless this is a regression in Django 1.8, please send the pull request against master. The ticket "Version" only indicates what version the issue was reported in.
comment:10 by , 9 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:11 by , 9 years ago
Sent a pull request, along with the patch. https://github.com/django/django/pull/6008. Can anybody check this pull request? And let me know if there are any improvements.
comment:12 by , 9 years ago
Has patch: | set |
---|
comment:13 by , 9 years ago
Patch needs improvement: | set |
---|
The test doesn't seem to act as a regression test as it passes without the fix.
comment:15 by , 9 years ago
Patch needs improvement: | unset |
---|
comment:16 by , 9 years ago
I'm not sure if the added logic is correct. Should it consider settings.APPEND_SLASH
as mentioned in the original report?
comment:17 by , 9 years ago
Yah, it should consider settings.APPEND_SLASH
. I will change the code accordingly.
comment:18 by , 9 years ago
Easy pickings: | unset |
---|---|
Patch needs improvement: | set |
comment:19 by , 9 years ago
If settings.APPEND_SLASH
is unset and referrer url equal to requested url without trailing slash, then it should send mail right?
comment:22 by , 9 years ago
Patch needs improvement: | unset |
---|
APPEND_SLASH should redirect if only /foo/ is valid a view. (check CommonMiddleware.should_redirect_with_slash)
Can you provide a test case?