Optional port in Host request header breaks get_current_site()

[Nick Pope]

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().

[Nick Pope]

​PR

[Claude Paroz]

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)

[Loic Bistuer]

@claudep my thought exactly.

[Nick Pope]

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.

(Edit: I've just realised that the current implementation would have always supported domain:port so as my current version of the PR would break backwards compatibility, although I'm not sure anyone necessarily expected domain:port to be work.)

[Claude Paroz]

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.

[Markus Holtermann]

Since it's not a regression in 1.8, I think it should be included in 1.9.

[Nick Pope]

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.

[Nick Pope]

Updated ​PR

[Tim Graham <timograham@…>]

In b3d5dc69:

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.

[Tim Graham <timograham@…>]

In 0d97e73d:

Fixed reverse sites_tests failures introduced in refs #24834.