[Django] #25971: BrokenLinkEmailsMiddleware shouldn't report 404s when URL = Referer + "/"
Django
------------------------------+--------------------
Reporter: jribbens | Owner: nobody
Type: Bug | Status: new
Component: Core (Other) | Version: 1.8
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 1 | UI/UX: 0
------------------------------+--------------------
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/25971>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: jribbens | Owner: nobody
Type: Bug | Status: new
Component: Core (Other) | Version: 1.8
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by emre):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
APPEND_SLASH should redirect if only /foo/ is valid a view. (check
CommonMiddleware.should_redirect_with_slash)
Can you provide a test case?
--
Ticket URL: <https://code.djangoproject.com/ticket/25971#comment:1>
Django
Reporter: jribbens | Owner: nobody
Type: Bug | Status: new
Component: Core (Other) | Version: 1.8
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Comment (by aaugustin):
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`.
--
Ticket URL: <https://code.djangoproject.com/ticket/25971#comment:2>
Django
Reporter: jribbens | Owner: nobody
Type: Bug | Status: new
Component: Core (Other) | Version: 1.8
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Comment (by jribbens):
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".
--
Ticket URL: <https://code.djangoproject.com/ticket/25971#comment:3>
Django
Reporter: jribbens | Owner: nobody
Type: Bug | Status: new
Component: Core (Other) | Version: 1.8
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Changes (by timgraham):
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/25971#comment:4>
Django
Reporter: jribbens | Owner: nobody
Type: Bug | Status: new
Component: Core (Other) | Version: 1.8
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Comment (by matason):
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?
--
Ticket URL: <https://code.djangoproject.com/ticket/25971#comment:5>
Django
Reporter: jribbens | Owner: nobody
Type: Bug | Status: new
Component: Core (Other) | Version: 1.8
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Comment (by jribbens):
As far as my limited understanding of the test framework goes, it looks
probably correct to me ;-)
--
Ticket URL: <https://code.djangoproject.com/ticket/25971#comment:6>
Django
Reporter: jribbens | Owner: nobody
Type: Bug | Status: new
Component: Core (Other) | Version: 1.8
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Comment (by matason):
Thanks jribbens, I'll see if I can come up with a pull request that fixes
the issue!
--
Ticket URL: <https://code.djangoproject.com/ticket/25971#comment:7>
Django
Reporter: jribbens | Owner: nobody
Type: Bug | Status: new
Component: Core (Other) | Version: 1.8
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Comment (by timgraham):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/25971#comment:8>
Django
Reporter: jribbens | Owner: nobody
Type: Bug | Status: new
Component: Core (Other) | Version: 1.8
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Comment (by matason):
Thanks timgraham, noted.
--
Ticket URL: <https://code.djangoproject.com/ticket/25971#comment:9>
Django
Reporter: jribbens | Owner:
| harikrishnakanchi
Type: Bug | Status: assigned
Component: Core (Other) | Version: 1.8
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Changes (by harikrishnakanchi):
* status: new => assigned
* owner: nobody => harikrishnakanchi
--
Ticket URL: <https://code.djangoproject.com/ticket/25971#comment:10>
Django
Reporter: jribbens | Owner:
| harikrishnakanchi
Type: Bug | Status: assigned
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Comment (by harikrishnakanchi):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/25971#comment:11>
Django
Reporter: jribbens | Owner:
| harikrishnakanchi
Type: Bug | Status: assigned
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by charettes):
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/25971#comment:12>
Django
Reporter: jribbens | Owner:
| harikrishnakanchi
Type: Bug | Status: assigned
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 0 => 1
Comment:
The test doesn't seem to act as a regression test as it passes without the
fix.
--
Ticket URL: <https://code.djangoproject.com/ticket/25971#comment:13>
Django
Reporter: jribbens | Owner:
| harikrishnakanchi
Type: Bug | Status: assigned
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by harikrishnakanchi):
Fixed the test case now.
--
Ticket URL: <https://code.djangoproject.com/ticket/25971#comment:14>
Django
Reporter: jribbens | Owner:
| harikrishnakanchi
Type: Bug | Status: assigned
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/25971#comment:15>
Django
Reporter: jribbens | Owner:
| harikrishnakanchi
Type: Bug | Status: assigned
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Comment (by timgraham):
I'm not sure if the added logic is correct. Should it consider
`settings.APPEND_SLASH` as mentioned in the original report?
--
Ticket URL: <https://code.djangoproject.com/ticket/25971#comment:16>
Django
Reporter: jribbens | Owner:
| harikrishnakanchi
Type: Bug | Status: assigned
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Comment (by harikrishnakanchi):
Yah, it should consider `settings.APPEND_SLASH`. I will change the code
accordingly.
--
Ticket URL: <https://code.djangoproject.com/ticket/25971#comment:17>
Django
Reporter: jribbens | Owner:
| harikrishnakanchi
Type: Bug | Status: assigned
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by timgraham):
* needs_better_patch: 0 => 1
* easy: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/25971#comment:18>
Django
Reporter: jribbens | Owner:
| harikrishnakanchi
Type: Bug | Status: assigned
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by harikrishnakanchi):
If `settings.APPEND_SLASH` is unset and referrer url equal to requested
url without trailing slash, then it should send mail right?
--
Ticket URL: <https://code.djangoproject.com/ticket/25971#comment:19>
Django
Reporter: jribbens | Owner:
| harikrishnakanchi
Type: Bug | Status: assigned
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by timgraham):
Sounds correct to me.
--
Ticket URL: <https://code.djangoproject.com/ticket/25971#comment:20>
Django
Reporter: jribbens | Owner:
| harikrishnakanchi
Type: Bug | Status: assigned
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by harikrishnakanchi):
Hey TIm, Changed the code accordingly. Can you look into once. Thanks.
--
Ticket URL: <https://code.djangoproject.com/ticket/25971#comment:21>
Django
Reporter: jribbens | Owner:
| harikrishnakanchi
Type: Bug | Status: assigned
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/25971#comment:22>
Django
Reporter: jribbens | Owner:
| harikrishnakanchi
Component: Core (Other) | Version: 1.8
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"74670498e902a0506e667cd21084c5e2eb71edfa" 7467049]:
{{{
#!CommitTicketReference repository=""
revision="74670498e902a0506e667cd21084c5e2eb71edfa"
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.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25971#comment:23>