#24834 closed Bug (fixed)
Optional port in Host request header breaks get_current_site()
Reported by: | Nick Pope | Owned by: | Nick Pope |
---|---|---|---|
Component: | contrib.sites | Version: | 1.5 |
Severity: | Normal | Keywords: | |
Cc: | Triage Stage: | Ready for checkin | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
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()
.
Change History (14)
comment:1 by , 10 years ago
Has patch: | set |
---|---|
Owner: | changed from | to
Status: | new → assigned |
comment:2 by , 10 years ago
comment:3 by , 10 years ago
Patch needs improvement: | set |
---|---|
Triage Stage: | Unreviewed → Accepted |
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)
comment:5 by , 10 years ago
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 isexample.com:8080
and the only port that can be connected to is8080
? - 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.
comment:6 by , 10 years ago
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.
comment:7 by , 10 years ago
Since it's not a regression in 1.8, I think it should be included in 1.9.
comment:8 by , 9 years ago
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
.
comment:10 by , 9 years ago
Patch needs improvement: | unset |
---|
comment:11 by , 9 years ago
Patch needs improvement: | set |
---|
comment:12 by , 9 years ago
Patch needs improvement: | unset |
---|---|
Triage Stage: | Accepted → Ready for checkin |
PR