Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#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


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 Changed 8 years ago by Nick Pope

Has patch: set
Owner: changed from nobody to Nick Pope
Status: newassigned

comment:2 Changed 8 years ago by Nick Pope

comment:3 Changed 8 years ago by Claude Paroz

Patch needs improvement: set
Triage Stage: UnreviewedAccepted

I think we should account for the scenario where someone runs several sites under different ports (,, 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:4 Changed 8 years ago by Loic Bistuer

@claudep my thought exactly.

comment:5 Changed 8 years ago by 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 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 where the host is 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., and 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.)

Last edited 8 years ago by Nick Pope (previous) (diff)

comment:6 Changed 8 years ago by 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.

comment:7 Changed 8 years ago by Markus Holtermann

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

comment:8 Changed 8 years ago by Nick Pope


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:9 Changed 8 years ago by Nick Pope

Updated PR

comment:10 Changed 8 years ago by Tim Graham

Patch needs improvement: unset

comment:11 Changed 8 years ago by Markus Holtermann

Patch needs improvement: set

comment:12 Changed 8 years ago by Markus Holtermann

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

comment:13 Changed 8 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: assignedclosed

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.

comment:14 Changed 8 years ago by Tim Graham <timograham@…>

In 0d97e73d:

Fixed reverse sites_tests failures introduced in refs #24834.

Note: See TracTickets for help on using tickets.
Back to Top