[Django] #25099: Cleanup HttpRequest representations
Django
--------------------------------------+--------------------
Reporter: vzima | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Core (Other) | Version: 1.8
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+--------------------
Since simplification of `HttpRequest.__repr__` in #12098. The method
`build_request_repr` is not used anywhere in the `django.http` where is
defined and is only used for representation of request in error emails.
But even that usage is excesive. In HTML version, the request details are
printed under the traceback and so it is useless to repeat the full
request details in local variables of every frame. The text version of the
email is generated separately, despite the fact that `ExceptionReporter`
provides `get_traceback_text` method which returns text content descibing
the error.
Ticket created on recommendation by timgraham from discussion in
https://github.com/django/django/pull/4947 for related ticket #22990.
--
Ticket URL: <https://code.djangoproject.com/ticket/25099>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Reporter: vzima | Owner: vzima
Type: | Status: assigned
Cleanup/optimization |
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: 0 | UI/UX: 0
Changes (by vzima):
* owner: nobody => vzima
* needs_docs: => 0
* status: new => assigned
* needs_tests: => 0
* needs_better_patch: => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/25099#comment:1>
Django
--------------------------------------+------------------------------------
Reporter: vzima | Owner: vzima
Type: Cleanup/optimization | Status: assigned
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
Changes (by timgraham):
* has_patch: 0 => 1
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/25099#comment:2>
Django
-------------------------------------+-------------------------------------
Reporter: vzima | Owner: vzima
Type: | Status: assigned
Cleanup/optimization |
Component: Core (Other) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* version: 1.8 => master
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/25099#comment:3>
Django
-------------------------------------+-------------------------------------
Reporter: vzima | Owner: vzima
Cleanup/optimization |
Component: Core (Other) | Version: master
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"8f8c54f70bfa3aa8e311514297f1eeded2c32593" 8f8c54f]:
{{{
#!CommitTicketReference repository=""
revision="8f8c54f70bfa3aa8e311514297f1eeded2c32593"
Fixed #25099 -- Cleaned up HttpRequest representations in error reporting.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25099#comment:4>
Django
Reporter: vzima | Owner: vzima
Component: Core (Other) | Version: master
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* status: closed => new
* has_patch: 1 => 0
* resolution: fixed =>
* severity: Normal => Release blocker
* stage: Ready for checkin => Accepted
Comment:
Unfortunately, part of the changes here cause the `AdminEmailHandler` to
crash on requests with invalid HTTP hosts (regression test attached).
vzima, do you have time to take a look?
--
Ticket URL: <https://code.djangoproject.com/ticket/25099#comment:5>
Django
--------------------------------------+------------------------------------
Reporter: vzima | Owner: vzima
Component: Core (Other) | Version: master
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* Attachment "25099-regress-test.diff" added.
Django
--------------------------------------+------------------------------------
Reporter: vzima | Owner: vzima
Component: Core (Other) | Version: master
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by vzima):
* status: new => assigned
Comment:
I'll try to look at it.
--
Ticket URL: <https://code.djangoproject.com/ticket/25099#comment:6>
Django
--------------------------------------+------------------------------------
Reporter: vzima | Owner: vzima
Type: Cleanup/optimization | Status: assigned
Component: Core (Other) | Version: master
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by vzima):
This is an interesting error. The problem is caused by the fact that
allowed hosts are checked inside `HttpRequest.get_host()` which makes this
function completely unusable in case of disallowed host. Among others it
is used in `HttpRequest.get_absolute_url()` which is used to provide full
URL in exception reports. The problem was hidden before, because exception
reports are handled after the validation of the HTTP host, so the error
was not triggered.
Quick and dirty solution would most likely be addition of extra argument
to the `HttpRequest.get_host()` method to suppress validation against the
ALLOWED_HOSTS.
But I'm puzzled by the fact that allowed hosts are validated in
`HttpRequest.get_host()` method and not explicitely. I even have a
suspicion that a request with disallowed host can be handled in cases
where middlewares which uses `get_host()` method, such as
`CommonMiddleware`, are disabled.
--
Ticket URL: <https://code.djangoproject.com/ticket/25099#comment:7>
Django
--------------------------------------+------------------------------------
Reporter: vzima | Owner: vzima
Type: Cleanup/optimization | Status: assigned
Component: Core (Other) | Version: master
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by carljm):
Allowed hosts are validated in `get_host()` specifically in order to
enable the latter possibility: we didn't want to enforce use of
`ALLOWED_HOSTS` on a site which never made use of the `Host` header at
all. If the `Host` header is not in fact used, then a spoofed `Host`
header has no effect, and there is no concern.
Rather than adding a new argument to `get_host()`, I'd prefer to introduce
a new private `_get_raw_host()` or similar, which could be called within
`get_host()` too. I guess that's needed (as opposed to just getting the
raw Host direct from `request.META` in order to handle `X-Forwarded-Host`
et al.
--
Ticket URL: <https://code.djangoproject.com/ticket/25099#comment:8>
Django
--------------------------------------+------------------------------------
Reporter: vzima | Owner: vzima
Type: Cleanup/optimization | Status: assigned
Component: Core (Other) | Version: master
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by vzima):
Patch is in PR https://github.com/django/django/pull/5228
I find out the bug is not a regression. The problem was there before
(tested in 1.8.4) but it manifested only if `include_html` was enabled.
I based the fix on carljm's proposal, but apart from `_get_raw_host()` I
had to add also `get_raw_uri()` method to provide alternative for
`build_absolute_uri()` which was used in the template.
--
Ticket URL: <https://code.djangoproject.com/ticket/25099#comment:9>
Django
--------------------------------------+------------------------------------
Reporter: vzima | Owner: vzima
Type: Cleanup/optimization | Status: assigned
Component: Core (Other) | Version: master
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by vzima):
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/25099#comment:10>
Django
Reporter: vzima | Owner: vzima
Type: | Status: assigned
Cleanup/optimization |
Component: Core (Other) | Version: master
Severity: Release blocker | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/25099#comment:11>
Django
-------------------------------------+-------------------------------------
Reporter: vzima | Owner: vzima
Cleanup/optimization |
Component: Core (Other) | Version: master
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"cf29b6b5610f242d18dcc31eb897c873109b67e2" cf29b6b5]:
{{{
#!CommitTicketReference repository=""
revision="cf29b6b5610f242d18dcc31eb897c873109b67e2"
Fixed #25099 -- Fixed crash in AdminEmailHandler on DisallowedHost.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25099#comment:12>
Django
-------------------------------------+-------------------------------------
Reporter: vzima | Owner: vzima
Type: | Status: closed
Cleanup/optimization |
Component: Core (Other) | Version: master
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"071af825398bab08246aa28c227514ed37cf4244" 071af825]:
{{{
#!CommitTicketReference repository=""
revision="071af825398bab08246aa28c227514ed37cf4244"
Refs #25099 -- Added removal of build_request_repr() to 1.9 release notes.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25099#comment:13>
Django
-------------------------------------+-------------------------------------
Reporter: vzima | Owner: vzima
Type: | Status: closed
Cleanup/optimization |
Component: Core (Other) | Version: master
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"428164fc81a8bf619d6a4fd92d3350c174e311a0" 428164fc]:
{{{
#!CommitTicketReference repository=""
revision="428164fc81a8bf619d6a4fd92d3350c174e311a0"
[1.9.x] Refs #25099 -- Added removal of build_request_repr() to 1.9
release notes.
Backport of 071af825398bab08246aa28c227514ed37cf4244 from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25099#comment:14>