Opened 6 years ago

Closed 6 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 Changed 6 years ago by Emre Yılmaz

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 Changed 6 years ago by Aymeric Augustin

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 Changed 6 years ago by Jon Ribbens

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 Changed 6 years ago by Tim Graham

Triage Stage: UnreviewedAccepted

comment:5 Changed 6 years ago by Chris Maiden

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 Changed 6 years ago by Jon Ribbens

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

comment:7 Changed 6 years ago by Chris Maiden

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

comment:8 Changed 6 years ago by Tim Graham

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 Changed 6 years ago by Chris Maiden

Thanks timgraham, noted.

comment:10 Changed 6 years ago by HARI KRISHNA KANCHI

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

comment:11 Changed 6 years ago by HARI KRISHNA KANCHI

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 Changed 6 years ago by Simon Charette

Has patch: set

comment:13 Changed 6 years ago by Tim Graham

Patch needs improvement: set

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

comment:14 Changed 6 years ago by HARI KRISHNA KANCHI

Fixed the test case now.

comment:15 Changed 6 years ago by Tim Graham

Patch needs improvement: unset

comment:16 Changed 6 years ago by Tim Graham

I'm not sure if the added logic is correct. Should it consider settings.APPEND_SLASH as mentioned in the original report?

comment:17 Changed 6 years ago by HARI KRISHNA KANCHI

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

comment:18 Changed 6 years ago by Tim Graham

Easy pickings: unset
Patch needs improvement: set

comment:19 Changed 6 years ago by HARI KRISHNA KANCHI

If settings.APPEND_SLASH is unset and referrer url equal to requested url without trailing slash, then it should send mail right?

comment:20 Changed 6 years ago by Tim Graham

Sounds correct to me.

comment:21 Changed 6 years ago by HARI KRISHNA KANCHI

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

comment:22 Changed 6 years ago by HARI KRISHNA KANCHI

Patch needs improvement: unset

comment:23 Changed 6 years ago by Tim Graham <timograham@…>

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