[Django] #24834: Optional port in Host request header breaks get_current_site()
Django
-------------------------------+--------------------
Reporter: ngnpope | Owner: nobody
Type: Bug | Status: new
Component: contrib.sites | Version: 1.5
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------
When calling `django.contrib.sites.shortcuts.get_current_site()` and
relying on `HttpRequest.get_host()` to determine the current site, the
query can fail if the Host header contains a port. This bug has been
present since 1.5 where `RequestSite` was introduced and also turns up in
1.8 with support for using the request in `SiteManager.get_current()`.
--
Ticket URL: <https://code.djangoproject.com/ticket/24834>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: ngnpope | Owner: ngnpope
Type: Bug | Status: assigned
Component: contrib.sites | Version: 1.5
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by ngnpope):
* status: new => assigned
* needs_better_patch: => 0
* needs_tests: => 0
* owner: nobody => ngnpope
* needs_docs: => 0
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/24834#comment:1>
Django
Reporter: ngnpope | Owner: ngnpope
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by ngnpope):
[https://github.com/django/django/pull/4688 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/24834#comment:2>
Django
Reporter: ngnpope | Owner: ngnpope
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by claudep):
* needs_better_patch: 0 => 1
* stage: Unreviewed => Accepted
Comment:
I think we should account for the scenario where someone runs several
sites under different ports (example.org:8081, example.org:8082, etc.) and
also includes the port in `Site.domain`.
Possible approaches:
- try first with `domain:port` and fallback to the version without the
port
- strip the port only if it is the default port (80/443)
--
Ticket URL: <https://code.djangoproject.com/ticket/24834#comment:3>
Django
Reporter: ngnpope | Owner: ngnpope
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by loic):
@claudep my thought exactly.
--
Ticket URL: <https://code.djangoproject.com/ticket/24834#comment:4>
Django
Reporter: ngnpope | Owner: ngnpope
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by ngnpope):
Fair enough. I can see that sites hosted on different ports might be
desirable to some, although it would be a minor use case. For more
background, my situation was a customer explicitly specifying a standard
port (80/443) for a web service request and an HTTP 500 error was thrown
out from the `CurrentSiteMiddleware` due to `Site.DoesNotExist` even
though the domain part of the host header would have correctly matched a
record in the database. Note that I do not configure `SITE_ID` as I want
the site record to be chosen automatically based on the request and
`SITE_ID` takes precedence.
Making these changes would mean that `Site.domain` is a bit of a misnomer,
but I don't think we would be able to change that to `Site.host` without
backwards incompatibility issues. C'est la vie.
Some questions:
- Would it make sense to always strip if the port is 80/443 or would you
definitely prefer to only do that as a last resort?
- Would it make sense to always strip any port so that you could have
`example.com` where the host is `example.com:8080` and the only port that
can be connected to is `8080`?
- I expect that we would need to make the documentation much clearer
regarding what values can be stored in `Site.domain`?
I feel that we want to ensure that people do not needlessly create
multiple records, e.g. `example.com`, `example.com:80` and
`example.com:443`. This may lead to confusion where other models have a
foreign key to `Site` and may lead to "missing" content.
--
Ticket URL: <https://code.djangoproject.com/ticket/24834#comment:5>
Django
Reporter: ngnpope | Owner: ngnpope
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by claudep):
I'd suggest going that route:
- try first the "full" domain like now
- catch `DoesNotExist` and try with the port stripped if any
+1 to improve documentation.
--
Ticket URL: <https://code.djangoproject.com/ticket/24834#comment:6>
Django
Reporter: ngnpope | Owner: ngnpope
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by MarkusH):
Since it's not a regression in 1.8, I think it should be included in 1.9.
--
Ticket URL: <https://code.djangoproject.com/ticket/24834#comment:7>
Django
Reporter: ngnpope | Owner: ngnpope
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by ngnpope):
Hi,
Sorry for the delay in getting back to this. I have already finished the
code changes and am just finishing up the documentation now after which I
will issue a new pull request.
I will be putting this forward for 1.9 but would like it to be considered
for a backport to 1.8 as, while not a regression, the current behaviour is
unexpected. It results in a situation where, even though the
`ALLOWED_HOSTS` settings is restricted to a specific domain and that
domain is present in a `Site` record, a user can submit a `Host` header
with that domain combined with a port and trigger `Site.DoesNotExist`,
particularly when using `CurrentSiteMiddleware`.
--
Ticket URL: <https://code.djangoproject.com/ticket/24834#comment:8>
Django
Reporter: ngnpope | Owner: ngnpope
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by ngnpope):
Updated [https://github.com/django/django/pull/4853 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/24834#comment:9>
Django
Reporter: ngnpope | Owner: ngnpope
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/24834#comment:10>
Django
Reporter: ngnpope | Owner: ngnpope
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by MarkusH):
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/24834#comment:11>
Django
Reporter: ngnpope | Owner: ngnpope
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by MarkusH):
* needs_better_patch: 1 => 0
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/24834#comment:12>
Django
Reporter: ngnpope | Owner: ngnpope
Component: contrib.sites | Version: 1.5
Severity: Normal | Resolution: fixed
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:"b3d5dc6932bf896a909e9871d508654494b34563" b3d5dc69]:
{{{
#!CommitTicketReference repository=""
revision="b3d5dc6932bf896a909e9871d508654494b34563"
Fixed #24834 -- Fixed get_current_site() when Host header contains port.
When the Host header contains a port, looking up the Site record fails
as the host will never match the domain.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24834#comment:13>
Django
Reporter: ngnpope | Owner: ngnpope
Type: Bug | Status: closed
Severity: Normal | 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:"0d97e73d95379722b8044fa09b1c1c4ee4c0a486" 0d97e73d]:
{{{
#!CommitTicketReference repository=""
revision="0d97e73d95379722b8044fa09b1c1c4ee4c0a486"
Fixed reverse sites_tests failures introduced in refs #24834.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24834#comment:14>