Opened 8 years ago

Closed 8 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 Emre Yılmaz, 8 years ago

APPEND_SLASH should redirect if only /foo/ is valid a view. (check CommonMiddleware.should_redirect_with_slash)

Can you provide a test case?

comment:2 by Aymeric Augustin, 8 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 Jon Ribbens, 8 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 Tim Graham, 8 years ago

Triage Stage: UnreviewedAccepted

comment:5 by Chris Maiden, 8 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 Jon Ribbens, 8 years ago

As far as my limited understanding of the test framework goes, it looks probably correct to me ;-)

comment:7 by Chris Maiden, 8 years ago

Thanks jribbens, I'll see if I can come up with a pull request that fixes the issue!

comment:8 by Tim Graham, 8 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:9 by Chris Maiden, 8 years ago

Thanks timgraham, noted.

comment:10 by HARI KRISHNA KANCHI, 8 years ago

Owner: changed from nobody to HARI KRISHNA KANCHI
Status: newassigned

comment:11 by HARI KRISHNA KANCHI, 8 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 Simon Charette, 8 years ago

Has patch: set

comment:13 by Tim Graham, 8 years ago

Patch needs improvement: set

The test doesn't seem to act as a regression test as it passes without the fix.

comment:14 by HARI KRISHNA KANCHI, 8 years ago

Fixed the test case now.

comment:15 by Tim Graham, 8 years ago

Patch needs improvement: unset

comment:16 by Tim Graham, 8 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 HARI KRISHNA KANCHI, 8 years ago

Yah, it should consider settings.APPEND_SLASH. I will change the code accordingly.

comment:18 by Tim Graham, 8 years ago

Easy pickings: unset
Patch needs improvement: set

comment:19 by HARI KRISHNA KANCHI, 8 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:20 by Tim Graham, 8 years ago

Sounds correct to me.

comment:21 by HARI KRISHNA KANCHI, 8 years ago

Hey TIm, Changed the code accordingly. Can you look into once. Thanks.

comment:22 by HARI KRISHNA KANCHI, 8 years ago

Patch needs improvement: unset

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

Resolution: fixed
Status: assignedclosed

In 7467049:

Fixed #25971 -- Made BrokenLinkEmailsMiddleware ignore APPEND_SLASH redirects.

If APPEND_SLASH=True and the referer is the URL without a trailing '/', then
BrokenLinkEmailsMiddleware shouldn't send an email.

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